Date:	Tue, 5 Sep 2006 14:05:14 +0200 (MEST)
From:	Jan Engelhardt <jengelh@...ux01.gwdg.de>
To:	Steven Whitehouse <swhiteho@...hat.com>
cc:	linux-kernel@...r.kernel.org,
	Russell Cattelan <cattelan@...hat.com>,
	David Teigland <teigland@...hat.com>,
	Ingo Molnar <mingo@...e.hu>, hch@...radead.org
Subject: Re: [PATCH 14/16] GFS2: The DLM interface module


>+/* make_strname - convert GFS lock numbers to a string */
>+
>+static inline void make_strname(struct lm_lockname *lockname,
>+				struct gdlm_strname *str)
>+{
>+	sprintf(str->name, "%8x%16llx", lockname->ln_type,
>+		(unsigned long long)lockname->ln_number);

Is this format specifier safe enough? "%08x%016llx" perhaps?

Imagine (if this happens at all):

  ln_type = 1; ln_number = 16;
  %8x = "1", "%16llx" = "10", giving us "110"

  ln_type = 17; ln_number = 0;
  %8x = "11", "%16llx" = "0", giving us "110".

Whoops, name clash.


>+int gdlm_get_lock(lm_lockspace_t *lockspace, struct lm_lockname *name,
>+		  lm_lock_t **lockp)
>+{
>+	struct gdlm_lock *lp;
>+	int error;
>+
>+	error = gdlm_create_lp((struct gdlm_ls *) lockspace, name, &lp);
>+
>+	*lockp = (lm_lock_t *) lp;

This cast is alright in itself. Considering however that lm_lock_t is
currently typedef'ed to void, it looks a little different. (One _could_ get rid
of it, but better not while it is called lm_lock_t. Leave as-is for now.)

>+static wait_queue_head_t send_wq;
>+static wait_queue_head_t recv_wq;
>+
>+struct plock_op {
>+	struct list_head list;
>+	int done;
>+	struct gdlm_plock_info info;
>+};
>+
>+static inline void set_version(struct gdlm_plock_info *info)
>+{
>+	info->version[0] = GDLM_PLOCK_VERSION_MAJOR;
>+	info->version[1] = GDLM_PLOCK_VERSION_MINOR;
>+	info->version[2] = GDLM_PLOCK_VERSION_PATCH;
>+}
>+
>+static int check_version(struct gdlm_plock_info *info)
>+{
>+	if ((GDLM_PLOCK_VERSION_MAJOR != info->version[0]) ||
>+	    (GDLM_PLOCK_VERSION_MINOR < info->version[1])) {
>+		log_error("plock device version mismatch: "
>+			  "kernel (%u.%u.%u), user (%u.%u.%u)",
>+			  GDLM_PLOCK_VERSION_MAJOR,
>+			  GDLM_PLOCK_VERSION_MINOR,
>+			  GDLM_PLOCK_VERSION_PATCH,
>+			  info->version[0],
>+			  info->version[1],
>+			  info->version[2]);
>+		return -EINVAL;
>+	}
>+	return 0;
>+}
>+
>+static void send_op(struct plock_op *op)
>+{
>+	set_version(&op->info);
>+	INIT_LIST_HEAD(&op->list);
>+	spin_lock(&ops_lock);
>+	list_add_tail(&op->list, &send_list);
>+	spin_unlock(&ops_lock);
>+	wake_up(&send_wq);
>+}
>+
>+int gdlm_plock(lm_lockspace_t *lockspace, struct lm_lockname *name,
>+	       struct file *file, int cmd, struct file_lock *fl)
>+{
>+	struct gdlm_ls *ls = (struct gdlm_ls *) lockspace;
>+	struct plock_op *op;
>+	int rv;
>+
>+	op = kzalloc(sizeof(*op), GFP_KERNEL);
>+	if (!op)
>+		return -ENOMEM;
>+
>+	op->info.optype		= GDLM_PLOCK_OP_LOCK;
>+	op->info.pid		= fl->fl_pid;
>+	op->info.ex		= (fl->fl_type == F_WRLCK);
>+	op->info.wait		= IS_SETLKW(cmd);
>+	op->info.fsid		= ls->id;
>+	op->info.number		= name->ln_number;
>+	op->info.start		= fl->fl_start;
>+	op->info.end		= fl->fl_end;
>+	op->info.owner		= (__u64)(long) fl->fl_owner;

Can't op->info.owner be a 'struct fowner *'? Is op->info.owner shared over the
network?

>+static ssize_t block_show(struct gdlm_ls *ls, char *buf)
>+{
>+	ssize_t ret;
>+	int val = 0;
>+
>+	if (test_bit(DFL_BLOCK_LOCKS, &ls->flags))
>+		val = 1;
>+	ret = sprintf(buf, "%d\n", val);

Safe enough - @buf big enough?

>+	if (val == 1)
>+		set_bit(DFL_BLOCK_LOCKS, &ls->flags);
>+	else if (val == 0) {
>+		clear_bit(DFL_BLOCK_LOCKS, &ls->flags);
>+		gdlm_submit_delayed(ls);
>+	} else
>+		ret = -EINVAL;

Ingo surely wants you to {} it.

>+static ssize_t id_show(struct gdlm_ls *ls, char *buf)
>+{
>+	return sprintf(buf, "%u\n", ls->id);
>+}
>+
>+static ssize_t jid_show(struct gdlm_ls *ls, char *buf)
>+{
>+	return sprintf(buf, "%d\n", ls->jid);
>+}
>+
>+static ssize_t first_show(struct gdlm_ls *ls, char *buf)
>+{
>+	return sprintf(buf, "%d\n", ls->first);
>+}
>+
>+static ssize_t first_done_show(struct gdlm_ls *ls, char *buf)
>+{
>+	return sprintf(buf, "%d\n", ls->first_done);
>+}
>+
>+static ssize_t recover_show(struct gdlm_ls *ls, char *buf)
>+{
>+	return sprintf(buf, "%d\n", ls->recover_jid);
>+}

Big enough?

>+static ssize_t recover_done_show(struct gdlm_ls *ls, char *buf)
>+{
>+	return sprintf(buf, "%d\n", ls->recover_jid_done);
>+}
>+
>+static ssize_t recover_status_show(struct gdlm_ls *ls, char *buf)
>+{
>+	return sprintf(buf, "%d\n", ls->recover_jid_status);
>+}





Jan Engelhardt
-- 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists