[virt-tools-list] [PATCH virt-viewer 3/3] ovirt-foreign-menu: Bypass errors from Host/Cluster/Data Center

Eduardo Lima (Etrunko) etrunko at redhat.com
Tue Feb 5 17:38:35 UTC 2019


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 at redhat.com>
>> ---
>>  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.

> 
>>      }
>>  
>>      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);
>> @@ -794,9 +802,14 @@ static void ovirt_foreign_menu_fetch_cluster_async(OvirtForeignMenu *menu,
>>  {
>>      g_return_if_fail(OVIRT_IS_FOREIGN_MENU(menu));
>>      g_return_if_fail(OVIRT_IS_PROXY(menu->priv->proxy));
>> -    g_return_if_fail(OVIRT_IS_HOST(menu->priv->host));
>> +    g_return_if_fail(OVIRT_IS_VM(menu->priv->vm));
> 
> Maybe keep these 2, but in the right block in the if (menu->priv->host
> == NULL) below?
> 

Fixed.

@@ -801,9 +810,16 @@ static void
ovirt_foreign_menu_fetch_cluster_async(OvirtForeignMenu *menu,
 {
     g_return_if_fail(OVIRT_IS_FOREIGN_MENU(menu));
     g_return_if_fail(OVIRT_IS_PROXY(menu->priv->proxy));
-    g_return_if_fail(OVIRT_IS_HOST(menu->priv->host));

-    menu->priv->cluster = ovirt_host_get_cluster(menu->priv->host);
+    /* If there is no host information, we get cluster from the VM */
+    if (menu->priv->host == NULL) {
+        g_return_if_fail(OVIRT_IS_VM(menu->priv->vm));
+        menu->priv->cluster = ovirt_vm_get_cluster(menu->priv->vm);
+    } else {
+        g_return_if_fail(OVIRT_IS_HOST(menu->priv->host));
+        menu->priv->cluster = ovirt_host_get_cluster(menu->priv->host);
+    }
+
     ovirt_resource_refresh_async(OVIRT_RESOURCE(menu->priv->cluster),
                                  menu->priv->proxy,
                                  g_task_get_cancellable(task),


>> +
>> +    /* 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.

> 
> Christophe
> 
>> -    menu->priv->cluster = ovirt_host_get_cluster(menu->priv->host);
>>      ovirt_resource_refresh_async(OVIRT_RESOURCE(menu->priv->cluster),
>>                                   menu->priv->proxy,
>>                                   g_task_get_cancellable(task),
>> @@ -817,9 +830,6 @@ static void host_fetched_cb(GObject *source_object,
>>      ovirt_resource_refresh_finish(resource, result, &error);
>>      if (error != NULL) {
>>          g_debug("failed to fetch Host: %s", error->message);
>> -        g_task_return_error(task, error);
>> -        g_object_unref(task);
>> -        return;
>>      }
>>  
>>      ovirt_foreign_menu_next_async_step(menu, task, STATE_HOST);
>> @@ -834,6 +844,15 @@ static void ovirt_foreign_menu_fetch_host_async(OvirtForeignMenu *menu,
>>      g_return_if_fail(OVIRT_IS_VM(menu->priv->vm));
>>  
>>      menu->priv->host = ovirt_vm_get_host(menu->priv->vm);
>> +
>> +    /* In some cases the VM XML does not include host information, so we just
>> +     * skip to the next step
>> +     */
>> +    if (menu->priv->host == NULL) {
>> +        ovirt_foreign_menu_next_async_step(menu, task, STATE_HOST);
>> +        return;
>> +    }
>> +
>>      ovirt_resource_refresh_async(OVIRT_RESOURCE(menu->priv->host),
>>                                   menu->priv->proxy,
>>                                   g_task_get_cancellable(task),
>> -- 
>> 2.14.4
>>
>> _______________________________________________
>> virt-tools-list mailing list
>> virt-tools-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/virt-tools-list


-- 
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etrunko at redhat.com

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20190205/cdf79ea0/attachment.sig>


More information about the virt-tools-list mailing list