[libvirt] [PATCH v3 4/5] Add a watchdog action `dump'

[Hu Tao]

On Tue, Nov 30, 2010 at 03:21:36PM -0700, Eric Blake wrote:
<...snip...>
> >  
> > -libvirt_qemu_la_SOURCES = libvirt-qemu.c
> > +libvirt_qemu_la_SOURCES = libvirt-qemu.c util/threadpool.c
> 
> Why is this change necessary?  Shouldn't libvirt_util.la already include
> threadpool.c, and the qemu driver already be linking with libvirt_util.la?

Is this ok?

-libvirt_driver_qemu_la_LIBADD = $(NUMACTL_LIBS)
+libvirt_driver_qemu_la_LIBADD = $(NUMACTL_LIBS) ../src/libvirt_util.la

Or link will fail:

  CCLD   libvirtd
libvirtd-libvirtd.o: In function `qemudRunLoop':
/mnt/data/kernel/libvirt/daemon/libvirtd.c:2229: undefined reference to `virWorkerPoolNew'
/mnt/data/kernel/libvirt/daemon/libvirtd.c:2287: undefined reference to `virWorkerPoolFree'
libvirtd-libvirtd.o: In function `qemudDispatchClientRead':
/mnt/data/kernel/libvirt/daemon/libvirtd.c:1778: undefined reference to `virWorkerPoolSendJob'
../src/.libs/libvirt_driver_qemu.a(libvirt_driver_qemu_la-qemu_driver.o): In function `qemuHandleDomainWatchdog':
/mnt/data/kernel/libvirt/src/qemu/qemu_driver.c:1224: undefined reference to `virWorkerPoolSendJob'
../src/.libs/libvirt_driver_qemu.a(libvirt_driver_qemu_la-qemu_driver.o): In function `qemudShutdown':
/mnt/data/kernel/libvirt/src/qemu/qemu_driver.c:2147: undefined reference to `virWorkerPoolFree'
../src/.libs/libvirt_driver_qemu.a(libvirt_driver_qemu_la-qemu_driver.o): In function `qemudStartup':
/mnt/data/kernel/libvirt/src/qemu/qemu_driver.c:2007: undefined reference to `virWorkerPoolNew'
collect2: ld returned 1 exit status

 
<...snip...>

> > +
> >      virDomainObjUnlock(vm);
> >  
> >      if (watchdogEvent || lifecycleEvent) {
> > @@ -1788,6 +1807,9 @@ qemudStartup(int privileged) {
> >          if (virAsprintf(&qemu_driver->snapshotDir,
> >                          "%s/lib/libvirt/qemu/snapshot", LOCALSTATEDIR) == -1)
> >              goto out_of_memory;
> > +        if (virAsprintf(&qemu_driver->autoDumpPath,
> > +                        "%s/lib/libvirt/qemu/dump", LOCALSTATEDIR) == -1)
> 
> At first glance, I'm not quite sure why autoDumpPath is configurable but
> not snapshotDir.  I guess it has to do with the fact that snapshots are
> under libvirt control (the user does not need to know that they exist),
> but dump files are intended to be consumed by the user (so the user
> should be able to specify an alternate location).

Yes.

> 
> > +            goto out_of_memory;
> >      } else {
> >          uid_t uid = geteuid();
> >          char *userdir = virGetUserDirectory(uid);
> > @@ -1816,6 +1838,8 @@ qemudStartup(int privileged) {
> >              goto out_of_memory;
> >          if (virAsprintf(&qemu_driver->snapshotDir, "%s/qemu/snapshot", base) == -1)
> >              goto out_of_memory;
> > +        if (virAsprintf(&qemu_driver->autoDumpPath, "%s/qemu/dump", base) == -1)
> > +            goto out_of_memory;
> 
> However, it does raise an issue.  Is qemu.conf only for privileged
> users, or do we have to worry about allowing non-privileged users also
> be able to pick up an alternate directory (especially since they can't
> dump to /var/log/...)?

qemu.conf is only for privileged users, but non-privileged users who
need to analyze dump files should ask admin to specify an auto-dump
directory they have right to read.

Or do you have a better idea?


-- 
Thanks,
Hu Tao