On Tue, Nov 22, 2011 at 02:07:23PM +0100, Christophe Fergeau wrote:
> Hey,
>
> On Tue, Nov 22, 2011 at 12:39:31PM +0000, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
> >
> > ---
> > libvirt-gobject/libvirt-gobject-connection.c | 49 ++++++++++++++++++++++++++
> > libvirt-gobject/libvirt-gobject-connection.h | 4 ++
>
> No addition of the new function in libvirt-gobject.sym?
>
> > 2 files changed, 53 insertions(+), 0 deletions(-)
> >
> > diff --git a/libvirt-gobject/libvirt-gobject-connection.c b/libvirt-gobject/libvirt-gobject-connection.c
> > index f52b825..b6e7f3b 100644
> > --- a/libvirt-gobject/libvirt-gobject-connection.c
> > +++ b/libvirt-gobject/libvirt-gobject-connection.c
> > @@ -1164,6 +1164,10 @@ GVirStream *gvir_connection_get_stream(GVirConnection *self,
> > * gvir_connection_create_domain:
> > * @conn: the connection on which to create the dmain
>
> domain
>
> > * @conf: the configuration for the new domain
> > + *
> > + * Create the configuration file for a new persistent domain.
> > + * The returned domain will initially be in the shutoff state.
> > + *
> > * Returns: (transfer full): the newly created domain
> > */
> > GVirDomain *gvir_connection_create_domain(GVirConnection *conn,
> > @@ -1201,6 +1205,51 @@ GVirDomain *gvir_connection_create_domain(GVirConnection *conn,
> > }
> >
> > /**
> > + * gvir_connection_start_domain:
> > + * @conn: the connection on which to create the dmain
>
> Same here, domain
Cut + paste typos :-)
>
> > + * @conf: the configuration for the new domain
> > + *
> > + * Start a new transient domain without persistent configuration.
> > + * The returned domain will initially be running.
> > + *
> > + * Returns: (transfer full): the newly created domain
> > + */
> > +GVirDomain *gvir_connection_start_domain(GVirConnection *conn,
> > + GVirConfigDomain *conf,
> > + guint flags,
> > + GError **err)
> > +{
> > + const gchar *xml;
> > + virDomainPtr handle;
> > + GVirConnectionPrivate *priv = conn->priv;
> > +
> > + xml = gvir_config_object_to_xml(GVIR_CONFIG_OBJECT(conf));
>
> xml shouldn't be const and need to be g_freed when it's no longer needed.
Oooh, gvir_connection_create_domain has the same flaw. I'll fix
both.
> > +
> > + g_return_val_if_fail(xml != NULL, NULL);
> > +
> > + if (!(handle = virDomainCreateXML(priv->conn, xml, flags))) {
> > + *err = gvir_error_new_literal(GVIR_CONNECTION_ERROR,
> > + 0,
> > + "Failed to create domain");
> > + return NULL;
> > + }
> > +
> > + GVirDomain *domain;
> > +
> > + domain = GVIR_DOMAIN(g_object_new(GVIR_TYPE_DOMAIN,
> > + "handle", handle,
> > + NULL));
> > +
> > + g_mutex_lock(priv->lock);
> > + g_hash_table_insert(priv->domains,
> > + (gpointer)gvir_domain_get_uuid(domain),
> > + domain);
> > + g_mutex_unlock(priv->lock);
> > +
> > + return g_object_ref(domain);
>
> Here I'd do
> g_mutex_lock(priv->lock);
> g_hash_table_insert(priv->domains,
> (gpointer)gvir_domain_get_uuid(domain),
> g_object_ref(domain));
> g_mutex_unlock(priv->lock);
>
> return domain;
>
> to make it clearer that the hash table needs a ref on the object. The way
> it's written, I had to check how priv->domains is declared to make sure
> that wasn't an extra _ref.
Again this was a cut+past from gvir_connection_start_domain, so
I'll change both to the style you describe.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list