6:54 a.m.
在 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(a)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