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