[libvirt] [libvirt-php][PATCH] screenshot of remote host is available
screenshot of remote host is available. get_domain_object make so many error to message log, so you had better not use . diff --git a/examples/libvirt.php b/examples/libvirt.php index 1ca71b6..bf903f6 100644 --- a/examples/libvirt.php +++ b/examples/libvirt.php @@ -29,9 +29,9 @@ } 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(); } diff --git a/src/libvirt-php.cb/src/libvirt-php.c index 87e0467..4a53e33 100644 --- a/src/libvirt-php.c +++ b/src/libvirt-php.c @@ -1880,6 +1880,7 @@ PHP_FUNCTION(libvirt_domain_get_screenshot) int port = -1; int scancode = 10; char *path; + char *hostname; path = get_feature_binary("screenshot"); if (access(path, X_OK) != 0) { @@ -1887,7 +1888,7 @@ PHP_FUNCTION(libvirt_domain_get_screenshot) RETURN_FALSE; } - GET_DOMAIN_FROM_ARGS("r|l",&zdomain, &scancode); + GET_DOMAIN_FROM_ARGS("rs|l",&zdomain, &hostname, &scancode); xml=virDomainGetXMLDesc(domain->domain, 0); if (xml==NULL) { @@ -1912,10 +1913,16 @@ PHP_FUNCTION(libvirt_domain_get_screenshot) RETURN_FALSE; if (childpid == 0) { + char *prm = NULL; char tmpp[8] = { 0 }; - + snprintf(tmpp, sizeof(tmpp), ":%d", port); - retval = execlp(path, basename(path), tmpp, file, NULL); + prm = emalloc((sizeof(tmpp) + sizeof(hostname)) * sizeof(char)); + sprintf(prm, "%s%s", hostname, tmpp); + + retval = execlp(path, basename(path), prm, file, NULL); + + free(prm); _exit( retval ); } else {
Hi Yukihiro, On Thu, Jun 02, 2011 at 06:26:27AM +0000, warp.kawada@gmail.com wrote:
screenshot of remote host is available. get_domain_object make so many error to message log, so you had better not use .
I must admit I don't fully understand the problem you are having, do you mean that when trying to use the PHP binding for the new API to get the screenshots you were having errors ? What kind of errors and could you describe the context a bit, it will help understanding the issue and verifying your patch, Michal could you have a look ? Seems the patch changes the way to lookup the domain, and tries to provide non-local access but I'm not 100% sure, thanks ! Daniel
diff --git a/examples/libvirt.php b/examples/libvirt.php index 1ca71b6..bf903f6 100644 --- a/examples/libvirt.php +++ b/examples/libvirt.php @@ -29,9 +29,9 @@ }
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(); }
diff --git a/src/libvirt-php.cb/src/libvirt-php.c index 87e0467..4a53e33 100644 --- a/src/libvirt-php.c +++ b/src/libvirt-php.c @@ -1880,6 +1880,7 @@ PHP_FUNCTION(libvirt_domain_get_screenshot) int port = -1; int scancode = 10; char *path; + char *hostname;
path = get_feature_binary("screenshot"); if (access(path, X_OK) != 0) { @@ -1887,7 +1888,7 @@ PHP_FUNCTION(libvirt_domain_get_screenshot) RETURN_FALSE; }
- GET_DOMAIN_FROM_ARGS("r|l",&zdomain, &scancode); + GET_DOMAIN_FROM_ARGS("rs|l",&zdomain, &hostname, &scancode);
xml=virDomainGetXMLDesc(domain->domain, 0); if (xml==NULL) { @@ -1912,10 +1913,16 @@ PHP_FUNCTION(libvirt_domain_get_screenshot) RETURN_FALSE;
if (childpid == 0) { + char *prm = NULL; char tmpp[8] = { 0 }; - + snprintf(tmpp, sizeof(tmpp), ":%d", port); - retval = execlp(path, basename(path), tmpp, file, NULL); + prm = emalloc((sizeof(tmpp) + sizeof(hostname)) * sizeof(char)); + sprintf(prm, "%s%s", hostname, tmpp); + + retval = execlp(path, basename(path), prm, file, NULL); + + free(prm); _exit( retval ); } else {
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 06/08/2011 05:13 AM, Daniel Veillard wrote:
Hi Yukihiro,
On Thu, Jun 02, 2011 at 06:26:27AM +0000, warp.kawada@gmail.com wrote:
screenshot of remote host is available. get_domain_object make so many error to message log, so you had better not use . I must admit I don't fully understand the problem you are having, do you mean that when trying to use the PHP binding for the new API to get the screenshots you were having errors ? What kind of errors and could you describe the context a bit, it will help understanding the issue and verifying your patch,
Michal could you have a look ? Seems the patch changes the way to lookup the domain, and tries to provide non-local access but I'm not 100% sure,
Sure, Daniel. I've been looking to this and I don't really follow what Yukihiro meant. Could you provide me more information on the error you're experiencing and also append the gcc version and PHP version? I didn't run into those issues on php-5.3.6-1.fc14.i686 and gcc-4.5.1-4.fc14.i686. Yukihiro, why did you change the get_domain_object() to lookup by UUID string and get the domain screenshot with using the second argument you introduced? This is concept is OK for getting the remote host screenshot however I don't really follow the lines about get_domain_object(). Could you please explain? Thanks, Michal -- Michal Novotny <minovotn@redhat.com>, RHCE Virtualization Team (xen userspace), Red Hat
[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@redhat.com>, RHCE Virtualization Team (xen userspace), Red Hat
participants (3)
-
Daniel Veillard -
Michal Novotny -
warp.kawada＠gmail.com