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

An update of the patches in 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. The main change in this version, is that the streams code has been pulled out of the QEMU driver, into a 'fdstream.h' file, so that 90% of the code can be shared across LXC/UML/Xen drivers. .x-sc_avoid_write | 1 daemon/remote.c | 95 +++++++ daemon/remote_dispatch_args.h | 2 daemon/remote_dispatch_prototypes.h | 16 + daemon/remote_dispatch_table.h | 10 daemon/stream.c | 7 include/libvirt/libvirt.h.in | 16 + include/libvirt/virterror.h | 3 src/Makefile.am | 1 src/driver.h | 10 src/esx/esx_driver.c | 1 src/fdstream.c | 472 ++++++++++++++++++++++++++++++++++++ src/fdstream.h | 44 +++ src/libvirt.c | 96 +++++++ src/libvirt_private.syms | 7 src/libvirt_public.syms | 2 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 | 359 ++++++--------------------- src/remote/remote_driver.c | 324 +++++++++++++++++++++--- src/remote/remote_protocol.c | 37 ++ src/remote/remote_protocol.h | 28 ++ src/remote/remote_protocol.x | 21 + src/remote_protocol-structs | 5 src/test/test_driver.c | 1 src/uml/uml_driver.c | 76 +++++ 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 +---- 36 files changed, 1712 insertions(+), 463 deletions(-) Daniel

The remoteIO() method has wierd calling conventions, where it is passed a pre-allocated 'struct remote_call *' but then free()s it itself, instead of letting the caller free(). This fixes those weird semantics * src/remote/remote_driver.c: Sanitize semantics of remoteIO method wrt to memory release --- src/remote/remote_driver.c | 25 +++++++++++++------------ 1 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index c8d9a4d..1c874b2 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8156,6 +8156,7 @@ remoteStreamPacket(virStreamPtr st, XDR xdr; struct remote_thread_call *thiscall; remote_message_header hdr; + int ret; memset(&hdr, 0, sizeof hdr); @@ -8225,8 +8226,9 @@ remoteStreamPacket(virStreamPtr st, } xdr_destroy (&xdr); - /* remoteIO frees 'thiscall' for us (XXX that's dubious semantics) */ - if (remoteIO(st->conn, priv, 0, thiscall) < 0) + ret = remoteIO(st->conn, priv, 0, thiscall); + VIR_FREE(thiscall); + if (ret < 0) return -1; return nbytes; @@ -8334,6 +8336,7 @@ remoteStreamRecv(virStreamPtr st, if (!privst->incomingOffset) { struct remote_thread_call *thiscall; + int ret; if (VIR_ALLOC(thiscall) < 0) { virReportOOMError(); @@ -8354,8 +8357,9 @@ remoteStreamRecv(virStreamPtr st, goto cleanup; } - /* remoteIO frees 'thiscall' for us (XXX that's dubious semantics) */ - if (remoteIO(st->conn, priv, 0, thiscall) < 0) + ret = remoteIO(st->conn, priv, 0, thiscall); + VIR_FREE(thiscall); + if (ret < 0) goto cleanup; } @@ -10056,12 +10060,10 @@ remoteIO(virConnectPtr conn, remoteError(VIR_ERR_INTERNAL_ERROR, _("failed to wake up polling thread: %s"), virStrerror(errno, errout, sizeof errout)); - VIR_FREE(thiscall); return -1; } else if (s != sizeof(ignore)) { remoteError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to wake up polling thread")); - VIR_FREE(thiscall); return -1; } @@ -10081,7 +10083,6 @@ remoteIO(virConnectPtr conn, } remoteError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to wait on condition")); - VIR_FREE(thiscall); return -1; } @@ -10132,10 +10133,8 @@ remoteIO(virConnectPtr conn, if (priv->watch >= 0) virEventUpdateHandle(priv->watch, VIR_EVENT_HANDLE_READABLE); - if (rv < 0) { - VIR_FREE(thiscall); + if (rv < 0) return -1; - } cleanup: DEBUG("All done with our call %d %p %p", thiscall->proc_nr, priv->waitDispatch, thiscall); @@ -10188,7 +10187,6 @@ cleanup: } else { rv = 0; } - VIR_FREE(thiscall); return rv; } @@ -10205,6 +10203,7 @@ call (virConnectPtr conn, struct private_data *priv, xdrproc_t ret_filter, char *ret) { struct remote_thread_call *thiscall; + int rv; thiscall = prepareCall(priv, flags, proc_nr, args_filter, args, ret_filter, ret); @@ -10214,7 +10213,9 @@ call (virConnectPtr conn, struct private_data *priv, return -1; } - return remoteIO(conn, priv, flags, thiscall); + rv = remoteIO(conn, priv, flags, thiscall); + VIR_FREE(thiscall); + return rv; } -- 1.7.2.3

