Re: [Libguestfs] [PATCH] New API: btrfs_replace_start

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

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

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