Re: [libvirt] [libvirt-glib] Add API to redefine an existing domain

2011-11-22 Thread Daniel P. Berrange 
On Mon, Nov 21, 2011 at 08:27:17PM +0100, Marc-André Lureau wrote:
 Hi
 
 On Mon, Nov 21, 2011 at 6:53 PM, Zeeshan Ali (Khattak)
 zeesha...@gnome.org wrote:
  +    g_return_val_if_fail(error == NULL || *error == NULL, FALSE);
 
 This is wrong, it should be error != NULL  *error == NULL.
 
  +    if (virDomainDefineXML(conn, xml) == NULL) {
  +        if (error != NULL)
  +            *error = gvir_error_new_literal(GVIR_DOMAIN_ERROR,
  +                                            0,
  +                                            Failed to set 
  +                                            domain configuration);
  +        return FALSE;
  +   }
 
 Can you please verify that the return value is safe to ignore?
 
 I would really prefer we add a runtime check that verifiy the handle
 is the same as the one currently associated with the domain.

The actual pointer returned will *not* be the same as the same
one you currently have, but assuming the XML has matching UUID
and name, it will point to the same domain.

So I guess what you want todo is verify the UUID and name in
the XML, before defining it.

Also, you need to call virDomainFree on the return value of
virDomainDefineXML to avoid memleak.

Regards,
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt-glib] Add API to redefine an existing domain

2011-11-22 Thread Zeeshan Ali (Khattak) 
On Tue, Nov 22, 2011 at 11:20 AM, Daniel P. Berrange
berra...@redhat.com wrote:
 On Mon, Nov 21, 2011 at 08:27:17PM +0100, Marc-André Lureau wrote:
 Hi

 On Mon, Nov 21, 2011 at 6:53 PM, Zeeshan Ali (Khattak)
 zeesha...@gnome.org wrote:
  +    g_return_val_if_fail(error == NULL || *error == NULL, FALSE);

 This is wrong, it should be error != NULL  *error == NULL.

  +    if (virDomainDefineXML(conn, xml) == NULL) {
  +        if (error != NULL)
  +            *error = gvir_error_new_literal(GVIR_DOMAIN_ERROR,
  +                                            0,
  +                                            Failed to set 
  +                                            domain configuration);
  +        return FALSE;
  +   }

 Can you please verify that the return value is safe to ignore?

 I would really prefer we add a runtime check that verifiy the handle
 is the same as the one currently associated with the domain.

 The actual pointer returned will *not* be the same as the same
 one you currently have, but assuming the XML has matching UUID
 and name, it will point to the same domain.

 So I guess what you want todo is verify the UUID and name in
 the XML, before defining it.

 Also, you need to call virDomainFree on the return value of
 virDomainDefineXML to avoid memleak.

   Thanks! I already pushed the patch that does this and Marc-Andre ack'ed.

-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt-glib] Add API to redefine an existing domain

2011-11-21 Thread Marc-André Lureau 
Hi
Hi

Agreed with the comments from teuf +

On Fri, Nov 18, 2011 at 8:24 PM, Zeeshan Ali (Khattak)
zeesha...@gnome.org wrote:
 +void gvir_connection_redefine_domain(GVirConnection *conn,
 +                                     GVirDomain *domain,
 +                                     GVirConfigDomain *conf,
 +                                     GError **err)
 +{

I would check that the given arguments are correct, != NULL or GVIR_IS_FOO.

I would return TRUE on success, and error can be NULL (regular glib convention)

 +    const gchar *xml;
 +    virDomainPtr handle;
 +    GVirDomain *dom;
 +    virDomainPtr dom_handle;
 +    GVirConnectionPrivate *priv = conn-priv;
 +
 +    xml = gvir_config_object_to_xml(GVIR_CONFIG_OBJECT(conf));
 +
 +    g_return_if_fail(xml != NULL);
 +
 +    g_mutex_lock(priv-lock);
 +    dom = g_hash_table_lookup (priv-domains,
 +                               (gpointer)gvir_domain_get_uuid(domain));
 +    g_mutex_unlock(priv-lock);
 +    g_return_if_fail(dom != NULL);
 +    /* FIXME: Check if config's domain ID is the same as domain passed */

I suppose this is missing Christophe gconfig patches.

 +    if (!(handle = virDomainDefineXML(priv-conn, xml))) {
 +        *err = gvir_error_new_literal(GVIR_CONNECTION_ERROR,
 +                                      0,
 +                                      Failed to redefine domain);
 +        return NULL;
 +    }
 +}