On 11/01/2010 10:11 AM, Daniel P. Berrange wrote:
The remoteIO() method has wierd calling conventions, where it is passed a pre-allocated 'struct remote_call *' but then free()s it itself, instead of letting the caller free(). This fixes those weird semantics
* src/remote/remote_driver.c: Sanitize semantics of remoteIO method wrt to memory release --- src/remote/remote_driver.c | 25 +++++++++++++------------ 1 files changed, 13 insertions(+), 12 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

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 11/01/2010 10:11 AM, 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
+ +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);
Any reason you aren't using the simpler style? cp(st, ...); But not a show-stopper. Looks good to me, so ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Mon, Nov 01, 2010 at 11:50:01AM -0600, Eric Blake wrote:
On 11/01/2010 10:11 AM, 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
+ +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);
Any reason you aren't using the simpler style?
cp(st, ...);
I prefer the '(cb)(st...)' style, because it makes it clearer that 'cb' is not a function name, but a function pointer. 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 :|

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 | 1 + src/lxc/lxc_driver.c | 4 +++ 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, 81 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..5cc6ae2 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -411,6 +411,7 @@ LIBVIRT_0.8.5 { virDomainGetMemoryParameters; virDomainGetVcpusFlags; virDomainSetVcpusFlags; + virDomainOpenConsole; } LIBVIRT_0.8.2; # .... 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..b36d8d8 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2842,8 +2842,12 @@ static virDriver lxcDriver = { NULL, /* domainRevertToSnapshot */ NULL, /* domainSnapshotDelete */ NULL, /* qemuDomainMonitorCommand */ +<<<<<<< HEAD lxcDomainSetMemoryParameters, /* domainSetMemoryParameters */ lxcDomainGetMemoryParameters, /* domainGetMemoryParameters */ +======= + NULL, /* domainOpenConsole */ +>>>>>>> Introduce a virDomainOpenConsole API }; 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 a7cce6a..b753ed3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13355,6 +13355,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 11/01/2010 10:11 AM, 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
diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index a8091b1..5cc6ae2 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -411,6 +411,7 @@ LIBVIRT_0.8.5 { virDomainGetMemoryParameters; virDomainGetVcpusFlags; virDomainSetVcpusFlags; + virDomainOpenConsole; } LIBVIRT_0.8.2;
You need to fix this to refer to 0.8.6 based on 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..b36d8d8 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2842,8 +2842,12 @@ static virDriver lxcDriver = { NULL, /* domainRevertToSnapshot */ NULL, /* domainSnapshotDelete */ NULL, /* qemuDomainMonitorCommand */ +<<<<<<< HEAD lxcDomainSetMemoryParameters, /* domainSetMemoryParameters */ lxcDomainGetMemoryParameters, /* domainGetMemoryParameters */ +======= + NULL, /* domainOpenConsole */ +>>>>>>> Introduce a virDomainOpenConsole API
And fix these rebase merge conflicts. ACK with those fixes. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt 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/lxc/lxc_driver.c | 3 - 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 ++ 10 files changed, 165 insertions(+), 23 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/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index b36d8d8..48a38d1 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2842,12 +2842,9 @@ static virDriver lxcDriver = { NULL, /* domainRevertToSnapshot */ NULL, /* domainSnapshotDelete */ NULL, /* qemuDomainMonitorCommand */ -<<<<<<< HEAD lxcDomainSetMemoryParameters, /* domainSetMemoryParameters */ lxcDomainGetMemoryParameters, /* domainGetMemoryParameters */ -======= NULL, /* domainOpenConsole */ ->>>>>>> Introduce a virDomainOpenConsole API }; static virStateDriver lxcStateDriver = { 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 11/01/2010 10:11 AM, 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/lxc/lxc_driver.c | 3 - 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 ++ +++ b/src/lxc/lxc_driver.c
@@ -2842,12 +2842,9 @@ static virDriver lxcDriver = { NULL, /* domainRevertToSnapshot */ NULL, /* domainSnapshotDelete */ NULL, /* qemuDomainMonitorCommand */ -<<<<<<< HEAD lxcDomainSetMemoryParameters, /* domainSetMemoryParameters */ lxcDomainGetMemoryParameters, /* domainGetMemoryParameters */ -======= NULL, /* domainOpenConsole */ ->>>>>>> Introduce a virDomainOpenConsole API };
This hunk should be floated up into 3/10. ACK with that fixed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt 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 11/01/2010 10:11 AM, 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. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt 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 --- include/libvirt/virterror.h | 3 +- src/Makefile.am | 1 + src/fdstream.c | 472 +++++++++++++++++++++++++++++++++++++++++++ src/fdstream.h | 44 ++++ src/libvirt_private.syms | 7 + src/qemu/qemu_driver.c | 284 +------------------------- src/util/virterror.c | 3 + 7 files changed, 534 insertions(+), 280 deletions(-) create mode 100644 src/fdstream.c create mode 100644 src/fdstream.h 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..1ad2454 --- /dev/null +++ b/src/fdstream.c @@ -0,0 +1,472 @@ +/* + * 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> +#include <sys/un.h> +#include <netinet/in.h> +#include <netinet/tcp.h> + +#include "fdstream.h" +#include "virterror_internal.h" +#include "datatypes.h" +#include "memory.h" +#include "event.h" +#include "util.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 void virFDStreamFree(struct virFDStreamData *fdst) +{ + if (fdst->fd != -1) + close(fdst->fd); + VIR_FREE(fdst); +} + + +static int +virFDStreamClose(virStreamPtr st) +{ + struct virFDStreamData *fdst = st->privateData; + + if (!fdst) + return 0; + + virMutexLock(&fdst->lock); + + virFDStreamFree(fdst); + + st->privateData = NULL; + + virMutexUnlock(&fdst->lock); + + return 0; +} + +static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes) +{ + struct virFDStreamData *fdst = st->privateData; + int ret; + + 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 (!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(fdst->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; +} + + +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: + close(fd); + return -1; +} + +int virFDStreamOpenFile(virStreamPtr st, + const char *path, + int flags) +{ + int fd = open(path, flags); + 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; +} + +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 cf64bd3..fc9021a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -335,6 +335,13 @@ virEventUpdateTimeout; virClose; +# fdstream.h +virFDStreamOpen; +virFDStreamConnectUNIX; +virFDStreamOpenFile; +virFDStreamCreateFile; + + # hash.h virHashAddEntry; virHashCreate; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b753ed3..2c07ac7 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 @@ -10746,279 +10747,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 @@ -11041,7 +10769,6 @@ qemudDomainMigratePrepareTunnel(virConnectPtr dconn, int internalret; char *unixfile = NULL; unsigned long long qemuCmdFlags; - struct qemuStreamMigFile *qemust = NULL; qemuDomainObjPrivatePtr priv = NULL; struct timeval now; @@ -11151,8 +10878,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) { @@ -11166,10 +10894,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 11/01/2010 10:12 AM, 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 --- include/libvirt/virterror.h | 3 +- src/Makefile.am | 1 + src/fdstream.c | 472 +++++++++++++++++++++++++++++++++++++++++++ src/fdstream.h | 44 ++++ src/libvirt_private.syms | 7 + src/qemu/qemu_driver.c | 284 +------------------------- src/util/virterror.c | 3 + 7 files changed, 534 insertions(+), 280 deletions(-) create mode 100644 src/fdstream.c create mode 100644 src/fdstream.h
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;
Is the switch from C89 style (no trailing comma) to the C99 style (optional trailing comma permitted) intentional? Personally, I like it, since adding a new value at the end no longer requires a random-looking diff of the previous entry just to add a comma. A rough heuristic analysis says that we have: $ git grep -l enum -- '*.h' '*.h.in' | xargs sed -n '/enum/,/}/p' \ | grep -B1 } |grep -v -- -- | grep -v } | wc -l 535 535 enum declarations, of which: $ git grep -l enum -- '*.h' '*.h.in' | xargs sed -n '/enum/,/}/p' \ | grep -B1 } |grep -v -- -- | grep -v } | grep , | wc -l 281 at least 281 of them end with a trailing comma (more than half, but not by much). I'm not quite sure how to write a syntax-checker to enforce it, though. At any rate, that's just style (we already require C99 for other reasons, so it doesn't affect the validity of your patch).
+++ b/src/fdstream.c +#include <config.h> + +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <unistd.h> +#include <sys/socket.h>
These are okay,
+#include <sys/un.h>
but this is missing on Mingw, with no gnulib replacement as of yet. Do we need to add some HAVE_SYS_UN_H checks?
+#include <netinet/in.h> +#include <netinet/tcp.h>
Likewise for HAVE_NETINET_TCP_H.
+ +static void virFDStreamEvent(int watch ATTRIBUTE_UNUSED, + int fd ATTRIBUTE_UNUSED, + int events, + void *opaque) +{ + cb(stream, events, cbopaque); + + virMutexLock(&fdst->lock); + fdst->dispatching = 0; + if (fdst->cbRemoved && ff) + (ff)(cbopaque);
Two different function pointer call styles here.
+ virMutexUnlock(&fdst->lock); +} + +static int +virFDStreamAddCallback(virStreamPtr st, + int events, + virStreamEventCallback cb, + void *opaque, + virFreeCallback ff)
Spacing is off due to the rename from qemu to vir.
+ + if ((fdst->watch = virEventAddHandle(fdst->fd, + events, + virFDStreamEvent, + st, + NULL)) < 0) {
Likewise.
+ +static void virFDStreamFree(struct virFDStreamData *fdst) +{ + if (fdst->fd != -1) + close(fdst->fd);
This should use VIR_FORCE_CLOSE(fdst->fd).
+ VIR_FREE(fdst); +} + + +static int +virFDStreamClose(virStreamPtr st) +{ + struct virFDStreamData *fdst = st->privateData; + + if (!fdst) + return 0; + + virMutexLock(&fdst->lock); + + virFDStreamFree(fdst);
Before freeing the stream, should this use VIR_CLOSE(fdst->fd) and return any close failures to the caller?
+static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes)
Should this return ssize_t...
+{ + struct virFDStreamData *fdst = st->privateData; + int ret;
and s/int/ssize_t/...
+ + if (!fdst) { + streamsReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("stream is not open")); + return -1; + } + + virMutexLock(&fdst->lock); + +retry: + ret = write(fdst->fd, bytes, nbytes);
...to avoid (theoretical) truncation from ssize_t to int?
+ +static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes) +{ + struct virFDStreamData *fdst = st->privateData; + int ret;
Likewise.
+ + +int virFDStreamConnectUNIX(virStreamPtr st, + const char *path, + bool abstract)
Should this code be conditionally compiled for Linux, and omitted on mingw?
+ +error: + close(fd);
VIR_FORCE_CLOSE(fd);
+ return -1; +} + +int virFDStreamOpenFile(virStreamPtr st, + const char *path, + int flags) +{ + int fd = open(path, flags);
This should check that (flags & O_CREAT) == 0, so as to avoid the kernel interpreting a third argument of garbage mode flags if a broken caller passes O_CREAT.
+ /* 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))) {
Should we also permit S_ISSOCK as an fd that supports reliable non-blocking behavior?
+ +error: + close(fd);
VIR_FORCE_CLOSE(fd);
+ return -1; +} + +int virFDStreamCreateFile(virStreamPtr st, + const char *path, + int flags, + mode_t mode) +{ + int fd = open(path, flags, mode);
Except for the difference in open() calls, this looks identical to virFDStreamOpenFile; can they share implementations?
+++ b/src/fdstream.h +# include "internal.h" +# include <stdbool.h> ... +int virFDStreamCreateFile(virStreamPtr st, + const char *path, + int flags, + mode_t mode);
Should you also include <sys/types.h> explicitly in this file for mode_t, since I don't see it directly listed in internal.h?
+ +#endif /* __VIR_FDSTREAM_H_ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index cf64bd3..fc9021a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -335,6 +335,13 @@ virEventUpdateTimeout; virClose;
+# fdstream.h +virFDStreamOpen; +virFDStreamConnectUNIX; +virFDStreamOpenFile; +virFDStreamCreateFile;
Alphabetically, this should be listed between event.h and files.h, and not files.h and hash.h.
+++ 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 ";
In just this context, I wondered why the trailing space? Then looking at the entire file, I instead wonder: why are VIR_FROM_NWFILTER and VIR_FROM_AUDIT the only ones that lack trailing space? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Mon, Nov 01, 2010 at 02:38:14PM -0600, Eric Blake wrote:
On 11/01/2010 10:12 AM, 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 --- include/libvirt/virterror.h | 3 +- src/Makefile.am | 1 + src/fdstream.c | 472 +++++++++++++++++++++++++++++++++++++++++++ src/fdstream.h | 44 ++++ src/libvirt_private.syms | 7 + src/qemu/qemu_driver.c | 284 +------------------------- src/util/virterror.c | 3 + 7 files changed, 534 insertions(+), 280 deletions(-) create mode 100644 src/fdstream.c create mode 100644 src/fdstream.h
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;
Is the switch from C89 style (no trailing comma) to the C99 style (optional trailing comma permitted) intentional? Personally, I like it, since adding a new value at the end no longer requires a random-looking diff of the previous entry just to add a comma.
IMHO, leaving off the comma needlessly enlarges future patches because 2 lines have to be changed to add an entry instead of just one.
+++ b/src/fdstream.c +#include <config.h> + +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <unistd.h> +#include <sys/socket.h>
These are okay,
+#include <sys/un.h>
but this is missing on Mingw, with no gnulib replacement as of yet. Do we need to add some HAVE_SYS_UN_H checks?
Yes, i guess so.
+#include <netinet/in.h> +#include <netinet/tcp.h>
Likewise for HAVE_NETINET_TCP_H.
Doesn't gnulib take care of TCP socket portability for us ?
+static int +virFDStreamClose(virStreamPtr st) +{ + struct virFDStreamData *fdst = st->privateData; + + if (!fdst) + return 0; + + virMutexLock(&fdst->lock); + + virFDStreamFree(fdst);
Before freeing the stream, should this use VIR_CLOSE(fdst->fd) and return any close failures to the caller?
virFDStreamFree is what actually closes the FD in this case, and could return an error status to the caller.
+static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes)
Should this return ssize_t...
No, this has to return an int, to match the public API calling convention.
+{ + struct virFDStreamData *fdst = st->privateData; + int ret;
and s/int/ssize_t/...
+ + if (!fdst) { + streamsReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("stream is not open")); + return -1; + } + + virMutexLock(&fdst->lock); + +retry: + ret = write(fdst->fd, bytes, nbytes);
...to avoid (theoretical) truncation from ssize_t to int?
It doesn't help, because the API has to return 'int' regardless.
+ + +int virFDStreamConnectUNIX(virStreamPtr st, + const char *path, + bool abstract)
Should this code be conditionally compiled for Linux, and omitted on mingw?
Yep, probably should.
+int virFDStreamOpenFile(virStreamPtr st, + const char *path, + int flags) +{ + int fd = open(path, flags);
This should check that (flags & O_CREAT) == 0, so as to avoid the kernel interpreting a third argument of garbage mode flags if a broken caller passes O_CREAT.
+ /* 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))) {
Should we also permit S_ISSOCK as an fd that supports reliable non-blocking behavior?
AFAIK, there's no way to open a socket as a path on the filesystem is there ? So there'd be no way that open(path) could return an FD for which S_ISSOCK() is true.
+int virFDStreamCreateFile(virStreamPtr st, + const char *path, + int flags, + mode_t mode) +{ + int fd = open(path, flags, mode);
Except for the difference in open() calls, this looks identical to virFDStreamOpenFile; can they share implementations?
When I fix the code to deal with non-blocking I/O on regular files, they will probably need some different handling in each case.
+++ 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 ";
In just this context, I wondered why the trailing space? Then looking at the entire file, I instead wonder: why are VIR_FROM_NWFILTER and VIR_FROM_AUDIT the only ones that lack trailing space?
Looks like a bug to me. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On 11/02/2010 05:42 AM, Daniel P. Berrange wrote:
- VIR_FROM_AUDIT /* Error from auditing subsystem */ + VIR_FROM_AUDIT, /* Error from auditing subsystem */ + VIR_FROM_STREAMS, /* Error from I/O streams */ } virErrorDomain;
Is the switch from C89 style (no trailing comma) to the C99 style (optional trailing comma permitted) intentional? Personally, I like it, since adding a new value at the end no longer requires a random-looking diff of the previous entry just to add a comma.
IMHO, leaving off the comma needlessly enlarges future patches because 2 lines have to be changed to add an entry instead of just one.
Sounds like we're in violent agreement here, about favoring C99 syntax. Is it worth a generic cleanup patch for other enum declarations that don't yet use trailing commas, or should we just save it for an as-encountered basis?
+#include <sys/un.h>
but this is missing on Mingw, with no gnulib replacement as of yet. Do we need to add some HAVE_SYS_UN_H checks?
Yes, i guess so.
+#include <netinet/in.h> +#include <netinet/tcp.h>
Likewise for HAVE_NETINET_TCP_H.
Doesn't gnulib take care of TCP socket portability for us ?
So far, gnulib provides <netinet/in.h>, but not <netinet/tcp.h>. But what exactly are we using that's only in netinet/tcp.h? It may be easy to port to gnulib, since mingw does have tcp support (sys/un.h is the much harder task, since mingw has no notion of unix sockets).
+static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes)
Should this return ssize_t...
No, this has to return an int, to match the public API calling convention.
Then we have to check for overflow, and explicitly return an error if nbytes > INT_MAX.
+ if ((st->flags & VIR_STREAM_NONBLOCK) && + (!S_ISCHR(sb.st_mode) && + !S_ISFIFO(sb.st_mode))) {
Should we also permit S_ISSOCK as an fd that supports reliable non-blocking behavior?
AFAIK, there's no way to open a socket as a path on the filesystem is there ? So there'd be no way that open(path) could return an FD for which S_ISSOCK() is true.
You're probably right that it's not possible to bind a socket to a standard file system. But it is possible via procfs (think /proc/nnn/fd/nnn of a process which already has a socket open), and there might also be some magic /dev/ or /sys/ mappings that can provide a socket (at any rate, bash provides magic handling of /dev/tcp/host/port, whether or not the kernel also has a magic filesystem to provide that support automatically). So it boils down to a question of whether we want to permit or reject sockets, on the pre-condition that the user finds a way to hand us a filename that resolves to a socket. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Tue, Nov 02, 2010 at 10:30:54AM -0600, Eric Blake wrote:
On 11/02/2010 05:42 AM, Daniel P. Berrange wrote:
+#include <netinet/in.h> +#include <netinet/tcp.h>
Likewise for HAVE_NETINET_TCP_H.
Doesn't gnulib take care of TCP socket portability for us ?
So far, gnulib provides <netinet/in.h>, but not <netinet/tcp.h>. But what exactly are we using that's only in netinet/tcp.h? It may be easy to port to gnulib, since mingw does have tcp support (sys/un.h is the much harder task, since mingw has no notion of unix sockets).
It seems netinet/tcp.h is not required on either platform and we don't need sys/un.h on Win32.
+static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes)
Should this return ssize_t...
No, this has to return an int, to match the public API calling convention.
Then we have to check for overflow, and explicitly return an error if nbytes > INT_MAX.
Ok
+ if ((st->flags & VIR_STREAM_NONBLOCK) && + (!S_ISCHR(sb.st_mode) && + !S_ISFIFO(sb.st_mode))) {
Should we also permit S_ISSOCK as an fd that supports reliable non-blocking behavior?
AFAIK, there's no way to open a socket as a path on the filesystem is there ? So there'd be no way that open(path) could return an FD for which S_ISSOCK() is true.
You're probably right that it's not possible to bind a socket to a standard file system. But it is possible via procfs (think /proc/nnn/fd/nnn of a process which already has a socket open), and there might also be some magic /dev/ or /sys/ mappings that can provide a socket (at any rate, bash provides magic handling of /dev/tcp/host/port, whether or not the kernel also has a magic filesystem to provide that support automatically). So it boils down to a question of whether we want to permit or reject sockets, on the pre-condition that the user finds a way to hand us a filename that resolves to a socket.
I tried add S_ISSOCK() too, but gnulib #defines this to '0' which then causes gcc to issue a warning that the expression always evaluates to true, because we build with -Wlogical-op. So I've left this change out. 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/02/2010 11:51 AM, Daniel P. Berrange wrote:
I tried add S_ISSOCK() too, but gnulib #defines this to '0' which then causes gcc to issue a warning that the expression always evaluates to true, because we build with -Wlogical-op. So I've left this change out.
Fair enough :) -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

