Hi, In data giovedì 4 giugno 2015 11:56:41, Pino Tsao ha scritto: > Disable the test case temporarily for 2 reasons: > 1. Because the default test disk size is 500M, while btrfs > convert command think it is too small to convert it(actually, > just add 10M or 20M more is enough). If the base test disks that are available in tests/c-api/tests are not enough, just write the test (creating handle, scratch disks, partitions, etc) in shell or perl, just like the other ones available in tests/btrfs. This kind of API needs at least some basic coverage.

> 2. Btrfs-progs has may have a tiny bug, when execute the command > in guestfish, it report some error, but convert the filesystem to > btrfs successfully. I don't understand, are you saying that `btrfs convert` will exit with failure but doing the actual changes? If so, is that a known/reported upstream bug?

> > Signed-off-by: Pino Tsao <caoj.fnst(a)cn.fujitsu.com&gt; > --- > daemon/btrfs.c | 29 +++++++++++++++++++++++++++++ > generator/actions.ml | 18 ++++++++++++++++++ > 2 files changed, 47 insertions(+) > > diff --git a/daemon/btrfs.c b/daemon/btrfs.c > index 39392f7..fd679cf 100644 > --- a/daemon/btrfs.c > +++ b/daemon/btrfs.c > @@ -38,6 +38,7 @@ GUESTFSD_EXT_CMD(str_btrfsck, btrfsck); > GUESTFSD_EXT_CMD(str_mkfs_btrfs, mkfs.btrfs); > GUESTFSD_EXT_CMD(str_umount, umount); > GUESTFSD_EXT_CMD(str_btrfsimage, btrfs-image); > +GUESTFSD_EXT_CMD(str_btrfsconvert, btrfs-convert); > > int > optgroup_btrfs_available (void) > @@ -2083,3 +2084,31 @@ do_btrfs_image (char *const *sources, const char *image, > > return 0; > } > + > +int > +do_btrfs_convert (const char *device, int rollback) > +{ > + const size_t MAX_ARGS = 64; > + const char *argv[MAX_ARGS]; > + size_t i = 0; > + CLEANUP_FREE char *err = NULL; > + CLEANUP_FREE char *out = NULL; > + int r; > + > + ADD_ARG (argv, i, str_btrfsconvert); > + ADD_ARG (argv, i, device); > + > + if ((optargs_bitmask & GUESTFS_BTRFS_CONVERT_ROLLBACK_BITMASK) && rollback) > + ADD_ARG (argv, i, "-r"); > + > + ADD_ARG (argv, i, NULL); > + > + r = commandv (&out, &err, argv); > + if (r == -1) { > + reply_with_error ("%s: %s", device, err); > + return -1; > + } > + > + return 0; > +} "out" seems not used, so no need to collect it from commandv.

> diff --git a/generator/actions.ml b/generator/actions.ml > index 1a89869..e42f02d 100644 > --- a/generator/actions.ml > +++ b/generator/actions.ml > @@ -12579,6 +12579,24 @@ numbered C<partnum> on device C<device>. > > It returns C<primary>, C<logical>, or C<extended>." }; > > + { defaults with > + name = "btrfs_convert"; > + style = RErr, [Device "device"], [OBool "rollback"]; > + proc_nr = Some 455; > + optional = Some "btrfs"; camel_name = "BTRFSConvert"; > + tests = [ > + InitEmpty, Disabled, TestRun ( > + [["mkfs"; "ext2"; "/dev/sda"; ""; "NOARG"; ""; ""; "NOARG"]; > + ["btrfs_convert"; "/dev/sda"; ""]; > + ["btrfs_convert"; "/dev/sda"; "true"]]), [] > + ]; ^ extra white spaces at the end

> + shortdesc = "convert from ext2/3/4 filesystem to btrfs or rollback"; > + longdesc = "\ > +This is used to convert existed ext2/3/4 to btrfs filesystem, and the original > +filesystem image is accessible as from separate subvolume named ext2_saved as file image. While I understand you copied this description from the btrfs-convert(1) man page, please at least make sure it is proper grammar: This is used to convert an existing ext2/3/4 filesystem to btrfs; a copy of the original filesystem image is still available in a separate subvolume named "ext2_saved".

Is it still correct? If the upstream documentation is still incorrect, please report the mistakes so it can be fixed there.

