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

Re: [libvirt] PATCH: Public API plumbing for virNodeDeviceCreateXML/Destroy



On Fri, Apr 24, 2009 at 01:08:30PM +0100, Daniel P. Berrange wrote:
> As previously discussed, Dave Allen's reworking the NPIV patches to 
> do creation/deleation of vports via the node device APIs. Thus, this
> patch adds the 2 new APIs required for this approach.

  I would really prefer to see this API in 0.6.3, even if there
is no backend implementation, this should ease our work and also
allow other people with distinct node storage implementation to
provide patches for this too. For this reason I would prefer to see
the API go in as earlier as possible in an official release to
allow people to work on those as needed.

> diff -r 2d1278bdf31f include/libvirt/libvirt.h
> --- a/include/libvirt/libvirt.h	Fri Apr 24 11:01:11 2009 +0100
> +++ b/include/libvirt/libvirt.h	Fri Apr 24 13:06:12 2009 +0100
> @@ -1124,6 +1124,12 @@ int                     virNodeDeviceDet
>  int                     virNodeDeviceReAttach   (virNodeDevicePtr dev);
>  int                     virNodeDeviceReset      (virNodeDevicePtr dev);
>  
> +virNodeDevicePtr        virNodeDeviceCreateXML  (virConnectPtr conn,
> +                                                 const char *xmlDesc,
> +                                                 unsigned int flags);
> +
> +int                     virNodeDeviceDestroy    (virNodeDevicePtr dev);
> +
>  /*
>   * Domain Event Notification
>   */

  Okay this API looks fine to me. I think we can push it as part of
0.6.3,

