[libvirt] [PATCH 00/11] Allow access to text console with virStream APIs (v3)

An update of the patches in http://www.redhat.com/archives/libvir-list/2010-November/msg00022.html http://www.redhat.com/archives/libvir-list/2010-August/msg00379.html The end goal is to allow 'virsh console' to work unprivileged, and/or over remote connections. This version fixes the problems Eric identified, and also fixes the build of virsh on mingw32, by adding missing thread portability routines. .x-sc_avoid_write | 1 configure.ac | 3 daemon/event.c | 87 +++--- daemon/remote.c | 52 +++ daemon/remote_dispatch_args.h | 1 daemon/remote_dispatch_prototypes.h | 8 daemon/remote_dispatch_table.h | 5 daemon/stream.c | 7 include/libvirt/libvirt.h.in | 6 include/libvirt/virterror.h | 3 src/Makefile.am | 1 src/driver.h | 6 src/esx/esx_driver.c | 1 src/fdstream.c | 504 ++++++++++++++++++++++++++++++++++++ src/fdstream.h | 44 +++ src/libvirt.c | 53 +++ src/libvirt_private.syms | 11 src/libvirt_public.syms | 5 src/lxc/lxc_driver.c | 66 ++++ src/opennebula/one_driver.c | 1 src/openvz/openvz_driver.c | 1 src/phyp/phyp_driver.c | 1 src/qemu/qemu_driver.c | 361 +++++-------------------- src/remote/remote_driver.c | 259 ++++++++++++++++-- src/remote/remote_protocol.c | 13 src/remote/remote_protocol.h | 10 src/remote/remote_protocol.x | 9 src/remote_protocol-structs | 5 src/test/test_driver.c | 1 src/uml/uml_driver.c | 76 +++++ src/util/threads-pthread.c | 45 +++ src/util/threads-pthread.h | 4 src/util/threads-win32.c | 47 +++ src/util/threads-win32.h | 3 src/util/threads.h | 15 + src/util/virterror.c | 3 src/vbox/vbox_tmpl.c | 1 src/xen/xen_driver.c | 58 ++++ src/xenapi/xenapi_driver.c | 1 tools/Makefile.am | 1 tools/console.c | 330 +++++++++++++++++------ tools/console.h | 2 tools/virsh.c | 76 +---- tools/virsh.pod | 7 44 files changed, 1695 insertions(+), 499 deletions(-) Daniel

