[libvirt] [libvirt-glib] [PATCH 3/4] GVirDomainSnapshot: Add _set_config

[Timm Bäder]

On 05.08, Christophe Fergeau wrote:
> On Sat, Aug 02, 2014 at 10:41:48AM +0200, mail at baedert.org wrote:
> > From: Timm Bäder <mail at baedert.org>
> > 
> > ... which is basically analogous to gvir_domain_set_config
> > ---
> >  libvirt-gobject/libvirt-gobject-domain-snapshot.c | 58 +++++++++++++++++++++++
> >  libvirt-gobject/libvirt-gobject-domain-snapshot.h |  5 ++
> >  libvirt-gobject/libvirt-gobject.sym               |  1 +
> >  3 files changed, 64 insertions(+)
> > 
> > diff --git a/libvirt-gobject/libvirt-gobject-domain-snapshot.c b/libvirt-gobject/libvirt-gobject-domain-snapshot.c
> > index 497288f..2c81882 100644
> > --- a/libvirt-gobject/libvirt-gobject-domain-snapshot.c
> > +++ b/libvirt-gobject/libvirt-gobject-domain-snapshot.c
> > @@ -297,3 +297,61 @@ gboolean gvir_domain_snapshot_revert_to(GVirDomainSnapshot *snapshot,
> >  
> >      return TRUE;
> >  }
> > +
> > +
> > +
> > +/**
> > + * gvir_domain_snapshot_set_config:
> > + * @snapshot: The domain snapshot
> > + * @conf: The new config object
> > + * @error: (allow-none): Place-holder for error or NULL
> > + *
> > + * Updates the given snapshot's configuration according to the
> > + * given GVirConfigDomainSnapshot.
> > + *
> > + * Returns: TRUE if no error was reported, FALSE otherwise.
> > + */
> > +gboolean gvir_domain_snapshot_set_config(GVirDomainSnapshot *snapshot,
> > +                                         GVirConfigDomainSnapshot *conf,
> > +                                         GError **error)
> > +{
> > +    gchar *xml;
> > +    virConnectPtr conn;
> > +    virDomainSnapshotPtr handle;
> > +    virDomainPtr domain;
> > +    GVirDomainSnapshotPrivate *priv;
> > +
> > +    g_return_val_if_fail(GVIR_IS_DOMAIN_SNAPSHOT(snapshot), FALSE);
> > +    g_return_val_if_fail(GVIR_CONFIG_IS_DOMAIN_SNAPSHOT(conf), FALSE);
> > +    g_return_val_if_fail(error == NULL || *error == NULL, FALSE);
> > +
> > +    priv = snapshot->priv;
> > +    handle = priv->handle;
> > +    domain = virDomainSnapshotGetDomain(handle);
> > +
> > +
> > +    if ((conn = virDomainSnapshotGetConnect(priv->handle)) == NULL) {
> > +        gvir_set_error_literal(error, GVIR_DOMAIN_SNAPSHOT_ERROR,
> > +                               0,
> > +                               "Failed to get domain connection");
> > +        return FALSE;
> > +    }
> > +
> > +
> > +    xml = gvir_config_object_to_xml(GVIR_CONFIG_OBJECT(conf));
> > +
> > +    handle = virDomainSnapshotCreateXML(domain,
> > +                                        xml,
> > +                                        VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE);
> > +    free(xml);
> 
> g_free here.
> 
> > +
> > +    if (handle == NULL) {
> > +        gvir_set_error(error, GVIR_DOMAIN_SNAPSHOT_ERROR,
> > +                       0,
> > +                       "Failed to create Snapshot `%s' from XML definition",
> 
> "snapshot" could have a lower case here.
> 
> I'm not exactly clear on what this method will be doing according to
> your 0/4. If it creates a new snapshot rather than modifying an
> existing one, maybe the name should be different/this should not be
> wrapped?
> 
> Christophe

Also in reply to the 0/4:
It will modify the existing one, as long as you don't change the name.
Like if you use get_config, then set_description on that config and
then _set_config again, no additional snapshot will be created.
If you change only the snapshot's name and then use set_config on it,
it'll create a new snapshot that is equivalent to the snapshot you
wanted to modify except for the name (and to really "modify" the old
snapshot, you now have to delete the old one, which is one reason why
we show the description to the user instead of the name in gnome-boxes).
I had a conversation with eblake on IRC about this and it seemed like
this is the wanted behavior (or at least it's a well-known limitation?).

So I guess the name is still correct, but maybe the docs should mention
that renaming won't work as expected?


Regards,
Timm