Libguestfs - Jun 2015 - Re: [PATCH v2 1/5] uuid: add support to change uuid of btrfs partition

In data mercoledì 24 giugno 2015 15:54:03, Chen Hanxiao ha
scritto:
> btrfs-progs v4.1 add support to change uuid of btrfs fs.
> 
> Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
> ---
> v2: put btrfs operation back to daemon/btrfs.c
>     move tests to tests/btrfs
> 
>  daemon/btrfs.c                 | 60
++++++++++++++++++++++++++++++++++++++++++
>  daemon/daemon.h                |  1 +
>  daemon/uuids.c                 |  9 +++++--
>  tests/btrfs/test-btrfs-misc.pl |  6 +++++
>  4 files changed, 74 insertions(+), 2 deletions(-)
> 
> diff --git a/daemon/btrfs.c b/daemon/btrfs.c
> index 20e5e6b..b82c1b9 100644
> --- a/daemon/btrfs.c
> +++ b/daemon/btrfs.c
> @@ -790,6 +790,44 @@ do_btrfs_device_delete (char *const *devices, const
char *fs)
>    return 0;
>  }
>  
> +
> +/* btrfstune command add two new options
> + * -U UUID      change fsid to UUID
> + * -u           change fsid, use a random one
> + * since v4.1
> + * We could check wheter 'btrfstune' support
> + * '-u' and '-U UUID' option by checking the output of
> + * 'btrfstune' command.
> + */
> +static int
> +test_btrftune_uuid_opt (void)
> +{
> +  static int result = -1;
> +  if (result != -1)
> +    return result;
> +
> +  CLEANUP_FREE char *err = NULL;
> +
> +  int r = commandr (NULL, &err, str_btrfstune, NULL);
> +
> +  if (r == -1) {
> +    reply_with_error ("btrfstune: %s", err);
> +    return -1;
> +  }
> +
> +  /* FIXME currently btrfstune do not support '--help'.
> +   * If got an invalid options, it will print its usage
> +   * in stderr.
> +   * We had to check it there.
> +   */
> +  if (strstr (err, "-U") == NULL || strstr (err, "-u")
== NULL)
> +    result = 0;
> +  else
> +    result = 1;
> +
> +  return result;
> +}
> +
>  int
>  do_btrfs_set_seeding (const char *device, int svalue)
>  {
> @@ -807,6 +845,28 @@ do_btrfs_set_seeding (const char *device, int svalue)
>    return 0;
>  }
>  
> +int
> +btrfstune_set_uuid (const char *device, const char *uuid)
Call it btrfs_set_uuid please, as the fact that it uses btrfstune is an
implementation detail of it.
> +{
> +  CLEANUP_FREE char *err = NULL;
> +  int r;
> +  int has_uuid_opts = test_btrftune_uuid_opt ();
> +
> +  if (has_uuid_opts == 0) {
Check for <= 0 here, to consider errors in test_btrftune_uuid_opt
on the safe side.
> +    reply_with_error ("btrfstune do not support '-u'");
 reply_with_error ("btrfs filesystems' UUID cannot be changed");
> +    return -1;
> +  }
> +
> +  r = commandr (NULL, &err, str_btrfstune, "-f",
"-U", uuid, device, NULL);
> +
> +  if (r == -1) {
> +    reply_with_error ("%s: %s", device, err);
> +    return -1;
> +  }
> +
> +  return 0;
> +}
> +
>  /* Takes optional arguments, consult optargs_bitmask. */
>  int
>  do_btrfs_fsck (const char *device, int64_t superblock, int repair)
> diff --git a/daemon/daemon.h b/daemon/daemon.h
> index 136e9a9..ee0c96f 100644
> --- a/daemon/daemon.h
> +++ b/daemon/daemon.h
> @@ -268,6 +268,7 @@ extern char *debug_bmap_device (const char *subcmd,
size_t argc, char *const *co
>  
>  /*-- in btrfs.c --*/
>  extern char *btrfs_get_label (const char *device);
> +extern int btrfstune_set_uuid (const char *device, const char *uuid);
>  
>  /*-- in ntfs.c --*/
>  extern char *ntfs_get_label (const char *device);
> diff --git a/daemon/uuids.c b/daemon/uuids.c
> index 06b33e9..f7791ab 100644
> --- a/daemon/uuids.c
> +++ b/daemon/uuids.c
> @@ -91,6 +91,12 @@ swapuuid (const char *device, const char *uuid)
>    return 0;
>  }
>  
> +static int
> +btrfsuuid (const char *device, const char *uuid)
> +{
> +  return btrfstune_set_uuid (device, uuid);
> +}
What is this wrapper for? Just call btrfs_set_uuid below.
>  int
>  do_set_uuid (const char *device, const char *uuid)
>  {
> @@ -111,8 +117,7 @@ do_set_uuid (const char *device, const char *uuid)
>      r = swapuuid (device, uuid);
>  
>    else if (STREQ (vfs_type, "btrfs")) {
> -    reply_with_error ("btrfs filesystems' UUID cannot be
changed");
> -    r = -1;
> +    r = btrfsuuid (device, uuid);
>    }
Minor style note: curly brackets can be removed now.
>    else {
> diff --git a/tests/btrfs/test-btrfs-misc.pl
b/tests/btrfs/test-btrfs-misc.pl
> index 2fe7c59..cd60990 100755
> --- a/tests/btrfs/test-btrfs-misc.pl
> +++ b/tests/btrfs/test-btrfs-misc.pl
> @@ -47,5 +47,11 @@ my $label = $g->vfs_label ("/dev/sda1");
>  die "unexpected label: expecting 'newlabel' but got
'$label'"
>      unless $label eq "newlabel";
>  
> +# Setting btrfs UUID
> +$g->set_uuid ("/dev/sda1",
"12345678-1234-1234-1234-123456789012");
> +my $uuid = $g->vfs_uuid ("/dev/sda1");
> +die "unexpected label: expecting 'newlabel' but got
'$uuid'"
> +    unless $uuid eq "12345678-1234-1234-1234-123456789012";
> +
Ignoring the "newlabel" copy&paste error: this code has the same
issue
as the old test added to the action tests in generator/actions.ml.
That is, if btrfs-tools < 4.1 is used, then the test will fail (instead
of being skipped).

Thinking further about it: it might be better to make set_uuid return
ENOTSUP for all the unsupported filesystems (including older
btrfs-tools case). This way, the perl snippet above could check, when
set_uuid fails, for the last_errno() and consider that a hard failure
if it was different than ENOTSUP. Something like (untested):

 eval {
     $g->set_uuid (...);
 };
 my $err = $g->last_errno ();
 if ($err == 0) {
     my $uuid = $g->vfs_uuid (...);
     die ...
 } elsif ($err != Errno::ENOTSUP()) {
     die $@
 }

-- 
Pino Toscano

Hi, Pino
> -----Original Message-----
> From: libguestfs-bounces@redhat.com [mailto:libguestfs-bounces@redhat.com]
On
> Behalf Of Pino Toscano
> Sent: Wednesday, June 24, 2015 6:16 PM
> To: libguestfs@redhat.com
> Subject: Re: [Libguestfs] [PATCH v2 1/5] uuid: add support to change uuid
of btrfs
> partition
> 
> In data mercoledì 24 giugno 2015 15:54:03, Chen Hanxiao ha scritto:
> > btrfs-progs v4.1 add support to change uuid of btrfs fs.
> >
> > Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
> > ---
> > v2: put btrfs operation back to daemon/btrfs.c
> >     move tests to tests/btrfs
> >
> >  daemon/btrfs.c                 | 60
++++++++++++++++++++++++++++++++++++++++++
> >  daemon/daemon.h                |  1 +
> >  daemon/uuids.c                 |  9 +++++--
> >  tests/btrfs/test-btrfs-misc.pl |  6 +++++
> >  4 files changed, 74 insertions(+), 2 deletions(-)
> >
> > diff --git a/daemon/btrfs.c b/daemon/btrfs.c
> > index 20e5e6b..b82c1b9 100644
> > --- a/daemon/btrfs.c
> > +++ b/daemon/btrfs.c
> > @@ -790,6 +790,44 @@ do_btrfs_device_delete (char *const *devices,
const char
> *fs)
> >    return 0;
> >  }
> >
> > +
> > +/* btrfstune command add two new options
> > + * -U UUID      change fsid to UUID
> > + * -u           change fsid, use a random one
> > + * since v4.1
> > + * We could check wheter 'btrfstune' support
> > + * '-u' and '-U UUID' option by checking the output
of
> > + * 'btrfstune' command.
> > + */
> > +static int
> > +test_btrftune_uuid_opt (void)
> > +{
> > +  static int result = -1;
> > +  if (result != -1)
> > +    return result;
> > +
> > +  CLEANUP_FREE char *err = NULL;
> > +
> > +  int r = commandr (NULL, &err, str_btrfstune, NULL);
> > +
> > +  if (r == -1) {
> > +    reply_with_error ("btrfstune: %s", err);
> > +    return -1;
> > +  }
> > +
> > +  /* FIXME currently btrfstune do not support '--help'.
> > +   * If got an invalid options, it will print its usage
> > +   * in stderr.
> > +   * We had to check it there.
> > +   */
> > +  if (strstr (err, "-U") == NULL || strstr (err,
"-u") == NULL)
> > +    result = 0;
> > +  else
> > +    result = 1;
> > +
> > +  return result;
> > +}
> > +
> >  int
> >  do_btrfs_set_seeding (const char *device, int svalue)
> >  {
> > @@ -807,6 +845,28 @@ do_btrfs_set_seeding (const char *device, int
svalue)
> >    return 0;
> >  }
> >
> > +int
> > +btrfstune_set_uuid (const char *device, const char *uuid)
> 
> Call it btrfs_set_uuid please, as the fact that it uses btrfstune is an
> implementation detail of it.
> 
> > +{
> > +  CLEANUP_FREE char *err = NULL;
> > +  int r;
> > +  int has_uuid_opts = test_btrftune_uuid_opt ();
> > +
> > +  if (has_uuid_opts == 0) {
> 
> Check for <= 0 here, to consider errors in test_btrftune_uuid_opt
> on the safe side.
> 
> > +    reply_with_error ("btrfstune do not support
'-u'");
> 
>  reply_with_error ("btrfs filesystems' UUID cannot be
changed");
How about:

errcode = ENOTSUP;

reply_with_error_errno (errcode, " btrfs filesystems' UUID cannot be
changed");

Then we got the errno, and set_uuid could tell whether we support this feature.

Regards,
- Chen
> 
> > +    return -1;
> > +  }
> > +
> > +  r = commandr (NULL, &err, str_btrfstune, "-f",
"-U", uuid, device, NULL);
> > +
> > +  if (r == -1) {
> > +    reply_with_error ("%s: %s", device, err);
> > +    return -1;
> > +  }
> > +
> > +  return 0;
> > +}
> > +
> >  /* Takes optional arguments, consult optargs_bitmask. */
> >  int
> >  do_btrfs_fsck (const char *device, int64_t superblock, int repair)
> > diff --git a/daemon/daemon.h b/daemon/daemon.h
> > index 136e9a9..ee0c96f 100644
> > --- a/daemon/daemon.h
> > +++ b/daemon/daemon.h
> > @@ -268,6 +268,7 @@ extern char *debug_bmap_device (const char
*subcmd, size_t
> argc, char *const *co
> >
> >  /*-- in btrfs.c --*/
> >  extern char *btrfs_get_label (const char *device);
> > +extern int btrfstune_set_uuid (const char *device, const char *uuid);
> >
> >  /*-- in ntfs.c --*/
> >  extern char *ntfs_get_label (const char *device);
> > diff --git a/daemon/uuids.c b/daemon/uuids.c
> > index 06b33e9..f7791ab 100644
> > --- a/daemon/uuids.c
> > +++ b/daemon/uuids.c
> > @@ -91,6 +91,12 @@ swapuuid (const char *device, const char *uuid)
> >    return 0;
> >  }
> >
> > +static int
> > +btrfsuuid (const char *device, const char *uuid)
> > +{
> > +  return btrfstune_set_uuid (device, uuid);
> > +}
> 
> What is this wrapper for? Just call btrfs_set_uuid below.
> 
> >  int
> >  do_set_uuid (const char *device, const char *uuid)
> >  {
> > @@ -111,8 +117,7 @@ do_set_uuid (const char *device, const char *uuid)
> >      r = swapuuid (device, uuid);
> >
> >    else if (STREQ (vfs_type, "btrfs")) {
> > -    reply_with_error ("btrfs filesystems' UUID cannot be
changed");
> > -    r = -1;
> > +    r = btrfsuuid (device, uuid);
> >    }
> 
> Minor style note: curly brackets can be removed now.
> 
> >    else {
> > diff --git a/tests/btrfs/test-btrfs-misc.pl
b/tests/btrfs/test-btrfs-misc.pl
> > index 2fe7c59..cd60990 100755
> > --- a/tests/btrfs/test-btrfs-misc.pl
> > +++ b/tests/btrfs/test-btrfs-misc.pl
> > @@ -47,5 +47,11 @@ my $label = $g->vfs_label
("/dev/sda1");
> >  die "unexpected label: expecting 'newlabel' but got
'$label'"
> >      unless $label eq "newlabel";
> >
> > +# Setting btrfs UUID
> > +$g->set_uuid ("/dev/sda1",
"12345678-1234-1234-1234-123456789012");
> > +my $uuid = $g->vfs_uuid ("/dev/sda1");
> > +die "unexpected label: expecting 'newlabel' but got
'$uuid'"
> > +    unless $uuid eq "12345678-1234-1234-1234-123456789012";
> > +
> 
> Ignoring the "newlabel" copy&paste error: this code has the
same issue
> as the old test added to the action tests in generator/actions.ml.
> That is, if btrfs-tools < 4.1 is used, then the test will fail (instead
> of being skipped).
> 
> Thinking further about it: it might be better to make set_uuid return
> ENOTSUP for all the unsupported filesystems (including older
> btrfs-tools case). This way, the perl snippet above could check, when
> set_uuid fails, for the last_errno() and consider that a hard failure
> if it was different than ENOTSUP. Something like (untested):
> 
>  eval {
>      $g->set_uuid (...);
>  };
>  my $err = $g->last_errno ();
>  if ($err == 0) {
>      my $uuid = $g->vfs_uuid (...);
>      die ...
>  } elsif ($err != Errno::ENOTSUP()) {
>      die $@
>  }
> 
> --
> Pino Toscano
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs

On Thu, Jun 25, 2015 at 09:49:58AM +0000, Chen, Hanxiao
wrote:
> How about:
> 
> errcode = ENOTSUP;
> 
> reply_with_error_errno (errcode, " btrfs filesystems' UUID cannot
be changed");
> 
> Then we got the errno, and set_uuid could tell whether we support this
feature.
Seems that would make sense, yes.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/