Re: [Libvir] PATCH 2/2: Support QEMU (+KVM) in libvirt

On Tue, Jan 09, 2007 at 09:44:44AM -0500, Daniel Veillard wrote: > On Fri, Jan 05, 2007 at 09:18:53PM +0000, Daniel P. Berrange wrote: > > The attached patch provides a new driver backend for libvirt which allows > > management of QEMU instances by talking to the libvirt QEMU daemon. It > > basically just marshalls libvirt API calls onto the wire & de-marshalls > > the reply. All the interesting functionality stuff is in the daemon. > > Okay, but I'm getting a bit lost about the potential daemons running, > there one can be autostarted, in the previous patch we also potentially > fork(fork(daemon)) so I'm wondering how many process are actually running > in the end, maybe I will need a couple of pictures, like in the current > architecture page http://libvirt.org/architecture.html (which will have > to be updated too :-) There's two ways its launched: - The session bus is auto-launched by libvirt. In this scenaro, libvirt does the double fork() magic to dis-inherit the libvirt_qemud process - The system bus is started manually. In this case, it runs in foreground unless you use the --daemon command line argument to make it do the double-fork() into background.

> Shouldn't qemud_internal.c gets its own error reporting function like the > other _internal.c which does the right stuff, I only spotted returns of > error values, and this is linked to the client binary so this seems to be > missing, right ? The only errors that should be occurring in qemud_internal.c are two classes: - Data transport errors (eg, failure to connect, TLS failures, etc) - Errors forwarded back from the daemon over the wire. I already deal with the latter. The former probably needs a little more work.

> > RCS file: /data/cvs/libvirt/src/virsh.c,v > > retrieving revision 1.41 > > diff -u -p -r1.41 virsh.c > > --- src/virsh.c 22 Nov 2006 17:48:29 -0000 1.41 > > +++ src/virsh.c 5 Jan 2007 21:09:08 -0000 > > @@ -292,7 +292,7 @@ cmdConnect(vshControl * ctl, vshCmd * cm > > > > if (ctl->name) > > free(ctl->name); > > - ctl->name = vshCommandOptString(cmd, "name", NULL); > > + ctl->name = vshStrdup(ctl, vshCommandOptString(cmd, "name", NULL)); > > > I'm a bit lost on that one, was that a separate bug ? > We switch from a static string to dynamically allocated one, right ? Opps, this is compeltely unrelated to QEMU - its a general bug. The string from vshCommandOptString is just a pointer to its internal buffer. This is free'd next time user enters a command in virsh. So by storing it in ctl->name we keep a pointer to a free'd string, with predictable SEGV a short while later. Simply dup'ing the string is enough.

> > @@ -268,6 +268,9 @@ virDefaultErrorFunc(virErrorPtr err) > > case VIR_FROM_RPC: > > dom = "XML-RPC "; > > break; > > + case VIR_FROM_QEMUD: > > + dom = "QEMUD "; > > + break; > > } > > if ((err->dom != NULL) && (err->code != VIR_ERR_INVALID_DOMAIN)) { > > domain = err->dom->name; > > I guess some new error code will need to be added too once quemud_internal.c > is being updated ... Hum, how do we process errors provided by the server ? The libvirt_qemud daemon includes <libvirt/virterror.h> and sends back the appropriate error codes over the wire, along with a message. This is used to call __virRaiseError - see the qemudPacketError() function at the top of the qemud_internal.c file - this function is called whenver the wire protocol gives back an unexpected result