Hi, On Wednesday 10 June 2015 17:54:18 Pino Tsao wrote: > Signed-off-by: Pino Tsao <caoj.fnst(a)cn.fujitsu.com&gt; > --- > 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?

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

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

Thanks,

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&gt; >>> --- >>> 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.

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

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

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.

Hi, On Friday 12 June 2015 19:54:26 Cao Jin wrote: >>>>> 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 While I appreciate the effort you are putting in writing and testing the new API, and the explanation about the added calls in checks, in my opinion the added tests are not (in they way they are added) useful. The problem is that if I, for example, make the btrfs_replace_start implementation a no-op (for example by having just "return 0" in it), the test would keep working as before, as the status-quo before and after each block of btrfs-replace-start calls is the same, and nobody would notice if not by running it. Sure, making it a no-op is a stretch, sure, but image if `btrfs replace` would suddenly do nothing: the result would be the same as above. Currently, the only thing checked is that the btrfs_replace_start calls don't fail, but they tell me nothing about whether each of them did the right thing.

That's why I would rather prefer a separate test for btrfs_replace_start, either a new guestfish calls in test-btrfs-devices.sh (using the disks created there already) or as a separate new bash/perl test altogether.