[Ocfs2-tools-devel] [PATCH 1/1] ocfs2-tools: Add setting custom UUID in tunefs.ocfs2 v2

[tristan]

Tiger,

Just few trivial comments inlined:)

Tiger Yang wrote:
> This patch enhance uuid-reset operation to set custom UUID.
> The format of the UUID could be 32 bytes or 36 bytes.
> For example: 178BDC83D50241EF94EB474A677d498B or
> 178BDC83-D502-41EF-94EB-474A677D498B.
>
> Signed-off-by: Tiger Yang <tiger.yang at oracle.com>
> ---
>  tunefs.ocfs2/ocfs2ne.c         |    8 ++-
>  tunefs.ocfs2/op_reset_uuid.c   |   85 ++++++++++++++++++++++++++++++++++++++--
>  tunefs.ocfs2/tunefs.ocfs2.8.in |    4 +-
>  3 files changed, 88 insertions(+), 9 deletions(-)
>
> diff --git a/tunefs.ocfs2/ocfs2ne.c b/tunefs.ocfs2/ocfs2ne.c
> index 48760c6..df4beb9 100644
> --- a/tunefs.ocfs2/ocfs2ne.c
> +++ b/tunefs.ocfs2/ocfs2ne.c
> @@ -490,10 +490,12 @@ static struct tunefs_option list_sparse_option = {
>  
>  static struct tunefs_option reset_uuid_option = {
>  	.opt_option	= {
> -		.name	= "uuid-reset",
> -		.val	= 'U',
> +		.name		= "uuid-reset",
> +		.val		= 'U',
> +		.has_arg	= 2,
>  	},
> -	.opt_help	= "-U|--uuid-reset",
> +	.opt_help	= "-U|--uuid-reset[=new-uuid]",
> +	.opt_handle	= &generic_handle_arg,
>  	.opt_op		= &reset_uuid_op,
>  };
>  
> diff --git a/tunefs.ocfs2/op_reset_uuid.c b/tunefs.ocfs2/op_reset_uuid.c
> index 25a21fb..7e91a1e 100644
> --- a/tunefs.ocfs2/op_reset_uuid.c
> +++ b/tunefs.ocfs2/op_reset_uuid.c
> @@ -27,8 +27,26 @@
>  
>  #include "libocfs2ne.h"
>  
> +/*
> + *	Translate 32 bytes uuid to 36 bytes uuid format.
> + *	for example:
> + *	32 bytes uuid: 178BDC83D50241EF94EB474A677D498B
> + *	36 bytes uuid: 178BDC83-D502-41EF-94EB-474A677D498B
> + */
> +static void uuid_translate(char *uuid_32, char *uuid_36)
> +{
> +	int i;
> +	char *cp = uuid_32;

a blank line to separate the declaration and codes?

> +	for (i = 0; i < 36; i++) {
> +		if ((i == 8) || (i == 13) || (i == 18) || (i == 23)) {

To jude four conditions in each iteration is expensive I guess, why not 
do it outside:

uuid_36[8] = uuid_36[13] = uuid_36[18] = uuid_36[23] = '-';

and inside the for loop, use:

if (uuid_36[i] == '-')
/* here we also do one condition judgement */
continue
else
uuid_36[i] = *cp++;


> +			uuid_36[i] = '-';
> +			continue;
> +		}
> +		uuid_36[i] = *cp++;
> +	}
> +}
>  
> -static errcode_t update_volume_uuid(ocfs2_filesys *fs)
> +static errcode_t update_volume_uuid(ocfs2_filesys *fs, char *uuid)
>  {
>  	errcode_t err;
>  	unsigned char new_uuid[OCFS2_VOL_UUID_LEN];
> @@ -38,11 +56,34 @@ static errcode_t update_volume_uuid(ocfs2_filesys *fs)
>  			    fs->fs_devname))
>  		return 0;
>  
> +	if (uuid != NULL) {
> +		if (!tools_interact_critical(
> +			"WARNING!!! OCFS2 uses the UUID to uniquely identify "
> +			"a file system. Having two OCFS2 file systems with "
> +			"the same UUID could, in the least, cause erratic "
> +			"behavior, and if unlucky, cause file system damage. "
> +			"Please choose the UUID with care.\n"
> +			"Still reset the UUID ?"))
> +			return 0;
> +	}
> +
>  	prog = tools_progress_start("Resetting UUID", "resetuuid", 1);
>  	if (!prog)
>  		return TUNEFS_ET_NO_MEMORY;
>  
> -	uuid_generate(new_uuid);
> +	if (uuid == NULL)
> +		uuid_generate(new_uuid);
> +	else {
> +		if (strlen(uuid) == 32) {
> +			char uuid_36[37] = {'\0'};

should the declaration of variable be at the very beginning of function?

> +
> +			/*translate 32 bytes uuid to 36 bytes uuid*/
> +			uuid_translate(uuid, uuid_36);
> +			/*uuid_parse only support 36 bytes uuid*/
> +			uuid_parse(uuid_36, new_uuid);
> +		} else
> +			uuid_parse(uuid, new_uuid);
> +	}
>  	memcpy(OCFS2_RAW_SB(fs->fs_super)->s_uuid, new_uuid,
>  	       OCFS2_VOL_UUID_LEN);
>  
> @@ -55,13 +96,49 @@ static errcode_t update_volume_uuid(ocfs2_filesys *fs)
>  	return err;
>  }
>  
> +static int reset_uuid_parse_option(struct tunefs_operation *op, char *arg)
> +{
> +	int i;
> +	char *cp = arg;
> +	int len;
> +
> +	if (arg == NULL)
> +		goto out;
> +
> +	len = strlen(arg);
> +	/*check the uuid length, we support 32/36 bytes uuid format*/
> +	if (len != 32 && len != 36)
> +		goto bail;
> +
> +	/*check wheter each character of uuid is a hexadecimal digit*/
> +	for (i = 0; i < len; i++, cp++) {
> +		if (len == 36) {
> +			if ((i == 8) || (i == 13) || (i == 18) || (i == 23)) {
> +				if (*cp == '-')
> +					continue;
> +				else
> +					goto bail;
> +			}
> +		}
> +		if (!isxdigit(*cp))
> +			goto bail;
> +	}
> +out:
> +	op->to_private = arg;
> +	return 0;
> +bail:
> +	errorf("Invalid UUID\n");
> +	return 1;
> +}
> +
>  static int reset_uuid_run(struct tunefs_operation *op, ocfs2_filesys *fs,
>  			  int flags)
>  {
>  	int rc = 0;
>  	errcode_t err;
> +	char *uuid = op->to_private;
>  
> -	err = update_volume_uuid(fs);
> +	err = update_volume_uuid(fs, uuid);
>  	if (err) {
>  		tcom_err(err, "- unable to reset the uuid on device \"%s\"",
>  			 fs->fs_devname);
> @@ -75,7 +152,7 @@ static int reset_uuid_run(struct tunefs_operation *op, ocfs2_filesys *fs,
>  DEFINE_TUNEFS_OP(reset_uuid,
>  		 "Usage: op_reset_uuid [opts] <device>\n",
>  		 TUNEFS_FLAG_RW,
> -		 NULL,
> +		 reset_uuid_parse_option,
>  		 reset_uuid_run);
>  
>  #ifdef DEBUG_EXE
> diff --git a/tunefs.ocfs2/tunefs.ocfs2.8.in b/tunefs.ocfs2/tunefs.ocfs2.8.in
> index 3bebd41..5f7c237 100644
> --- a/tunefs.ocfs2/tunefs.ocfs2.8.in
> +++ b/tunefs.ocfs2/tunefs.ocfs2.8.in
> @@ -78,8 +78,8 @@ printf(3) type formatters. The list of type specifiers is as follows:
>  Quiet mode.
>  
>  .TP
> -\fB\-U, \-\-uuid\-reset\fR
> -Change the volume UUID (auto-generated) for the file system.
> +\fB\-U, \-\-uuid\-reset\fR[=\fInew-uuid]\fR
> +Set the volume UUID for the file system. If not provided, will auto generate a new UUID. The format of the provided UUID should be 178BDC83D50241EF94EB474A677D498B or 178BDC83-D502-41EF-94EB-474A677D498B.
>  
>  .TP
>  \fB\-v, \-\-verbose\fR