After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 765297 - Missing support for datacenters/clusters
Missing support for datacenters/clusters
Status: RESOLVED OBSOLETE
Product: libgovirt
Classification: Other
Component: general
0.3.x
Other Linux
: Normal normal
: ---
Assigned To: libgovirt maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2016-04-20 08:49 UTC by Christophe Fergeau
Modified: 2021-05-25 11:23 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Retrieve host id and cluster id from vm (4.60 KB, patch)
2017-04-18 18:46 UTC, Eduardo Lima (Etrunko)
none Details | Review
Retrieve data center id from storage domain (4.38 KB, patch)
2017-04-18 18:46 UTC, Eduardo Lima (Etrunko)
none Details | Review
Add auxiliary ovirt_sub_collection_new_from_resource (8.36 KB, patch)
2017-04-18 18:47 UTC, Eduardo Lima (Etrunko)
none Details | Review
New APIs to enable search queries for collections (7.31 KB, patch)
2017-04-18 18:48 UTC, Eduardo Lima (Etrunko)
none Details | Review
Support for Hosts (15.15 KB, patch)
2017-04-18 18:48 UTC, Eduardo Lima (Etrunko)
none Details | Review
Support for clusters (15.76 KB, patch)
2017-04-18 18:49 UTC, Eduardo Lima (Etrunko)
none Details | Review
Support for Data Centers (15.83 KB, patch)
2017-04-18 18:49 UTC, Eduardo Lima (Etrunko)
none Details | Review
Support for hosts (15.19 KB, patch)
2017-04-18 18:58 UTC, Eduardo Lima (Etrunko)
none Details | Review
Support for clusters (15.80 KB, patch)
2017-04-18 18:59 UTC, Eduardo Lima (Etrunko)
none Details | Review
Support for data centers (15.88 KB, patch)
2017-04-18 18:59 UTC, Eduardo Lima (Etrunko)
none Details | Review
Remove unecessary ovirt_api_init_from_xml() (1.70 KB, patch)
2017-05-04 21:44 UTC, Eduardo Lima (Etrunko)
none Details | Review
Remove unused function ovirt_rest_xml_node_get_content() (1.71 KB, patch)
2017-05-04 21:44 UTC, Eduardo Lima (Etrunko)
accepted-commit_now Details | Review
New function ovirt_rest_xml_node_get_attr_from_path() (3.13 KB, patch)
2017-05-04 21:45 UTC, Eduardo Lima (Etrunko)
accepted-commit_now Details | Review
Factor out property value setting from ovirt_resource_parse_xml() (4.23 KB, patch)
2017-05-04 21:45 UTC, Eduardo Lima (Etrunko)
none Details | Review
Add support for attributes in ovirt_resource_parse_xml() (3.78 KB, patch)
2017-05-04 21:46 UTC, Eduardo Lima (Etrunko)
none Details | Review
Move out ovirt_resource_parse_xml() to ovirt-utils (17.22 KB, patch)
2017-05-04 21:47 UTC, Eduardo Lima (Etrunko)
none Details | Review
Retrieve data center ids (4.38 KB, patch)
2017-05-04 21:47 UTC, Eduardo Lima (Etrunko)
none Details | Review
Retrieve host and cluster ids (4.65 KB, patch)
2017-05-04 21:48 UTC, Eduardo Lima (Etrunko)
none Details | Review
Set vm state property using OvirtXmlElement struct (2.90 KB, patch)
2017-05-04 21:49 UTC, Eduardo Lima (Etrunko)
none Details | Review
Introduce auxiliary function ovirt_sub_collection_new_from_resource() (8.34 KB, patch)
2017-05-04 21:49 UTC, Eduardo Lima (Etrunko)
none Details | Review
New API functions to enable search queries of collections (7.31 KB, patch)
2017-05-04 21:50 UTC, Eduardo Lima (Etrunko)
none Details | Review
Implement support for hosts (15.11 KB, patch)
2017-05-04 21:50 UTC, Eduardo Lima (Etrunko)
none Details | Review
Implement support for clusters (15.74 KB, patch)
2017-05-04 21:51 UTC, Eduardo Lima (Etrunko)
none Details | Review
Implement support for data centers (15.38 KB, patch)
2017-05-04 21:51 UTC, Eduardo Lima (Etrunko)
none Details | Review
Factor out property value setting from ovirt_resource_parse_xml() (4.23 KB, patch)
2017-05-12 02:25 UTC, Eduardo Lima (Etrunko)
reviewed Details | Review
Use explicit initialization of struct OvirtXmlElement members (3.33 KB, patch)
2017-05-12 02:26 UTC, Eduardo Lima (Etrunko)
accepted-commit_now Details | Review
Move out ovirt_resource_parse_xml() to ovirt-utils (10.79 KB, patch)
2017-05-12 02:26 UTC, Eduardo Lima (Etrunko)
accepted-commit_now Details | Review
Add OvirtXmlElement as argument to init_from_xml() virtual function (8.48 KB, patch)
2017-05-12 02:27 UTC, Eduardo Lima (Etrunko)
needs-work Details | Review
Remove unused function ovirt_rest_xml_node_get_content() (1.76 KB, patch)
2017-05-12 02:27 UTC, Eduardo Lima (Etrunko)
accepted-commit_now Details | Review
Rename ovirt_rest_xml_node_get_content_va() to ovirt_rest_xml_node_find() (2.46 KB, patch)
2017-05-12 02:27 UTC, Eduardo Lima (Etrunko)
none Details | Review
Retrieve node attributes in ovirt_resource_parse_xml() (2.45 KB, patch)
2017-05-12 02:28 UTC, Eduardo Lima (Etrunko)
accepted-commit_now Details | Review
Support G_TYPE_STRING in _set_property_value_from_type() (910 bytes, patch)
2017-05-12 02:28 UTC, Eduardo Lima (Etrunko)
accepted-commit_now Details | Review
Retrieve host and cluster ids (4.65 KB, patch)
2017-05-12 02:28 UTC, Eduardo Lima (Etrunko)
none Details | Review
Support G_TYPE_STRV in _set_property_value_from_type() (4.02 KB, patch)
2017-05-12 02:28 UTC, Eduardo Lima (Etrunko)
reviewed Details | Review
Retrieve data center ids (3.36 KB, patch)
2017-05-12 02:29 UTC, Eduardo Lima (Etrunko)
none Details | Review
Introduce auxiliary function ovirt_sub_collection_new_from_resource() (8.34 KB, patch)
2017-05-12 02:29 UTC, Eduardo Lima (Etrunko)
accepted-commit_now Details | Review
Remove unecessary ovirt_api_init_from_xml() (1.78 KB, patch)
2017-05-12 02:29 UTC, Eduardo Lima (Etrunko)
needs-work Details | Review
New API functions to enable search queries of collections (7.35 KB, patch)
2017-05-12 02:30 UTC, Eduardo Lima (Etrunko)
reviewed Details | Review
Implement support for hosts (15.10 KB, patch)
2017-05-12 02:30 UTC, Eduardo Lima (Etrunko)
none Details | Review
Implement support for clusters (15.72 KB, patch)
2017-05-12 02:30 UTC, Eduardo Lima (Etrunko)
none Details | Review
Implement support for data centers (15.39 KB, patch)
2017-05-12 02:30 UTC, Eduardo Lima (Etrunko)
none Details | Review
Rename ovirt_rest_xml_node_get_content_va() to ovirt_rest_xml_node_find() (2.51 KB, patch)
2017-05-12 19:45 UTC, Eduardo Lima (Etrunko)
none Details | Review
Factor out property value setting from ovirt_resource_parse_xml() (4.23 KB, patch)
2017-05-19 04:38 UTC, Eduardo Lima (Etrunko)
accepted-commit_now Details | Review
Use explicit initialization of struct OvirtXmlElement members (3.33 KB, patch)
2017-05-19 04:39 UTC, Eduardo Lima (Etrunko)
accepted-commit_now Details | Review
Move out ovirt_resource_parse_xml() to ovirt-utils (10.43 KB, patch)
2017-05-19 04:39 UTC, Eduardo Lima (Etrunko)
accepted-commit_now Details | Review
Remove unused function ovirt_rest_xml_node_get_content() (1.77 KB, patch)
2017-05-19 04:40 UTC, Eduardo Lima (Etrunko)
accepted-commit_now Details | Review
Rename ovirt_rest_xml_node_get_content_va() to ovirt_rest_xml_node_find() (2.45 KB, patch)
2017-05-19 04:40 UTC, Eduardo Lima (Etrunko)
accepted-commit_now Details | Review
Introduce function ovirt_rest_xml_node_get_attr_from_path() (1.17 KB, patch)
2017-05-19 04:40 UTC, Eduardo Lima (Etrunko)
reviewed Details | Review
Retrieve node attributes in ovirt_resource_parse_xml() (1.82 KB, patch)
2017-05-19 04:40 UTC, Eduardo Lima (Etrunko)
accepted-commit_now Details | Review
Support G_TYPE_STRING in _set_property_value_from_type() (910 bytes, patch)
2017-05-19 04:41 UTC, Eduardo Lima (Etrunko)
accepted-commit_now Details | Review
Retrieve host and cluster ids (4.83 KB, patch)
2017-05-19 04:41 UTC, Eduardo Lima (Etrunko)
none Details | Review
Support G_TYPE_STRV in _set_property_value_from_type() (3.94 KB, patch)
2017-05-19 04:41 UTC, Eduardo Lima (Etrunko)
reviewed Details | Review
Retrieve data center ids (3.36 KB, patch)
2017-05-19 04:42 UTC, Eduardo Lima (Etrunko)
reviewed Details | Review
Introduce auxiliary function ovirt_sub_collection_new_from_resource() (8.29 KB, patch)
2017-05-19 04:42 UTC, Eduardo Lima (Etrunko)
accepted-commit_now Details | Review
New API functions to enable search queries of collections (7.30 KB, patch)
2017-05-19 04:42 UTC, Eduardo Lima (Etrunko)
reviewed Details | Review
Implement support for hosts (15.06 KB, patch)
2017-05-19 04:43 UTC, Eduardo Lima (Etrunko)
reviewed Details | Review
Implement support for clusters (15.68 KB, patch)
2017-05-19 04:43 UTC, Eduardo Lima (Etrunko)
reviewed Details | Review
Implement support for data centers (15.35 KB, patch)
2017-05-19 04:43 UTC, Eduardo Lima (Etrunko)
accepted-commit_now Details | Review
Factor out property value setting from ovirt_resource_parse_xml() (3.87 KB, patch)
2017-06-22 20:49 UTC, Eduardo Lima (Etrunko)
committed Details | Review
Use explicit initialization of struct OvirtXmlElement members (2.73 KB, patch)
2017-06-22 20:50 UTC, Eduardo Lima (Etrunko)
none Details | Review
Move out ovirt_resource_parse_xml() to ovirt-utils (10.10 KB, patch)
2017-06-22 20:50 UTC, Eduardo Lima (Etrunko)
committed Details | Review
Remove unused function ovirt_rest_xml_node_get_content() (1.76 KB, patch)
2017-06-22 20:51 UTC, Eduardo Lima (Etrunko)
committed Details | Review
Rename ovirt_rest_xml_node_get_content_va() to ovirt_rest_xml_node_find() (2.44 KB, patch)
2017-06-22 20:52 UTC, Eduardo Lima (Etrunko)
committed Details | Review
Retrieve node attributes in ovirt_resource_parse_xml() (2.48 KB, patch)
2017-06-22 20:52 UTC, Eduardo Lima (Etrunko)
committed Details | Review
Support G_TYPE_STRING in _set_property_value_from_type() (902 bytes, patch)
2017-06-22 20:56 UTC, Eduardo Lima (Etrunko)
committed Details | Review
Retrieve host and cluster ids (4.82 KB, patch)
2017-06-22 20:56 UTC, Eduardo Lima (Etrunko)
none Details | Review
Support G_TYPE_STRV in _set_property_value_from_type() (3.85 KB, patch)
2017-06-22 20:57 UTC, Eduardo Lima (Etrunko)
committed Details | Review
Retrieve data center ids (3.35 KB, patch)
2017-06-22 20:58 UTC, Eduardo Lima (Etrunko)
committed Details | Review
Introduce auxiliary function ovirt_sub_collection_new_from_resource() (8.28 KB, patch)
2017-06-22 20:58 UTC, Eduardo Lima (Etrunko)
committed Details | Review
New API functions to enable search queries of collections (8.01 KB, patch)
2017-06-22 20:59 UTC, Eduardo Lima (Etrunko)
none Details | Review
Implement support for hosts (15.05 KB, patch)
2017-06-22 21:00 UTC, Eduardo Lima (Etrunko)
none Details | Review
Implement support for clusters (15.68 KB, patch)
2017-06-22 21:00 UTC, Eduardo Lima (Etrunko)
none Details | Review
Implement support for data centers (15.37 KB, patch)
2017-06-22 21:05 UTC, Eduardo Lima (Etrunko)
none Details | Review
Use explicit initialization of struct OvirtXmlElement members (3.39 KB, patch)
2017-06-28 13:24 UTC, Eduardo Lima (Etrunko)
committed Details | Review
Retrieve host and cluster ids (4.54 KB, patch)
2017-06-28 15:44 UTC, Eduardo Lima (Etrunko)
none Details | Review
New API functions to enable search queries of collections (7.51 KB, patch)
2017-06-29 18:04 UTC, Eduardo Lima (Etrunko)
committed Details | Review
Introduce ovirt_resource_new*() functions (3.72 KB, patch)
2017-07-14 20:28 UTC, Eduardo Lima (Etrunko)
none Details | Review
Use ovirt_resource_new* functions instead of g_initable_new (5.00 KB, patch)
2017-07-14 20:29 UTC, Eduardo Lima (Etrunko)
committed Details | Review
Move resource type definitions to ovirt-types.h (5.24 KB, patch)
2017-07-14 20:29 UTC, Eduardo Lima (Etrunko)
committed Details | Review
Initial support for hosts (16.82 KB, patch)
2017-07-14 20:29 UTC, Eduardo Lima (Etrunko)
none Details | Review
Initial support for clusters (17.44 KB, patch)
2017-07-14 20:29 UTC, Eduardo Lima (Etrunko)
none Details | Review
Initial support for data centers (16.19 KB, patch)
2017-07-14 20:29 UTC, Eduardo Lima (Etrunko)
none Details | Review
vm: Introduce ovirt_vm_get_host() (6.24 KB, patch)
2017-07-14 20:29 UTC, Eduardo Lima (Etrunko)
none Details | Review
vm: Introduce ovirt_vm_get_cluster() (6.02 KB, patch)
2017-07-14 20:29 UTC, Eduardo Lima (Etrunko)
none Details | Review
host: Introduce ovirt_host_get_cluster() (2.30 KB, patch)
2017-07-14 20:30 UTC, Eduardo Lima (Etrunko)
none Details | Review
cluster: Introduce ovirt_cluster_get_data_center() (3.04 KB, patch)
2017-07-14 20:30 UTC, Eduardo Lima (Etrunko)
none Details | Review
Introduce ovirt_resource_new*() functions (3.65 KB, patch)
2017-07-21 19:06 UTC, Eduardo Lima (Etrunko)
none Details | Review
Initial support for hosts (16.81 KB, patch)
2017-07-21 19:13 UTC, Eduardo Lima (Etrunko)
committed Details | Review
Initial support for clusters (17.44 KB, patch)
2017-07-21 19:13 UTC, Eduardo Lima (Etrunko)
committed Details | Review
Initial support for data centers (16.19 KB, patch)
2017-07-21 19:13 UTC, Eduardo Lima (Etrunko)
committed Details | Review
vm: Introduce ovirt_vm_get_host() (6.62 KB, patch)
2017-07-21 19:18 UTC, Eduardo Lima (Etrunko)
none Details | Review
vm: Introduce ovirt_vm_get_cluster() (6.40 KB, patch)
2017-07-21 19:18 UTC, Eduardo Lima (Etrunko)
none Details | Review
host: Introduce ovirt_host_get_cluster() (3.12 KB, patch)
2017-07-21 19:19 UTC, Eduardo Lima (Etrunko)
none Details | Review
cluster: Introduce ovirt_cluster_get_data_center() (3.93 KB, patch)
2017-07-21 19:19 UTC, Eduardo Lima (Etrunko)
none Details | Review
Introduce ovirt_resource_new*() functions (2.83 KB, patch)
2017-07-24 12:01 UTC, Eduardo Lima (Etrunko)
committed Details | Review
vm: Introduce ovirt_vm_get_host() (6.65 KB, patch)
2017-07-24 13:59 UTC, Eduardo Lima (Etrunko)
committed Details | Review
vm: Introduce ovirt_vm_get_cluster() (6.40 KB, patch)
2017-07-24 13:59 UTC, Eduardo Lima (Etrunko)
committed Details | Review
host: Introduce ovirt_host_get_cluster() (3.12 KB, patch)
2017-07-24 14:00 UTC, Eduardo Lima (Etrunko)
committed Details | Review
cluster: Introduce ovirt_cluster_get_data_center() (3.37 KB, patch)
2017-07-24 14:00 UTC, Eduardo Lima (Etrunko)
committed Details | Review
resource: Remove construct-only restriction for 'xml-node' property (1.51 KB, patch)
2017-08-04 20:19 UTC, Eduardo Lima (Etrunko)
none Details | Review
Add missing #include in govirt.h (866 bytes, patch)
2017-08-04 20:24 UTC, Eduardo Lima (Etrunko)
committed Details | Review
resource: Remove construct-only restriction for 'xml-node' property (1.88 KB, patch)
2017-09-08 18:24 UTC, Eduardo Lima (Etrunko)
none Details | Review
resource: Update xml node in ovirt_resource_init_from_xml_real (2.36 KB, patch)
2017-09-11 13:13 UTC, Christophe Fergeau
committed Details | Review
resource: Fix ovirt_resource_init_from_xml_real precondition (2.23 KB, patch)
2017-09-11 13:14 UTC, Christophe Fergeau
committed Details | Review

