subject:"\[libvirt\] \[PATCH v3 4\/5\] Add a watchdog action `dump'"

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

2010-12-02 Thread Daniel P. Berrange

On Wed, Dec 01, 2010 at 06:39:14PM -0700, Eric Blake wrote:
 On 12/01/2010 05:50 PM, Hu Tao wrote:
  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
 
 Nope; rather...
 
  
  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'
 
 That means you need to modify src/libvirt_private.syms to export the new
 public interfaces from threadpool.h (it should be pretty easy to figure
 out what edits to make, the tricky part is realizing you need to touch
 that file in the first place).
 
  +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?
 
 Is it better to dump a non-privileged log into ~/.libvirt/qemu/dump, so
 that it's automatically user-accessible?  We already use ~/.libvirt for
 other non-privileged files.

I think you're mixing up unprivileged users using the privileged
qemu:///system, with unprivileged users using the unprivileged
driver qemu://session. Only the latter uses ~/.libvirt and this
patch should already be using ~/.libvirt/qemu/dump in that
scenario.

Daniel

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2010-12-01 Thread 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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2010-12-01 Thread Eric Blake

On 12/01/2010 05:50 PM, Hu Tao wrote:
 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

Nope; rather...

 
 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'

That means you need to modify src/libvirt_private.syms to export the new
public interfaces from threadpool.h (it should be pretty easy to figure
out what edits to make, the tricky part is realizing you need to touch
that file in the first place).

 +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?

Is it better to dump a non-privileged log into ~/.libvirt/qemu/dump, so
that it's automatically user-accessible?  We already use ~/.libvirt for
other non-privileged files.

-- 
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2010-11-30 Thread Eric Blake

