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

[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 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?

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