* src/util/virterror.c (virErrorDomainName): Always supply trailing space if category was present. ---
+++ 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 ";
In just this context, I wondered why the trailing space? Then looking at the entire file, I instead wonder: why are VIR_FROM_NWFILTER and VIR_FROM_AUDIT the only ones that lack trailing space? Looks like a bug to me.
Then I'm pushing this under the trivial rule (okay, it's a bit of a stretch, since the whitespace is significant as part of a string literal rather than insignificant as in indentation, but I didn't think anyone would mind :) src/util/virterror.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/virterror.c b/src/util/virterror.c index 70749a7..dc08aaa 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -179,7 +179,7 @@ static const char *virErrorDomainName(virErrorDomain domain) { dom = "CPU "; break; case VIR_FROM_NWFILTER: - dom = "Network Filter"; + dom = "Network Filter "; break; case VIR_FROM_HOOK: dom = "Sync Hook "; @@ -188,7 +188,7 @@ static const char *virErrorDomainName(virErrorDomain domain) { dom = "Domain Snapshot "; break; case VIR_FROM_AUDIT: - dom = "Audit"; + dom = "Audit "; break; } return(dom); -- 1.7.2.3

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 | 76 +++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 75 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2c07ac7..022b8e9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12977,6 +12977,80 @@ 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"), devname); + goto cleanup; + } + + if (chr->type != VIR_DOMAIN_CHR_TYPE_PTY) { + qemuReportError(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); + qemuDriverUnlock(driver); + return ret; +} + + static virDriver qemuDriver = { VIR_DRV_QEMU, "QEMU", @@ -13081,7 +13155,7 @@ static virDriver qemuDriver = { qemuDomainMonitorCommand, /* qemuDomainMonitorCommand */ qemuDomainSetMemoryParameters, /* domainSetMemoryParameters */ qemuDomainGetMemoryParameters, /* domainGetMemoryParameters */ - NULL, /* domainOpenConsole */ + qemuDomainOpenConsole, /* domainOpenConsole */ }; -- 1.7.2.3

