[Libguestfs] [PATCH RFC][Resend] New API: btrfs_convert

[Pino Toscano]

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 at cn.fujitsu.com>
> ---
>  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,
-- 
Pino Toscano