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

[Christophe Fergeau]

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?



> >>      }
> >>  
> >>      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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20190206/0482a096/attachment.sig>