[Libvir] [PATCH] Add virConnectGetHostname and virConnectGetURI calls, virsh commands, and some related fixes
5:09 a.m.
This patch adds two calls:
/**
+ * virConnectGetHostname:
+ * @conn: pointer to a hypervisor connection
+ *
+ * This returns the system hostname on which the hypervisor is
+ * running (the result of the gethostname(2) system call). If
+ * we are connected to a remote system, then this returns the
+ * hostname of the remote system.
+ *
+ * Returns the hostname which must be freed by the caller, or
+ * NULL if there was an error.
+ */
+char *
+virConnectGetHostname (virConnectPtr conn)
and:
+/**
+ * virConnectGetURI:
+ * @conn: pointer to a hypervisor connection
+ *
+ * This returns the URI (name) of the hypervisor connection.
+ * Normally this is the same as or similar to the string passed
+ * to the virConnectOpen/virConnectOpenReadOnly call, but
+ * the driver may make the URI canonical. If name == NULL
+ * was passed to virConnectOpen, then the driver will return
+ * a non-NULL URI which can be used to connect to the same
+ * hypervisor later.
+ *
+ * Returns the URI string which must be freed by the caller, or
+ * NULL if there was an error.
+ */
+char *
+virConnectGetURI (virConnectPtr conn)
It also fixes a kind of accidental memory leak which turns out not to be
a memory leak. In the current version of libvirt CVS, when using remote
connections, remote's private data is not freed by remote_internal.c.
However it turns out that it _is_ freed by qemuNetworkClose. Obviously
some confusion there, but this patch fixes that. (Fixing that is a
dependency of this patch, which is why the two are done together in one
patch).
I've added "virsh hostname" and "virsh uri" commands:
$ src/virsh -c test:///default uri
test:///default
$ src/virsh -c test:///default hostname
localhost.localdomain
(Yeah, I haven't set the hostname on this machine ...)
I've updated the Xen, test and remote drivers to support the calls.
However I didn't update qemu because of Dan's big changes to qemu. Once
we have decided whether to put in Dan's changes, I'll go back and
implement this for qemu. (I'm actually not sure I would need to
implement them, if remote supports the calls already).
I haven't updated the Python bindings, but will do so next.
I decided not to implement the proposed virConnectGetTransport call
because I think it needs more thought. It would obviously be useful to
find out whether the current connection is local or remote, encrypted or
unencrypted, authenticated or unauthenticated, because all of these
things could be usefully communicated back to the user by icons in
virt-manager. Simply returning the transport name doesn't really do
this. The user might also want to find out _how_ the session is
encrypted (what TLS parameters are in use), and again a simple string
can't do that.
Before applying this patch you will need to do:
rm qemud/remote_dispatch_localvars.h \
qemud/remote_dispatch_proc_switch.h \
qemud/remote_dispatch_prototypes.h \
qemud/remote_protocol.c \
qemud/remote_protocol.h
and after applying you will need to do:
make -C qemud remote_protocol.c
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
5:47 a.m.
On Tue, Jun 26, 2007 at 11:09:21AM +0100, Richard W.M. Jones wrote:
This patch adds two calls:
/**
+ * virConnectGetHostname:
+ * @conn: pointer to a hypervisor connection
+ *
+ * This returns the system hostname on which the hypervisor is
+ * running (the result of the gethostname(2) system call). If
+ * we are connected to a remote system, then this returns the
+ * hostname of the remote system.
+ *
+ * Returns the hostname which must be freed by the caller, or
+ * NULL if there was an error.
+ */
+char *
+virConnectGetHostname (virConnectPtr conn)
and:
+/**
+ * virConnectGetURI:
+ * @conn: pointer to a hypervisor connection
+ *
+ * This returns the URI (name) of the hypervisor connection.
+ * Normally this is the same as or similar to the string passed
+ * to the virConnectOpen/virConnectOpenReadOnly call, but
+ * the driver may make the URI canonical. If name == NULL
+ * was passed to virConnectOpen, then the driver will return
+ * a non-NULL URI which can be used to connect to the same
+ * hypervisor later.
+ *
+ * Returns the URI string which must be freed by the caller, or
+ * NULL if there was an error.
+ */
+char *
+virConnectGetURI (virConnectPtr conn)
One could argue that the first call is redundant w.r.t. the second but
I understand the convenience aspect.
It also fixes a kind of accidental memory leak which turns out not to
be
a memory leak. In the current version of libvirt CVS, when using remote
connections, remote's private data is not freed by remote_internal.c.
However it turns out that it _is_ freed by qemuNetworkClose. Obviously
some confusion there, but this patch fixes that. (Fixing that is a
dependency of this patch, which is why the two are done together in one
patch).
okay
I've added "virsh hostname" and "virsh uri"
commands:
$ src/virsh -c test:///default uri
test:///default
$ src/virsh -c test:///default hostname
localhost.localdomain
those commands makes more sense when using the shell of virsh, but yes
looks good !
(Yeah, I haven't set the hostname on this machine ...)
I've updated the Xen, test and remote drivers to support the calls.
However I didn't update qemu because of Dan's big changes to qemu. Once
we have decided whether to put in Dan's changes, I'll go back and
implement this for qemu. (I'm actually not sure I would need to
implement them, if remote supports the calls already).
I think I gave +1 to all patches maybe except 16/20 is there really
something holding those patches in the queue ?
I haven't updated the Python bindings, but will do so next.
it's probable that those 2 calles will be handled automatically
since the arg is a known structure and the return value is a string.
It's a matter of rebuilding the API xml with the new headers once applied
and let the generator do its job.
I decided not to implement the proposed virConnectGetTransport call
because I think it needs more thought. It would obviously be useful to
I prefer the idea of returning the full URI, then it's just a matter
of parsing if you really want to extract.
In general let's try to use the most generic identifiers for API building.
find out whether the current connection is local or remote, encrypted
or
unencrypted, authenticated or unauthenticated, because all of these
things could be usefully communicated back to the user by icons in
virt-manager. Simply returning the transport name doesn't really do
this. The user might also want to find out _how_ the session is
encrypted (what TLS parameters are in use), and again a simple string
can't do that.
Before applying this patch you will need to do:
rm qemud/remote_dispatch_localvars.h \
qemud/remote_dispatch_proc_switch.h \
qemud/remote_dispatch_prototypes.h \
qemud/remote_protocol.c \
qemud/remote_protocol.h
and after applying you will need to do:
make -C qemud remote_protocol.c
+++ qemud/remote.c 26 Jun 2007 09:50:53 -0000
@@ -460,6 +460,22 @@ remoteDispatchGetVersion (struct qemud_c
}
static int
+remoteDispatchGetHostname (struct qemud_client *client,
+ remote_message_header *req,
+ void *args ATTRIBUTE_UNUSED,
+ remote_get_hostname_ret *ret)
+{
+ char *hostname;
+ CHECK_CONN(client);
+
+ hostname = virConnectGetHostname (client->conn);
+ if (hostname == NULL) return -1;
+
+ ret->hostname = hostname;
+ return 0;
+}
Index: qemud/remote_protocol.x
===================================================================
RCS file: /data/cvs/libvirt/qemud/remote_protocol.x,v
retrieving revision 1.2
diff -u -p -r1.2 remote_protocol.x
--- qemud/remote_protocol.x 22 Jun 2007 13:16:10 -0000 1.2
+++ qemud/remote_protocol.x 26 Jun 2007 09:50:53 -0000
@@ -178,6 +178,10 @@ struct remote_get_version_ret {
hyper hv_ver;
};
+struct remote_get_hostname_ret {
+ remote_nonnull_string hostname;
+};
+
struct remote_get_max_vcpus_args {
/* The only backend which supports this call is Xen HV, and
* there the type is ignored so it could be NULL.
@@ -605,7 +609,8 @@ enum remote_procedure {
REMOTE_PROC_DOMAIN_SAVE = 55,
REMOTE_PROC_DOMAIN_GET_SCHEDULER_TYPE = 56,
REMOTE_PROC_DOMAIN_GET_SCHEDULER_PARAMETERS = 57,
- REMOTE_PROC_DOMAIN_SET_SCHEDULER_PARAMETERS = 58
+ REMOTE_PROC_DOMAIN_SET_SCHEDULER_PARAMETERS = 58,
+ REMOTE_PROC_GET_HOSTNAME = 59
};
I'm just a bit surprized about this, shouldn't the uri be copied in
the local stub instead of forwarding the call on the wire ? I don't see
how it could change and that avoid an RPC, that something we can cache without
side effect, right ? So why add this to the set of remote calls ?
also it touches qemud.c I'm afraid this may interefere with Dan's patches
right ?
@@ -1052,12 +1055,15 @@ static int qemuNetworkOpen(virConnectPtr
static int
qemuNetworkClose (virConnectPtr conn)
{
- qemuNetworkPrivatePtr netpriv = (qemuNetworkPrivatePtr)
conn->networkPrivateData;
-
- if (!netpriv->shared)
- close(netpriv->qemud_fd);
- free(netpriv);
- conn->networkPrivateData = NULL;
+ if (STRNEQ (conn->driver->name, "remote")) {
+ qemuNetworkPrivatePtr netpriv =
+ (qemuNetworkPrivatePtr) conn->networkPrivateData;
+
+ if (!netpriv->shared)
+ close(netpriv->qemud_fd);
+ free(netpriv);
+ conn->networkPrivateData = NULL;
+ }
return 0;
}
[...]
diff -u -p -r1.7 remote_internal.c
--- src/remote_internal.c 25 Jun 2007 13:05:03 -0000 1.7
+++ src/remote_internal.c 26 Jun 2007 09:50:59 -0000
@@ -58,6 +58,7 @@ struct private_data {
gnutls_session_t session; /* GnuTLS session (if uses_tls != 0). */
char *type; /* Cached return from remoteType. */
int counter; /* Generates serial numbers for RPC. */
+ char *uri; /* Original (remote) URI. */
};
so it's cached or it's forwarded, I'm getting confused there.
+/* This call is unusual because it doesn't go over RPC. The
+ * full URI is known (only) at the client end of the connection.
+ */
Ahhh, okay, fine :-)
+static char *
+remoteGetURI (virConnectPtr conn)
+{
+ GET_PRIVATE (conn, NULL);
+ char *str;
+
+ str = strdup (priv->uri);
+ if (str == NULL) {
+ error (conn, VIR_ERR_SYSTEM_ERROR, strerror (errno));
+ return NULL;
+ }
+ return str;
+}
+
#ifdef WITH_TEST
[...]
in the test driver.
+
+#define _GNU_SOURCE /* for asprintf */
+
[...]
+
+char *
+testGetURI (virConnectPtr conn)
+{
+ testPrivatePtr priv = (testPrivatePtr) conn->privateData;
+ char *uri;
+
+ if (asprintf (&uri, "test://%s", priv->path) == -1) {
Can we avoid using asprintf, especially in new code, I don't think the
convenience wins over the portability problem, thanks
Or it's just a conveninent way to indicate where a new path creation
routine should be added...
+ testError (conn, NULL, VIR_ERR_SYSTEM_ERROR, strerror
(errno));
+ return NULL;
+ }
+ return uri;
+}
+
int testNodeGetInfo(virConnectPtr conn,
virNodeInfoPtr info)
{
[...]
@@ -129,13 +130,20 @@ xenUnifiedOpen (virConnectPtr conn, cons
xmlFreeURI(uri);
/* Allocate per-connection private data. */
- priv = malloc (sizeof *priv);
+ priv = calloc (1, sizeof *priv);
hum, good idea
if (!priv) {
xenUnifiedError (NULL, VIR_ERR_NO_MEMORY, "allocating private data");
return VIR_DRV_OPEN_ERROR;
}
conn->privateData = priv;
[...]
--- src/xen_unified.h 30 Apr 2007 16:57:15 -0000 1.3
+++ src/xen_unified.h 26 Jun 2007 09:51:05 -0000
@@ -50,6 +50,9 @@ struct _xenUnifiedPrivate {
* xen_unified.c.
*/
int opened[XEN_UNIFIED_NR_DRIVERS];
+
+ /* Canonical URI. */
+ char *name;
};
is that the canonical URI or the FQDN ? Maybe the field need to be renamed
(and possibly the comment).
I think there is a tiny bot of cleanup needed, but +1 in principle :-)
thanks !
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
6 a.m.
Daniel Veillard wrote:
> I haven't updated the Python bindings, but will do so next.
it's probable that those 2 calles will be handled automatically
since the arg is a known structure and the return value is a string.
It's a matter of rebuilding the API xml with the new headers once applied
and let the generator do its job.
I'll check. I'm really ill today ...
Can we avoid using asprintf, especially in new code, I don't
think the
convenience wins over the portability problem, thanks
Or it's just a conveninent way to indicate where a new path creation
routine should be added...
Even better, can we add asprintf to the code and compile it into libvirt
if configure.in detects that it is absent from libc? Asprintf is really
useful...
> --- src/xen_unified.h 30 Apr 2007 16:57:15 -0000 1.3
> +++ src/xen_unified.h 26 Jun 2007 09:51:05 -0000
> @@ -50,6 +50,9 @@ struct _xenUnifiedPrivate {
> * xen_unified.c.
> */
> int opened[XEN_UNIFIED_NR_DRIVERS];
> +
> + /* Canonical URI. */
> + char *name;
> };
is that the canonical URI or the FQDN ? Maybe the field need to be renamed
(and possibly the comment).
It's the canonical URI. For example NULL || "xen" ->
"xen:///":
# src/virsh -c xen uri
xen:///
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
6:42 a.m.
On Tue, Jun 26, 2007 at 12:00:01PM +0100, Richard W.M. Jones wrote:
Daniel Veillard wrote:
>>I haven't updated the Python bindings, but will do so next.
>
> it's probable that those 2 calles will be handled automatically
>since the arg is a known structure and the return value is a string.
>It's a matter of rebuilding the API xml with the new headers once applied
>and let the generator do its job.
I'll check. I'm really ill today ...
no hurry :-)
>Can we avoid using asprintf, especially in new code, I don't
think the
>convenience wins over the portability problem, thanks
>Or it's just a conveninent way to indicate where a new path creation
>routine should be added...
Even better, can we add asprintf to the code and compile it into libvirt
if configure.in detects that it is absent from libc? Asprintf is really
useful...
And then I would not call it asprintf, and would rather have it used
on all arches. We need a routine for building paths dynamically, that's where
we use asprintf in teh code, and we could do very easilly with a vararg function
using a MAX_PATH buffer. Using a specialized function is more in line with
the actual code and allows for a simpler implementation.
>>--- src/xen_unified.h 30 Apr 2007 16:57:15 -0000 1.3
>>+++ src/xen_unified.h 26 Jun 2007 09:51:05 -0000
>>@@ -50,6 +50,9 @@ struct _xenUnifiedPrivate {
>> * xen_unified.c.
>> */
>> int opened[XEN_UNIFIED_NR_DRIVERS];
>>+
>>+ /* Canonical URI. */
>>+ char *name;
>> };
>
> is that the canonical URI or the FQDN ? Maybe the field need to be
> renamed
>(and possibly the comment).
It's the canonical URI. For example NULL || "xen" ->
"xen:///":
so let's rename the field to canonical_uri !
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
7:21 a.m.
Daniel Veillard wrote:
On Tue, Jun 26, 2007 at 11:09:21AM +0100, Richard W.M. Jones wrote:
> I haven't updated the Python bindings, but will do so next.
it's probable that those 2 calles will be handled automatically
since the arg is a known structure and the return value is a string.
It's a matter of rebuilding the API xml with the new headers once applied
and let the generator do its job.
Rather obscurely, docs/apibuild.py was having trouble parsing
remote_internal.c, which was causing the API XML files not to build,
which was causing the Python API not to build correctly either.
Attached patch corrects that, corrects a bug in python/Makefile.am
(libvirt.py not being rebuilt), and rearranges python/Makefile.am to
make it a bit easier to understand.
So now it does build Python bindings for getHostname and getURI.
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
8:42 a.m.
On Tue, Jun 26, 2007 at 01:21:35PM +0100, Richard W.M. Jones wrote:
Daniel Veillard wrote:
>On Tue, Jun 26, 2007 at 11:09:21AM +0100, Richard W.M. Jones wrote:
>>I haven't updated the Python bindings, but will do so next.
>
> it's probable that those 2 calles will be handled automatically
>since the arg is a known structure and the return value is a string.
>It's a matter of rebuilding the API xml with the new headers once applied
>and let the generator do its job.
Rather obscurely, docs/apibuild.py was having trouble parsing
remote_internal.c, which was causing the API XML files not to build,
which was causing the Python API not to build correctly either.
yeah the C parser of apibuild can be nasty at times,
Attached patch corrects that, corrects a bug in python/Makefile.am
(libvirt.py not being rebuilt), and rearranges python/Makefile.am to
make it a bit easier to understand.
So now it does build Python bindings for getHostname and getURI.
Looks fine, +1
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
6359
days inactive
6359
days old
5 comments
2 participants
participants (2)
-
Daniel Veillard -
Richard W.M. Jones