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@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/