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

Re: [Libvir] PATCH: Fix remote driver to handle network API for local non-QEMU



On Fri, Jun 29, 2007 at 03:08:47PM +0100, Daniel P. Berrange wrote:
> On Wed, Jun 27, 2007 at 07:04:00PM +0100, Daniel P. Berrange wrote:
> > The remote_internal.c code is basically a no-op in its remoteNetworkOpen
> > method. This means the remote driver will only handle the networking
> > APIs, if it is also handling the main domain APIs. This works fine if
> > connecting to a remote URI, or if using QEMU URIs, but it does not work
> > if using the test or Xen drivers.
> > 
> > To make this work the remoteNetworkOpen method needs to open a connection
> > to the remote daemon IIF the remoteOpen method did not already open one.
> > 
> > So the attached patch adds the neccessary logic in remoteNetworkOpen. It
> > also adds code to remoteNetworkClose to make it shutdown& free the connection
> > IIF it was opened by the remoteNetworkOpen method. This is what the extra
> > 'networkOnly' field in the 'struct private_data' is used to check.
> > 
> > Second, all of the implementations of virNetwork* APIs in the remote_internal.c
> > driver must use the  'struct private_data *' from networkPrivateData field
> > in virConnectPtr, not the 'privateData' field. This is because the 'privateData'
> > field is probably storing data related to the Xen or Test drivers.
> > 
> > Third, this also fixes the qemu_driver.c which incorrectly used the 
> > privateData field insteadof the networkPrivateData field for 2 of the
> > networking APIs, and finally makes sure the qemu_driver.c also acccepts
> > the use of qemu:///session APIs for root user.
> 
> A new version of the patch which has all of the above plus:
> 
>  * Fixed the getType function in the qemu driver to return 'QEMU' instead
>    of 'qemu', since the former is what we used to provide.
> 
>  * Adds in auto-start of the libvirtd if using  qemu:///session of
>    test://* (needed for networking) urls as non-root
> 
>  qemu_driver.c     |    9 -
>  remote_internal.c |  382 +++++++++++++++++++++++++++++++++++++++++++-----------
>  2 files changed, 312 insertions(+), 79 deletions(-)
[...]
[...]
> @@ -60,6 +64,7 @@ struct private_data {
>      char *type;                 /* Cached return from remoteType. */
>      int counter;                /* Generates serial numbers for RPC. */
>      char *uri;                  /* Original (remote) URI. */
> +    int networkOnly;            /* Only used for network API */
>  };

small suggestion:
mabye the field name could be made more generic and would be reusable
for more flags.

> @@ -72,6 +77,16 @@ struct private_data {
>      }                                                                   \
>      assert (priv->magic == MAGIC)
>  
> +#define GET_NETWORK_PRIVATE(conn,retcode)                                       \
> +    struct private_data *priv = (struct private_data *) (conn)->networkPrivateData; \
> +    assert (priv);                                                      \
> +    if (priv->magic == DEAD) {                                          \
> +        error (conn, VIR_ERR_INVALID_ARG,                               \
> +               "tried to use a closed or uninitialised handle");        \
> +        return (retcode);                                               \
> +    }                                                                   \
> +    assert (priv->magic == MAGIC)
> +
>  static int call (virConnectPtr conn, struct private_data *priv, int in_open, int proc_nr, xdrproc_t args_filter, char *args, xdrproc_t ret_filter, char *ret);

  Hum .... asserts .... Can't we make a normal test/error/exit handling
so that errors there can be chanelled the same way as any other ?

[...]
> +
> +/* Must not overlap with virDrvOpenFlags */
> +enum {
> +    VIR_DRV_OPEN_REMOTE_UNIX = (1 << 8),
> +    VIR_DRV_OPEN_REMOTE_USER = (1 << 9),
> +    VIR_DRV_OPEN_AUTOSTART = (1 << 10),
> +} virDrvOpenRemoteFlags;

  then maybe both enums definitions should be defined next to each other
finding the requirement when editing somewhere else relies on luck, let's move
the definition closer.


  I don't really see a problem, but I don't have a good understanding of that
code yet, 

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]