On Thu, May 02, 2013 at 06:33:44PM +0300, Zeeshan Ali (Khattak) wrote:
> On Wed, May 1, 2013 at 10:59 PM, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote:
> > +
> > +/*
> > +const char *gvir_config_domain_snapshot_get_memory(GVirConfigDomainSnapshot *snapshot)
>
>
> Why commented-out? AFAICT from the docs, this deserves a separate object.
Ah, I knew I still had something to fix somewhere before sending this
series, I just couldn't remember what, thanks!
However, I'm not sure I'll go with a separate object, the doc says:
"memory
On input, this is an optional request for how to handle VM memory
state. For an offline domain or a disk-only snapshot, attribute snapshot
must be no, since there is no VM state saved; otherwise, the attribute can
be internal if the memory state is piggy-backed with other internal disk
state, or external along with a second attribute file giving the absolute
path of the file holding the VM memory state. Since 1.0.1"
This would mean GVirConfigDomainSnapshotMemoryNone,
GVirConfigDomainSnapshotMemoryInternal and
GVirConfigDomainSnapshotMemoryExternal classes so that we can expose the
file attribute only on the last of these classes. The MemoryNone class
seems a bit weird to me, and 3 classes to make an attribute optional sounds
a bit overkill.
I think I'll go with
gvir_config_domain_snapshot_get_memory();
and
gvir_config_domain_snapshot_get_memory_file();
which will return NULL unless the snapshot type is external.
> > +
> > +/* FIXME: GDateTime is only available since glib 2.26, libvirt-glib depends
> > + * on glib 2.22, is the dep ok?
> > + */
>
> time_t seems pretty appropriate here.
Ok!
> > +GDateTime *gvir_config_domain_snapshot_get_creation_time(GVirConfigDomainSnapshot *snapshot)
> > +{
> > + g_return_val_if_fail(GVIR_CONFIG_IS_DOMAIN_SNAPSHOT(snapshot), NULL);
> > + guint64 creation_time;
> > +
> > + creation_time = gvir_config_object_get_node_content_uint64(GVIR_CONFIG_OBJECT(snapshot),
> > + "creationTime");
> > + return g_date_time_new_from_unix_utc(creation_time);
> > +}
> > +
> > +
> > +GVirConfigDomainSnapshotState gvir_config_domain_snapshot_get_state(GVirConfigDomainSnapshot *snapshot)
> > +{
> > + g_return_val_if_fail(GVIR_CONFIG_IS_DOMAIN_SNAPSHOT(snapshot),
> > + GVIR_CONFIG_DOMAIN_SNAPSHOT_STATE_NOSTATE);
> > +
> > + return gvir_config_object_get_node_content_genum(GVIR_CONFIG_OBJECT(snapshot),
> > + "state",
> > + GVIR_CONFIG_TYPE_DOMAIN_SNAPSHOT_STATE,
> > + GVIR_CONFIG_DOMAIN_SNAPSHOT_STATE_NOSTATE);
> > +}
> > +
> > +
> > +const char *gvir_config_domain_snapshot_get_parent(GVirConfigDomainSnapshot *snapshot)
> > +{
>
> Shouldn't this be returning a GVirConfigDomainSnapshot instance?
Hmm, that's a good point, this is made a bit tricky because of the "Older
versions of libvirt stored only a single child element, uuid". This was
changed in 2011 for libvirt 0.9.5 by
http://libvirt.org/git/?p=libvirt.git;a=commit;h=f609cb85ca11d6d178972e46ebeb9985bf4ccfaf
Given that we already require libvirt 0.10.2, I'd say it's fine to ignore
this 'older version' case.
Christophe
Attachment:
pgpO4qGHU0y0j.pgp
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list