[libvirt] [PATCH v7 13/23] backup: Implement backup APIs for remote driver

[Daniel P. Berrangé]

On Wed, Mar 27, 2019 at 07:06:32AM -0500, Eric Blake wrote:
> On 3/27/19 6:36 AM, Daniel P. Berrangé wrote:
> > On Wed, Mar 27, 2019 at 05:10:44AM -0500, Eric Blake wrote:
> >> The remote code generator had to be taught about the new
> >> virDomainCheckpointPtr type, and about the capitalization of
> >> virDomainSnapshotCreateXML2(), at which point the remote driver code
> >> for backups can be generated.
> >>
> >> Signed-off-by: Eric Blake <eblake at redhat.com>
> >> ---
> >>  src/remote/remote_daemon_dispatch.c |  22 ++-
> >>  src/remote/remote_driver.c          |  34 +++-
> >>  src/remote/remote_protocol.x        | 259 +++++++++++++++++++++++++++-
> >>  src/remote_protocol-structs         | 139 +++++++++++++++
> >>  src/rpc/gendispatch.pl              |  34 ++--
> >>  5 files changed, 468 insertions(+), 20 deletions(-)
> > 
> > 
> >> +struct remote_domain_backup_begin_args {
> >> +    remote_nonnull_domain dom;
> >> +    remote_string disk_xml;
> >> +    remote_string checkpoint_xml;
> >> +    unsigned int flags;
> >> +};
> >> +
> >> +struct remote_domain_backup_begin_ret {
> >> +    int result;
> > 
> > Feels like this should be 'id', since IIUC it is returning a job
> > ID.
> 
> Can swap (generator should handle it just fine)
> 
> 
> >> +};
> > 
> > With that one rename above
> > 
> > Reviewed-by: Daniel P. Berrangé <berrange at redhat.com>
> 
> This was the patch where I had a question on ACL semantics:
> 
> +     * @generate: both
> +     * @acl: domain:snapshot
> +     * @acl: domain:fs_freeze:VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE
> +     */
> +    REMOTE_PROC_DOMAIN_SNAPSHOT_CREATE_XML2 = 418
> 
> Is there an easy way to make this also do a check against @acl:
> domain:checkpoint, but only when the checkpointXml flag is non-NULL? The
> quickest I could think of would be automatically setting a
> VIR_DOMAIN_SNAPSHOT_CREATE_CHECKPOINT flag that
> libvirt-domain-snapshot.c automatically sets if checkpointXml is clear,
> so that I could then write @acl:
> domain:checkpoint:VIR_DOMAIN_SNAPSHOT_CREATE_CHECKPOINT. We've set
> witness flags in other situations (such as VIR_TYPED_PARAM_STRING_OKAY),
> but never for the purpose of an ACL check. On the other hand, tweaking
> the ACL code generator to take more than just 'flags' for doing checks
> seems like a lot of work for very little gain.

Yeha, it would require us to add more magic to the ACL generator.

I'm not saying that's wrong, just that is requires some non-neglible
work.

IMHO it is fine for the ACL checks to be paranoid be apply too strong
a check. We can always relax it later if we get demand (unlikely)

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|