[Libvir] PATCH 1/2 QEMU driver - internal driver

[Daniel Veillard]

On Tue, Feb 13, 2007 at 07:03:42PM +0000, Daniel P. Berrange wrote:
> The attached patch implements the library driver for QEMU.
> 
> The driver is pretty much identical in style to the xen proxy driver. There
> are two supported URLs:
> 
>    qemu:///session  - a per-user (private) daemon.Can be run by unprivileged
>                       users. Config files kept in $HOME/.qemu and the socket
>                       is in the abstract namespace at $HOME/.qemu/sock
>                       The daemon for session instance is spawned on demand
> 
>    qemu:///system   - a per-machine (public) daemon. Must be launched ahead
>                       of time by root. Config files kept in /etc/qemu and
>                       socket on the FS at /var/run/qemu/sock & sock-ro
>                       The read-write socket is restricted to root only, while
>                       the read-only socket is public. 
> 
> This patch also:
> 
>   - makes virsh use a read-write connection by default
>   - adds extra error info to virterror & friends

  Looks just fine, but I still have a few comments see below :-)

[...]

> +    /* Block sending entire outgoing packet */
> +    while (outLeft) {
> +        int got = write(conn->handle, out+outDone, outLeft);

  stylistic, I prefer to have the variables defined at the function level,
but I could understand why one would argue otherwise :-)
Appears in many other places, definitely not a blocker though !

> +
> +int qemuListDomains(virConnectPtr conn,
> +                     int *ids,
> +                     int maxids){
> +    struct qemud_packet req, reply;
> +    int i, nDomains;
> +
> +    req.header.type = QEMUD_PKT_LIST_DOMAINS;
> +    req.header.dataSize = 0;
> +
> +    if (qemuProcessRequest(conn, NULL, &req, &reply) < 0) {
> +        return -1;
> +    }
> +
> +    nDomains = reply.data.listDomainsReply.numDomains;
> +    if (nDomains > maxids)
> +        return -1;

  Is the semantic really to error instead of truncating in that case ?
it seems to me the code in other drivers just pass the first maxids ones.

> +    for (i = 0 ; i < nDomains ; i++) {
> +        ids[i] = reply.data.listDomainsReply.domains[i];
> +    }
> +
> +    return nDomains;
> +}
> +
> +
> +virDomainPtr
> +qemuDomainCreateLinux(virConnectPtr conn, const char *xmlDesc,
> +                       unsigned int flags ATTRIBUTE_UNUSED){
> +    struct qemud_packet req, reply;
> +    virDomainPtr dom;
> +    int len = strlen(xmlDesc);
> +
> +    if (len > (QEMUD_MAX_XML_LEN-1)) {

  maybe we need to provide a clear error there

> +        return NULL;
> +    }
> +

> +int qemuListDefinedDomains(virConnectPtr conn,
> +int qemuDomainCreate(virDomainPtr dom) {
> +virDomainPtr qemuDomainDefineXML(virConnectPtr conn, const char *xml) {
> +int qemuUndefine(virDomainPtr dom) {

Seems to me all of these drivers entry point should be made static since
they are not exported from the .h, right ? Only the registration routine
ought to be exported (very clean :-).

> +#ifndef __VIR_QEMU_INTERNAL_H__
> +#define __VIR_QEMU_INTERNAL_H__
> +
> +#include <libvirt/virterror.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +    void qemuRegister(void);
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +#endif /* __VIR_QEMU_INTERNAL_H__ */

  Feel free to push to CVS,

    thanks !

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/