Description Christophe Fergeau 2016-04-20 08:49:31 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.
Comment 1 Christophe Fergeau 2016-06-16 09:43:04 UTC
Bug #767723 is more work than solving just this bug, but it would solve this bug as a side-effect.
Comment 2 Eduardo Lima (Etrunko) 2017-04-18 18:46:13 UTC
Created attachment 350017 [details] [review]
Retrieve host id and cluster id from vm
Comment 3 Eduardo Lima (Etrunko) 2017-04-18 18:46:41 UTC
Created attachment 350018 [details] [review]
Retrieve data center id from storage domain
Comment 4 Eduardo Lima (Etrunko) 2017-04-18 18:47:20 UTC
Created attachment 350019 [details] [review]
Add auxiliary ovirt_sub_collection_new_from_resource
Comment 5 Eduardo Lima (Etrunko) 2017-04-18 18:48:07 UTC
Created attachment 350020 [details] [review]
New APIs to enable search queries for collections
Comment 6 Eduardo Lima (Etrunko) 2017-04-18 18:48:42 UTC
Created attachment 350021 [details] [review]
Support for Hosts
Comment 7 Eduardo Lima (Etrunko) 2017-04-18 18:49:14 UTC
Created attachment 350022 [details] [review]
Support for clusters
Comment 8 Eduardo Lima (Etrunko) 2017-04-18 18:49:49 UTC
Created attachment 350023 [details] [review]
Support for Data Centers
Comment 9 Eduardo Lima (Etrunko) 2017-04-18 18:53:41 UTC
Patches available at my github repository. The full diff and commits can be seen at

