Re: [libvirt] [PATCH v3 11/18] blockcopy: split out virsh implementation

On 09/05/14 11:29, Peter Krempa wrote: > On 08/31/14 06:02, Eric Blake wrote: >> I'm about to extend the capabilities of blockcopy. Hiding a few >> common lines of implementation gets in the way of the new required >> logic, and putting the new logic in the common implementation won't >> benefit any of the other blockjob operations. Therefore, it is >> simpler to just do the work inline. There should be no semantic >> change in this patch. >> >> * tools/virsh-domain.c (blockJobImpl): Drop unused variable. Move >> block copy guts... >> (cmdBlockCopy): ...into their lone caller. >> >> Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; >> --- >> tools/virsh-domain.c | 45 ++++++++++++++++++++++++++------------------- >> 1 file changed, 26 insertions(+), 19 deletions(-) >> >> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c >> index 813d8e0..b92b2eb 100644 >> --- a/tools/virsh-domain.c >> +++ b/tools/virsh-domain.c > > >> @@ -1907,6 +1894,9 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) >> bool quit = false; >> int abort_flags = 0; >> >> + if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) >> + return false; >> + >> blocking |= vshCommandOptBool(cmd, "timeout") || pivot || finish; >> if (blocking) { >> if (pivot && finish) { > > ... > >> @@ -1935,7 +1923,26 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) >> return false; >> } >> >> - if (!blockJobImpl(ctl, cmd, VSH_CMD_BLOCK_JOB_COPY, &dom)) >> + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) >> + goto cleanup; >> + >> + if (vshCommandOptULWrap(cmd, "bandwidth", &bandwidth) < 0) { >> + vshError(ctl, "%s", _("bandwidth must be a number")); >> + goto cleanup; >> + } >> + >> + if (vshCommandOptBool(cmd, "shallow")) >> + flags |= VIR_DOMAIN_BLOCK_REBASE_SHALLOW; >> + if (vshCommandOptBool(cmd, "reuse-external")) >> + flags |= VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT; >> + if (vshCommandOptBool(cmd, "raw")) >> + flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_RAW; >> + if (vshCommandOptBool(cmd, "blockdev")) >> + flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_DEV; >> + if (vshCommandOptStringReq(ctl, cmd, "dest", &dest) < 0) >> + goto cleanup; >> + > > You could extract the flags and string at the top where you extract > @path to be consistent, but that's more bikeshedding than useful. > Hmmm, next patch is moving it to the destination I've suggested. It'd be better to move it to the correct position right away then ...