[Iscsitarget-devel] [PATCH1/2] hierarchical parameters treatment and scsi_id unified

[Luiz A. D. de L. <lui...@gm...>]

- Param parse loop moved to volume.c. Now each iotype just process
individual parameters.
- Unknown parameters for iotype will get processed by volume.
- All param loops uses the same code now
- Default iotype definition moved to include file
- Moved set_scsi_id and set_scsi_sn to volume.c

I sent this patch before but I'll send again as I finished patch 2.

-- 
 Luiz Angelo Daros de Luca
 lui...@gm...
 ICQ: 19290419

 I Know, "Where you wanted to go today",
 but I decided to stop here instead!
 MS Windows

diff -r -u iscsitarget-trunk/kernel/block-io.c iscsitarget/kernel/block-io.c
--- iscsitarget-trunk/kernel/block-io.c	2008-03-31 16:49:22.000000000 -0300
+++ iscsitarget/kernel/block-io.c	2008-04-22 17:22:06.000000000 -0300
@@ -173,22 +173,6 @@
 	return err;
 }

-static int
-set_scsiid(struct iet_volume *volume, const char *id)
-{
-	size_t len;
-
-	if ((len = strlen(id)) > SCSI_ID_LEN - VENDOR_ID_LEN) {
-		eprintk("SCSI ID too long, %zd provided, %u max\n", len,
-			SCSI_ID_LEN - VENDOR_ID_LEN);
-		return -EINVAL;
-	}
-
-	memcpy(volume->scsi_id + VENDOR_ID_LEN, id, len);
-
-	return 0;
-}
-
 static void
 gen_scsiid(struct iet_volume *volume, struct inode *inode)
 {
@@ -214,94 +198,41 @@
 	*(p + 3) = (unsigned int) inode->i_sb->s_dev;
 }

-static int
-set_scsisn(struct iet_volume *volume, const char *sn)
-{
-	size_t len;
-
-	if ((len = strlen(sn)) > SCSI_SN_LEN) {
-		eprintk("SCSI SN too long, %zd provided, %u max\n", len,
-			SCSI_SN_LEN);
-		return -EINVAL;
-	}
-
-	memcpy(volume->scsi_sn, sn, len);
-
-	return 0;
-}
-
 /* Create an enumeration of our accepted actions */
 enum
 {
-	Opt_scsiid, Opt_scsisn, Opt_path, Opt_ignore, Opt_err,
+	Opt_path, Opt_err,
 };

 /* Create a match table using our action enums and their matching options */
 static match_table_t tokens = {
-	{Opt_scsiid, "ScsiId=%s"},
-	{Opt_scsisn, "ScsiSN=%s"},
 	{Opt_path, "Path=%s"},
-	{Opt_ignore, "Type=%s"},
-	{Opt_ignore, "IOMode=%s"},
 	{Opt_err, NULL},
 };

