Libguestfs - Jun 2015 - Re: [PATCH RFC][Resend] New API: btrfs_convert

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).
    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.

Signed-off-by: Pino Tsao <caoj.fnst@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;
+}
+
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"]]),
[]
+    ];    
+    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.
+
+If you want to rollback to ext2/3/4, set rollback to true." };
+
 ]
 
 (* Non-API meta-commands available only in guestfish.
-- 
2.1.0

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@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

Hi Toscano

在 2015年06月05日 00:37, 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.
Ok, I can do that. But actually, I think the following 2nd problem 
matters more, I mean, even if I write the test, but it will still fail 
,because of the tiny problem of btrfs-convert
>
>>      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?
>
Yes, exactly, it exit with failure but actually got what we want: 
convert fs from ext2/3/4 to btrfs successfully. While it can rollback 
normally without any error. I did the test in guestfish using a img 
file, and I also checked the result by examining img file outside the 
guestfish, prove that the API worked.

I searched in the btrfs mail list archive, did`t see it is a known bug. 
because the btrfs convert command works well in my host. It just has 
that problem in supermin appliance for me for now.
I will keep following this problem, with my colleague, who is a 
contributor of btrfs-progs
>>
>> Signed-off-by: Pino Tsao <caoj.fnst@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.
>
affirmative.
>> 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
affirmative.
>
>> +    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".
>
affirmative.
> Is it still correct?
> If the upstream documentation is still incorrect, please report the
> mistakes so it can be fixed there.
I am not sure what correct you are talking about. I use the latest 
btrfs-progs(4.0.1), and I check it with "btrfs subvolume list
<path>":

	ID 256 gen 13 top level 5 path ext2_saved

the orginal filesystem is accessible as a file named "image" in dir 
named "ext2_save", where the subvolue mount
>
> Thanks,
>
-- 
Yours Sincerely,

Pino Tsao

Hi Toscano,
     please see my comment inline in the last section:)

在 2015年06月05日 00:37, 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@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?
I think I misunderstood you just now. Yes, it is still right. I think 
your correction is better and more clearly, the original description is 
a little complicated for guys who are not native speaker of English.
> If the upstream documentation is still incorrect, please report the
> mistakes so it can be fixed there.
>
> Thanks,
>
-- 
Yours Sincerely,

Cao Jin