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.
>
>> }
>>
>> 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@xxxxxxxxxx
>> https://www.redhat.com/mailman/listinfo/virt-tools-list
--
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etrunko@xxxxxxxxxx
Attachment:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