On 11/01/2010 10:12 AM, 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 | 76 +++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 75 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]; + }
Are we guaranteed that all devices will have a non-NULL and unique alias, or do we need to do either of these: 1. break on the first hit (rather than favoring the last instance of a duplicate alias) 2. use STREQ_NULLABLE(devname, ...alias), to be robust against an unaliased device
+ if (!chr) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find character device %s"), devname);
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"), devname);
NULLSTR(devname) -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Mon, Nov 01, 2010 at 03:39:30PM -0600, Eric Blake wrote:
On 11/01/2010 10:12 AM, 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 | 76 +++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 75 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]; + }
Are we guaranteed that all devices will have a non-NULL and unique alias, or do we need to do either of these:
Yes, in QEMU they are all given non-NULL unique aliases. 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/01/2010 03:39 PM, Eric Blake wrote:
+ for (i = 0 ; !chr && i < vm->def->nserials ; i++) { + if (STREQ(devname, vm->def->serials[i]->info.alias)) + chr = vm->def->serials[i]; + }
Are we guaranteed that all devices will have a non-NULL and unique alias,
You answered this...
+ if (!chr) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find character device %s"), devname);
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"), devname);
NULLSTR(devname)
But these two fixes still need to be made, since devname is allowed to be NULL. ACK, with those fixes. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt 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 ++++--------- 5 files changed, 275 insertions(+), 135 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); -- 1.7.2.3

