[libvirt] process= support for 'qemu-kvm -name' [Bug 576950]
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. If someone could give me some high-level guidance, I'd be happy to keep going on this. john -- John Morrissey _o /\ ---- __o jwm@horde.net _-< \_ / \ ---- < \, www.horde.net/ __(_)/_(_)________/ \_______(_) /_(_)__
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.
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.
Yes it would be sufficient to just drop any ',process=' bit.
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);
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 # 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 qemu-system-x86_64 (foo) 14508 pts/1 00:00:00 sleep 14511 pts/1 00:00:00 ps Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
[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@horde.net _-< \_ / \ ---- < \, www.horde.net/ __(_)/_(_)________/ \_______(_) /_(_)__
On Mon, Oct 18, 2010 at 01:45:12PM -0400, John Morrissey wrote:
[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?
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...
I'll look at doing something to increase the length limitation in qemu.
john -- John Morrissey _o /\ ---- __o jwm@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 */
The patch looks fine, as long as you can add the qemu.conf config param to turn it on/off Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
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. john -- John Morrissey _o /\ ---- __o jwm@horde.net _-< \_ / \ ---- < \, www.horde.net/ __(_)/_(_)________/ \_______(_) /_(_)__
On Mon, Oct 18, 2010 at 06:17:14PM -0400, John Morrissey wrote:
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 */ };
Patch looks fine but I didn't applied it yet because "make check" raises an error now: TEST: qemuhelptest .....Computed flags do not match: got 0x64f0a8ffd7e, expected 0x24f0a8ffd7e !Computed flags do not match: got 0x4425ef9dc6e, expected 0x425ef9dc6e !Computed flags do not match: got 0x64ede6ffd7e, expected 0x24ede6ffd7e ! 8 FAIL FAIL: qemuhelptest I think it's simply that the new flag must be added to some of the expected result in tests/qemuhelptest.c, should not be hard to find out and clear, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Tue, Oct 19, 2010 at 05:45:42PM +0200, Daniel Veillard wrote:
Patch looks fine but I didn't applied it yet because "make check" raises an error now:
D'oh, I nearly beat you to noticing. :-) I was just building some packages with this patch, and noticed the test suite failure since the packages run it as part of the build process. Updated patch attached, sorry for the bother. john -- John Morrissey _o /\ ---- __o jwm@horde.net _-< \_ / \ ---- < \, www.horde.net/ __(_)/_(_)________/ \_______(_) /_(_)__
On Tue, Oct 19, 2010 at 12:04:27PM -0400, John Morrissey wrote:
On Tue, Oct 19, 2010 at 05:45:42PM +0200, Daniel Veillard wrote:
Patch looks fine but I didn't applied it yet because "make check" raises an error now:
D'oh, I nearly beat you to noticing. :-) I was just building some packages with this patch, and noticed the test suite failure since the packages run it as part of the build process.
Updated patch attached, sorry for the bother.
Looks just fine, applied and pushed ! thanks for the quick fix :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
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); -- John Morrissey _o /\ ---- __o jwm@horde.net _-< \_ / \ ---- < \, www.horde.net/ __(_)/_(_)________/ \_______(_) /_(_)__
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 -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
2010/11/10 Daniel P. Berrange <berrange@redhat.com>:
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. Matthias
On Fri, Nov 12, 2010 at 03:16:09PM +0100, Matthias Bolte wrote:
2010/11/10 Daniel P. Berrange <berrange@redhat.com>:
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.
Thanks I had applied it to my tree last week but forgot to push it :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
John Morrissey -
Matthias Bolte