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

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,

In data venerdì 5 giugno 2015 13:47:12, Pino ha scritto:
> 在 2015年06月05日 00:37, Pino Toscano 写道:
> > 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.
Is the issue (btrfs convert doing the job but exiting with error)
happening only within the appliance of libguestfs, or also when run
in a normal installation?

 * if the failures happens only within libguestfs, then we need to
   figure out what's the issue, and what btrfs convert could require
   from the appliance

 * if it happens also when run in a normal installation, that's an
   upstream bug to fix

In any case, it does not make sense to add an API which does not
provide a reliable return value. Either it returns a failure and does
nothing, or it returns success and does the requested changes. 

-- 
Pino Toscano

Hi

在 2015年06月05日 22:38, Pino Toscano 写道:
> Hi,
>
> In data venerdì 5 giugno 2015 13:47:12, Pino ha scritto:
>> 在 2015年06月05日 00:37, Pino Toscano 写道:
>>> 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.
>
> Is the issue (btrfs convert doing the job but exiting with error)
> happening only within the appliance of libguestfs, or also when run
> in a normal installation?
>
>   * if the failures happens only within libguestfs, then we need to
>     figure out what's the issue, and what btrfs convert could require
>     from the appliance
>
>   * if it happens also when run in a normal installation, that's an
>     upstream bug to fix
>
> In any case, it does not make sense to add an API which does not
> provide a reliable return value. Either it returns a failure and does
> nothing, or it returns success and does the requested changes.
>
For now, the issue only happens within the appliance of libguestfs. It 
is ok in a normal installation.

Here is my questions now: Do we have approach to debug in kernel in 
appliance? I am aware that I can fprintf messages in the daemon to 
stderr, but it seems it isn`t enough for me now:(

-- 
Yours Sincerely,

Pino Tsao