And I would verify that handle is == to current domain handle. If not,
we probably need to replace it or we need to throw an error. At the
minimum it would be nice to leave a comment after verifying that it is
safe to ignore return value.

(then send an updated patch for ack)

regards

-- 
Marc-André Lureau

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt-glib] Add API to redefine an existing domain

2011-11-21 Thread Marc-André Lureau 
Hi

On Mon, Nov 21, 2011 at 6:53 PM, Zeeshan Ali (Khattak)
zeesha...@gnome.org wrote:
 +    g_return_val_if_fail(error == NULL || *error == NULL, FALSE);

This is wrong, it should be error != NULL  *error == NULL.

 +    if (virDomainDefineXML(conn, xml) == NULL) {
 +        if (error != NULL)
 +            *error = gvir_error_new_literal(GVIR_DOMAIN_ERROR,
 +                                            0,
 +                                            Failed to set 
 +                                            domain configuration);
 +        return FALSE;
 +   }

Can you please verify that the return value is safe to ignore?

I would really prefer we add a runtime check that verifiy the handle
is the same as the one currently associated with the domain.

-- 
Marc-André Lureau

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt-glib] Add API to redefine an existing domain

2011-11-21 Thread Zeeshan Ali (Khattak) 
On Mon, Nov 21, 2011 at 9:27 PM, Marc-André Lureau
marcandre.lur...@gmail.com wrote:
 Hi

 On Mon, Nov 21, 2011 at 6:53 PM, Zeeshan Ali (Khattak)
 zeesha...@gnome.org wrote:
 +    g_return_val_if_fail(error == NULL || *error == NULL, FALSE);

 This is wrong, it should be error != NULL  *error == NULL.

 +    if (virDomainDefineXML(conn, xml) == NULL) {
 +        if (error != NULL)
 +            *error = gvir_error_new_literal(GVIR_DOMAIN_ERROR,
 +                                            0,
 +                                            Failed to set 
 +                                            domain configuration);
 +        return FALSE;
 +   }

 Can you please verify that the return value is safe to ignore?

  Sorry, i don't get it. Verify how?

 I would really prefer we add a runtime check that verifiy the handle
 is the same as the one currently associated with the domain.

  Yeah, i'll add that! Forgot that it still applies.

-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt-glib] Add API to redefine an existing domain

2011-11-21 Thread Marc-André Lureau 
On Mon, Nov 21, 2011 at 8:45 PM, Zeeshan Ali (Khattak)
zeesha...@gnome.org wrote:
 Sorry, i don't get it. Verify how?

By doing what I proposed bellow, reading libvirt code, ask on libvirt
list/maintainers, test etc..

 I would really prefer we add a runtime check that verifiy the handle
 is the same as the one currently associated with the domain.

  Yeah, i'll add that! Forgot that it still applies.

thanks

-- 
Marc-André Lureau

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt-glib] Add API to redefine an existing domain

2011-11-21 Thread Zeeshan Ali (Khattak) 
On Mon, Nov 21, 2011 at 9:58 PM, Marc-André Lureau
marcandre.lur...@gmail.com wrote:
 On Mon, Nov 21, 2011 at 8:45 PM, Zeeshan Ali (Khattak)
 zeesha...@gnome.org wrote:
 Sorry, i don't get it. Verify how?

 By doing what I proposed bellow, reading libvirt code, ask on libvirt
 list/maintainers, test etc..

  Ah that, yeah i already did that. Patching coming..

-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt-glib] Add API to redefine an existing domain

2011-11-21 Thread Marc-André Lureau 
On Mon, Nov 21, 2011 at 9:02 PM, Zeeshan Ali (Khattak)
zeesha...@gnome.org wrote:
 +    g_return_val_if_fail(err == NULL || *err == NULL, FALSE);

