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

[Eduardo Lima (Etrunko)]

Ouch, it looks like I missed this mail in my Inbox, somehow it slipped
through the filters and was not moved to the folder.

On 2/6/19 8:15 AM, Christophe Fergeau wrote:
> 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 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.
> 
> 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?
> 

You got the steps right, but in storage_domain_validate() the
data_center pointer will be tested in another function,
storage_domain_attached_to_data_center() and if it being NULL, it is
considered attached to the data center, to make it compatible with the
old method, as the comment in the hunk above says.

> 
> 
>>>>      }
>>>>  
>>>>      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.
> 

Okay, added a test for cluster, which will be on next version.


Regards, Eduardo.


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