On 11/30/2010 12:13 AM, Hu Tao wrote:
 `dump' watchdog action lets libvirtd to dump the guest when receives a
 watchdog event (which probably means a guest crash)
 
 Currently only qemu is supported.
 ---
  src/Makefile.am|2 +-
  src/conf/domain_conf.c |1 +
  src/conf/domain_conf.h |1 +
  src/qemu/qemu.conf |5 +++
  src/qemu/qemu_conf.c   |   13 +++-
  src/qemu/qemu_conf.h   |4 ++
  src/qemu/qemu_driver.c |   75 
 
  7 files changed, 99 insertions(+), 2 deletions(-)
 
 diff --git a/src/Makefile.am b/src/Makefile.am
 index 5febd76..9484c2d 100644
 --- a/src/Makefile.am
 +++ b/src/Makefile.am
 @@ -1089,7 +1089,7 @@ libvirt_test_la_LIBADD = $(libvirt_la_LIBADD)
  libvirt_test_la_LDFLAGS = $(test_LDFLAGS) $(AM_LDFLAGS)
  libvirt_test_la_CFLAGS = $(AM_CFLAGS)
  
 -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?

  libvirt_qemu_la_LDFLAGS = $(VERSION_SCRIPT_FLAGS)$(LIBVIRT_QEMU_SYMBOL_FILE) 
 \
-version-info $(LIBVIRT_VERSION_INFO) \
$(CYGWIN_EXTRA_LDFLAGS) $(MINGW_EXTRA_LDFLAGS) \
 diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
 index 3f14cee..a6cb444 100644
 --- a/src/conf/domain_conf.c
 +++ b/src/conf/domain_conf.c
 @@ -245,6 +245,7 @@ VIR_ENUM_IMPL(virDomainWatchdogAction, 
 VIR_DOMAIN_WATCHDOG_ACTION_LAST,
shutdown,
poweroff,
pause,
 +  dump,
none)
  
  VIR_ENUM_IMPL(virDomainVideo, VIR_DOMAIN_VIDEO_TYPE_LAST,
 diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
 index 899b19f..7f50b79 100644
 --- a/src/conf/domain_conf.h
 +++ b/src/conf/domain_conf.h
 @@ -462,6 +462,7 @@ enum virDomainWatchdogAction {
  VIR_DOMAIN_WATCHDOG_ACTION_SHUTDOWN,
  VIR_DOMAIN_WATCHDOG_ACTION_POWEROFF,
  VIR_DOMAIN_WATCHDOG_ACTION_PAUSE,
 +VIR_DOMAIN_WATCHDOG_ACTION_DUMP,
  VIR_DOMAIN_WATCHDOG_ACTION_NONE,
  
  VIR_DOMAIN_WATCHDOG_ACTION_LAST
 diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
 index f4f965e..fb35ebc 100644
 --- a/src/qemu/qemu.conf
 +++ b/src/qemu/qemu.conf
 @@ -191,6 +191,11 @@
  # save_image_format = raw
  # dump_image_format = raw
  
 +# When a domain is configured to be auto-dumped when libvirtd receives a
 +# watchdog event from qemu guest, libvirtd will saves dump files in directory

s/saves/save/

 +# specified by auto_dump_path. Default value is /var/lib/libvirt/qemu/dump
 +#
 +# auto_dump_path = /var/lib/libvirt/qemu/dump
  
  # If provided by the host and a hugetlbfs mount point is configured,
  # a guest may request huge page backing.  When this mount point is
 diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
 index 35caccc..accd0c4 100644
 --- a/src/qemu/qemu_conf.c
 +++ b/src/qemu/qemu_conf.c
 @@ -386,6 +386,17 @@ int qemudLoadDriverConfig(struct qemud_driver *driver,
  }
  }
  
 +p = virConfGetValue (conf, auto_dump_path);
 +CHECK_TYPE (auto_dump_path, VIR_CONF_STRING);
 +if (p  p-str) {

Where did you provide the default autoDumpPath if auto_dump_path is not
specified in the .conf file?

 +VIR_FREE(driver-autoDumpPath);
 +if (!(driver-autoDumpPath = strdup(p-str))) {
 +virReportOOMError();
 +virConfFree(conf);
 +return -1;
 +}
 +}
 +
   p = virConfGetValue (conf, hugetlbfs_mount);
   CHECK_TYPE (hugetlbfs_mount, VIR_CONF_STRING);
   if (p  p-str) {
 @@ -5369,7 +5380,7 @@ int qemudBuildCommandLine(virConnectPtr conn,
  }
  ADD_ARG(optstr);
  
 -const char *action = 
 virDomainWatchdogActionTypeToString(watchdog-action);
 +const char *action = 
 virDomainWatchdogActionTypeToString(watchdog-action == 
 VIR_DOMAIN_WATCHDOG_ACTION_DUMP ? VIR_DOMAIN_WATCHDOG_ACTION_PAUSE : 
 watchdog-action);

Please wrap to 80 columns when possible.  For example, I would have done
something like:

{
int act = watchdog-action;
if (act == VIR_DOMAIN_WATCHDOG_ACTION_DUMP)
act = VIR_DOMAIN_WATCHDOG_ACTION_PAUSE;
const char *action = virDomainWatchdogActionTypeToString(act);
}

  if (!action) {
  qemuReportError(VIR_ERR_INTERNAL_ERROR,
  %s, _(invalid watchdog action));
 diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
 index 790ce98..72f961a 100644
 --- a/src/qemu/qemu_conf.h
 +++ b/src/qemu/qemu_conf.h
 @@ -106,6 +106,8 @@ enum qemud_cmd_flags {
  struct qemud_driver {
  virMutex lock;
  
 +struct virWorkerPool *workerPool;
 +

Where's the header that declares virWorkerPool?

  int privileged;
  
  uid_t user;
 @@ -173,6 +175,8 @@ struct qemud_driver {
  char *saveImageFormat;
  char *dumpImageFormat;
  
 +char

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

2010-11-29 Thread Hu Tao

`dump' watchdog action lets libvirtd to dump the guest when receives a
watchdog event (which probably means a guest crash)

Currently only qemu is supported.
---
 src/Makefile.am|2 +-
 src/conf/domain_conf.c |1 +
 src/conf/domain_conf.h |1 +
 src/qemu/qemu.conf |5 +++
 src/qemu/qemu_conf.c   |   13 +++-
 src/qemu/qemu_conf.h   |4 ++
 src/qemu/qemu_driver.c |   75 
 7 files changed, 99 insertions(+), 2 deletions(-)

diff --git a/src/Makefile.am b/src/Makefile.am
index 5febd76..9484c2d 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1089,7 +1089,7 @@ libvirt_test_la_LIBADD = $(libvirt_la_LIBADD)
 libvirt_test_la_LDFLAGS = $(test_LDFLAGS) $(AM_LDFLAGS)
 libvirt_test_la_CFLAGS = $(AM_CFLAGS)
 