this needs update, as I wrote before.

 +    xml = gvir_config_object_to_xml(GVIR_CONFIG_OBJECT(conf));

hmm, don't we need to free xml?

 +    virDomainGetUUIDString(priv-handle, uuid);
 +
 +    if (strcmp (uuid, priv-uuid) != 0) {
 +        if (err != NULL)
 +            *err = gvir_error_new_literal(GVIR_DOMAIN_ERROR,

I prefer g_strcmp0, because it's safer.

But the main issue is that you don't say clearly if we can safely
ignore that new handle, at the very least if there is no leakage. A
comment in the code would be welcome.

regards

-- 
Marc-André Lureau

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt-glib] Add API to redefine an existing domain

2011-11-21 Thread Zeeshan Ali (Khattak) 
On Mon, Nov 21, 2011 at 10:12 PM, Marc-André Lureau
marcandre.lur...@gmail.com wrote:
 On Mon, Nov 21, 2011 at 9:02 PM, Zeeshan Ali (Khattak)
 zeesha...@gnome.org wrote:
 +    g_return_val_if_fail(err == NULL || *err == NULL, FALSE);

  As I explained on IRC, it is justified AFAICT. Either you pass a
'NULL' as the error arg or you pass an uninitialized place-holder for
error.

  Fixed other issues you pointed to.

-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt-glib] Add API to redefine an existing domain

2011-11-21 Thread Marc-André Lureau 
On Mon, Nov 21, 2011 at 10:09 PM, Zeeshan Ali (Khattak)
zeesha...@gnome.org wrote:
 As I explained on IRC, it is justified AFAICT. Either you pass a
 'NULL' as the error arg or you pass an uninitialized place-holder for
 error.

right, sorry for the confusion


-- 
Marc-André Lureau

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt-glib] Add API to redefine an existing domain

2011-11-21 Thread Marc-André Lureau 
ack

