[Libguestfs] [PATCH] New API: btrfs_replace_start
Cao Jin
caoj.fnst at cn.fujitsu.com
Fri Jun 12 11:54:26 UTC 2015
在 2015年06月12日 17:12, Pino Toscano 写道:
> On Friday 12 June 2015 10:58:34 Pino Tsao wrote:
>> Hi,
>>
>> 在 2015年06月11日 17:43, Pino Toscano 写道:
>>> Hi,
>>>
>>> On Wednesday 10 June 2015 17:54:18 Pino Tsao wrote:
>>>> Signed-off-by: Pino Tsao <caoj.fnst at cn.fujitsu.com>
>>>> ---
>>>> daemon/btrfs.c | 40 +++++++++++++++++++++++++++++++++++++++
>>>> generator/actions.ml | 19 +++++++++++++++++++
>>>> tests/btrfs/test-btrfs-devices.sh | 8 ++++++++
>>>> 3 files changed, 67 insertions(+)
>>>>
>>>> diff --git a/daemon/btrfs.c b/daemon/btrfs.c
>>>> index 39392f7..acc300d 100644
>>>> --- a/daemon/btrfs.c
>>>> +++ b/daemon/btrfs.c
>>>> @@ -2083,3 +2083,43 @@ do_btrfs_image (char *const *sources, const char *image,
>>>>
>>>> return 0;
>>>> }
>>>> +
>>>> +int
>>>> +do_btrfs_replace_start (const char *srcdev, const char *targetdev,
>>>> + const char* mntpoint, int force)
>>>> +{
>>>> + const size_t MAX_ARGS = 64;
>>>> + const char *argv[MAX_ARGS];
>>>> + size_t i = 0;
>>>> + CLEANUP_FREE char *err = NULL;
>>>> + CLEANUP_FREE char *path_buf = NULL;
>>>> + int r;
>>>> +
>>>> + path_buf = sysroot_path (mntpoint);
>>>> + if (path_buf == NULL) {
>>>> + reply_with_perror ("malloc");
>>>> + return -1;
>>>> + }
>>>> +
>>>> + ADD_ARG (argv, i, str_btrfs);
>>>> + ADD_ARG (argv, i, "replace");
>>>> + ADD_ARG (argv, i, "start");
>>>> + ADD_ARG (argv, i, srcdev);
>>>> + ADD_ARG (argv, i, targetdev);
>>>> + ADD_ARG (argv, i, path_buf);
>>>> + ADD_ARG (argv, i, "-B");
>>>
>>> I get that -B turns the operation from a background one to synchronous,
>>> so why does this API have a _start suffix?
>>>
>>
>> Because btrfs replace command has 3 subcommand:start/cancel/status, this
>> is the 1st subcommand. For now, implement the necessary start cmd. so I
>> think maybe it is better & more clearly to add start subcommand suffix there
>
> I get that. OTOH, if -B makes the operation synchronous, then an API
> called _start which does all the work synchronously does not make much
> sense. Either it is named with _start doing a background job (and
> there need to be also APIs to stop and check the status), or it has no
> _start suffix and does the job synchronously.
>
Agree.
Actually, according to my test, the btrfs replace start cmd don`t work
without "-B"(exit without error, but don`t get what we want), don`t know
why. Also, I prefer this api work in synchronous way. OTOH, I think no
need to implement cancel/status, user could do replace cmd again instead
of cancel(just like tests I wrote).
So, I will change the api name without _start suffix as you said.
>>
>>>> +
>>>> + if ((optargs_bitmask & GUESTFS_BTRFS_REPLACE_START_FORCE_BITMASK) && force)
>>>> + ADD_ARG (argv, i, "-f");
>>>
>>> Shouldn't -f be always passed, instead of having to choose it?
>>>
>> Here is thing: if user didn`t know the targetdev has filesystem while
>> has valuable data inside, I think it is reasonable to give a hint, then
>> user could deside to change a targetdev, or use "-f", force to wipe out
>> the filesystem
>
> This is something the user must know in advance. Other libguestfs APIs,
> for example mkfs, just do the job regardless of what was there before,
> and I think this API behave the same.
>
> Your thought would work if the only way to use libguestfs APIs is a
> shell, but it cannot apply on non-interactive ways such as C/Python/etc
> applications.
>
Agree that the API should behave the same.
Yes, I forget that the api may be used in non-interactive ways..
OK, I will remove the -f options.
>>>> +
>>>> + ADD_ARG (argv, i, NULL);
>>>> +
>>>> + r = commandv (NULL, &err, argv);
>>>> + if (r == -1) {
>>>> + reply_with_error ("%s: %s", mntpoint, err);
>>>> + return -1;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> diff --git a/generator/actions.ml b/generator/actions.ml
>>>> index 1a89869..4443600 100644
>>>> --- a/generator/actions.ml
>>>> +++ b/generator/actions.ml
>>>> @@ -12579,6 +12579,25 @@ numbered C<partnum> on device C<device>.
>>>>
>>>> It returns C<primary>, C<logical>, or C<extended>." };
>>>>
>>>> + { defaults with
>>>> + name = "btrfs_replace_start"; added = (1, 29, 46);
>>>> + style = RErr, [Device "srcdev"; Device "targetdev"; Pathname "mntpoint"], [OBool "force"];
>>>> + proc_nr = Some 455;
>>>> + optional = Some "btrfs"; camel_name = "BTRFSReplaceStart";
>>>> + test_excuse = "It is better to have 3+ test disk to do the test, so put the test in 'tests/btrfs' directory";
>>>> + shortdesc = "replace a btrfs managed device with another device";
>>>> + longdesc = "\
>>>> +Replace device of a btrfs filesystem. On a live filesystem, duplicate the data
>>>> +to the target device which is currently stored on the source device.
>>>> +After completion of the operation, the source device is wiped out and
>>>> +removed from the filesystem.
>>>> +
>>>> +The <targetdev> needs to be same size or larger than the <srcdev>. Devices
>>>> +which are currently mounted are never allowed to be used as the <targetdev>
>>>> +
>>>> +Option 'force=true' means using and overwriting <targetdev> even if
>>>> +it looks like containing a valid btrfs filesystem." };
>>>> +
>>>> ]
>>>>
>>>> (* Non-API meta-commands available only in guestfish.
>>>> diff --git a/tests/btrfs/test-btrfs-devices.sh b/tests/btrfs/test-btrfs-devices.sh
>>>> index 3935c60..463b0a8 100755
>>>> --- a/tests/btrfs/test-btrfs-devices.sh
>>>> +++ b/tests/btrfs/test-btrfs-devices.sh
>>>> @@ -66,6 +66,8 @@ btrfs-device-add "/dev/sdc1 /dev/sdd1" /
>>>> btrfs-device-delete "/dev/sdb1" /
>>>> btrfs-device-add "/dev/sdb1" /
>>>> btrfs-device-delete "/dev/sdc1 /dev/sdd1" /
>>>> +btrfs-replace-start "/dev/sda1" "/dev/sdd1" / force:true
>>>> +btrfs-replace-start "/dev/sdd1" "/dev/sda1" / force:true
>>>>
>>>> mkdir /data2
>>>> tar-in $srcdir/../data/filesanddirs-10M.tar.xz /data2 compress:xz
>>>> @@ -74,6 +76,8 @@ btrfs-device-add "/dev/sdc1 /dev/sdd1" /
>>>> btrfs-device-delete "/dev/sdb1" /
>>>> btrfs-device-add "/dev/sdb1" /
>>>> btrfs-device-delete "/dev/sdc1 /dev/sdd1" /
>>>> +btrfs-replace-start "/dev/sda1" "/dev/sdd1" / force:true
>>>> +btrfs-replace-start "/dev/sdd1" "/dev/sda1" / force:true
>>>>
>>>> mkdir /data3
>>>> tar-in $srcdir/../data/filesanddirs-10M.tar.xz /data3 compress:xz
>>>> @@ -82,6 +86,8 @@ btrfs-device-add "/dev/sdc1 /dev/sdd1" /
>>>> btrfs-device-delete "/dev/sdb1" /
>>>> btrfs-device-add "/dev/sdb1" /
>>>> btrfs-device-delete "/dev/sdc1 /dev/sdd1" /
>>>> +btrfs-replace-start "/dev/sda1" "/dev/sdd1" / force:true
>>>> +btrfs-replace-start "/dev/sdd1" "/dev/sda1" / force:true
>>>>
>>>> mkdir /data4
>>>> tar-in $srcdir/../data/filesanddirs-10M.tar.xz /data4 compress:xz
>>>> @@ -90,6 +96,8 @@ btrfs-device-add "/dev/sdc1 /dev/sdd1" /
>>>> btrfs-device-delete "/dev/sdb1" /
>>>> btrfs-device-add "/dev/sdb1" /
>>>> btrfs-device-delete "/dev/sdc1 /dev/sdd1" /
>>>> +btrfs-replace-start "/dev/sda1" "/dev/sdd1" / force:true
>>>> +btrfs-replace-start "/dev/sdd1" "/dev/sda1" / force:true
>>>
>>> What are these tests supposed to check? Other than calling
>>> btrfs-replace-start and checking it does not fail, how can the result
>>> of this operation be actually checked?
>>>
>>
>> These tests are used for test whether btrfs replace will success or not.
>> The existed add/delete test cases may also have the "problem" you
>> mentioned: don`t know how to actually check the result, like, is the
>> device really added/deleted in the btrfs? I have tested the api both in
>> guestfish and the test case script, in guestfish, it is easy to check
>> whether the device is replaced or not, just mount and check(of course,
>> the api worked). In test case, it is not convenient. But actually, in
>> test case, if btrfs-replace-start fails, the script will exit with
>> errors, I encountered this situation when debug this case.
>> Just "replace sda with sdd" is not enough for the test case, but after
>> adding "replace sdd with sda", I think it is pretty sure that the case
>> can actually check the result. Because even if 1st replace exit without
>> error but actually not replaced, the 2nd replace will exit with error.
>> So the 2nd replace add assurance. Also, I have ran the case successfully.
>
> This is nice description, but still we need *automated* test cases,
> otherwise checking that the API works is a nightmare.
>
Yes, the test cases should be automated.
Maybe there is a litter mistake here. what I want say is: the tests I
wrote, is automated, and the result is capable of checking the result
actually. I mentioned a little about how it can actually check the result.
As you know, the test script under tests/btrfs/ are automated, my tests
are added in the test-btrfs-devices.sh, so, my api test is automated.
And the rest we should concern is that, could the tests check the result
actually? Answer is: Yes. Here is the thing:
the tests I wrote:
A: btrfs-replace-start "/dev/sda1" "/dev/sdd1" / force:true
B: btrfs-replace-start "/dev/sdd1" "/dev/sda1" / force:true
When I use the api without option "-B"(the api won`t work with "-B"),
the tests can`t pass, will fail at B, because A actally didn`t work(but
don`t fail at A), so B will fail. I add B, just for the purpose that it
can actually check the api`s result.
I mentioned I have tested the api in guestfish, means that I am sure
this api can work
the tests beside mine are introduced by Rich(the file is created by
him), long time ago. I think my tests have the same test logic as him,
in the purpose that it could check the result actually.
> Actually, some part of this automated job was just described by you
> above: create test disks (maybe with some other filesystem?), do the
> replacement, check that there are btrfs filesystems where you expected
> them (see list_devices, list_partitions, list_filesystems, ...), and
> maybe mount the results in case you need to check the content as well.
>
--
Yours Sincerely,
Cao Jin
More information about the Libguestfs
mailing list