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