On Tue, Feb 05, 2019 at 03:38:35PM -0200, Eduardo Lima (Etrunko) wrote: > On 7/17/18 11:48 AM, Christophe Fergeau wrote: > > On Fri, Jul 06, 2018 at 09:59:23AM -0300, Eduardo Lima (Etrunko) wrote: > >> When accessing ovirt as a regular user, it may happen that queries to > >> Hosts, Clusters and Data Centers return errors due to insufficient > >> permissions, while they will work fine if access is done by admin user. > >> In this case, we skip the errors and fallback to the old method. > >> > >> Signed-off-by: Eduardo Lima (Etrunko) <etrunko@xxxxxxxxxx> > >> --- > >> src/ovirt-foreign-menu.c | 43 +++++++++++++++++++++++++++++++------------ > >> 1 file changed, 31 insertions(+), 12 deletions(-) > >> > >> diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c > >> index 70a0b50..8ed08c9 100644 > >> --- a/src/ovirt-foreign-menu.c > >> +++ b/src/ovirt-foreign-menu.c > >> @@ -624,12 +624,20 @@ static void ovirt_foreign_menu_fetch_vm_cdrom_async(OvirtForeignMenu *menu, > >> > >> #ifdef HAVE_OVIRT_DATA_CENTER > >> static gboolean storage_domain_attached_to_data_center(OvirtStorageDomain *domain, > >> - OvirtDataCenter *data_center) > >> + OvirtDataCenter *data_center) > >> { > >> GStrv data_center_ids; > >> char *data_center_guid; > >> gboolean match; > >> > >> + /* For some reason we did not get data center information, so just return > >> + * TRUE as it will work like a fallback to old method, where we did not > >> + * check relationship with data center and storage domain.*/ > >> + if (data_center == NULL) { > >> + g_debug("Could not get data center info, considering storage domain is attached to it"); > >> + return TRUE; > >> + } > >> + > >> g_object_get(domain, "data-center-ids", &data_center_ids, NULL); > >> g_object_get(data_center, "guid", &data_center_guid, NULL); > >> match = g_strv_contains((const gchar * const *) data_center_ids, data_center_guid); > >> @@ -743,9 +751,6 @@ static void data_center_fetched_cb(GObject *source_object, > >> ovirt_resource_refresh_finish(resource, result, &error); > >> if (error != NULL) { > >> g_debug("failed to fetch Data Center: %s", error->message); > >> - g_task_return_error(task, error); > >> - g_object_unref(task); > >> - return; > > > > I don't think the hunk just above > > (storage_domain_attached_to_data_center) takes this situation into > > account, menu->priv->data_center is non-null, but it will be 'empty', so > > storage_domain_attached_to_data_center is probably not going to do the > > right thing. > > This callback function is only called if the previous condition tested > in the hunk below (function ovirt_foreign menu_fetch_data_center_async) > is valid. Starting from ovirt_foreign_menu_fetch_data_center_async(), let's assume we trigger the if (menu->priv->data_center == NULL) { ovirt_foreign_menu_next_async_step(menu, task, STATE_DATA_CENTER); return; } codepath, which gets us to ovirt_foreign_menu_next_async_step(): case STATE_STORAGE_DOMAIN: if (menu->priv->files == NULL) { ovirt_foreign_menu_fetch_storage_domain_async(menu, task); break; } This calls ovirt_collection_fetch_async(collection, menu->priv->proxy, g_task_get_cancellable(task), storage_domains_fetched_cb, task); and, assuming this call is successful (even if fetching the data center was not), storage_domains_fetched_cb() will end up calling storage_domain_validate() which makes use of menu->priv->data_center even if it could be NULL Did I get one of these steps wrong? > >> } > >> > >> ovirt_foreign_menu_next_async_step(menu, task, STATE_DATA_CENTER); > >> @@ -760,6 +765,12 @@ static void ovirt_foreign_menu_fetch_data_center_async(OvirtForeignMenu *menu, > >> g_return_if_fail(OVIRT_IS_CLUSTER(menu->priv->cluster)); > >> > >> menu->priv->data_center = ovirt_cluster_get_data_center(menu->priv->cluster); > >> + > >> + if (menu->priv->data_center == NULL) { > >> + ovirt_foreign_menu_next_async_step(menu, task, STATE_DATA_CENTER); > >> + return; > >> + } > >> + > >> ovirt_resource_refresh_async(OVIRT_RESOURCE(menu->priv->data_center), > >> menu->priv->proxy, > >> g_task_get_cancellable(task), > >> @@ -780,9 +791,6 @@ static void cluster_fetched_cb(GObject *source_object, > >> ovirt_resource_refresh_finish(resource, result, &error); > >> if (error != NULL) { > >> g_debug("failed to fetch Cluster: %s", error->message); > >> - g_task_return_error(task, error); > >> - g_object_unref(task); > >> - return; > >> } > >> > >> ovirt_foreign_menu_next_async_step(menu, task, STATE_CLUSTER); > >> + > >> + /* If there is no host information, we get cluster from the VM */ > >> + if (menu->priv->host == NULL) > >> + menu->priv->cluster = ovirt_vm_get_cluster(menu->priv->vm); > >> + else > >> + menu->priv->cluster = ovirt_host_get_cluster(menu->priv->host); > >> > > > > I think we should make 100% sure menu->priv->cluster is not NULL at this > > point. > > It is not possible to happen, it can only be 'empty' but never NULL. Even on malformed/invalid XML data? ovirt_{vm,host}_get_cluster() both end up calling g_initable_new_valist() to create the 'cluster' instance which can return NULL. Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list