[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libvir] [PATCH] Add virConnectGetHostname and virConnectGetURI calls, virsh commands, and some related fixes



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/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]