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@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?
>
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@xxxxxxxxxx
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
