On 6/7/19 10:37 AM, Christophe Fergeau wrote: > On Fri, Jun 07, 2019 at 10:04:19AM -0300, Eduardo Lima (Etrunko) wrote: >> 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. > > Is there a bug against the oVirt REST API for this? > Not that I am aware of. >> >> 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. > > My complaint is that *all* errors are being ignored, even the ones which > might be meaningful, not just the permissions errors. I understand, but currently we get a generic "400 Bad Request" error, which is also used in other cases, while it should be something like a "401 Unauthorized". Using g_error_matches() would not help here, I think. < HTTP/1.1 400 Bad Request < Soup-Debug-Timestamp: 1559921229 < Soup-Debug: SoupMessage 1 (0x5570ecf0e8e0) < Date: Fri, 07 Jun 2019 15:27:09 GMT < Server: Apache/2.4.6 (Red Hat Enterprise Linux) OpenSSL/1.0.2k-fips mod_auth_gssapi/1.5.1 mod_nss/1.0.14 NSS/3.28.4 mod_wsgi/3.4 Python/2.7.5 < Content-Type: application/xml;charset=UTF-8 < Content-Length: 188 < Correlation-Id: 4417d860-9453-4e5e-94dc-fb1f7c964db8 < Connection: close < < <?xml version="1.0" encoding="UTF-8" standalone="yes"?> < <fault> < <reason>Operation Failed</reason> < <detail>query execution failed due to insufficient permissions.</detail> < </fault> > > Christophe > -- 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
