Re: [libvirt] [PATCH v7 16/23] backup: Implement virsh support for backup
On 3/27/19 7:30 AM, Peter Krempa wrote:
On Wed, Mar 27, 2019 at 05:10:47 -0500, Eric Blake wrote:
> Introduce a few more new virsh commands for performing backup jobs, as
> well as creating a checkpoint atomically with a snapshot.
>
> At this time, I did not opt for a convenience command
> 'backup-begin-as' that cobbles together appropriate XML from the
> user's command line arguments, but that may be a viable future
> extension. Similarly, since backup is a potentially long-running
> operation, it might be nice to add some sugar that automatically
> handles waiting for the job to end, rather than making the user have
> to poll or figure out virsh event to do the same.
>
> Signed-off-by: Eric Blake <eblake(a)redhat.com>
> ---
> tools/virsh-domain.c | 247 ++++++++++++++++++++++++++++++++++++++++-
> tools/virsh-snapshot.c | 37 +++++-
> tools/virsh.pod | 64 ++++++++++-
> 3 files changed, 337 insertions(+), 11 deletions(-)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 1970710c07..4ae456146b 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -1,7 +1,7 @@
> /*
> * virsh-domain.c: Commands to manage domain
> *
> - * Copyright (C) 2005, 2007-2016 Red Hat, Inc.
> + * Copyright (C) 2005-2019 Red Hat, Inc.
This seems wrong. It's adding dates in the past. Ideally you disable
that script for libvirt altogether as it creates pointless churn.
I'm always wary of not updating copyrights, but I also see the
complaints about possibly updating it incorrectly. Unless a lawyer gives
me a strong reason behind one way or the other, it's obvious enough that
many files have NOT been maintained after initial commit, so I can go
ahead and flip the kill switch in my .emacs file for auto-copyrights to
maintain status quo. I also don't mind scrubbing my series to remove the
churn I've already caused, if that makes you feel better (but for files
that my series DOES add, I will be listing dates starting from any file
I copied from, through 2019).
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public
[1] Using 'check' instead of 'checkpoint' in variable names may be
misleading below in multiple instances. It might evoke that it's
supposed to be checked rather than referring to a checkpoint.
Fitting in 80 columns is harder with 'checkpoint', but I can do the
rename for legibility.
> +
> +static bool
> +cmdBackupDumpXML(vshControl *ctl,
> + const vshCmd *cmd)
> +{
> + virDomainPtr dom = NULL;
> + bool ret = false;
> + char *xml = NULL;
> + unsigned int flags = 0;
> + int id = 0;
> +
> + if (vshCommandOptBool(cmd, "security-info"))
This option is not mentioned in opts_backup_dumpxml.
Rebase leftovers; as there is no VIR_DOMAIN_BACKUP_XML_SECURE, this flag
should be removed from the code.
> + flags |= VIR_DOMAIN_XML_SECURE;
And this flags setting is bogus.
> unsigned int flags = 0;
> + VIR_AUTOFREE(char *check_buffer) = NULL;
VIR_AUTOFREE and legacy code is used inconsistently in this patch.
I can try to use the new stuff in more places (new code introduction is
a good time; but this is also copy-and-paste from existing code; there's
also the issue of timing delays, where the longer my patches are
out-of-tree, the more the point they copied from has changed in the
meantime).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 488 bytes)