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 <[email protected]> >> --- >> 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
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
