[libvirt] [PATCH 0/8 v3] Enable libvirt to attach to existing QEMU instances

An update to http://www.redhat.com/archives/libvir-list/2011-June/msg00888.html New in v3: - Nothing. Rebase to current GIT only.

Introduce a new API in libvirt-qemu.so virDomainPtr virDomainQemuAttach(virConnectPtr domain, unsigned long long pid, unsigned int flags); This allows libvirtd to attach to an existing, externally launched QEMU process. This is useful for QEMU developers who prefer to launch QEMU themselves for debugging/devel reasons, but still want the benefit of libvirt based tools like virt-top, virt-viewer, etc * include/libvirt/libvirt-qemu.h: Define virDomainQemuAttach * src/driver.h, src/libvirt-qemu.c, src/libvirt_qemu.syms: Driver glue for virDomainQemuAttach --- include/libvirt/libvirt-qemu.h | 4 +++ src/driver.h | 6 +++++ src/libvirt-qemu.c | 41 ++++++++++++++++++++++++++++++++++++++++ src/libvirt_qemu.syms | 5 ++++ 4 files changed, 56 insertions(+), 0 deletions(-) diff --git a/include/libvirt/libvirt-qemu.h b/include/libvirt/libvirt-qemu.h index f172eff..2f0f1c6 100644 --- a/include/libvirt/libvirt-qemu.h +++ b/include/libvirt/libvirt-qemu.h @@ -28,6 +28,10 @@ typedef enum { int virDomainQemuMonitorCommand(virDomainPtr domain, const char *cmd, char **result, unsigned int flags); +virDomainPtr virDomainQemuAttach(virConnectPtr domain, + unsigned long long pid, + unsigned int flags); + # ifdef __cplusplus } # endif diff --git a/src/driver.h b/src/driver.h index 871a4ae..fd801f4 100644 --- a/src/driver.h +++ b/src/driver.h @@ -566,6 +566,11 @@ typedef int (*virDrvDomainQemuMonitorCommand)(virDomainPtr domain, const char *cmd, char **result, unsigned int flags); +typedef virDomainPtr + (*virDrvDomainQemuAttach)(virConnectPtr conn, + unsigned long long pid, + unsigned int flags); + typedef int (*virDrvDomainOpenConsole)(virDomainPtr dom, const char *devname, @@ -786,6 +791,7 @@ struct _virDriver { virDrvDomainRevertToSnapshot domainRevertToSnapshot; virDrvDomainSnapshotDelete domainSnapshotDelete; virDrvDomainQemuMonitorCommand qemuDomainMonitorCommand; + virDrvDomainQemuAttach qemuDomainAttach; virDrvDomainOpenConsole domainOpenConsole; virDrvDomainInjectNMI domainInjectNMI; virDrvDomainMigrateBegin3 domainMigrateBegin3; diff --git a/src/libvirt-qemu.c b/src/libvirt-qemu.c index 46727c8..f0bcac1 100644 --- a/src/libvirt-qemu.c +++ b/src/libvirt-qemu.c @@ -79,3 +79,44 @@ error: virDispatchError(conn); return -1; } + + +virDomainPtr +virDomainQemuAttach(virConnectPtr conn, + unsigned long long pid, + unsigned int flags) +{ + VIR_DEBUG("conn=%p, pid=%llu, flags=%u", conn, pid, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__); + virDispatchError(NULL); + return NULL; + } + + if (pid <= 1) { + virLibDomainError(domain, VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } + + if (conn->flags & VIR_CONNECT_RO) { + virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + + if (conn->driver->qemuDomainAttach) { + virDomainPtr ret; + ret = conn->driver->qemuDomainAttach(conn, pid, flags); + if (!ret) + goto error; + return ret; + } + + virLibConnError(conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(conn); + return NULL; +} diff --git a/src/libvirt_qemu.syms b/src/libvirt_qemu.syms index 5702d36..1bb8b62 100644 --- a/src/libvirt_qemu.syms +++ b/src/libvirt_qemu.syms @@ -14,3 +14,8 @@ LIBVIRT_QEMU_0.8.3 { global: virDomainQemuMonitorCommand; }; + +LIBVIRT_QEMU_0.9.3 { + global: + virDomainQemuAttach; +} LIBVIRT_QEMU_0.8.3; -- 1.7.4.4

2011/7/4 Daniel P. Berrange <berrange@redhat.com>:
Introduce a new API in libvirt-qemu.so
virDomainPtr virDomainQemuAttach(virConnectPtr domain, unsigned long long pid, unsigned int flags);
This allows libvirtd to attach to an existing, externally launched QEMU process. This is useful for QEMU developers who prefer to launch QEMU themselves for debugging/devel reasons, but still want the benefit of libvirt based tools like virt-top, virt-viewer, etc
* include/libvirt/libvirt-qemu.h: Define virDomainQemuAttach * src/driver.h, src/libvirt-qemu.c, src/libvirt_qemu.syms: Driver glue for virDomainQemuAttach --- include/libvirt/libvirt-qemu.h | 4 +++ src/driver.h | 6 +++++ src/libvirt-qemu.c | 41 ++++++++++++++++++++++++++++++++++++++++ src/libvirt_qemu.syms | 5 ++++ 4 files changed, 56 insertions(+), 0 deletions(-)
+virDomainPtr +virDomainQemuAttach(virConnectPtr conn, + unsigned long long pid, + unsigned int flags) +{ + VIR_DEBUG("conn=%p, pid=%llu, flags=%u", conn, pid, flags);
Shouldn't this function have documentation? Hm, virDomainQemuMonitorCommand isn't documented either.
diff --git a/src/libvirt_qemu.syms b/src/libvirt_qemu.syms index 5702d36..1bb8b62 100644 --- a/src/libvirt_qemu.syms +++ b/src/libvirt_qemu.syms @@ -14,3 +14,8 @@ LIBVIRT_QEMU_0.8.3 { global: virDomainQemuMonitorCommand; }; + +LIBVIRT_QEMU_0.9.3 { + global: + virDomainQemuAttach; +} LIBVIRT_QEMU_0.8.3;
0.9.3 was released in the meantime, needs to be 0.9.4 now. ACK. -- Matthias Bolte http://photron.blogspot.com

On Tue, Jul 05, 2011 at 12:54:32PM +0200, Matthias Bolte wrote:
2011/7/4 Daniel P. Berrange <berrange@redhat.com>:
Introduce a new API in libvirt-qemu.so
virDomainPtr virDomainQemuAttach(virConnectPtr domain, unsigned long long pid, unsigned int flags);
This allows libvirtd to attach to an existing, externally launched QEMU process. This is useful for QEMU developers who prefer to launch QEMU themselves for debugging/devel reasons, but still want the benefit of libvirt based tools like virt-top, virt-viewer, etc
* include/libvirt/libvirt-qemu.h: Define virDomainQemuAttach * src/driver.h, src/libvirt-qemu.c, src/libvirt_qemu.syms: Driver glue for virDomainQemuAttach --- include/libvirt/libvirt-qemu.h | 4 +++ src/driver.h | 6 +++++ src/libvirt-qemu.c | 41 ++++++++++++++++++++++++++++++++++++++++ src/libvirt_qemu.syms | 5 ++++ 4 files changed, 56 insertions(+), 0 deletions(-)
+virDomainPtr +virDomainQemuAttach(virConnectPtr conn, + unsigned long long pid, + unsigned int flags) +{ + VIR_DEBUG("conn=%p, pid=%llu, flags=%u", conn, pid, flags);
Shouldn't this function have documentation? Hm, virDomainQemuMonitorCommand isn't documented either.
diff --git a/src/libvirt_qemu.syms b/src/libvirt_qemu.syms index 5702d36..1bb8b62 100644 --- a/src/libvirt_qemu.syms +++ b/src/libvirt_qemu.syms @@ -14,3 +14,8 @@ LIBVIRT_QEMU_0.8.3 { global: virDomainQemuMonitorCommand; }; + +LIBVIRT_QEMU_0.9.3 { + global: + virDomainQemuAttach; +} LIBVIRT_QEMU_0.8.3;
0.9.3 was released in the meantime, needs to be 0.9.4 now.
Fixed this & added some docs. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 07/04/2011 04:28 AM, Daniel P. Berrange wrote:
Introduce a new API in libvirt-qemu.so
virDomainPtr virDomainQemuAttach(virConnectPtr domain, unsigned long long pid,
We already assert elsewhere in our code base that pid_t will always fit in int. For example, see _virDomainObj in domain_conf.h. Passing a ull here seems like it might be overkill. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Fri, Jul 08, 2011 at 02:52:36PM -0600, Eric Blake wrote:
On 07/04/2011 04:28 AM, Daniel P. Berrange wrote:
Introduce a new API in libvirt-qemu.so
virDomainPtr virDomainQemuAttach(virConnectPtr domain, unsigned long long pid,
We already assert elsewhere in our code base that pid_t will always fit in int. For example, see _virDomainObj in domain_conf.h. Passing a ull here seems like it might be overkill.
Last time I posted this series there was some question whether we'd be ok with just 'unsigned int pid', so I changed to long long. I can change it back again trivially though if we think unsigned int will in face be safe. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

* daemon/remote.c: Server side dispatcher * src/remote/remote_driver.c: Client side dispatcher * src/remote/qemu_protocol.x: Wire protocol definition --- daemon/remote.c | 41 +++++++++++++++++++++++++++++++++++++++++ src/remote/qemu_protocol.x | 13 ++++++++++++- src/remote/remote_driver.c | 30 ++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 1 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 2889908..85fc978 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -2645,6 +2645,47 @@ cleanup: static int +qemuDispatchDomainAttach(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessageHeaderPtr hdr ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + qemu_domain_attach_args *args, + qemu_domain_attach_ret *ret) +{ + virDomainPtr dom = NULL; + int rv = -1; + struct daemonClientPrivate *priv = + virNetServerClientGetPrivateData(client); + + if (!priv->conn) { + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + if (!(dom = virDomainQemuAttach(priv->conn, + args->pid, + args->flags))) + goto cleanup; + + make_nonnull_domain(&ret->dom, dom); + + rv = 0; + +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (dom) + virDomainFree(dom); + return rv; +} + + +#include "remote_dispatch_bodies.h" +#include "qemu_dispatch_bodies.h" + + +>>>>>>> Define remote wire protocol & impls for virDomainQemuAttach +static int remoteDispatchDomainMigrateBegin3(virNetServerPtr server ATTRIBUTE_UNUSED, virNetServerClientPtr client ATTRIBUTE_UNUSED, virNetMessageHeaderPtr hdr ATTRIBUTE_UNUSED, diff --git a/src/remote/qemu_protocol.x b/src/remote/qemu_protocol.x index fa453f4..4d510ff 100644 --- a/src/remote/qemu_protocol.x +++ b/src/remote/qemu_protocol.x @@ -37,6 +37,16 @@ struct qemu_monitor_command_ret { remote_nonnull_string result; }; + +struct qemu_domain_attach_args { + unsigned hyper pid; + unsigned int flags; +}; + +struct qemu_domain_attach_ret { + remote_nonnull_domain dom; +}; + /* Define the program number, protocol version and procedure numbers here. */ const QEMU_PROGRAM = 0x20008087; const QEMU_PROTOCOL_VERSION = 1; @@ -45,5 +55,6 @@ enum qemu_procedure { /* Each function must have a two-word comment. The first word is * whether remote_generator.pl handles daemon, the second whether * it handles src/remote. */ - QEMU_PROC_MONITOR_COMMAND = 1 /* skipgen skipgen */ + QEMU_PROC_MONITOR_COMMAND = 1, /* skipgen skipgen */ + QEMU_PROC_DOMAIN_ATTACH = 2 /* skipgen skipgen */ }; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index f318740..c4842a2 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -3547,6 +3547,35 @@ done: } +static virDomainPtr +remoteQemuDomainAttach(virConnectPtr conn, unsigned long long pid, unsigned int flags) +{ + virDomainPtr rv = NULL; + struct private_data *priv = conn->privateData; + qemu_domain_attach_args args; + qemu_domain_attach_ret ret; + + remoteDriverLock(priv); + + args.pid = pid; + args.flags = flags; + + memset(&ret, 0, sizeof ret); + + if (call(conn, priv, REMOTE_CALL_QEMU, QEMU_PROC_DOMAIN_ATTACH, + (xdrproc_t)xdr_qemu_domain_attach_args, (char *)&args, + (xdrproc_t)xdr_qemu_domain_attach_ret, (char *)&ret) == -1) + goto done; + + rv = get_nonnull_domain(conn, ret.dom); + xdr_free((xdrproc_t)xdr_qemu_domain_attach_ret, (char *)&ret); + +done: + remoteDriverUnlock(priv); + return rv; +} + + static char * remoteDomainMigrateBegin3(virDomainPtr domain, const char *xmlin, @@ -4222,6 +4251,7 @@ static virDriver remote_driver = { .domainRevertToSnapshot = remoteDomainRevertToSnapshot, /* 0.8.0 */ .domainSnapshotDelete = remoteDomainSnapshotDelete, /* 0.8.0 */ .qemuDomainMonitorCommand = remoteQemuDomainMonitorCommand, /* 0.8.3 */ + .qemuDomainAttach = remoteQemuDomainAttach, /* 0.9.3 */ .domainOpenConsole = remoteDomainOpenConsole, /* 0.8.6 */ .domainInjectNMI = remoteDomainInjectNMI, /* 0.9.2 */ .domainMigrateBegin3 = remoteDomainMigrateBegin3, /* 0.9.2 */ -- 1.7.4.4

2011/7/4 Daniel P. Berrange <berrange@redhat.com>:
* daemon/remote.c: Server side dispatcher * src/remote/remote_driver.c: Client side dispatcher * src/remote/qemu_protocol.x: Wire protocol definition --- daemon/remote.c | 41 +++++++++++++++++++++++++++++++++++++++++ src/remote/qemu_protocol.x | 13 ++++++++++++- src/remote/remote_driver.c | 30 ++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 1 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c index 2889908..85fc978 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -2645,6 +2645,47 @@ cleanup:
static int +qemuDispatchDomainAttach(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessageHeaderPtr hdr ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + qemu_domain_attach_args *args, + qemu_domain_attach_ret *ret) +{ + virDomainPtr dom = NULL; + int rv = -1; + struct daemonClientPrivate *priv = + virNetServerClientGetPrivateData(client); + + if (!priv->conn) { + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + if (!(dom = virDomainQemuAttach(priv->conn, + args->pid, + args->flags))) + goto cleanup; + + make_nonnull_domain(&ret->dom, dom); + + rv = 0; + +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (dom) + virDomainFree(dom); + return rv; +}
The generator should be able to deal with this, did you try?
+ +#include "remote_dispatch_bodies.h" +#include "qemu_dispatch_bodies.h" + + +>>>>>>> Define remote wire protocol & impls for virDomainQemuAttach
The includes and this line looks bogus, probably a rebase problem.
+static virDomainPtr +remoteQemuDomainAttach(virConnectPtr conn, unsigned long long pid, unsigned int flags) +{ + virDomainPtr rv = NULL; + struct private_data *priv = conn->privateData; + qemu_domain_attach_args args; + qemu_domain_attach_ret ret; + + remoteDriverLock(priv); + + args.pid = pid; + args.flags = flags; + + memset(&ret, 0, sizeof ret); + + if (call(conn, priv, REMOTE_CALL_QEMU, QEMU_PROC_DOMAIN_ATTACH, + (xdrproc_t)xdr_qemu_domain_attach_args, (char *)&args, + (xdrproc_t)xdr_qemu_domain_attach_ret, (char *)&ret) == -1) + goto done; + + rv = get_nonnull_domain(conn, ret.dom); + xdr_free((xdrproc_t)xdr_qemu_domain_attach_ret, (char *)&ret); + +done: + remoteDriverUnlock(priv); + return rv; +}
The generator should also be able to deal with this one.
static char * remoteDomainMigrateBegin3(virDomainPtr domain, const char *xmlin, @@ -4222,6 +4251,7 @@ static virDriver remote_driver = { .domainRevertToSnapshot = remoteDomainRevertToSnapshot, /* 0.8.0 */ .domainSnapshotDelete = remoteDomainSnapshotDelete, /* 0.8.0 */ .qemuDomainMonitorCommand = remoteQemuDomainMonitorCommand, /* 0.8.3 */ + .qemuDomainAttach = remoteQemuDomainAttach, /* 0.9.3 */
s/0.9.3/0.9.4/ -- Matthias Bolte http://photron.blogspot.com

From: "Daniel P. Berrange" <berrange@redhat.com> This tweaks the RPC generator to cope with some naming conventions used for the QEMU specific APIs * daemon/remote.c: Server side dispatcher * src/remote/remote_driver.c: Client side dispatcher * src/remote/qemu_protocol.x: Wire protocol definition * src/rpc/gendispatch.pl: Use '$structprefix' in method names, fix QEMU flags and fix dispatcher method names --- src/remote/qemu_protocol.x | 13 ++++++++++++- src/remote/remote_driver.c | 1 + src/rpc/gendispatch.pl | 11 +++++++++-- 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/src/remote/qemu_protocol.x b/src/remote/qemu_protocol.x index fa453f4..c6a5648 100644 --- a/src/remote/qemu_protocol.x +++ b/src/remote/qemu_protocol.x @@ -37,6 +37,16 @@ struct qemu_monitor_command_ret { remote_nonnull_string result; }; + +struct qemu_domain_attach_args { + unsigned hyper pid; + unsigned int flags; +}; + +struct qemu_domain_attach_ret { + remote_nonnull_domain dom; +}; + /* Define the program number, protocol version and procedure numbers here. */ const QEMU_PROGRAM = 0x20008087; const QEMU_PROTOCOL_VERSION = 1; @@ -45,5 +55,6 @@ enum qemu_procedure { /* Each function must have a two-word comment. The first word is * whether remote_generator.pl handles daemon, the second whether * it handles src/remote. */ - QEMU_PROC_MONITOR_COMMAND = 1 /* skipgen skipgen */ + QEMU_PROC_MONITOR_COMMAND = 1, /* skipgen skipgen */ + QEMU_PROC_DOMAIN_ATTACH = 2 /* autogen autogen */ }; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index f318740..4eea3a4 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -4222,6 +4222,7 @@ static virDriver remote_driver = { .domainRevertToSnapshot = remoteDomainRevertToSnapshot, /* 0.8.0 */ .domainSnapshotDelete = remoteDomainSnapshotDelete, /* 0.8.0 */ .qemuDomainMonitorCommand = remoteQemuDomainMonitorCommand, /* 0.8.3 */ + .qemuDomainAttach = qemuDomainAttach, /* 0.9.4 */ .domainOpenConsole = remoteDomainOpenConsole, /* 0.8.6 */ .domainInjectNMI = remoteDomainInjectNMI, /* 0.9.2 */ .domainMigrateBegin3 = remoteDomainMigrateBegin3, /* 0.9.2 */ diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index 027560c..56841c8 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -821,6 +821,8 @@ elsif ($opt_b) { $proc_name = "ConnectBaselineCPU" } elsif ($call->{ProcName} eq "CPUCompare") { $proc_name = "ConnectCompareCPU" + } elsif ($structprefix eq "qemu" && $call->{ProcName} =~ /^Domain/) { + $proc_name =~ s/^(Domain)/${1}Qemu/; } if ($single_ret_as_list) { @@ -1323,7 +1325,7 @@ elsif ($opt_k) { # print function print "\n"; print "static $single_ret_type\n"; - print "remote$call->{ProcName}("; + print "$structprefix$call->{ProcName}("; print join(", ", @args_list); @@ -1417,8 +1419,13 @@ elsif ($opt_k) { print " memset(&ret, 0, sizeof ret);\n"; } + my $callflags = "0"; + if ($structprefix eq "qemu") { + $callflags = "REMOTE_CALL_QEMU"; + } + print "\n"; - print " if (call($priv_src, priv, 0, ${procprefix}_PROC_$call->{UC_NAME},\n"; + print " if (call($priv_src, priv, $callflags, ${procprefix}_PROC_$call->{UC_NAME},\n"; print " (xdrproc_t)xdr_$argtype, (char *)$call_args,\n"; print " (xdrproc_t)xdr_$rettype, (char *)$call_ret) == -1) {\n"; -- 1.7.6

2011/7/8 Daniel P. Berrange <berrange@redhat.com>:
From: "Daniel P. Berrange" <berrange@redhat.com>
This tweaks the RPC generator to cope with some naming conventions used for the QEMU specific APIs
* daemon/remote.c: Server side dispatcher * src/remote/remote_driver.c: Client side dispatcher * src/remote/qemu_protocol.x: Wire protocol definition * src/rpc/gendispatch.pl: Use '$structprefix' in method names, fix QEMU flags and fix dispatcher method names --- src/remote/qemu_protocol.x | 13 ++++++++++++- src/remote/remote_driver.c | 1 + src/rpc/gendispatch.pl | 11 +++++++++-- 3 files changed, 22 insertions(+), 3 deletions(-)
ACK. -- Matthias Bolte http://photron.blogspot.com

On 07/08/2011 06:20 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
This tweaks the RPC generator to cope with some naming conventions used for the QEMU specific APIs
* daemon/remote.c: Server side dispatcher * src/remote/remote_driver.c: Client side dispatcher * src/remote/qemu_protocol.x: Wire protocol definition * src/rpc/gendispatch.pl: Use '$structprefix' in method names, fix QEMU flags and fix dispatcher method names --- src/remote/qemu_protocol.x | 13 ++++++++++++- src/remote/remote_driver.c | 1 + src/rpc/gendispatch.pl | 11 +++++++++-- 3 files changed, 22 insertions(+), 3 deletions(-)
Hmm. We have src/remote_protocol-structs to catch any incompatible on-the-wire changes to remote_protocol.x, but nothing to track changes to qemu_protocol.x. Is this something that needs fixing to help us avoid unintended wire breakage? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 07/08/2011 07:40 AM, Eric Blake wrote:
On 07/08/2011 06:20 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
This tweaks the RPC generator to cope with some naming conventions used for the QEMU specific APIs
* daemon/remote.c: Server side dispatcher * src/remote/remote_driver.c: Client side dispatcher * src/remote/qemu_protocol.x: Wire protocol definition * src/rpc/gendispatch.pl: Use '$structprefix' in method names, fix QEMU flags and fix dispatcher method names --- src/remote/qemu_protocol.x | 13 ++++++++++++- src/remote/remote_driver.c | 1 + src/rpc/gendispatch.pl | 11 +++++++++-- 3 files changed, 22 insertions(+), 3 deletions(-)
Hmm. We have src/remote_protocol-structs to catch any incompatible on-the-wire changes to remote_protocol.x, but nothing to track changes to qemu_protocol.x. Is this something that needs fixing to help us avoid unintended wire breakage?
Answering my own question: https://www.redhat.com/archives/libvir-list/2011-July/msg00463.html We'll have a merge commit for whichever of these two patches goes in second :) -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Tue, Jul 05, 2011 at 01:07:45PM +0200, Matthias Bolte wrote:
2011/7/4 Daniel P. Berrange <berrange@redhat.com>:
* daemon/remote.c: Server side dispatcher * src/remote/remote_driver.c: Client side dispatcher * src/remote/qemu_protocol.x: Wire protocol definition --- daemon/remote.c | 41 +++++++++++++++++++++++++++++++++++++++++ src/remote/qemu_protocol.x | 13 ++++++++++++- src/remote/remote_driver.c | 30 ++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 1 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c index 2889908..85fc978 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -2645,6 +2645,47 @@ cleanup:
static int +qemuDispatchDomainAttach(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessageHeaderPtr hdr ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + qemu_domain_attach_args *args, + qemu_domain_attach_ret *ret) +{ + virDomainPtr dom = NULL; + int rv = -1; + struct daemonClientPrivate *priv = + virNetServerClientGetPrivateData(client); + + if (!priv->conn) { + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + if (!(dom = virDomainQemuAttach(priv->conn, + args->pid, + args->flags))) + goto cleanup; + + make_nonnull_domain(&ret->dom, dom); + + rv = 0; + +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (dom) + virDomainFree(dom); + return rv; +}
The generator should be able to deal with this, did you try?
I couldn't, but it turns out the changes were fairly easy.
+#include "remote_dispatch_bodies.h" +#include "qemu_dispatch_bodies.h" + + +>>>>>>> Define remote wire protocol & impls for virDomainQemuAttach
The includes and this line looks bogus, probably a rebase problem.
Yep, totally bogus line.
+static virDomainPtr +remoteQemuDomainAttach(virConnectPtr conn, unsigned long long pid, unsigned int flags) +{ + virDomainPtr rv = NULL; + struct private_data *priv = conn->privateData; + qemu_domain_attach_args args; + qemu_domain_attach_ret ret; + + remoteDriverLock(priv); + + args.pid = pid; + args.flags = flags; + + memset(&ret, 0, sizeof ret); + + if (call(conn, priv, REMOTE_CALL_QEMU, QEMU_PROC_DOMAIN_ATTACH, + (xdrproc_t)xdr_qemu_domain_attach_args, (char *)&args, + (xdrproc_t)xdr_qemu_domain_attach_ret, (char *)&ret) == -1) + goto done; + + rv = get_nonnull_domain(conn, ret.dom); + xdr_free((xdrproc_t)xdr_qemu_domain_attach_ret, (char *)&ret); + +done: + remoteDriverUnlock(priv); + return rv; +}
The generator should also be able to deal with this one.
It did after some changes I'm reposting this one patch since its the only one that needed non-trivial fixups Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

This command allows libvirt to attach to an existing QEMU instance. $ qemu-kvm -cdrom ~/demo.iso \ -monitor unix:/tmp/demo,server,nowait \ -name foo \ -uuid cece4f9f-dff0-575d-0e8e-01fe380f12ea & $ QEMUPID=$! $ virsh qemu-attach $QEMUPID --- tools/virsh.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 26 +++++++++++++++++++++++++- 2 files changed, 72 insertions(+), 1 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 9a189fd..f7a1274 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -11611,6 +11611,52 @@ cleanup: return ret; } +/* + * "qemu-attach" command + */ +static const vshCmdInfo info_qemu_attach[] = { + {"help", N_("Qemu Attach")}, + {"desc", N_("Qemu Attach")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_qemu_attach[] = { + {"pid", VSH_OT_DATA, VSH_OFLAG_REQ, N_("pid")}, + {NULL, 0, 0, NULL} +}; + +static bool +cmdQemuAttach(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom = NULL; + bool ret = false; + unsigned int flags = 0; + unsigned long long pid; + + if (!vshConnectionUsability(ctl, ctl->conn)) + goto cleanup; + + if (vshCommandOptULongLong(cmd, "pid", &pid) <= 0) { + vshError(ctl, "%s", _("missing pid value")); + goto cleanup; + } + + if (!(dom = virDomainQemuAttach(ctl->conn, pid, flags))) + goto cleanup; + + if (dom != NULL) { + vshPrint(ctl, _("Domain %s attached to pid %llu\n"), + virDomainGetName(dom), pid); + virDomainFree(dom); + ret = true; + } else { + vshError(ctl, _("Failed to attach to pid %llu"), pid); + } + +cleanup: + return ret; +} + static const vshCmdDef domManagementCmds[] = { {"attach-device", cmdAttachDevice, opts_attach_device, info_attach_device, 0}, @@ -11874,6 +11920,7 @@ static const vshCmdDef hostAndHypervisorCmds[] = { {"nodecpustats", cmdNodeCpuStats, opts_node_cpustats, info_nodecpustats, 0}, {"nodeinfo", cmdNodeinfo, NULL, info_nodeinfo, 0}, {"nodememstats", cmdNodeMemStats, opts_node_memstats, info_nodememstats, 0}, + {"qemu-attach", cmdQemuAttach, opts_qemu_attach, info_qemu_attach}, {"qemu-monitor-command", cmdQemuMonitorCommand, opts_qemu_monitor_command, info_qemu_monitor_command, 0}, {"sysinfo", cmdSysinfo, NULL, info_sysinfo, 0}, diff --git a/tools/virsh.pod b/tools/virsh.pod index 736b919..56251aa 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -131,7 +131,8 @@ group as an option. For example: connect (re)connect to hypervisor freecell NUMA free memory hostname print the hypervisor hostname - qemu-monitor-command Qemu Monitor Command + qemu-attach Attach to existing QEMU process + qemu-monitor-command QEMU Monitor Command sysinfo print the hypervisor sysinfo uri print the hypervisor canonical URI @@ -1517,6 +1518,29 @@ problems to the libvirt developers; the reports will be ignored. =over 4 +=item B<qemu-attach> I<pid> + +Attach an externally launched QEMU process to the libvirt QEMU driver. +The QEMU process must have been created with a monitor connection +using the UNIX driver. Ideally the process will also have had the +'-name' argument specified. + +=over 4 + + $ qemu-kvm -cdrom ~/demo.iso \ + -monitor unix:/tmp/demo,server,nowait \ + -name foo \ + -uuid cece4f9f-dff0-575d-0e8e-01fe380f12ea & + $ QEMUPID=$! + $ virsh qemu-attach $QEMUPID + +=back + +Not all functions of libvirt are expected to work reliably after +attaching to an externally launched QEMU process. There may be +issues with the guest ABI changing upon migration, and hotunplug +may not work. + =item B<qemu-monitor-command> I<domain> I<command> optional I<--hmp> Send an arbitrary monitor command I<command> to domain I<domain> through the -- 1.7.4.4

2011/7/4 Daniel P. Berrange <berrange@redhat.com>:
This command allows libvirt to attach to an existing QEMU instance.
$ qemu-kvm -cdrom ~/demo.iso \ -monitor unix:/tmp/demo,server,nowait \ -name foo \ -uuid cece4f9f-dff0-575d-0e8e-01fe380f12ea & $ QEMUPID=$! $ virsh qemu-attach $QEMUPID --- tools/virsh.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 26 +++++++++++++++++++++++++- 2 files changed, 72 insertions(+), 1 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 9a189fd..f7a1274 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -11611,6 +11611,52 @@ cleanup: return ret; }
+/* + * "qemu-attach" command + */ +static const vshCmdInfo info_qemu_attach[] = { + {"help", N_("Qemu Attach")}, + {"desc", N_("Qemu Attach")},
Below you replaced Qemu with QEMU, but here you use Qemu. ACK. -- Matthias Bolte http://photron.blogspot.com

Avoid re-formatting the pidfile path everytime we need it. Create it once when starting the guest, and preserve it until the guest is shutdown. * src/libvirt_private.syms, src/util/util.c, src/util/util.h: Add virFileReadPidPath * src/qemu/qemu_domain.h: Add pidfile field * src/qemu/qemu_process.c: Store pidfile path in qemuDomainObjPrivate --- src/libvirt_private.syms | 1 + src/qemu/qemu_domain.h | 1 + src/qemu/qemu_process.c | 27 +++++++++++++++------------ src/util/util.c | 45 ++++++++++++++++++++++++++++++--------------- src/util/util.h | 2 ++ 5 files changed, 49 insertions(+), 27 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 626ac6c..a7fc179 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1000,6 +1000,7 @@ virFilePid; virFileReadAll; virFileReadLimFD; virFileReadPid; +virFileReadPidPath; virFileResolveLink; virFileSanitizePath; virFileStripSuffix; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index f282df2..b617b9e 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -82,6 +82,7 @@ struct _qemuDomainObjPrivate { bool monError; unsigned long long monStart; bool gotShutdown; + char *pidfile; int nvcpupids; int *vcpupids; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f2c439b..bbbc36f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -76,6 +76,7 @@ qemuProcessRemoveDomainStatus(struct qemud_driver *driver, { char ebuf[1024]; char *file = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; if (virAsprintf(&file, "%s/%s.xml", driver->stateDir, vm->def->name) < 0) { virReportOOMError(); @@ -87,11 +88,12 @@ qemuProcessRemoveDomainStatus(struct qemud_driver *driver, vm->def->name, virStrerror(errno, ebuf, sizeof(ebuf))); VIR_FREE(file); - if (virFileDeletePid(driver->stateDir, vm->def->name) != 0) + if (priv->pidfile && + unlink(priv->pidfile) < 0 && + errno != ENOENT) VIR_WARN("Failed to remove PID file for %s: %s", vm->def->name, virStrerror(errno, ebuf, sizeof(ebuf))); - return 0; } @@ -2339,7 +2341,6 @@ int qemuProcessStart(virConnectPtr conn, int ret; off_t pos = -1; char ebuf[1024]; - char *pidfile = NULL; int logfile = -1; char *timestamp; qemuDomainObjPrivatePtr priv = vm->privateData; @@ -2491,16 +2492,18 @@ int qemuProcessStart(virConnectPtr conn, priv->monStart = 0; priv->gotShutdown = false; - if ((ret = virFileDeletePid(driver->stateDir, vm->def->name)) != 0) { - virReportSystemError(ret, - _("Cannot remove stale PID file for %s"), - vm->def->name); + VIR_FREE(priv->pidfile); + if (!(priv->pidfile = virFilePid(driver->stateDir, vm->def->name))) { + virReportSystemError(errno, + "%s", _("Failed to build pidfile path.")); goto cleanup; } - if (!(pidfile = virFilePid(driver->stateDir, vm->def->name))) { + if (unlink(priv->pidfile) < 0 && + errno != ENOENT) { virReportSystemError(errno, - "%s", _("Failed to build pidfile path.")); + _("Cannot remove stale PID file %s"), + priv->pidfile); goto cleanup; } @@ -2591,16 +2594,15 @@ int qemuProcessStart(virConnectPtr conn, virCommandSetOutputFD(cmd, &logfile); virCommandSetErrorFD(cmd, &logfile); virCommandNonblockingFDs(cmd); - virCommandSetPidFile(cmd, pidfile); + virCommandSetPidFile(cmd, priv->pidfile); virCommandDaemonize(cmd); virCommandRequireHandshake(cmd); ret = virCommandRun(cmd, NULL); - VIR_FREE(pidfile); /* wait for qemu process to show up */ if (ret == 0) { - if (virFileReadPid(driver->stateDir, vm->def->name, &vm->pid)) { + if (virFileReadPidPath(priv->pidfile, &vm->pid)) { qemuReportError(VIR_ERR_INTERNAL_ERROR, _("Domain %s didn't show up"), vm->def->name); ret = -1; @@ -2946,6 +2948,7 @@ retry: priv->nvcpupids = 0; qemuCapsFree(priv->qemuCaps); priv->qemuCaps = NULL; + VIR_FREE(priv->pidfile); /* The "release" hook cleans up additional resources */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { diff --git a/src/util/util.c b/src/util/util.c index 13973c3..5f93658 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1231,42 +1231,57 @@ cleanup: return rc; } -int virFileReadPid(const char *dir, - const char *name, - pid_t *pid) + +int virFileReadPidPath(const char *path, + pid_t *pid) { - int rc; FILE *file; - char *pidfile = NULL; + int rc; + *pid = 0; - if (name == NULL || dir == NULL) { - rc = EINVAL; + if (!(file = fopen(path, "r"))) { + rc = errno; goto cleanup; } - if (!(pidfile = virFilePid(dir, name))) { - rc = ENOMEM; + if (fscanf(file, "%d", pid) != 1) { + rc = EINVAL; + VIR_FORCE_FCLOSE(file); goto cleanup; } - if (!(file = fopen(pidfile, "r"))) { + if (VIR_FCLOSE(file) < 0) { rc = errno; goto cleanup; } - if (fscanf(file, "%d", pid) != 1) { + rc = 0; + + cleanup: + return rc; +} + + +int virFileReadPid(const char *dir, + const char *name, + pid_t *pid) +{ + int rc; + char *pidfile = NULL; + *pid = 0; + + if (name == NULL || dir == NULL) { rc = EINVAL; - VIR_FORCE_FCLOSE(file); goto cleanup; } - if (VIR_FCLOSE(file) < 0) { - rc = errno; + if (!(pidfile = virFilePid(dir, name))) { + rc = ENOMEM; goto cleanup; } - rc = 0; + rc = virFileReadPidPath(pidfile, pid); cleanup: VIR_FREE(pidfile); diff --git a/src/util/util.h b/src/util/util.h index 1555653..f4e0148 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -125,6 +125,8 @@ int virFileWritePidPath(const char *path, int virFileWritePid(const char *dir, const char *name, pid_t pid) ATTRIBUTE_RETURN_CHECK; +int virFileReadPidPath(const char *path, + pid_t *pid) ATTRIBUTE_RETURN_CHECK; int virFileReadPid(const char *dir, const char *name, pid_t *pid) ATTRIBUTE_RETURN_CHECK; -- 1.7.4.4

2011/7/4 Daniel P. Berrange <berrange@redhat.com>:
Avoid re-formatting the pidfile path everytime we need it. Create it once when starting the guest, and preserve it until the guest is shutdown.
* src/libvirt_private.syms, src/util/util.c, src/util/util.h: Add virFileReadPidPath * src/qemu/qemu_domain.h: Add pidfile field * src/qemu/qemu_process.c: Store pidfile path in qemuDomainObjPrivate --- src/libvirt_private.syms | 1 + src/qemu/qemu_domain.h | 1 + src/qemu/qemu_process.c | 27 +++++++++++++++------------ src/util/util.c | 45 ++++++++++++++++++++++++++++++--------------- src/util/util.h | 2 ++ 5 files changed, 49 insertions(+), 27 deletions(-)
ACK. -- Matthias Bolte http://photron.blogspot.com

When converting QEMU argv into a virDomainDefPtr, also extract the pidfile, monitor character device config and the monitor mode. * src/qemu/qemu_command.c, src/qemu/qemu_command.h: Extract pidfile & monitor config from QEMU argv * src/qemu/qemu_driver.c, tests/qemuargv2xmltest.c: Add extra params when calling qemuParseCommandLineString --- src/qemu/qemu_command.c | 150 +++++++++++++++++++++++++++------------------ src/qemu/qemu_command.h | 10 +++- src/qemu/qemu_driver.c | 9 +++- tests/qemuargv2xmltest.c | 3 +- 4 files changed, 108 insertions(+), 64 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6e4480e..e711f0e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5510,84 +5510,80 @@ cleanup: /* * Tries to parse a QEMU serial/parallel device */ -static virDomainChrDefPtr -qemuParseCommandLineChr(const char *val) +static int +qemuParseCommandLineChr(virDomainChrSourceDefPtr source, + const char *val) { - virDomainChrDefPtr def; - - if (!(def = virDomainChrDefNew())) - goto error; - if (STREQ(val, "null")) { - def->source.type = VIR_DOMAIN_CHR_TYPE_NULL; + source->type = VIR_DOMAIN_CHR_TYPE_NULL; } else if (STREQ(val, "vc")) { - def->source.type = VIR_DOMAIN_CHR_TYPE_VC; + source->type = VIR_DOMAIN_CHR_TYPE_VC; } else if (STREQ(val, "pty")) { - def->source.type = VIR_DOMAIN_CHR_TYPE_PTY; + source->type = VIR_DOMAIN_CHR_TYPE_PTY; } else if (STRPREFIX(val, "file:")) { - def->source.type = VIR_DOMAIN_CHR_TYPE_FILE; - def->source.data.file.path = strdup(val+strlen("file:")); - if (!def->source.data.file.path) + source->type = VIR_DOMAIN_CHR_TYPE_FILE; + source->data.file.path = strdup(val+strlen("file:")); + if (!source->data.file.path) goto no_memory; } else if (STRPREFIX(val, "pipe:")) { - def->source.type = VIR_DOMAIN_CHR_TYPE_PIPE; - def->source.data.file.path = strdup(val+strlen("pipe:")); - if (!def->source.data.file.path) + source->type = VIR_DOMAIN_CHR_TYPE_PIPE; + source->data.file.path = strdup(val+strlen("pipe:")); + if (!source->data.file.path) goto no_memory; } else if (STREQ(val, "stdio")) { - def->source.type = VIR_DOMAIN_CHR_TYPE_STDIO; + source->type = VIR_DOMAIN_CHR_TYPE_STDIO; } else if (STRPREFIX(val, "udp:")) { const char *svc1, *host2, *svc2; - def->source.type = VIR_DOMAIN_CHR_TYPE_UDP; + source->type = VIR_DOMAIN_CHR_TYPE_UDP; val += strlen("udp:"); svc1 = strchr(val, ':'); host2 = svc1 ? strchr(svc1, '@') : NULL; svc2 = host2 ? strchr(host2, ':') : NULL; if (svc1) - def->source.data.udp.connectHost = strndup(val, svc1-val); + source->data.udp.connectHost = strndup(val, svc1-val); else - def->source.data.udp.connectHost = strdup(val); + source->data.udp.connectHost = strdup(val); - if (!def->source.data.udp.connectHost) + if (!source->data.udp.connectHost) goto no_memory; if (svc1) { svc1++; if (host2) - def->source.data.udp.connectService = strndup(svc1, host2-svc1); + source->data.udp.connectService = strndup(svc1, host2-svc1); else - def->source.data.udp.connectService = strdup(svc1); + source->data.udp.connectService = strdup(svc1); - if (!def->source.data.udp.connectService) + if (!source->data.udp.connectService) goto no_memory; } if (host2) { host2++; if (svc2) - def->source.data.udp.bindHost = strndup(host2, svc2-host2); + source->data.udp.bindHost = strndup(host2, svc2-host2); else - def->source.data.udp.bindHost = strdup(host2); + source->data.udp.bindHost = strdup(host2); - if (!def->source.data.udp.bindHost) + if (!source->data.udp.bindHost) goto no_memory; } if (svc2) { svc2++; - def->source.data.udp.bindService = strdup(svc2); - if (!def->source.data.udp.bindService) + source->data.udp.bindService = strdup(svc2); + if (!source->data.udp.bindService) goto no_memory; } } else if (STRPREFIX(val, "tcp:") || STRPREFIX(val, "telnet:")) { const char *opt, *svc; - def->source.type = VIR_DOMAIN_CHR_TYPE_TCP; + source->type = VIR_DOMAIN_CHR_TYPE_TCP; if (STRPREFIX(val, "tcp:")) { val += strlen("tcp:"); } else { val += strlen("telnet:"); - def->source.data.tcp.protocol = VIR_DOMAIN_CHR_TCP_PROTOCOL_TELNET; + source->data.tcp.protocol = VIR_DOMAIN_CHR_TCP_PROTOCOL_TELNET; } svc = strchr(val, ':'); if (!svc) { @@ -5597,38 +5593,38 @@ qemuParseCommandLineChr(const char *val) } opt = strchr(svc, ','); if (opt && strstr(opt, "server")) - def->source.data.tcp.listen = true; + source->data.tcp.listen = true; - def->source.data.tcp.host = strndup(val, svc-val); - if (!def->source.data.tcp.host) + source->data.tcp.host = strndup(val, svc-val); + if (!source->data.tcp.host) goto no_memory; svc++; if (opt) { - def->source.data.tcp.service = strndup(svc, opt-svc); + source->data.tcp.service = strndup(svc, opt-svc); } else { - def->source.data.tcp.service = strdup(svc); + source->data.tcp.service = strdup(svc); } - if (!def->source.data.tcp.service) + if (!source->data.tcp.service) goto no_memory; } else if (STRPREFIX(val, "unix:")) { const char *opt; val += strlen("unix:"); opt = strchr(val, ','); - def->source.type = VIR_DOMAIN_CHR_TYPE_UNIX; + source->type = VIR_DOMAIN_CHR_TYPE_UNIX; if (opt) { if (strstr(opt, "listen")) - def->source.data.nix.listen = true; - def->source.data.nix.path = strndup(val, opt-val); + source->data.nix.listen = true; + source->data.nix.path = strndup(val, opt-val); } else { - def->source.data.nix.path = strdup(val); + source->data.nix.path = strdup(val); } - if (!def->source.data.nix.path) + if (!source->data.nix.path) goto no_memory; } else if (STRPREFIX(val, "/dev")) { - def->source.type = VIR_DOMAIN_CHR_TYPE_DEV; - def->source.data.file.path = strdup(val); - if (!def->source.data.file.path) + source->type = VIR_DOMAIN_CHR_TYPE_DEV; + source->data.file.path = strdup(val); + if (!source->data.file.path) goto no_memory; } else { qemuReportError(VIR_ERR_INTERNAL_ERROR, @@ -5636,13 +5632,12 @@ qemuParseCommandLineChr(const char *val) goto error; } - return def; + return 0; no_memory: virReportOOMError(); error: - virDomainChrDefFree(def); - return NULL; + return -1; } @@ -5816,7 +5811,10 @@ error: */ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, const char **progenv, - const char **progargv) + const char **progargv, + char **pidfile, + virDomainChrSourceDefPtr *monConfig, + bool *monJSON) { virDomainDefPtr def; int i; @@ -5829,6 +5827,13 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, int nvirtiodisk = 0; qemuDomainCmdlineDefPtr cmd = NULL; + if (pidfile) + *pidfile = NULL; + if (monConfig) + *monConfig = NULL; + if (monJSON) + *monJSON = false; + if (!progargv[0]) { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no emulator path found")); @@ -6185,7 +6190,11 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, WANT_VALUE(); if (STRNEQ(val, "none")) { virDomainChrDefPtr chr; - if (!(chr = qemuParseCommandLineChr(val))) + + if (!(chr = virDomainChrDefNew())) + goto error; + + if (qemuParseCommandLineChr(&chr->source, val) < 0) goto error; if (VIR_REALLOC_N(def->serials, def->nserials+1) < 0) { virDomainChrDefFree(chr); @@ -6199,7 +6208,11 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, WANT_VALUE(); if (STRNEQ(val, "none")) { virDomainChrDefPtr chr; - if (!(chr = qemuParseCommandLineChr(val))) + + if (!(chr = virDomainChrDefNew())) + goto error; + + if (qemuParseCommandLineChr(&chr->source, val) < 0) goto error; if (VIR_REALLOC_N(def->parallels, def->nparallels+1) < 0) { virDomainChrDefFree(chr); @@ -6376,13 +6389,25 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, /* ignore, always added by libvirt */ } else if (STREQ(arg, "-pidfile")) { WANT_VALUE(); - /* ignore, used by libvirt as needed */ + if (pidfile) + if (!(*pidfile = strdup(val))) + goto no_memory; } else if (STREQ(arg, "-incoming")) { WANT_VALUE(); /* ignore, used via restore/migrate APIs */ } else if (STREQ(arg, "-monitor")) { WANT_VALUE(); - /* ignore, used internally by libvirt */ + if (monConfig) { + virDomainChrSourceDefPtr chr; + + if (VIR_ALLOC(chr) < 0) + goto no_memory; + + if (qemuParseCommandLineChr(chr, val) < 0) + goto error; + + *monConfig = chr; + } } else if (STREQ(arg, "-S")) { /* ignore, always added by libvirt */ } else { @@ -6523,11 +6548,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, VIR_FREE(nics); - if (!def->name) { - if (!(def->name = strdup("unnamed"))) - goto no_memory; - } - if (virDomainDefAddImplicitControllers(def) < 0) goto error; @@ -6546,12 +6566,21 @@ error: VIR_FREE(cmd); virDomainDefFree(def); VIR_FREE(nics); + if (monConfig) { + virDomainChrSourceDefFree(*monConfig); + *monConfig = NULL; + } + if (pidfile) + VIR_FREE(*pidfile); return NULL; } virDomainDefPtr qemuParseCommandLineString(virCapsPtr caps, - const char *args) + const char *args, + char **pidfile, + virDomainChrSourceDefPtr *monConfig, + bool *monJSON) { const char **progenv = NULL; const char **progargv = NULL; @@ -6561,7 +6590,8 @@ virDomainDefPtr qemuParseCommandLineString(virCapsPtr caps, if (qemuStringToArgvEnv(args, &progenv, &progargv) < 0) goto cleanup; - def = qemuParseCommandLine(caps, progenv, progargv); + def = qemuParseCommandLine(caps, progenv, progargv, + pidfile, monConfig, monJSON); cleanup: for (i = 0 ; progargv && progargv[i] ; i++) diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 96ec669..89502b7 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -142,9 +142,15 @@ int qemudCanonicalizeMachine(struct qemud_driver *driver, virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, const char **progenv, - const char **progargv); + const char **progargv, + char **pidfile, + virDomainChrSourceDefPtr *monConfig, + bool *monJSON); virDomainDefPtr qemuParseCommandLineString(virCapsPtr caps, - const char *args); + const char *args, + char **pidfile, + virDomainChrSourceDefPtr *monConfig, + bool *monJSON); int qemuDomainAssignPCIAddresses(virDomainDefPtr def); qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 363a361..9486594 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3887,11 +3887,18 @@ static char *qemuDomainXMLFromNative(virConnectPtr conn, } qemuDriverLock(driver); - def = qemuParseCommandLineString(driver->caps, config); + def = qemuParseCommandLineString(driver->caps, config, + NULL, NULL, NULL); qemuDriverUnlock(driver); if (!def) goto cleanup; + if (!def->name && + !(def->name = strdup("unnamed"))) { + virReportOOMError(); + goto cleanup; + } + xml = virDomainDefFormat(def, VIR_DOMAIN_XML_INACTIVE); cleanup: diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c index db68b60..bade95d 100644 --- a/tests/qemuargv2xmltest.c +++ b/tests/qemuargv2xmltest.c @@ -45,7 +45,8 @@ static int testCompareXMLToArgvFiles(const char *xml, if (virtTestLoadFile(xml, &expectxml) < 0) goto fail; - if (!(vmdef = qemuParseCommandLineString(driver.caps, cmd))) + if (!(vmdef = qemuParseCommandLineString(driver.caps, cmd, + NULL, NULL, NULL))) goto fail; if ((log = virtTestLogContentAndReset()) == NULL) -- 1.7.4.4

2011/7/4 Daniel P. Berrange <berrange@redhat.com>:
When converting QEMU argv into a virDomainDefPtr, also extract the pidfile, monitor character device config and the monitor mode.
* src/qemu/qemu_command.c, src/qemu/qemu_command.h: Extract pidfile & monitor config from QEMU argv * src/qemu/qemu_driver.c, tests/qemuargv2xmltest.c: Add extra params when calling qemuParseCommandLineString --- src/qemu/qemu_command.c | 150 +++++++++++++++++++++++++++------------------ src/qemu/qemu_command.h | 10 +++- src/qemu/qemu_driver.c | 9 +++- tests/qemuargv2xmltest.c | 3 +- 4 files changed, 108 insertions(+), 64 deletions(-)
@@ -6523,11 +6548,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
VIR_FREE(nics);
- if (!def->name) { - if (!(def->name = strdup("unnamed"))) - goto no_memory; - } - if (virDomainDefAddImplicitControllers(def) < 0) goto error;
Okay you moved this to qemuDomainXMLFromNative, so now a direct call to qemuParseCommandLine will produce an invalid virDomainDefPtr as def->name can be NULL when -name was not given in the command line. It seems that this is okay for now as the only caller is currently qemuDomainXMLFromNative. You added a second caller in 8/8 that explicitly deals with def->name == NULL. Might be worth a comment to qemuParseCommandLine that mentions this.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 363a361..9486594 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3887,11 +3887,18 @@ static char *qemuDomainXMLFromNative(virConnectPtr conn, }
qemuDriverLock(driver); - def = qemuParseCommandLineString(driver->caps, config); + def = qemuParseCommandLineString(driver->caps, config, + NULL, NULL, NULL); qemuDriverUnlock(driver); if (!def) goto cleanup;
+ if (!def->name && + !(def->name = strdup("unnamed"))) { + virReportOOMError(); + goto cleanup; + } + xml = virDomainDefFormat(def, VIR_DOMAIN_XML_INACTIVE);
ACK. -- Matthias Bolte http://photron.blogspot.com

To enable attaching to externally launched QEMU, we need to be able to reverse engineer a guest XML config based on the argv for a PID in /proc * src/qemu/qemu_command.c, src/qemu/qemu_command.h: Add qemuParseCommandLinePid which extracts QEMU config from argv in /proc, given a PID number --- src/qemu/qemu_command.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_command.h | 5 ++ 2 files changed, 111 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e711f0e..dbc9e0c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6604,3 +6604,109 @@ cleanup: return def; } + + +static int qemuParseProcFileStrings(unsigned long long pid, + const char *name, + const char ***list) +{ + char *path = NULL; + int ret = -1; + char *data = NULL; + ssize_t len; + char *tmp; + size_t nstr = 0; + const char **str = NULL; + int i; + + if (virAsprintf(&path, "/proc/%llu/%s", pid, name) < 0) { + virReportOOMError(); + goto cleanup; + } + + if ((len = virFileReadAll(path, 1024*128, &data)) < 0) + goto cleanup; + + tmp = data; + while (tmp < (data + len)) { + if (VIR_EXPAND_N(str, nstr, 1) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (!(str[nstr-1] = strdup(tmp))) { + virReportOOMError(); + goto cleanup; + } + /* Skip arg */ + tmp += strlen(tmp); + /* Skip \0 separator */ + tmp++; + } + + if (VIR_EXPAND_N(str, nstr, 1) < 0) { + virReportOOMError(); + goto cleanup; + } + + str[nstr-1] = NULL; + + ret = nstr-1; + *list = str; + +cleanup: + if (ret < 0) { + for (i = 0 ; str && str[i] ; i++) + VIR_FREE(str[i]); + VIR_FREE(str); + } + VIR_FREE(data); + VIR_FREE(path); + return ret; +} + +virDomainDefPtr qemuParseCommandLinePid(virCapsPtr caps, + unsigned long long pid, + char **pidfile, + virDomainChrSourceDefPtr *monConfig, + bool *monJSON) +{ + virDomainDefPtr def = NULL; + const char **progargv = NULL; + const char **progenv = NULL; + char *exepath = NULL; + char *emulator; + int i; + + if (qemuParseProcFileStrings(pid, "cmdline", &progargv) < 0 || + qemuParseProcFileStrings(pid, "environ", &progenv) < 0) + goto cleanup; + + if (!(def = qemuParseCommandLine(caps, progenv, progargv, + pidfile, monConfig, monJSON))) + goto cleanup; + + if (virAsprintf(&exepath, "/proc/%llu/exe", pid) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (virFileResolveLink(exepath, &emulator) < 0) { + virReportSystemError(errno, + _("Unable to resolve %s for pid %llu"), + exepath, pid); + goto cleanup; + } + VIR_FREE(def->emulator); + def->emulator = emulator; + +cleanup: + VIR_FREE(exepath); + for (i = 0 ; progargv && progargv[i] ; i++) + VIR_FREE(progargv[i]); + VIR_FREE(progargv); + for (i = 0 ; progenv && progenv[i] ; i++) + VIR_FREE(progenv[i]); + VIR_FREE(progenv); + return def; +} diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 89502b7..928b53e 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -151,6 +151,11 @@ virDomainDefPtr qemuParseCommandLineString(virCapsPtr caps, char **pidfile, virDomainChrSourceDefPtr *monConfig, bool *monJSON); +virDomainDefPtr qemuParseCommandLinePid(virCapsPtr caps, + unsigned long long pid, + char **pidfile, + virDomainChrSourceDefPtr *monConfig, + bool *monJSON); int qemuDomainAssignPCIAddresses(virDomainDefPtr def); qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def); -- 1.7.4.4

2011/7/4 Daniel P. Berrange <berrange@redhat.com>:
To enable attaching to externally launched QEMU, we need to be able to reverse engineer a guest XML config based on the argv for a PID in /proc
* src/qemu/qemu_command.c, src/qemu/qemu_command.h: Add qemuParseCommandLinePid which extracts QEMU config from argv in /proc, given a PID number --- src/qemu/qemu_command.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_command.h | 5 ++ 2 files changed, 111 insertions(+), 0 deletions(-)
ACK. -- Matthias Bolte http://photron.blogspot.com

When attaching to an external QEMU process, it is neccessary to check if the process is using KVM or not. This can be done using a monitor command * src/qemu/qemu_monitor.c, src/qemu/qemu_monitor.h, src/qemu/qemu_monitor_json.c, src/qemu/qemu_monitor_json.h, src/qemu/qemu_monitor_text.c, src/qemu/qemu_monitor_text.h: Add API for checking if KVM is enabled --- src/qemu/qemu_monitor.c | 21 +++++++++++++++++++ src/qemu/qemu_monitor.h | 2 + src/qemu/qemu_monitor_json.c | 46 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 2 + src/qemu/qemu_monitor_text.c | 22 ++++++++++++++++++++ src/qemu/qemu_monitor_text.h | 2 + 6 files changed, 95 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 8573262..e593642 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1122,6 +1122,27 @@ int qemuMonitorGetCPUInfo(qemuMonitorPtr mon, return ret; } + +int qemuMonitorGetVirtType(qemuMonitorPtr mon, + int *virtType) +{ + int ret; + VIR_DEBUG("mon=%p", mon); + + if (!mon) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("monitor must not be NULL")); + return -1; + } + + if (mon->json) + ret = qemuMonitorJSONGetVirtType(mon, virtType); + else + ret = qemuMonitorTextGetVirtType(mon, virtType); + return ret; +} + + int qemuMonitorGetBalloonInfo(qemuMonitorPtr mon, unsigned long *currmem) { diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index cb8f799..893f3e9 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -191,6 +191,8 @@ int qemuMonitorSystemPowerdown(qemuMonitorPtr mon); int qemuMonitorGetCPUInfo(qemuMonitorPtr mon, int **pids); +int qemuMonitorGetVirtType(qemuMonitorPtr mon, + int *virtType); int qemuMonitorGetBalloonInfo(qemuMonitorPtr mon, unsigned long *currmem); int qemuMonitorGetMemoryStats(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 81b7f8c..4db2b78 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1042,6 +1042,52 @@ int qemuMonitorJSONGetCPUInfo(qemuMonitorPtr mon, } +int qemuMonitorJSONGetVirtType(qemuMonitorPtr mon, + int *virtType) +{ + int ret; + virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-kvm", + NULL); + virJSONValuePtr reply = NULL; + + *virtType = VIR_DOMAIN_VIRT_QEMU; + + if (!cmd) + return -1; + + ret = qemuMonitorJSONCommand(mon, cmd, &reply); + + if (ret == 0) + ret = qemuMonitorJSONCheckError(cmd, reply); + + if (ret == 0) { + virJSONValuePtr data; + bool val = false; + + if (!(data = virJSONValueObjectGet(reply, "return"))) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("info kvm reply was missing return data")); + ret = -1; + goto cleanup; + } + + if (virJSONValueObjectGetBoolean(data, "enabled", &val) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("info kvm reply missing 'running' field")); + ret = -1; + goto cleanup; + } + if (val) + *virtType = VIR_DOMAIN_VIRT_KVM; + } + +cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} + + /* * Returns: 0 if balloon not supported, +1 if balloon query worked * or -1 on failure diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index c571adb..380e26a 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -53,6 +53,8 @@ int qemuMonitorJSONSystemReset(qemuMonitorPtr mon); int qemuMonitorJSONGetCPUInfo(qemuMonitorPtr mon, int **pids); +int qemuMonitorJSONGetVirtType(qemuMonitorPtr mon, + int *virtType); int qemuMonitorJSONGetBalloonInfo(qemuMonitorPtr mon, unsigned long *currmem); int qemuMonitorJSONGetMemoryStats(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index aa5d1c6..0965a08 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -512,6 +512,28 @@ error: return 0; } + +int qemuMonitorTextGetVirtType(qemuMonitorPtr mon, + int *virtType) +{ + char *reply = NULL; + + *virtType = VIR_DOMAIN_VIRT_QEMU; + + if (qemuMonitorHMPCommand(mon, "info kvm", &reply) < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + "%s", _("could not query kvm status")); + return -1; + } + + if (strstr(reply, "enabled")) + *virtType = VIR_DOMAIN_VIRT_KVM; + + VIR_FREE(reply); + return 0; +} + + static int parseMemoryStat(char **text, unsigned int tag, const char *search, virDomainMemoryStatPtr stat) { diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h index 2d9288f..e53f693 100644 --- a/src/qemu/qemu_monitor_text.h +++ b/src/qemu/qemu_monitor_text.h @@ -50,6 +50,8 @@ int qemuMonitorTextSystemReset(qemuMonitorPtr mon); int qemuMonitorTextGetCPUInfo(qemuMonitorPtr mon, int **pids); +int qemuMonitorTextGetVirtType(qemuMonitorPtr mon, + int *virtType); int qemuMonitorTextGetBalloonInfo(qemuMonitorPtr mon, unsigned long *currmem); int qemuMonitorTextGetMemoryStats(qemuMonitorPtr mon, -- 1.7.4.4

2011/7/4 Daniel P. Berrange <berrange@redhat.com>:
When attaching to an external QEMU process, it is neccessary to check if the process is using KVM or not. This can be done using a monitor command
* src/qemu/qemu_monitor.c, src/qemu/qemu_monitor.h, src/qemu/qemu_monitor_json.c, src/qemu/qemu_monitor_json.h, src/qemu/qemu_monitor_text.c, src/qemu/qemu_monitor_text.h: Add API for checking if KVM is enabled --- src/qemu/qemu_monitor.c | 21 +++++++++++++++++++ src/qemu/qemu_monitor.h | 2 + src/qemu/qemu_monitor_json.c | 46 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 2 + src/qemu/qemu_monitor_text.c | 22 ++++++++++++++++++++ src/qemu/qemu_monitor_text.h | 2 + 6 files changed, 95 insertions(+), 0 deletions(-)
ACK. -- Matthias Bolte http://photron.blogspot.com

Given a PID, the QEMU driver reads /proc/$PID/cmdline and /proc/$PID/environ to get the configuration. This is fed into the ARGV->XML convertor to build an XML configuration for the process. /proc/$PID/exe is resolved to identify the full command binary path After checking for name/uuid uniqueness, an attempt is made to connect to the monitor socket. If successful then 'info status' and 'info kvm' are issued to determine whether the CPUs are running and if KVM is enabled. * src/qemu/qemu_driver.c: Implement virDomainQemuAttach * src/qemu/qemu_process.h, src/qemu/qemu_process.c: Add qemuProcessAttach to connect to the monitor of an existing QEMU process --- src/conf/domain_conf.c | 3 +- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 2 + src/qemu/qemu_driver.c | 91 +++++++++++++++++++- src/qemu/qemu_process.c | 218 ++++++++++++++++++++++++++++++++++++++++++++--- src/qemu/qemu_process.h | 8 ++ 6 files changed, 308 insertions(+), 15 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 109a947..c5b6dd5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -55,7 +55,8 @@ VIR_ENUM_IMPL(virDomainTaint, VIR_DOMAIN_TAINT_LAST, "custom-monitor", "high-privileges", "shell-scripts", - "disk-probing"); + "disk-probing", + "external-launch"); VIR_ENUM_IMPL(virDomainVirt, VIR_DOMAIN_VIRT_LAST, "qemu", diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 258289a..071bf50 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1249,6 +1249,7 @@ enum virDomainTaintFlags { VIR_DOMAIN_TAINT_HIGH_PRIVILEGES, /* Running with undesirably high privileges */ VIR_DOMAIN_TAINT_SHELL_SCRIPTS, /* Network configuration using opaque shell scripts */ VIR_DOMAIN_TAINT_DISK_PROBING, /* Relying on potentially unsafe disk format probing */ + VIR_DOMAIN_TAINT_EXTERNAL_LAUNCH, /* Externally launched guest domain */ VIR_DOMAIN_TAINT_LAST }; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index dbc9e0c..aa66b55 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6182,6 +6182,8 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, if (!(def->name = strndup(val, process - val))) goto no_memory; } + if (STREQ(def->name, "")) + VIR_FREE(def->name); } else if (STREQ(arg, "-M")) { WANT_VALUE(); if (!(def->os.machine = strdup(val))) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9486594..05643bc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4129,7 +4129,7 @@ qemudCanonicalizeMachineFromInfo(virDomainDefPtr def, if (!machine->canonical) continue; - if (STRNEQ(def->os.machine, machine->name)) + if (def->os.machine && STRNEQ(def->os.machine, machine->name)) continue; if (!(*canonical = strdup(machine->canonical))) { @@ -4156,7 +4156,7 @@ qemudCanonicalizeMachineDirect(virDomainDefPtr def, char **canonical) if (!machines[i]->canonical) continue; - if (STRNEQ(def->os.machine, machines[i]->name)) + if (def->os.machine && STRNEQ(def->os.machine, machines[i]->name)) continue; *canonical = machines[i]->canonical; @@ -8347,6 +8347,92 @@ cleanup: } +static virDomainPtr qemuDomainAttach(virConnectPtr conn, + unsigned long long pid, + unsigned int flags) +{ + struct qemud_driver *driver = conn->privateData; + virDomainObjPtr vm = NULL; + virDomainDefPtr def = NULL; + virDomainPtr dom = NULL; + virDomainChrSourceDefPtr monConfig = NULL; + bool monJSON = false; + char *pidfile; + + virCheckFlags(0, NULL); + + qemuDriverLock(driver); + + if (!(def = qemuParseCommandLinePid(driver->caps, pid, + &pidfile, &monConfig, &monJSON))) + goto cleanup; + + if (!monConfig) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("No monitor connection for pid %llu"), + pid); + goto cleanup; + } + if (monConfig->type != VIR_DOMAIN_CHR_TYPE_UNIX) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Cannot connect to monitor connection of type '%s' for pid %llu"), + virDomainChrTypeToString(monConfig->type), pid); + goto cleanup; + } + + if (!(def->name) && + virAsprintf(&def->name, "attach-pid-%llu", pid) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (virDomainObjIsDuplicate(&driver->domains, def, 1) < 0) + goto cleanup; + + if (qemudCanonicalizeMachine(driver, def) < 0) + goto cleanup; + + if (qemuDomainAssignPCIAddresses(def) < 0) + goto cleanup; + + if (!(vm = virDomainAssignDef(driver->caps, + &driver->domains, + def, false))) + goto cleanup; + + def = NULL; + + if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) + goto cleanup; + + if (qemuProcessAttach(conn, driver, vm, pid, + pidfile, monConfig, monJSON) < 0) { + monConfig = NULL; + goto endjob; + } + + monConfig = NULL; + + dom = virGetDomain(conn, vm->def->name, vm->def->uuid); + if (dom) dom->id = vm->def->id; + +endjob: + if (qemuDomainObjEndJob(vm) == 0) { + vm = NULL; + goto cleanup; + } + +cleanup: + virDomainDefFree(def); + virDomainChrSourceDefFree(monConfig); + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + VIR_FREE(pidfile); + return dom; +} + + static int qemuDomainOpenConsole(virDomainPtr dom, const char *devname, @@ -8539,6 +8625,7 @@ static virDriver qemuDriver = { .domainRevertToSnapshot = qemuDomainRevertToSnapshot, /* 0.8.0 */ .domainSnapshotDelete = qemuDomainSnapshotDelete, /* 0.8.0 */ .qemuDomainMonitorCommand = qemuDomainMonitorCommand, /* 0.8.3 */ + .qemuDomainAttach = qemuDomainAttach, /* 0.9.3 */ .domainOpenConsole = qemuDomainOpenConsole, /* 0.8.6 */ .domainInjectNMI = qemuDomainInjectNMI, /* 0.9.2 */ .domainMigrateBegin3 = qemuDomainMigrateBegin3, /* 0.9.2 */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index bbbc36f..affdc20 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -61,6 +61,7 @@ #define VIR_FROM_THIS VIR_FROM_QEMU #define START_POSTFIX ": starting up\n" +#define ATTACH_POSTFIX ": attaching\n" #define SHUTDOWN_POSTFIX ": shutting down\n" /** @@ -1125,31 +1126,34 @@ qemuProcessReadLogFD(int logfd, char *buf, int maxlen, int off) tmpbuf[ret] = '\0'; } + static int qemuProcessWaitForMonitor(struct qemud_driver* driver, virDomainObjPtr vm, virBitmapPtr qemuCaps, off_t pos) { - char *buf; + char *buf = NULL; size_t buf_size = 4096; /* Plenty of space to get startup greeting */ - int logfd; + int logfd = -1; int ret = -1; virHashTablePtr paths = NULL; qemuDomainObjPrivatePtr priv; - if ((logfd = qemuDomainOpenLog(driver, vm, pos)) < 0) - return -1; + if (pos != -1) { + if ((logfd = qemuDomainOpenLog(driver, vm, pos)) < 0) + return -1; - if (VIR_ALLOC_N(buf, buf_size) < 0) { - virReportOOMError(); - return -1; - } + if (VIR_ALLOC_N(buf, buf_size) < 0) { + virReportOOMError(); + return -1; + } - if (qemuProcessReadLogOutput(vm, logfd, buf, buf_size, - qemuProcessFindCharDevicePTYs, - "console", 30) < 0) - goto closelog; + if (qemuProcessReadLogOutput(vm, logfd, buf, buf_size, + qemuProcessFindCharDevicePTYs, + "console", 30) < 0) + goto closelog; + } VIR_DEBUG("Connect monitor to %p '%s'", vm, vm->def->name); if (qemuConnectMonitor(driver, vm) < 0) { @@ -1195,6 +1199,8 @@ closelog: virStrerror(errno, ebuf, sizeof ebuf)); } + VIR_FREE(buf); + return ret; } @@ -2974,6 +2980,194 @@ retry: } +int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, + struct qemud_driver *driver, + virDomainObjPtr vm, + int pid, + const char *pidfile, + virDomainChrSourceDefPtr monConfig, + bool monJSON) +{ + char ebuf[1024]; + int logfile = -1; + char *timestamp; + qemuDomainObjPrivatePtr priv = vm->privateData; + bool running = true; + virSecurityLabelPtr seclabel = NULL; + + VIR_DEBUG("Beginning VM attach process"); + + if (virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("VM is already active")); + return -1; + } + + /* Do this upfront, so any part of the startup process can add + * runtime state to vm->def that won't be persisted. This let's us + * report implicit runtime defaults in the XML, like vnc listen/socket + */ + VIR_DEBUG("Setting current domain def as transient"); + if (virDomainObjSetDefTransient(driver->caps, vm, true) < 0) + goto cleanup; + + vm->def->id = driver->nextvmid++; + + if (virFileMakePath(driver->logDir) != 0) { + virReportSystemError(errno, + _("cannot create log directory %s"), + driver->logDir); + goto cleanup; + } + + VIR_FREE(priv->pidfile); + if (pidfile && + !(priv->pidfile = strdup(pidfile))) + goto no_memory; + + VIR_DEBUG("Detect security driver config"); + vm->def->seclabel.type = VIR_DOMAIN_SECLABEL_STATIC; + if (VIR_ALLOC(seclabel) < 0) + goto no_memory; + if (virSecurityManagerGetProcessLabel(driver->securityManager, + vm, seclabel) < 0) + goto cleanup; + if (!(vm->def->seclabel.model = strdup(driver->caps->host.secModel.model))) + goto no_memory; + if (!(vm->def->seclabel.label = strdup(seclabel->label))) + goto no_memory; + + VIR_DEBUG("Creating domain log file"); + if ((logfile = qemuDomainCreateLog(driver, vm, false)) < 0) + goto cleanup; + + VIR_DEBUG("Determining emulator version"); + qemuCapsFree(priv->qemuCaps); + priv->qemuCaps = NULL; + if (qemuCapsExtractVersionInfo(vm->def->emulator, + vm->def->os.arch, + NULL, + &priv->qemuCaps) < 0) + goto cleanup; + + if (VIR_ALLOC(priv->monConfig) < 0) { + virReportOOMError(); + goto cleanup; + } + + VIR_DEBUG("Preparing monitor state"); + priv->monConfig = monConfig; + monConfig = NULL; + priv->monJSON = monJSON; + + priv->gotShutdown = false; + + /* + * Normally PCI addresses are assigned in the virDomainCreate + * or virDomainDefine methods. We might still need to assign + * some here to cope with the question of upgrades. Regardless + * we also need to populate the PCi address set cache for later + * use in hotplug + */ + if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { + VIR_DEBUG("Assigning domain PCI addresses"); + /* Populate cache with current addresses */ + if (priv->pciaddrs) { + qemuDomainPCIAddressSetFree(priv->pciaddrs); + priv->pciaddrs = NULL; + } + if (!(priv->pciaddrs = qemuDomainPCIAddressSetCreate(vm->def))) + goto cleanup; + + /* Assign any remaining addresses */ + if (qemuAssignDevicePCISlots(vm->def, priv->pciaddrs) < 0) + goto cleanup; + + priv->persistentAddrs = 1; + } else { + priv->persistentAddrs = 0; + } + + if ((timestamp = virTimestamp()) == NULL) { + virReportOOMError(); + goto cleanup; + } else { + if (safewrite(logfile, timestamp, strlen(timestamp)) < 0 || + safewrite(logfile, ATTACH_POSTFIX, strlen(ATTACH_POSTFIX)) < 0) { + VIR_WARN("Unable to write timestamp to logfile: %s", + virStrerror(errno, ebuf, sizeof ebuf)); + } + + VIR_FREE(timestamp); + } + + qemuDomainObjTaint(driver, vm, VIR_DOMAIN_TAINT_EXTERNAL_LAUNCH, logfile); + + vm->pid = pid; + + VIR_DEBUG("Waiting for monitor to show up"); + if (qemuProcessWaitForMonitor(driver, vm, priv->qemuCaps, -1) < 0) + goto cleanup; + + VIR_DEBUG("Detecting VCPU PIDs"); + if (qemuProcessDetectVcpuPIDs(driver, vm) < 0) + goto cleanup; + + /* If we have -device, then addresses are assigned explicitly. + * If not, then we have to detect dynamic ones here */ + if (!qemuCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { + VIR_DEBUG("Determining domain device PCI addresses"); + if (qemuProcessInitPCIAddresses(driver, vm) < 0) + goto cleanup; + } + + VIR_DEBUG("Getting initial memory amount"); + qemuDomainObjEnterMonitorWithDriver(driver, vm); + if (qemuMonitorGetBalloonInfo(priv->mon, &vm->def->mem.cur_balloon) < 0) { + qemuDomainObjExitMonitorWithDriver(driver, vm); + goto cleanup; + } + if (qemuMonitorGetStatus(priv->mon, &running) < 0) { + qemuDomainObjExitMonitorWithDriver(driver, vm); + goto cleanup; + } + if (qemuMonitorGetVirtType(priv->mon, &vm->def->virtType) < 0) { + qemuDomainObjExitMonitorWithDriver(driver, vm); + goto cleanup; + } + qemuDomainObjExitMonitorWithDriver(driver, vm); + + if (!virDomainObjIsActive(vm)) + goto cleanup; + + if (running) + virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, + VIR_DOMAIN_RUNNING_UNPAUSED); + else + virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_UNKNOWN); + + VIR_DEBUG("Writing domain status to disk"); + if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0) + goto cleanup; + + VIR_FORCE_CLOSE(logfile); + VIR_FREE(seclabel); + + return 0; + +no_memory: + virReportOOMError(); +cleanup: + /* We jump here if we failed to start the VM for any reason, or + * if we failed to initialize the now running VM. kill it off and + * pretend we never started it */ + VIR_FORCE_CLOSE(logfile); + VIR_FREE(seclabel); + virDomainChrSourceDefFree(monConfig); + return -1; +} + + int qemuProcessAutoDestroyInit(struct qemud_driver *driver) { if (!(driver->autodestroy = virHashCreate(5, NULL))) diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 0e74d2a..449d7f1 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -56,6 +56,14 @@ void qemuProcessStop(struct qemud_driver *driver, int migrated, virDomainShutoffReason reason); +int qemuProcessAttach(virConnectPtr conn, + struct qemud_driver *driver, + virDomainObjPtr vm, + int pid, + const char *pidfile, + virDomainChrSourceDefPtr monConfig, + bool monJSON); + void qemuProcessKill(virDomainObjPtr vm); int qemuProcessAutoDestroyInit(struct qemud_driver *driver); -- 1.7.4.4

2011/7/4 Daniel P. Berrange <berrange@redhat.com>:
Given a PID, the QEMU driver reads /proc/$PID/cmdline and /proc/$PID/environ to get the configuration. This is fed into the ARGV->XML convertor to build an XML configuration for the process.
/proc/$PID/exe is resolved to identify the full command binary path
After checking for name/uuid uniqueness, an attempt is made to connect to the monitor socket. If successful then 'info status' and 'info kvm' are issued to determine whether the CPUs are running and if KVM is enabled.
* src/qemu/qemu_driver.c: Implement virDomainQemuAttach * src/qemu/qemu_process.h, src/qemu/qemu_process.c: Add qemuProcessAttach to connect to the monitor of an existing QEMU process --- src/conf/domain_conf.c | 3 +- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 2 + src/qemu/qemu_driver.c | 91 +++++++++++++++++++- src/qemu/qemu_process.c | 218 ++++++++++++++++++++++++++++++++++++++++++++--- src/qemu/qemu_process.h | 8 ++ 6 files changed, 308 insertions(+), 15 deletions(-)
static int qemuDomainOpenConsole(virDomainPtr dom, const char *devname, @@ -8539,6 +8625,7 @@ static virDriver qemuDriver = { .domainRevertToSnapshot = qemuDomainRevertToSnapshot, /* 0.8.0 */ .domainSnapshotDelete = qemuDomainSnapshotDelete, /* 0.8.0 */ .qemuDomainMonitorCommand = qemuDomainMonitorCommand, /* 0.8.3 */ + .qemuDomainAttach = qemuDomainAttach, /* 0.9.3 */
s/0.9.3/0.9.4/
+int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, + struct qemud_driver *driver, + virDomainObjPtr vm, + int pid, + const char *pidfile, + virDomainChrSourceDefPtr monConfig, + bool monJSON) +{ + char ebuf[1024]; + int logfile = -1; + char *timestamp; + qemuDomainObjPrivatePtr priv = vm->privateData; + bool running = true; + virSecurityLabelPtr seclabel = NULL; + + VIR_DEBUG("Beginning VM attach process"); + + if (virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("VM is already active")); + return -1; + } + + /* Do this upfront, so any part of the startup process can add + * runtime state to vm->def that won't be persisted. This let's us + * report implicit runtime defaults in the XML, like vnc listen/socket + */ + VIR_DEBUG("Setting current domain def as transient"); + if (virDomainObjSetDefTransient(driver->caps, vm, true) < 0) + goto cleanup; + + vm->def->id = driver->nextvmid++; + + if (virFileMakePath(driver->logDir) != 0) { + virReportSystemError(errno, + _("cannot create log directory %s"), + driver->logDir); + goto cleanup; + }
This doesn't work as virFileMakePath doesn't set errno for all errors, but returns an errno value. Needs to be something like this int err; if ((err = virFileMakePath(driver->logDir)) != 0) { virReportSystemError(err, _("cannot create log directory %s"), driver->logDir); goto cleanup; } Also grep'ing for virFileMakePath shows that there are many instances that use virFileMakePath in the wrong way.
+ VIR_FREE(priv->pidfile); + if (pidfile && + !(priv->pidfile = strdup(pidfile))) + goto no_memory; + + VIR_DEBUG("Detect security driver config"); + vm->def->seclabel.type = VIR_DOMAIN_SECLABEL_STATIC; + if (VIR_ALLOC(seclabel) < 0) + goto no_memory; + if (virSecurityManagerGetProcessLabel(driver->securityManager, + vm, seclabel) < 0) + goto cleanup; + if (!(vm->def->seclabel.model = strdup(driver->caps->host.secModel.model))) + goto no_memory; + if (!(vm->def->seclabel.label = strdup(seclabel->label))) + goto no_memory; + + VIR_DEBUG("Creating domain log file"); + if ((logfile = qemuDomainCreateLog(driver, vm, false)) < 0) + goto cleanup; + + VIR_DEBUG("Determining emulator version"); + qemuCapsFree(priv->qemuCaps); + priv->qemuCaps = NULL; + if (qemuCapsExtractVersionInfo(vm->def->emulator, + vm->def->os.arch, + NULL, + &priv->qemuCaps) < 0) + goto cleanup; + + if (VIR_ALLOC(priv->monConfig) < 0) { + virReportOOMError(); + goto cleanup; + } + + VIR_DEBUG("Preparing monitor state"); + priv->monConfig = monConfig;
Allocating and overwriting priv->monConfig leaks memory here. ACK, with those problems fixed. -- Matthias Bolte http://photron.blogspot.com

On Tue, Jul 05, 2011 at 03:09:23PM +0200, Matthias Bolte wrote:
2011/7/4 Daniel P. Berrange <berrange@redhat.com>:
Given a PID, the QEMU driver reads /proc/$PID/cmdline and /proc/$PID/environ to get the configuration. This is fed into the ARGV->XML convertor to build an XML configuration for the process.
/proc/$PID/exe is resolved to identify the full command binary path
After checking for name/uuid uniqueness, an attempt is made to connect to the monitor socket. If successful then 'info status' and 'info kvm' are issued to determine whether the CPUs are running and if KVM is enabled.
* src/qemu/qemu_driver.c: Implement virDomainQemuAttach * src/qemu/qemu_process.h, src/qemu/qemu_process.c: Add qemuProcessAttach to connect to the monitor of an existing QEMU process --- src/conf/domain_conf.c | 3 +- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 2 + src/qemu/qemu_driver.c | 91 +++++++++++++++++++- src/qemu/qemu_process.c | 218 ++++++++++++++++++++++++++++++++++++++++++++--- src/qemu/qemu_process.h | 8 ++ 6 files changed, 308 insertions(+), 15 deletions(-)
static int qemuDomainOpenConsole(virDomainPtr dom, const char *devname, @@ -8539,6 +8625,7 @@ static virDriver qemuDriver = { .domainRevertToSnapshot = qemuDomainRevertToSnapshot, /* 0.8.0 */ .domainSnapshotDelete = qemuDomainSnapshotDelete, /* 0.8.0 */ .qemuDomainMonitorCommand = qemuDomainMonitorCommand, /* 0.8.3 */ + .qemuDomainAttach = qemuDomainAttach, /* 0.9.3 */
s/0.9.3/0.9.4/
+int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, + struct qemud_driver *driver, + virDomainObjPtr vm, + int pid, + const char *pidfile, + virDomainChrSourceDefPtr monConfig, + bool monJSON) +{ + char ebuf[1024]; + int logfile = -1; + char *timestamp; + qemuDomainObjPrivatePtr priv = vm->privateData; + bool running = true; + virSecurityLabelPtr seclabel = NULL; + + VIR_DEBUG("Beginning VM attach process"); + + if (virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("VM is already active")); + return -1; + } + + /* Do this upfront, so any part of the startup process can add + * runtime state to vm->def that won't be persisted. This let's us + * report implicit runtime defaults in the XML, like vnc listen/socket + */ + VIR_DEBUG("Setting current domain def as transient"); + if (virDomainObjSetDefTransient(driver->caps, vm, true) < 0) + goto cleanup; + + vm->def->id = driver->nextvmid++; + + if (virFileMakePath(driver->logDir) != 0) { + virReportSystemError(errno, + _("cannot create log directory %s"), + driver->logDir); + goto cleanup; + }
This doesn't work as virFileMakePath doesn't set errno for all errors, but returns an errno value. Needs to be something like this
int err; if ((err = virFileMakePath(driver->logDir)) != 0) { virReportSystemError(err, _("cannot create log directory %s"), driver->logDir); goto cleanup; }
Also grep'ing for virFileMakePath shows that there are many instances that use virFileMakePath in the wrong way.
THis has since been fixed.
+ if (VIR_ALLOC(priv->monConfig) < 0) { + virReportOOMError(); + goto cleanup; + } + + VIR_DEBUG("Preparing monitor state"); + priv->monConfig = monConfig;
Allocating and overwriting priv->monConfig leaks memory here.
Fixed that too Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (3)
-
Daniel P. Berrange
-
Eric Blake
-
Matthias Bolte