I wrote (attached here, and to the bug) a quick patch that sets the process name to the same value as the window title. I'm unsure where to go from here. Should I add support for converting "native" QEMU command lines to libvirt XML? What would that look like, since I'm not modifying the libvirt format? Should it just drop any ,process= from the QEMU command line it's parsing? I also imagine the test cases will need updating.

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 616af6e..a175cd7 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1126,8 +1126,11 @@ static unsigned long long qemudComputeCmdFlags(const char *help, flags |= QEMUD_CMD_FLAG_ENABLE_KVM; if (strstr(help, "-no-reboot")) flags |= QEMUD_CMD_FLAG_NO_REBOOT; - if (strstr(help, "-name")) + if (strstr(help, "-name")) { flags |= QEMUD_CMD_FLAG_NAME; + if (strstr(help, ",process=")) + flags |= QEMUD_CMD_FLAG_NAME_PROCESS; + } if (strstr(help, "-uuid")) flags |= QEMUD_CMD_FLAG_UUID; if (strstr(help, "-xen-domid")) @@ -3550,7 +3553,15 @@ int qemudBuildCommandLine(virConnectPtr conn, if (qemuCmdFlags & QEMUD_CMD_FLAG_NAME) { ADD_ARG_LIT("-name"); - ADD_ARG_LIT(def->name); + if (qemuCmdFlags & QEMUD_CMD_FLAG_NAME_PROCESS) { + char *name; + if (virAsprintf(&name, "%s,process=%s", + def->name, def->name) < 0) + goto no_memory; + ADD_ARG_LIT(name);

[plowing through a stack of small patches that i've been neglecting... sorry for the delay.] On Tue, Mar 30, 2010 at 03:05:06PM +0100, Daniel P. Berrange wrote: > On Sun, Mar 28, 2010 at 10:19:51PM -0400, John Morrissey wrote: > > I wrote (attached here, and to the bug) a quick patch that sets the > > process name to the same value as the window title. [snip] > > @@ -3550,7 +3553,15 @@ int qemudBuildCommandLine(virConnectPtr conn, > > > > if (qemuCmdFlags & QEMUD_CMD_FLAG_NAME) { > > ADD_ARG_LIT("-name"); > > - ADD_ARG_LIT(def->name); > > + if (qemuCmdFlags & QEMUD_CMD_FLAG_NAME_PROCESS) { > > + char *name; > > + if (virAsprintf(&name, "%s,process=%s", > > + def->name, def->name) < 0) > > + goto no_memory; > > + ADD_ARG_LIT(name); > > I think it will be quite misleading to do this. eg a VM named 'foo' > > # qemu-system-x86_64 -vnc :2 -hda /var/lib/libvirt/images/plain.img -name foo,process=foo > > Now the process listing shows > > # ps -w > PID TTY TIME CMD > 12009 pts/1 00:00:01 bash > 12646 pts/1 00:00:00 ksmtuned > 14494 pts/1 00:00:02 foo > 14508 pts/1 00:00:00 sleep > 14511 pts/1 00:00:00 ps > > which leaves no indication that 'foo' is a QEMU process at all which is > rather bad IMHO. At the very least I think we should keep the binary base > name here, and have the VM name as a postfix, eg so it shows Unfortunately, qemu uses prctl() to set the process title, which has a limit of 16 characters. How about "qemu:$VM_NAME" for the process title (attached), so we waste as little as possible?

I'll look at doing something to increase the length limitation in qemu. john -- John Morrissey _o /\ ---- __o jwm(a)horde.net _-< \_ / \ ---- < \, www.horde.net/ __(_)/_(_)________/ \_______(_) /_(_)__

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 83c0f83..d3eba0c 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1165,8 +1165,11 @@ static unsigned long long qemudComputeCmdFlags(const char *help, flags |= QEMUD_CMD_FLAG_ENABLE_KVM; if (strstr(help, "-no-reboot")) flags |= QEMUD_CMD_FLAG_NO_REBOOT; - if (strstr(help, "-name")) + if (strstr(help, "-name")) { flags |= QEMUD_CMD_FLAG_NAME; + if (strstr(help, ",process=")) + flags |= QEMUD_CMD_FLAG_NAME_PROCESS; + } if (strstr(help, "-uuid")) flags |= QEMUD_CMD_FLAG_UUID; if (strstr(help, "-xen-domid")) @@ -4025,7 +4028,15 @@ int qemudBuildCommandLine(virConnectPtr conn, if (qemuCmdFlags & QEMUD_CMD_FLAG_NAME) { ADD_ARG_LIT("-name"); - ADD_ARG_LIT(def->name); + if (qemuCmdFlags & QEMUD_CMD_FLAG_NAME_PROCESS) { + char *name; + if (virAsprintf(&name, "%s,process=\"qemu:%s\"", + def->name, def->name) < 0) + goto no_memory; + ADD_ARG_LIT(name); + } else { + ADD_ARG_LIT(def->name); + } } if (qemuCmdFlags & QEMUD_CMD_FLAG_UUID) { ADD_ARG_LIT("-uuid"); @@ -6462,9 +6473,16 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, if (strstr(val, "menu=on")) def->os.bootmenu = 1; } else if (STREQ(arg, "-name")) { + char *process; WANT_VALUE(); - if (!(def->name = strdup(val))) - goto no_memory; + process = strstr(val, ",process="); + if (process == NULL) { + if (!(def->name = strdup(val))) + goto no_memory; + } else { + if (!(def->name = strndup(val, process - val))) + goto no_memory; + } } else if (STREQ(arg, "-M")) { WANT_VALUE(); if (!(def->os.machine = strdup(val))) diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index d2e6857..1e4483a 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -95,6 +95,7 @@ enum qemud_cmd_flags { QEMUD_CMD_FLAG_ENABLE_KQEMU = (1LL << 39), /* -enable-kqemu flag */ QEMUD_CMD_FLAG_FSDEV = (1LL << 40), /* -fstype filesystem passthrough */ QEMUD_CMD_FLAG_NESTING = (1LL << 41), /* -enable-nesting (SVM/VMX) */ + QEMUD_CMD_FLAG_NAME_PROCESS = (1LL << 42), /* Is -name process= available */ }; /* Main driver state */

On Mon, Oct 18, 2010 at 06:58:34PM +0100, Daniel P. Berrange wrote: > On Mon, Oct 18, 2010 at 01:45:12PM -0400, John Morrissey wrote: > > Unfortunately, qemu uses prctl() to set the process title, which has a > > limit of 16 characters. How about "qemu:$VM_NAME" for the process title > > (attached), so we waste as little as possible? > > Urgh, 16 characters is tiny :-( I think we had better add a configuration > parameter to /etc/qemu/qemu.conf to allow admins to turn this feature > on/off, with the default being off. At least until QEMU can support a > sensibly sized process name... [snip] > The patch looks fine, as long as you can add the qemu.conf config > param to turn it on/off Sure, attached. I tried to make some of the whitespace in the sample config more consistent; hope that doesn't cause headaches.

--- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -370,6 +370,10 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, CHECK_TYPE ("allow_disk_format_probing", VIR_CONF_LONG); if (p) driver->allowDiskFormatProbing = p->l; + p = virConfGetValue (conf, "set_process_name"); + CHECK_TYPE ("set_process_name", VIR_CONF_LONG); + if (p) driver->setProcessName = p->l; + virConfFree (conf); return 0; } @@ -1165,8 +1169,11 @@ static unsigned long long qemudComputeCmdFlags(const char *help, flags |= QEMUD_CMD_FLAG_ENABLE_KVM; if (strstr(help, "-no-reboot")) flags |= QEMUD_CMD_FLAG_NO_REBOOT; - if (strstr(help, "-name")) + if (strstr(help, "-name")) { flags |= QEMUD_CMD_FLAG_NAME; + if (strstr(help, ",process=")) + flags |= QEMUD_CMD_FLAG_NAME_PROCESS; + } if (strstr(help, "-uuid")) flags |= QEMUD_CMD_FLAG_UUID; if (strstr(help, "-xen-domid"))

diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index d2e6857..005031d 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -95,6 +95,7 @@ enum qemud_cmd_flags { QEMUD_CMD_FLAG_ENABLE_KQEMU = (1LL << 39), /* -enable-kqemu flag */ QEMUD_CMD_FLAG_FSDEV = (1LL << 40), /* -fstype filesystem passthrough */ QEMUD_CMD_FLAG_NESTING = (1LL << 41), /* -enable-nesting (SVM/VMX) */ + QEMUD_CMD_FLAG_NAME_PROCESS = (1LL << 42), /* Is -name process= available */ };

On Wed, Oct 20, 2010 at 10:34:00AM +0200, Daniel Veillard wrote: > Looks just fine, applied and pushed ! Was just working on some changes for qemu; looking at this fresh made me notice I'd overlooked the unnecessary quoting of the command line. AFAICT the quoting isn't necessary. john diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 9e6e162..85e5998 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -4224,7 +4224,7 @@ int qemudBuildCommandLine(virConnectPtr conn, if (driver->setProcessName && (qemuCmdFlags & QEMUD_CMD_FLAG_NAME_PROCESS)) { char *name; - if (virAsprintf(&name, "%s,process=\"qemu:%s\"", + if (virAsprintf(&name, "%s,process=qemu:%s", def->name, def->name) < 0) goto no_memory; ADD_ARG_LIT(name);

2010/11/10 Daniel P. Berrange <berrange(a)redhat.com&gt;: > On Tue, Nov 09, 2010 at 01:41:43PM -0500, John Morrissey wrote: >> On Wed, Oct 20, 2010 at 10:34:00AM +0200, Daniel Veillard wrote: >> >   Looks just fine, applied and pushed ! >> >> Was just working on some changes for qemu; looking at this fresh made me >> notice I'd overlooked the unnecessary quoting of the command line. AFAICT >> the quoting isn't necessary. >> >> john >> >> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c >> index 9e6e162..85e5998 100644 >> --- a/src/qemu/qemu_conf.c >> +++ b/src/qemu/qemu_conf.c >> @@ -4224,7 +4224,7 @@ int qemudBuildCommandLine(virConnectPtr conn, >>          if (driver->setProcessName && >>              (qemuCmdFlags & QEMUD_CMD_FLAG_NAME_PROCESS)) { >>              char *name; >> -            if (virAsprintf(&name, "%s,process=\"qemu:%s\"", >> +            if (virAsprintf(&name, "%s,process=qemu:%s", >>                              def->name, def->name) < 0) >>                  goto no_memory; >>              ADD_ARG_LIT(name); > > ACK > > Daniel > Seems like this patch has not been pushed yet. Therefore, I've applied and pushed it now.