-libvirt_qemu_la_SOURCES = libvirt-qemu.c
+libvirt_qemu_la_SOURCES = libvirt-qemu.c util/threadpool.c
 libvirt_qemu_la_LDFLAGS = $(VERSION_SCRIPT_FLAGS)$(LIBVIRT_QEMU_SYMBOL_FILE) \
   -version-info $(LIBVIRT_VERSION_INFO) \
   $(CYGWIN_EXTRA_LDFLAGS) $(MINGW_EXTRA_LDFLAGS) \
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3f14cee..a6cb444 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -245,6 +245,7 @@ VIR_ENUM_IMPL(virDomainWatchdogAction, 
VIR_DOMAIN_WATCHDOG_ACTION_LAST,
   shutdown,
   poweroff,
   pause,
+  dump,
   none)
 
 VIR_ENUM_IMPL(virDomainVideo, VIR_DOMAIN_VIDEO_TYPE_LAST,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 899b19f..7f50b79 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -462,6 +462,7 @@ enum virDomainWatchdogAction {
 VIR_DOMAIN_WATCHDOG_ACTION_SHUTDOWN,
 VIR_DOMAIN_WATCHDOG_ACTION_POWEROFF,
 VIR_DOMAIN_WATCHDOG_ACTION_PAUSE,
+VIR_DOMAIN_WATCHDOG_ACTION_DUMP,
 VIR_DOMAIN_WATCHDOG_ACTION_NONE,
 
 VIR_DOMAIN_WATCHDOG_ACTION_LAST
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index f4f965e..fb35ebc 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -191,6 +191,11 @@
 # save_image_format = raw
 # dump_image_format = raw
 
+# When a domain is configured to be auto-dumped when libvirtd receives a
+# watchdog event from qemu guest, libvirtd will saves dump files in directory
+# specified by auto_dump_path. Default value is /var/lib/libvirt/qemu/dump
+#
+# auto_dump_path = /var/lib/libvirt/qemu/dump
 
 # If provided by the host and a hugetlbfs mount point is configured,
 # a guest may request huge page backing.  When this mount point is
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 35caccc..accd0c4 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -386,6 +386,17 @@ int qemudLoadDriverConfig(struct qemud_driver *driver,
 }
 }
 
+p = virConfGetValue (conf, auto_dump_path);
+CHECK_TYPE (auto_dump_path, VIR_CONF_STRING);
+if (p  p-str) {
+VIR_FREE(driver-autoDumpPath);
+if (!(driver-autoDumpPath = strdup(p-str))) {
+virReportOOMError();
+virConfFree(conf);
+return -1;
+}
+}
+
  p = virConfGetValue (conf, hugetlbfs_mount);
  CHECK_TYPE (hugetlbfs_mount, VIR_CONF_STRING);
  if (p  p-str) {
@@ -5369,7 +5380,7 @@ int qemudBuildCommandLine(virConnectPtr conn,
 }
 ADD_ARG(optstr);
 
-const char *action = 
virDomainWatchdogActionTypeToString(watchdog-action);
+const char *action = 
virDomainWatchdogActionTypeToString(watchdog-action == 
VIR_DOMAIN_WATCHDOG_ACTION_DUMP ? VIR_DOMAIN_WATCHDOG_ACTION_PAUSE : 
watchdog-action);
 if (!action) {
 qemuReportError(VIR_ERR_INTERNAL_ERROR,
 %s, _(invalid watchdog action));
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 790ce98..72f961a 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -106,6 +106,8 @@ enum qemud_cmd_flags {
 struct qemud_driver {
 virMutex lock;
 
+struct virWorkerPool *workerPool;
+
 int privileged;
 
 uid_t user;
@@ -173,6 +175,8 @@ struct qemud_driver {
 char *saveImageFormat;
 char *dumpImageFormat;
 
+char *autoDumpPath;
+
 pciDeviceList *activePciHostdevs;
 
 virBitmapPtr reservedVNCPorts;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 80ce9f6..550c6da 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -55,6 +55,7 @@
 #include qemu_driver.h
 #include qemu_conf.h
 #include qemu_monitor.h
+#include qemu_monitor_json.h
 #include qemu_bridge_filter.h
 #include c-ctype.h
 #include event.h
@@ -85,6 +86,7 @@
 #include files.h
 #include fdstream.h
 #include configmake.h
+#include threadpool.h
 
 #define VIR_FROM_THIS VIR_FROM_QEMU
 
@@ -137,8 +139,15 @@ struct _qemuDomainObjPrivate {
 int persistentAddrs;
 };
 
+struct watchdogEvent
+{
+virDomainObjPtr vm;
+int action;
+};
+
 static int getCompressionType(struct qemud_driver

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

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

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

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

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

5 matches

Site Navigation

Mail list logo

Footer information