https://github.com/GNOME/libgovirt/compare/master...etrunko:master
Comment 10 Eduardo Lima (Etrunko) 2017-04-18 18:58:38 UTC
Created attachment 350024 [details] [review]
Support for hosts

Added missing g_object_clear_object(&hosts->priv->vms);
Comment 11 Eduardo Lima (Etrunko) 2017-04-18 18:59:26 UTC
Created attachment 350025 [details] [review]
Support for clusters

Added missing g_object_clear_object(&clusters->priv->hosts);
Comment 12 Eduardo Lima (Etrunko) 2017-04-18 18:59:59 UTC
Created attachment 350026 [details] [review]
Support for data centers

Added missing g_object_clear_object(&data_centers->priv->clusters);
Comment 13 Eduardo Lima (Etrunko) 2017-05-04 21:40:25 UTC
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
Comment 14 Eduardo Lima (Etrunko) 2017-05-04 21:44:16 UTC
Created attachment 351123 [details] [review]
Remove unecessary ovirt_api_init_from_xml()
Comment 15 Eduardo Lima (Etrunko) 2017-05-04 21:44:51 UTC
Created attachment 351124 [details] [review]
Remove unused function ovirt_rest_xml_node_get_content()
Comment 16 Eduardo Lima (Etrunko) 2017-05-04 21:45:21 UTC
Created attachment 351125 [details] [review]
New function ovirt_rest_xml_node_get_attr_from_path()
Comment 17 Eduardo Lima (Etrunko) 2017-05-04 21:45:53 UTC
Created attachment 351126 [details] [review]
Factor out property value setting from ovirt_resource_parse_xml()
Comment 18 Eduardo Lima (Etrunko) 2017-05-04 21:46:25 UTC
Created attachment 351127 [details] [review]
Add support for attributes in ovirt_resource_parse_xml()
Comment 19 Eduardo Lima (Etrunko) 2017-05-04 21:47:08 UTC
Created attachment 351128 [details] [review]
Move out ovirt_resource_parse_xml() to ovirt-utils
Comment 20 Eduardo Lima (Etrunko) 2017-05-04 21:47:36 UTC
Created attachment 351129 [details] [review]
Retrieve data center ids
Comment 21 Eduardo Lima (Etrunko) 2017-05-04 21:48:00 UTC
Created attachment 351130 [details] [review]
Retrieve host and cluster ids
Comment 22 Eduardo Lima (Etrunko) 2017-05-04 21:49:14 UTC
Created attachment 351131 [details] [review]
Set vm state property using OvirtXmlElement struct
Comment 23 Eduardo Lima (Etrunko) 2017-05-04 21:49:48 UTC
Created attachment 351132 [details] [review]
Introduce auxiliary function ovirt_sub_collection_new_from_resource()
Comment 24 Eduardo Lima (Etrunko) 2017-05-04 21:50:15 UTC
Created attachment 351133 [details] [review]
New API functions to enable search queries of collections
Comment 25 Eduardo Lima (Etrunko) 2017-05-04 21:50:40 UTC
Created attachment 351134 [details] [review]
Implement support for hosts
Comment 26 Eduardo Lima (Etrunko) 2017-05-04 21:51:05 UTC
Created attachment 351135 [details] [review]
Implement support for clusters
Comment 27 Eduardo Lima (Etrunko) 2017-05-04 21:51:19 UTC
Created attachment 351136 [details] [review]
Implement support for data centers
Comment 28 Christophe Fergeau 2017-05-05 09:33:47 UTC
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 ;)
Comment 29 Christophe Fergeau 2017-05-05 09:34:46 UTC
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.
Comment 30 Christophe Fergeau 2017-05-05 09:51:37 UTC
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());
Comment 31 Christophe Fergeau 2017-05-05 12:35:35 UTC
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)
Comment 32 Christophe Fergeau 2017-05-05 12:43:25 UTC
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.
Comment 33 Christophe Fergeau 2017-05-05 12:55:30 UTC
Review of attachment 351125 [details] [review]:

.
Comment 34 Christophe Fergeau 2017-05-05 12:56:51 UTC
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?
Comment 35 Christophe Fergeau 2017-05-05 13:02:54 UTC
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?
Comment 36 Christophe Fergeau 2017-05-05 13:04:43 UTC
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?)
Comment 37 Christophe Fergeau 2017-05-05 13:07:34 UTC
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.
Comment 38 Christophe Fergeau 2017-05-05 13:21:31 UTC
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()
Comment 39 Christophe Fergeau 2017-05-05 13:26:04 UTC
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?
Comment 40 Christophe Fergeau 2017-05-05 13:37:11 UTC
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()
Comment 41 Christophe Fergeau 2017-05-05 13:52:12 UTC
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.
Comment 42 Eduardo Lima (Etrunko) 2017-05-10 18:19:59 UTC
(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.
Comment 43 Eduardo Lima (Etrunko) 2017-05-10 18:22:27 UTC
(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
Comment 44 Eduardo Lima (Etrunko) 2017-05-10 18:28:49 UTC
(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()"
Comment 45 Eduardo Lima (Etrunko) 2017-05-10 18:44:26 UTC
(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.
Comment 46 Eduardo Lima (Etrunko) 2017-05-10 19:02:47 UTC
(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
Comment 47 Eduardo Lima (Etrunko) 2017-05-10 19:05:55 UTC
(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.
Comment 48 Eduardo Lima (Etrunko) 2017-05-10 20:53:52 UTC
(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
Comment 49 Eduardo Lima (Etrunko) 2017-05-11 14:58:34 UTC
(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.
Comment 50 Christophe Fergeau 2017-05-11 15:10:52 UTC
(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).
Comment 51 Eduardo Lima (Etrunko) 2017-05-12 02:25:15 UTC
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.
Comment 52 Eduardo Lima (Etrunko) 2017-05-12 02:25:51 UTC
Created attachment 351678 [details] [review]
Factor out property value setting from ovirt_resource_parse_xml()
Comment 53 Eduardo Lima (Etrunko) 2017-05-12 02:26:17 UTC
Created attachment 351679 [details] [review]
Use explicit initialization of struct OvirtXmlElement members
Comment 54 Eduardo Lima (Etrunko) 2017-05-12 02:26:34 UTC
Created attachment 351680 [details] [review]
Move out ovirt_resource_parse_xml() to ovirt-utils
Comment 55 Eduardo Lima (Etrunko) 2017-05-12 02:27:00 UTC
Created attachment 351681 [details] [review]
Add OvirtXmlElement as argument to init_from_xml() virtual function
Comment 56 Eduardo Lima (Etrunko) 2017-05-12 02:27:22 UTC
Created attachment 351682 [details] [review]
Remove unused function ovirt_rest_xml_node_get_content()
Comment 57 Eduardo Lima (Etrunko) 2017-05-12 02:27:47 UTC
Created attachment 351683 [details] [review]
Rename ovirt_rest_xml_node_get_content_va() to ovirt_rest_xml_node_find()
Comment 58 Eduardo Lima (Etrunko) 2017-05-12 02:28:05 UTC
Created attachment 351684 [details] [review]
Retrieve node attributes in ovirt_resource_parse_xml()
Comment 59 Eduardo Lima (Etrunko) 2017-05-12 02:28:20 UTC
Created attachment 351685 [details] [review]
Support G_TYPE_STRING in _set_property_value_from_type()
Comment 60 Eduardo Lima (Etrunko) 2017-05-12 02:28:34 UTC
Created attachment 351686 [details] [review]
Retrieve host and cluster ids
Comment 61 Eduardo Lima (Etrunko) 2017-05-12 02:28:51 UTC
Created attachment 351687 [details] [review]
Support G_TYPE_STRV in _set_property_value_from_type()
Comment 62 Eduardo Lima (Etrunko) 2017-05-12 02:29:12 UTC
Created attachment 351688 [details] [review]
Retrieve data center ids
Comment 63 Eduardo Lima (Etrunko) 2017-05-12 02:29:32 UTC
Created attachment 351689 [details] [review]
Introduce auxiliary function ovirt_sub_collection_new_from_resource()
Comment 64 Eduardo Lima (Etrunko) 2017-05-12 02:29:49 UTC
Created attachment 351690 [details] [review]
Remove unecessary ovirt_api_init_from_xml()
Comment 65 Eduardo Lima (Etrunko) 2017-05-12 02:30:05 UTC
Created attachment 351691 [details] [review]
New API functions to enable search queries of collections
Comment 66 Eduardo Lima (Etrunko) 2017-05-12 02:30:24 UTC
Created attachment 351692 [details] [review]
Implement support for hosts
Comment 67 Eduardo Lima (Etrunko) 2017-05-12 02:30:41 UTC
Created attachment 351693 [details] [review]
Implement support for clusters
Comment 68 Eduardo Lima (Etrunko) 2017-05-12 02:30:56 UTC
Created attachment 351694 [details] [review]
Implement support for data centers
Comment 69 Eduardo Lima (Etrunko) 2017-05-12 19:45:37 UTC
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
Comment 70 Christophe Fergeau 2017-05-15 10:18:20 UTC
(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.
Comment 71 Christophe Fergeau 2017-05-15 10:25:35 UTC
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?
Comment 72 Christophe Fergeau 2017-05-15 10:26:11 UTC
Review of attachment 351678 [details] [review]:

Overall looks fine.
Comment 73 Christophe Fergeau 2017-05-15 10:30:22 UTC
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.
Comment 74 Christophe Fergeau 2017-05-15 15:56:09 UTC
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 ;)
Comment 75 Christophe Fergeau 2017-05-15 16:01:23 UTC
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()
Comment 76 Christophe Fergeau 2017-05-15 16:13:33 UTC
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.
Comment 77 Christophe Fergeau 2017-05-15 16:14:06 UTC
Review of attachment 351682 [details] [review]:

.
Comment 78 Christophe Fergeau 2017-05-15 16:21:11 UTC
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?
Comment 79 Christophe Fergeau 2017-05-15 16:21:56 UTC
Review of attachment 351684 [details] [review]:

.
Comment 80 Christophe Fergeau 2017-05-15 16:22:57 UTC
Review of attachment 351685 [details] [review]:

.
Comment 81 Christophe Fergeau 2017-05-15 16:24:04 UTC
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.
Comment 82 Christophe Fergeau 2017-05-15 16:57:35 UTC
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.
Comment 83 Eduardo Lima (Etrunko) 2017-05-15 22:53:17 UTC
(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.
Comment 84 Christophe Fergeau 2017-05-16 07:42:35 UTC
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.
Comment 85 Christophe Fergeau 2017-05-16 08:23:05 UTC
Review of attachment 351689 [details] [review]:

::: govirt/ovirt-api.c
@@ +137,3 @@
 }
 
+

blank line addition?
Comment 86 Christophe Fergeau 2017-05-16 08:23:08 UTC
Review of attachment 351689 [details] [review]:

::: govirt/ovirt-api.c
@@ +137,3 @@
 }
 
+

blank line addition?
Comment 87 Christophe Fergeau 2017-05-16 08:23:42 UTC
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?
Comment 88 Christophe Fergeau 2017-05-16 08:34:26 UTC
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 89 Eduardo Lima (Etrunko) 2017-05-19 02:58:40 UTC
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
>
Comment 90 Eduardo Lima (Etrunko) 2017-05-19 03:08:16 UTC
(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.
Comment 91 Eduardo Lima (Etrunko) 2017-05-19 03:17:33 UTC
(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.
Comment 92 Eduardo Lima (Etrunko) 2017-05-19 03:20:09 UTC
(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 ;)
Comment 93 Eduardo Lima (Etrunko) 2017-05-19 03:22:03 UTC
(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.
Comment 94 Eduardo Lima (Etrunko) 2017-05-19 03:25:07 UTC
(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.
Comment 95 Eduardo Lima (Etrunko) 2017-05-19 03:45:29 UTC
(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.
Comment 96 Eduardo Lima (Etrunko) 2017-05-19 03:46:36 UTC
(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
Comment 97 Eduardo Lima (Etrunko) 2017-05-19 03:48:37 UTC
(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.
Comment 98 Eduardo Lima (Etrunko) 2017-05-19 03:59:17 UTC
(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 &quote; or &apos;
Comment 99 Eduardo Lima (Etrunko) 2017-05-19 04:33:42 UTC
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.
Comment 100 Eduardo Lima (Etrunko) 2017-05-19 04:38:58 UTC
Created attachment 352124 [details] [review]
Factor out property value setting from ovirt_resource_parse_xml()
Comment 101 Eduardo Lima (Etrunko) 2017-05-19 04:39:24 UTC
Created attachment 352125 [details] [review]
Use explicit initialization of struct OvirtXmlElement members
Comment 102 Eduardo Lima (Etrunko) 2017-05-19 04:39:45 UTC
Created attachment 352126 [details] [review]
Move out ovirt_resource_parse_xml() to ovirt-utils
Comment 103 Eduardo Lima (Etrunko) 2017-05-19 04:40:01 UTC
Created attachment 352127 [details] [review]
Remove unused function ovirt_rest_xml_node_get_content()
Comment 104 Eduardo Lima (Etrunko) 2017-05-19 04:40:19 UTC
Created attachment 352128 [details] [review]
Rename ovirt_rest_xml_node_get_content_va() to ovirt_rest_xml_node_find()
Comment 105 Eduardo Lima (Etrunko) 2017-05-19 04:40:35 UTC
Created attachment 352129 [details] [review]
Introduce function ovirt_rest_xml_node_get_attr_from_path()
Comment 106 Eduardo Lima (Etrunko) 2017-05-19 04:40:56 UTC
Created attachment 352130 [details] [review]
Retrieve node attributes in ovirt_resource_parse_xml()
Comment 107 Eduardo Lima (Etrunko) 2017-05-19 04:41:11 UTC
Created attachment 352131 [details] [review]
Support G_TYPE_STRING in _set_property_value_from_type()
Comment 108 Eduardo Lima (Etrunko) 2017-05-19 04:41:29 UTC
Created attachment 352132 [details] [review]
Retrieve host and cluster ids
Comment 109 Eduardo Lima (Etrunko) 2017-05-19 04:41:55 UTC
Created attachment 352133 [details] [review]
Support G_TYPE_STRV in _set_property_value_from_type()
Comment 110 Eduardo Lima (Etrunko) 2017-05-19 04:42:18 UTC
Created attachment 352134 [details] [review]
Retrieve data center ids
Comment 111 Eduardo Lima (Etrunko) 2017-05-19 04:42:37 UTC
Created attachment 352135 [details] [review]
Introduce auxiliary function ovirt_sub_collection_new_from_resource()
Comment 112 Eduardo Lima (Etrunko) 2017-05-19 04:42:57 UTC
Created attachment 352136 [details] [review]
New API functions to enable search queries of collections
Comment 113 Eduardo Lima (Etrunko) 2017-05-19 04:43:11 UTC
Created attachment 352137 [details] [review]
Implement support for hosts
Comment 114 Eduardo Lima (Etrunko) 2017-05-19 04:43:32 UTC
Created attachment 352138 [details] [review]
Implement support for clusters
Comment 115 Eduardo Lima (Etrunko) 2017-05-19 04:43:50 UTC
Created attachment 352139 [details] [review]
Implement support for data centers
Comment 116 Christophe Fergeau 2017-05-22 16:03:29 UTC
(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.
Comment 117 Christophe Fergeau 2017-05-22 16:07:00 UTC
(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).
Comment 118 Christophe Fergeau 2017-05-22 16:15:31 UTC
(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."
Comment 119 Christophe Fergeau 2017-05-23 12:26:31 UTC
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).
Comment 120 Christophe Fergeau 2017-05-23 12:28:56 UTC
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.
Comment 121 Christophe Fergeau 2017-05-23 15:36:45 UTC
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.
Comment 122 Christophe Fergeau 2017-05-24 10:24:26 UTC
Review of attachment 352129 [details] [review]:

I would not have split this, but kept it together with the next patch ;) (both are sufficiently small)
Comment 123 Christophe Fergeau 2017-05-24 10:36:56 UTC
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."
Comment 124 Christophe Fergeau 2017-05-24 10:41:30 UTC
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, .. .?
Comment 125 Christophe Fergeau 2017-05-24 10:45:28 UTC
Review of attachment 352124 [details] [review]:

.
Comment 126 Christophe Fergeau 2017-05-24 10:45:36 UTC
Review of attachment 352125 [details] [review]:

.
Comment 127 Christophe Fergeau 2017-05-24 10:45:47 UTC
Review of attachment 352127 [details] [review]:

.
Comment 128 Christophe Fergeau 2017-05-24 10:46:01 UTC
Review of attachment 352128 [details] [review]:

.
Comment 129 Christophe Fergeau 2017-05-24 10:46:14 UTC
Review of attachment 352129 [details] [review]:

.
Comment 130 Christophe Fergeau 2017-05-24 10:46:21 UTC
Review of attachment 352130 [details] [review]:

.
Comment 131 Christophe Fergeau 2017-05-24 10:46:28 UTC
Review of attachment 352131 [details] [review]:

.
Comment 132 Christophe Fergeau 2017-05-24 10:47:17 UTC
Review of attachment 352135 [details] [review]:

.
Comment 133 Christophe Fergeau 2017-05-24 14:40:45 UTC
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?
Comment 134 Christophe Fergeau 2017-05-24 14:44:01 UTC
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.
Comment 135 Christophe Fergeau 2017-05-24 14:48:37 UTC
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().
Comment 136 Christophe Fergeau 2017-05-24 14:53:49 UTC
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
Comment 137 Christophe Fergeau 2017-05-24 15:00:45 UTC
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
Comment 138 Christophe Fergeau 2017-05-24 15:03:31 UTC
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
Comment 139 Eduardo Lima (Etrunko) 2017-06-22 19:57:32 UTC
(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.
Comment 140 Eduardo Lima (Etrunko) 2017-06-22 19:58:16 UTC
(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.
Comment 141 Eduardo Lima (Etrunko) 2017-06-22 20:00:53 UTC
(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
Comment 142 Eduardo Lima (Etrunko) 2017-06-22 20:01:29 UTC
(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.
Comment 143 Eduardo Lima (Etrunko) 2017-06-22 20:02:00 UTC
(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.
Comment 144 Eduardo Lima (Etrunko) 2017-06-22 20:02:58 UTC
(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.
Comment 145 Eduardo Lima (Etrunko) 2017-06-22 20:04:46 UTC
(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.
Comment 146 Eduardo Lima (Etrunko) 2017-06-22 20:08:47 UTC
(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.
Comment 147 Eduardo Lima (Etrunko) 2017-06-22 20:09:29 UTC
(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.
Comment 148 Eduardo Lima (Etrunko) 2017-06-22 20:09:54 UTC
(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.
Comment 149 Eduardo Lima (Etrunko) 2017-06-22 20:10:09 UTC
(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.
Comment 150 Eduardo Lima (Etrunko) 2017-06-22 20:10:46 UTC
(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.
Comment 151 Eduardo Lima (Etrunko) 2017-06-22 20:49:29 UTC
Created attachment 354267 [details] [review]
Factor out property value setting from ovirt_resource_parse_xml()
Comment 152 Eduardo Lima (Etrunko) 2017-06-22 20:50:05 UTC
Created attachment 354269 [details] [review]
Use explicit initialization of struct OvirtXmlElement members
Comment 153 Eduardo Lima (Etrunko) 2017-06-22 20:50:42 UTC
Created attachment 354270 [details] [review]
Move out ovirt_resource_parse_xml() to ovirt-utils
Comment 154 Eduardo Lima (Etrunko) 2017-06-22 20:51:33 UTC
Created attachment 354271 [details] [review]
Remove unused function ovirt_rest_xml_node_get_content()
Comment 155 Eduardo Lima (Etrunko) 2017-06-22 20:52:11 UTC
Created attachment 354272 [details] [review]
Rename ovirt_rest_xml_node_get_content_va() to ovirt_rest_xml_node_find()
Comment 156 Eduardo Lima (Etrunko) 2017-06-22 20:52:38 UTC
Created attachment 354273 [details] [review]
Retrieve node attributes in ovirt_resource_parse_xml()
Comment 157 Eduardo Lima (Etrunko) 2017-06-22 20:56:22 UTC
Created attachment 354274 [details] [review]
Support G_TYPE_STRING in _set_property_value_from_type()
Comment 158 Eduardo Lima (Etrunko) 2017-06-22 20:56:49 UTC
Created attachment 354275 [details] [review]
Retrieve host and cluster ids
Comment 159 Eduardo Lima (Etrunko) 2017-06-22 20:57:30 UTC
Created attachment 354276 [details] [review]
Support G_TYPE_STRV in _set_property_value_from_type()
Comment 160 Eduardo Lima (Etrunko) 2017-06-22 20:58:00 UTC
Created attachment 354277 [details] [review]
Retrieve data center ids
Comment 161 Eduardo Lima (Etrunko) 2017-06-22 20:58:48 UTC
Created attachment 354278 [details] [review]
Introduce auxiliary function ovirt_sub_collection_new_from_resource()
Comment 162 Eduardo Lima (Etrunko) 2017-06-22 20:59:21 UTC
Created attachment 354279 [details] [review]
New API functions to enable search queries of collections
Comment 163 Eduardo Lima (Etrunko) 2017-06-22 21:00:03 UTC
Created attachment 354280 [details] [review]
Implement support for hosts
Comment 164 Eduardo Lima (Etrunko) 2017-06-22 21:00:37 UTC
Created attachment 354281 [details] [review]
Implement support for clusters
Comment 165 Eduardo Lima (Etrunko) 2017-06-22 21:05:36 UTC
Created attachment 354282 [details] [review]
Implement support for data centers
Comment 166 Christophe Fergeau 2017-06-28 10:32:27 UTC
(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?
Comment 167 Christophe Fergeau 2017-06-28 11:33:15 UTC
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.
Comment 168 Eduardo Lima (Etrunko) 2017-06-28 13:24:02 UTC
Created attachment 354623 [details] [review]
Use explicit initialization of struct OvirtXmlElement members
Comment 169 Eduardo Lima (Etrunko) 2017-06-28 13:24:41 UTC
(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.
Comment 170 Eduardo Lima (Etrunko) 2017-06-28 13:26:06 UTC
(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 ;)
Comment 171 Christophe Fergeau 2017-06-28 15:05:25 UTC
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.
Comment 172 Eduardo Lima (Etrunko) 2017-06-28 15:44:33 UTC
Created attachment 354628 [details] [review]
Retrieve host and cluster ids
Comment 173 Eduardo Lima (Etrunko) 2017-06-28 15:47:39 UTC
(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
Comment 174 Christophe Fergeau 2017-06-29 09:58:11 UTC
(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?
Comment 175 Christophe Fergeau 2017-06-29 09:59:34 UTC
(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 :)
Comment 176 Christophe Fergeau 2017-06-29 11:24:11 UTC
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 :-/
Comment 177 Eduardo Lima (Etrunko) 2017-06-29 18:04:33 UTC
Created attachment 354702 [details] [review]
New API functions to enable search queries of collections
Comment 178 Christophe Fergeau 2017-06-30 12:35:54 UTC
(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 179 Eduardo Lima (Etrunko) 2017-07-10 12:44:50 UTC
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
Comment 180 Eduardo Lima (Etrunko) 2017-07-14 20:28:56 UTC
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>
Comment 181 Eduardo Lima (Etrunko) 2017-07-14 20:29:06 UTC
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>
Comment 182 Eduardo Lima (Etrunko) 2017-07-14 20:29:13 UTC
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>
Comment 183 Eduardo Lima (Etrunko) 2017-07-14 20:29:22 UTC
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>
Comment 184 Eduardo Lima (Etrunko) 2017-07-14 20:29:29 UTC
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>
Comment 185 Eduardo Lima (Etrunko) 2017-07-14 20:29:38 UTC
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>
Comment 186 Eduardo Lima (Etrunko) 2017-07-14 20:29:50 UTC
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>
Comment 187 Eduardo Lima (Etrunko) 2017-07-14 20:29:58 UTC
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>
Comment 188 Eduardo Lima (Etrunko) 2017-07-14 20:30:10 UTC
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>
Comment 189 Eduardo Lima (Etrunko) 2017-07-14 20:30:17 UTC
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 190 Eduardo Lima (Etrunko) 2017-07-14 20:32:41 UTC
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.
Comment 191 Christophe Fergeau 2017-07-21 14:50:31 UTC
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.
Comment 192 Christophe Fergeau 2017-07-21 15:10:05 UTC
Review of attachment 355631 [details] [review]:

Note that G_GNUC_INTERNAL is purely informative, what matters is t he .syms file.
Comment 193 Christophe Fergeau 2017-07-21 15:10:55 UTC
Review of attachment 355632 [details] [review]:

.
Comment 194 Christophe Fergeau 2017-07-21 15:41:43 UTC
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
Comment 195 Christophe Fergeau 2017-07-21 15:41:58 UTC
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
Comment 196 Christophe Fergeau 2017-07-21 15:42:15 UTC
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
Comment 197 Christophe Fergeau 2017-07-21 15:47:48 UTC
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 :)
Comment 198 Christophe Fergeau 2017-07-21 15:50:29 UTC
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
Comment 199 Christophe Fergeau 2017-07-21 15:50:32 UTC
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
Comment 200 Christophe Fergeau 2017-07-21 15:53:09 UTC
Review of attachment 355639 [details] [review]:

.
Comment 201 Eduardo Lima (Etrunko) 2017-07-21 19:04:52 UTC
(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.
Comment 202 Eduardo Lima (Etrunko) 2017-07-21 19:06:00 UTC
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>
Comment 203 Eduardo Lima (Etrunko) 2017-07-21 19:11:17 UTC
(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.
Comment 204 Eduardo Lima (Etrunko) 2017-07-21 19:13:33 UTC
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>
Comment 205 Eduardo Lima (Etrunko) 2017-07-21 19:13:42 UTC
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>
Comment 206 Eduardo Lima (Etrunko) 2017-07-21 19:13:50 UTC
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>
Comment 207 Eduardo Lima (Etrunko) 2017-07-21 19:17:19 UTC
(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
Comment 208 Eduardo Lima (Etrunko) 2017-07-21 19:18:48 UTC
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>
Comment 209 Eduardo Lima (Etrunko) 2017-07-21 19:18:56 UTC
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>
Comment 210 Eduardo Lima (Etrunko) 2017-07-21 19:19:05 UTC
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>
Comment 211 Eduardo Lima (Etrunko) 2017-07-21 19:19:20 UTC
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>
Comment 212 Christophe Fergeau 2017-07-24 08:24:41 UTC
(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.
Comment 213 Eduardo Lima (Etrunko) 2017-07-24 12:01:08 UTC
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>
Comment 214 Christophe Fergeau 2017-07-24 13:15:55 UTC
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.
Comment 215 Eduardo Lima (Etrunko) 2017-07-24 13:59:04 UTC
(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.
Comment 216 Eduardo Lima (Etrunko) 2017-07-24 13:59:46 UTC
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>
Comment 217 Eduardo Lima (Etrunko) 2017-07-24 13:59:54 UTC
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>
Comment 218 Eduardo Lima (Etrunko) 2017-07-24 14:00:02 UTC
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>
Comment 219 Eduardo Lima (Etrunko) 2017-07-24 14:00:10 UTC
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>
Comment 220 Christophe Fergeau 2017-07-24 16:10:01 UTC
Review of attachment 354277 [details] [review]:

.
Comment 221 Christophe Fergeau 2017-07-24 16:10:05 UTC
Review of attachment 356141 [details] [review]:

.
Comment 222 Christophe Fergeau 2017-07-24 16:10:08 UTC
Review of attachment 356142 [details] [review]:

.
Comment 223 Christophe Fergeau 2017-07-24 16:10:15 UTC
Review of attachment 356143 [details] [review]:

.
Comment 224 Christophe Fergeau 2017-07-24 16:10:19 UTC
Review of attachment 356290 [details] [review]:

.
Comment 225 Christophe Fergeau 2017-07-24 16:10:23 UTC
Review of attachment 356303 [details] [review]:

.
Comment 226 Christophe Fergeau 2017-07-24 16:10:28 UTC
Review of attachment 356304 [details] [review]:

.
Comment 227 Christophe Fergeau 2017-07-24 16:10:31 UTC
Review of attachment 356305 [details] [review]:

.
Comment 228 Christophe Fergeau 2017-07-24 16:10:36 UTC
Review of attachment 356306 [details] [review]:

.
Comment 229 Christophe Fergeau 2017-07-24 16:12:32 UTC
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?
Comment 230 Eduardo Lima (Etrunko) 2017-07-24 18:55:33 UTC
(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.
Comment 231 Christophe Fergeau 2017-07-25 16:23:30 UTC
(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
Comment 232 Eduardo Lima (Etrunko) 2017-08-04 20:19:24 UTC
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>
Comment 233 Eduardo Lima (Etrunko) 2017-08-04 20:24:58 UTC
Created attachment 356983 [details] [review]
Add missing #include in govirt.h

Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
Comment 234 Eduardo Lima (Etrunko) 2017-08-04 20:26:47 UTC
Review of attachment 356983 [details] [review]:

Committed as trivial.
Comment 235 Eduardo Lima (Etrunko) 2017-09-06 16:17:05 UTC
(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?
Comment 236 Christophe Fergeau 2017-09-08 12:27:05 UTC
Review of attachment 356981 [details] [review]:

In which usecases is this going to get used?
Comment 237 Eduardo Lima (Etrunko) 2017-09-08 13:49:42 UTC
(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
Comment 238 Christophe Fergeau 2017-09-08 14:36:42 UTC
(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?
Comment 239 Eduardo Lima (Etrunko) 2017-09-08 14:56:23 UTC
(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.
Comment 240 Christophe Fergeau 2017-09-08 15:10:50 UTC
(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?
Comment 241 Eduardo Lima (Etrunko) 2017-09-08 15:28:11 UTC
(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.
Comment 242 Christophe Fergeau 2017-09-08 15:53:32 UTC
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 :-/
Comment 243 Eduardo Lima (Etrunko) 2017-09-08 16:07:31 UTC
(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.
Comment 244 Christophe Fergeau 2017-09-08 16:27:29 UTC
(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.
Comment 245 Eduardo Lima (Etrunko) 2017-09-08 16:45:58 UTC
(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
Comment 246 Christophe Fergeau 2017-09-08 17:06:47 UTC
(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"
Comment 247 Eduardo Lima (Etrunko) 2017-09-08 18:24:35 UTC
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>
Comment 248 Christophe Fergeau 2017-09-11 13:13:56 UTC
Created attachment 359526 [details] [review]
resource: Update xml node in ovirt_resource_init_from_xml_real
Comment 249 Christophe Fergeau 2017-09-11 13:14:49 UTC
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.
Comment 250 Christophe Fergeau 2017-09-11 13:18:44 UTC
(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.
Comment 251 Eduardo Lima (Etrunko) 2017-09-11 13:26:32 UTC
(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.
Comment 252 Eduardo Lima (Etrunko) 2017-09-18 13:22:27 UTC
Review of attachment 359526 [details] [review]:

.
Comment 253 Eduardo Lima (Etrunko) 2017-09-18 13:22:34 UTC
Review of attachment 359527 [details] [review]:

l
Comment 254 Eduardo Lima (Etrunko) 2017-09-18 13:24:22 UTC
With those last patches committed, I think we can finally close this bug :)
Comment 255 André Klapper 2021-05-25 11:23:40 UTC
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.