Re: [Spice-devel] VM pools and libgovirt
Hey, On Fri, Dec 06, 2013 at 06:14:52PM -0500, i iordanov wrote: Hi Christophe, I have another patch for you to consider. This one removes some duplicated code by creating and using a utility function to ovirt-utils.c. Ah cool, thanks. This mostly looks good, though I'd probably add a check that the property has the right type using g_object_class_find_property. I also switched out the two G_MAXUINT's in OvirtVmPool for G_MAXUINT32's. Hmm, I'm not sure about that one, what is the reasoning for it? I'd move this to a different commit. Thanks for working on getting rid of that duplicated code! Christophe pgpmaMH2ib_p7.pgp Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] VM pools and libgovirt
Hi Christophe, On Mon, Dec 9, 2013 at 11:35 AM, Christophe Fergeau cferg...@redhat.comwrote: Ah cool, thanks. This mostly looks good, though I'd probably add a check that the property has the right type using g_object_class_find_property. That's a good idea. I also switched out the two G_MAXUINT's in OvirtVmPool for G_MAXUINT32's. Hmm, I'm not sure about that one, what is the reasoning for it? I'd move this to a different commit. Fair point about the different commit. I was aligning them to your choice of making ovirt_utils_guint_from_string() return false if value64 exceeds G_MAXUINT32. This is a quick change, so I'll get you a new patch shortly. Thanks! iordan ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] VM pools and libgovirt
Hi Christophe,
Here's the new patch with the extra safety checks.
Cheers!
iordan
On Mon, Dec 9, 2013 at 11:56 AM, i iordanov iiorda...@gmail.com wrote:
Hi Christophe,
On Mon, Dec 9, 2013 at 11:35 AM, Christophe Fergeau
cferg...@redhat.comwrote:
Ah cool, thanks. This mostly looks good, though I'd probably add a check
that the property has the right type using g_object_class_find_property.
That's a good idea.
I also switched out the two G_MAXUINT's in OvirtVmPool for
G_MAXUINT32's.
Hmm, I'm not sure about that one, what is the reasoning for it? I'd move
this to a different commit.
Fair point about the different commit. I was aligning them to your choice
of making ovirt_utils_guint_from_string() return false if value64 exceeds
G_MAXUINT32.
This is a quick change, so I'll get you a new patch shortly.
Thanks!
iordan
--
The conscious mind has only one thread of execution.
From 04eadda83949733020a5cffc1dc26768063f31e9 Mon Sep 17 00:00:00 2001
From: Iordan Iordanov iiorda...@gmail.com
Date: Mon, 9 Dec 2013 12:23:43 -0500
Subject: [PATCH] Reduced code duplication in OvirtVmPool by adding a utility
function.
The functions to get size, prestarted_vms, and user_max_vms were all almost
the same code because each one was getting an integer from an XML node.
To deal with the code duplication, a utility function was added which
given a g_object and an XML node, sets a specified g_object property
from a specified XML sub-node. This patch removes the separate functions
and replaces them with calls to the new utility function, called
g_object_set_guint_property_from_xml().
---
govirt/ovirt-utils.c | 23 +++
govirt/ovirt-utils.h |4
govirt/ovirt-vm-pool.c | 57 +++-
3 files changed, 30 insertions(+), 54 deletions(-)
diff --git a/govirt/ovirt-utils.c b/govirt/ovirt-utils.c
index 3b9593a..dc4585e 100644
--- a/govirt/ovirt-utils.c
+++ b/govirt/ovirt-utils.c
@@ -212,3 +212,26 @@ G_GNUC_INTERNAL gboolean ovirt_utils_gerror_from_xml_fault(RestXmlNode *root, GE
return TRUE;
}
+
+
+G_GNUC_INTERNAL gboolean g_object_set_guint_property_from_xml(GObject *g_object,
+ RestXmlNode *node,
+ const gchar *node_name,
+ const gchar *prop_name)
+{
+RestXmlNode *sub_node;
+GParamSpec *spec;
+sub_node = rest_xml_node_find(node, node_name);
+if (sub_node != NULL sub_node-content != NULL) {
+guint value;
+if (!ovirt_utils_guint_from_string(sub_node-content, value)) {
+return FALSE;
+}
+spec = g_object_class_find_property(G_OBJECT_GET_CLASS(g_object), prop_name);
+if (spec != NULL spec-value_type == G_TYPE_UINT) {
+g_object_set(g_object, prop_name, value, NULL);
+return TRUE;
+}
+}
+return FALSE;
+}
diff --git a/govirt/ovirt-utils.h b/govirt/ovirt-utils.h
index f627c13..e92a2ec 100644
--- a/govirt/ovirt-utils.h
+++ b/govirt/ovirt-utils.h
@@ -32,6 +32,10 @@ G_BEGIN_DECLS
RestXmlNode *ovirt_rest_xml_node_from_call(RestProxyCall *call);
const char *ovirt_rest_xml_node_get_content(RestXmlNode *node, ...);
gboolean ovirt_utils_gerror_from_xml_fault(RestXmlNode *root, GError **error);
+gboolean g_object_set_guint_property_from_xml(GObject *g_object,
+ RestXmlNode *node,
+ const gchar *node_name,
+ const gchar *prop_name);
const char *ovirt_utils_genum_get_nick (GType enum_type, gint value);
int ovirt_utils_genum_get_value (GType enum_type, const char *nick,
diff --git a/govirt/ovirt-vm-pool.c b/govirt/ovirt-vm-pool.c
index ebbf3ae..61caafb 100644
--- a/govirt/ovirt-vm-pool.c
+++ b/govirt/ovirt-vm-pool.c
@@ -196,61 +196,10 @@ gboolean ovirt_vm_pool_allocate_vm_finish(OvirtVmPool *vm_pool,
}
-static gboolean vm_pool_set_size_from_xml(OvirtVmPool *vm_pool, RestXmlNode *node)
-{
-RestXmlNode *size_node;
-size_node = rest_xml_node_find(node, size);
-if (size_node != NULL) {
-guint size;
-g_return_val_if_fail(size_node-content != NULL, FALSE);
-if (!ovirt_utils_guint_from_string(size_node-content, size)) {
-return FALSE;
-}
-g_object_set(G_OBJECT(vm_pool), size, size, NULL);
-return TRUE;
-}
-return FALSE;
-}
-
-
-static gboolean vm_pool_set_prestarted_vms_from_xml(OvirtVmPool *vm_pool, RestXmlNode *node)
-{
-RestXmlNode *prestarted_vms_node;
-prestarted_vms_node = rest_xml_node_find(node, prestarted_vms);
-if (prestarted_vms_node != NULL) {
-guint prestarted_vms;
-g_return_val_if_fail(prestarted_vms_node-content != NULL, FALSE);
-if
Re: [Spice-devel] VM pools and libgovirt
On Mon, Dec 09, 2013 at 11:56:31AM -0500, i iordanov wrote: On Mon, Dec 9, 2013 at 11:35 AM, Christophe Fergeau cferg...@redhat.comwrote: I also switched out the two G_MAXUINT's in OvirtVmPool for G_MAXUINT32's. Hmm, I'm not sure about that one, what is the reasoning for it? I'd move this to a different commit. Fair point about the different commit. I was aligning them to your choice of making ovirt_utils_guint_from_string() return false if value64 exceeds G_MAXUINT32. Ah, my bad, I'd just change the G_MAXUINT32 to G_MAXUINT in ovirt_utils_guint_from_string() Christophe pgpjuVPJUTqAX.pgp Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] VM pools and libgovirt
On Mon, Dec 09, 2013 at 12:25:51PM -0500, i iordanov wrote: Hi Christophe, Here's the new patch with the extra safety checks. Thanks, I've pushed it after fixing the use of tabs in ovirt_vm_pool_refresh_from_xml() I've also fixed the G_MAXUINT32 test in the string-guint conversion. Christophe pgpSQPjXMoJuE.pgp Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] VM pools and libgovirt
Hi Christophe,
I have another patch for you to consider. This one removes some duplicated
code by creating and using a utility function to ovirt-utils.c. I also
switched out the two G_MAXUINT's in OvirtVmPool for G_MAXUINT32's.
We can make use of it any time an integer attribute needs to be parsed.
Maybe it should be in the OvirtResource class, but I'll leave that for you
to decide. Also, feel free to change naming or anything else you don't like.
Thanks!
iordan
On Thu, Dec 5, 2013 at 4:05 AM, Christophe Fergeau cferg...@redhat.comwrote:
On Wed, Dec 04, 2013 at 11:12:09PM -0500, i iordanov wrote:
On Wed, Dec 4, 2013 at 10:30 AM, Christophe Fergeau cferg...@redhat.com
wrote:
It would be nice if you could check I did not break anything for you (I
haven't tested ovirt_vm_pool_activate_vm() at all). Then I can push
all of
it to git master ;)
I tested and it allocates VMs just as well as the before your changes. It
also reports the parsed attributes correctly for me.
Cool, thanks for the testing, I can push it upstream then!
Christophe
--
The conscious mind has only one thread of execution.
From 0da8920996800ce271cbe3fff1f554a9c0367e0e Mon Sep 17 00:00:00 2001
From: Iordan Iordanov iiorda...@gmail.com
Date: Fri, 6 Dec 2013 13:26:07 -0500
Subject: [PATCH] Reduced code duplication in OvirtVmPool by adding a utility
function.
The functions to get size, prestarted_vms, and user_max_vms were all almost
the same code because each one was getting an integer from an XML node.
To deal with the code duplication, a utility function was added which
given a g_object and an XML node, sets a specified g_object property
from a specified XML sub-node. This patch removes the separate functions
and replaces them with calls to the new utility function, called
g_object_set_guint_property_from_xml().
---
govirt/ovirt-utils.c | 20 +++
govirt/ovirt-utils.h |4 +++
govirt/ovirt-vm-pool.c | 63 +---
3 files changed, 30 insertions(+), 57 deletions(-)
diff --git a/govirt/ovirt-utils.c b/govirt/ovirt-utils.c
index 3b9593a..8601cfe 100644
--- a/govirt/ovirt-utils.c
+++ b/govirt/ovirt-utils.c
@@ -212,3 +212,23 @@ G_GNUC_INTERNAL gboolean ovirt_utils_gerror_from_xml_fault(RestXmlNode *root, GE
return TRUE;
}
+
+
+G_GNUC_INTERNAL gboolean g_object_set_guint_property_from_xml(GObject *g_object,
+ RestXmlNode *node,
+ const gchar *node_name,
+ const gchar *prop_name)
+{
+RestXmlNode *sub_node;
+sub_node = rest_xml_node_find(node, node_name);
+if (sub_node != NULL) {
+guint value;
+g_return_val_if_fail(sub_node-content != NULL, FALSE);
+if (!ovirt_utils_guint_from_string(sub_node-content, value)) {
+return FALSE;
+}
+g_object_set(g_object, prop_name, value, NULL);
+return TRUE;
+}
+return FALSE;
+}
diff --git a/govirt/ovirt-utils.h b/govirt/ovirt-utils.h
index f627c13..e92a2ec 100644
--- a/govirt/ovirt-utils.h
+++ b/govirt/ovirt-utils.h
@@ -32,6 +32,10 @@ G_BEGIN_DECLS
RestXmlNode *ovirt_rest_xml_node_from_call(RestProxyCall *call);
const char *ovirt_rest_xml_node_get_content(RestXmlNode *node, ...);
gboolean ovirt_utils_gerror_from_xml_fault(RestXmlNode *root, GError **error);
+gboolean g_object_set_guint_property_from_xml(GObject *g_object,
+ RestXmlNode *node,
+ const gchar *node_name,
+ const gchar *prop_name);
const char *ovirt_utils_genum_get_nick (GType enum_type, gint value);
int ovirt_utils_genum_get_value (GType enum_type, const char *nick,
diff --git a/govirt/ovirt-vm-pool.c b/govirt/ovirt-vm-pool.c
index ebbf3ae..b2000dc 100644
--- a/govirt/ovirt-vm-pool.c
+++ b/govirt/ovirt-vm-pool.c
@@ -138,7 +138,7 @@ static void ovirt_vm_pool_class_init(OvirtVmPoolClass *klass)
Size of pool,
The number of VMs in the pool,
0,
- G_MAXUINT,
+ G_MAXUINT32,
0,
G_PARAM_READWRITE));
g_object_class_install_property(object_class,
@@ -147,7 +147,7 @@ static void ovirt_vm_pool_class_init(OvirtVmPoolClass *klass)
Prestarted VMs,
The number of VMs prestarted in the pool,
0,
-
Re: [Spice-devel] VM pools and libgovirt
On Wed, Dec 04, 2013 at 11:12:09PM -0500, i iordanov wrote: On Wed, Dec 4, 2013 at 10:30 AM, Christophe Fergeau cferg...@redhat.comwrote: It would be nice if you could check I did not break anything for you (I haven't tested ovirt_vm_pool_activate_vm() at all). Then I can push all of it to git master ;) I tested and it allocates VMs just as well as the before your changes. It also reports the parsed attributes correctly for me. Cool, thanks for the testing, I can push it upstream then! Christophe pgpg5I96hs9pp.pgp Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] VM pools and libgovirt
Hey, On Tue, Dec 03, 2013 at 06:21:22PM +0100, Christophe Fergeau wrote: On Tue, Dec 03, 2013 at 11:46:40AM -0500, i iordanov wrote: Would you like me to generate a patch with all the suggested changes and a better log entry? I've been doing that actually, with various changes: - renamed allocatevm to allocate_vm - renamed _vmpool_ to _vm_pool - moved ovirt_vm_pool_xml.c to ovirt_vm_pool.c as it was not that big - used guint for the various VmPool properties - detect parsing errors when parsing numbers - fixed the copyright headers (the new files should be (C) 2013 yourself, not (C) 2013 Red Hat Inc) I still have these changes roughly split up if you don't like some of them and want me to drop them. I've also added an async active_vm variant, and pushed all of this to http://cgit.freedesktop.org/~teuf/govirt/log/ It would be nice if you could check I did not break anything for you (I haven't tested ovirt_vm_pool_activate_vm() at all). Then I can push all of it to git master ;) Thanks, Christophe pgpMB5NOtT2Mt.pgp Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] VM pools and libgovirt
Hi Christophe, I'll do some testing this evening and will let you know. Thanks! iordan On Wed, Dec 4, 2013 at 10:30 AM, Christophe Fergeau cferg...@redhat.comwrote: Hey, On Tue, Dec 03, 2013 at 06:21:22PM +0100, Christophe Fergeau wrote: On Tue, Dec 03, 2013 at 11:46:40AM -0500, i iordanov wrote: Would you like me to generate a patch with all the suggested changes and a better log entry? I've been doing that actually, with various changes: - renamed allocatevm to allocate_vm - renamed _vmpool_ to _vm_pool - moved ovirt_vm_pool_xml.c to ovirt_vm_pool.c as it was not that big - used guint for the various VmPool properties - detect parsing errors when parsing numbers - fixed the copyright headers (the new files should be (C) 2013 yourself, not (C) 2013 Red Hat Inc) I still have these changes roughly split up if you don't like some of them and want me to drop them. I've also added an async active_vm variant, and pushed all of this to http://cgit.freedesktop.org/~teuf/govirt/log/ It would be nice if you could check I did not break anything for you (I haven't tested ovirt_vm_pool_activate_vm() at all). Then I can push all of it to git master ;) Thanks, Christophe -- The conscious mind has only one thread of execution. ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] VM pools and libgovirt
Hi, On Wed, Dec 4, 2013 at 10:30 AM, Christophe Fergeau cferg...@redhat.comwrote: It would be nice if you could check I did not break anything for you (I haven't tested ovirt_vm_pool_activate_vm() at all). Then I can push all of it to git master ;) I tested and it allocates VMs just as well as the before your changes. It also reports the parsed attributes correctly for me. Thanks! iordan -- The conscious mind has only one thread of execution. ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] VM pools and libgovirt
Hi Christophe,
On Mon, Dec 2, 2013 at 4:33 AM, Christophe Fergeau cferg...@redhat.comwrote:
Thanks a lot for working on that!
My pleasure! Attached find the patch, generated the way you wanted.
Cheers,
iordan
--
The conscious mind has only one thread of execution.
From d940ea24d8146a8d728ac5b7e38ec03eaf808001 Mon Sep 17 00:00:00 2001
From: Iordan Iordanov iiorda...@gmail.com
Date: Tue, 3 Dec 2013 02:58:45 -0500
Subject: [PATCH] - Added initial support for VM pools. - The library is now
able to allocate VMs from the pool synchronously. - The
attributes size, prestarted_vms, and max_user_vms are
parsed.
---
govirt/Makefile.am|4 +
govirt/govirt-private.h |1 +
govirt/ovirt-api.c| 26
govirt/ovirt-api.h|1 +
govirt/ovirt-proxy.h |1 +
govirt/ovirt-resource.c |1 +
govirt/ovirt-utils.c |7 +
govirt/ovirt-utils.h |1 +
govirt/ovirt-vmpool-private.h | 40 ++
govirt/ovirt-vmpool-xml.c | 80 +++
govirt/ovirt-vmpool.c | 316 +
govirt/ovirt-vmpool.h | 67 +
12 files changed, 545 insertions(+)
create mode 100644 govirt/ovirt-vmpool-private.h
create mode 100644 govirt/ovirt-vmpool-xml.c
create mode 100644 govirt/ovirt-vmpool.c
create mode 100644 govirt/ovirt-vmpool.h
diff --git a/govirt/Makefile.am b/govirt/Makefile.am
index d9e70a5..0eade7e 100644
--- a/govirt/Makefile.am
+++ b/govirt/Makefile.am
@@ -26,6 +26,7 @@ libgovirt_la_HEADERS = \
ovirt-storage-domain.h \
ovirt-types.h \
ovirt-vm.h \
+ ovirt-vmpool.h \
ovirt-vm-display.h \
$(NULL)
@@ -42,6 +43,7 @@ noinst_HEADERS = \
ovirt-storage-domain-private.h\
ovirt-utils.h \
ovirt-vm-private.h \
+ ovirt-vmpool-private.h \
$(NULL)
libgovirt_la_SOURCES = \
@@ -62,6 +64,8 @@ libgovirt_la_SOURCES = \
ovirt-vm.c \
ovirt-vm-display.c \
ovirt-vm-xml.c \
+ ovirt-vmpool.c \
+ ovirt-vmpool-xml.c \
$(NULL)
nodist_libgovirt_la_HEADERS = \
diff --git a/govirt/govirt-private.h b/govirt/govirt-private.h
index 972ebac..3258aae 100644
--- a/govirt/govirt-private.h
+++ b/govirt/govirt-private.h
@@ -33,6 +33,7 @@
#include govirt/ovirt-storage-domain-private.h
#include govirt/ovirt-utils.h
#include govirt/ovirt-vm-private.h
+#include govirt/ovirt-vmpool-private.h
#include govirt/glib-compat.h
#endif /* __OVIRT_PRIVATE_H__ */
diff --git a/govirt/ovirt-api.c b/govirt/ovirt-api.c
index ddb2300..bdcc576 100644
--- a/govirt/ovirt-api.c
+++ b/govirt/ovirt-api.c
@@ -41,6 +41,7 @@
struct _OvirtApiPrivate {
OvirtCollection *storage_domains;
OvirtCollection *vms;
+OvirtCollection *vmpools;
};
@@ -72,6 +73,7 @@ static void ovirt_api_dispose(GObject *object)
g_clear_object(api-priv-storage_domains);
g_clear_object(api-priv-vms);
+g_clear_object(api-priv-vmpools);
G_OBJECT_CLASS(ovirt_api_parent_class)-dispose(object);
}
@@ -131,6 +133,30 @@ OvirtCollection *ovirt_api_get_vms(OvirtApi *api)
return api-priv-vms;
}
+/**
+ * ovirt_api_get_vmpools:
+ * @api: a #OvirtApi
+ *
+ * Return value: (transfer full):
+ */
+OvirtCollection *ovirt_api_get_vmpools(OvirtApi *api)
+{
+const char *href;
+
+g_return_val_if_fail(OVIRT_IS_API(api), NULL);
+
+if (api-priv-vmpools != NULL)
+return api-priv-vmpools;
+
+href = ovirt_resource_get_sub_collection(OVIRT_RESOURCE(api), vmpools);
+if (href == NULL)
+return NULL;
+
+api-priv-vmpools = ovirt_collection_new(href, vmpools, OVIRT_TYPE_VMPOOL, vmpool);
+
+return api-priv-vmpools;
+}
+
/**
* ovirt_api_get_storage_domains:
diff --git a/govirt/ovirt-api.h b/govirt/ovirt-api.h
index d1de522..3af1bc0 100644
--- a/govirt/ovirt-api.h
+++ b/govirt/ovirt-api.h
@@ -63,6 +63,7 @@ OvirtApi *ovirt_api_new(void);
OvirtCollection *ovirt_api_get_storage_domains(OvirtApi *api);
OvirtCollection *ovirt_api_get_vms(OvirtApi *api);
+OvirtCollection *ovirt_api_get_vmpools(OvirtApi *api);
G_END_DECLS
diff --git a/govirt/ovirt-proxy.h b/govirt/ovirt-proxy.h
index 985b76d..83d7024 100644
--- a/govirt/ovirt-proxy.h
+++ b/govirt/ovirt-proxy.h
@@ -26,6 +26,7 @@
#include govirt/ovirt-api.h
#include govirt/ovirt-types.h
#include govirt/ovirt-vm.h
+#include govirt/ovirt-vmpool.h
G_BEGIN_DECLS
diff --git a/govirt/ovirt-resource.c b/govirt/ovirt-resource.c
index fca06e8..81241be 100644
--- a/govirt/ovirt-resource.c
+++ b/govirt/ovirt-resource.c
@@ -25,6 +25,7 @@
#include string.h
#include rest/rest-xml-node.h
+#include rest/rest-xml-parser.h
#include govirt-private.h
#include ovirt-error.h
diff --git a/govirt/ovirt-utils.c b/govirt/ovirt-utils.c
index 618992a..6799ddb 100644
--- a/govirt/ovirt-utils.c
+++ b/govirt/ovirt-utils.c
@@ -126,6 +126,13 @@ ovirt_utils_boolean_from_string(const char *value)
return (g_strcmp0(value, true) == 0);
}
Re: [Spice-devel] VM pools and libgovirt
Hey Iordan,
On Tue, Dec 03, 2013 at 03:07:09AM -0500, i iordanov wrote:
My pleasure! Attached find the patch, generated the way you wanted.
Thanks for the patch, a few quick initial comments:
- formatting of the commit log was off, see (for example)
https://wiki.openstack.org/wiki/GitCommitMessages#Summary_of_GIT_commit_message_structure
for the recommended format of git commit logs (helps when using 'git
shortlog' for example)
This document as a whole is an interesting read imo ;)
- I'd use ovirt_vmpool_allocate_vm (rather than _allocatevm)
- imo the ovirt_invoke_action stuff should be moved to a generic place
before this patch, but I can look into that
- there were a few minor white space issues (see patch below)
- you need to export the new public functions in govirt/govirt.sym (see
patch below)
Christophe
git a/govirt/govirt.sym b/govirt/govirt.sym
index bc53901..bb7be79 100644
--- a/govirt/govirt.sym
+++ b/govirt/govirt.sym
@@ -87,5 +87,12 @@ GOVIRT_0.2.1 {
ovirt_vm_get_cdroms;
} GOVIRT_0.2.0;
+GOVIRT_0.3.1 {
+ovirt_api_get_vmpools;
+
+ovirt_vmpool_get_type;
+ovirt_vmpool_new;
+ovirt_vmpool_allocatevm;
+} GOVIRT_0.2.1;
# define new API here using predicted next version number
diff --git a/govirt/ovirt-vmpool-xml.c b/govirt/ovirt-vmpool-xml.c
index ad17b8d..bb359bc 100644
--- a/govirt/ovirt-vmpool-xml.c
+++ b/govirt/ovirt-vmpool-xml.c
@@ -34,7 +34,7 @@ static gboolean vmpool_set_size_from_xml(OvirtVmPool
*vmpool, RestXmlNode *node)
RestXmlNode *size_node;
size_node = rest_xml_node_find(node, size);
if (size_node != NULL) {
- gint size;
+gint size;
g_return_val_if_fail(size_node-content != NULL, FALSE);
size = (gint)ovirt_utils_guint64_from_string(size_node-content);
g_object_set(G_OBJECT(vmpool), size, size, NULL);
diff --git a/govirt/ovirt-vmpool.c b/govirt/ovirt-vmpool.c
index 93180a8..b39d34e 100644
--- a/govirt/ovirt-vmpool.c
+++ b/govirt/ovirt-vmpool.c
@@ -38,9 +38,9 @@ static gboolean parse_action_response(RestProxyCall
*call, OvirtVmPool *vmpool,
(G_TYPE_INSTANCE_GET_PRIVATE((obj), OVIRT_TYPE_VMPOOL,
OvirtVmPoolPrivate))
struct _OvirtVmPoolPrivate {
- gint prestarted_vms;
- gint max_user_vms;
- gint size;
+gint prestarted_vms;
+gint max_user_vms;
+gint size;
};
G_DEFINE_TYPE(OvirtVmPool, ovirt_vmpool, OVIRT_TYPE_RESOURCE);
@@ -75,7 +75,7 @@ static void ovirt_vmpool_get_property(GObject *object,
g_value_set_int(value, vmpool-priv-prestarted_vms);
break;
case PROP_MAX_USER_VMS:
- g_value_set_int(value, vmpool-priv-max_user_vms);
+g_value_set_int(value, vmpool-priv-max_user_vms);
break;
default:
G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
[
pgpq9rXrpNHdD.pgp
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] VM pools and libgovirt
Hi Christophe, On Tue, Dec 3, 2013 at 5:12 AM, Christophe Fergeau cferg...@redhat.comwrote: Thanks for the patch, a few quick initial comments: - formatting of the commit log was off, see (for example) https://wiki.openstack.org/wiki/GitCommitMessages#Summary_of_GIT_commit_message_structure for the recommended format of git commit logs (helps when using 'git shortlog' for example) This document as a whole is an interesting read imo ;) Thanks for the tip! - I'd use ovirt_vmpool_allocate_vm (rather than _allocatevm) I used allocatevm because the action is actually allocatevm not allocate_vm, but if you think the function name doesn't need to be in line with the action, then I agree. - imo the ovirt_invoke_action stuff should be moved to a generic place before this patch, but I can look into that Yes, it's basically duplicated code, but I didn't think it was up to me to restructure :). - there were a few minor white space issues (see patch below) My editor was misconfigured... - you need to export the new public functions in govirt/govirt.sym (see patch below) Yes, I meant to ask you about that, but I wasn't sure what version to put, so I left it up to you (which was a good idea I think :)) Would you like me to generate a patch with all the suggested changes and a better log entry? Cheers! iordan -- The conscious mind has only one thread of execution. ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] VM pools and libgovirt
On Tue, Dec 03, 2013 at 11:46:40AM -0500, i iordanov wrote:
Would you like me to generate a patch with all the suggested changes and a
better log entry?
I've been doing that actually, with various changes:
- renamed allocatevm to allocate_vm
- renamed _vmpool_ to _vm_pool
- moved ovirt_vm_pool_xml.c to ovirt_vm_pool.c as it was not that big
- used guint for the various VmPool properties
- detect parsing errors when parsing numbers
- fixed the copyright headers (the new files should be (C) 2013 yourself,
not (C) 2013 Red Hat Inc)
I still have these changes roughly split up if you don't like some of them
and want me to drop them.
Christophe
From 436eb3fd47ee1cc28dd9a2c68e3bf4c2c7827f60 Mon Sep 17 00:00:00 2001
From: Iordan Iordanov iiorda...@gmail.com
Date: Tue, 3 Dec 2013 03:07:09 -0500
Subject: [PATCH] Added initial support for VM pools.
The library is now able to allocate VMs from the pool synchronously.
The attributes size, prestarted_vms, and max_user_vms are parsed.
---
govirt/Makefile.am | 2 +
govirt/govirt.h | 1 +
govirt/govirt.sym | 7 +
govirt/ovirt-api.c | 28
govirt/ovirt-api.h | 1 +
govirt/ovirt-resource.c | 1 +
govirt/ovirt-utils.c| 45 ++
govirt/ovirt-utils.h| 2 +
govirt/ovirt-vm-pool.c | 368
govirt/ovirt-vm-pool.h | 65 +
10 files changed, 520 insertions(+)
create mode 100644 govirt/ovirt-vm-pool.c
create mode 100644 govirt/ovirt-vm-pool.h
diff --git a/govirt/Makefile.am b/govirt/Makefile.am
index d9e70a5..645545d 100644
--- a/govirt/Makefile.am
+++ b/govirt/Makefile.am
@@ -26,6 +26,7 @@ libgovirt_la_HEADERS =
\
ovirt-storage-domain.h \
ovirt-types.h \
ovirt-vm.h \
+ ovirt-vm-pool.h \
ovirt-vm-display.h \
$(NULL)
@@ -62,6 +63,7 @@ libgovirt_la_SOURCES =
\
ovirt-vm.c \
ovirt-vm-display.c \
ovirt-vm-xml.c \
+ ovirt-vm-pool.c \
$(NULL)
nodist_libgovirt_la_HEADERS = \
diff --git a/govirt/govirt.h b/govirt/govirt.h
index 69f878b..fb7756f 100644
--- a/govirt/govirt.h
+++ b/govirt/govirt.h
@@ -34,5 +34,6 @@
#include govirt/ovirt-storage-domain.h
#include govirt/ovirt-vm.h
#include govirt/ovirt-vm-display.h
+#include govirt/ovirt-vm-pool.h
#endif /* __OVIRT_H__ */
diff --git a/govirt/govirt.sym b/govirt/govirt.sym
index bc53901..49636d5 100644
--- a/govirt/govirt.sym
+++ b/govirt/govirt.sym
@@ -87,5 +87,12 @@ GOVIRT_0.2.1 {
ovirt_vm_get_cdroms;
} GOVIRT_0.2.0;
+GOVIRT_0.3.1 {
+ovirt_api_get_vm_pools;
+
+ovirt_vm_pool_get_type;
+ovirt_vm_pool_new;
+ovirt_vm_pool_allocate_vm;
+} GOVIRT_0.2.1;
# define new API here using predicted next version number
diff --git a/govirt/ovirt-api.c b/govirt/ovirt-api.c
index ddb2300..4b6d141 100644
--- a/govirt/ovirt-api.c
+++ b/govirt/ovirt-api.c
@@ -2,6 +2,7 @@
* ovirt-api.c: oVirt API entry point
*
* Copyright (C) 2012, 2013 Red Hat, Inc.
+ * Copyright (C) 2013 Iordan Iordanov i...@iiordanov.com
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -27,6 +28,7 @@
#include ovirt-proxy.h
#include ovirt-rest-call.h
#include ovirt-api.h
+#include ovirt-vm-pool.h
#include govirt-private.h
#include rest/rest-xml-node.h
@@ -41,6 +43,7 @@
struct _OvirtApiPrivate {
OvirtCollection *storage_domains;
OvirtCollection *vms;
+OvirtCollection *vm_pools;
};
@@ -72,6 +75,7 @@ static void ovirt_api_dispose(GObject *object)
g_clear_object(api-priv-storage_domains);
g_clear_object(api-priv-vms);
+g_clear_object(api-priv-vm_pools);
G_OBJECT_CLASS(ovirt_api_parent_class)-dispose(object);
}
@@ -131,6 +135,30 @@ OvirtCollection *ovirt_api_get_vms(OvirtApi *api)
return api-priv-vms;
}
+/**
+ * ovirt_api_get_vm_pools:
+ * @api: a #OvirtApi
+ *
+ * Return value: (transfer full):
+ */
+OvirtCollection *ovirt_api_get_vm_pools(OvirtApi *api)
+{
+const char *href;
+
+g_return_val_if_fail(OVIRT_IS_API(api), NULL);
+
+if (api-priv-vm_pools != NULL)
+return api-priv-vm_pools;
+
+href = ovirt_resource_get_sub_collection(OVIRT_RESOURCE(api), vmpools);
+if (href == NULL)
+return NULL;
+
+api-priv-vm_pools = ovirt_collection_new(href, vmpools,
OVIRT_TYPE_VM_POOL, vmpool);
+
+return api-priv-vm_pools;
+}
+
/**
* ovirt_api_get_storage_domains:
Re: [Spice-devel] VM pools and libgovirt
On Fri, Nov 29, 2013 at 10:32:03PM -0500, i iordanov wrote: 0) Do you require the asynchronous functions before I submit my changes? Would be better, but I can live without it/add it myself, don't worry ;) 1) Would a patch against libgovirt 0.30 be acceptable, or would you require a patch for the master branch of git? There weren't many changes between 0.3.0 and git master, so a patch against 0.3.0 would be good 2) Is a patch file acceptable, or do you prefer some other way of submitting the changes? A patch I can apply with git am would be best so that I get a changelog and authorship information out of it. If you are working from the tarball, you can unpack the tarball, cd into libgovirt-0.3.0/, git init; git add .; git commit -m initial import; apply your patch, git commit -a, write commit log, git format-patch -1, and send me the file which was generated. Thanks a lot for working on that! Christophe pgpADFUxlXtRw.pgp Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] VM pools and libgovirt
Hi Christophe, Good news! I have added the functionality we discussed to libgovirt 0.30 and it works. The only thing I don't have is ovirt_vmpool_invoke_action_async() and related functions. I use the synchronous call. Can you please tell me: 0) Do you require the asynchronous functions before I submit my changes? 1) Would a patch against libgovirt 0.30 be acceptable, or would you require a patch for the master branch of git? 2) Is a patch file acceptable, or do you prefer some other way of submitting the changes? Thanks! iordan -- The conscious mind has only one thread of execution. ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] VM pools and libgovirt
Hey Iordan, On Mon, Nov 25, 2013 at 09:17:55PM -0500, i iordanov wrote: Do you guys think that there is a use-case for the client to be able to show the pool to the user and allow him/her to start a VM from that pool right then and there without needing the web interface? Probably makes sense, that's your call ;) If so, and providing the REST API allows that, would you be interested in adding such functionality to libgovirt? From a quick look at the documentation https://access.redhat.com/site/documentation/en-US/Red_Hat_Enterprise_Virtualization/3.3-Beta/html/Developer_Guide/chap-Virtual_Machine_Pools.html it should not be too hard to add support for it (though https://bugzilla.redhat.com/show_bug.cgi?id=1034710 is missing in the doc). I think what is needed is to derive an OvirtVmPool class from OvirtResource which would parse the few extra attributes in the vm pool (size, prestarted_vms), and which would expose the 'allocatevm' action. Then OvirtApi would gain a method to get an OvirtCollection wrapping the 'vmpools' collection. Do you think you can provide a patch for this? Christophe pgpvAzahk7rLy.pgp Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] VM pools and libgovirt
On Tue, Nov 26, 2013 at 6:41 AM, Christophe Fergeau cferg...@redhat.comwrote: From a quick look at the documentation https://access.redhat.com/site/documentation/en-US/Red_Hat_Enterprise_Virtualization/3.3-Beta/html/Developer_Guide/chap-Virtual_Machine_Pools.html it should not be too hard to add support for it (though https://bugzilla.redhat.com/show_bug.cgi?id=1034710 is missing in the doc). I think what is needed is to derive an OvirtVmPool class from OvirtResource which would parse the few extra attributes in the vm pool (size, prestarted_vms), and which would expose the 'allocatevm' action. Then OvirtApi would gain a method to get an OvirtCollection wrapping the 'vmpools' collection. Do you think you can provide a patch for this? This sounds like an interesting project! From the following file in my oVirt installation: /usr/lib/python2.7/site-packages/ovirtsdk/infrastructure/brokers.py I gather that a POST request must be made to /api/vmpools/SOMEVMPOOLID/allocatevm with a certain content, certain headers, etc. However, I cannot craft a successful POST manually. I guess I'm missing some crucial information which must be supplied in the documentation? Thanks! iordan -- The conscious mind has only one thread of execution. ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] VM pools and libgovirt
On Tue, Nov 26, 2013 at 9:03 AM, Christophe Fergeau cferg...@redhat.comwrote: I'd expect that doing something similar to ovirt_vm_action() should work? This action stuff needs to be moved from OvirtVm to OvirtResource, but I did not get to it yet ;) Yes, but at least one thing that we don't know is what actions one would take on a vmpool, right? iordan -- The conscious mind has only one thread of execution. ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] VM pools and libgovirt
On Tue, Nov 26, 2013 at 09:12:16AM -0500, i iordanov wrote: I take this back, the action here would be allocatevm, right? I should try playing with this. Yes it is, you can get the list of actions by accessing /api/vmpools/ in your webbrowser (OvirtVm has code to parse the available actions and put them in a hash table). Christophe pgptmfzITjkM1.pgp Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] VM pools and libgovirt
Yes, I had noticed that which is why I retracted my question :D. Thanks Christophe! iordan On Tue, Nov 26, 2013 at 9:17 AM, Christophe Fergeau cferg...@redhat.comwrote: On Tue, Nov 26, 2013 at 09:12:16AM -0500, i iordanov wrote: I take this back, the action here would be allocatevm, right? I should try playing with this. Yes it is, you can get the list of actions by accessing /api/vmpools/ in your webbrowser (OvirtVm has code to parse the available actions and put them in a hash table). Christophe -- The conscious mind has only one thread of execution. ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