On Mon, Nov 21, 2011 at 10:10 PM, Zeeshan Ali (Khattak)
zeesha...@gnome.org wrote:
 From: Zeeshan Ali (Khattak) zeesha...@gnome.org

 ---
  libvirt-gobject/libvirt-gobject-domain.c |   68 
 ++
  libvirt-gobject/libvirt-gobject-domain.h |    3 +
  libvirt-gobject/libvirt-gobject.sym      |    1 +
  3 files changed, 72 insertions(+), 0 deletions(-)

 diff --git a/libvirt-gobject/libvirt-gobject-domain.c 
 b/libvirt-gobject/libvirt-gobject-domain.c
 index 1fa27bd..7ff820b 100644
 --- a/libvirt-gobject/libvirt-gobject-domain.c
 +++ b/libvirt-gobject/libvirt-gobject-domain.c
 @@ -449,6 +449,74 @@ GVirConfigDomain *gvir_domain_get_config(GVirDomain *dom,
     return conf;
  }

 +/**
 + * gvir_domain_set_config:
 + * @domain: the domain
 + * @conf: the new configuration for the domain
 + * @err: (allow-none): Place-holder for error or NULL
 + *
 + * Resets configuration of an existing domain.
 + *
 + * Note: If domain is already running, the new configuration will not take
 + * affect until domain reboots.
 + *
 + * Returns: TRUE on success, FALSE if an error occurred.
 + */
 +gboolean gvir_domain_set_config(GVirDomain *domain,
 +                                GVirConfigDomain *conf,
 +                                GError **err)
 +{
 +    gchar *xml;
 +    virConnectPtr conn;
 +    virDomainPtr handle;
 +    gchar uuid[VIR_UUID_STRING_BUFLEN];
 +    GVirDomainPrivate *priv = domain-priv;
 +
 +    g_return_val_if_fail(GVIR_IS_DOMAIN (domain), FALSE);
 +    g_return_val_if_fail(GVIR_IS_CONFIG_DOMAIN (conf), FALSE);
 +    g_return_val_if_fail(err == NULL || *err == NULL, FALSE);
 +
 +    xml = gvir_config_object_to_xml(GVIR_CONFIG_OBJECT(conf));
 +
 +    g_return_val_if_fail(xml != NULL, FALSE);
 +
 +    if ((conn = virDomainGetConnect(priv-handle)) == NULL) {
 +        if (err != NULL)
 +            *err = gvir_error_new_literal(GVIR_DOMAIN_ERROR,
 +                                          0,
 +                                          Failed to get domain connection);
 +        g_free (xml);
 +
 +        return FALSE;
 +    }
 +
 +    handle = virDomainDefineXML(conn, xml);
 +    g_free (xml);
 +
 +    if (handle == NULL) {
 +        if (err != NULL)
 +            *err = gvir_error_new_literal(GVIR_DOMAIN_ERROR,
 +                                          0,
 +                                          Failed to set 
 +                                          domain configuration);
 +        return FALSE;
 +    }
 +
 +    virDomainGetUUIDString(handle, uuid);
 +    virDomainFree(handle);
 +
 +    if (g_strcmp0 (uuid, priv-uuid) != 0) {
 +        if (err != NULL)
 +            *err = gvir_error_new_literal(GVIR_DOMAIN_ERROR,
 +                                          0,
 +                                          Failed to set 
 +                                          domain configuration);
 +
 +        return FALSE;
 +    }
 +
 +    return TRUE;
 +}

  /**
  * gvir_domain_get_info:
 diff --git a/libvirt-gobject/libvirt-gobject-domain.h 
 b/libvirt-gobject/libvirt-gobject-domain.h
 index 94bd53e..0479de8 100644
 --- a/libvirt-gobject/libvirt-gobject-domain.h
 +++ b/libvirt-gobject/libvirt-gobject-domain.h
 @@ -123,6 +123,9 @@ GVirDomainInfo *gvir_domain_get_info(GVirDomain *dom,
  GVirConfigDomain *gvir_domain_get_config(GVirDomain *dom,
                                          guint64 flags,
                                          GError **err);
 +gboolean gvir_domain_set_config(GVirDomain *domain,
 +                                GVirConfigDomain *conf,
 +                                GError **err);

  gchar *gvir_domain_screenshot(GVirDomain *dom,
                               GVirStream *stream,
 diff --git a/libvirt-gobject/libvirt-gobject.sym 
 b/libvirt-gobject/libvirt-gobject.sym
 index 164b6b8..46c53f9 100644
 --- a/libvirt-gobject/libvirt-gobject.sym
 +++ b/libvirt-gobject/libvirt-gobject.sym
 @@ -53,6 +53,7 @@ LIBVIRT_GOBJECT_0.0.1 {
        gvir_domain_shutdown;
        gvir_domain_reboot;
        gvir_domain_get_config;
 +       gvir_domain_set_config;
        gvir_domain_get_info;
        gvir_domain_screenshot;

 --
 1.7.7.1

 --
 libvir-list mailing list
 libvir-list@redhat.com
 https://www.redhat.com/mailman/listinfo/libvir-list




-- 
Marc-André Lureau

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt-glib] Add API to redefine an existing domain

2011-11-18 Thread Christophe Fergeau 
Hey,

Reading this patch, I'm wondering if gvir_domain_set_config might achieve
what you are after (it's not implemented either)

On Fri, Nov 18, 2011 at 09:24:32PM +0200, Zeeshan Ali (Khattak) wrote:
 From: Zeeshan Ali (Khattak) zeesha...@gnome.org
 
 ---
  libvirt-gobject/libvirt-gobject-connection.c |   37 
 ++
  libvirt-gobject/libvirt-gobject-connection.h |4 +++
  libvirt-gobject/libvirt-gobject.sym  |1 +
  3 files changed, 42 insertions(+), 0 deletions(-)
 
 diff --git a/libvirt-gobject/libvirt-gobject-connection.c 
 b/libvirt-gobject/libvirt-gobject-connection.c
 index 6c8de11..471c795 100644
 --- a/libvirt-gobject/libvirt-gobject-connection.c
 +++ b/libvirt-gobject/libvirt-gobject-connection.c
 @@ -1201,6 +1201,43 @@ GVirDomain 
 *gvir_connection_create_domain(GVirConnection *conn,
  }
  
  /**
 + * gvir_connection_redefine_domain:
 + * @conn: the connection on which the dmain exists
 + * @conf: the new configuration for the domain
 + *
 + * Redefines an existing domain.
 + */
 +void gvir_connection_redefine_domain(GVirConnection *conn,
 + GVirDomain *domain,
 + GVirConfigDomain *conf,
 + GError **err)

