Re: [libvirt-php][PATCH] screenshot of remote host is available

[Michal Novotny <minovotn@xxxxxxxxxx>]

[snip]
>> function domain_get_screenshot($domain) {
>> - $dom = $this->get_domain_object($domain);
>> -
>> - $tmp = libvirt_domain_get_screenshot($dom);
>> + $dom = libvirt_domain_lookup_by_uuid_string($this->conn, $domain);
>> + $hostname = $this->get_hostname();
>> + $tmp = libvirt_domain_get_screenshot($dom, $hostname);
>> return ($tmp) ? $tmp : $this->_set_last_error();
>> }
>>

Hi Yukihiro,
I did have a look to the class itself and I still don't follow what you
mean.

Please see current version of get_domain_object() PHP method:

                function get_domain_object($nameRes) {
                        if (is_resource($nameRes))
                                return $nameRes;

                        $dom=libvirt_domain_lookup_by_name($this->conn,
$nameRes);
                        if (!$dom) {
                               
$dom=libvirt_domain_lookup_by_uuid_string($this->conn, $nameRes);
                                if (!$dom)
                                        return $this->_set_last_error();
                        }

                        return $dom;
                }

You changed this to lookup by UUID string directly however what's the
problem here?

Let's analyse the code above:
1) It's checking whether nameRes is a resource or not, if it's a
resource then it's being returned with no modifications
2) We try to lookup by name and if there's still nothing found then we
try to lookup by UUID string instead
3) If there's no domain object set then we set the last error string and
return false (which is error value of _set_last_error() method),
otherwise we return the domain object

Since this function is widely used then it's not good to change
domain_get_screenshot() to use domain lookup by UUID string (which would
kill the option to lookup by domain name) and you should patch the
get_domain_object() function instead.

The ability to get the screenshot of remote host is a really nice thing
and good to have, thanks for this but I can't ACK and apply the patch
because of the hunk above. Could you please provide us more information
what the problem there is ?

Nevertheless, if there's a problem it's not good to patch the code to
use some other function instead of properly patching the affected
function. Could you please fix the get_domain_object() function and post
it with this remote host screenshot hunk too?

Thanks,
Michal

-- 
Michal Novotny <minovotn@xxxxxxxxxx>, RHCE
Virtualization Team (xen userspace), Red Hat

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list