> diff -r 2d1278bdf31f qemud/remote.c
> --- a/qemud/remote.c	Fri Apr 24 11:01:11 2009 +0100
> +++ b/qemud/remote.c	Fri Apr 24 13:06:12 2009 +0100
> @@ -4323,6 +4323,54 @@ remoteDispatchNodeDeviceReset (struct qe

  Okay this is directly dependant on the signature of the 2 functions.

> diff -r 2d1278bdf31f qemud/remote_dispatch_args.h
> --- a/qemud/remote_dispatch_args.h	Fri Apr 24 11:01:11 2009 +0100
> +++ b/qemud/remote_dispatch_args.h	Fri Apr 24 13:06:12 2009 +0100

  same here 

> diff -r 2d1278bdf31f qemud/remote_dispatch_prototypes.h
> --- a/qemud/remote_dispatch_prototypes.h	Fri Apr 24 11:01:11 2009 +0100
> +++ b/qemud/remote_dispatch_prototypes.h	Fri Apr 24 13:06:12 2009 +0100
> @@ -527,6 +527,20 @@ static int remoteDispatchNetworkUndefine

  and here

> diff -r 2d1278bdf31f qemud/remote_dispatch_ret.h
> --- a/qemud/remote_dispatch_ret.h	Fri Apr 24 11:01:11 2009 +0100
> +++ b/qemud/remote_dispatch_ret.h	Fri Apr 24 13:06:12 2009 +0100

  and here

> diff -r 2d1278bdf31f qemud/remote_dispatch_table.h
> --- a/qemud/remote_dispatch_table.h	Fri Apr 24 11:01:11 2009 +0100
> +++ b/qemud/remote_dispatch_table.h	Fri Apr 24 13:06:12 2009 +0100
[...]
> -{   /* DomainGetSecurityLabel => 118 */
> +{   /* DomainGetSecurityLabel => 121 */
[...]
> -{   /* NodeGetSecurityModel => 119 */
> +{   /* NodeGetSecurityModel => 122 */

  Any reason for the renumbering even though it seems harmless ?

  skipping generated stuff...

> diff -r 2d1278bdf31f qemud/remote_protocol.x
> --- a/qemud/remote_protocol.x	Fri Apr 24 11:01:11 2009 +0100
> +++ b/qemud/remote_protocol.x	Fri Apr 24 13:06:12 2009 +0100
> @@ -1109,6 +1109,19 @@ struct remote_node_device_reset_args {
>      remote_nonnull_string name;
>  };
>  
> +struct remote_node_device_create_xml_args {
> +    remote_nonnull_string xml_desc;
> +    int flags;
> +};
> +
> +struct remote_node_device_create_xml_ret {
> +    remote_nonnull_node_device dev;
> +};
> +
> +struct remote_node_device_destroy_args {
> +    remote_nonnull_string name;
> +};
> +
>  
>  /**
>   * Events Register/Deregister:
> @@ -1270,7 +1283,10 @@ enum remote_procedure {
>      REMOTE_PROC_NODE_DEVICE_RESET = 120,
>  
>      REMOTE_PROC_DOMAIN_GET_SECURITY_LABEL = 121,
> -    REMOTE_PROC_NODE_GET_SECURITY_MODEL = 122
> +    REMOTE_PROC_NODE_GET_SECURITY_MODEL = 122,
> +
> +    REMOTE_PROC_NODE_DEVICE_CREATE_XML = 123,
> +    REMOTE_PROC_NODE_DEVICE_DESTROY = 124
>  };
>  

   looks fine

> diff -r 2d1278bdf31f src/driver.h
> --- a/src/driver.h	Fri Apr 24 11:01:11 2009 +0100
> +++ b/src/driver.h	Fri Apr 24 13:06:12 2009 +0100
> @@ -684,6 +684,11 @@ typedef int (*virDevMonDeviceListCaps)(v
>                                         char **const names,
>                                         int maxnames);
>  
> +typedef virNodeDevicePtr (*virDrvNodeDeviceCreateXML)(virConnectPtr conn,
> +                                                      const char *xmlDesc,
> +                                                      unsigned int flags);
> +typedef int (*virDrvNodeDeviceDestroy)(virNodeDevicePtr dev);
> +
>  /**
>   * _virDeviceMonitor:
>   *
> @@ -702,6 +707,8 @@ struct _virDeviceMonitor {
>      virDevMonDeviceGetParent deviceGetParent;
>      virDevMonDeviceNumOfCaps deviceNumOfCaps;
>      virDevMonDeviceListCaps deviceListCaps;
> +    virDrvNodeDeviceCreateXML deviceCreateXML;
> +    virDrvNodeDeviceDestroy deviceDestroy;
>  };
>  

  okay

> diff -r 2d1278bdf31f src/libvirt.c
> --- a/src/libvirt.c	Fri Apr 24 11:01:11 2009 +0100
> +++ b/src/libvirt.c	Fri Apr 24 13:06:12 2009 +0100
[...]
> +/**
> + * virNodeDeviceCreateXML:
> + * @conn: pointer to the hypervisor connection
> + * @xmlDesc: string containing an XML description of the device to be created
> + * @flags: callers should always pass 0
> + *
> + * Create a new device on the VM host machine, for example, virtual
> + * HBAs created using vport_create.
> + *
> + * Returns a node device object if successful, NULL in case of failure
> + */
> +virNodeDevicePtr
> +virNodeDeviceCreateXML(virConnectPtr conn,
> +                       const char *xmlDesc,
> +                       unsigned int flags)
> +{
> +    VIR_DEBUG("conn=%p, xmlDesc=%s, flags=%d", conn, xmlDesc, flags);
> +
> +    virResetLastError();
> +
> +    if (!VIR_IS_CONNECT(conn)) {
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        return NULL;
> +    }
> +
> +    if (conn->flags & VIR_CONNECT_RO) {
> +        virLibConnError(conn, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
> +        goto error;
> +    }
> +
> +    if (xmlDesc == NULL) {
> +        virLibConnError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        goto error;
> +    }
> +
> +    if (conn->deviceMonitor->deviceCreateXML) {

  are we always 100% sure conn->deviceMonitor is non NULL ?

> +        virNodeDevicePtr dev = conn->deviceMonitor->deviceCreateXML(conn, xmlDesc, flags);
> +        if (dev == NULL)
> +            goto error;
> +        return dev;
> +    }
> +
> +    virLibConnError (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__);
> +
> +error:
> +    /* Copy to connection error object for back compatability */
> +    virSetConnError(conn);
> +    return NULL;
> +}
> +
> +
> +/**
> + * virNodeDeviceDestroy:
> + * @dev: a device object
> + *
> + * Destroy the device object. The virtual device is removed from the host operating system.
> + * This function may require privileged access
> + *
> + * Returns 0 in case of success and -1 in case of failure.
> + */
> +int
> +virNodeDeviceDestroy(virNodeDevicePtr dev)
> +{
> +    int retval = 0;
> +
> +    DEBUG("dev=%p", dev);
> +
> +    virResetLastError();
> +
> +    if (!VIR_IS_CONNECTED_NODE_DEVICE(dev)) {
> +        virLibNodeDeviceError(NULL, VIR_ERR_INVALID_NODE_DEVICE, __FUNCTION__);
> +        return (-1);
> +    }
> +
> +    if (dev->conn->flags & VIR_CONNECT_RO) {
> +        virLibConnError(dev->conn, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
> +        goto error;
> +    }
> +
> +    if (dev->conn->deviceMonitor->deviceDestroy) {
> +        retval = dev->conn->deviceMonitor->deviceDestroy(dev);
> +        if (retval < 0) {
> +            goto error;
> +        }
> +
> +        return 0;
> +    }
> +
> +    virLibConnError (dev->conn, VIR_ERR_NO_SUPPORT, __FUNCTION__);
> +
> +error:
> +    /* Copy to connection error object for back compatability */
> +    virSetConnError(dev->conn);
> +    return -1;
> +}
> +
> +

  okay looks fine

> diff -r 2d1278bdf31f src/libvirt_public.syms
> --- a/src/libvirt_public.syms	Fri Apr 24 11:01:11 2009 +0100
> +++ b/src/libvirt_public.syms	Fri Apr 24 13:06:12 2009 +0100
> @@ -258,4 +258,10 @@ LIBVIRT_0.6.1 {
>  	virNodeGetSecurityModel;
>  } LIBVIRT_0.6.0;
>  
> +LIBVIRT_0.6.3 {
> +    global:
> +	virNodeDeviceCreateXML;
> +	virNodeDeviceDestroy;
> +} LIBVIRT_0.6.1;
> +
>  # .... define new API here using predicted next version number ....

  fine too

> diff -r 2d1278bdf31f src/remote_internal.c
> --- a/src/remote_internal.c	Fri Apr 24 11:01:11 2009 +0100
> +++ b/src/remote_internal.c	Fri Apr 24 13:06:12 2009 +0100

  okay.

  ACK, fine for commit, though if you could double check the couple
or questions raised,

  thanks !

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


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