-static int
-parse_blockio_params (struct iet_volume *volume, char *params)
+/* Process a single param */
+int blockio_do_param(struct iet_volume *volume, char *param)
 {
-	int err = 0;
-	char *p, *q;
-
-	/* Loop through parameters separated by commas, look up our
-	 * parameter in match table, return enumeration and arguments
-	 * select case based on the returned enum and run the action */
-	while ((p = strsep(&params, ",")) != NULL) {
-		substring_t args[MAX_OPT_ARGS];
-		int token;
-		if (!*p)
-			continue;
-		token = match_token(p, tokens, args);
-		switch (token) {
-		case Opt_scsiid:
-			if (!(q = match_strdup(&args[0]))) {
-				err = -ENOMEM;
-				goto out;
-			}
-			err = set_scsiid(volume, q);
-			kfree(q);
-			if (err < 0)
-				goto out;
-			break;
-		case Opt_scsisn:
-			if (!(q = match_strdup(&args[0]))) {
-				err = -ENOMEM;
-				goto out;
-			}
-			err = set_scsisn(volume, q);
-			kfree(q);
-			if (err < 0)
-				goto out;
-			break;
-		case Opt_path:
-			if (!(q = match_strdup(&args[0]))) {
-				err = -ENOMEM;
-				goto out;
-			}
-			err = blockio_open_path(volume, q);
-			kfree(q);
-			if (err < 0)
-				goto out;
+	substring_t args[MAX_OPT_ARGS];
+	int token, err;
+	char *value;
+
+	token = match_token(param, tokens, args);
+	err = 0;
+
+	switch (token) {
+	case Opt_path:
+		if (!(value = match_strdup(&args[0]))) {
+			err = -ENOMEM;
 			break;
-		case Opt_ignore:
-			break;
-		default:
-			eprintk("Bad option %s for Lun %u on Target %s \n",
-				p, volume->lun, volume->target->name);
-			return -EINVAL;
 		}
+		err = blockio_open_path(volume, value);
+		kfree(value);
+		break;
+	default:
+		err = volume_do_param(volume, param);
 	}

-  out:
 	return err;
 }

@@ -335,7 +266,7 @@

 	volume->private = bio_data;

-	if ((err = parse_blockio_params(volume, args)) < 0) {
+	if ((err = parse_volume_params(volume, args, blockio_do_param)) < 0) {
 		eprintk("Error attaching Lun %u to Target %s \n",
 			volume->lun, volume->target->name);
 		goto out;
diff -r -u iscsitarget-trunk/kernel/file-io.c iscsitarget/kernel/file-io.c
--- iscsitarget-trunk/kernel/file-io.c	2008-03-31 16:49:22.000000000 -0300
+++ iscsitarget/kernel/file-io.c	2008-04-22 17:10:49.000000000 -0300
@@ -123,21 +123,6 @@
 	return err;
 }

-static int set_scsiid(struct iet_volume *volume, const char *id)
-{
-	size_t len;
-
-	if ((len = strlen(id)) > SCSI_ID_LEN - VENDOR_ID_LEN) {
-		eprintk("SCSI ID too long, %zd provided, %u max\n", len,
-			SCSI_ID_LEN - VENDOR_ID_LEN);
-		return -EINVAL;
-	}
-
-	memcpy(volume->scsi_id + VENDOR_ID_LEN, id, len);
-
-	return 0;
-}
-
 static void gen_scsiid(struct iet_volume *volume, struct inode *inode)
 {
 	int i;
@@ -156,83 +141,38 @@
 	*(p + 3) = (unsigned int) inode->i_sb->s_dev;
 }

-static int set_scsisn(struct iet_volume *volume, const char *sn)
-{
-	size_t len;
-
-	if ((len = strlen(sn)) > SCSI_SN_LEN) {
-		eprintk("SCSI SN too long, %zd provided, %u max\n", len,
-			SCSI_SN_LEN);
-		return -EINVAL;
-	}
-	memcpy(volume->scsi_sn, sn, len);
-	return 0;
-}
-
 enum {
-	Opt_scsiid, Opt_scsisn, Opt_path, Opt_ignore, Opt_err,
+	Opt_path, Opt_err,
 };

 static match_table_t tokens = {
-	{Opt_scsiid, "ScsiId=%s"},
-	{Opt_scsisn, "ScsiSN=%s"},
 	{Opt_path, "Path=%s"},
-	{Opt_ignore, "Type=%s"},
-	{Opt_ignore, "IOMode=%s"},
 	{Opt_err, NULL},
 };

-static int parse_fileio_params(struct iet_volume *volume, char *params)
+/* Process a single param */
+int fileio_do_param(struct iet_volume *volume, char *param)
 {
-	int err = 0;
-	char *p, *q;
-
-	while ((p = strsep(&params, ",")) != NULL) {
-		substring_t args[MAX_OPT_ARGS];
-		int token;
-		if (!*p)
-			continue;
-		token = match_token(p, tokens, args);
-		switch (token) {
-		case Opt_scsiid:
-			if (!(q = match_strdup(&args[0]))) {
-				err = -ENOMEM;
-				goto out;
-			}
-			err = set_scsiid(volume, q);
-			kfree(q);
-			if (err < 0)
-				goto out;
+	substring_t args[MAX_OPT_ARGS];
+	int token, err;
+	char *value;
+
+	token = match_token(param, tokens, args);
+	err = 0;
+
+	switch (token) {
+	case Opt_path:
+		if (!(value = match_strdup(&args[0]))) {
+			err = -ENOMEM;
 			break;
-		case Opt_scsisn:
-			if (!(q = match_strdup(&args[0]))) {
-				err = -ENOMEM;
-				goto out;
-			}
-			err = set_scsisn(volume, q);
-			kfree(q);
-			if (err < 0)
-				goto out;
-			break;
-		case Opt_path:
-			if (!(q = match_strdup(&args[0]))) {
-				err = -ENOMEM;
-				goto out;
-			}
-			err = open_path(volume, q);
-			kfree(q);
-			if (err < 0)
-				goto out;
-			break;
-		case Opt_ignore:
-			break;
-		default:
-			eprintk("Unknown %s\n", p);
-			return -EINVAL;
 		}
+		err = open_path(volume, value);
+		kfree(value);
+		break;
+	default:
+		err = volume_do_param(volume, param);
 	}

-out:
 	return err;
 }

@@ -264,7 +204,7 @@

 	lu->private = p;

-	if ((err = parse_fileio_params(lu, args)) < 0) {
+	if ((err = parse_volume_params(lu, args, fileio_do_param)) < 0) {
 		eprintk("%d\n", err);
 		goto out;
 	}
diff -r -u iscsitarget-trunk/kernel/iotype.h iscsitarget/kernel/iotype.h
--- iscsitarget-trunk/kernel/iotype.h	2008-03-31 16:49:22.000000000 -0300
+++ iscsitarget/kernel/iotype.h	2008-04-22 17:10:44.000000000 -0300
@@ -8,6 +8,8 @@
 #ifndef __IOTYPE_H__
 #define __IOTYPE_H__

+#define DEFAULT_IOTYPE "fileio"
+
 struct iotype {
 	const char *name;
 	struct list_head iot_list;
diff -r -u iscsitarget-trunk/kernel/iscsi.h iscsitarget/kernel/iscsi.h
--- iscsitarget-trunk/kernel/iscsi.h	2008-03-31 16:49:22.000000000 -0300
+++ iscsitarget/kernel/iscsi.h	2008-04-22 17:09:13.000000000 -0300
@@ -338,6 +338,9 @@
 extern int volume_reserve(struct iet_volume *volume, u64 sid);
 extern int volume_release(struct iet_volume *volume, u64 sid, int force);
 extern int is_volume_reserved(struct iet_volume *volume, u64 sid);
+int parse_volume_params(struct iet_volume *volume, const char *params,
+		int (*parse_fn)(struct iet_volume *, char *));
+int volume_do_param(struct iet_volume *volume, char *param);

 /* tio.c */
 extern int tio_init(void);
diff -r -u iscsitarget-trunk/kernel/null-io.c iscsitarget/kernel/null-io.c
--- iscsitarget-trunk/kernel/null-io.c	2008-03-31 16:49:22.000000000 -0300
+++ iscsitarget/kernel/null-io.c	2008-04-22 17:12:44.000000000 -0300
@@ -29,34 +29,32 @@
 	{Opt_err, NULL},
 };

-static int parse_nullio_params(struct iet_volume *volume, char *params)
+/* Process a single param */
+int nullio_do_param(struct iet_volume *volume, char *param)
 {
-	int err = 0;
-	char *p, *q;
+	substring_t args[MAX_OPT_ARGS];
+	int token, err;
+	char *value;
 	struct nullio_data *data = volume->private;

-	while ((p = strsep(&params, ",")) != NULL) {
-		substring_t args[MAX_OPT_ARGS];
-		int token;
-		if (!*p)
-			continue;
-		token = match_token(p, tokens, args);
-		switch (token) {
-		case Opt_sectors:
-			q = match_strdup(&args[0]);
-			if (!q)
-				return -ENOMEM;
-			data->sectors = simple_strtoull(q, NULL, 10);
-			kfree(q);
-			break;
-		case Opt_ignore:
-			break;
-		default:
-			eprintk("Unknown %s\n", p);
-			return -EINVAL;
+	token = match_token(param, tokens, args);
+	err = 0;
+
+	switch (token) {
+	case Opt_ignore:
+		break;
+	case Opt_sectors:
+		if (!(value = match_strdup(&args[0]))) {
+			err = -ENOMEM;
 			break;
 		}
+		data->sectors = simple_strtoull(value, NULL, 10);
+		kfree(value);
+		break;
+	default:
+		err = volume_do_param(volume, param);
 	}
+
 	return err;
 }

@@ -84,7 +82,7 @@

 	lu->private = p;

-	if ((err = parse_nullio_params(lu, args)) < 0) {
+	if ((err = parse_volume_params(lu, args, nullio_do_param)) < 0) {
 		eprintk("%d\n", err);
 		goto out;
 	}
diff -r -u iscsitarget-trunk/kernel/volume.c iscsitarget/kernel/volume.c
--- iscsitarget-trunk/kernel/volume.c	2008-03-31 16:49:22.000000000 -0300
+++ iscsitarget/kernel/volume.c	2008-04-22 17:32:42.000000000 -0300
@@ -21,62 +21,173 @@
 	return NULL;
 }

+/* Create an enumeration of our accepted actions */
 enum {
 	Opt_type,
 	Opt_iomode,
+	Opt_ignore,
+	Opt_scsiid,
+	Opt_scsisn,
 	Opt_err,
 };

-static match_table_t tokens = {
+static match_table_t iotokens = {
 	{Opt_type, "Type=%s"},
 	{Opt_iomode, "IOMode=%s"},
 	{Opt_err, NULL},
 };

-static int set_iotype(struct iet_volume *volume, char *params)
+/* Process just iotokens params */
+static int setio_do_param(struct iet_volume *volume, char *param)
 {
-	int err = 0;
+	int token, err;
+	char *value;
 	substring_t args[MAX_OPT_ARGS];
-	char *p, *argp = NULL, *buf = (char *) get_zeroed_page(GFP_USER);
+	
+	err = 0;
+
+	token = match_token(param, iotokens, args);
+	switch (token) {
+	case Opt_type:
+		if (!(value = match_strdup(&args[0]))) {
+			err = -ENOMEM;
+			break;
+		}
+		if (volume->iotype) {
+			eprintk("Duplicated param 'Type=' for Lun %u on Target %s \n",
+				volume->lun, volume->target->name);
+			err = -EINVAL;
+		}
+		if (!(volume->iotype = get_iotype(value))) {
+			eprintk("Failed to initialize iotype %s Lun %u on Target %s\n",
+				value, volume->lun, volume->target->name);
+			err = -ENOENT;
+		}
+		kfree(value);
+		break;
+	case Opt_iomode:
+		if (!(value = match_strdup(&args[0]))) {
+			err = -ENOMEM;
+			break;
+		}
+		if (!strcmp(value, "ro"))
+			SetLUReadonly(volume);
+		else if (!strcmp(value, "wb"))
+			SetLUAsync(volume);
+		kfree(value);
+		break;
+	default:
+		break;
+	}
+
+	return err;
+}
+
+/* Create a match table using our action enums and their matching options */
+static match_table_t tokens = {
+	{Opt_ignore, "Type=%s"},
+	{Opt_ignore, "IOMode=%s"},
+	{Opt_scsiid, "ScsiId=%s"},
+	{Opt_scsisn, "ScsiSN=%s"},
+	{Opt_err, NULL},
+};
+
+/* parse params */
+int parse_volume_params (struct iet_volume *volume, const char *params,
+		int (*parse_fn)(struct iet_volume *, char *))
+{
+	int err = 0;
+	char *param;
+	char *cur;
+	char *buf = (char *) get_zeroed_page(GFP_USER);

-	if (!buf)
-		return -ENOMEM;
 	strncpy(buf, params, PAGE_CACHE_SIZE);
+	cur = buf;	

-	while ((p = strsep(&buf, ",")) != NULL) {
-		int token;
+	/* Loop through parameters separated by commas, look up our
+	 * parameter in match table, return enumeration and arguments
+	 * select case based on the returned enum and run the action */
+	while ((param = strsep(&cur, ",")) != NULL) {

-		if (!*p)
+		if (!*param)
 			continue;
-		token = match_token(p, tokens, args);
-		switch (token) {
-		case Opt_type:
-			if (!(argp = match_strdup(&args[0])))
-				err = -ENOMEM;
-			if (argp && !(volume->iotype = get_iotype(argp)))
-				err = -ENOENT;
-			kfree(argp);
-			break;
-		case Opt_iomode:
-			if (!(argp = match_strdup(&args[0])))
-				err = -ENOMEM;
-			if (argp && !strcmp(argp, "ro"))
-				SetLUReadonly(volume);
-			else if (argp && !strcmp(argp, "wb"))
-				SetLUAsync(volume);
-			kfree(argp);
-			break;
-		default:
+
+		if ((err = parse_fn(volume, param)) < 0) {
+			eprintk("Param failed! %d \n", err);
 			break;
 		}
 	}

-	if (!err && !volume->iotype && !(volume->iotype = get_iotype("fileio"))) {
-		eprintk("%s\n", "Cannot find fileio");
-		err = -EINVAL;
+	free_page((unsigned long) buf);
+
+	return err;
+}
+
+static int
+set_scsiid(struct iet_volume *volume, const char *id)
+{
+	size_t len;
+
+	if ((len = strlen(id)) > SCSI_ID_LEN - VENDOR_ID_LEN) {
+		eprintk("SCSI ID too long, %zd provided, %u max\n", len,
+			SCSI_ID_LEN - VENDOR_ID_LEN);
+		return -EINVAL;
 	}

-	free_page((unsigned long) buf);
+	memcpy(volume->scsi_id + VENDOR_ID_LEN, id, len);
+
+	return 0;
+}
+
+static int set_scsisn(struct iet_volume *volume, const char *sn)
+{
+	size_t len;
+
+	if ((len = strlen(sn)) > SCSI_SN_LEN) {
+		eprintk("SCSI SN too long, %zd provided, %u max\n", len,
+			SCSI_SN_LEN);
+		return -EINVAL;
+	}
+
+	memcpy(volume->scsi_sn, sn, len);
+
+	return 0;
+}
+
+/* Process a single param */
+int volume_do_param(struct iet_volume *volume, char *param)
+{
+	substring_t args[MAX_OPT_ARGS];
+	int token, err;
+	char *value;
+	
+	err = 0;		
+	token = match_token(param, tokens, args);
+
+	switch (token) {
+	case Opt_scsiid:
+		if (!(value = match_strdup(&args[0]))) {
+			err = -ENOMEM;
+			break;
+		}
+		err = set_scsiid(volume, value);
+		kfree(value);
+		break;
+	case Opt_scsisn:
+		if (!(value = match_strdup(&args[0]))) {
+			err = -ENOMEM;
+			break;
+		}
+		err = set_scsisn(volume, value);
+		kfree(value);
+		break;
+	case Opt_ignore:
+		break;
+	default:
+		eprintk("Bad option %s for Lun %u on Target %s \n",
+			param, volume->lun, volume->target->name);
+		err = -EINVAL;
+	}

 	return err;
 }
@@ -108,16 +219,25 @@
 	}

 	ret = copy_from_user(args, (void *)(unsigned long)info->args_ptr,
-			     info->args_len);
+				info->args_len);
 	if (ret) {
 		ret = -EFAULT;
 		goto free_args;
 	}

-	ret = set_iotype(volume, args);
+
+	ret = parse_volume_params(volume, args, setio_do_param);
 	if (ret < 0)
 		goto free_args;

+	/* if iotype not defined, use DEFAULT_IOTYPE */
+	if (!(volume->iotype)) {
+		if (!(volume->iotype = get_iotype(DEFAULT_IOTYPE))) {
+			ret = -ENOENT;
+			goto free_args;
+		}
+	}
+
 	ret = volume->iotype->attach(volume, args);
 	if (ret < 0)
 		goto free_args;