The current remote driver code for streams only supports blocking I/O mode. This is fine for the usage with migration but is a problem for more general use cases, in particular bi-directional streams. This adds supported for the stream callbacks and non-blocking I/O. with the minor caveat is that it doesn't actually do non-blocking I/O for sending stream data, only receiving it. A future patch will try to do non-blocking sends, but this is quite tricky to get right. * src/remote/remote_driver.c: Allow non-blocking I/O for streams and support callbacks --- src/remote/remote_driver.c | 188 ++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 172 insertions(+), 16 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 1c874b2..61da8ff 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -132,6 +132,13 @@ struct private_stream_data { unsigned int serial; unsigned int proc_nr; + virStreamEventCallback cb; + void *cbOpaque; + virFreeCallback cbFree; + int cbEvents; + int cbTimer; + int cbDispatch; + /* XXX this is potentially unbounded if the client * app has domain events registered, since packets * may be read off wire, while app isn't ready to @@ -200,9 +207,10 @@ struct private_data { }; enum { - REMOTE_CALL_IN_OPEN = (1 << 0), + REMOTE_CALL_IN_OPEN = (1 << 0), REMOTE_CALL_QUIET_MISSING_RPC = (1 << 1), - REMOTE_QEMU_CALL = (1 << 2), + REMOTE_CALL_QEMU = (1 << 2), + REMOTE_CALL_NONBLOCK = (1 << 3), }; @@ -8144,6 +8152,20 @@ remoteStreamOpen(virStreamPtr st, } +static void +remoteStreamEventTimerUpdate(struct private_stream_data *privst) +{ + if (!privst->cb) + return; + + if (!privst->cbEvents) + virEventUpdateTimeout(privst->cbTimer, -1); + else if (privst->incoming && + (privst->cbEvents & VIR_STREAM_EVENT_READABLE)) + virEventUpdateTimeout(privst->cbTimer, 0); +} + + static int remoteStreamPacket(virStreamPtr st, int status, @@ -8338,6 +8360,12 @@ remoteStreamRecv(virStreamPtr st, struct remote_thread_call *thiscall; int ret; + if (st->flags & VIR_STREAM_NONBLOCK) { + DEBUG0("Non-blocking mode and no data available"); + rv = -2; + goto cleanup; + } + if (VIR_ALLOC(thiscall) < 0) { virReportOOMError(); goto cleanup; @@ -8381,6 +8409,8 @@ remoteStreamRecv(virStreamPtr st, rv = 0; } + remoteStreamEventTimerUpdate(privst); + DEBUG("Done %d", rv); cleanup: @@ -8391,28 +8421,153 @@ cleanup: return rv; } + +static void +remoteStreamEventTimer(int timer ATTRIBUTE_UNUSED, void *opaque) +{ + virStreamPtr st = opaque; + struct private_data *priv = st->conn->privateData; + struct private_stream_data *privst = st->privateData; + + remoteDriverLock(priv); + if (privst->cb && + (privst->cbEvents & VIR_STREAM_EVENT_READABLE) && + privst->incomingOffset) { + virStreamEventCallback cb = privst->cb; + void *cbOpaque = privst->cbOpaque; + virFreeCallback cbFree = privst->cbFree; + + privst->cbDispatch = 1; + remoteDriverUnlock(priv); + (cb)(st, VIR_STREAM_EVENT_READABLE, cbOpaque); + remoteDriverLock(priv); + privst->cbDispatch = 0; + + if (!privst->cb && cbFree) + (cbFree)(cbOpaque); + } + remoteDriverUnlock(priv); +} + + +static void +remoteStreamEventTimerFree(void *opaque) +{ + virStreamPtr st = opaque; + virUnrefStream(st); +} + + static int -remoteStreamEventAddCallback(virStreamPtr stream ATTRIBUTE_UNUSED, - int events ATTRIBUTE_UNUSED, - virStreamEventCallback cb ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED, - virFreeCallback ff ATTRIBUTE_UNUSED) +remoteStreamEventAddCallback(virStreamPtr st, + int events, + virStreamEventCallback cb, + void *opaque, + virFreeCallback ff) { - return -1; + struct private_data *priv = st->conn->privateData; + struct private_stream_data *privst = st->privateData; + int ret = -1; + + remoteDriverLock(priv); + + if (events & ~VIR_STREAM_EVENT_READABLE) { + remoteError(VIR_ERR_INTERNAL_ERROR, + _("unsupported stream events %d"), events); + goto cleanup; + } + + if (privst->cb) { + remoteError(VIR_ERR_INTERNAL_ERROR, + _("multiple stream callbacks not supported")); + goto cleanup; + } + + virStreamRef(st); + if ((privst->cbTimer = + virEventAddTimeout(-1, + remoteStreamEventTimer, + st, + remoteStreamEventTimerFree)) < 0) { + virUnrefStream(st); + goto cleanup; + } + + privst->cb = cb; + privst->cbOpaque = opaque; + privst->cbFree = ff; + privst->cbEvents = events; + + ret = 0; + +cleanup: + remoteDriverUnlock(priv); + return ret; } static int -remoteStreamEventUpdateCallback(virStreamPtr stream ATTRIBUTE_UNUSED, - int events ATTRIBUTE_UNUSED) +remoteStreamEventUpdateCallback(virStreamPtr st, + int events) { - return -1; + struct private_data *priv = st->conn->privateData; + struct private_stream_data *privst = st->privateData; + int ret = -1; + + remoteDriverLock(priv); + + if (events & ~VIR_STREAM_EVENT_READABLE) { + remoteError(VIR_ERR_INTERNAL_ERROR, + _("unsupported stream events %d"), events); + goto cleanup; + } + + if (!privst->cb) { + remoteError(VIR_ERR_INTERNAL_ERROR, + _("no stream callback registered")); + goto cleanup; + } + + privst->cbEvents = events; + + remoteStreamEventTimerUpdate(privst); + + ret = 0; + +cleanup: + remoteDriverUnlock(priv); + return ret; } static int -remoteStreamEventRemoveCallback(virStreamPtr stream ATTRIBUTE_UNUSED) +remoteStreamEventRemoveCallback(virStreamPtr st) { - return -1; + struct private_data *priv = st->conn->privateData; + struct private_stream_data *privst = st->privateData; + int ret = -1; + + remoteDriverLock(priv); + + if (!privst->cb) { + remoteError(VIR_ERR_INTERNAL_ERROR, + _("no stream callback registered")); + goto cleanup; + } + + if (!privst->cbDispatch && + privst->cbFree) + (privst->cbFree)(privst->cbOpaque); + privst->cb = NULL; + privst->cbOpaque = NULL; + privst->cbFree = NULL; + privst->cbEvents = 0; + virEventRemoveTimeout(privst->cbTimer); + + ret = 0; + +cleanup: + remoteDriverUnlock(priv); + return ret; } static int @@ -9065,7 +9220,7 @@ remoteQemuDomainMonitorCommand (virDomainPtr domain, const char *cmd, args.flags = flags; memset (&ret, 0, sizeof ret); - if (call (domain->conn, priv, REMOTE_QEMU_CALL, QEMU_PROC_MONITOR_COMMAND, + if (call (domain->conn, priv, REMOTE_CALL_QEMU, QEMU_PROC_MONITOR_COMMAND, (xdrproc_t) xdr_qemu_monitor_command_args, (char *) &args, (xdrproc_t) xdr_qemu_monitor_command_ret, (char *) &ret) == -1) goto done; @@ -9119,7 +9274,7 @@ prepareCall(struct private_data *priv, rv->ret = ret; rv->want_reply = 1; - if (flags & REMOTE_QEMU_CALL) { + if (flags & REMOTE_CALL_QEMU) { hdr.prog = QEMU_PROGRAM; hdr.vers = QEMU_PROTOCOL_VERSION; } @@ -9512,7 +9667,7 @@ processCallDispatch(virConnectPtr conn, struct private_data *priv, expectedprog = REMOTE_PROGRAM; expectedvers = REMOTE_PROTOCOL_VERSION; - if (flags & REMOTE_QEMU_CALL) { + if (flags & REMOTE_CALL_QEMU) { expectedprog = QEMU_PROGRAM; expectedvers = QEMU_PROTOCOL_VERSION; } @@ -9738,6 +9893,7 @@ processCallDispatchStream(virConnectPtr conn ATTRIBUTE_UNUSED, thecall->mode = REMOTE_MODE_COMPLETE; } else { VIR_WARN("Got aysnc data packet offset=%d", privst->incomingOffset); + remoteStreamEventTimerUpdate(privst); } return 0; } -- 1.7.2.3

On Tue, Nov 02, 2010 at 05:49:05PM +0000, Daniel P. Berrange wrote:
The current remote driver code for streams only supports blocking I/O mode. This is fine for the usage with migration but is a problem for more general use cases, in particular bi-directional streams.
This adds supported for the stream callbacks and non-blocking I/O. with the minor caveat is that it doesn't actually do non-blocking I/O for sending stream data, only receiving it. A future patch will try to do non-blocking sends, but this is quite tricky to get right.
* src/remote/remote_driver.c: Allow non-blocking I/O for streams and support callbacks --- src/remote/remote_driver.c | 188 ++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 172 insertions(+), 16 deletions(-)
ACK, looks fine, 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/

To enable virsh console (or equivalent) to be used remotely it is necessary to provide remote access to the /dev/pts/XXX pseudo-TTY associated with the console/serial/parallel device in the guest. The virStream API provide a bi-directional I/O stream capability that can be used for this purpose. This patch thus introduces a virDomainOpenConsole API that uses the stream APIs. * src/libvirt.c, src/libvirt_public.syms, include/libvirt/libvirt.h.in, src/driver.h: Define the new virDomainOpenConsole API * src/esx/esx_driver.c, src/lxc/lxc_driver.c, src/opennebula/one_driver.c, src/openvz/openvz_driver.c, src/phyp/phyp_driver.c, src/qemu/qemu_driver.c, src/remote/remote_driver.c, src/test/test_driver.c, src/uml/uml_driver.c, src/vbox/vbox_tmpl.c, src/xen/xen_driver.c, src/xenapi/xenapi_driver.c: Stub API entry point --- include/libvirt/libvirt.h.in | 6 ++++ src/driver.h | 6 ++++ src/esx/esx_driver.c | 1 + src/libvirt.c | 53 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++++ src/lxc/lxc_driver.c | 1 + src/opennebula/one_driver.c | 1 + src/openvz/openvz_driver.c | 1 + src/phyp/phyp_driver.c | 1 + src/qemu/qemu_driver.c | 1 + src/remote/remote_driver.c | 1 + src/test/test_driver.c | 1 + src/uml/uml_driver.c | 1 + src/vbox/vbox_tmpl.c | 1 + src/xen/xen_driver.c | 1 + src/xenapi/xenapi_driver.c | 1 + 16 files changed, 82 insertions(+), 0 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 81db3a2..cc82e5c 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2400,6 +2400,12 @@ int virNWFilterGetUUIDString (virNWFilterPtr nwfilter, char * virNWFilterGetXMLDesc (virNWFilterPtr nwfilter, int flags); + +int virDomainOpenConsole(virDomainPtr dom, + const char *devname, + virStreamPtr st, + unsigned int flags); + #ifdef __cplusplus } #endif diff --git a/src/driver.h b/src/driver.h index 79a96c1..6417ee9 100644 --- a/src/driver.h +++ b/src/driver.h @@ -480,6 +480,11 @@ typedef int (*virDrvQemuDomainMonitorCommand)(virDomainPtr domain, const char *cmd, char **result, unsigned int flags); +typedef int + (*virDrvDomainOpenConsole)(virDomainPtr dom, + const char *devname, + virStreamPtr st, + unsigned int flags); /** @@ -598,6 +603,7 @@ struct _virDriver { virDrvQemuDomainMonitorCommand qemuDomainMonitorCommand; virDrvDomainSetMemoryParameters domainSetMemoryParameters; virDrvDomainGetMemoryParameters domainGetMemoryParameters; + virDrvDomainOpenConsole domainOpenConsole; }; typedef int diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index b3e1284..861247f 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4253,6 +4253,7 @@ static virDriver esxDriver = { NULL, /* qemuDomainMonitorCommand */ NULL, /* domainSetMemoryParameters */ NULL, /* domainGetMemoryParameters */ + NULL, /* domainOpenConsole */ }; diff --git a/src/libvirt.c b/src/libvirt.c index aebd3bc..eb68377 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -13118,3 +13118,56 @@ virDomainSnapshotFree(virDomainSnapshotPtr snapshot) } return 0; } + +/** + * virDomainOpenConsole: + * @domain: a domain object + * @devname: the console, serial or parallel port device alias, or NULL + * @st: a stream to associate with the console + * @flags: unused, pass 0 + * + * This opens the backend associated with a console, serial or + * parallel port device on a guest, if the backend is supported. + * If the @devname is omitted, then the first console or serial + * device is opened. The console is associated with the passed + * in @st stream, which should have been opened in non-blocking + * mode for bi-directional I/O. + * + * returns 0 if the console was opened, -1 on error + */ +int virDomainOpenConsole(virDomainPtr dom, + const char *devname, + virStreamPtr st, + unsigned int flags) +{ + virConnectPtr conn; + DEBUG("dom=%p devname=%s, st=%p flags=%u", dom, NULLSTR(devname), st, flags); + + virResetLastError(); + + if (!VIR_IS_DOMAIN(dom)) { + virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + conn = dom->conn; + if (conn->flags & VIR_CONNECT_RO) { + virLibDomainError(dom, VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + + if (conn->driver->domainOpenConsole) { + int ret; + ret = conn->driver->domainOpenConsole(dom, devname, st, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(conn); + return -1; +} diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index a8091b1..4ef4c5a 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -413,4 +413,9 @@ LIBVIRT_0.8.5 { virDomainSetVcpusFlags; } LIBVIRT_0.8.2; +LIBVIRT_0.8.6 { + global: + virDomainOpenConsole; +} LIBVIRT_0.8.5; + # .... define new API here using predicted next version number .... diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index d39b60e..48a38d1 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2844,6 +2844,7 @@ static virDriver lxcDriver = { NULL, /* qemuDomainMonitorCommand */ lxcDomainSetMemoryParameters, /* domainSetMemoryParameters */ lxcDomainGetMemoryParameters, /* domainGetMemoryParameters */ + NULL, /* domainOpenConsole */ }; static virStateDriver lxcStateDriver = { diff --git a/src/opennebula/one_driver.c b/src/opennebula/one_driver.c index 199fca3..43a2847 100644 --- a/src/opennebula/one_driver.c +++ b/src/opennebula/one_driver.c @@ -822,6 +822,7 @@ static virDriver oneDriver = { NULL, /* qemuDomainMonitorCommand */ NULL, /* domainSetMemoryParameters */ NULL, /* domainGetMemoryParameters */ + NULL, /* domainOpenConsole */ }; static virStateDriver oneStateDriver = { diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index b7c2754..e2fb281 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -1690,6 +1690,7 @@ static virDriver openvzDriver = { NULL, /* qemuDomainMonitorCommand */ NULL, /* domainSetMemoryParameters */ NULL, /* domainGetMemoryParameters */ + NULL, /* domainOpenConsole */ }; int openvzRegister(void) { diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 3d0ed11..bf55581 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -4036,6 +4036,7 @@ static virDriver phypDriver = { NULL, /* qemuMonitorCommand */ NULL, /* domainSetMemoryParameters */ NULL, /* domainGetMemoryParameters */ + NULL, /* domainOpenConsole */ }; static virStorageDriver phypStorageDriver = { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4e590e3..d70ae8b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13409,6 +13409,7 @@ static virDriver qemuDriver = { qemuDomainMonitorCommand, /* qemuDomainMonitorCommand */ qemuDomainSetMemoryParameters, /* domainSetMemoryParameters */ qemuDomainGetMemoryParameters, /* domainGetMemoryParameters */ + NULL, /* domainOpenConsole */ }; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 61da8ff..2ad5ef7 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -10697,6 +10697,7 @@ static virDriver remote_driver = { remoteQemuDomainMonitorCommand, /* qemuDomainMonitorCommand */ remoteDomainSetMemoryParameters, /* domainSetMemoryParameters */ remoteDomainGetMemoryParameters, /* domainGetMemoryParameters */ + NULL, /* domainOpenConsole */ }; static virNetworkDriver network_driver = { diff --git a/src/test/test_driver.c b/src/test/test_driver.c index a9d3d89..23f79a5 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -5447,6 +5447,7 @@ static virDriver testDriver = { NULL, /* qemuDomainMonitorCommand */ NULL, /* domainSetMemoryParameters */ NULL, /* domainGetMemoryParameters */ + NULL, /* domainOpenConsole */ }; static virNetworkDriver testNetworkDriver = { diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 5161012..e0bb4e5 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -2200,6 +2200,7 @@ static virDriver umlDriver = { NULL, /* qemuDomainMonitorCommand */ NULL, /* domainSetMemoryParamters */ NULL, /* domainGetMemoryParamters */ + NULL, /* domainOpenConsole */ }; static int diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index ddbca97..78f945c 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -8464,6 +8464,7 @@ virDriver NAME(Driver) = { NULL, /* qemuDomainMonitorCommand */ NULL, /* domainSetMemoryParameters */ NULL, /* domainGetMemoryParameters */ + NULL, /* domainOpenConsole */ }; virNetworkDriver NAME(NetworkDriver) = { diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 66e8518..9cc46ae 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -2087,6 +2087,7 @@ static virDriver xenUnifiedDriver = { NULL, /* qemuDomainMonitorCommand */ NULL, /* domainSetMemoryParameters */ NULL, /* domainGetMemoryParameters */ + NULL, /* domainOpenConsole */ }; /** diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index 5ccdede..03b0a6a 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -1869,6 +1869,7 @@ static virDriver xenapiDriver = { NULL, /* qemuDomainMonitorCommand */ NULL, /* domainSetMemoryParameters */ NULL, /* domainGetMemoryParameters */ + NULL, /* domainOpenConsole */ }; /** -- 1.7.2.3

On Tue, Nov 02, 2010 at 05:49:06PM +0000, Daniel P. Berrange wrote:
To enable virsh console (or equivalent) to be used remotely it is necessary to provide remote access to the /dev/pts/XXX pseudo-TTY associated with the console/serial/parallel device in the guest. The virStream API provide a bi-directional I/O stream capability that can be used for this purpose. This patch thus introduces a virDomainOpenConsole API that uses the stream APIs.
* src/libvirt.c, src/libvirt_public.syms, include/libvirt/libvirt.h.in, src/driver.h: Define the new virDomainOpenConsole API * src/esx/esx_driver.c, src/lxc/lxc_driver.c, src/opennebula/one_driver.c, src/openvz/openvz_driver.c, src/phyp/phyp_driver.c, src/qemu/qemu_driver.c, src/remote/remote_driver.c, src/test/test_driver.c, src/uml/uml_driver.c, src/vbox/vbox_tmpl.c, src/xen/xen_driver.c, src/xenapi/xenapi_driver.c: Stub API entry point --- include/libvirt/libvirt.h.in | 6 ++++ src/driver.h | 6 ++++ src/esx/esx_driver.c | 1 + src/libvirt.c | 53 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++++ src/lxc/lxc_driver.c | 1 + src/opennebula/one_driver.c | 1 + src/openvz/openvz_driver.c | 1 + src/phyp/phyp_driver.c | 1 + src/qemu/qemu_driver.c | 1 + src/remote/remote_driver.c | 1 + src/test/test_driver.c | 1 + src/uml/uml_driver.c | 1 + src/vbox/vbox_tmpl.c | 1 + src/xen/xen_driver.c | 1 + src/xenapi/xenapi_driver.c | 1 + 16 files changed, 82 insertions(+), 0 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 81db3a2..cc82e5c 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2400,6 +2400,12 @@ int virNWFilterGetUUIDString (virNWFilterPtr nwfilter, char * virNWFilterGetXMLDesc (virNWFilterPtr nwfilter, int flags);
+ +int virDomainOpenConsole(virDomainPtr dom, + const char *devname, + virStreamPtr st, + unsigned int flags); +
Okay, looks fine !
+/** + * virDomainOpenConsole: + * @domain: a domain object
should be @dom: to get API extraction right
+ * @devname: the console, serial or parallel port device alias, or NULL + * @st: a stream to associate with the console + * @flags: unused, pass 0 + * + * This opens the backend associated with a console, serial or + * parallel port device on a guest, if the backend is supported. + * If the @devname is omitted, then the first console or serial + * device is opened. The console is associated with the passed + * in @st stream, which should have been opened in non-blocking + * mode for bi-directional I/O. + * + * returns 0 if the console was opened, -1 on error + */ +int virDomainOpenConsole(virDomainPtr dom, + const char *devname, + virStreamPtr st, + unsigned int flags) +{ + virConnectPtr conn; + DEBUG("dom=%p devname=%s, st=%p flags=%u", dom, NULLSTR(devname), st, flags); + + virResetLastError(); + + if (!VIR_IS_DOMAIN(dom)) { + virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + conn = dom->conn; + if (conn->flags & VIR_CONNECT_RO) { + virLibDomainError(dom, VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + + if (conn->driver->domainOpenConsole) { + int ret; + ret = conn->driver->domainOpenConsole(dom, devname, st, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(conn); + return -1; +} diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index a8091b1..4ef4c5a 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -413,4 +413,9 @@ LIBVIRT_0.8.5 { virDomainSetVcpusFlags; } LIBVIRT_0.8.2;
+LIBVIRT_0.8.6 { + global: + virDomainOpenConsole; +} LIBVIRT_0.8.5; +
ACK, but just fix that minor comment first 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/

This provides an implementation of the virDomainOpenConsole API for the remote driver client and server. * daemon/remote.c: Server side impl * src/remote/remote_driver.c: Client impl * src/remote/remote_protocol.x: Wire definition --- daemon/remote.c | 52 ++++++++++++++++++++++ daemon/remote_dispatch_args.h | 1 + daemon/remote_dispatch_prototypes.h | 8 +++ daemon/remote_dispatch_table.h | 5 ++ src/remote/remote_driver.c | 82 +++++++++++++++++++++++++++-------- src/remote/remote_protocol.c | 13 ++++++ src/remote/remote_protocol.h | 10 ++++ src/remote/remote_protocol.x | 9 +++- src/remote_protocol-structs | 5 ++ 9 files changed, 165 insertions(+), 20 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 886d53d..c3e6c3a 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -6906,6 +6906,58 @@ qemuDispatchMonitorCommand (struct qemud_server *server ATTRIBUTE_UNUSED, } +static int +remoteDispatchDomainOpenConsole(struct qemud_server *server ATTRIBUTE_UNUSED, + struct qemud_client *client, + virConnectPtr conn, + remote_message_header *hdr, + remote_error *rerr, + remote_domain_open_console_args *args, + void *ret ATTRIBUTE_UNUSED) +{ + int r; + struct qemud_client_stream *stream; + virDomainPtr dom; + + CHECK_CONN (client); + + dom = get_nonnull_domain (conn, args->domain); + if (dom == NULL) { + remoteDispatchConnError(rerr, conn); + return -1; + } + + stream = remoteCreateClientStream(conn, hdr); + if (!stream) { + virDomainFree(dom); + remoteDispatchOOMError(rerr); + return -1; + } + + r = virDomainOpenConsole(dom, + args->devname ? *args->devname : NULL, + stream->st, + args->flags); + if (r == -1) { + virDomainFree(dom); + remoteFreeClientStream(client, stream); + remoteDispatchConnError(rerr, conn); + return -1; + } + + if (remoteAddClientStream(client, stream, 1) < 0) { + virDomainFree(dom); + remoteDispatchConnError(rerr, conn); + virStreamAbort(stream->st); + remoteFreeClientStream(client, stream); + return -1; + } + + virDomainFree(dom); + return 0; +} + + /*----- Helpers. -----*/ /* get_nonnull_domain and get_nonnull_network turn an on-wire diff --git a/daemon/remote_dispatch_args.h b/daemon/remote_dispatch_args.h index 9583e9c..971af80 100644 --- a/daemon/remote_dispatch_args.h +++ b/daemon/remote_dispatch_args.h @@ -169,3 +169,4 @@ remote_domain_get_memory_parameters_args val_remote_domain_get_memory_parameters_args; remote_domain_set_vcpus_flags_args val_remote_domain_set_vcpus_flags_args; remote_domain_get_vcpus_flags_args val_remote_domain_get_vcpus_flags_args; + remote_domain_open_console_args val_remote_domain_open_console_args; diff --git a/daemon/remote_dispatch_prototypes.h b/daemon/remote_dispatch_prototypes.h index 6b35851..15c7ec7 100644 --- a/daemon/remote_dispatch_prototypes.h +++ b/daemon/remote_dispatch_prototypes.h @@ -466,6 +466,14 @@ static int remoteDispatchDomainMigrateSetMaxDowntime( remote_error *err, remote_domain_migrate_set_max_downtime_args *args, void *ret); +static int remoteDispatchDomainOpenConsole( + struct qemud_server *server, + struct qemud_client *client, + virConnectPtr conn, + remote_message_header *hdr, + remote_error *err, + remote_domain_open_console_args *args, + void *ret); static int remoteDispatchDomainPinVcpu( struct qemud_server *server, struct qemud_client *client, diff --git a/daemon/remote_dispatch_table.h b/daemon/remote_dispatch_table.h index dd2adc7..4cfa1b1 100644 --- a/daemon/remote_dispatch_table.h +++ b/daemon/remote_dispatch_table.h @@ -1007,3 +1007,8 @@ .args_filter = (xdrproc_t) xdr_remote_domain_get_vcpus_flags_args, .ret_filter = (xdrproc_t) xdr_remote_domain_get_vcpus_flags_ret, }, +{ /* DomainOpenConsole => 201 */ + .fn = (dispatch_fn) remoteDispatchDomainOpenConsole, + .args_filter = (xdrproc_t) xdr_remote_domain_open_console_args, + .ret_filter = (xdrproc_t) xdr_void, +}, diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 2ad5ef7..57aec96 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8158,11 +8158,16 @@ remoteStreamEventTimerUpdate(struct private_stream_data *privst) if (!privst->cb) return; - if (!privst->cbEvents) - virEventUpdateTimeout(privst->cbTimer, -1); - else if (privst->incoming && - (privst->cbEvents & VIR_STREAM_EVENT_READABLE)) + VIR_DEBUG("Check timer offset=%d %d", privst->incomingOffset, privst->cbEvents); + if ((privst->incomingOffset && + (privst->cbEvents & VIR_STREAM_EVENT_READABLE)) || + (privst->cbEvents & VIR_STREAM_EVENT_WRITABLE)) { + VIR_DEBUG0("Enabling event timer"); virEventUpdateTimeout(privst->cbTimer, 0); + } else { + VIR_DEBUG0("Disabling event timer"); + virEventUpdateTimeout(privst->cbTimer, -1); + } } @@ -8428,24 +8433,33 @@ remoteStreamEventTimer(int timer ATTRIBUTE_UNUSED, void *opaque) virStreamPtr st = opaque; struct private_data *priv = st->conn->privateData; struct private_stream_data *privst = st->privateData; + int events = 0; remoteDriverLock(priv); + if (privst->cb && (privst->cbEvents & VIR_STREAM_EVENT_READABLE) && - privst->incomingOffset) { + privst->incomingOffset) + events |= VIR_STREAM_EVENT_READABLE; + if (privst->cb && + (privst->cbEvents & VIR_STREAM_EVENT_WRITABLE)) + events |= VIR_STREAM_EVENT_WRITABLE; + VIR_DEBUG("Got Timer dispatch %d %d offset=%d", events, privst->cbEvents, privst->incomingOffset); + if (events) { virStreamEventCallback cb = privst->cb; void *cbOpaque = privst->cbOpaque; virFreeCallback cbFree = privst->cbFree; privst->cbDispatch = 1; remoteDriverUnlock(priv); - (cb)(st, VIR_STREAM_EVENT_READABLE, cbOpaque); + (cb)(st, events, cbOpaque); remoteDriverLock(priv); privst->cbDispatch = 0; if (!privst->cb && cbFree) (cbFree)(cbOpaque); } + remoteDriverUnlock(priv); } @@ -8471,12 +8485,6 @@ remoteStreamEventAddCallback(virStreamPtr st, remoteDriverLock(priv); - if (events & ~VIR_STREAM_EVENT_READABLE) { - remoteError(VIR_ERR_INTERNAL_ERROR, - _("unsupported stream events %d"), events); - goto cleanup; - } - if (privst->cb) { remoteError(VIR_ERR_INTERNAL_ERROR, _("multiple stream callbacks not supported")); @@ -8498,6 +8506,8 @@ remoteStreamEventAddCallback(virStreamPtr st, privst->cbFree = ff; privst->cbEvents = events; + remoteStreamEventTimerUpdate(privst); + ret = 0; cleanup: @@ -8515,12 +8525,6 @@ remoteStreamEventUpdateCallback(virStreamPtr st, remoteDriverLock(priv); - if (events & ~VIR_STREAM_EVENT_READABLE) { - remoteError(VIR_ERR_INTERNAL_ERROR, - _("unsupported stream events %d"), events); - goto cleanup; - } - if (!privst->cb) { remoteError(VIR_ERR_INTERNAL_ERROR, _("no stream callback registered")); @@ -9202,6 +9206,46 @@ done: } +static int +remoteDomainOpenConsole(virDomainPtr dom, + const char *devname, + virStreamPtr st, + unsigned int flags) +{ + struct private_data *priv = dom->conn->privateData; + struct private_stream_data *privst = NULL; + int rv = -1; + remote_domain_open_console_args args; + + remoteDriverLock(priv); + + if (!(privst = remoteStreamOpen(st, 1, REMOTE_PROC_DOMAIN_OPEN_CONSOLE, priv->counter))) + goto done; + + st->driver = &remoteStreamDrv; + st->privateData = privst; + + make_nonnull_domain (&args.domain, dom); + args.devname = devname ? (char **)&devname : NULL; + args.flags = flags; + + if (call(dom->conn, priv, 0, REMOTE_PROC_DOMAIN_OPEN_CONSOLE, + (xdrproc_t) xdr_remote_domain_open_console_args, (char *) &args, + (xdrproc_t) xdr_void, NULL) == -1) { + remoteStreamRelease(st); + goto done; + } + + rv = 0; + +done: + remoteDriverUnlock(priv); + + return rv; + +} + + /*----------------------------------------------------------------------*/ static int @@ -10697,7 +10741,7 @@ static virDriver remote_driver = { remoteQemuDomainMonitorCommand, /* qemuDomainMonitorCommand */ remoteDomainSetMemoryParameters, /* domainSetMemoryParameters */ remoteDomainGetMemoryParameters, /* domainGetMemoryParameters */ - NULL, /* domainOpenConsole */ + remoteDomainOpenConsole, /* domainOpenConsole */ }; static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.c b/src/remote/remote_protocol.c index 38ea050..41f5e7d 100644 --- a/src/remote/remote_protocol.c +++ b/src/remote/remote_protocol.c @@ -3709,6 +3709,19 @@ xdr_remote_domain_snapshot_delete_args (XDR *xdrs, remote_domain_snapshot_delete } bool_t +xdr_remote_domain_open_console_args (XDR *xdrs, remote_domain_open_console_args *objp) +{ + + if (!xdr_remote_nonnull_domain (xdrs, &objp->domain)) + return FALSE; + if (!xdr_remote_string (xdrs, &objp->devname)) + return FALSE; + if (!xdr_u_int (xdrs, &objp->flags)) + return FALSE; + return TRUE; +} + +bool_t xdr_remote_procedure (XDR *xdrs, remote_procedure *objp) { diff --git a/src/remote/remote_protocol.h b/src/remote/remote_protocol.h index d75e76c..8dc89a5 100644 --- a/src/remote/remote_protocol.h +++ b/src/remote/remote_protocol.h @@ -2097,6 +2097,13 @@ struct remote_domain_snapshot_delete_args { int flags; }; typedef struct remote_domain_snapshot_delete_args remote_domain_snapshot_delete_args; + +struct remote_domain_open_console_args { + remote_nonnull_domain domain; + remote_string devname; + u_int flags; +}; +typedef struct remote_domain_open_console_args remote_domain_open_console_args; #define REMOTE_PROGRAM 0x20008086 #define REMOTE_PROTOCOL_VERSION 1 @@ -2301,6 +2308,7 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_GET_MEMORY_PARAMETERS = 198, REMOTE_PROC_DOMAIN_SET_VCPUS_FLAGS = 199, REMOTE_PROC_DOMAIN_GET_VCPUS_FLAGS = 200, + REMOTE_PROC_DOMAIN_OPEN_CONSOLE = 201, }; typedef enum remote_procedure remote_procedure; @@ -2670,6 +2678,7 @@ extern bool_t xdr_remote_domain_snapshot_current_args (XDR *, remote_domain_sna extern bool_t xdr_remote_domain_snapshot_current_ret (XDR *, remote_domain_snapshot_current_ret*); extern bool_t xdr_remote_domain_revert_to_snapshot_args (XDR *, remote_domain_revert_to_snapshot_args*); extern bool_t xdr_remote_domain_snapshot_delete_args (XDR *, remote_domain_snapshot_delete_args*); +extern bool_t xdr_remote_domain_open_console_args (XDR *, remote_domain_open_console_args*); extern bool_t xdr_remote_procedure (XDR *, remote_procedure*); extern bool_t xdr_remote_message_type (XDR *, remote_message_type*); extern bool_t xdr_remote_message_status (XDR *, remote_message_status*); @@ -3013,6 +3022,7 @@ extern bool_t xdr_remote_domain_snapshot_current_args (); extern bool_t xdr_remote_domain_snapshot_current_ret (); extern bool_t xdr_remote_domain_revert_to_snapshot_args (); extern bool_t xdr_remote_domain_snapshot_delete_args (); +extern bool_t xdr_remote_domain_open_console_args (); extern bool_t xdr_remote_procedure (); extern bool_t xdr_remote_message_type (); extern bool_t xdr_remote_message_status (); diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index d57e6d0..e84afe5 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1853,6 +1853,11 @@ struct remote_domain_snapshot_delete_args { int flags; }; +struct remote_domain_open_console_args { + remote_nonnull_domain domain; + remote_string devname; + unsigned int flags; +}; /*----- Protocol. -----*/ @@ -2079,7 +2084,9 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_SET_MEMORY_PARAMETERS = 197, REMOTE_PROC_DOMAIN_GET_MEMORY_PARAMETERS = 198, REMOTE_PROC_DOMAIN_SET_VCPUS_FLAGS = 199, - REMOTE_PROC_DOMAIN_GET_VCPUS_FLAGS = 200 + REMOTE_PROC_DOMAIN_GET_VCPUS_FLAGS = 200, + + REMOTE_PROC_DOMAIN_OPEN_CONSOLE = 201 /* * Notice how the entries are grouped in sets of 10 ? diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index d505886..3054bbf 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1352,6 +1352,11 @@ struct remote_domain_snapshot_delete_args { remote_nonnull_domain_snapshot snap; int flags; }; +struct remote_domain_open_console_args { + remote_nonnull_domain domain; + remote_string devname; + u_int flags; +}; struct remote_message_header { u_int prog; u_int vers; -- 1.7.2.3

On Tue, Nov 02, 2010 at 05:49:07PM +0000, Daniel P. Berrange wrote:
This provides an implementation of the virDomainOpenConsole API for the remote driver client and server.
* daemon/remote.c: Server side impl * src/remote/remote_driver.c: Client impl * src/remote/remote_protocol.x: Wire definition --- daemon/remote.c | 52 ++++++++++++++++++++++ daemon/remote_dispatch_args.h | 1 + daemon/remote_dispatch_prototypes.h | 8 +++ daemon/remote_dispatch_table.h | 5 ++ src/remote/remote_driver.c | 82 +++++++++++++++++++++++++++-------- src/remote/remote_protocol.c | 13 ++++++ src/remote/remote_protocol.h | 10 ++++ src/remote/remote_protocol.x | 9 +++- src/remote_protocol-structs | 5 ++ 9 files changed, 165 insertions(+), 20 deletions(-)
Okay, ACK My main question is actually what happen if the guest emitsa lot of console data (let say some program is really flooding the pipe) and the remote client doesn't consume the stream. virStream is lossless as far as I understand and there will be a limit to the buffering on the file descriptor and the network pipe, so as a result the guest may stall (assuming a device coming from QEmu, the qemu process will block on the console I/O), right ? It would be an interesting experiment to try ... 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, Nov 03, 2010 at 12:07:07PM +0100, Daniel Veillard wrote:
On Tue, Nov 02, 2010 at 05:49:07PM +0000, Daniel P. Berrange wrote:
This provides an implementation of the virDomainOpenConsole API for the remote driver client and server.
* daemon/remote.c: Server side impl * src/remote/remote_driver.c: Client impl * src/remote/remote_protocol.x: Wire definition --- daemon/remote.c | 52 ++++++++++++++++++++++ daemon/remote_dispatch_args.h | 1 + daemon/remote_dispatch_prototypes.h | 8 +++ daemon/remote_dispatch_table.h | 5 ++ src/remote/remote_driver.c | 82 +++++++++++++++++++++++++++-------- src/remote/remote_protocol.c | 13 ++++++ src/remote/remote_protocol.h | 10 ++++ src/remote/remote_protocol.x | 9 +++- src/remote_protocol-structs | 5 ++ 9 files changed, 165 insertions(+), 20 deletions(-)
Okay, ACK My main question is actually what happen if the guest emitsa lot of console data (let say some program is really flooding the pipe) and the remote client doesn't consume the stream. virStream is lossless as far as I understand and there will be a limit to the buffering on the file descriptor and the network pipe, so as a result the guest may stall (assuming a device coming from QEmu, the qemu process will block on the console I/O), right ? It would be an interesting experiment to try ...
There's two questions to answer here really If the libvirt client doesn't consume from the libvirtd server quickly enough, then libvirtd will stop consuming frm QEMU. Basically libvirtd will only allow 1 pending outbound stream packet for transmission at a time (max 256 kb data). So libvirtd is safe from unbounded mem usage in buffering here. The behaviour of QEMU i think depends on the device. A virtio console should block the guest OS from writing data. IIC, a serial device will either loose data, or block the guest, depending on whether flow control is properly used in the guest. If a libvirt application doesn't consume data from the libvirt client quickly enough, the memory usage *can* balloon up unbounded, because the virStream on the libvirt client doesn't do any throttling when reading data from libvirtd. This could be addressed later, but since it is client side, I'm not too concerned. 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 Wed, Nov 03, 2010 at 11:19:34AM +0000, Daniel P. Berrange wrote:
On Wed, Nov 03, 2010 at 12:07:07PM +0100, Daniel Veillard wrote:
On Tue, Nov 02, 2010 at 05:49:07PM +0000, Daniel P. Berrange wrote:
This provides an implementation of the virDomainOpenConsole API for the remote driver client and server.
* daemon/remote.c: Server side impl * src/remote/remote_driver.c: Client impl * src/remote/remote_protocol.x: Wire definition --- daemon/remote.c | 52 ++++++++++++++++++++++ daemon/remote_dispatch_args.h | 1 + daemon/remote_dispatch_prototypes.h | 8 +++ daemon/remote_dispatch_table.h | 5 ++ src/remote/remote_driver.c | 82 +++++++++++++++++++++++++++-------- src/remote/remote_protocol.c | 13 ++++++ src/remote/remote_protocol.h | 10 ++++ src/remote/remote_protocol.x | 9 +++- src/remote_protocol-structs | 5 ++ 9 files changed, 165 insertions(+), 20 deletions(-)
Okay, ACK My main question is actually what happen if the guest emitsa lot of console data (let say some program is really flooding the pipe) and the remote client doesn't consume the stream. virStream is lossless as far as I understand and there will be a limit to the buffering on the file descriptor and the network pipe, so as a result the guest may stall (assuming a device coming from QEmu, the qemu process will block on the console I/O), right ? It would be an interesting experiment to try ...
There's two questions to answer here really
right :-)
If the libvirt client doesn't consume from the libvirtd server quickly enough, then libvirtd will stop consuming frm QEMU. Basically libvirtd will only allow 1 pending outbound stream packet for transmission at a time (max 256 kb data). So libvirtd is safe from unbounded mem usage in buffering here. The behaviour of QEMU i think depends on the device. A virtio console should block the guest OS from writing data. IIC, a serial device will either loose data, or block the guest, depending on whether flow control is properly used in the guest.
Okay, we can't make assumptions on the producer behaviour.
If a libvirt application doesn't consume data from the libvirt client quickly enough, the memory usage *can* balloon up unbounded, because the virStream on the libvirt client doesn't do any throttling when reading data from libvirtd. This could be addressed later, but since it is client side, I'm not too concerned.
We could add a virStream opening extra flag to limit buffering, but not urgent. 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/

Now that bi-directional, non-blocking streams are supported in the remote driver, some of the VIR_WARN statements need to be reduced to VIR_DEBUG. * src/remote/remote_driver.c: Lower logging level --- src/remote/remote_driver.c | 26 +++++++++++++------------- 1 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 57aec96..0ec8c39 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8273,7 +8273,7 @@ remoteStreamHasError(virStreamPtr st) { return 0; } - VIR_WARN0("Raising async error"); + VIR_DEBUG0("Raising async error"); virRaiseErrorFull(st->conn, __FILE__, __FUNCTION__, __LINE__, privst->err.domain, @@ -9893,8 +9893,8 @@ processCallDispatchStream(virConnectPtr conn ATTRIBUTE_UNUSED, privst = privst->next; if (!privst) { - VIR_WARN("No registered stream matching serial=%d, proc=%d", - hdr->serial, hdr->proc); + VIR_DEBUG("No registered stream matching serial=%d, proc=%d", + hdr->serial, hdr->proc); return -1; } @@ -9913,7 +9913,7 @@ processCallDispatchStream(virConnectPtr conn ATTRIBUTE_UNUSED, case REMOTE_CONTINUE: { int avail = privst->incomingLength - privst->incomingOffset; int need = priv->bufferLength - priv->bufferOffset; - VIR_WARN0("Got a stream data packet"); + VIR_DEBUG0("Got a stream data packet"); /* XXX flag stream as complete somwhere if need==0 */ @@ -9921,7 +9921,7 @@ processCallDispatchStream(virConnectPtr conn ATTRIBUTE_UNUSED, int extra = need - avail; if (VIR_REALLOC_N(privst->incoming, privst->incomingLength + extra) < 0) { - VIR_WARN0("Out of memory"); + VIR_DEBUG0("Out of memory handling stream data"); return -1; } privst->incomingLength += extra; @@ -9933,19 +9933,19 @@ processCallDispatchStream(virConnectPtr conn ATTRIBUTE_UNUSED, privst->incomingOffset += (priv->bufferLength - priv->bufferOffset); if (thecall && thecall->want_reply) { - VIR_WARN("Got sync data packet offset=%d", privst->incomingOffset); + VIR_DEBUG("Got sync data packet offset=%d", privst->incomingOffset); thecall->mode = REMOTE_MODE_COMPLETE; } else { - VIR_WARN("Got aysnc data packet offset=%d", privst->incomingOffset); + VIR_DEBUG("Got aysnc data packet offset=%d", privst->incomingOffset); remoteStreamEventTimerUpdate(privst); } return 0; } case REMOTE_OK: - VIR_WARN0("Got a synchronous confirm"); + VIR_DEBUG0("Got a synchronous confirm"); if (!thecall) { - VIR_WARN0("Got unexpected stream finish confirmation"); + VIR_DEBUG0("Got unexpected stream finish confirmation"); return -1; } thecall->mode = REMOTE_MODE_COMPLETE; @@ -9953,7 +9953,7 @@ processCallDispatchStream(virConnectPtr conn ATTRIBUTE_UNUSED, case REMOTE_ERROR: if (thecall && thecall->want_reply) { - VIR_WARN0("Got a synchronous error"); + VIR_DEBUG0("Got a synchronous error"); /* Give the error straight to this call */ memset (&thecall->err, 0, sizeof thecall->err); if (!xdr_remote_error (xdr, &thecall->err)) { @@ -9962,16 +9962,16 @@ processCallDispatchStream(virConnectPtr conn ATTRIBUTE_UNUSED, } thecall->mode = REMOTE_MODE_ERROR; } else { - VIR_WARN0("Got a asynchronous error"); + VIR_DEBUG0("Got a asynchronous error"); /* No call, so queue the error against the stream */ if (privst->has_error) { - VIR_WARN0("Got unexpected duplicate stream error"); + VIR_DEBUG0("Got unexpected duplicate stream error"); return -1; } privst->has_error = 1; memset (&privst->err, 0, sizeof privst->err); if (!xdr_remote_error (xdr, &privst->err)) { - VIR_WARN0("Failed to unmarshall error"); + VIR_DEBUG0("Failed to unmarshall error"); return -1; } } -- 1.7.2.3

On Tue, Nov 02, 2010 at 05:49:08PM +0000, Daniel P. Berrange wrote:
Now that bi-directional, non-blocking streams are supported in the remote driver, some of the VIR_WARN statements need to be reduced to VIR_DEBUG.
* src/remote/remote_driver.c: Lower logging level --- src/remote/remote_driver.c | 26 +++++++++++++------------- 1 files changed, 13 insertions(+), 13 deletions(-)
ACK, 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/

To avoid the need for duplicating implementations of virStream drivers, provide a generic implementation that can handle any FD based stream. This code is copied from the existing impl in the QEMU driver, with the locking moved into the stream impl, and addition of a read callback The FD stream code will refuse to operate on regular files or block devices, since those can't report EAGAIN properly when they would block on I/O * include/libvirt/virterror.h, include/libvirt/virterror.h: Add VIR_FROM_STREAM error domain * src/qemu/qemu_driver.c: Remove code obsoleted by the new generic streams driver. * src/fdstream.h, src/fdstream.c, src/fdstream.c, src/libvirt_private.syms: Generic reusable FD based streams --- configure.ac | 3 +- include/libvirt/virterror.h | 3 +- src/Makefile.am | 1 + src/fdstream.c | 504 +++++++++++++++++++++++++++++++++++++++++++ src/fdstream.h | 44 ++++ src/libvirt_private.syms | 7 + src/qemu/qemu_driver.c | 284 +------------------------ src/util/virterror.c | 3 + 8 files changed, 568 insertions(+), 281 deletions(-) create mode 100644 src/fdstream.c create mode 100644 src/fdstream.h diff --git a/configure.ac b/configure.ac index 6b30d90..8f55992 100644 --- a/configure.ac +++ b/configure.ac @@ -109,7 +109,8 @@ LIBS=$old_libs dnl Availability of various common headers (non-fatal if missing). AC_CHECK_HEADERS([pwd.h paths.h regex.h sys/syslimits.h sys/un.h \ - sys/poll.h syslog.h mntent.h net/ethernet.h linux/magic.h]) + sys/poll.h syslog.h mntent.h net/ethernet.h linux/magic.h \ + sys/un.h]) AC_CHECK_LIB([intl],[gettext],[]) diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index 94d686c..abf6945 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -73,7 +73,8 @@ typedef enum { VIR_FROM_NWFILTER, /* Error from network filter driver */ VIR_FROM_HOOK, /* Error from Synchronous hooks */ VIR_FROM_DOMAIN_SNAPSHOT, /* Error from domain snapshot */ - VIR_FROM_AUDIT /* Error from auditing subsystem */ + VIR_FROM_AUDIT, /* Error from auditing subsystem */ + VIR_FROM_STREAMS, /* Error from I/O streams */ } virErrorDomain; diff --git a/src/Makefile.am b/src/Makefile.am index 4a11c37..2022b85 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -94,6 +94,7 @@ DRIVER_SOURCES = \ driver.c driver.h \ internal.h \ datatypes.c datatypes.h \ + fdstream.c fdstream.h \ $(NODE_INFO_SOURCES) \ libvirt.c libvirt_internal.h diff --git a/src/fdstream.c b/src/fdstream.c new file mode 100644 index 0000000..4325053 --- /dev/null +++ b/src/fdstream.c @@ -0,0 +1,504 @@ +/* + * fdstream.h: generic streams impl for file descriptors + * + * Copyright (C) 2009-2010 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + */ + +#include <config.h> + +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <unistd.h> +#include <sys/socket.h> +#if HAVE_SYS_UN_H +#include <sys/un.h> +#endif +#include <netinet/in.h> + +#include "fdstream.h" +#include "virterror_internal.h" +#include "datatypes.h" +#include "memory.h" +#include "event.h" +#include "util.h" +#include "files.h" + +#define VIR_FROM_THIS VIR_FROM_STREAMS +#define streamsReportError(code, ...) \ + virReportErrorHelper(NULL, VIR_FROM_THIS, code, __FILE__, \ + __FUNCTION__, __LINE__, __VA_ARGS__) + +/* Tunnelled migration stream support */ +struct virFDStreamData { + int fd; + + int watch; + unsigned int cbRemoved; + unsigned int dispatching; + virStreamEventCallback cb; + void *opaque; + virFreeCallback ff; + + virMutex lock; +}; + +static int virFDStreamRemoveCallback(virStreamPtr stream) +{ + struct virFDStreamData *fdst = stream->privateData; + int ret = -1; + + if (!fdst) { + streamsReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("stream is not open")); + return -1; + } + + virMutexLock(&fdst->lock); + if (fdst->watch == 0) { + streamsReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("stream does not have a callback registered")); + goto cleanup; + } + + virEventRemoveHandle(fdst->watch); + if (fdst->dispatching) + fdst->cbRemoved = 1; + else if (fdst->ff) + (fdst->ff)(fdst->opaque); + + fdst->watch = 0; + fdst->ff = NULL; + fdst->cb = NULL; + fdst->opaque = NULL; + + ret = 0; + +cleanup: + virMutexUnlock(&fdst->lock); + return ret; +} + +static int virFDStreamUpdateCallback(virStreamPtr stream, int events) +{ + struct virFDStreamData *fdst = stream->privateData; + int ret = -1; + + if (!fdst) { + streamsReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("stream is not open")); + return -1; + } + + virMutexLock(&fdst->lock); + if (fdst->watch == 0) { + streamsReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("stream does not have a callback registered")); + goto cleanup; + } + + virEventUpdateHandle(fdst->watch, events); + + ret = 0; + +cleanup: + virMutexUnlock(&fdst->lock); + return ret; +} + +static void virFDStreamEvent(int watch ATTRIBUTE_UNUSED, + int fd ATTRIBUTE_UNUSED, + int events, + void *opaque) +{ + virStreamPtr stream = opaque; + struct virFDStreamData *fdst = stream->privateData; + virStreamEventCallback cb; + void *cbopaque; + virFreeCallback ff; + + if (!fdst) + return; + + virMutexLock(&fdst->lock); + if (!fdst->cb) { + virMutexUnlock(&fdst->lock); + return; + } + + cb = fdst->cb; + cbopaque = fdst->opaque; + ff = fdst->ff; + fdst->dispatching = 1; + virMutexUnlock(&fdst->lock); + + cb(stream, events, cbopaque); + + virMutexLock(&fdst->lock); + fdst->dispatching = 0; + if (fdst->cbRemoved && ff) + (ff)(cbopaque); + virMutexUnlock(&fdst->lock); +} + +static int +virFDStreamAddCallback(virStreamPtr st, + int events, + virStreamEventCallback cb, + void *opaque, + virFreeCallback ff) +{ + struct virFDStreamData *fdst = st->privateData; + int ret = -1; + + if (!fdst) { + streamsReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("stream is not open")); + return -1; + } + + virMutexLock(&fdst->lock); + if (fdst->watch != 0) { + streamsReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("stream already has a callback registered")); + goto cleanup; + } + + if ((fdst->watch = virEventAddHandle(fdst->fd, + events, + virFDStreamEvent, + st, + NULL)) < 0) { + streamsReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot register file watch on stream")); + goto cleanup; + } + + fdst->cbRemoved = 0; + fdst->cb = cb; + fdst->opaque = opaque; + fdst->ff = ff; + virStreamRef(st); + + ret = 0; + +cleanup: + virMutexUnlock(&fdst->lock); + return ret; +} + +static int virFDStreamFree(struct virFDStreamData *fdst) +{ + int ret; + ret = VIR_CLOSE(fdst->fd); + VIR_FREE(fdst); + return ret; +} + + +static int +virFDStreamClose(virStreamPtr st) +{ + struct virFDStreamData *fdst = st->privateData; + int ret; + + if (!fdst) + return 0; + + virMutexLock(&fdst->lock); + + ret = virFDStreamFree(fdst); + + st->privateData = NULL; + + virMutexUnlock(&fdst->lock); + + return ret; +} + +static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes) +{ + struct virFDStreamData *fdst = st->privateData; + int ret; + + if (nbytes > INT_MAX) { + virReportSystemError(ERANGE, "%s", + _("Too many bytes to write to stream")); + return -1; + } + + if (!fdst) { + streamsReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("stream is not open")); + return -1; + } + + virMutexLock(&fdst->lock); + +retry: + ret = write(fdst->fd, bytes, nbytes); + if (ret < 0) { + if (errno == EAGAIN || errno == EWOULDBLOCK) { + ret = -2; + } else if (errno == EINTR) { + goto retry; + } else { + ret = -1; + virReportSystemError(errno, "%s", + _("cannot write to stream")); + } + } + + virMutexUnlock(&fdst->lock); + return ret; +} + + +static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes) +{ + struct virFDStreamData *fdst = st->privateData; + int ret; + + if (nbytes > INT_MAX) { + virReportSystemError(ERANGE, "%s", + _("Too many bytes to read from stream")); + return -1; + } + + if (!fdst) { + streamsReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("stream is not open")); + return -1; + } + + virMutexLock(&fdst->lock); + +retry: + ret = read(fdst->fd, bytes, nbytes); + if (ret < 0) { + if (errno == EAGAIN || errno == EWOULDBLOCK) { + ret = -2; + } else if (errno == EINTR) { + goto retry; + } else { + ret = -1; + virReportSystemError(errno, "%s", + _("cannot read from stream")); + } + } + + virMutexUnlock(&fdst->lock); + return ret; +} + + +static virStreamDriver virFDStreamDrv = { + .streamSend = virFDStreamWrite, + .streamRecv = virFDStreamRead, + .streamFinish = virFDStreamClose, + .streamAbort = virFDStreamClose, + .streamAddCallback = virFDStreamAddCallback, + .streamUpdateCallback = virFDStreamUpdateCallback, + .streamRemoveCallback = virFDStreamRemoveCallback +}; + +int virFDStreamOpen(virStreamPtr st, + int fd) +{ + struct virFDStreamData *fdst; + + if ((st->flags & VIR_STREAM_NONBLOCK) && + virSetNonBlock(fd) < 0) + return -1; + + if (VIR_ALLOC(fdst) < 0) { + virReportOOMError(); + return -1; + } + + fdst->fd = fd; + if (virMutexInit(&fdst->lock) < 0) { + VIR_FREE(fdst); + streamsReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to initialize mutex")); + return -1; + } + + st->driver = &virFDStreamDrv; + st->privateData = fdst; + + return 0; +} + + +#if HAVE_SYS_UN_H +int virFDStreamConnectUNIX(virStreamPtr st, + const char *path, + bool abstract) +{ + struct sockaddr_un sa; + int i = 0; + int timeout = 3; + int ret; + + int fd = socket(AF_UNIX, SOCK_STREAM, 0); + if (fd < 0) { + virReportSystemError(errno, "%s", _("Unable to open UNIX socket")); + goto error; + } + + memset(&sa, 0, sizeof(sa)); + sa.sun_family = AF_UNIX; + if (abstract) { + if (virStrcpy(sa.sun_path+1, path, sizeof(sa.sun_path)-1) == NULL) + goto error; + sa.sun_path[0] = '\0'; + } else { + if (virStrcpy(sa.sun_path, path, sizeof(sa.sun_path)) == NULL) + goto error; + } + + do { + ret = connect(fd, (struct sockaddr *)&sa, sizeof(sa)); + if (ret == 0) + break; + + if (errno == ENOENT || errno == ECONNREFUSED) { + /* ENOENT : Socket may not have shown up yet + * ECONNREFUSED : Leftover socket hasn't been removed yet */ + continue; + } + + goto error; + } while ((++i <= timeout*5) && (usleep(.2 * 1000000) <= 0)); + + if (virFDStreamOpen(st, fd) < 0) + goto error; + return 0; + +error: + VIR_FORCE_CLOSE(fd); + return -1; +} +#else +int virFDStreamConnectUNIX(virStreamPtr st ATTRIBUTE_UNUSED, + const char *path ATTRIBUTE_UNUSED, + bool abstract ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("UNIX domain sockets are not supported on this platform")); + return -1; +} +#endif + +int virFDStreamOpenFile(virStreamPtr st, + const char *path, + int flags) +{ + int fd; + struct stat sb; + + if (flags & O_CREAT) { + streamsReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unexpected O_CREAT flag when opening existing file")); + } + + if ((fd = open(path, flags)) < 0) { + virReportSystemError(errno, + _("Unable to open stream for '%s'"), + path); + return -1; + } + + if (fstat(fd, &sb) < 0) { + virReportSystemError(errno, + _("Unable to access stream for '%s'"), + path); + goto error; + } + + /* Thanks to the POSIX i/o model, we can't reliably get + * non-blocking I/O on block devs/regular files. To + * support those we need to fork a helper process todo + * the I/O so we just have a fifo. Or use AIO :-( + */ + if ((st->flags & VIR_STREAM_NONBLOCK) && + (!S_ISCHR(sb.st_mode) && + !S_ISFIFO(sb.st_mode))) { + streamsReportError(VIR_ERR_INTERNAL_ERROR, + _("Non-blocking I/O is not supported on %s"), + path); + goto error; + } + + if (virFDStreamOpen(st, fd) < 0) + goto error; + + return 0; + +error: + close(fd); + return -1; +} + +int virFDStreamCreateFile(virStreamPtr st, + const char *path, + int flags, + mode_t mode) +{ + int fd = open(path, flags, mode); + struct stat sb; + + if (fd < 0) { + virReportSystemError(errno, + _("Unable to open stream for '%s'"), + path); + return -1; + } + + if (fstat(fd, &sb) < 0) { + virReportSystemError(errno, + _("Unable to access stream for '%s'"), + path); + goto error; + } + + /* Thanks to the POSIX i/o model, we can't reliably get + * non-blocking I/O on block devs/regular files. To + * support those we need to fork a helper process todo + * the I/O so we just have a fifo. Or use AIO :-( + */ + if ((st->flags & VIR_STREAM_NONBLOCK) && + (!S_ISCHR(sb.st_mode) && + !S_ISFIFO(sb.st_mode))) { + streamsReportError(VIR_ERR_INTERNAL_ERROR, + _("Non-blocking I/O is not supported on %s"), + path); + goto error; + } + + if (virFDStreamOpen(st, fd) < 0) + goto error; + + return 0; + +error: + close(fd); + return -1; +} + diff --git a/src/fdstream.h b/src/fdstream.h new file mode 100644 index 0000000..f8d22d5 --- /dev/null +++ b/src/fdstream.h @@ -0,0 +1,44 @@ +/* + * fdstream.h: generic streams impl for file descriptors + * + * Copyright (C) 2009-2010 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + */ + + +#ifndef __VIR_FDSTREAM_H_ +# define __VIR_FDSTREAM_H_ + +# include "internal.h" +# include <stdbool.h> + +int virFDStreamOpen(virStreamPtr st, + int fd); + +int virFDStreamConnectUNIX(virStreamPtr st, + const char *path, + bool abstract); + +int virFDStreamOpenFile(virStreamPtr st, + const char *path, + int flags); +int virFDStreamCreateFile(virStreamPtr st, + const char *path, + int flags, + mode_t mode); + +#endif /* __VIR_FDSTREAM_H_ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 430c7c1..693e2ed 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -331,6 +331,13 @@ virEventUpdateHandle; virEventUpdateTimeout; +# fdstream.h +virFDStreamOpen; +virFDStreamConnectUNIX; +virFDStreamOpenFile; +virFDStreamCreateFile; + + # files.h virClose; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d70ae8b..c6c3133 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -81,6 +81,7 @@ #include "hooks.h" #include "storage_file.h" #include "virtaudit.h" +#include "fdstream.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -10800,279 +10801,6 @@ qemuDomainIsMigratable(virDomainDefPtr def) return true; } - -/* Tunnelled migration stream support */ -struct qemuStreamMigFile { - int fd; - - int watch; - unsigned int cbRemoved; - unsigned int dispatching; - virStreamEventCallback cb; - void *opaque; - virFreeCallback ff; -}; - -static int qemuStreamMigRemoveCallback(virStreamPtr stream) -{ - struct qemud_driver *driver = stream->conn->privateData; - struct qemuStreamMigFile *qemust = stream->privateData; - int ret = -1; - - if (!qemust) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("stream is not open")); - return -1; - } - - qemuDriverLock(driver); - if (qemust->watch == 0) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("stream does not have a callback registered")); - goto cleanup; - } - - virEventRemoveHandle(qemust->watch); - if (qemust->dispatching) - qemust->cbRemoved = 1; - else if (qemust->ff) - (qemust->ff)(qemust->opaque); - - qemust->watch = 0; - qemust->ff = NULL; - qemust->cb = NULL; - qemust->opaque = NULL; - - ret = 0; - -cleanup: - qemuDriverUnlock(driver); - return ret; -} - -static int qemuStreamMigUpdateCallback(virStreamPtr stream, int events) -{ - struct qemud_driver *driver = stream->conn->privateData; - struct qemuStreamMigFile *qemust = stream->privateData; - int ret = -1; - - if (!qemust) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("stream is not open")); - return -1; - } - - qemuDriverLock(driver); - if (qemust->watch == 0) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("stream does not have a callback registered")); - goto cleanup; - } - - virEventUpdateHandle(qemust->watch, events); - - ret = 0; - -cleanup: - qemuDriverUnlock(driver); - return ret; -} - -static void qemuStreamMigEvent(int watch ATTRIBUTE_UNUSED, - int fd ATTRIBUTE_UNUSED, - int events, - void *opaque) -{ - virStreamPtr stream = opaque; - struct qemud_driver *driver = stream->conn->privateData; - struct qemuStreamMigFile *qemust = stream->privateData; - virStreamEventCallback cb; - void *cbopaque; - virFreeCallback ff; - - qemuDriverLock(driver); - if (!qemust || !qemust->cb) { - qemuDriverUnlock(driver); - return; - } - - cb = qemust->cb; - cbopaque = qemust->opaque; - ff = qemust->ff; - qemust->dispatching = 1; - qemuDriverUnlock(driver); - - cb(stream, events, cbopaque); - - qemuDriverLock(driver); - qemust->dispatching = 0; - if (qemust->cbRemoved && ff) - (ff)(cbopaque); - qemuDriverUnlock(driver); -} - -static int -qemuStreamMigAddCallback(virStreamPtr st, - int events, - virStreamEventCallback cb, - void *opaque, - virFreeCallback ff) -{ - struct qemud_driver *driver = st->conn->privateData; - struct qemuStreamMigFile *qemust = st->privateData; - int ret = -1; - - if (!qemust) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("stream is not open")); - return -1; - } - - qemuDriverLock(driver); - if (qemust->watch != 0) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("stream already has a callback registered")); - goto cleanup; - } - - if ((qemust->watch = virEventAddHandle(qemust->fd, - events, - qemuStreamMigEvent, - st, - NULL)) < 0) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot register file watch on stream")); - goto cleanup; - } - - qemust->cbRemoved = 0; - qemust->cb = cb; - qemust->opaque = opaque; - qemust->ff = ff; - virStreamRef(st); - - ret = 0; - -cleanup: - qemuDriverUnlock(driver); - return ret; -} - -static void qemuStreamMigFree(struct qemuStreamMigFile *qemust) -{ - if (qemust->fd != -1) - close(qemust->fd); - VIR_FREE(qemust); -} - -static struct qemuStreamMigFile *qemuStreamMigOpen(virStreamPtr st, - const char *unixfile) -{ - struct qemuStreamMigFile *qemust = NULL; - struct sockaddr_un sa_qemu; - int i = 0; - int timeout = 3; - int ret; - - if (VIR_ALLOC(qemust) < 0) { - virReportOOMError(); - return NULL; - } - - qemust->fd = socket(AF_UNIX, SOCK_STREAM, 0); - if (qemust->fd < 0) - goto cleanup; - - memset(&sa_qemu, 0, sizeof(sa_qemu)); - sa_qemu.sun_family = AF_UNIX; - if (virStrcpy(sa_qemu.sun_path, unixfile, sizeof(sa_qemu.sun_path)) == NULL) - goto cleanup; - - do { - ret = connect(qemust->fd, (struct sockaddr *)&sa_qemu, sizeof(sa_qemu)); - if (ret == 0) - break; - - if (errno == ENOENT || errno == ECONNREFUSED) { - /* ENOENT : Socket may not have shown up yet - * ECONNREFUSED : Leftover socket hasn't been removed yet */ - continue; - } - - goto cleanup; - } while ((++i <= timeout*5) && (usleep(.2 * 1000000) <= 0)); - - if ((st->flags & VIR_STREAM_NONBLOCK) && virSetNonBlock(qemust->fd) < 0) - goto cleanup; - - return qemust; - -cleanup: - qemuStreamMigFree(qemust); - return NULL; -} - -static int -qemuStreamMigClose(virStreamPtr st) -{ - struct qemud_driver *driver = st->conn->privateData; - struct qemuStreamMigFile *qemust = st->privateData; - - if (!qemust) - return 0; - - qemuDriverLock(driver); - - qemuStreamMigFree(qemust); - - st->privateData = NULL; - - qemuDriverUnlock(driver); - - return 0; -} - -static int qemuStreamMigWrite(virStreamPtr st, const char *bytes, size_t nbytes) -{ - struct qemud_driver *driver = st->conn->privateData; - struct qemuStreamMigFile *qemust = st->privateData; - int ret; - - if (!qemust) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("stream is not open")); - return -1; - } - - qemuDriverLock(driver); - -retry: - ret = write(qemust->fd, bytes, nbytes); - if (ret < 0) { - if (errno == EAGAIN || errno == EWOULDBLOCK) { - ret = -2; - } else if (errno == EINTR) { - goto retry; - } else { - ret = -1; - virReportSystemError(errno, "%s", - _("cannot write to stream")); - } - } - - qemuDriverUnlock(driver); - return ret; -} - -static virStreamDriver qemuStreamMigDrv = { - .streamSend = qemuStreamMigWrite, - .streamFinish = qemuStreamMigClose, - .streamAbort = qemuStreamMigClose, - .streamAddCallback = qemuStreamMigAddCallback, - .streamUpdateCallback = qemuStreamMigUpdateCallback, - .streamRemoveCallback = qemuStreamMigRemoveCallback -}; - /* Prepare is the first step, and it runs on the destination host. * * This version starts an empty VM listening on a localhost TCP port, and @@ -11095,7 +10823,6 @@ qemudDomainMigratePrepareTunnel(virConnectPtr dconn, int internalret; char *unixfile = NULL; unsigned long long qemuCmdFlags; - struct qemuStreamMigFile *qemust = NULL; qemuDomainObjPrivatePtr priv = NULL; struct timeval now; @@ -11205,8 +10932,9 @@ qemudDomainMigratePrepareTunnel(virConnectPtr dconn, goto endjob; } - qemust = qemuStreamMigOpen(st, unixfile); - if (qemust == NULL) { + if (virFDStreamConnectUNIX(st, + unixfile, + false) < 0) { qemuDomainStartAudit(vm, "migrated", false); qemudShutdownVMDaemon(driver, vm, 0); if (!vm->persistent) { @@ -11220,10 +10948,8 @@ qemudDomainMigratePrepareTunnel(virConnectPtr dconn, goto endjob; } - st->driver = &qemuStreamMigDrv; - st->privateData = qemust; - qemuDomainStartAudit(vm, "migrated", true); + event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, VIR_DOMAIN_EVENT_STARTED_MIGRATED); diff --git a/src/util/virterror.c b/src/util/virterror.c index 70749a7..4214d6b 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -190,6 +190,9 @@ static const char *virErrorDomainName(virErrorDomain domain) { case VIR_FROM_AUDIT: dom = "Audit"; break; + case VIR_FROM_STREAMS: + dom = "Streams "; + break; } return(dom); } -- 1.7.2.3

On Tue, Nov 02, 2010 at 05:49:09PM +0000, Daniel P. Berrange wrote:
To avoid the need for duplicating implementations of virStream drivers, provide a generic implementation that can handle any FD based stream. This code is copied from the existing impl in the QEMU driver, with the locking moved into the stream impl, and addition of a read callback
The FD stream code will refuse to operate on regular files or block devices, since those can't report EAGAIN properly when they would block on I/O
* include/libvirt/virterror.h, include/libvirt/virterror.h: Add VIR_FROM_STREAM error domain * src/qemu/qemu_driver.c: Remove code obsoleted by the new generic streams driver. * src/fdstream.h, src/fdstream.c, src/fdstream.c, src/libvirt_private.syms: Generic reusable FD based streams [...] + + virMutexLock(&fdst->lock);
Somehow unrelated, but as I was going through the patch to check mostly on error handling it appeard to me we kept all the virMutex functions void, and in the pthread implementation we discard any return value to pthread_mutex_lock/unlock ... I wonder if errors should not be caught in those sub functions, and the erro stored in the virMutex structure itself.
+retry: + ret = read(fdst->fd, bytes, nbytes); + if (ret < 0) { + if (errno == EAGAIN || errno == EWOULDBLOCK) { + ret = -2; + } else if (errno == EINTR) { + goto retry; + } else { + ret = -1; + virReportSystemError(errno, "%s", + _("cannot read from stream")); + } + }
I was wondering about reusing saferead/write but they have a different semantic on blocking, [...]
+int virFDStreamOpen(virStreamPtr st, + int fd) [...] +#if HAVE_SYS_UN_H +int virFDStreamConnectUNIX(virStreamPtr st, + const char *path, + bool abstract) [...] +int virFDStreamOpenFile(virStreamPtr st, + const char *path, + int flags)
I was wondering about remote socket, not needed right now but should be doable too, right ? ACK, 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, Nov 03, 2010 at 01:57:05PM +0100, Daniel Veillard wrote:
On Tue, Nov 02, 2010 at 05:49:09PM +0000, Daniel P. Berrange wrote:
To avoid the need for duplicating implementations of virStream drivers, provide a generic implementation that can handle any FD based stream. This code is copied from the existing impl in the QEMU driver, with the locking moved into the stream impl, and addition of a read callback
The FD stream code will refuse to operate on regular files or block devices, since those can't report EAGAIN properly when they would block on I/O
* include/libvirt/virterror.h, include/libvirt/virterror.h: Add VIR_FROM_STREAM error domain * src/qemu/qemu_driver.c: Remove code obsoleted by the new generic streams driver. * src/fdstream.h, src/fdstream.c, src/fdstream.c, src/libvirt_private.syms: Generic reusable FD based streams [...] + + virMutexLock(&fdst->lock);
Somehow unrelated, but as I was going through the patch to check mostly on error handling it appeard to me we kept all the virMutex functions void, and in the pthread implementation we discard any return value to pthread_mutex_lock/unlock ... I wonder if errors should not be caught in those sub functions, and the erro stored in the virMutex structure itself.
We explicit chose not to return errors from the lock/unlock functions, because all but one of the errors cannot occur in the simplified mutex API we provide. The remaining scenario of EDEADLOCK could occur, if the default mutex impl did error checking, but in reality the just actually deadlocks instead. Add error checking in the mutex callers would lead to a huge increase in code complexity for no real world benefits.
+retry: + ret = read(fdst->fd, bytes, nbytes); + if (ret < 0) { + if (errno == EAGAIN || errno == EWOULDBLOCK) { + ret = -2; + } else if (errno == EINTR) { + goto retry; + } else { + ret = -1; + virReportSystemError(errno, "%s", + _("cannot read from stream")); + } + }
I was wondering about reusing saferead/write but they have a different semantic on blocking,
Yep, saferead/write cannot be used on any FD with O_NONBLOCK set, because they'll just spin in a 100% CPU loop whenever EAGAIN occurs, until they get nbytes worth of data.
[...]
+int virFDStreamOpen(virStreamPtr st, + int fd) [...] +#if HAVE_SYS_UN_H +int virFDStreamConnectUNIX(virStreamPtr st, + const char *path, + bool abstract) [...] +int virFDStreamOpenFile(virStreamPtr st, + const char *path, + int flags)
I was wondering about remote socket, not needed right now but should be doable too, right ?
Yeah, pretty trivial to add later 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 11/03/2010 07:23 AM, Daniel P. Berrange wrote:
I was wondering about reusing saferead/write but they have a different semantic on blocking,
Yep, saferead/write cannot be used on any FD with O_NONBLOCK set, because they'll just spin in a 100% CPU loop whenever EAGAIN occurs, until they get nbytes worth of data.
Is it worth teaching saferead/write to use fcntl() to determine if an fd is O_NONBLOCK? And if so, should it outright reject an O_NONBLOCK fd (to help us diagnose bugs) or be documented as allowing an EAGAIN failure on non-blocking fds? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Wed, Nov 03, 2010 at 10:38:33AM -0600, Eric Blake wrote:
On 11/03/2010 07:23 AM, Daniel P. Berrange wrote:
I was wondering about reusing saferead/write but they have a different semantic on blocking,
Yep, saferead/write cannot be used on any FD with O_NONBLOCK set, because they'll just spin in a 100% CPU loop whenever EAGAIN occurs, until they get nbytes worth of data.
Is it worth teaching saferead/write to use fcntl() to determine if an fd is O_NONBLOCK? And if so, should it outright reject an O_NONBLOCK fd (to help us diagnose bugs) or be documented as allowing an EAGAIN failure on non-blocking fds?
The API design doesn't lend itself to this, because it would result in a horribly inefficient way of reading data fcntl(fd) read(fd, buf, 1024); fcntl(fd) read(fd, buf, 1024); fcntl(fd) read(fd, buf, 1024); fcntl(fd) read(fd, buf, 1024); fcntl(fd) read(fd, buf, 1024); fcntl(fd) read(fd, buf, 1024); fcntl(fd) read(fd, buf, 1024); ... 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 :|

This provides an implementation of the virDomainOpenConsole API with the QEMU driver. For the streams code, this reuses most of the code previously added for the tunnelled migration streams since it is generic. * src/qemu/qemu_driver.c: Support virDomainOpenConsole --- src/qemu/qemu_driver.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 77 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c6c3133..db3546f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13031,6 +13031,82 @@ cleanup: return ret; } + +static int +qemuDomainOpenConsole(virDomainPtr dom, + const char *devname, + virStreamPtr st, + unsigned int flags) +{ + struct qemud_driver *driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + int ret = -1; + int i; + virDomainChrDefPtr chr = NULL; + + virCheckFlags(0, -1); + + qemuDriverLock(driver); + virUUIDFormat(dom->uuid, uuidstr); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + if (!vm) { + qemuReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto cleanup; + } + + if (devname) { + if (vm->def->console && + STREQ(devname, vm->def->console->info.alias)) + chr = vm->def->console; + for (i = 0 ; !chr && i < vm->def->nserials ; i++) { + if (STREQ(devname, vm->def->serials[i]->info.alias)) + chr = vm->def->serials[i]; + } + for (i = 0 ; !chr && i < vm->def->nparallels ; i++) { + if (STREQ(devname, vm->def->parallels[i]->info.alias)) + chr = vm->def->parallels[i]; + } + } else { + if (vm->def->console) + chr = vm->def->console; + else if (vm->def->nserials) + chr = vm->def->serials[0]; + } + + if (!chr) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find character device %s"), + NULLSTR(devname)); + goto cleanup; + } + + if (chr->type != VIR_DOMAIN_CHR_TYPE_PTY) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("character device %s is not using a PTY"), + NULLSTR(devname)); + goto cleanup; + } + + if (virFDStreamOpenFile(st, chr->data.file.path, O_RDWR) < 0) + goto cleanup; + + ret = 0; +cleanup: + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + return ret; +} + + static virDriver qemuDriver = { VIR_DRV_QEMU, "QEMU", @@ -13135,7 +13211,7 @@ static virDriver qemuDriver = { qemuDomainMonitorCommand, /* qemuDomainMonitorCommand */ qemuDomainSetMemoryParameters, /* domainSetMemoryParameters */ qemuDomainGetMemoryParameters, /* domainGetMemoryParameters */ - NULL, /* domainOpenConsole */ + qemuDomainOpenConsole, /* domainOpenConsole */ }; -- 1.7.2.3

On Tue, Nov 02, 2010 at 05:49:10PM +0000, Daniel P. Berrange wrote:
This provides an implementation of the virDomainOpenConsole API with the QEMU driver. For the streams code, this reuses most of the code previously added for the tunnelled migration streams since it is generic.
* src/qemu/qemu_driver.c: Support virDomainOpenConsole --- src/qemu/qemu_driver.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 77 insertions(+), 1 deletions(-) [...] + if (devname) { + if (vm->def->console && + STREQ(devname, vm->def->console->info.alias)) + chr = vm->def->console; + for (i = 0 ; !chr && i < vm->def->nserials ; i++) { + if (STREQ(devname, vm->def->serials[i]->info.alias)) + chr = vm->def->serials[i];
small nit, maybe we should break in the loop if found, a priori it doesn't change the semantic though. Something like a goto found: when doing the affectation, but this makes the code a bit more complex...
+ } + for (i = 0 ; !chr && i < vm->def->nparallels ; i++) { + if (STREQ(devname, vm->def->parallels[i]->info.alias)) + chr = vm->def->parallels[i]; + } + } else { + if (vm->def->console) + chr = vm->def->console; + else if (vm->def->nserials) + chr = vm->def->serials[0]; + }
the following test could be removed then directly error and goto cleanup
+ if (!chr) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find character device %s"), + NULLSTR(devname)); + goto cleanup; + }
found:
+ + if (chr->type != VIR_DOMAIN_CHR_TYPE_PTY) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("character device %s is not using a PTY"), + NULLSTR(devname)); + goto cleanup; + }
ACK, code is still fine as-is 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/

The util/threads.c/h code already has APIs for mutexes, condition variables and thread locals. This commit adds in code for actually creating threads. * src/libvirt_private.syms: Export new symbols * src/util/threads.h: Define APIs virThreadCreate, virThreadSelf, virThreadIsSelf and virThreadJoin * src/util/threads-win32.c, src/util/threads-win32.h: Win32 impl of threads * src/util/threads-pthread.c, src/util/threads-pthread.h: POSIX impl of threads --- src/libvirt_private.syms | 4 +++ src/util/threads-pthread.c | 45 ++++++++++++++++++++++++++++++++++++++++++ src/util/threads-pthread.h | 4 +++ src/util/threads-win32.c | 47 ++++++++++++++++++++++++++++++++++++++++++++ src/util/threads-win32.h | 3 ++ src/util/threads.h | 15 ++++++++++++++ 6 files changed, 118 insertions(+), 0 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 693e2ed..d8287ce 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -713,6 +713,10 @@ virMutexInit; virMutexInitRecursive; virMutexLock; virMutexUnlock; +virThreadCreate; +virThreadIsSelf; +virThreadJoin; +virThreadSelf; # usb.h diff --git a/src/util/threads-pthread.c b/src/util/threads-pthread.c index 030b33f..0bc413c 100644 --- a/src/util/threads-pthread.c +++ b/src/util/threads-pthread.c @@ -129,6 +129,51 @@ void virCondBroadcast(virCondPtr c) pthread_cond_broadcast(&c->cond); } +struct virThreadArgs { + virThreadFunc func; + void *opaque; +}; + +static void *virThreadHelper(void *data) +{ + struct virThreadArgs *args = data; + args->func(args->opaque); + return NULL; +} + +int virThreadCreate(virThreadPtr thread, + bool joinable, + virThreadFunc func, + void *opaque) +{ + struct virThreadArgs args = { func, opaque }; + pthread_attr_t attr; + pthread_attr_init(&attr); + if (!joinable) + pthread_attr_setdetachstate(&attr, 1); + + int ret = pthread_create(&thread->thread, &attr, virThreadHelper, &args); + if (ret != 0) { + errno = ret; + return -1; + } + return 0; +} + +void virThreadSelf(virThreadPtr thread) +{ + thread->thread = pthread_self(); +} + +bool virThreadIsSelf(virThreadPtr thread) +{ + return pthread_self() == thread->thread ? true : false; +} + +void virThreadJoin(virThreadPtr thread) +{ + pthread_join(thread->thread, NULL); +} int virThreadLocalInit(virThreadLocalPtr l, virThreadLocalCleanup c) diff --git a/src/util/threads-pthread.h b/src/util/threads-pthread.h index 6404d1d..b25d0c2 100644 --- a/src/util/threads-pthread.h +++ b/src/util/threads-pthread.h @@ -31,6 +31,10 @@ struct virCond { pthread_cond_t cond; }; +struct virThread { + pthread_t thread; +}; + struct virThreadLocal { pthread_key_t key; }; diff --git a/src/util/threads-win32.c b/src/util/threads-win32.c index fe1fcd0..3f69f41 100644 --- a/src/util/threads-win32.c +++ b/src/util/threads-win32.c @@ -21,6 +21,8 @@ #include <config.h> +#include <process.h> + #include "memory.h" struct virThreadLocalData { @@ -205,6 +207,51 @@ void virCondBroadcast(virCondPtr c) } +struct virThreadArgs { + virThreadFunc func; + void *opaque; +}; + +static unsigned int __stdcall virThreadHelper(void *data) +{ + struct virThreadArgs *args = data; + args->func(args->opaque); + return 0; +} + +int virThreadCreate(virThreadPtr thread, + bool joinable ATTRIBUTE_UNUSED, + virThreadFunc func, + void *opaque) +{ + struct virThreadArgs args = { func, opaque }; + thread->thread = (HANDLE)_beginthreadex(NULL, 0, virThreadHelper, &args, 0, NULL); + return 0; +} + +void virThreadSelf(virThreadPtr thread) +{ + HANDLE handle = GetCurrentThread(); + HANDLE process = GetCurrentProcess(); + + DuplicateHandle(process, handle, process, + &thread->thread, 0, FALSE, + DUPLICATE_SAME_ACCESS); +} + +bool virThreadIsSelf(virThreadPtr thread) +{ + virThread self; + virThreadSelf(&self); + return self.thread == thread->thread ? true : false; +} + +void virThreadJoin(virThreadPtr thread) +{ + WaitForSingleObject(thread->thread, INFINITE); + CloseHandle(thread->thread); +} + int virThreadLocalInit(virThreadLocalPtr l, virThreadLocalCleanup c) diff --git a/src/util/threads-win32.h b/src/util/threads-win32.h index 783d91d..7142884 100644 --- a/src/util/threads-win32.h +++ b/src/util/threads-win32.h @@ -33,6 +33,9 @@ struct virCond { HANDLE *waiters; }; +struct virThread { + HANDLE thread; +}; struct virThreadLocal { DWORD key; diff --git a/src/util/threads.h b/src/util/threads.h index db54ea0..b3b827d 100644 --- a/src/util/threads.h +++ b/src/util/threads.h @@ -22,6 +22,8 @@ #ifndef __THREADS_H_ # define __THREADS_H_ +# include <stdbool.h> + # include "internal.h" typedef struct virMutex virMutex; @@ -33,10 +35,23 @@ typedef virCond *virCondPtr; typedef struct virThreadLocal virThreadLocal; typedef virThreadLocal *virThreadLocalPtr; +typedef struct virThread virThread; +typedef virThread *virThreadPtr; + int virThreadInitialize(void) ATTRIBUTE_RETURN_CHECK; void virThreadOnExit(void); +typedef void (*virThreadFunc)(void *opaque); + +int virThreadCreate(virThreadPtr thread, + bool joinable, + virThreadFunc func, + void *opaque) ATTRIBUTE_RETURN_CHECK; +void virThreadSelf(virThreadPtr thread); +bool virThreadIsSelf(virThreadPtr thread); +void virThreadJoin(virThreadPtr thread); + int virMutexInit(virMutexPtr m) ATTRIBUTE_RETURN_CHECK; int virMutexInitRecursive(virMutexPtr m) ATTRIBUTE_RETURN_CHECK; void virMutexDestroy(virMutexPtr m); -- 1.7.2.3

On Tue, Nov 02, 2010 at 05:49:11PM +0000, Daniel P. Berrange wrote:
The util/threads.c/h code already has APIs for mutexes, condition variables and thread locals. This commit adds in code for actually creating threads.
* src/libvirt_private.syms: Export new symbols * src/util/threads.h: Define APIs virThreadCreate, virThreadSelf, virThreadIsSelf and virThreadJoin * src/util/threads-win32.c, src/util/threads-win32.h: Win32 impl of threads * src/util/threads-pthread.c, src/util/threads-pthread.h: POSIX impl of threads --- src/libvirt_private.syms | 4 +++ src/util/threads-pthread.c | 45 ++++++++++++++++++++++++++++++++++++++++++ src/util/threads-pthread.h | 4 +++ src/util/threads-win32.c | 47 ++++++++++++++++++++++++++++++++++++++++++++ src/util/threads-win32.h | 3 ++ src/util/threads.h | 15 ++++++++++++++ 6 files changed, 118 insertions(+), 0 deletions(-)
ACK 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/

2010/11/2 Daniel P. Berrange <berrange@redhat.com>:
The util/threads.c/h code already has APIs for mutexes, condition variables and thread locals. This commit adds in code for actually creating threads.
+ +void virThreadSelf(virThreadPtr thread) +{ + Â Â thread->thread = pthread_self(); +} + +bool virThreadIsSelf(virThreadPtr thread) +{ + Â Â return pthread_self() == thread->thread ? true : false; +}
The pthread_self manpage says that it's not safe to compare pthread_t's using == and advises to use pthread_equal for this.
+ +void virThreadJoin(virThreadPtr thread) +{ + Â Â pthread_join(thread->thread, NULL); +}
 int virThreadLocalInit(virThreadLocalPtr l,             virThreadLocalCleanup c)
diff --git a/src/util/threads-win32.c b/src/util/threads-win32.c index fe1fcd0..3f69f41 100644 --- a/src/util/threads-win32.c +++ b/src/util/threads-win32.c @@ -21,6 +21,8 @@
 #include <config.h>
+#include <process.h> + Â #include "memory.h"
 struct virThreadLocalData { @@ -205,6 +207,51 @@ void virCondBroadcast(virCondPtr c)  }
+struct virThreadArgs { + Â Â virThreadFunc func; + Â Â void *opaque; +}; + +static unsigned int __stdcall virThreadHelper(void *data) +{ + Â Â struct virThreadArgs *args = data; + Â Â args->func(args->opaque); + Â Â return 0; +} + +int virThreadCreate(virThreadPtr thread, + Â Â Â Â Â Â Â Â Â Â bool joinable ATTRIBUTE_UNUSED, + Â Â Â Â Â Â Â Â Â Â virThreadFunc func, + Â Â Â Â Â Â Â Â Â Â void *opaque) +{ + Â Â struct virThreadArgs args = { func, opaque }; + Â Â thread->thread = (HANDLE)_beginthreadex(NULL, 0, virThreadHelper, &args, 0, NULL); + Â Â return 0; +}
When you create a non-joinable thread then you won't call virThreadJoin on it and you''ll leak the handle. Therefore, we should use _beginthread here instead of _beginthreadex when creating a non-joinable thread, because _endthread will close the thread's handle when the thread's function returns. See http://msdn.microsoft.com/en-us/library/kdzttdcb(VS.71).aspx
+ +void virThreadSelf(virThreadPtr thread) +{ + Â Â HANDLE handle = GetCurrentThread(); + Â Â HANDLE process = GetCurrentProcess(); + + Â Â DuplicateHandle(process, handle, process, + Â Â Â Â Â Â Â Â Â Â &thread->thread, 0, FALSE, + Â Â Â Â Â Â Â Â Â Â DUPLICATE_SAME_ACCESS); +}
If virThreadJoin is not called on virThreadPtr initialized by virThreadSelf then the duplicated handle leaks. See http://msdn.microsoft.com/en-us/library/ms683182(VS.85).aspx
+bool virThreadIsSelf(virThreadPtr thread) +{ + Â Â virThread self; + Â Â virThreadSelf(&self); + Â Â return self.thread == thread->thread ? true : false; +}
Therefore, this definitely leaks a handle per call.
+void virThreadJoin(virThreadPtr thread) +{ + Â Â WaitForSingleObject(thread->thread, INFINITE); + Â Â CloseHandle(thread->thread); +} +
This might create really subtle double-CloseHandle bugs when you call it twice on the same virThreadPtr or call it accidentally on an non-joinable thread that already exited and _endthread had close the handle. In both cases Windows might already reused the closed handle and the second call to CloseHandle might close something completely unrelated. Matthias

On Sat, Nov 06, 2010 at 12:41:11AM +0100, Matthias Bolte wrote:
2010/11/2 Daniel P. Berrange <berrange@redhat.com>:
+struct virThreadArgs { + Â Â virThreadFunc func; + Â Â void *opaque; +}; + +static unsigned int __stdcall virThreadHelper(void *data) +{ + Â Â struct virThreadArgs *args = data; + Â Â args->func(args->opaque); + Â Â return 0; +} + +int virThreadCreate(virThreadPtr thread, + Â Â Â Â Â Â Â Â Â Â bool joinable ATTRIBUTE_UNUSED, + Â Â Â Â Â Â Â Â Â Â virThreadFunc func, + Â Â Â Â Â Â Â Â Â Â void *opaque) +{ + Â Â struct virThreadArgs args = { func, opaque }; + Â Â thread->thread = (HANDLE)_beginthreadex(NULL, 0, virThreadHelper, &args, 0, NULL); + Â Â return 0; +}
When you create a non-joinable thread then you won't call virThreadJoin on it and you''ll leak the handle. Therefore, we should use _beginthread here instead of _beginthreadex when creating a non-joinable thread, because _endthread will close the thread's handle when the thread's function returns.
See http://msdn.microsoft.com/en-us/library/kdzttdcb(VS.71).aspx
+ +void virThreadSelf(virThreadPtr thread) +{ + Â Â HANDLE handle = GetCurrentThread(); + Â Â HANDLE process = GetCurrentProcess(); + + Â Â DuplicateHandle(process, handle, process, + Â Â Â Â Â Â Â Â Â Â &thread->thread, 0, FALSE, + Â Â Â Â Â Â Â Â Â Â DUPLICATE_SAME_ACCESS); +}
If virThreadJoin is not called on virThreadPtr initialized by virThreadSelf then the duplicated handle leaks.
See http://msdn.microsoft.com/en-us/library/ms683182(VS.85).aspx
To deal with this, I'm using the same trick that GLib2 uses for libgthread, and storing the 'self' HANDLE in a thread local that we then close when the thread exits.
+void virThreadJoin(virThreadPtr thread) +{ + Â Â WaitForSingleObject(thread->thread, INFINITE); + Â Â CloseHandle(thread->thread); +} +
This might create really subtle double-CloseHandle bugs when you call it twice on the same virThreadPtr or call it accidentally on an non-joinable thread that already exited and _endthread had close the handle. In both cases Windows might already reused the closed handle and the second call to CloseHandle might close something completely unrelated.
Initializing thread->thread back to 0 should take care of that problem. 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 :|

The code currently uses pthreads APIs directly. This is not portable to Win32 threads. Switch it over to use the portability APIs. Also add a wrapper for pipe() which is subtely different on Win32 * daemon/event.c: Switch to use virMutex & virThread. --- daemon/event.c | 87 +++++++++++++++++++++++++++----------------------------- 1 files changed, 42 insertions(+), 45 deletions(-) diff --git a/daemon/event.c b/daemon/event.c index 6971409..06f9aad 100644 --- a/daemon/event.c +++ b/daemon/event.c @@ -69,9 +69,9 @@ struct virEventTimeout { /* State for the main event loop */ struct virEventLoop { - pthread_mutex_t lock; + virMutex lock; int running; - pthread_t leader; + virThread leader; int wakeupfd[2]; int handlesCount; int handlesAlloc; @@ -90,16 +90,6 @@ static int nextWatch = 1; /* Unique ID for the next timer to be registered */ static int nextTimer = 1; -static void virEventLock(void) -{ - pthread_mutex_lock(&eventLoop.lock); -} - -static void virEventUnlock(void) -{ - pthread_mutex_unlock(&eventLoop.lock); -} - /* * Register a callback for monitoring file handle events. * NB, it *must* be safe to call this from within a callback @@ -111,13 +101,13 @@ int virEventAddHandleImpl(int fd, int events, virFreeCallback ff) { int watch; EVENT_DEBUG("Add handle fd=%d events=%d cb=%p opaque=%p", fd, events, cb, opaque); - virEventLock(); + virMutexLock(&eventLoop.lock); if (eventLoop.handlesCount == eventLoop.handlesAlloc) { EVENT_DEBUG("Used %d handle slots, adding %d more", eventLoop.handlesAlloc, EVENT_ALLOC_EXTENT); if (VIR_REALLOC_N(eventLoop.handles, (eventLoop.handlesAlloc + EVENT_ALLOC_EXTENT)) < 0) { - virEventUnlock(); + virMutexUnlock(&eventLoop.lock); return -1; } eventLoop.handlesAlloc += EVENT_ALLOC_EXTENT; @@ -137,7 +127,7 @@ int virEventAddHandleImpl(int fd, int events, eventLoop.handlesCount++; virEventInterruptLocked(); - virEventUnlock(); + virMutexUnlock(&eventLoop.lock); return watch; } @@ -151,7 +141,7 @@ void virEventUpdateHandleImpl(int watch, int events) { return; } - virEventLock(); + virMutexLock(&eventLoop.lock); for (i = 0 ; i < eventLoop.handlesCount ; i++) { if (eventLoop.handles[i].watch == watch) { eventLoop.handles[i].events = @@ -160,7 +150,7 @@ void virEventUpdateHandleImpl(int watch, int events) { break; } } - virEventUnlock(); + virMutexUnlock(&eventLoop.lock); } /* @@ -178,7 +168,7 @@ int virEventRemoveHandleImpl(int watch) { return -1; } - virEventLock(); + virMutexLock(&eventLoop.lock); for (i = 0 ; i < eventLoop.handlesCount ; i++) { if (eventLoop.handles[i].deleted) continue; @@ -187,11 +177,11 @@ int virEventRemoveHandleImpl(int watch) { EVENT_DEBUG("mark delete %d %d", i, eventLoop.handles[i].fd); eventLoop.handles[i].deleted = 1; virEventInterruptLocked(); - virEventUnlock(); + virMutexUnlock(&eventLoop.lock); return 0; } } - virEventUnlock(); + virMutexUnlock(&eventLoop.lock); return -1; } @@ -212,13 +202,13 @@ int virEventAddTimeoutImpl(int frequency, return -1; } - virEventLock(); + virMutexLock(&eventLoop.lock); if (eventLoop.timeoutsCount == eventLoop.timeoutsAlloc) { EVENT_DEBUG("Used %d timeout slots, adding %d more", eventLoop.timeoutsAlloc, EVENT_ALLOC_EXTENT); if (VIR_REALLOC_N(eventLoop.timeouts, (eventLoop.timeoutsAlloc + EVENT_ALLOC_EXTENT)) < 0) { - virEventUnlock(); + virMutexUnlock(&eventLoop.lock); return -1; } eventLoop.timeoutsAlloc += EVENT_ALLOC_EXTENT; @@ -238,7 +228,7 @@ int virEventAddTimeoutImpl(int frequency, eventLoop.timeoutsCount++; ret = nextTimer-1; virEventInterruptLocked(); - virEventUnlock(); + virMutexUnlock(&eventLoop.lock); return ret; } @@ -256,7 +246,7 @@ void virEventUpdateTimeoutImpl(int timer, int frequency) { return; } - virEventLock(); + virMutexLock(&eventLoop.lock); for (i = 0 ; i < eventLoop.timeoutsCount ; i++) { if (eventLoop.timeouts[i].timer == timer) { eventLoop.timeouts[i].frequency = frequency; @@ -268,7 +258,7 @@ void virEventUpdateTimeoutImpl(int timer, int frequency) { break; } } - virEventUnlock(); + virMutexUnlock(&eventLoop.lock); } /* @@ -286,7 +276,7 @@ int virEventRemoveTimeoutImpl(int timer) { return -1; } - virEventLock(); + virMutexLock(&eventLoop.lock); for (i = 0 ; i < eventLoop.timeoutsCount ; i++) { if (eventLoop.timeouts[i].deleted) continue; @@ -294,11 +284,11 @@ int virEventRemoveTimeoutImpl(int timer) { if (eventLoop.timeouts[i].timer == timer) { eventLoop.timeouts[i].deleted = 1; virEventInterruptLocked(); - virEventUnlock(); + virMutexUnlock(&eventLoop.lock); return 0; } } - virEventUnlock(); + virMutexUnlock(&eventLoop.lock); return -1; } @@ -426,9 +416,9 @@ static int virEventDispatchTimeouts(void) { eventLoop.timeouts[i].expiresAt = now + eventLoop.timeouts[i].frequency; - virEventUnlock(); + virMutexUnlock(&eventLoop.lock); (cb)(timer, opaque); - virEventLock(); + virMutexLock(&eventLoop.lock); } } return 0; @@ -475,10 +465,10 @@ static int virEventDispatchHandles(int nfds, struct pollfd *fds) { EVENT_DEBUG("Dispatch n=%d f=%d w=%d e=%d %p", i, fds[n].fd, eventLoop.handles[i].watch, fds[n].revents, eventLoop.handles[i].opaque); - virEventUnlock(); + virMutexUnlock(&eventLoop.lock); (cb)(eventLoop.handles[i].watch, fds[n].fd, hEvents, opaque); - virEventLock(); + virMutexLock(&eventLoop.lock); } } @@ -575,9 +565,9 @@ int virEventRunOnce(void) { struct pollfd *fds = NULL; int ret, timeout, nfds; - virEventLock(); + virMutexLock(&eventLoop.lock); eventLoop.running = 1; - eventLoop.leader = pthread_self(); + virThreadSelf(&eventLoop.leader); if (virEventCleanupTimeouts() < 0 || virEventCleanupHandles() < 0) @@ -587,7 +577,7 @@ int virEventRunOnce(void) { virEventCalculateTimeout(&timeout) < 0) goto error; - virEventUnlock(); + virMutexUnlock(&eventLoop.lock); retry: EVENT_DEBUG("Poll on %d handles %p timeout %d", nfds, fds, timeout); @@ -600,7 +590,7 @@ int virEventRunOnce(void) { goto error_unlocked; } - virEventLock(); + virMutexLock(&eventLoop.lock); if (virEventDispatchTimeouts() < 0) goto error; @@ -613,31 +603,38 @@ int virEventRunOnce(void) { goto error; eventLoop.running = 0; - virEventUnlock(); + virMutexUnlock(&eventLoop.lock); VIR_FREE(fds); return 0; error: - virEventUnlock(); + virMutexUnlock(&eventLoop.lock); error_unlocked: VIR_FREE(fds); return -1; } + static void virEventHandleWakeup(int watch ATTRIBUTE_UNUSED, int fd, int events ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { char c; - virEventLock(); + virMutexLock(&eventLoop.lock); ignore_value(saferead(fd, &c, sizeof(c))); - virEventUnlock(); + virMutexUnlock(&eventLoop.lock); +} + +#ifdef WIN32 +static inline int pipe(int fd[2]) { + return _pipe(fd, 4096, 0); } +#endif int virEventInit(void) { - if (pthread_mutex_init(&eventLoop.lock, NULL) != 0) + if (virMutexInit(&eventLoop.lock) < 0) return -1; if (pipe(eventLoop.wakeupfd) < 0 || @@ -660,8 +657,8 @@ static int virEventInterruptLocked(void) char c = '\0'; if (!eventLoop.running || - pthread_self() == eventLoop.leader) { - VIR_DEBUG("Skip interrupt, %d %d", eventLoop.running, (int)eventLoop.leader); + virThreadIsSelf(&eventLoop.leader)) { + VIR_DEBUG("Skip interrupt, %d %d", eventLoop.running, (int)eventLoop.leader.thread); return 0; } @@ -674,9 +671,9 @@ static int virEventInterruptLocked(void) int virEventInterrupt(void) { int ret; - virEventLock(); + virMutexLock(&eventLoop.lock); ret = virEventInterruptLocked(); - virEventUnlock(); + virMutexUnlock(&eventLoop.lock); return ret; } -- 1.7.2.3

On Tue, Nov 02, 2010 at 05:49:12PM +0000, Daniel P. Berrange wrote:
The code currently uses pthreads APIs directly. This is not portable to Win32 threads. Switch it over to use the portability APIs. Also add a wrapper for pipe() which is subtely different on Win32
* daemon/event.c: Switch to use virMutex & virThread.
nice ! ACK 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/

This re-writes the 'virsh console' command so that it uses the new streams API. This lets it run remotely and/or as a non-root user. This requires that virsh be linked against the simple event loop from libvirtd in daemon/event.c As an added bonus, it can now connect to any console device, not just the first one. * tools/Makefile.am: Link to event.c * tools/console.c, tools/console.h: Rewrite to use the virDomainOpenConsole() APIs with streams * tools/virsh.c: Support choosing the console name via --devname $NAME --- .x-sc_avoid_write | 1 + tools/Makefile.am | 1 + tools/console.c | 330 ++++++++++++++++++++++++++++++++++++++++------------- tools/console.h | 2 +- tools/virsh.c | 76 ++++--------- tools/virsh.pod | 7 +- 6 files changed, 280 insertions(+), 137 deletions(-) diff --git a/.x-sc_avoid_write b/.x-sc_avoid_write index 1f893b8..7bb8078 100644 --- a/.x-sc_avoid_write +++ b/.x-sc_avoid_write @@ -5,3 +5,4 @@ ^src/xen/xend_internal\.c$ ^daemon/libvirtd.c$ ^gnulib/ +^tools/console.c$ diff --git a/tools/Makefile.am b/tools/Makefile.am index bfe4455..f6f19bd 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -38,6 +38,7 @@ virt-pki-validate.1: virt-pki-validate virsh_SOURCES = \ console.c console.h \ + ../daemon/event.c ../daemon/event.h \ virsh.c virsh_LDFLAGS = $(WARN_LDFLAGS) $(COVERAGE_LDFLAGS) diff --git a/tools/console.c b/tools/console.c index 60e62e2..d003ab4 100644 --- a/tools/console.c +++ b/tools/console.c @@ -34,15 +34,41 @@ # include <errno.h> # include <unistd.h> # include <signal.h> +# include <stdbool.h> -# include "console.h" # include "internal.h" +# include "console.h" # include "logging.h" # include "util.h" +# include "memory.h" +# include "virterror_internal.h" + +# include "daemon/event.h" /* ie Ctrl-] as per telnet */ # define CTRL_CLOSE_BRACKET '\35' +# define VIR_FROM_THIS VIR_FROM_NONE + +struct virConsoleBuffer { + size_t length; + size_t offset; + char *data; +}; + +typedef struct virConsole virConsole; +typedef virConsole *virConsolePtr; +struct virConsole { + virStreamPtr st; + bool quit; + + int stdinWatch; + int stdoutWatch; + + struct virConsoleBuffer streamToTerminal; + struct virConsoleBuffer terminalToStream; +}; + static int got_signal = 0; static void do_signal(int sig ATTRIBUTE_UNUSED) { got_signal = 1; @@ -61,22 +87,191 @@ cfmakeraw (struct termios *attr) } # endif /* !HAVE_CFMAKERAW */ -int vshRunConsole(const char *tty) { - int ttyfd, ret = -1; +static void +virConsoleEventOnStream(virStreamPtr st, + int events, void *opaque) +{ + virConsolePtr con = opaque; + + if (events & VIR_STREAM_EVENT_READABLE) { + size_t avail = con->streamToTerminal.length - + con->streamToTerminal.offset; + int got; + + if (avail < 1024) { + if (VIR_REALLOC_N(con->streamToTerminal.data, + con->streamToTerminal.length + 1024) < 0) { + virReportOOMError(); + con->quit = true; + return; + } + con->streamToTerminal.length += 1024; + avail += 1024; + } + + got = virStreamRecv(st, + con->streamToTerminal.data + + con->streamToTerminal.offset, + avail); + if (got == -2) + return; /* blocking */ + if (got <= 0) { + con->quit = true; + return; + } + con->streamToTerminal.offset += got; + if (con->streamToTerminal.offset) + virEventUpdateHandleImpl(con->stdoutWatch, + VIR_EVENT_HANDLE_WRITABLE); + } + + if (events & VIR_STREAM_EVENT_WRITABLE && + con->terminalToStream.offset) { + ssize_t done; + size_t avail; + done = virStreamSend(con->st, + con->terminalToStream.data, + con->terminalToStream.offset); + if (done == -2) + return; /* blocking */ + if (done < 0) { + con->quit = true; + return; + } + memmove(con->terminalToStream.data, + con->terminalToStream.data + done, + con->terminalToStream.offset - done); + con->terminalToStream.offset -= done; + + avail = con->terminalToStream.length - con->terminalToStream.offset; + if (avail > 1024) { + if (VIR_REALLOC_N(con->terminalToStream.data, + con->terminalToStream.offset + 1024) < 0) + {} + con->terminalToStream.length = con->terminalToStream.offset + 1024; + } + } + if (!con->terminalToStream.offset) + virStreamEventUpdateCallback(con->st, + VIR_STREAM_EVENT_READABLE); + + if (events & VIR_STREAM_EVENT_ERROR || + events & VIR_STREAM_EVENT_HANGUP) { + con->quit = true; + } +} + +static void +virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED, + int fd ATTRIBUTE_UNUSED, + int events, + void *opaque) +{ + virConsolePtr con = opaque; + + if (events & VIR_EVENT_HANDLE_READABLE) { + size_t avail = con->terminalToStream.length - + con->terminalToStream.offset; + int got; + + if (avail < 1024) { + if (VIR_REALLOC_N(con->terminalToStream.data, + con->terminalToStream.length + 1024) < 0) { + virReportOOMError(); + con->quit = true; + return; + } + con->terminalToStream.length += 1024; + avail += 1024; + } + + got = read(fd, + con->terminalToStream.data + + con->terminalToStream.offset, + avail); + if (got < 0) { + if (errno != EAGAIN) { + con->quit = true; + } + return; + } + if (got == 0) { + con->quit = true; + return; + } + if (con->terminalToStream.data[con->terminalToStream.offset] == CTRL_CLOSE_BRACKET) { + con->quit = true; + return; + } + + con->terminalToStream.offset += got; + if (con->terminalToStream.offset) + virStreamEventUpdateCallback(con->st, + VIR_STREAM_EVENT_READABLE | + VIR_STREAM_EVENT_WRITABLE); + } + + if (events & VIR_EVENT_HANDLE_ERROR || + events & VIR_EVENT_HANDLE_HANGUP) { + con->quit = true; + } +} + +static void +virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED, + int fd, + int events, + void *opaque) +{ + virConsolePtr con = opaque; + + if (events & VIR_EVENT_HANDLE_WRITABLE && + con->streamToTerminal.offset) { + ssize_t done; + size_t avail; + done = write(fd, + con->streamToTerminal.data, + con->streamToTerminal.offset); + if (done < 0) { + if (errno != EAGAIN) { + con->quit = true; + } + return; + } + memmove(con->streamToTerminal.data, + con->streamToTerminal.data + done, + con->streamToTerminal.offset - done); + con->streamToTerminal.offset -= done; + + avail = con->streamToTerminal.length - con->streamToTerminal.offset; + if (avail > 1024) { + if (VIR_REALLOC_N(con->streamToTerminal.data, + con->streamToTerminal.offset + 1024) < 0) + {} + con->streamToTerminal.length = con->streamToTerminal.offset + 1024; + } + } + + if (!con->streamToTerminal.offset) + virEventUpdateHandleImpl(con->stdoutWatch, 0); + + if (events & VIR_EVENT_HANDLE_ERROR || + events & VIR_EVENT_HANDLE_HANGUP) { + con->quit = true; + } +} + + +int vshRunConsole(virDomainPtr dom, const char *devname) +{ + int ret = -1; struct termios ttyattr, rawattr; void (*old_sigquit)(int); void (*old_sigterm)(int); void (*old_sigint)(int); void (*old_sighup)(int); void (*old_sigpipe)(int); - - - /* We do not want this to become the controlling TTY */ - if ((ttyfd = open(tty, O_NOCTTY | O_RDWR)) < 0) { - VIR_ERROR(_("unable to open tty %s: %s"), - tty, strerror(errno)); - return -1; - } + virConsolePtr con = NULL; /* Put STDIN into raw mode so that stuff typed does not echo to the screen (the TTY reads will @@ -86,7 +281,7 @@ int vshRunConsole(const char *tty) { if (tcgetattr(STDIN_FILENO, &ttyattr) < 0) { VIR_ERROR(_("unable to get tty attributes: %s"), strerror(errno)); - goto closetty; + return -1; } rawattr = ttyattr; @@ -95,7 +290,7 @@ int vshRunConsole(const char *tty) { if (tcsetattr(STDIN_FILENO, TCSAFLUSH, &rawattr) < 0) { VIR_ERROR(_("unable to set tty attributes: %s"), strerror(errno)); - goto closetty; + goto resettty; } @@ -110,76 +305,55 @@ int vshRunConsole(const char *tty) { old_sigpipe = signal(SIGPIPE, do_signal); got_signal = 0; + if (VIR_ALLOC(con) < 0) { + virReportOOMError(); + goto cleanup; + } - /* Now lets process STDIN & tty forever.... */ - for (; !got_signal ;) { - unsigned int i; - struct pollfd fds[] = { - { STDIN_FILENO, POLLIN, 0 }, - { ttyfd, POLLIN, 0 }, - }; - - /* Wait for data to be available for reading on - STDIN or the tty */ - if (poll(fds, (sizeof(fds)/sizeof(struct pollfd)), -1) < 0) { - if (got_signal) - goto cleanup; - - if (errno == EINTR || errno == EAGAIN) - continue; + con->st = virStreamNew(virDomainGetConnect(dom), + VIR_STREAM_NONBLOCK); + if (!con->st) + goto cleanup; + + if (virDomainOpenConsole(dom, devname, con->st, 0) < 0) + goto cleanup; + + con->stdinWatch = virEventAddHandleImpl(STDIN_FILENO, + VIR_EVENT_HANDLE_READABLE, + virConsoleEventOnStdin, + con, + NULL); + con->stdoutWatch = virEventAddHandleImpl(STDOUT_FILENO, + 0, + virConsoleEventOnStdout, + con, + NULL); + + virStreamEventAddCallback(con->st, + VIR_STREAM_EVENT_READABLE, + virConsoleEventOnStream, + con, + NULL); + + while (!con->quit) { + if (virEventRunOnce() < 0) + break; + } - VIR_ERROR(_("failure waiting for I/O: %s"), strerror(errno)); - goto cleanup; - } + virStreamEventRemoveCallback(con->st); + virEventRemoveHandleImpl(con->stdinWatch); + virEventRemoveHandleImpl(con->stdoutWatch); - for (i = 0 ; i < (sizeof(fds)/sizeof(struct pollfd)) ; i++) { - if (!fds[i].revents) - continue; - - /* Process incoming data available for read */ - if (fds[i].revents & POLLIN) { - char buf[4096]; - int got, sent = 0, destfd; - - if ((got = read(fds[i].fd, buf, sizeof(buf))) < 0) { - VIR_ERROR(_("failure reading input: %s"), - strerror(errno)); - goto cleanup; - } - - /* Quit if end of file, or we got the Ctrl-] key */ - if (!got || - (got == 1 && - buf[0] == CTRL_CLOSE_BRACKET)) - goto done; - - /* Data from stdin goes to the TTY, - data from the TTY goes to STDOUT */ - if (fds[i].fd == STDIN_FILENO) - destfd = ttyfd; - else - destfd = STDOUT_FILENO; - - while (sent < got) { - int done; - if ((done = safewrite(destfd, buf + sent, got - sent)) - <= 0) { - VIR_ERROR(_("failure writing output: %s"), - strerror(errno)); - goto cleanup; - } - sent += done; - } - } else { /* Any other flag from poll is an error condition */ - goto cleanup; - } - } - } - done: ret = 0; cleanup: + if (con) { + if (con->st) + virStreamFree(con->st); + VIR_FREE(con); + } + /* Restore original signal handlers */ signal(SIGQUIT, old_sigpipe); signal(SIGQUIT, old_sighup); @@ -187,13 +361,11 @@ int vshRunConsole(const char *tty) { signal(SIGQUIT, old_sigterm); signal(SIGQUIT, old_sigquit); +resettty: /* Put STDIN back into the (sane?) state we found it in before starting */ tcsetattr(STDIN_FILENO, TCSAFLUSH, &ttyattr); - closetty: - close(ttyfd); - return ret; } diff --git a/tools/console.h b/tools/console.h index d0df78d..580268d 100644 --- a/tools/console.h +++ b/tools/console.h @@ -25,7 +25,7 @@ # ifndef WIN32 -int vshRunConsole(const char *tty); +int vshRunConsole(virDomainPtr dom, const char *devname); # endif /* !WIN32 */ diff --git a/tools/virsh.c b/tools/virsh.c index b485eff..948c256 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -50,6 +50,7 @@ #include "util.h" #include "memory.h" #include "xml.h" +#include "../daemon/event.h" static char *progname; @@ -676,36 +677,16 @@ static const vshCmdInfo info_console[] = { static const vshCmdOptDef opts_console[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"devname", VSH_OT_STRING, 0, N_("character device name")}, {NULL, 0, 0, NULL} }; static int -cmdRunConsole(vshControl *ctl, virDomainPtr dom) +cmdRunConsole(vshControl *ctl, virDomainPtr dom, const char *devname) { - xmlDocPtr xml = NULL; - xmlXPathObjectPtr obj = NULL; - xmlXPathContextPtr ctxt = NULL; int ret = FALSE; - char *doc; - char *thatHost = NULL; - char *thisHost = NULL; virDomainInfo dominfo; - if (!(thisHost = virGetHostname(ctl->conn))) { - vshError(ctl, "%s", _("Failed to get local hostname")); - goto cleanup; - } - - if (!(thatHost = virConnectGetHostname(ctl->conn))) { - vshError(ctl, "%s", _("Failed to get connection hostname")); - goto cleanup; - } - - if (STRNEQ(thisHost, thatHost)) { - vshError(ctl, "%s", _("Cannot connect to a remote console device")); - goto cleanup; - } - if (virDomainGetInfo(dom, &dominfo) < 0) { vshError(ctl, "%s", _("Unable to get domain status")); goto cleanup; @@ -716,38 +697,12 @@ cmdRunConsole(vshControl *ctl, virDomainPtr dom) goto cleanup; } - doc = virDomainGetXMLDesc(dom, 0); - if (!doc) - goto cleanup; - - xml = xmlReadDoc((const xmlChar *) doc, "domain.xml", NULL, - XML_PARSE_NOENT | XML_PARSE_NONET | - XML_PARSE_NOWARNING); - VIR_FREE(doc); - if (!xml) - goto cleanup; - ctxt = xmlXPathNewContext(xml); - if (!ctxt) - goto cleanup; - - obj = xmlXPathEval(BAD_CAST "string(/domain/devices/console/@tty)", ctxt); - if ((obj != NULL) && ((obj->type == XPATH_STRING) && - (obj->stringval != NULL) && (obj->stringval[0] != 0))) { - vshPrintExtra(ctl, _("Connected to domain %s\n"), virDomainGetName(dom)); - vshPrintExtra(ctl, "%s", _("Escape character is ^]\n")); - if (vshRunConsole((const char *)obj->stringval) == 0) - ret = TRUE; - } else { - vshPrintExtra(ctl, "%s", _("No console available for domain\n")); - } - xmlXPathFreeObject(obj); + vshPrintExtra(ctl, _("Connected to domain %s\n"), virDomainGetName(dom)); + vshPrintExtra(ctl, "%s", _("Escape character is ^]\n")); + if (vshRunConsole(dom, devname) == 0) + ret = TRUE; cleanup: - xmlXPathFreeContext(ctxt); - if (xml) - xmlFreeDoc(xml); - VIR_FREE(thisHost); - VIR_FREE(thatHost); return ret; } @@ -757,6 +712,7 @@ cmdConsole(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom; int ret; + const char *devname; if (!vshConnectionUsability(ctl, ctl->conn)) return FALSE; @@ -764,7 +720,9 @@ cmdConsole(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return FALSE; - ret = cmdRunConsole(ctl, dom); + devname = vshCommandOptString(cmd, "devname", NULL); + + ret = cmdRunConsole(ctl, dom, devname); virDomainFree(dom); return ret; @@ -1241,7 +1199,7 @@ cmdCreate(vshControl *ctl, const vshCmd *cmd) virDomainGetName(dom), from); #ifndef WIN32 if (console) - cmdRunConsole(ctl, dom); + cmdRunConsole(ctl, dom, NULL); #endif virDomainFree(dom); } else { @@ -1406,7 +1364,7 @@ cmdStart(vshControl *ctl, const vshCmd *cmd) virDomainGetName(dom)); #ifndef WIN32 if (console) - cmdRunConsole(ctl, dom); + cmdRunConsole(ctl, dom, NULL); #endif } else { vshError(ctl, _("Failed to start domain %s"), virDomainGetName(dom)); @@ -11079,6 +11037,14 @@ vshInit(vshControl *ctl) /* set up the signals handlers to catch disconnections */ vshSetupSignals(); + virEventRegisterImpl(virEventAddHandleImpl, + virEventUpdateHandleImpl, + virEventRemoveHandleImpl, + virEventAddTimeoutImpl, + virEventUpdateTimeoutImpl, + virEventRemoveTimeoutImpl); + virEventInit(); + ctl->conn = virConnectOpenAuth(ctl->name, virConnectAuthPtrDefault, ctl->readonly ? VIR_CONNECT_RO : 0); diff --git a/tools/virsh.pod b/tools/virsh.pod index 5932aaa..2e7a653 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -311,9 +311,12 @@ Configure a domain to be automatically started at boot. The option I<--disable> disables autostarting. -=item B<console> I<domain-id> +=item B<console> I<domain-id> [I<devname>] -Connect the virtual serial console for the guest. +Connect the virtual serial console for the guest. The optional +I<devname> parameter refers to the device alias of an alternate +console, serial or parallel device configured for the guest. +If omitted, the primary console will be opened. =item B<create> I<FILE> optional I<--console> I<--paused> -- 1.7.2.3

On Tue, Nov 02, 2010 at 05:49:13PM +0000, Daniel P. Berrange wrote:
This re-writes the 'virsh console' command so that it uses the new streams API. This lets it run remotely and/or as a non-root user. This requires that virsh be linked against the simple event loop from libvirtd in daemon/event.c As an added bonus, it can now connect to any console device, not just the first one.
ACK, 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/

When closing open streams after a client quits, the event callback was not removed. This mean that poll() was using a closed FD and returning POLLNVAL in a busy-wait loop. * daemon/stream.c: Disconnect stream callbacks --- daemon/stream.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-) diff --git a/daemon/stream.c b/daemon/stream.c index d64fe73..cac54ea 100644 --- a/daemon/stream.c +++ b/daemon/stream.c @@ -108,6 +108,7 @@ remoteStreamEvent(virStreamPtr st, int events, void *opaque) remote_error rerr; memset(&rerr, 0, sizeof rerr); stream->closed = 1; + virStreamEventRemoveCallback(stream->st); virStreamAbort(stream->st); if (events & VIR_STREAM_EVENT_HANGUP) remoteDispatchFormatError(&rerr, "%s", _("stream had unexpected termination")); @@ -345,8 +346,10 @@ remoteRemoveClientStream(struct qemud_client *client, } } - if (!stream->closed) + if (!stream->closed) { + virStreamEventRemoveCallback(stream->st); virStreamAbort(stream->st); + } while (curr) { if (curr == stream) { @@ -429,6 +432,7 @@ remoteStreamHandleFinish(struct qemud_client *client, memset(&rerr, 0, sizeof rerr); stream->closed = 1; + virStreamEventRemoveCallback(stream->st); ret = virStreamFinish(stream->st); if (ret < 0) { @@ -462,6 +466,7 @@ remoteStreamHandleAbort(struct qemud_client *client, memset(&rerr, 0, sizeof rerr); stream->closed = 1; + virStreamEventRemoveCallback(stream->st); virStreamAbort(stream->st); if (msg->hdr.status == REMOTE_ERROR) -- 1.7.2.3

On Tue, Nov 02, 2010 at 05:49:14PM +0000, Daniel P. Berrange wrote:
When closing open streams after a client quits, the event callback was not removed. This mean that poll() was using a closed FD and returning POLLNVAL in a busy-wait loop.
* daemon/stream.c: Disconnect stream callbacks
ACK 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/

* src/lxc/lxc_driver.c, src/lxc/lxc_driver.c, src/xen/xen_driver.c: Wire up virDomainOpenConsole --- src/lxc/lxc_driver.c | 67 ++++++++++++++++++++++++++++++++++++++++++- src/uml/uml_driver.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++---- src/xen/xen_driver.c | 59 +++++++++++++++++++++++++++++++++++++- 3 files changed, 195 insertions(+), 8 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 48a38d1..f0d16a7 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -51,6 +51,7 @@ #include "uuid.h" #include "stats_linux.h" #include "hooks.h" +#include "fdstream.h" #define VIR_FROM_THIS VIR_FROM_LXC @@ -2738,6 +2739,70 @@ cleanup: return ret; } +static int +lxcDomainOpenConsole(virDomainPtr dom, + const char *devname, + virStreamPtr st, + unsigned int flags) +{ + lxc_driver_t *driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + int ret = -1; + virDomainChrDefPtr chr = NULL; + + virCheckFlags(0, -1); + + lxcDriverLock(driver); + virUUIDFormat(dom->uuid, uuidstr); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + if (!vm) { + lxcError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + if (!virDomainObjIsActive(vm)) { + lxcError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto cleanup; + } + + if (devname) { + /* XXX support device aliases in future */ + lxcError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Named device aliases are not supported")); + goto cleanup; + } else { + if (vm->def->console) + chr = vm->def->console; + else if (vm->def->nserials) + chr = vm->def->serials[0]; + } + + if (!chr) { + lxcError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot find default console device")); + goto cleanup; + } + + if (chr->type != VIR_DOMAIN_CHR_TYPE_PTY) { + lxcError(VIR_ERR_INTERNAL_ERROR, + _("character device %s is not using a PTY"), devname); + goto cleanup; + } + + if (virFDStreamOpenFile(st, chr->data.file.path, O_RDWR) < 0) + goto cleanup; + + ret = 0; +cleanup: + if (vm) + virDomainObjUnlock(vm); + lxcDriverUnlock(driver); + return ret; +} + /* Function Tables */ static virDriver lxcDriver = { @@ -2844,7 +2909,7 @@ static virDriver lxcDriver = { NULL, /* qemuDomainMonitorCommand */ lxcDomainSetMemoryParameters, /* domainSetMemoryParameters */ lxcDomainGetMemoryParameters, /* domainGetMemoryParameters */ - NULL, /* domainOpenConsole */ + lxcDomainOpenConsole, /* domainOpenConsole */ }; static virStateDriver lxcStateDriver = { diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index e0bb4e5..e54db3d 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -59,6 +59,7 @@ #include "datatypes.h" #include "logging.h" #include "domain_nwfilter.h" +#include "fdstream.h" #define VIR_FROM_THIS VIR_FROM_UML @@ -2025,11 +2026,11 @@ cleanup: static int -umlDomainBlockPeek (virDomainPtr dom, - const char *path, - unsigned long long offset, size_t size, - void *buffer, - unsigned int flags ATTRIBUTE_UNUSED) +umlDomainBlockPeek(virDomainPtr dom, + const char *path, + unsigned long long offset, size_t size, + void *buffer, + unsigned int flags ATTRIBUTE_UNUSED) { struct uml_driver *driver = dom->conn->privateData; virDomainObjPtr vm; @@ -2095,6 +2096,70 @@ cleanup: } +static int +umlDomainOpenConsole(virDomainPtr dom, + const char *devname, + virStreamPtr st, + unsigned int flags) +{ + struct uml_driver *driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + int ret = -1; + virDomainChrDefPtr chr = NULL; + + virCheckFlags(0, -1); + + umlDriverLock(driver); + virUUIDFormat(dom->uuid, uuidstr); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + if (!vm) { + umlReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + if (!virDomainObjIsActive(vm)) { + umlReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto cleanup; + } + + if (devname) { + /* XXX support device aliases in future */ + umlReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Named device aliases are not supported")); + goto cleanup; + } else { + if (vm->def->console) + chr = vm->def->console; + else if (vm->def->nserials) + chr = vm->def->serials[0]; + } + + if (!chr) { + umlReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find character device %s"), devname); + goto cleanup; + } + + if (chr->type != VIR_DOMAIN_CHR_TYPE_PTY) { + umlReportError(VIR_ERR_INTERNAL_ERROR, + _("character device %s is not using a PTY"), devname); + goto cleanup; + } + + if (virFDStreamOpenFile(st, chr->data.file.path, O_RDWR) < 0) + goto cleanup; + + ret = 0; +cleanup: + if (vm) + virDomainObjUnlock(vm); + umlDriverUnlock(driver); + return ret; +} + static virDriver umlDriver = { VIR_DRV_UML, @@ -2200,7 +2265,7 @@ static virDriver umlDriver = { NULL, /* qemuDomainMonitorCommand */ NULL, /* domainSetMemoryParamters */ NULL, /* domainGetMemoryParamters */ - NULL, /* domainOpenConsole */ + umlDomainOpenConsole, /* domainOpenConsole */ }; static int diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 9cc46ae..008a4c9 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -24,6 +24,7 @@ #include <string.h> #include <errno.h> #include <sys/types.h> +#include <fcntl.h> #include <xen/dom0_ops.h> #include <libxml/uri.h> @@ -46,6 +47,7 @@ #include "node_device_conf.h" #include "pci.h" #include "uuid.h" +#include "fdstream.h" #define VIR_FROM_THIS VIR_FROM_XEN @@ -1980,6 +1982,61 @@ out: } +static int +xenUnifiedDomainOpenConsole(virDomainPtr dom, + const char *devname, + virStreamPtr st, + unsigned int flags) +{ + virDomainDefPtr def = NULL; + int ret = -1; + virDomainChrDefPtr chr = NULL; + + virCheckFlags(0, -1); + + if (dom->id == -1) { + xenUnifiedError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto cleanup; + } + + if (devname) { + /* XXX support device aliases in future */ + xenUnifiedError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Named device aliases are not supported")); + goto cleanup; + } + + def = xenDaemonDomainFetch(dom->conn, dom->id, dom->name, NULL); + if (!def) + goto cleanup; + + if (def->console) + chr = def->console; + else if (def->nserials) + chr = def->serials[0]; + + if (!chr) { + xenUnifiedError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot find default console device")); + goto cleanup; + } + + if (chr->type != VIR_DOMAIN_CHR_TYPE_PTY) { + xenUnifiedError(VIR_ERR_INTERNAL_ERROR, + _("character device %s is not using a PTY"), devname); + goto cleanup; + } + + if (virFDStreamOpenFile(st, chr->data.file.path, O_RDWR) < 0) + goto cleanup; + + ret = 0; +cleanup: + if (def) + virDomainDefFree(def); + return ret; +} /*----- Register with libvirt.c, and initialize Xen drivers. -----*/ /* The interface which we export upwards to libvirt.c. */ @@ -2087,7 +2144,7 @@ static virDriver xenUnifiedDriver = { NULL, /* qemuDomainMonitorCommand */ NULL, /* domainSetMemoryParameters */ NULL, /* domainGetMemoryParameters */ - NULL, /* domainOpenConsole */ + xenUnifiedDomainOpenConsole, /* domainOpenConsole */ }; /** -- 1.7.2.3

On Tue, Nov 02, 2010 at 05:49:15PM +0000, Daniel P. Berrange wrote:
* src/lxc/lxc_driver.c, src/lxc/lxc_driver.c, src/xen/xen_driver.c: Wire up virDomainOpenConsole --- src/lxc/lxc_driver.c | 67 ++++++++++++++++++++++++++++++++++++++++++- src/uml/uml_driver.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++---- src/xen/xen_driver.c | 59 +++++++++++++++++++++++++++++++++++++- 3 files changed, 195 insertions(+), 8 deletions(-)
[...]
+ + if (chr->type != VIR_DOMAIN_CHR_TYPE_PTY) { + umlReportError(VIR_ERR_INTERNAL_ERROR, + _("character device %s is not using a PTY"), devname); + goto cleanup; + }
just wondering, why would that happen ? ACK, 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, Nov 03, 2010 at 02:53:57PM +0100, Daniel Veillard wrote:
On Tue, Nov 02, 2010 at 05:49:15PM +0000, Daniel P. Berrange wrote:
* src/lxc/lxc_driver.c, src/lxc/lxc_driver.c, src/xen/xen_driver.c: Wire up virDomainOpenConsole --- src/lxc/lxc_driver.c | 67 ++++++++++++++++++++++++++++++++++++++++++- src/uml/uml_driver.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++---- src/xen/xen_driver.c | 59 +++++++++++++++++++++++++++++++++++++- 3 files changed, 195 insertions(+), 8 deletions(-)
[...]
+ + if (chr->type != VIR_DOMAIN_CHR_TYPE_PTY) { + umlReportError(VIR_ERR_INTERNAL_ERROR, + _("character device %s is not using a PTY"), devname); + goto cleanup; + }
just wondering, why would that happen ?
With QEMU/UML you can configure virtual serial/console devices to be exposed in host in many different ways, eg as a TCP socket. This code only attempts to deal with devices setup with /dev/pts/NNN PTYs 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 :|
participants (4)
-
Daniel P. Berrange
-
Daniel Veillard
-
Eric Blake
-
Matthias Bolte