On 11/01/2010 10:12 AM, 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.
* 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 changes?
+ if (con->terminalToStream.data[con->terminalToStream.offset] == CTRL_CLOSE_BRACKET) { + con->quit = true; + return; + }
Is there any way to type an escape sequence, such as ^v in common stty usage, in order to allow sending a literal ^] through to the console instead of always making it quit?
+ if (con) { + if (con->st) + virStreamFree(con->st);
Should virStreamFree tolerate a NULL argument, at which point it should be added to the list in cfg.mk of free()-like functions that should not have an extra if() preceding usage? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Mon, Nov 01, 2010 at 03:55:08PM -0600, Eric Blake wrote:
On 11/01/2010 10:12 AM, 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.
* 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 changes?
+ if (con->terminalToStream.data[con->terminalToStream.offset] == CTRL_CLOSE_BRACKET) { + con->quit = true; + return; + }
Is there any way to type an escape sequence, such as ^v in common stty usage, in order to allow sending a literal ^] through to the console instead of always making it quit?
Not at this time. This was also true of the old impl, so not a regression
+ if (con) { + if (con->st) + virStreamFree(con->st);
Should virStreamFree tolerate a NULL argument, at which point it should be added to the list in cfg.mk of free()-like functions that should not have an extra if() preceding usage?
Our public APIs all raise an error if you pass NULL to any of the virXXXXXFree() APIs. 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 :|

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 11/01/2010 10:12 AM, 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 --- daemon/stream.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt 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 11/01/2010 10:12 AM, 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(-)
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Daniel P. Berrange
-
Eric Blake