[virt-tools-list] [PATCH virt-viewer v3] ovirt-foreign-menu: Bypass errors from Host/Cluster/Data Center
Eduardo Lima (Etrunko)
etrunko at redhat.com
Fri Jun 7 13:04:19 UTC 2019
On 6/6/19 1:22 PM, Christophe Fergeau wrote:
> Hey,
>
> I'm not really comfortable with that patch, which ignores some errors,
> and adds some code not to crash when we do that, in the hope that the
> end result will make sense. I'm under the impression that if the oVirt
> instance does not have the permissions issues that you mention, but if
> instead we get a temporary networking issue at the wrong time (for
> example), then rather than doing the proper checks, we'll just ignore
> the error, and assume the image we got is accessible to our VM?
There is no crash avoidance, what this patch does is to replicate the
previous behavior when the code only did queries for VM and storage
domains, which depending on the setup can lead to showing wrong ISO
images for that VM. But those setups are not very common, and most of
the cases it was fine.
>
> Isn't this a problem in the oVirt REST API if you can access a
> datacenter, a VM, but have no way to know if they are related?
>
That's it, we need to know the common thing between the VM and a storage
domain, and that is the datacenter. It would indeed be much better if
the information about it was available right away in the VM xml, but at
the moment we need to send a few other requests to get to it:
VM -> Host -> Cluster -> Data Center.
Anyway, IMO, it is better to fallback to the old method and show the
available ISO images, than to not show any at all and break in the
middle with an error message about insufficient permissions.
Regards, Eduardo.
> A few small comments below,
>
Fixed all leaks.
> On Tue, May 28, 2019 at 11:11:41AM -0300, Eduardo Lima (Etrunko) wrote:
>> ping
>>
>> On 4/10/19 5:20 PM, 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 | 51 ++++++++++++++++++++++++++++++----------
>>> 1 file changed, 39 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c
>>> index 59c3d48..83bdf5b 100644
>>> --- a/src/ovirt-foreign-menu.c
>>> +++ b/src/ovirt-foreign-menu.c
>>> @@ -627,12 +627,21 @@ G_GNUC_END_IGNORE_DEPRECATIONS
>>> }
>>>
>>> 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 between 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 = strv_contains((const gchar * const *) data_center_ids, data_center_guid);
>>> @@ -746,9 +755,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;
>
> This leaks 'error'
>
>>> }
>>>
>>> ovirt_foreign_menu_next_async_step(menu, task, STATE_DATA_CENTER);
>>> @@ -763,6 +769,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),
>>> @@ -783,9 +795,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;
>
> This leaks 'error'
>
>>> }
>>>
>>> ovirt_foreign_menu_next_async_step(menu, task, STATE_CLUSTER);
>>> @@ -797,9 +806,21 @@ 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);
>>> + }
>>> +
>>> + if (menu->priv->cluster == NULL) {
>>> + ovirt_foreign_menu_next_async_step(menu, task, STATE_CLUSTER);
>>> + return;
>>> + }
>>> +
>>> ovirt_resource_refresh_async(OVIRT_RESOURCE(menu->priv->cluster),
>>> menu->priv->proxy,
>>> g_task_get_cancellable(task),
>>> @@ -820,9 +841,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;
>
> This leaks 'error'
>
Fixed all leaks.
--
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/20190607/7d323746/attachment.sig>
More information about the virt-tools-list
mailing list