For the record, I'm not sure I like the naming here.

 +{
 +const gchar *xml;
 +virDomainPtr handle;
 +GVirDomain *dom;
 +virDomainPtr dom_handle;

This is unused

 +GVirConnectionPrivate *priv = conn-priv;
 +
 +xml = gvir_config_object_to_xml(GVIR_CONFIG_OBJECT(conf));
 +
 +g_return_if_fail(xml != NULL);
 +
 +g_mutex_lock(priv-lock);
 +dom = g_hash_table_lookup (priv-domains,
 +   (gpointer)gvir_domain_get_uuid(domain));
 +g_mutex_unlock(priv-lock);
 +g_return_if_fail(dom != NULL);
 +/* FIXME: Check if config's domain ID is the same as domain passed */
 +
 +if (!(handle = virDomainDefineXML(priv-conn, xml))) {

Shouldn't handle be assigned to some persistent state? My guess is
domain-priv-handle.

Christophe


pgprWlToguMXo.pgp
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt-glib] Add API to redefine an existing domain

2011-11-18 Thread Zeeshan Ali (Khattak) 
On Sat, Nov 19, 2011 at 12:56 AM, Christophe Fergeau
cferg...@redhat.com wrote:
 Hey,

 Reading this patch, I'm wondering if gvir_domain_set_config might achieve
 what you are after (it's not implemented either)

  Yeah, that might actually be better.


 +    GVirConnectionPrivate *priv = conn-priv;
 +
 +    xml = gvir_config_object_to_xml(GVIR_CONFIG_OBJECT(conf));
 +
 +    g_return_if_fail(xml != NULL);
 +
 +    g_mutex_lock(priv-lock);
 +    dom = g_hash_table_lookup (priv-domains,
 +                               (gpointer)gvir_domain_get_uuid(domain));
 +    g_mutex_unlock(priv-lock);
 +    g_return_if_fail(dom != NULL);
 +    /* FIXME: Check if config's domain ID is the same as domain passed */
 +
 +    if (!(handle = virDomainDefineXML(priv-conn, xml))) {

 Shouldn't handle be assigned to some persistent state? My guess is
 domain-priv-handle.

 No, there is supposed to be already a corresponding domain with the
handle in hash table. Don't let the 'DomainDefineXML' name fool you.

-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt-glib] Add API to redefine an existing domain

2011-11-18 Thread Christophe Fergeau 
On Sat, Nov 19, 2011 at 01:02:28AM +0200, Zeeshan Ali (Khattak) wrote:
 On Sat, Nov 19, 2011 at 12:56 AM, Christophe Fergeau
 cferg...@redhat.com wrote:
  Hey,
 
  Reading this patch, I'm wondering if gvir_domain_set_config might achieve
  what you are after (it's not implemented either)
 
   Yeah, that might actually be better.
 
 
  +    GVirConnectionPrivate *priv = conn-priv;
  +
  +    xml = gvir_config_object_to_xml(GVIR_CONFIG_OBJECT(conf));
  +
  +    g_return_if_fail(xml != NULL);
  +
  +    g_mutex_lock(priv-lock);
  +    dom = g_hash_table_lookup (priv-domains,
  +                               (gpointer)gvir_domain_get_uuid(domain));
  +    g_mutex_unlock(priv-lock);
  +    g_return_if_fail(dom != NULL);
  +    /* FIXME: Check if config's domain ID is the same as domain passed */
  +
  +    if (!(handle = virDomainDefineXML(priv-conn, xml))) {
 
  Shouldn't handle be assigned to some persistent state? My guess is
  domain-priv-handle.
 
  No, there is supposed to be already a corresponding domain with the
 handle in hash table. Don't let the 'DomainDefineXML' name fool you.

No need for the handle variable then. This is one of the things that
fooled me :)

Christophe


pgpSNZJXy8LNy.pgp
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list