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@xxxxxxxxxx>
>> ---
>> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
