[Libguestfs] [PATCH] New API: btrfs_replace_start
Pino Toscano
ptoscano at redhat.com
Fri Jun 12 09:12:52 UTC 2015
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.
>
> >> +
> >> + 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.
--
Pino Toscano
More information about the Libguestfs
mailing list