Thanks,

Hi, In data giovedì 4 giugno 2015 11:56:41, Pino Tsao ha scritto: > Disable the test case temporarily for 2 reasons: > 1. Because the default test disk size is 500M, while btrfs > convert command think it is too small to convert it(actually, > just add 10M or 20M more is enough). If the base test disks that are available in tests/c-api/tests are not enough, just write the test (creating handle, scratch disks, partitions, etc) in shell or perl, just like the other ones available in tests/btrfs. This kind of API needs at least some basic coverage. > 2. Btrfs-progs has may have a tiny bug, when execute the command > in guestfish, it report some error, but convert the filesystem to > btrfs successfully. I don't understand, are you saying that `btrfs convert` will exit with failure but doing the actual changes? If so, is that a known/reported upstream bug? > > Signed-off-by: Pino Tsao <caoj.fnst(a)cn.fujitsu.com&gt; > --- > daemon/btrfs.c | 29 +++++++++++++++++++++++++++++ > generator/actions.ml | 18 ++++++++++++++++++ > 2 files changed, 47 insertions(+) > > diff --git a/daemon/btrfs.c b/daemon/btrfs.c > index 39392f7..fd679cf 100644 > --- a/daemon/btrfs.c > +++ b/daemon/btrfs.c > @@ -38,6 +38,7 @@ GUESTFSD_EXT_CMD(str_btrfsck, btrfsck); > GUESTFSD_EXT_CMD(str_mkfs_btrfs, mkfs.btrfs); > GUESTFSD_EXT_CMD(str_umount, umount); > GUESTFSD_EXT_CMD(str_btrfsimage, btrfs-image); > +GUESTFSD_EXT_CMD(str_btrfsconvert, btrfs-convert); > > int > optgroup_btrfs_available (void) > @@ -2083,3 +2084,31 @@ do_btrfs_image (char *const *sources, const char *image, > > return 0; > } > + > +int > +do_btrfs_convert (const char *device, int rollback) > +{ > + const size_t MAX_ARGS = 64; > + const char *argv[MAX_ARGS]; > + size_t i = 0; > + CLEANUP_FREE char *err = NULL; > + CLEANUP_FREE char *out = NULL; > + int r; > + > + ADD_ARG (argv, i, str_btrfsconvert); > + ADD_ARG (argv, i, device); > + > + if ((optargs_bitmask & GUESTFS_BTRFS_CONVERT_ROLLBACK_BITMASK) && rollback) > + ADD_ARG (argv, i, "-r"); > + > + ADD_ARG (argv, i, NULL); > + > + r = commandv (&out, &err, argv); > + if (r == -1) { > + reply_with_error ("%s: %s", device, err); > + return -1; > + } > + > + return 0; > +} "out" seems not used, so no need to collect it from commandv. > diff --git a/generator/actions.ml b/generator/actions.ml > index 1a89869..e42f02d 100644 > --- a/generator/actions.ml > +++ b/generator/actions.ml > @@ -12579,6 +12579,24 @@ numbered C<partnum> on device C<device>. > > It returns C<primary>, C<logical>, or C<extended>." }; > > + { defaults with > + name = "btrfs_convert"; > + style = RErr, [Device "device"], [OBool "rollback"]; > + proc_nr = Some 455; > + optional = Some "btrfs"; camel_name = "BTRFSConvert"; > + tests = [ > + InitEmpty, Disabled, TestRun ( > + [["mkfs"; "ext2"; "/dev/sda"; ""; "NOARG"; ""; ""; "NOARG"]; > + ["btrfs_convert"; "/dev/sda"; ""]; > + ["btrfs_convert"; "/dev/sda"; "true"]]), [] > + ]; ^ extra white spaces at the end > + shortdesc = "convert from ext2/3/4 filesystem to btrfs or rollback"; > + longdesc = "\ > +This is used to convert existed ext2/3/4 to btrfs filesystem, and the original > +filesystem image is accessible as from separate subvolume named ext2_saved as file image. While I understand you copied this description from the btrfs-convert(1) man page, please at least make sure it is proper grammar: This is used to convert an existing ext2/3/4 filesystem to btrfs; a copy of the original filesystem image is still available in a separate subvolume named "ext2_saved". Is it still correct?

If the upstream documentation is still incorrect, please report the mistakes so it can be fixed there. Thanks,