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

-----Original Message----- From: libguestfs-bounces(a)redhat.com [mailto:libguestfs-bounces@redhat.com] On Behalf Of Pino Toscano Sent: Wednesday, June 24, 2015 6:16 PM To: libguestfs(a)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(a)cn.fujitsu.com&gt; > --- > 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 _______________________________________________ Libguestfs mailing list Libguestfs(a)redhat.com https://www.redhat.com/mailman/listinfo/libguestfs