1:26 a.m.
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 {
10:13 p.m.
Hi Yukihiro,
On Thu, Jun 02, 2011 at 06:26:27AM +0000, warp.kawada(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
4:03 a.m.
On 06/08/2011 05:13 AM, Daniel Veillard wrote:
Hi Yukihiro,
On Thu, Jun 02, 2011 at 06:26:27AM +0000, warp.kawada(a)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(a)redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat
6:43 a.m.
[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(a)redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat
4916
days inactive
4922
days old
3 comments
3 participants
participants (3)
-
Daniel Veillard -
Michal Novotny -
warp.kawada＠gmail.com