GNOME Bugzilla – Bug 765297
Missing support for datacenters/clusters
Last modified: 2021-05-25 11:23:40 UTC
For foreign menu support, virt-viewer needs to find the ISO domain which is associated with the current VM. The way to do it is to find the ISO domain associated with the datacenter the VM is in. /ovirt-engine/api/vms/$uuid has a link to the cluster the VM is in, /ovirt-engine/api/storagedomains/$uuid has the datacenter the ISO domain is in. We need some support for parsing datacenters and/or clusters nodes in libgovirt and exposing this in the API so that we can do the needed matching.
Bug #767723 is more work than solving just this bug, but it would solve this bug as a side-effect.
Created attachment 350017 [details] [review] Retrieve host id and cluster id from vm
Created attachment 350018 [details] [review] Retrieve data center id from storage domain
Created attachment 350019 [details] [review] Add auxiliary ovirt_sub_collection_new_from_resource
Created attachment 350020 [details] [review] New APIs to enable search queries for collections
Created attachment 350021 [details] [review] Support for Hosts
Created attachment 350022 [details] [review] Support for clusters
Created attachment 350023 [details] [review] Support for Data Centers
Patches available at my github repository. The full diff and commits can be seen at https://github.com/GNOME/libgovirt/compare/master...etrunko:master
Created attachment 350024 [details] [review] Support for hosts Added missing g_object_clear_object(&hosts->priv->vms);
Created attachment 350025 [details] [review] Support for clusters Added missing g_object_clear_object(&clusters->priv->hosts);
Created attachment 350026 [details] [review] Support for data centers Added missing g_object_clear_object(&data_centers->priv->clusters);
I have a new series on my github repository, it introduces some refactoring, especially in ovirt-storage-domain so that the ovirt_resource_parse_xml function can be reused by other objects. https://github.com/GNOME/libgovirt/compare/master...etrunko:master
Created attachment 351123 [details] [review] Remove unecessary ovirt_api_init_from_xml()
Created attachment 351124 [details] [review] Remove unused function ovirt_rest_xml_node_get_content()
Created attachment 351125 [details] [review] New function ovirt_rest_xml_node_get_attr_from_path()
Created attachment 351126 [details] [review] Factor out property value setting from ovirt_resource_parse_xml()
Created attachment 351127 [details] [review] Add support for attributes in ovirt_resource_parse_xml()
Created attachment 351128 [details] [review] Move out ovirt_resource_parse_xml() to ovirt-utils
Created attachment 351129 [details] [review] Retrieve data center ids
Created attachment 351130 [details] [review] Retrieve host and cluster ids
Created attachment 351131 [details] [review] Set vm state property using OvirtXmlElement struct
Created attachment 351132 [details] [review] Introduce auxiliary function ovirt_sub_collection_new_from_resource()
Created attachment 351133 [details] [review] New API functions to enable search queries of collections
Created attachment 351134 [details] [review] Implement support for hosts
Created attachment 351135 [details] [review] Implement support for clusters
Created attachment 351136 [details] [review] Implement support for data centers
Review of attachment 351128 [details] [review]: ::: govirt/ovirt-resource.h @@ +58,3 @@ gboolean (*init_from_xml)(OvirtResource *resource, RestXmlNode *node, + OvirtXmlElement *elements, This is going to be an ABI break. Did not look carefully enough at the series to see if this can be avoided ;)
Review of attachment 351131 [details] [review]: ::: govirt/ovirt-vm.h @@ +65,2 @@ typedef enum { + OVIRT_VM_STATE_UNKNOWN, This is going to break ABI too.
Review of attachment 351123 [details] [review]: Why? This deserves an explanation in the commit log, this looks like it would break ovirt_resource_refresh(ovirt_api_new());
Review of attachment 351124 [details] [review]: Most uses were removed in dbf8dd85 "Add generic resource parser" and the last use in 18d7c005 "storage-domain: Parse storage domain status" (please add this to the commit log)
Review of attachment 351124 [details] [review]: ::: govirt/ovirt-utils.c @@ -101,3 @@ - va_start(args, node); - - content = ovirt_rest_xml_node_get_content_va(node, &args, NULL); If you remove this call, the va_list arg to ovirt_rest_xml_node_get_content_va() becomes unused.
Review of attachment 351125 [details] [review]: .
Review of attachment 351126 [details] [review]: Well, now you have chained if/else blocks (for enums, OvirtResource), chained with a switch/case. I have a slight preference for how it initially was as it's all consistent. Can you split off the G_TYPE_STRING change?
Review of attachment 351125 [details] [review]: ::: govirt/ovirt-utils.c @@ +94,3 @@ +ovirt_rest_xml_node_get_attr_from_path(RestXmlNode *node, const char *path, const char *attr) +{ + GStrv pathv = g_strsplit(path, "/", -1); maybe g_return_val_if_fail(attr != NULL); since this function is not going to handle NULL attribute names?
Review of attachment 351127 [details] [review]: More than half of this patch is related to cosmetic renaming/reformatting/..., can you split that up? (eg a commit doing the s/xml_node/xml_path + the reformatting of storage_domain_elements, and a commit adding support for attributes?)
Review of attachment 351128 [details] [review]: I believe it should be possible to move ovirt_resoure_parse_xml() to ovirt-utils without any other change, and then add up the rest on top of that? Too much code movement lumped together with other changes, I'm not really willing to mentally split that for a review.
Review of attachment 351129 [details] [review]: ::: govirt/ovirt-storage-domain.c @@ +86,3 @@ break; + case PROP_DATA_CENTER_IDS: + g_value_set_pointer(value, domain->priv->data_center_ids); Could be g_value_set_boxed() @@ +124,3 @@ break; + case PROP_DATA_CENTER_IDS: + g_array_unref(domain->priv->data_center_ids); You cannot call this on a NULL GArray. @@ +151,3 @@ + + if (domain->priv->data_center_ids == NULL) { + domain->priv->data_center_ids = g_array_new(FALSE, FALSE, sizeof(gchar *)); I think I'd just do this in the _init() method, and assume domain->priv->data_center_ids is non-NULL. @@ +199,3 @@ + sub_node = rest_xml_node_find(node, "data_centers"); + if (sub_node != NULL) + g_hash_table_foreach(sub_node->children, (GHFunc) _append_data_center_id, resource); An alternative would be g_hash_table_iter_init (&iter, hash_table); while (g_hash_table_iter_next (&iter, &key, &value)) { // do something with key and value } (but no strong preference between the 2) Have you considered adding G_TYPE_STRV support to ovirt_resource_parse_xml()? @@ +298,3 @@ param_spec); + + param_spec = g_param_spec_pointer("data-center-ids", g_param_spec_boxed()
Review of attachment 351130 [details] [review]: ::: govirt/ovirt-vm.c @@ -126,3 @@ parent_class = OVIRT_RESOURCE_CLASS(ovirt_vm_parent_class); - return parent_class->init_from_xml(resource, node, elements, error); This feels odd, we were passed 'elements' as an argument, but we actually ignore it, and pass down something else?
Review of attachment 351131 [details] [review]: ::: govirt/ovirt-vm-xml.c @@ -148,3 @@ - return FALSE; - } - state_node = rest_xml_node_find(state_node, "state"); Not sure it's going to be possible to handle status/state in the generic code, but this could at least use ovirt_rest_xml_node_get_content_from_path()
Review of attachment 351133 [details] [review]: I would add some example search queries to the API doc (or the commit log), and ideally a link to some document explaining that if this exists. "functionality" ::: govirt/govirt.sym @@ +113,3 @@ + ovirt_api_get_storage_domains_search; + ovirt_api_get_vms_search; + ovirt_api_get_vm_pools_search; 0.3.4 is released already so this should go to a new section.
(In reply to Christophe Fergeau from comment #30) > Review of attachment 351123 [details] [review] [review]: > > Why? This deserves an explanation in the commit log, this looks like it > would break ovirt_resource_refresh(ovirt_api_new()); I am not sure why would it break. All ovirt_api_init_from_xml() function does is to call the parent function, notice the #if 0 block.
(In reply to Christophe Fergeau from comment #31) > Review of attachment 351124 [details] [review] [review]: > > Most uses were removed in dbf8dd85 "Add generic resource parser" and the > last use in 18d7c005 "storage-domain: Parse storage domain status" > (please add this to the commit log) Done
(In reply to Christophe Fergeau from comment #32) > Review of attachment 351124 [details] [review] [review]: > > ::: govirt/ovirt-utils.c > @@ -101,3 @@ > - va_start(args, node); > - > - content = ovirt_rest_xml_node_get_content_va(node, &args, NULL); > > If you remove this call, the va_list arg to > ovirt_rest_xml_node_get_content_va() becomes unused. So what do you think about changing the ovirt_rest_xml_node_get_content() function to account for the path only? I could merge this with the next commit "New function ovirt_rest_xml_node_get_attr_from_path()"
(In reply to Christophe Fergeau from comment #34) > Review of attachment 351126 [details] [review] [review]: > > Well, now you have chained if/else blocks (for enums, OvirtResource), > chained with a switch/case. I have a slight preference for how it initially > was as it's all consistent. Can you split off the G_TYPE_STRING change? Hmmm, so it's a matter of preference then, I don't really mind about keeping it the way it was. IMHO, this new way makes it explicit about testing the not fundamental types first and only then going to the fundamental types (after all it is not possible to make the first two into the switch anyway). I kind of find it harder to read when we expand it with new types, such as G_TYPE_STRING and the soon to be needed G_TYPE_UINT, and so on. Anyway, just tell me what you like best and I will keep it that way.
(In reply to Christophe Fergeau from comment #35) > Review of attachment 351125 [details] [review] [review]: > > ::: govirt/ovirt-utils.c > @@ +94,3 @@ > +ovirt_rest_xml_node_get_attr_from_path(RestXmlNode *node, const char *path, > const char *attr) > +{ > + GStrv pathv = g_strsplit(path, "/", -1); > > maybe g_return_val_if_fail(attr != NULL); since this function is not going > to handle NULL attribute names? Done
(In reply to Eduardo Lima (Etrunko) from comment #46) > (In reply to Christophe Fergeau from comment #35) > > Review of attachment 351125 [details] [review] [review] [review]: > > > > ::: govirt/ovirt-utils.c > > @@ +94,3 @@ > > +ovirt_rest_xml_node_get_attr_from_path(RestXmlNode *node, const char *path, > > const char *attr) > > +{ > > + GStrv pathv = g_strsplit(path, "/", -1); > > > > maybe g_return_val_if_fail(attr != NULL); since this function is not going > > to handle NULL attribute names? > > Done Well, taking a better look, this is already done in rest_xml_node_get_attr(), so no need to test it again, I think.
(In reply to Christophe Fergeau from comment #37) > Review of attachment 351128 [details] [review] [review]: > > I believe it should be possible to move ovirt_resoure_parse_xml() to > ovirt-utils without any other change, and then add up the rest on top of > that? Too much code movement lumped together with other changes, I'm not > really willing to mentally split that for a review. Done
(In reply to Christophe Fergeau from comment #38) > Review of attachment 351129 [details] [review] [review]: > > ::: govirt/ovirt-storage-domain.c > @@ +86,3 @@ > break; > + case PROP_DATA_CENTER_IDS: > + g_value_set_pointer(value, domain->priv->data_center_ids); > > Could be g_value_set_boxed() Done. > > @@ +124,3 @@ > break; > + case PROP_DATA_CENTER_IDS: > + g_array_unref(domain->priv->data_center_ids); > > You cannot call this on a NULL GArray. Fixed. > > @@ +151,3 @@ > + > + if (domain->priv->data_center_ids == NULL) { > + domain->priv->data_center_ids = g_array_new(FALSE, FALSE, > sizeof(gchar *)); > > I think I'd just do this in the _init() method, and assume > domain->priv->data_center_ids is non-NULL. > > @@ +199,3 @@ > + sub_node = rest_xml_node_find(node, "data_centers"); > + if (sub_node != NULL) > + g_hash_table_foreach(sub_node->children, (GHFunc) > _append_data_center_id, resource); > > An alternative would be > g_hash_table_iter_init (&iter, hash_table); > while (g_hash_table_iter_next (&iter, &key, &value)) > { > // do something with key and value > } > (but no strong preference between the 2) > > Have you considered adding G_TYPE_STRV support to ovirt_resource_parse_xml()? Yes, the first version of this patch was done with GStrv insteaf o GArray, which one should we stick with? > > @@ +298,3 @@ > param_spec); > + > + param_spec = g_param_spec_pointer("data-center-ids", > > g_param_spec_boxed() Done.
(In reply to Eduardo Lima (Etrunko) from comment #49) > (In reply to Christophe Fergeau from comment #38) > > Review of attachment 351129 [details] [review] [review] [review]: > > @@ +199,3 @@ > > + sub_node = rest_xml_node_find(node, "data_centers"); > > + if (sub_node != NULL) > > + g_hash_table_foreach(sub_node->children, (GHFunc) > > _append_data_center_id, resource); > > > > An alternative would be > > g_hash_table_iter_init (&iter, hash_table); > > while (g_hash_table_iter_next (&iter, &key, &value)) > > { > > // do something with key and value > > } > > (but no strong preference between the 2) > > > > Have you considered adding G_TYPE_STRV support to ovirt_resource_parse_xml()? > > Yes, the first version of this patch was done with GStrv insteaf o GArray, > which one should we stick with? > Both ? :) I mean, if you have a NULL-terminated GArray of char *, then Garray::data will be a GStrv. So they are not mutually exclusive. For building the GStrv, you really want to use GArray. But this can be generic code in ovirt_resource_parse_xml() which will ultimately return GStrv (or a GStrv wrapped in a GArray if the refcounting is needed).
Sending new series with commits reordered in more logical sets: Patches 0001 to 0004: - Refactoring and moving ovirt_resource_parse_xml() from storage-domain to ovirt-utils and finally adding it to the init_from_xml() virtual function of OvirtResource. Patches 0005 to 0009: - Refactoring and renaming ovirt_rest_xml_node_get_content_va() to return the node instead of the content. Support for retrieving an attribute from a subnode. Adding support for G_TYPE_STRING to ovirt_rest_xml_node_parse() and finally retrieve host/cluster ids from a VM. Patches 0010 to 0011: - Add support for G_TYPE_STRV to ovirt_rest_xml_node_parse() and retrieve data center ids from storage domain. Patches 0012 to 0014: - Refactoring API functions to reuse code for creating sub-collections and also use queries. Patches 0015 to 0017: - Support for hosts, clusters and data centers.
Created attachment 351678 [details] [review] Factor out property value setting from ovirt_resource_parse_xml()
Created attachment 351679 [details] [review] Use explicit initialization of struct OvirtXmlElement members
Created attachment 351680 [details] [review] Move out ovirt_resource_parse_xml() to ovirt-utils
Created attachment 351681 [details] [review] Add OvirtXmlElement as argument to init_from_xml() virtual function
Created attachment 351682 [details] [review] Remove unused function ovirt_rest_xml_node_get_content()
Created attachment 351683 [details] [review] Rename ovirt_rest_xml_node_get_content_va() to ovirt_rest_xml_node_find()
Created attachment 351684 [details] [review] Retrieve node attributes in ovirt_resource_parse_xml()
Created attachment 351685 [details] [review] Support G_TYPE_STRING in _set_property_value_from_type()
Created attachment 351686 [details] [review] Retrieve host and cluster ids
Created attachment 351687 [details] [review] Support G_TYPE_STRV in _set_property_value_from_type()
Created attachment 351688 [details] [review] Retrieve data center ids
Created attachment 351689 [details] [review] Introduce auxiliary function ovirt_sub_collection_new_from_resource()
Created attachment 351690 [details] [review] Remove unecessary ovirt_api_init_from_xml()
Created attachment 351691 [details] [review] New API functions to enable search queries of collections
Created attachment 351692 [details] [review] Implement support for hosts
Created attachment 351693 [details] [review] Implement support for clusters
Created attachment 351694 [details] [review] Implement support for data centers
Created attachment 351749 [details] [review] Rename ovirt_rest_xml_node_get_content_va() to ovirt_rest_xml_node_find() Fix crash in this patch after more thorough testing
(In reply to Eduardo Lima (Etrunko) from comment #42) > (In reply to Christophe Fergeau from comment #30) > > Review of attachment 351123 [details] [review] [review] [review]: > > > > Why? This deserves an explanation in the commit log, this looks like it > > would break ovirt_resource_refresh(ovirt_api_new()); > > I am not sure why would it break. All ovirt_api_init_from_xml() function > does is to call the parent function, notice the #if 0 block. ovirt_resource_refresh() calls ovirt_resource_init_from_xml() which does: (both in git master and after your patches) g_return_val_if_fail(klass->init_from_xml != NULL, FALSE); Before your patch, OvirtApi::init_from_xml is non-NULL, so the parent implementation is called. After your patch, OvirtApi::init_from_xml is NULL, so we get a runtime warning, and the parent implementation is not used.
Review of attachment 351678 [details] [review]: ::: govirt/ovirt-storage-domain.c @@ +282,3 @@ + GType type, + const char *value_str, + RestXmlNode *node) Fwiw, the API is slightly odd, with 2 "redundant" args, but only one must be set at the same time. This deserves a short explanatory comment/or some check when entering the function. @@ +294,3 @@ + /* All other types require valid value_str */ + if (value_str == NULL) + return FALSE; This is a slight change of behaviour, before OVIRT_TYPE_RESOURCE also required a non-NULL value_str even though this was not needed/wasteful. @@ +335,3 @@ { + const char *value_str; + GValue value = { 0, }; I don't think this needs to be moved?
Review of attachment 351678 [details] [review]: Overall looks fine.
Review of attachment 351679 [details] [review]: ::: govirt/ovirt-storage-domain.c @@ +326,3 @@ const char *prop_name; + GType type; + const char *xml_path; Fwiw, I would only move this around when adding xml_attr. @@ +358,3 @@ OvirtXmlElement storage_domain_elements[] = { + { .prop_name = "type", + .type = OVIRT_TYPE_STORAGE_DOMAIN_TYPE, Not related to this patch, nor to this patch series, but I'm wondering if we could use g_object_class_find_property/G_PARAM_SPEC_TYPE() (at least optionnally) to get rid of the requirement to specify the type (once again, just thinking out loud, no changes needed there :) @@ +385,3 @@ + .xml_path = "status/state", + }, + { NULL , } I think I'd keep the G_TYPE_INVALID there.
Review of attachment 351680 [details] [review]: "performed", not "perforemd" Too many of your commits logs have an "Also, do XXX and YYY", or in this case "There were a couple of tweaks". Far too often, this indicates something that does not really belong in the commit, but would deserve to be split. I'm mentioning it here, but in this case, it's probably fine ;) ::: govirt/ovirt-resource.h @@ +74,3 @@ + GType type; + const char *xml_path; +}; ovirt-resource.h is a public header, I'm not convinced we want to expose it here. ::: govirt/ovirt-storage-domain.c @@ -57,3 @@ -ovirt_storage_domain_refresh_from_xml(OvirtStorageDomain *domain, - RestXmlNode *node); - I would keep _refresh_from_xml separate from _init_from_xml as this is more flexible if in the future we want to update the content of a preexisting OvirtResource object. Since such an API has not materialized in the last years, I won't argue too much to keep it ;)
Review of attachment 351680 [details] [review]: ::: govirt/ovirt-storage-domain.c @@ -57,3 @@ -ovirt_storage_domain_refresh_from_xml(OvirtStorageDomain *domain, - RestXmlNode *node); - Ah, actually this is ovirt_resource_refresh() which uses ovirt_resource_init_from_xml()
Review of attachment 351681 [details] [review]: This is an ABI break, and I don't really understand how the proposed API change is supposed to be used. ::: govirt/ovirt-resource.h @@ +58,3 @@ gboolean (*init_from_xml)(OvirtResource *resource, RestXmlNode *node, + OvirtXmlElement *elements, This is a public header, so an ABI break. ::: govirt/ovirt-storage-domain.c @@ +176,2 @@ parent_class = OVIRT_RESOURCE_CLASS(ovirt_storage_domain_parent_class); + return parent_class->init_from_xml(resource, node, storage_domain_elements, error); We did not do anything with the passed in "elements", we silently drop it, and pass storage_domain_elements to the parent class? In general, I would not expect the OvirtXmlElement to be something the parent class needs. If we have an OvirtFooStorageDomain inheriting from OvirtStorageDomain, then OvirtFooStorageDomain will set its specialized properties using its own OvirtXmlElement, then it will call OvirtStorageDomain::init_from_xml, which will set more properties on the current object using its own set of OvirtXmlElement, then it will call the parent class vfunc, which may have another set of OvirtXmlElement it needs to parse and so on.
Review of attachment 351682 [details] [review]: .
Review of attachment 351749 [details] [review]: Removing the argument would give a trivial patch. Changing the type of the returned value would imo fit better in the next commit. Or is it making that next commit too complicated?
Review of attachment 351684 [details] [review]: .
Review of attachment 351685 [details] [review]: .
Review of attachment 351690 [details] [review]: This is discussed in comment #70 (In reply to Eduardo Lima (Etrunko) from comment #42) > (In reply to Christophe Fergeau from comment #30) > > Review of attachment 351123 [details] [review] [review] [review] [review]: > > > > Why? This deserves an explanation in the commit log, this looks like it > > would break ovirt_resource_refresh(ovirt_api_new()); > > I am not sure why would it break. All ovirt_api_init_from_xml() function > does is to call the parent function, notice the #if 0 block. ovirt_resource_refresh() calls ovirt_resource_init_from_xml() which does: (both in git master and after your patches) g_return_val_if_fail(klass->init_from_xml != NULL, FALSE); Before your patch, OvirtApi::init_from_xml is non-NULL, so the parent implementation is called. After your patch, OvirtApi::init_from_xml is NULL, so we get a runtime warning, and the parent implementation is not used.
Review of attachment 351688 [details] [review]: Unrelated to your patch, but really wondering why /api/storagedomains does not have a <link href="xxx/datacenters/" rel="datacenters"/> entry rather than just <data_centers><data_center id="xxx"/></data_centers> This is not even consistent with /api/vm/cluster which has both href and id specified.
(In reply to Christophe Fergeau from comment #70) > (In reply to Eduardo Lima (Etrunko) from comment #42) > > (In reply to Christophe Fergeau from comment #30) > > > Review of attachment 351123 [details] [review] [review] [review] [review]: > > > > > > Why? This deserves an explanation in the commit log, this looks like it > > > would break ovirt_resource_refresh(ovirt_api_new()); > > > > I am not sure why would it break. All ovirt_api_init_from_xml() function > > does is to call the parent function, notice the #if 0 block. > > ovirt_resource_refresh() calls ovirt_resource_init_from_xml() which does: > (both in git master and after your patches) > g_return_val_if_fail(klass->init_from_xml != NULL, FALSE); > > Before your patch, OvirtApi::init_from_xml is non-NULL, so the parent > implementation is called. After your patch, OvirtApi::init_from_xml is NULL, > so we get a runtime warning, and the parent implementation is not used. Oh, I didn't see that before, so I will drop that patch then.
Review of attachment 351687 [details] [review]: Note that we don't use at all the code path reading the node content, just the one reading the attributes. ::: govirt/ovirt-utils.c @@ +127,3 @@ + } + + g_array_append_vals(array, g_strdup(value), 1); g_array_append_val(array, g_strdup(value)); @@ +132,3 @@ + ret = (GStrv) array->data; + g_array_unref(array); + return ret; Could be "return g_array_free(array, FALSE);" @@ +156,3 @@ + } + + g_value_set_boxed(value, strv_value); I think you are going to leak strv_value if you don't use g_value_take_boxed() @@ -165,3 @@ - else - value_str = ovirt_rest_xml_node_get_content_from_path(node, elements->xml_path); - I would have done this in "storage-domain: Factor out property value setting from ovirt_resource_parse_xml()", but no strong feeling.
Review of attachment 351689 [details] [review]: ::: govirt/ovirt-api.c @@ +137,3 @@ } + blank line addition?
Review of attachment 351687 [details] [review]: ::: govirt/ovirt-utils.c @@ +149,3 @@ g_value_set_object(value, resource_value); goto end; + } else if (g_type_is_a(type, G_TYPE_STRV)) { Why not in the switch/case below btw?
Review of attachment 351691 [details] [review]: ::: govirt/ovirt-api.h @@ +64,2 @@ OvirtCollection *ovirt_api_get_storage_domains(OvirtApi *api); +OvirtCollection *ovirt_api_get_storage_domains_search(OvirtApi *api, const char *query); Maybe ovirt_api_search_storage_domains() or similar would be a better name? ::: govirt/ovirt-collection.c @@ +376,3 @@ + + /* strip out {query} substring */ + substr = strstr(link, "{query}"); I'd use g_strrstr() to be 100% sure we match the last occurrence. @@ +378,3 @@ + substr = strstr(link, "{query}"); + if (substr != NULL) + *substr = '\0'; Return NULL if we don't find {search} ? Or do you prefer to wait until _fetch() time to error out? @@ +380,3 @@ + *substr = '\0'; + + link_query = g_strconcat(link, query, NULL); query probably needs to be escaped somehow?
Comment on attachment 351690 [details] [review] Remove unecessary ovirt_api_init_from_xml() >From 1817fcd80cdf556e14f98850d30cf071d9e59adf Mon Sep 17 00:00:00 2001 >From: "Eduardo Lima (Etrunko)" <etrunko@redhat.com> >Date: Fri, 28 Apr 2017 18:29:56 -0300 >Subject: [PATCH 13/17] api: Remove unecessary ovirt_api_init_from_xml() > >Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com> >--- > govirt/ovirt-api.c | 23 ----------------------- > 1 file changed, 23 deletions(-) > >diff --git a/govirt/ovirt-api.c b/govirt/ovirt-api.c >index a3cca0f..33d32c2 100644 >--- a/govirt/ovirt-api.c >+++ b/govirt/ovirt-api.c >@@ -50,26 +50,6 @@ struct _OvirtApiPrivate { > G_DEFINE_TYPE(OvirtApi, ovirt_api, OVIRT_TYPE_RESOURCE); > > >-static gboolean ovirt_api_init_from_xml(OvirtResource *resource, >- RestXmlNode *node, >- OvirtXmlElement *elements, >- GError **error) >-{ >- OvirtResourceClass *parent_class; >-#if 0 >- gboolean parsed_ok; >- >- parsed_ok = ovirt_api_refresh_from_xml(OVIRT_API(resource), node); >- if (!parsed_ok) { >- return FALSE; >- } >-#endif >- parent_class = OVIRT_RESOURCE_CLASS(ovirt_api_parent_class); >- >- return parent_class->init_from_xml(resource, node, elements, error); >-} >- >- > static void ovirt_api_dispose(GObject *object) > { > OvirtApi *api = OVIRT_API(object); >@@ -85,13 +65,10 @@ static void ovirt_api_dispose(GObject *object) > static void ovirt_api_class_init(OvirtApiClass *klass) > { > GObjectClass *object_class = G_OBJECT_CLASS(klass); >- OvirtResourceClass *resource_class = OVIRT_RESOURCE_CLASS(klass); > > g_type_class_add_private(klass, sizeof(OvirtApiPrivate)); > > object_class->dispose = ovirt_api_dispose; >- >- resource_class->init_from_xml = ovirt_api_init_from_xml; > } > > static void ovirt_api_init(G_GNUC_UNUSED OvirtApi *api) >-- >2.9.3 >
(In reply to Christophe Fergeau from comment #71) > Review of attachment 351678 [details] [review] [review]: > > ::: govirt/ovirt-storage-domain.c > @@ +282,3 @@ > + GType type, > + const char *value_str, > + RestXmlNode *node) > > Fwiw, the API is slightly odd, with 2 "redundant" args, but only one must be > set at the same time. This deserves a short explanatory comment/or some > check when entering the function. > This is going to change in future commit, where we take xml path and attr instead of the value itself. > @@ +294,3 @@ > + /* All other types require valid value_str */ > + if (value_str == NULL) > + return FALSE; > > This is a slight change of behaviour, before OVIRT_TYPE_RESOURCE also > required a non-NULL value_str even though this was not needed/wasteful. > I don't see much of a problem with this change. Any objection? > @@ +335,3 @@ > { > + const char *value_str; > + GValue value = { 0, }; > > I don't think this needs to be moved? Just a matter preference I guess. When I first started factoring this function out I wanted to make the loop leaner and I think the first thing I did was to move the declaration to the begin of the function.
(In reply to Christophe Fergeau from comment #73) > Review of attachment 351679 [details] [review] [review]: > > ::: govirt/ovirt-storage-domain.c > @@ +326,3 @@ > const char *prop_name; > + GType type; > + const char *xml_path; > > Fwiw, I would only move this around when adding xml_attr. > Well, I changed it here otherwise I would also the order of the initialization in that future patch.. > @@ +358,3 @@ > OvirtXmlElement storage_domain_elements[] = { > + { .prop_name = "type", > + .type = OVIRT_TYPE_STORAGE_DOMAIN_TYPE, > > Not related to this patch, nor to this patch series, but I'm wondering if we > could use g_object_class_find_property/G_PARAM_SPEC_TYPE() (at least > optionnally) to get rid of the requirement to specify the type (once again, > just thinking out loud, no changes needed there :) It is a simple change, and I did this change in another patch, that could be merged after this series, because it would basically conflict with each and every other patch here. https://github.com/etrunko/libgovirt/commit/c5752a286b532c95b7782145036ecb9b287acb1b > > @@ +385,3 @@ > + .xml_path = "status/state", > + }, > + { NULL , } > > I think I'd keep the G_TYPE_INVALID there. I think it would require to keep the consistency and then explicitly initialize all other fields then. In my opinion it looks cleaner than the code below: + { .prop_name = NULL, + .type = G_TYPE_INVALID, + .xml_path = NULL, + }, But of course can be changed without problems.
(In reply to Christophe Fergeau from comment #74) > Review of attachment 351680 [details] [review] [review]: > > "performed", not "perforemd" Fixed. > Too many of your commits logs have an "Also, do XXX and YYY", or in this > case "There were a couple of tweaks". Far too often, this indicates > something that does not really belong in the commit, but would deserve to be > split. I'm mentioning it here, but in this case, it's probably fine ;) I agree that in this case it should stay like it is now, but I splitted the the other commit which was doing more than one thing. > > ::: govirt/ovirt-resource.h > @@ +74,3 @@ > + GType type; > + const char *xml_path; > +}; > > ovirt-resource.h is a public header, I'm not convinced we want to expose it > here. > I moved the struct definition to ovirt-utils.h. > ::: govirt/ovirt-storage-domain.c > @@ -57,3 @@ > -ovirt_storage_domain_refresh_from_xml(OvirtStorageDomain *domain, > - RestXmlNode *node); > - > > I would keep _refresh_from_xml separate from _init_from_xml as this is more > flexible if in the future we want to update the content of a preexisting > OvirtResource object. Since such an API has not materialized in the last > years, I won't argue too much to keep it ;)
(In reply to Christophe Fergeau from comment #76) > Review of attachment 351681 [details] [review] [review]: > > This is an ABI break, and I don't really understand how the proposed API > change is supposed to be used. > > ::: govirt/ovirt-resource.h > @@ +58,3 @@ > gboolean (*init_from_xml)(OvirtResource *resource, > RestXmlNode *node, > + OvirtXmlElement *elements, > > This is a public header, so an ABI break. > > ::: govirt/ovirt-storage-domain.c > @@ +176,2 @@ > parent_class = OVIRT_RESOURCE_CLASS(ovirt_storage_domain_parent_class); > + return parent_class->init_from_xml(resource, node, > storage_domain_elements, error); > > We did not do anything with the passed in "elements", we silently drop it, > and pass storage_domain_elements to the parent class? > In general, I would not expect the OvirtXmlElement to be something the > parent class needs. > If we have an OvirtFooStorageDomain inheriting from OvirtStorageDomain, then > OvirtFooStorageDomain will set its specialized properties using its own > OvirtXmlElement, then it will call OvirtStorageDomain::init_from_xml, which > will set more properties on the current object using its own set of > OvirtXmlElement, then it will call the parent class vfunc, which may have > another set of OvirtXmlElement it needs to parse and so on. Dropped the patch and now explicitly call ovirt_rest_xml_node_parse where it is necessary.
(In reply to Christophe Fergeau from comment #84) > Review of attachment 351687 [details] [review] [review]: > > Note that we don't use at all the code path reading the node content, just > the one reading the attributes. > > ::: govirt/ovirt-utils.c > @@ +127,3 @@ > + } > + > + g_array_append_vals(array, g_strdup(value), 1); > > g_array_append_val(array, g_strdup(value)); Can not be done, see documentation: "g_array_append_val() is a macro which uses a reference to the value parameter v . This means that you cannot use it with literal values such as "27". You must use variables." > > @@ +132,3 @@ > + ret = (GStrv) array->data; > + g_array_unref(array); > + return ret; > > Could be "return g_array_free(array, FALSE);" > Fixed. > @@ +156,3 @@ > + } > + > + g_value_set_boxed(value, strv_value); > > I think you are going to leak strv_value if you don't use > g_value_take_boxed() > Fixed. > @@ -165,3 @@ > - else > - value_str = ovirt_rest_xml_node_get_content_from_path(node, > elements->xml_path); > - > > I would have done this in "storage-domain: Factor out property value setting > from ovirt_resource_parse_xml()", but no strong feeling.
(In reply to Christophe Fergeau from comment #78) > Review of attachment 351749 [details] [review] [review]: > > Removing the argument would give a trivial patch. Changing the type of the > returned value would imo fit better in the next commit. Or is it making that > next commit too complicated? The thing is that by removing the arg I need to change the while block to remove its usage, and next commit would also change those same lines by changing the GStrv parameter to a char *. It can be done, but I don't really know if it is worth looking at the history as a whole.
(In reply to Christophe Fergeau from comment #85) > Review of attachment 351689 [details] [review] [review]: > > ::: govirt/ovirt-api.c > @@ +137,3 @@ > } > > + > > blank line addition? fixed
(In reply to Christophe Fergeau from comment #87) > Review of attachment 351687 [details] [review] [review]: > > ::: govirt/ovirt-utils.c > @@ +149,3 @@ > g_value_set_object(value, resource_value); > goto end; > + } else if (g_type_is_a(type, G_TYPE_STRV)) { > > Why not in the switch/case below btw? This can not be done, because G_TYPE_STRV is a boxed type. The macro translates to g_strv_get_type() and not a value in enum. From documentation: #define G_TYPE_STRV (g_strv_get_type ()) The GType for a boxed type holding a NULL-terminated array of strings.
(In reply to Christophe Fergeau from comment #88) > Review of attachment 351691 [details] [review] [review]: > > ::: govirt/ovirt-api.h > @@ +64,2 @@ > OvirtCollection *ovirt_api_get_storage_domains(OvirtApi *api); > +OvirtCollection *ovirt_api_get_storage_domains_search(OvirtApi *api, const > char *query); > > Maybe ovirt_api_search_storage_domains() or similar would be a better name? > Yes, it looks better indeed, fixed. > ::: govirt/ovirt-collection.c > @@ +376,3 @@ > + > + /* strip out {query} substring */ > + substr = strstr(link, "{query}"); > > I'd use g_strrstr() to be 100% sure we match the last occurrence. Fixed. > > @@ +378,3 @@ > + substr = strstr(link, "{query}"); > + if (substr != NULL) > + *substr = '\0'; > > Return NULL if we don't find {search} ? Or do you prefer to wait until > _fetch() time to error out? > I don't really know, I think this "{query}" placeholder that is returned is really useless, it is there just to make things more complex. I am far from being an expert in REST APIs, but in the end I would return the value and let users append directly. > @@ +380,3 @@ > + *substr = '\0'; > + > + link_query = g_strconcat(link, query, NULL); > > query probably needs to be escaped somehow? I also don't know, the change I did in virt-viewer is very simple, a query for "id". - vms = ovirt_api_get_vms(menu->priv->api); + char *query = g_strdup_printf("id=%s", menu->priv->vm_guid); + vms = ovirt_api_get_vms_search(menu->priv->api, query); + g_free(query); So in the end the call would be like: ovirt-engine/api/vms?search=id=3116bc76-ce36-4ad4-beb2-6afa0d33dfdb I also tested with: serch=id="3116bc76-ce36-4ad4-beb2-6afa0d33dfdb" does not work search="id='3116bc76-ce36-4ad4-beb2-6afa0d33dfdb'" also does not work, and neither it does by escaping the ' symbol with "e; or '
Now that I have a new series of patches to be submitted, and I really don't know what changed from the last time, I am submitting everything again. Unfortunately this is a big shortcoming from this bugzilla review interface. The major change was that I dropped the patch that would change the signature of init_from_xml() virtual function to receive a pointer OvirtXmlElement struct. The current status (before new submission) is the following: Factor out property value setting from ovirt_resource_parse_xml() reviewed > Use explicit initialization of struct OvirtXmlElement members accepted-commit_now > Move out ovirt_resource_parse_xml() to ovirt-utils accepted-commit_now > Remove unused function ovirt_rest_xml_node_get_content() accepted-commit_now Rename ovirt_rest_xml_node_get_content_va() to ovirt_rest_xml_node_find() none > Retrieve node attributes in ovirt_resource_parse_xml() accepted-commit_now > Support G_TYPE_STRING in _set_property_value_from_type() accepted-commit_now Retrieve host and cluster ids none Support G_TYPE_STRV in _set_property_value_from_type() reviewed Retrieve data center ids none > Introduce auxiliary function ovirt_sub_collection_new_from_resource() accepted-commit_now New API functions to enable search queries of collections reviewed Implement support for hosts none Implement support for clusters none Implement support for data centers none But I would actually take a look in all patches. Again, I pushed the series to my github tree, which can make the visualization easier: https://github.com/GNOME/libgovirt/compare/master...etrunko:master Hopefully the plans to migrate the repositories to a hosted Gitlab instance will soon come into reality and this won't happen anymore.
Created attachment 352124 [details] [review] Factor out property value setting from ovirt_resource_parse_xml()
Created attachment 352125 [details] [review] Use explicit initialization of struct OvirtXmlElement members
Created attachment 352126 [details] [review] Move out ovirt_resource_parse_xml() to ovirt-utils
Created attachment 352127 [details] [review] Remove unused function ovirt_rest_xml_node_get_content()
Created attachment 352128 [details] [review] Rename ovirt_rest_xml_node_get_content_va() to ovirt_rest_xml_node_find()
Created attachment 352129 [details] [review] Introduce function ovirt_rest_xml_node_get_attr_from_path()
Created attachment 352130 [details] [review] Retrieve node attributes in ovirt_resource_parse_xml()
Created attachment 352131 [details] [review] Support G_TYPE_STRING in _set_property_value_from_type()
Created attachment 352132 [details] [review] Retrieve host and cluster ids
Created attachment 352133 [details] [review] Support G_TYPE_STRV in _set_property_value_from_type()
Created attachment 352134 [details] [review] Retrieve data center ids
Created attachment 352135 [details] [review] Introduce auxiliary function ovirt_sub_collection_new_from_resource()
Created attachment 352136 [details] [review] New API functions to enable search queries of collections
Created attachment 352137 [details] [review] Implement support for hosts
Created attachment 352138 [details] [review] Implement support for clusters
Created attachment 352139 [details] [review] Implement support for data centers
(In reply to Eduardo Lima (Etrunko) from comment #90) > (In reply to Christophe Fergeau from comment #71) > > Review of attachment 351678 [details] [review] [review] [review]: > > > > ::: govirt/ovirt-storage-domain.c > > @@ +282,3 @@ > > + GType type, > > + const char *value_str, > > + RestXmlNode *node) > > > > Fwiw, the API is slightly odd, with 2 "redundant" args, but only one must be > > set at the same time. This deserves a short explanatory comment/or some > > check when entering the function. > > > > This is going to change in future commit, where we take xml path and attr > instead of the value itself. > > > @@ +294,3 @@ > > + /* All other types require valid value_str */ > > + if (value_str == NULL) > > + return FALSE; > > > > This is a slight change of behaviour, before OVIRT_TYPE_RESOURCE also > > required a non-NULL value_str even though this was not needed/wasteful. > > > > I don't see much of a problem with this change. Any objection? The change by itself is probably not a problem. My small objection is we have a commit with quite a bit of code movement, which should be refactoring (ie no behaviour change), and in the middle of it, there a subtle behaviour change. The kind of commit I don't like to end up on when git bisecting :) This is the only reason why I mentioned this. > > > @@ +335,3 @@ > > { > > + const char *value_str; > > + GValue value = { 0, }; > > > > I don't think this needs to be moved? > > Just a matter preference I guess. When I first started factoring this > function out I wanted to make the loop leaner and I think the first thing I > did was to move the declaration to the begin of the function. In general, I believe the smaller the scope of variable, the better (the less variables you have to keep track of in a block of code, the easier it is to read). Hence the comment.
(In reply to Eduardo Lima (Etrunko) from comment #91) > (In reply to Christophe Fergeau from comment #73) > > Review of attachment 351679 [details] [review] [review] [review]: > > > > ::: govirt/ovirt-storage-domain.c > > @@ +326,3 @@ > > const char *prop_name; > > + GType type; > > + const char *xml_path; > > > > Fwiw, I would only move this around when adding xml_attr. > > > > Well, I changed it here otherwise I would also the order of the > initialization in that future patch.. Oh, since this commit adds named initializers, I don't think changing the order now or later is going to cause a problem. (ie I don't think order of initializers has to match order of fields in the struct).
(In reply to Eduardo Lima (Etrunko) from comment #94) > (In reply to Christophe Fergeau from comment #84) > > Review of attachment 351687 [details] [review] [review] [review]: > > > > Note that we don't use at all the code path reading the node content, just > > the one reading the attributes. > > > > ::: govirt/ovirt-utils.c > > @@ +127,3 @@ > > + } > > + > > + g_array_append_vals(array, g_strdup(value), 1); > > > > g_array_append_val(array, g_strdup(value)); > > Can not be done, see documentation: > > "g_array_append_val() is a macro which uses a reference to the value > parameter v . This means that you cannot use it with literal values such as > "27". You must use variables." Hmm, I would expect a pointer to a char * to be passed to g_array_append_vals() in that case, not a char * as you did "a pointer to the elements to append to the end of the array."
Review of attachment 351691 [details] [review]: Does not really matter, but bit of background on this "{query}" 'thing' ::: govirt/ovirt-collection.c @@ +380,3 @@ + *substr = '\0'; + + link_query = g_strconcat(link, query, NULL); Double-checked what spice-gtk is doing for 'username', and I think what we need to use is g_uri_escape_string(query).
Review of attachment 351691 [details] [review]: ::: govirt/ovirt-collection.c @@ +380,3 @@ + *substr = '\0'; + + link_query = g_strconcat(link, query, NULL); having second thoughts there, if the query is id=foo, then the part which needs escaping would be just "foo" I think, not the 'id' part :-/ Maybe just escaping everything, but telling g_uri_escape to ignore "=" will be fine as a first approximation.
Review of attachment 352126 [details] [review]: Just one comment. ::: govirt/ovirt-storage-domain.c @@ +176,3 @@ + return ovirt_rest_xml_node_parse(node, G_OBJECT(resource), storage_domain_elements) && + parent_class->init_from_xml(resource, node, error); I don't think sticking these 2 function calls on the same line brings much, the previous code is imo much more readable.
Review of attachment 352129 [details] [review]: I would not have split this, but kept it together with the next patch ;) (both are sufficiently small)
Review of attachment 352133 [details] [review]: ::: govirt/ovirt-utils.c @@ +101,3 @@ + GHashTableIter iter; + gpointer sub_node; + const char *value; This can go in the inner block. @@ +123,3 @@ + } + + g_array_append_vals(array, g_strdup(value), 1); See my comment in https://bugzilla.gnome.org/show_bug.cgi?id=765297#c118 I would expect a pointer to a char * to be passed to g_array_append_vals() in that case, not a char * as you did g_array_append_vals() API doc says: "a pointer to the elements to append to the end of the array."
Review of attachment 352136 [details] [review]: ::: govirt/ovirt-api.c @@ +139,3 @@ + * ovirt_api_search_vms: + * @api: a #OvirtApi + * @query: search query Any link documenting what format query should follow? Would we want someting a little by higher level like ovirt_api_search_vms_by_id, ovirt_api_search_vm_by_name, .. .?
Review of attachment 352124 [details] [review]: .
Review of attachment 352125 [details] [review]: .
Review of attachment 352127 [details] [review]: .
Review of attachment 352128 [details] [review]: .
Review of attachment 352129 [details] [review]: .
Review of attachment 352130 [details] [review]: .
Review of attachment 352131 [details] [review]: .
Review of attachment 352135 [details] [review]: .
Review of attachment 352132 [details] [review]: Wondering if it would be better API-wise to return an empty OvirtHost/OvirtCluster instance with only id/href set, and document that ovirt_resource_refresh() needs to be called on these before using them? Or do we have to use these new _search() APIs to go from id to the corresponding OvirtResource?
Review of attachment 352134 [details] [review]: Still would prefer if we could expose this as an OvirtCollection, but the REST API does not really make that easy... I tried asking on IRC about "Unrelated to your patch, but really wondering why /api/storagedomains does not have a <link href="xxx/datacenters/" rel="datacenters"/> entry rather than just <data_centers><data_center id="xxx"/></data_centers>", but did not get ana answer, maybe I'll file a bug.
Review of attachment 352134 [details] [review]: ::: govirt/ovirt-storage-domain.c @@ +125,3 @@ + case PROP_DATA_CENTER_IDS: + g_strfreev(domain->priv->data_center_ids); + domain->priv->data_center_ids = g_value_get_boxed(value); I think this should be g_value_dup_boxed().
Review of attachment 352137 [details] [review]: ::: govirt/ovirt-host.c @@ +1,2 @@ +/* + * ovirt-host.c: oVirt storage host handling s/storage// @@ +104,3 @@ + + return ovirt_rest_xml_node_parse(node, G_OBJECT(resource), host_elements) && + parent_class->init_from_xml(resource, node, error); Please don't put all of this on the same line. @@ +121,3 @@ + object_class->set_property = ovirt_host_set_property; + + param_spec = g_param_spec_string("cluster-id", Still same questioning as to how to expose such ids in the API, either as the raw ID with the library user having to do the lookup, or as OvirtResource that the user could just fetch. @@ +164,3 @@ + * Gets a #OvirtCollection representing the list of remote vms from a + * host object. This method does not initiate any network + * activity, the remote cdrom list must be then be fetched using s/cdrom/host/ ::: govirt/ovirt-host.h @@ +1,2 @@ +/* + * ovirt-host.h: oVirt storage domain resource s/storage domain/host
Review of attachment 352138 [details] [review]: ::: govirt/govirt.sym @@ +114,3 @@ GOVIRT_0.4.0 { + ovirt_api_get_clusters; + ovirt_api_search_clusters; I would group this together with the other search functions? ::: govirt/ovirt-cluster-private.h @@ +1,2 @@ +/* + * ovirt-cluster-private.h: oVirt storage domain resource s/storage domain/cluster/ ::: govirt/ovirt-cluster.c @@ +1,2 @@ +/* + * ovirt-cluster.c: oVirt storage cluster handling s/storage// @@ +104,3 @@ + + return ovirt_rest_xml_node_parse(node, G_OBJECT(resource), cluster_elements) && + parent_class->init_from_xml(resource, node, error); Please split @@ +163,3 @@ + * + * Gets a #OvirtCollection representing the list of remote hosts from a + * cluster object. This method does not initiate any network Extra space before 'This' @@ +164,3 @@ + * Gets a #OvirtCollection representing the list of remote hosts from a + * cluster object. This method does not initiate any network + * activity, the remote cdrom list must be then be fetched using s/cdrom/host ::: govirt/ovirt-cluster.h @@ +1,2 @@ +/* + * ovirt-cluster.h: oVirt storage domain resource s/storage domain/cluster
Review of attachment 352139 [details] [review]: ::: govirt/govirt.sym @@ +116,3 @@ ovirt_api_search_clusters; + ovirt_api_get_data_centers; + ovirt_api_search_data_centers; I would move this with the other search_ methods ::: govirt/ovirt-data-center-private.h @@ +1,2 @@ +/* + * ovirt-data_center-private.h: oVirt storage domain resource "storage domain" ::: govirt/ovirt-data-center.c @@ +1,2 @@ +/* + * ovirt-data_center.c: oVirt storage data_center handling storage data_center @@ +86,3 @@ +/** + * ovirt_data_center_get_clusters: + * @data_center: a #Ovirtdata_center Ovirtdata_center @@ +90,3 @@ + * Gets a #OvirtCollection representing the list of remote clusters from a + * data center object. This method does not initiate any network + * activity, the remote cdrom list must be then be fetched using cdrom @@ +116,3 @@ +/** + * ovirt_data_center_get_storage_domains: + * @data_center: a #Ovirtdata_center Ovirtdata_center @@ +118,3 @@ + * @data_center: a #Ovirtdata_center + * + * Gets a #OvirtCollection representing the list of remote storage_domains from a s/storage_domains/storage domains/ ? @@ +120,3 @@ + * Gets a #OvirtCollection representing the list of remote storage_domains from a + * data center object. This method does not initiate any network + * activity, the remote cdrom list must be then be fetched using cdrom @@ +124,3 @@ + * + * Return value: (transfer none): a #OvirtCollection representing the list + * of storage_domains associated with @data_center. Same as above
(In reply to Christophe Fergeau from comment #116) > (In reply to Eduardo Lima (Etrunko) from comment #90) > > (In reply to Christophe Fergeau from comment #71) > > > Review of attachment 351678 [details] [review] [review] [review] [review]: > > > > > > @@ +335,3 @@ > > > { > > > + const char *value_str; > > > + GValue value = { 0, }; > > > > > > I don't think this needs to be moved? > > > > Just a matter preference I guess. When I first started factoring this > > function out I wanted to make the loop leaner and I think the first thing I > > did was to move the declaration to the begin of the function. > > In general, I believe the smaller the scope of variable, the better (the > less variables you have to keep track of in a block of code, the easier it > is to read). Hence the comment. Okay, changed the patch and kept those variables declared inside the loop.
(In reply to Christophe Fergeau from comment #118) > (In reply to Eduardo Lima (Etrunko) from comment #94) > > (In reply to Christophe Fergeau from comment #84) > > > Review of attachment 351687 [details] [review] [review] [review] [review]: > > > > > > Note that we don't use at all the code path reading the node content, just > > > the one reading the attributes. > > > > > > ::: govirt/ovirt-utils.c > > > @@ +127,3 @@ > > > + } > > > + > > > + g_array_append_vals(array, g_strdup(value), 1); > > > > > > g_array_append_val(array, g_strdup(value)); > > > > Can not be done, see documentation: > > > > "g_array_append_val() is a macro which uses a reference to the value > > parameter v . This means that you cannot use it with literal values such as > > "27". You must use variables." > > Hmm, I would expect a pointer to a char * to be passed to > g_array_append_vals() in that case, not a char * as you did > "a pointer to the elements to append to the end of the array." Oh, that was indeed a mistake, thanks. Fixed now.
(In reply to Christophe Fergeau from comment #120) > Review of attachment 351691 [details] [review] [review]: > > ::: govirt/ovirt-collection.c > @@ +380,3 @@ > + *substr = '\0'; > + > + link_query = g_strconcat(link, query, NULL); > > having second thoughts there, if the query is id=foo, then the part which > needs escaping would be just "foo" I think, not the 'id' part :-/ > Maybe just escaping everything, but telling g_uri_escape to ignore "=" will > be fine as a first approximation. I looked at the REST API guide, and there is not much information about escaping, but I think it should be the whole query string, including the '=' symbol. Reference section 5.56.2.4: https://access.redhat.com/documentation/en-us/red_hat_virtualization/4.1/html-single/rest_api_guide/#services-events-methods-list
(In reply to Christophe Fergeau from comment #121) > Review of attachment 352126 [details] [review] [review]: > > Just one comment. > > ::: govirt/ovirt-storage-domain.c > @@ +176,3 @@ > > + return ovirt_rest_xml_node_parse(node, G_OBJECT(resource), > storage_domain_elements) && > + parent_class->init_from_xml(resource, node, error); > > I don't think sticking these 2 function calls on the same line brings much, > the previous code is imo much more readable. Okay, I put it back the way it was.
(In reply to Christophe Fergeau from comment #122) > Review of attachment 352129 [details] [review] [review]: > > I would not have split this, but kept it together with the next patch ;) > (both are sufficiently small) Done.
(In reply to Christophe Fergeau from comment #123) > Review of attachment 352133 [details] [review] [review]: > > ::: govirt/ovirt-utils.c > @@ +101,3 @@ > + GHashTableIter iter; > + gpointer sub_node; > + const char *value; > > This can go in the inner block. > Done. > @@ +123,3 @@ > + } > + > + g_array_append_vals(array, g_strdup(value), 1); > > See my comment in https://bugzilla.gnome.org/show_bug.cgi?id=765297#c118 > I would expect a pointer to a char * to be passed to g_array_append_vals() > in that case, not a char * as you did > g_array_append_vals() API doc says: "a pointer to the elements to append to > the end of the array." Thanks, as I said in comment 140, this is now fixed.
(In reply to Christophe Fergeau from comment #124) > Review of attachment 352136 [details] [review] [review]: > > ::: govirt/ovirt-api.c > @@ +139,3 @@ > + * ovirt_api_search_vms: > + * @api: a #OvirtApi > + * @query: search query > > Any link documenting what format query should follow? > Would we want someting a little by higher level like > ovirt_api_search_vms_by_id, ovirt_api_search_vm_by_name, .. .? I could not really find specific documentation about it, only something related to it as noted in comment 141.
(In reply to Christophe Fergeau from comment #133) > Review of attachment 352132 [details] [review] [review]: > > Wondering if it would be better API-wise to return an empty > OvirtHost/OvirtCluster instance with only id/href set, and document that > ovirt_resource_refresh() needs to be called on these before using them? > Or do we have to use these new _search() APIs to go from id to the > corresponding OvirtResource? I think it would make sense if we had the link to the resources mentioned, not only the id as it is done at the moment.
(In reply to Christophe Fergeau from comment #135) > Review of attachment 352134 [details] [review] [review]: > > ::: govirt/ovirt-storage-domain.c > @@ +125,3 @@ > + case PROP_DATA_CENTER_IDS: > + g_strfreev(domain->priv->data_center_ids); > + domain->priv->data_center_ids = g_value_get_boxed(value); > > I think this should be g_value_dup_boxed(). Fixed.
(In reply to Christophe Fergeau from comment #136) > Review of attachment 352137 [details] [review] [review]: > > ::: govirt/ovirt-host.c > @@ +1,2 @@ > +/* > + * ovirt-host.c: oVirt storage host handling > > s/storage// > > @@ +104,3 @@ > + > + return ovirt_rest_xml_node_parse(node, G_OBJECT(resource), > host_elements) && > + parent_class->init_from_xml(resource, node, error); > > Please don't put all of this on the same line. > > @@ +121,3 @@ > + object_class->set_property = ovirt_host_set_property; > + > + param_spec = g_param_spec_string("cluster-id", > > Still same questioning as to how to expose such ids in the API, either as > the raw ID with the library user having to do the lookup, or as > OvirtResource that the user could just fetch. > > @@ +164,3 @@ > + * Gets a #OvirtCollection representing the list of remote vms from a > + * host object. This method does not initiate any network > + * activity, the remote cdrom list must be then be fetched using > > s/cdrom/host/ > > ::: govirt/ovirt-host.h > @@ +1,2 @@ > +/* > + * ovirt-host.h: oVirt storage domain resource > > s/storage domain/host All fixed.
(In reply to Christophe Fergeau from comment #137) > Review of attachment 352138 [details] [review] [review]: > > ::: govirt/govirt.sym > @@ +114,3 @@ > GOVIRT_0.4.0 { > + ovirt_api_get_clusters; > + ovirt_api_search_clusters; > > I would group this together with the other search functions? > > ::: govirt/ovirt-cluster-private.h > @@ +1,2 @@ > +/* > + * ovirt-cluster-private.h: oVirt storage domain resource > > s/storage domain/cluster/ > > ::: govirt/ovirt-cluster.c > @@ +1,2 @@ > +/* > + * ovirt-cluster.c: oVirt storage cluster handling > > s/storage// > > @@ +104,3 @@ > + > + return ovirt_rest_xml_node_parse(node, G_OBJECT(resource), > cluster_elements) && > + parent_class->init_from_xml(resource, node, error); > > Please split > > @@ +163,3 @@ > + * > + * Gets a #OvirtCollection representing the list of remote hosts from a > + * cluster object. This method does not initiate any network > > Extra space before 'This' > > @@ +164,3 @@ > + * Gets a #OvirtCollection representing the list of remote hosts from a > + * cluster object. This method does not initiate any network > + * activity, the remote cdrom list must be then be fetched using > > s/cdrom/host > > ::: govirt/ovirt-cluster.h > @@ +1,2 @@ > +/* > + * ovirt-cluster.h: oVirt storage domain resource > > s/storage domain/cluster All fixed.
(In reply to Christophe Fergeau from comment #138) > Review of attachment 352139 [details] [review] [review]: > > ::: govirt/govirt.sym > @@ +116,3 @@ > ovirt_api_search_clusters; > + ovirt_api_get_data_centers; > + ovirt_api_search_data_centers; > > I would move this with the other search_ methods > > ::: govirt/ovirt-data-center-private.h > @@ +1,2 @@ > +/* > + * ovirt-data_center-private.h: oVirt storage domain resource > > "storage domain" > > ::: govirt/ovirt-data-center.c > @@ +1,2 @@ > +/* > + * ovirt-data_center.c: oVirt storage data_center handling > > storage data_center > > @@ +86,3 @@ > +/** > + * ovirt_data_center_get_clusters: > + * @data_center: a #Ovirtdata_center > > Ovirtdata_center > > @@ +90,3 @@ > + * Gets a #OvirtCollection representing the list of remote clusters from a > + * data center object. This method does not initiate any network > + * activity, the remote cdrom list must be then be fetched using > > cdrom > > @@ +116,3 @@ > +/** > + * ovirt_data_center_get_storage_domains: > + * @data_center: a #Ovirtdata_center > > Ovirtdata_center > > @@ +118,3 @@ > + * @data_center: a #Ovirtdata_center > + * > + * Gets a #OvirtCollection representing the list of remote storage_domains > from a > > s/storage_domains/storage domains/ ? > > @@ +120,3 @@ > + * Gets a #OvirtCollection representing the list of remote storage_domains > from a > + * data center object. This method does not initiate any network > + * activity, the remote cdrom list must be then be fetched using > > cdrom > > @@ +124,3 @@ > + * > + * Return value: (transfer none): a #OvirtCollection representing the list > + * of storage_domains associated with @data_center. > > Same as above All fixed.
Created attachment 354267 [details] [review] Factor out property value setting from ovirt_resource_parse_xml()
Created attachment 354269 [details] [review] Use explicit initialization of struct OvirtXmlElement members
Created attachment 354270 [details] [review] Move out ovirt_resource_parse_xml() to ovirt-utils
Created attachment 354271 [details] [review] Remove unused function ovirt_rest_xml_node_get_content()
Created attachment 354272 [details] [review] Rename ovirt_rest_xml_node_get_content_va() to ovirt_rest_xml_node_find()
Created attachment 354273 [details] [review] Retrieve node attributes in ovirt_resource_parse_xml()
Created attachment 354274 [details] [review] Support G_TYPE_STRING in _set_property_value_from_type()
Created attachment 354275 [details] [review] Retrieve host and cluster ids
Created attachment 354276 [details] [review] Support G_TYPE_STRV in _set_property_value_from_type()
Created attachment 354277 [details] [review] Retrieve data center ids
Created attachment 354278 [details] [review] Introduce auxiliary function ovirt_sub_collection_new_from_resource()
Created attachment 354279 [details] [review] New API functions to enable search queries of collections
Created attachment 354280 [details] [review] Implement support for hosts
Created attachment 354281 [details] [review] Implement support for clusters
Created attachment 354282 [details] [review] Implement support for data centers
(In reply to Eduardo Lima (Etrunko) from comment #146) > (In reply to Christophe Fergeau from comment #133) > > Review of attachment 352132 [details] [review] [review] [review]: > > > > Wondering if it would be better API-wise to return an empty > > OvirtHost/OvirtCluster instance with only id/href set, and document that > > ovirt_resource_refresh() needs to be called on these before using them? > > Or do we have to use these new _search() APIs to go from id to the > > corresponding OvirtResource? > > I think it would make sense if we had the link to the resources mentioned, > not only the id as it is done at the moment. Yes, but isn't this a bug in the REST API that it gives us an id rather than a link to the resource?
Review of attachment 354269 [details] [review]: Not sure what happened here, but the previous version ( https://bug765297.bugzilla-attachments.gnome.org/attachment.cgi?id=352125 ) built fine, while this one is missing the ovirt_resource_parse_xml changes.
Created attachment 354623 [details] [review] Use explicit initialization of struct OvirtXmlElement members
(In reply to Christophe Fergeau from comment #167) > Review of attachment 354269 [details] [review] [review]: > > Not sure what happened here, but the previous version ( > https://bug765297.bugzilla-attachments.gnome.org/attachment.cgi?id=352125 ) > built fine, while this one is missing the ovirt_resource_parse_xml changes. Sorry, build was indeed broken because I moved the variable declaration back to inside of the loop block, and kept the old names. Should be fixed now.
(In reply to Christophe Fergeau from comment #166) > (In reply to Eduardo Lima (Etrunko) from comment #146) > > (In reply to Christophe Fergeau from comment #133) > > > Review of attachment 352132 [details] [review] [review] [review] [review]: > > > > > > Wondering if it would be better API-wise to return an empty > > > OvirtHost/OvirtCluster instance with only id/href set, and document that > > > ovirt_resource_refresh() needs to be called on these before using them? > > > Or do we have to use these new _search() APIs to go from id to the > > > corresponding OvirtResource? > > > > I think it would make sense if we had the link to the resources mentioned, > > not only the id as it is done at the moment. > > Yes, but isn't this a bug in the REST API that it gives us an id rather than > a link to the resource? Totally agree with you here ;)
Review of attachment 354275 [details] [review]: ::: govirt/ovirt-vm.c @@ +155,3 @@ + return ovirt_vm_refresh_from_xml(OVIRT_VM(resource), node) && + ovirt_rest_xml_node_parse(node, G_OBJECT(resource), vm_elements) && + parent_class->init_from_xml(resource, node, error); I'd rather that this gets split.
Created attachment 354628 [details] [review] Retrieve host and cluster ids
(In reply to Christophe Fergeau from comment #171) > Review of attachment 354275 [details] [review] [review]: > > ::: govirt/ovirt-vm.c > @@ +155,3 @@ > + return ovirt_vm_refresh_from_xml(OVIRT_VM(resource), node) && > + ovirt_rest_xml_node_parse(node, G_OBJECT(resource), vm_elements) > && > + parent_class->init_from_xml(resource, node, error); > > I'd rather that this gets split. Fixed
(In reply to Eduardo Lima (Etrunko) from comment #141) > (In reply to Christophe Fergeau from comment #120) > > Review of attachment 351691 [details] [review] [review] [review]: > > > > ::: govirt/ovirt-collection.c > > @@ +380,3 @@ > > + *substr = '\0'; > > + > > + link_query = g_strconcat(link, query, NULL); > > > > having second thoughts there, if the query is id=foo, then the part which > > needs escaping would be just "foo" I think, not the 'id' part :-/ > > Maybe just escaping everything, but telling g_uri_escape to ignore "=" will > > be fine as a first approximation. > > I looked at the REST API guide, and there is not much information about > escaping, but I think it should be the whole query string, including the '=' > symbol. Reference section 5.56.2.4: > https://access.redhat.com/documentation/en-us/red_hat_virtualization/4.1/ > html-single/rest_api_guide/#services-events-methods-list Looks like there is not much information either about what a valid search string would be. Can you file a bug against RHEV documentation so that this is considered?
(In reply to Eduardo Lima (Etrunko) from comment #146) > (In reply to Christophe Fergeau from comment #133) > > Review of attachment 352132 [details] [review] [review] [review]: > > > > Wondering if it would be better API-wise to return an empty > > OvirtHost/OvirtCluster instance with only id/href set, and document that > > ovirt_resource_refresh() needs to be called on these before using them? > > Or do we have to use these new _search() APIs to go from id to the > > corresponding OvirtResource? > > I think it would make sense if we had the link to the resources mentioned, > not only the id as it is done at the moment. Have you reached out to the RHEV folks to see if the link is missing on purpose, or if this a REST API bug? This would be helpful to make the correct decision regarding the API to use :)
Review of attachment 354279 [details] [review]: ::: govirt/ovirt-api.c @@ +141,3 @@ + * @query: search query + * + * Return value: (transfer none): This is not (transfer none) but (transfer full). A detailed documentation string would be nice (do I need ot _fetch()/refresh() or not after calling this? what does a search query looks like?) ::: govirt/ovirt-collection.c @@ +378,3 @@ + * we need to strip out {query} substring. For reference see section 5.194.1 + * of the REST API guide document: + * https://access.redhat.com/documentation/en-us/red_hat_virtualization/4.1/html-single/rest_api_guide/#services-system-methods-get Ah, when I was asking for query string details, I was thinking in API doc. It seems there is not much about it in RHEV documentation, so I guess this will need to be improved later. I'd drop that comment for now. @@ +386,3 @@ + /* Query string must be escaped, see section 5.56.2.4 of the REST API + * guide document for a brief explanation about search query strings: + * https://access.redhat.com/documentation/en-us/red_hat_virtualization/4.1/html-single/rest_api_guide/#services-events-methods-list Same here, since RHEV documentation does not document query strings, the link is not really adding much :-/
Created attachment 354702 [details] [review] New API functions to enable search queries of collections
(In reply to Christophe Fergeau from comment #175) > (In reply to Eduardo Lima (Etrunko) from comment #146) > > (In reply to Christophe Fergeau from comment #133) > > > Review of attachment 352132 [details] [review] [review] [review] [review]: > > > > > > Wondering if it would be better API-wise to return an empty > > > OvirtHost/OvirtCluster instance with only id/href set, and document that > > > ovirt_resource_refresh() needs to be called on these before using them? > > > Or do we have to use these new _search() APIs to go from id to the > > > corresponding OvirtResource? > > > > I think it would make sense if we had the link to the resources mentioned, > > not only the id as it is done at the moment. > > Have you reached out to the RHEV folks to see if the link is missing on > purpose, or if this a REST API bug? This would be helpful to make the > correct decision regarding the API to use :) I filed https://bugzilla.redhat.com/show_bug.cgi?id=1466781 after some discussion with them.
Comment on attachment 354274 [details] [review] Support G_TYPE_STRING in _set_property_value_from_type() >From 8a277789e3dfe57541a489ad58fdccf8dc758f16 Mon Sep 17 00:00:00 2001 >From: "Eduardo Lima (Etrunko)" <etrunko@redhat.com> >Date: Wed, 10 May 2017 15:46:27 -0300 >Subject: [PATCH] utils: Support G_TYPE_STRING in > _set_property_value_from_type() > >Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com> >--- > govirt/ovirt-utils.c | 4 ++++ > 1 file changed, 4 insertions(+) > >diff --git a/govirt/ovirt-utils.c b/govirt/ovirt-utils.c >index b9b7c15..fafb9ec 100644 >--- a/govirt/ovirt-utils.c >+++ b/govirt/ovirt-utils.c >@@ -124,6 +124,10 @@ _set_property_value_from_type(GValue *value, > g_value_set_boolean(value, bool_value); > break; > } >+ case G_TYPE_STRING: { >+ g_value_set_string(value, value_str); >+ break; >+ } > case G_TYPE_UINT64: { > guint64 int64_value = g_ascii_strtoull(value_str, NULL, 0); > g_value_set_uint64(value, int64_value); >-- >2.9.4
Created attachment 355630 [details] [review] Introduce ovirt_resource_new*() functions These functions should be used to replace usage of g_initable_new() around the codebase, as it avoids code duplication by combining all different ways of creating a resource in a single function. Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Created attachment 355631 [details] [review] Use ovirt_resource_new* functions instead of g_initable_new This patch also fix some functions that were supposed to be tagged as G_GNUC_INTERNAL. Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Created attachment 355632 [details] [review] Move resource type definitions to ovirt-types.h With recently added ovirt_resource_new* functions, it is now possible to create any type of resource, being only necessary to pass the type as argument. In order to avoid interdependencies between objects, we need a common place for definition of the resource types. For instance, it should be possible to access a OvirtHost from a OvirtVm and vice-versa. Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Created attachment 355633 [details] [review] Initial support for hosts At the moment, we only care about the information about the cluster the host is part of, and the list of the vms associated with it. Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Created attachment 355634 [details] [review] Initial support for clusters Like the previous commit, at the moment, we only care about the information of the data center the cluster is part of, and the list of the hosts associated with it. Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Created attachment 355635 [details] [review] Initial support for data centers For this higher level object, the list of clusters and storage domains associated with it are stored. Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Created attachment 355636 [details] [review] vm: Introduce ovirt_vm_get_host() With initial support for hosts implemented, this new function can be used to retrieve the host the virtual machine belongs to. Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Created attachment 355637 [details] [review] vm: Introduce ovirt_vm_get_cluster() Similar to previous commit, this new function can be used to retrieve the cluster the virtual machine belongs to. Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Created attachment 355638 [details] [review] host: Introduce ovirt_host_get_cluster() Following the same principle as previous commits, this functions can be used to retrieve the cluster that includes this host. Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Created attachment 355639 [details] [review] cluster: Introduce ovirt_cluster_get_data_center() This function can be used to retrieve the data center associated with the cluster. Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Comment on attachment 354277 [details] [review] Retrieve data center ids This patch remains untouched. It will be applied as the last one of the series.
Review of attachment 355630 [details] [review]: ::: govirt/govirt.sym @@ +117,3 @@ ovirt_api_search_vm_pools; + + ovirt_resource_new; I don't think this needs to be exported? ::: govirt/ovirt-resource.c @@ +1080,3 @@ + va_end(var_args); + + if (local_error != NULL) { If 'error' was non-NULL, it was passed to g_initable_new_valist() and local_error will always be NULL; I'd always pass 'local_error' to g_initable_new_valist() and use g_propagate_error if local_error is non-NULL. @@ +1082,3 @@ + if (local_error != NULL) { + GTypeQuery query; + g_type_query(type, &query); You can use g_type_name() instead.
Review of attachment 355631 [details] [review]: Note that G_GNUC_INTERNAL is purely informative, what matters is t he .syms file.
Review of attachment 355632 [details] [review]: .
Review of attachment 355633 [details] [review]: ::: govirt/govirt.sym @@ +122,3 @@ + ovirt_host_get_type; + ovirt_host_get_vms; + ovirt_host_new; Not directly related to this patch as this is done for most OvirtResource, but I wonder if the ovirt_host_new() needs to be exported ::: govirt/ovirt-host-private.h @@ +2,3 @@ + * ovirt-host-private.h: oVirt storage domain resource + * + * Copyright (C) 2013 Red Hat, Inc. 2017
Review of attachment 355634 [details] [review]: ::: govirt/ovirt-cluster-private.h @@ +2,3 @@ + * ovirt-cluster-private.h: oVirt storage domain resource + * + * Copyright (C) 2013 Red Hat, Inc. can be 2017
Review of attachment 355635 [details] [review]: ::: govirt/ovirt-data-center-private.h @@ +2,3 @@ + * ovirt-data_center-private.h: oVirt storage domain resource + * + * Copyright (C) 2013 Red Hat, Inc. 2017
Review of attachment 355636 [details] [review]: ::: govirt/ovirt-vm.c @@ +79,3 @@ break; + case PROP_HOST_HREF: + g_value_set_string(value, vm->priv->host_href); Maybe we could have the same magic here than in _get_host if host_id is non NULL but this is NULL. @@ +159,3 @@ + if (!ovirt_rest_xml_node_parse(node, G_OBJECT(resource), vm_elements)) + return FALSE; + Now that I pushed for this generic function, I find it a bit cumbersome that we have to add properties we are not really going to use :)
Review of attachment 355637 [details] [review]: ::: govirt/ovirt-vm.c @@ +89,3 @@ break; + case PROP_CLUSTER_HREF: + g_value_set_string(value, vm->priv->cluster_href); Same comment about generating the correct href value when cluster_id is set but not cluster_href
Review of attachment 355639 [details] [review]: .
(In reply to Christophe Fergeau from comment #191) > Review of attachment 355630 [details] [review] [review]: > > ::: govirt/govirt.sym > @@ +117,3 @@ > ovirt_api_search_vm_pools; > + > + ovirt_resource_new; > > I don't think this needs to be exported? > Well, I did the same as other resources, the _new function is exported, but others are not. > ::: govirt/ovirt-resource.c > @@ +1080,3 @@ > + va_end(var_args); > + > + if (local_error != NULL) { > > If 'error' was non-NULL, it was passed to g_initable_new_valist() and > local_error will always be NULL; > I'd always pass 'local_error' to g_initable_new_valist() and use > g_propagate_error if local_error is non-NULL. Fixed. > > @@ +1082,3 @@ > + if (local_error != NULL) { > + GTypeQuery query; > + g_type_query(type, &query); > > You can use g_type_name() instead. Also fixed.
Created attachment 356140 [details] [review] Introduce ovirt_resource_new*() functions These functions should be used to replace usage of g_initable_new() around the codebase, as it avoids code duplication by combining all different ways of creating a resource in a single function. Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
(In reply to Christophe Fergeau from comment #194) > Review of attachment 355633 [details] [review] [review]: > > ::: govirt/ovirt-host-private.h > @@ +2,3 @@ > + * ovirt-host-private.h: oVirt storage domain resource > + * > + * Copyright (C) 2013 Red Hat, Inc. > > 2017 (In reply to Christophe Fergeau from comment #195) > Review of attachment 355634 [details] [review] [review]: > > ::: govirt/ovirt-cluster-private.h > @@ +2,3 @@ > + * ovirt-cluster-private.h: oVirt storage domain resource > + * > + * Copyright (C) 2013 Red Hat, Inc. > > can be 2017 (In reply to Christophe Fergeau from comment #196) > Review of attachment 355635 [details] [review] [review]: > > ::: govirt/ovirt-data-center-private.h > @@ +2,3 @@ > + * ovirt-data_center-private.h: oVirt storage domain resource > + * > + * Copyright (C) 2013 Red Hat, Inc. > > 2017 All copyright notices and descriptions were fixed.
Created attachment 356141 [details] [review] Initial support for hosts At the moment, we only care about the information about the cluster the host is part of, and the list of the vms associated with it. Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Created attachment 356142 [details] [review] Initial support for clusters Like the previous commit, at the moment, we only care about the information of the data center the cluster is part of, and the list of the hosts associated with it. Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Created attachment 356143 [details] [review] Initial support for data centers For this higher level object, the list of clusters and storage domains associated with it are stored. Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
(In reply to Christophe Fergeau from comment #197) > Review of attachment 355636 [details] [review] [review]: > > ::: govirt/ovirt-vm.c > @@ +79,3 @@ > break; > + case PROP_HOST_HREF: > + g_value_set_string(value, vm->priv->host_href); > > Maybe we could have the same magic here than in _get_host if host_id is non > NULL but this is NULL. > Oh yes, I created a new static function that returns returning the href from the id if it is not available. The same applies to the other resources. > @@ +159,3 @@ > + if (!ovirt_rest_xml_node_parse(node, G_OBJECT(resource), vm_elements)) > + return FALSE; > + > > Now that I pushed for this generic function, I find it a bit cumbersome that > we have to add properties we are not really going to use :) (In reply to Christophe Fergeau from comment #198) > Review of attachment 355637 [details] [review] [review]: > > ::: govirt/ovirt-vm.c > @@ +89,3 @@ > break; > + case PROP_CLUSTER_HREF: > + g_value_set_string(value, vm->priv->cluster_href); > > Same comment about generating the correct href value when cluster_id is set > but not cluster_href (In reply to Christophe Fergeau from comment #199) > Review of attachment 355637 [details] [review] [review]: > > ::: govirt/ovirt-vm.c > @@ +89,3 @@ > break; > + case PROP_CLUSTER_HREF: > + g_value_set_string(value, vm->priv->cluster_href); > > Same comment about generating the correct href value when cluster_id is set > but not cluster_href
Created attachment 356144 [details] [review] vm: Introduce ovirt_vm_get_host() With initial support for hosts implemented, this new function can be used to retrieve the host the virtual machine belongs to. Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Created attachment 356145 [details] [review] vm: Introduce ovirt_vm_get_cluster() Similar to previous commit, this new function can be used to retrieve the cluster the virtual machine belongs to. Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Created attachment 356146 [details] [review] host: Introduce ovirt_host_get_cluster() Following the same principle as previous commits, this functions can be used to retrieve the cluster that includes this host. Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Created attachment 356147 [details] [review] cluster: Introduce ovirt_cluster_get_data_center() This function can be used to retrieve the data center associated with the cluster. Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
(In reply to Eduardo Lima (Etrunko) from comment #201) > (In reply to Christophe Fergeau from comment #191) > > Review of attachment 355630 [details] [review] [review] [review]: > > > > ::: govirt/govirt.sym > > @@ +117,3 @@ > > ovirt_api_search_vm_pools; > > + > > + ovirt_resource_new; > > > > I don't think this needs to be exported? > > > > Well, I did the same as other resources, the _new function is exported, but > others are not. But OvirtResource is very special, it's more an abstract base class for all resources rather than something I expect client code to instantiate. So I don't think it's useful to export ovirt_resource_new(). If it's needed one day, we can always export it once we have a use case for it, while if we export it now, unexporting it would break ABI.
Created attachment 356290 [details] [review] Introduce ovirt_resource_new*() functions These functions should be used to replace usage of g_initable_new() around the codebase, as it avoids code duplication by combining all different ways of creating a resource in a single function. Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Review of attachment 356144 [details] [review]: ::: govirt/ovirt-vm.c @@ +73,3 @@ +} + +static char *get_host_href(OvirtVm *vm) This returns const char *, and same comment for all the similar functions.
(In reply to Christophe Fergeau from comment #214) > Review of attachment 356144 [details] [review] [review]: > > ::: govirt/ovirt-vm.c > @@ +73,3 @@ > +} > + > +static char *get_host_href(OvirtVm *vm) > > This returns const char *, and same comment for all the similar functions. All fixed.
Created attachment 356303 [details] [review] vm: Introduce ovirt_vm_get_host() With initial support for hosts implemented, this new function can be used to retrieve the host the virtual machine belongs to. Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Created attachment 356304 [details] [review] vm: Introduce ovirt_vm_get_cluster() Similar to previous commit, this new function can be used to retrieve the cluster the virtual machine belongs to. Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Created attachment 356305 [details] [review] host: Introduce ovirt_host_get_cluster() Following the same principle as previous commits, this functions can be used to retrieve the cluster that includes this host. Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Created attachment 356306 [details] [review] cluster: Introduce ovirt_cluster_get_data_center() This function can be used to retrieve the data center associated with the cluster. Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Review of attachment 354277 [details] [review]: .
Review of attachment 356141 [details] [review]: .
Review of attachment 356142 [details] [review]: .
Review of attachment 356143 [details] [review]: .
Review of attachment 356290 [details] [review]: .
Review of attachment 356303 [details] [review]: .
Review of attachment 356304 [details] [review]: .
Review of attachment 356305 [details] [review]: .
Review of attachment 356306 [details] [review]: .
Review of attachment 354277 [details] [review]: Is this only going to be exposed as a GStrv property? Or should we have some C API to get this as well?
(In reply to Christophe Fergeau from comment #229) > Review of attachment 354277 [details] [review] [review]: > > Is this only going to be exposed as a GStrv property? Or should we have some > C API to get this as well? It can be added, but at the moment, all we need is the Id's so we can compare against the data center.
(In reply to Eduardo Lima (Etrunko) from comment #207) > (In reply to Christophe Fergeau from comment #197) > > Review of attachment 355636 [details] [review] [review] [review]: > > > > ::: govirt/ovirt-vm.c > > @@ +79,3 @@ > > break; > > + case PROP_HOST_HREF: > > + g_value_set_string(value, vm->priv->host_href); > > > > Maybe we could have the same magic here than in _get_host if host_id is non > > NULL but this is NULL. > > > > Oh yes, I created a new static function that returns returning the href from > the id if it is not available. The same applies to the other resources. Sorry for not realizing that earlier, but if I'm not mistaken, this is not really needed for all of these, current and past oVirt REST API properly fill href for these. The only place where we are missing the href is /storage_domains/data_centers (ie anything which is a list of id) > > > @@ +159,3 @@ > > + if (!ovirt_rest_xml_node_parse(node, G_OBJECT(resource), vm_elements)) > > + return FALSE; > > + > > > > Now that I pushed for this generic function, I find it a bit cumbersome that > > we have to add properties we are not really going to use :) > > (In reply to Christophe Fergeau from comment #198) > > Review of attachment 355637 [details] [review] [review] [review]: > > > > ::: govirt/ovirt-vm.c > > @@ +89,3 @@ > > break; > > + case PROP_CLUSTER_HREF: > > + g_value_set_string(value, vm->priv->cluster_href); > > > > Same comment about generating the correct href value when cluster_id is set > > but not cluster_href > > (In reply to Christophe Fergeau from comment #199) > > Review of attachment 355637 [details] [review] [review] [review]: > > > > ::: govirt/ovirt-vm.c > > @@ +89,3 @@ > > break; > > + case PROP_CLUSTER_HREF: > > + g_value_set_string(value, vm->priv->cluster_href); > > > > Same comment about generating the correct href value when cluster_id is set > > but not cluster_href
Created attachment 356981 [details] [review] resource: Remove construct-only restriction for 'xml-node' property It is required since it is now possible to create a resource without a XML node. Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Created attachment 356983 [details] [review] Add missing #include in govirt.h Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Review of attachment 356983 [details] [review]: Committed as trivial.
(In reply to Eduardo Lima (Etrunko) from comment #232) > Created attachment 356981 [details] [review] [review] > resource: Remove construct-only restriction for 'xml-node' property > > It is required since it is now possible to create a resource without a > XML node. > > Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com> Can you review this one?
Review of attachment 356981 [details] [review]: In which usecases is this going to get used?
(In reply to Christophe Fergeau from comment #236) > Review of attachment 356981 [details] [review] [review]: > > In which usecases is this going to get used? In ovirt_vm_get_{host,cluster}() and similar functions, ovirt_host_get_cluster(), ovirt_cluster_get_data_center(), we are returned a resource with only id/href and it is necessary to do a ovirt_resource_refresh(). See patches for virt-viewer in https://www.redhat.com/archives/virt-tools-list/2017-August/msg00121.html
(In reply to Eduardo Lima (Etrunko) from comment #237) > (In reply to Christophe Fergeau from comment #236) > > Review of attachment 356981 [details] [review] [review] [review]: > > > > In which usecases is this going to get used? > > In ovirt_vm_get_{host,cluster}() and similar functions, > ovirt_host_get_cluster(), ovirt_cluster_get_data_center(), we are returned a > resource with only id/href and it is necessary to do a > ovirt_resource_refresh(). See patches for virt-viewer in > https://www.redhat.com/archives/virt-tools-list/2017-August/msg00121.html Ok, ovirt_resource_init_from_xml_real() is doing g_object_set(G_OBJECT(resource), "xml-node", node, NULL); which is not going to work if xml-node is construct-only. So basically ovirt_resource_refresh_from_xml is broken without this patch? (regardless of the recent changes). This should be easy to trigger in a small test case?
(In reply to Christophe Fergeau from comment #238) > (In reply to Eduardo Lima (Etrunko) from comment #237) > > (In reply to Christophe Fergeau from comment #236) > > > Review of attachment 356981 [details] [review] [review] [review] [review]: > > > > > > In which usecases is this going to get used? > > > > In ovirt_vm_get_{host,cluster}() and similar functions, > > ovirt_host_get_cluster(), ovirt_cluster_get_data_center(), we are returned a > > resource with only id/href and it is necessary to do a > > ovirt_resource_refresh(). See patches for virt-viewer in > > https://www.redhat.com/archives/virt-tools-list/2017-August/msg00121.html > > Ok, ovirt_resource_init_from_xml_real() is doing > g_object_set(G_OBJECT(resource), "xml-node", node, NULL); which is not going > to work if xml-node is construct-only. So basically > ovirt_resource_refresh_from_xml is broken without this patch? (regardless of > the recent changes). This should be easy to trigger in a small test case? No, because init_from_xml_real() is called in construction time, I took some time to figure that out, but it comes from the GInitable interface. What changed is that we now need to pass the xml node later.
(In reply to Eduardo Lima (Etrunko) from comment #239) > (In reply to Christophe Fergeau from comment #238) > No, because init_from_xml_real() is called in construction time, I would think ovirt_resource_refresh -> ovirt_resource_init_from_xml -> klass->init_from_xml will eventually call OvirtResource::init_from_xml aka ovirt_resource_init_from_xml_real? > I took some > time to figure that out, but it comes from the GInitable interface. What > changed is that we now need to pass the xml node later. When then if it's unrelated to ovirt_resource_refresh?
(In reply to Christophe Fergeau from comment #240) > (In reply to Eduardo Lima (Etrunko) from comment #239) > > (In reply to Christophe Fergeau from comment #238) > > No, because init_from_xml_real() is called in construction time, > > I would think ovirt_resource_refresh -> ovirt_resource_init_from_xml -> > klass->init_from_xml will eventually call OvirtResource::init_from_xml aka > ovirt_resource_init_from_xml_real? > > > I took some > > time to figure that out, but it comes from the GInitable interface. What > > changed is that we now need to pass the xml node later. > > When then if it's unrelated to ovirt_resource_refresh? Sorry for the confusion, what I meant is that before the patches, we always had the xml-node assigned with the resource because we were always retrieving the resources from the OvirtApi functions (which in turn called OvirtCollection). It was already possible to create an empty resource with ovirt_resource_new() and then set all the necessary properties manually before calling ovirt_resouce_refresh(), which is basically what the new patches are doing under the hood.
Which problems are you getting when OvirtResource::xml-node is not set? One additional question I now have is why we keep the xml-node around? Not updating the xml-node in ovirt_resource_init_from_xml_real was probably an unnoticed bug all along :-/
(In reply to Christophe Fergeau from comment #242) > Which problems are you getting when OvirtResource::xml-node is not set? The problem was only not being able set it other than in construction time as explained. > One > additional question I now have is why we keep the xml-node around? > Not updating the xml-node in ovirt_resource_init_from_xml_real was probably > an unnoticed bug all along :-/ I have no idea why it was done that way, but surely seems wrong.
(In reply to Eduardo Lima (Etrunko) from comment #243) > (In reply to Christophe Fergeau from comment #242) > > Which problems are you getting when OvirtResource::xml-node is not set? > > The problem was only not being able set it other than in construction time > as explained. > This is the root cause of the problem. I have no idea whether you were getting crashes when trying to do X then Y, or if everything was successful, but with wrong results, or something else.
(In reply to Christophe Fergeau from comment #244) > (In reply to Eduardo Lima (Etrunko) from comment #243) > > (In reply to Christophe Fergeau from comment #242) > > > Which problems are you getting when OvirtResource::xml-node is not set? > > > > The problem was only not being able set it other than in construction time > > as explained. > > > > This is the root cause of the problem. I have no idea whether you were > getting crashes when trying to do X then Y, or if everything was successful, > but with wrong results, or something else. No crashes, I just got the error while trying to refresh the Host/Cluster/DataCenter from the VM. (remote-viewer:13635): libgovirt-CRITICAL **: ovirt_resource_init_from_xml_real: assertion 'resource->priv->xml != NULL' failed
(In reply to Eduardo Lima (Etrunko) from comment #245) > (In reply to Christophe Fergeau from comment #244) > > (In reply to Eduardo Lima (Etrunko) from comment #243) > > > (In reply to Christophe Fergeau from comment #242) > > > > Which problems are you getting when OvirtResource::xml-node is not set? > > > > > > The problem was only not being able set it other than in construction time > > > as explained. > > > > > > > This is the root cause of the problem. I have no idea whether you were > > getting crashes when trying to do X then Y, or if everything was successful, > > but with wrong results, or something else. > > No crashes, I just got the error while trying to refresh the > Host/Cluster/DataCenter from the VM. > > (remote-viewer:13635): libgovirt-CRITICAL **: > ovirt_resource_init_from_xml_real: assertion 'resource->priv->xml != NULL' > failed Then I would add that context to the commit log "Using the new ovirt_vm_get_host() (or whatever it's named) API, and then calling ovirt_resource_refresh() on it would trigger this runtime warning: (remote-viewer:13635): libgovirt-CRITICAL **: ovirt_resource_init_from_xml_real: assertion 'resource->priv->xml != NULL' failed"
Created attachment 359414 [details] [review] resource: Remove construct-only restriction for 'xml-node' property It is required since it is now possible to create a resource without a XML node associated with it, in functions recently added, such as ovirt_vm_get_{host,cluster}(), ovirt_host_get_cluster() and ovirt_cluster_get_data_center(). By calling one of those functions and then ovirt_resource_refresh() with the returned object, this runtime warning pops out: libgovirt-CRITICAL **: ovirt_resource_init_from_xml_real: assertion 'resource->priv->xml != NULL' failed" Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Created attachment 359526 [details] [review] resource: Update xml node in ovirt_resource_init_from_xml_real
Created attachment 359527 [details] [review] resource: Fix ovirt_resource_init_from_xml_real precondition I got the order of the 2 patches reversed, I'd put this one first, even though they are reasonably independent from each other.
(In reply to Eduardo Lima (Etrunko) from comment #247) > Created attachment 359414 [details] [review] [review] > resource: Remove construct-only restriction for 'xml-node' property > Gave some more thought to this. At the moment, OvirtResource::xml-node can only be set when an OvirtResource is first instantiated, and cannot even be read back. With your change, this means users of the library could do g_object_set(ovirt_resource, "xml-node", xml_node, NULL); as the property (after your patch) is not restricted to being construct-only. However, OvirtResource::set_property() is not doing the right thing, ie it's not calling init_from_xml() to update the OvirtResource with the new XML content. I think I'd go with the 2 proposed patches instead, and directly change OvirtResource::xml-node from ovirt_resource_init_from_xml_real() rather than going through a GObject property.
(In reply to Christophe Fergeau from comment #250) > (In reply to Eduardo Lima (Etrunko) from comment #247) > > Created attachment 359414 [details] [review] [review] [review] > > resource: Remove construct-only restriction for 'xml-node' property > > > > Gave some more thought to this. At the moment, OvirtResource::xml-node can > only be set when an OvirtResource is first instantiated, and cannot even be > read back. With your change, this means users of the library could do > g_object_set(ovirt_resource, "xml-node", xml_node, NULL); as the property > (after your patch) is not restricted to being construct-only. However, > OvirtResource::set_property() is not doing the right thing, ie it's not > calling init_from_xml() to update the OvirtResource with the new XML content. > > I think I'd go with the 2 proposed patches instead, and directly change > OvirtResource::xml-node from ovirt_resource_init_from_xml_real() rather than > going through a GObject property. Indeed makes more sense than what I proposed. I will run some quick tests with those before committing them.
Review of attachment 359526 [details] [review]: .
Review of attachment 359527 [details] [review]: l
With those last patches committed, I think we can finally close this bug :)
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new enhancement request ticket at https://gitlab.gnome.org/GNOME/libgovirt/-/issues/ Thank you for your understanding and your help.