Re: [Libvir] PATCH 1/2 QEMU driver - internal driver
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/