Libguestfs - Jun 2015 - Re: [PATCH v3.1 1/9] uuid: add support to change uuid of btrfs partition

In data venerdì 26 giugno 2015 17:35:36, 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>
> ---
> v3.1: fix a typo
> v3: set errno as ENOTSUP when btrfstune -u is not available
> 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                 |  6 ++---
>  tests/btrfs/test-btrfs-misc.pl | 17 ++++++++++++
>  4 files changed, 80 insertions(+), 4 deletions(-)
> 
> diff --git a/daemon/btrfs.c b/daemon/btrfs.c
> index 20e5e6b..a69c512 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_btrfstune_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'.
So --help should be passed anyway to the btrfstune invocation, so with
newer versions hopefully there will not be complaints about it anymore.
> +   * 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
> +btrfs_set_uuid (const char *device, const char *uuid)
> +{
> +  CLEANUP_FREE char *err = NULL;
> +  int r;
> +  int has_uuid_opts = test_btrfstune_uuid_opt ();
> +
> +  if (has_uuid_opts <= 0) {
> +    reply_with_error_errno (ENOTSUP, "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..b74ba3c 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 btrfs_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..f98d8e5 100644
> --- a/daemon/uuids.c
> +++ b/daemon/uuids.c
> @@ -110,10 +110,8 @@ do_set_uuid (const char *device, const char *uuid)
>    else if (STREQ (vfs_type, "swap"))
>      r = swapuuid (device, uuid);
>  
> -  else if (STREQ (vfs_type, "btrfs")) {
> -    reply_with_error ("btrfs filesystems' UUID cannot be
changed");
> -    r = -1;
> -  }
> +  else if (STREQ (vfs_type, "btrfs"))
> +    r = btrfs_set_uuid (device, uuid);
>  
>    else {
>      reply_with_error ("don't know how to set the UUID for
'%s' filesystems",
> diff --git a/tests/btrfs/test-btrfs-misc.pl
b/tests/btrfs/test-btrfs-misc.pl
> index 2fe7c59..b47caab 100755
> --- a/tests/btrfs/test-btrfs-misc.pl
> +++ b/tests/btrfs/test-btrfs-misc.pl
> @@ -20,6 +20,7 @@
>  
>  use strict;
>  use warnings;
> +use Errno;
>  
>  use Sys::Guestfs;
>  
> @@ -47,5 +48,21 @@ my $label = $g->vfs_label ("/dev/sda1");
>  die "unexpected label: expecting 'newlabel' but got
'$label'"
>      unless $label eq "newlabel";
>  
> +# Setting btrfs UUID
> +eval {
> +    $g->set_uuid ("/dev/sda1",
"12345678-1234-1234-1234-123456789012");
> +};
> +# FIXME: ignore ESRCH
What is ESRCH about?
> +my $err = $g->last_errno ();
> +
> +if ($err == 0) {
> +    my $uuid = $g->vfs_uuid ("/dev/sda1");
> +    die "unexpected uuid expecting
> +     '12345678-1234-1234-1234-123456789012' but got
'$uuid'"
> +    unless $uuid eq "12345678-1234-1234-1234-123456789012";
> +} elsif ($err == Errno::ENOTSUP()) {
> +    warn "$0: skipping test for btrfs UUID change feature is not
available\n";
> +}
IIRC 'warn' prints the newline at the end already.

This is missing to check when $err is a valid error.

Thanks,
-- 
Pino Toscano

> -----Original Message-----
> From: libguestfs-bounces@redhat.com [mailto:libguestfs-bounces@redhat.com]
On
> Behalf Of Pino Toscano
> Sent: Friday, June 26, 2015 8:12 PM
> To: libguestfs@redhat.com
> Subject: Re: [Libguestfs] [PATCH v3.1 1/9] uuid: add support to change uuid
of btrfs
> partition
> 
> In data venerdì 26 giugno 2015 17:35:36, 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>
> > ---
> > v3.1: fix a typo
> > v3: set errno as ENOTSUP when btrfstune -u is not available
> > 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                 |  6 ++---
> >  tests/btrfs/test-btrfs-misc.pl | 17 ++++++++++++
> >  4 files changed, 80 insertions(+), 4 deletions(-)
> >
> > diff --git a/daemon/btrfs.c b/daemon/btrfs.c
> > index 20e5e6b..a69c512 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_btrfstune_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'.
> 
> So --help should be passed anyway to the btrfstune invocation, so with
> newer versions hopefully there will not be complaints about it anymore.
Fine.
> 
[snip]
> > diff --git a/tests/btrfs/test-btrfs-misc.pl
b/tests/btrfs/test-btrfs-misc.pl
> > index 2fe7c59..b47caab 100755
> > --- a/tests/btrfs/test-btrfs-misc.pl
> > +++ b/tests/btrfs/test-btrfs-misc.pl
> > @@ -20,6 +20,7 @@
> >
> >  use strict;
> >  use warnings;
> > +use Errno;
> >
> >  use Sys::Guestfs;
> >
> > @@ -47,5 +48,21 @@ my $label = $g->vfs_label
("/dev/sda1");
> >  die "unexpected label: expecting 'newlabel' but got
'$label'"
> >      unless $label eq "newlabel";
> >
> > +# Setting btrfs UUID
> > +eval {
> > +    $g->set_uuid ("/dev/sda1",
"12345678-1234-1234-1234-123456789012");
> > +};
> > +# FIXME: ignore ESRCH
> 
> What is ESRCH about?
The problem is every time I got a ESRCH here.
I have no idea about this.

Can we just ignore it by:
} elsif ($err == Errno::ESRCH()) {
    ;
} else {
   die $@;
}
> 
> > +my $err = $g->last_errno ();
> > +
> > +if ($err == 0) {
> > +    my $uuid = $g->vfs_uuid ("/dev/sda1");
> > +    die "unexpected uuid expecting
> > +     '12345678-1234-1234-1234-123456789012' but got
'$uuid'"
> > +    unless $uuid eq "12345678-1234-1234-1234-123456789012";
> > +} elsif ($err == Errno::ENOTSUP()) {
> > +    warn "$0: skipping test for btrfs UUID change feature is not
available\n";
> > +}
> 
> IIRC 'warn' prints the newline at the end already.
> 
> This is missing to check when $err is a valid error.
> 
Can we just:
} else {
   die $@;
}

or some hints?

Regards,
- Chen

On Fri, Jun 26, 2015 at 02:12:00PM +0200, Pino Toscano
wrote:
> > +    warn "$0: skipping test for btrfs UUID change feature is not
available\n";
> > +}
> 
> IIRC 'warn' prints the newline at the end already.
Actually in Perl, 'warn' and 'die' have magical behaviour here.
Without \n, they print the current line number.  With \n, the current
line number is suppressed:

$ perl -e 'die "foo"'
foo at -e line 1.
$ perl -e 'die "foo\n"'
foo

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

In data lunedì 29 giugno 2015 02:36:57, Chen, Hanxiao ha
scritto:
> 
> > -----Original Message-----
> > From: libguestfs-bounces@redhat.com
[mailto:libguestfs-bounces@redhat.com] On
> > Behalf Of Pino Toscano
> > Sent: Friday, June 26, 2015 8:12 PM
> > To: libguestfs@redhat.com
> > Subject: Re: [Libguestfs] [PATCH v3.1 1/9] uuid: add support to change
uuid of btrfs
> > partition
> > 
> > In data venerdì 26 giugno 2015 17:35:36, 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>
> > > ---
> > > v3.1: fix a typo
> > > v3: set errno as ENOTSUP when btrfstune -u is not available
> > > 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                 |  6 ++---
> > >  tests/btrfs/test-btrfs-misc.pl | 17 ++++++++++++
> > >  4 files changed, 80 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/daemon/btrfs.c b/daemon/btrfs.c
> > > index 20e5e6b..a69c512 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_btrfstune_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'.
> > 
> > So --help should be passed anyway to the btrfstune invocation, so with
> > newer versions hopefully there will not be complaints about it
anymore.
> 
> Fine.
> 
> > 
> [snip]
> > > diff --git a/tests/btrfs/test-btrfs-misc.pl
b/tests/btrfs/test-btrfs-misc.pl
> > > index 2fe7c59..b47caab 100755
> > > --- a/tests/btrfs/test-btrfs-misc.pl
> > > +++ b/tests/btrfs/test-btrfs-misc.pl
> > > @@ -20,6 +20,7 @@
> > >
> > >  use strict;
> > >  use warnings;
> > > +use Errno;
> > >
> > >  use Sys::Guestfs;
> > >
> > > @@ -47,5 +48,21 @@ my $label = $g->vfs_label
("/dev/sda1");
> > >  die "unexpected label: expecting 'newlabel' but got
'$label'"
> > >      unless $label eq "newlabel";
> > >
> > > +# Setting btrfs UUID
> > > +eval {
> > > +    $g->set_uuid ("/dev/sda1",
"12345678-1234-1234-1234-123456789012");
> > > +};
> > > +# FIXME: ignore ESRCH
> > 
> > What is ESRCH about?
> 
> The problem is every time I got a ESRCH here.
> I have no idea about this.
> 
> Can we just ignore it by:
> } elsif ($err == Errno::ESRCH()) {
>     ;
> } else {
>    die $@;
> }
No, please investigate why you are getting such errno.

One guess is that a leftover from a failing operation, and that
set_uuid actually succeeded. In that case, checking first for $@ might
do the trick:

 eval {
     ...
 }
 if ($@) {
     # failed, check last_errno
 } else {
     # good, compare with vfs_uuid
 }
> > > +my $err = $g->last_errno ();
> > > +
> > > +if ($err == 0) {
> > > +    my $uuid = $g->vfs_uuid ("/dev/sda1");
> > > +    die "unexpected uuid expecting
> > > +     '12345678-1234-1234-1234-123456789012' but got
'$uuid'"
> > > +    unless $uuid eq
"12345678-1234-1234-1234-123456789012";
> > > +} elsif ($err == Errno::ENOTSUP()) {
> > > +    warn "$0: skipping test for btrfs UUID change feature
is not available\n";
> > > +}
> > 
> > IIRC 'warn' prints the newline at the end already.
> > 
> > This is missing to check when $err is a valid error.
> > 
> 
> Can we just:
> } else {
>    die $@;
> }
Yes, that should be fine.

Thanks,
-- 
Pino Toscano