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