[libvirt] Add new API for accessing remote guest text console
The 'virsh console' command has been an oddity that only works when run locally, as the same UID as the QEMU instance. This is because it directly opens /dev/pty/XXX. This introduces a formal API for accessing consoles that uses the virStreamPtr APIs. Now any app can open consoles anywhere it can connect to libvirt
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 wierd semantics * src/remote/remote_driver.c: Santize 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 cb0d8e1..fd761c8 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -7972,6 +7972,7 @@ remoteStreamPacket(virStreamPtr st, XDR xdr; struct remote_thread_call *thiscall; remote_message_header hdr; + int ret; memset(&hdr, 0, sizeof hdr); @@ -8041,8 +8042,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; @@ -8150,6 +8152,7 @@ remoteStreamRecv(virStreamPtr st, if (!privst->incomingOffset) { struct remote_thread_call *thiscall; + int ret; if (VIR_ALLOC(thiscall) < 0) { virReportOOMError(); @@ -8170,8 +8173,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; } @@ -9872,12 +9876,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; } @@ -9897,7 +9899,6 @@ remoteIO(virConnectPtr conn, } remoteError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to wait on condition")); - VIR_FREE(thiscall); return -1; } @@ -9948,10 +9949,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); @@ -10004,7 +10003,6 @@ cleanup: } else { rv = 0; } - VIR_FREE(thiscall); return rv; } @@ -10021,6 +10019,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); @@ -10030,7 +10029,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.1
On 08/17/2010 11:02 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 wierd semantics
s/wierd/weird/g
* src/remote/remote_driver.c: Santize semantics of remoteIO
s/Santize/Sanitize/
method wrt to memory release --- src/remote/remote_driver.c | 25 +++++++++++++------------ 1 files changed, 13 insertions(+), 12 deletions(-)
@@ -10021,6 +10019,7 @@ call (virConnectPtr conn, struct private_data *priv, xdrproc_t ret_filter, char *ret) { struct remote_thread_call *thiscall; + int rv;
Any reason you used 'ret' in some hunks, but 'rv' in this one? $ git grep 'return rv;' | wc 167 501 7084 $ git grep 'return res;' | wc 12 36 518 $ git grep 'return ret;' | wc 1250 3838 49749 Consistency argues for naming it 'ret'. ACK with that nit fixed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Thu, Aug 19, 2010 at 01:50:43PM -0600, Eric Blake wrote:
On 08/17/2010 11:02 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 wierd semantics
s/wierd/weird/g
* src/remote/remote_driver.c: Santize semantics of remoteIO
s/Santize/Sanitize/
method wrt to memory release --- src/remote/remote_driver.c | 25 +++++++++++++------------ 1 files changed, 13 insertions(+), 12 deletions(-)
@@ -10021,6 +10019,7 @@ call (virConnectPtr conn, struct private_data *priv, xdrproc_t ret_filter, char *ret) { struct remote_thread_call *thiscall; + int rv;
Any reason you used 'ret' in some hunks, but 'rv' in this one?
There is already a variable 'ret' passed in as a parameter. This is an unfortunate naming choice in the remote driver code, but there's so many instances of it that it isn't worth changing. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
The 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 todo 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 fd761c8..54bc09f 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), }; @@ -7960,6 +7968,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, @@ -8154,6 +8176,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; @@ -8197,6 +8225,8 @@ remoteStreamRecv(virStreamPtr st, rv = 0; } + remoteStreamEventTimerUpdate(privst); + DEBUG("Done %d", rv); cleanup: @@ -8207,28 +8237,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 @@ -8881,7 +9036,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; @@ -8935,7 +9090,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; } @@ -9328,7 +9483,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; } @@ -9554,6 +9709,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.1
On 08/17/2010 11:02 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 todo non-blocking sends, but this is
s/todo/to do/
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(-)
+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);
Do we have/want a return value from this callback? If so, what would we do about a non-zero return value?
+ remoteDriverLock(priv); + privst->cbDispatch = 0; + + if (!privst->cb && cbFree)
Can never be true - privst->cb had to be non-NULL 12 lines earlier to get to this point. I think you meant just 'if (cbFree)'.
+ (cbFree)(cbOpaque);
Any reason that cp is called while the driver is unlocked, but cbFree is called while the lock is still held? It seems like if we are worried that the callbacks might deadlock if we still hold the driver lock, then we should treat both callbacks in the same manner. Also, any reason you use (cb)() instead of the simpler cp() calling convention?
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);
Depending on whether you feel the callback should be run without the driver lock, this might need changing. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Thu, Aug 19, 2010 at 02:14:58PM -0600, Eric Blake wrote:
On 08/17/2010 11:02 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 todo non-blocking sends, but this is
s/todo/to do/
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(-)
+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);
Do we have/want a return value from this callback? If so, what would we do about a non-zero return value?
You could have a boolean return value to indicate whether the callback should be removed or not. I find that pattern a little confusing though, because I can never remember whether 'true' or 'false' mean remove or keep. Since we run unlocked, the callback can explicitly remove itself by calling into the APIs
+ remoteDriverLock(priv); + privst->cbDispatch = 0; + + if (!privst->cb && cbFree)
Can never be true - privst->cb had to be non-NULL 12 lines earlier to get to this point. I think you meant just 'if (cbFree)'.
Remember we just unlocked the driver. If the callback had invoked virStreamRemoveCallback, then privst->cb is now NULL. If we see this condition, then we have to now free the data. Re-entrancy is fun :-)
+ (cbFree)(cbOpaque);
Any reason that cp is called while the driver is unlocked, but cbFree is called while the lock is still held? It seems like if we are worried that the callbacks might deadlock if we still hold the driver lock, then we should treat both callbacks in the same manner.
The main callback is very likely to call back into libvirt. The free callback's only purpose is to release memory, so there is no reasonable expectation of it calling back into libvirt.
Also, any reason you use (cb)() instead of the simpler cp() calling convention?
No particular reason. 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 08/20/2010 03:07 AM, Daniel P. Berrange wrote:
+ privst->cbDispatch = 1; + remoteDriverUnlock(priv); + (cb)(st, VIR_STREAM_EVENT_READABLE, cbOpaque);
Do we have/want a return value from this callback? If so, what would we do about a non-zero return value?
You could have a boolean return value to indicate whether the callback should be removed or not. I find that pattern a little confusing though, because I can never remember whether 'true' or 'false' mean remove or keep. Since we run unlocked, the callback can explicitly remove itself by calling into the APIs
Thus, a boolean is the wrong choice; that would argue that if the callback returns anything, it should be an enum value, where the name implies the desired action. I guess I'm thinking more of an iterator callback, where it is handy to return a non-zero value to abort iterating over further elements. On the other hand, it is always harder to add a return value down the road if we come up with a compelling reason, if it requires changing signatures and existing callbacks; is it worth having a return value (and ignoring it) now, to allow for easier use of such a value in the future? But that's not a reason to reject this patch.
+ remoteDriverLock(priv); + privst->cbDispatch = 0; + + if (!privst->cb && cbFree)
Can never be true - privst->cb had to be non-NULL 12 lines earlier to get to this point. I think you meant just 'if (cbFree)'.
Remember we just unlocked the driver. If the callback had invoked virStreamRemoveCallback, then privst->cb is now NULL. If we see this condition, then we have to now free the data. Re-entrancy is fun :-)
Oh. Thanks for pointing that out. You're correct; no change needed.
+ (cbFree)(cbOpaque);
Any reason that cp is called while the driver is unlocked, but cbFree is called while the lock is still held? It seems like if we are worried that the callbacks might deadlock if we still hold the driver lock, then we should treat both callbacks in the same manner.
The main callback is very likely to call back into libvirt. The free callback's only purpose is to release memory, so there is no reasonable expectation of it calling back into libvirt.
Should we document that as part of the contract of the cbFree callback? At any rate, you've answered my questions, so: ACK, with the spelling nit in the commit message resolved. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
To enable virsh console (or equivalent) to be used remotely it is neccessary to provide remote access to the /dev/pts/XXX psuedo-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 | 54 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++++ src/lxc/lxc_driver.c | 1 + src/opennebula/one_driver.c | 1 + src/openvz/openvz_driver.c | 1 + src/phyp/phyp_driver.c | 1 + src/qemu/qemu_driver.c | 1 + src/remote/remote_driver.c | 1 + src/test/test_driver.c | 1 + src/uml/uml_driver.c | 1 + src/vbox/vbox_tmpl.c | 1 + src/xen/xen_driver.c | 1 + src/xenapi/xenapi_driver.c | 1 + 16 files changed, 83 insertions(+), 0 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index b45f7ec..bd2023a 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2278,6 +2278,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 e443c1c..2ef1b8f 100644 --- a/src/driver.h +++ b/src/driver.h @@ -461,6 +461,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); /** @@ -575,6 +580,7 @@ struct _virDriver { virDrvDomainRevertToSnapshot domainRevertToSnapshot; virDrvDomainSnapshotDelete domainSnapshotDelete; virDrvQemuDomainMonitorCommand qemuDomainMonitorCommand; + virDrvDomainOpenConsole domainOpenConsole; }; typedef int diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 4fb357b..648e4fe 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4318,6 +4318,7 @@ static virDriver esxDriver = { esxDomainRevertToSnapshot, /* domainRevertToSnapshot */ esxDomainSnapshotDelete, /* domainSnapshotDelete */ NULL, /* qemuDomainMonitorCommand */ + NULL, /* domainOpenConsole */ }; diff --git a/src/libvirt.c b/src/libvirt.c index 3ec5724..58bf567 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -12849,3 +12849,57 @@ virDomainSnapshotFree(virDomainSnapshotPtr snapshot) } return 0; } + +/** + * virDomainOpenConsole: + * @dom: the domain whose console to open + * @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 849c163..2863e63 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -405,4 +405,9 @@ LIBVIRT_0.8.2 { virDomainCreateWithFlags; } LIBVIRT_0.8.1; +LIBVIRT_0.8.3 { + global: + 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 6fe20b1..149ea3d 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2626,6 +2626,7 @@ static virDriver lxcDriver = { NULL, /* domainRevertToSnapshot */ NULL, /* domainSnapshotDelete */ NULL, /* qemuDomainMonitorCommand */ + NULL, /* domainOpenConsole */ }; static virStateDriver lxcStateDriver = { diff --git a/src/opennebula/one_driver.c b/src/opennebula/one_driver.c index e70f17b..49396d6 100644 --- a/src/opennebula/one_driver.c +++ b/src/opennebula/one_driver.c @@ -818,6 +818,7 @@ static virDriver oneDriver = { NULL, /* domainRevertToSnapshot */ NULL, /* domainSnapshotDelete */ NULL, /* qemuDomainMonitorCommand */ + NULL, /* domainOpenConsole */ }; static virStateDriver oneStateDriver = { diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index d2f91c6..1bb8864 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -1653,6 +1653,7 @@ static virDriver openvzDriver = { NULL, /* domainRevertToSnapshot */ NULL, /* domainSnapshotDelete */ NULL, /* qemuDomainMonitorCommand */ + NULL, /* domainOpenConsole */ }; int openvzRegister(void) { diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 251111d..94f10bc 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -3980,6 +3980,7 @@ static virDriver phypDriver = { NULL, /* domainRevertToSnapshot */ NULL, /* domainSnapshotDelete */ NULL, /* qemuMonitorCommand */ + NULL, /* domainOpenConsole */ }; static virStorageDriver phypStorageDriver = { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 054373a..397bd8a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12746,6 +12746,7 @@ static virDriver qemuDriver = { qemuDomainRevertToSnapshot, /* domainRevertToSnapshot */ qemuDomainSnapshotDelete, /* domainSnapshotDelete */ qemuDomainMonitorCommand, /* qemuDomainMonitorCommand */ + NULL, /* domainOpenConsole */ }; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 54bc09f..860ae46 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -10509,6 +10509,7 @@ static virDriver remote_driver = { remoteDomainRevertToSnapshot, /* domainRevertToSnapshot */ remoteDomainSnapshotDelete, /* domainSnapshotDelete */ remoteQemuDomainMonitorCommand, /* qemuDomainMonitorCommand */ + NULL, /* domainOpenConsole */ }; static virNetworkDriver network_driver = { diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 6c06cbc..414bb9a 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -5323,6 +5323,7 @@ static virDriver testDriver = { NULL, /* domainRevertToSnapshot */ NULL, /* domainSnapshotDelete */ NULL, /* qemuDomainMonitorCommand */ + NULL, /* domainOpenConsole */ }; static virNetworkDriver testNetworkDriver = { diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 04493ba..ff6ee33 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1953,6 +1953,7 @@ static virDriver umlDriver = { NULL, /* domainRevertToSnapshot */ NULL, /* domainSnapshotDelete */ NULL, /* qemuDomainMonitorCommand */ + NULL, /* domainOpenConsole */ }; diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 31fec67..6d3943d 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -8250,6 +8250,7 @@ virDriver NAME(Driver) = { vboxDomainRevertToSnapshot, /* domainRevertToSnapshot */ vboxDomainSnapshotDelete, /* domainSnapshotDelete */ NULL, /* qemuDomainMonitorCommand */ + NULL, /* domainOpenConsole */ }; virNetworkDriver NAME(NetworkDriver) = { diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index ddbfa7a..61edae6 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -2005,6 +2005,7 @@ static virDriver xenUnifiedDriver = { NULL, /* domainRevertToSnapshot */ NULL, /* domainSnapshotDelete */ NULL, /* qemuDomainMonitorCommand */ + NULL, /* domainOpenConsole */ }; /** diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index fb3c91d..59c1987 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -1821,6 +1821,7 @@ static virDriver xenapiDriver = { NULL, /* domainRevertToSnapshot */ NULL, /* domainSnapshotDelete */ NULL, /* qemuDomainMonitorCommand */ + NULL, /* domainOpenConsole */ }; /** -- 1.7.2.1
On 08/17/2010 11:02 AM, Daniel P. Berrange wrote:
To enable virsh console (or equivalent) to be used remotely it is neccessary to provide remote access to the /dev/pts/XXX
s/neccessary/necessary
psuedo-TTY associated with the console/serial/parallel device
s/psuedo/pseudo/
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
index 4fb357b..648e4fe 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4318,6 +4318,7 @@ static virDriver esxDriver = { esxDomainRevertToSnapshot, /* domainRevertToSnapshot */ esxDomainSnapshotDelete, /* domainSnapshotDelete */ NULL, /* qemuDomainMonitorCommand */ + NULL, /* domainOpenConsole */
Consistent indentation of the comment. (It's a shame that we aren't consistent between the various device drivers, so that you can't just copy and paste a new NULL line across all of them.)
+/** + * virDomainOpenConsole: + * @dom: the domain whose console to open
Reads awkwardly. How about: the domain where a console will be opened
+++ b/src/libvirt_public.syms @@ -405,4 +405,9 @@ LIBVIRT_0.8.2 { virDomainCreateWithFlags; } LIBVIRT_0.8.1;
+LIBVIRT_0.8.3 { + global: + virDomainOpenConsole; +} LIBVIRT_0.8.2;
0.8.3 is already out; this needs bumping to 0.8.4. ACK with those nits fixed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 17-08-2010 19:02, Daniel P. Berrange wrote:
diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 849c163..2863e63 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -405,4 +405,9 @@ LIBVIRT_0.8.2 { virDomainCreateWithFlags; } LIBVIRT_0.8.1;
+LIBVIRT_0.8.3 { + global: + virDomainOpenConsole; +} LIBVIRT_0.8.2; +
I think this should have read: +LIBVIRT_0.8.4 { + global: + virDomainOpenConsole; +} LIBVIRT_0.8.2; + ..since 0.8.3 is already out the door. -- Soren Hansen Ubuntu Developer http://www.ubuntu.com/
This provides an implementation of the virDomainOpenConsole API for the remote driver client and server. * daemon/remote.c: Server side impl * src/remote/remote_driver.c: Client impl * src/remote/remote_protocol.x: Wire definition --- daemon/remote.c | 52 +++++++++++++++++ daemon/remote_dispatch_args.h | 1 + daemon/remote_dispatch_prototypes.h | 8 +++ daemon/remote_dispatch_table.h | 5 ++ src/remote/remote_driver.c | 108 ++++++++++++++++++++++++---------- src/remote/remote_protocol.c | 13 ++++ src/remote/remote_protocol.h | 10 +++ src/remote/remote_protocol.x | 8 ++- 8 files changed, 172 insertions(+), 33 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 118654c..10980db 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -6610,6 +6610,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 ee95043..23b6293 100644 --- a/daemon/remote_dispatch_args.h +++ b/daemon/remote_dispatch_args.h @@ -165,3 +165,4 @@ remote_domain_snapshot_delete_args val_remote_domain_snapshot_delete_args; remote_domain_get_block_info_args val_remote_domain_get_block_info_args; remote_domain_create_with_flags_args val_remote_domain_create_with_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 cf1a0f9..b744c28 100644 --- a/daemon/remote_dispatch_prototypes.h +++ b/daemon/remote_dispatch_prototypes.h @@ -450,6 +450,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 ef00edd..73ec505 100644 --- a/daemon/remote_dispatch_table.h +++ b/daemon/remote_dispatch_table.h @@ -987,3 +987,8 @@ .args_filter = (xdrproc_t) xdr_remote_domain_create_with_flags_args, .ret_filter = (xdrproc_t) xdr_remote_domain_create_with_flags_ret, }, +{ /* DomainOpenConsole => 197 */ + .fn = (dispatch_fn) remoteDispatchDomainOpenConsole, + .args_filter = (xdrproc_t) xdr_remote_domain_open_console_args, + .ret_filter = (xdrproc_t) xdr_void, +}, diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 860ae46..a3608d7 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -7974,11 +7974,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); + } } @@ -8084,7 +8089,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, @@ -8244,24 +8249,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); } @@ -8287,12 +8301,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")); @@ -8314,6 +8322,8 @@ remoteStreamEventAddCallback(virStreamPtr st, privst->cbFree = ff; privst->cbEvents = events; + remoteStreamEventTimerUpdate(privst); + ret = 0; cleanup: @@ -8331,12 +8341,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")); @@ -9018,6 +9022,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 @@ -9665,8 +9709,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; } @@ -9685,7 +9729,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 */ @@ -9693,7 +9737,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; @@ -9705,19 +9749,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; @@ -9725,7 +9769,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)) { @@ -9734,16 +9778,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; } } @@ -10509,7 +10553,7 @@ static virDriver remote_driver = { remoteDomainRevertToSnapshot, /* domainRevertToSnapshot */ remoteDomainSnapshotDelete, /* domainSnapshotDelete */ remoteQemuDomainMonitorCommand, /* qemuDomainMonitorCommand */ - NULL, /* domainOpenConsole */ + remoteDomainOpenConsole, /* domainOpenConsole */ }; static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.c b/src/remote/remote_protocol.c index 2483004..0a3b914 100644 --- a/src/remote/remote_protocol.c +++ b/src/remote/remote_protocol.c @@ -3588,6 +3588,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 afe9287..1b90d21 100644 --- a/src/remote/remote_protocol.h +++ b/src/remote/remote_protocol.h @@ -2033,6 +2033,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 @@ -2233,6 +2240,7 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_GET_BLOCK_INFO = 194, REMOTE_PROC_DOMAIN_EVENT_IO_ERROR_REASON = 195, REMOTE_PROC_DOMAIN_CREATE_WITH_FLAGS = 196, + REMOTE_PROC_DOMAIN_OPEN_CONSOLE = 197, }; typedef enum remote_procedure remote_procedure; @@ -2594,6 +2602,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*); @@ -2929,6 +2938,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 8af469c..5d10901 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1798,6 +1798,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. -----*/ @@ -2020,7 +2025,8 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_SNAPSHOT_DELETE = 193, REMOTE_PROC_DOMAIN_GET_BLOCK_INFO = 194, REMOTE_PROC_DOMAIN_EVENT_IO_ERROR_REASON = 195, - REMOTE_PROC_DOMAIN_CREATE_WITH_FLAGS = 196 + REMOTE_PROC_DOMAIN_CREATE_WITH_FLAGS = 196, + REMOTE_PROC_DOMAIN_OPEN_CONSOLE = 197 /* * Notice how the entries are grouped in sets of 10 ? -- 1.7.2.1
On 08/17/2010 11:02 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/remote/remote_driver.c | 108 ++++++++++++++++++++++++---------- src/remote/remote_protocol.c | 13 ++++ src/remote/remote_protocol.h | 10 +++ src/remote/remote_protocol.x | 8 ++- 8 files changed, 172 insertions(+), 33 deletions(-)
No change to src/remote_protocol-structs? Install the dwarves package; this will double-check that you aren't breaking any existing APIs, but it will flag that this new call is an API addition worthy of an update to src/remote_protocol-structs.
@@ -9665,8 +9709,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);
Quite a few conversions from VIR_WARN to VIR_DEBUG in this patch. Should they be split into a separate patch, since they are independent of the new command plumbing? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Thu, Aug 19, 2010 at 02:29:39PM -0600, Eric Blake wrote:
On 08/17/2010 11:02 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/remote/remote_driver.c | 108 ++++++++++++++++++++++++---------- src/remote/remote_protocol.c | 13 ++++ src/remote/remote_protocol.h | 10 +++ src/remote/remote_protocol.x | 8 ++- 8 files changed, 172 insertions(+), 33 deletions(-)
No change to src/remote_protocol-structs? Install the dwarves package; this will double-check that you aren't breaking any existing APIs, but it will flag that this new call is an API addition worthy of an update to src/remote_protocol-structs.
Fixed that. I was in the habit of doing 'cd tests && make check' to avoid the wait for gnulib tests.
@@ -9665,8 +9709,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);
Quite a few conversions from VIR_WARN to VIR_DEBUG in this patch. Should they be split into a separate patch, since they are independent of the new command plumbing?
Yep, split them out to a separate patch Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
This provides an implementation of the virDomainOpenConsole API with the QEMU driver. For the streams code, this reuses most of the code previously added for the tunnelled migration streams since it is generic. * src/qemu/qemu_driver.c: Support virDomainOpenConsole --- src/qemu/qemu_driver.c | 267 +++++++++++++++++++++++++++++++++++++---------- 1 files changed, 210 insertions(+), 57 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 397bd8a..0e0e924 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10158,7 +10158,7 @@ static void qemuDomainEventQueue(struct qemud_driver *driver, /* Migration support. */ /* Tunnelled migration stream support */ -struct qemuStreamMigFile { +struct qemuStreamFD { int fd; int watch; @@ -10169,10 +10169,10 @@ struct qemuStreamMigFile { virFreeCallback ff; }; -static int qemuStreamMigRemoveCallback(virStreamPtr stream) +static int qemuStreamFDRemoveCallback(virStreamPtr stream) { struct qemud_driver *driver = stream->conn->privateData; - struct qemuStreamMigFile *qemust = stream->privateData; + struct qemuStreamFD *qemust = stream->privateData; int ret = -1; if (!qemust) { @@ -10206,10 +10206,10 @@ cleanup: return ret; } -static int qemuStreamMigUpdateCallback(virStreamPtr stream, int events) +static int qemuStreamFDUpdateCallback(virStreamPtr stream, int events) { struct qemud_driver *driver = stream->conn->privateData; - struct qemuStreamMigFile *qemust = stream->privateData; + struct qemuStreamFD *qemust = stream->privateData; int ret = -1; if (!qemust) { @@ -10234,14 +10234,14 @@ cleanup: return ret; } -static void qemuStreamMigEvent(int watch ATTRIBUTE_UNUSED, - int fd ATTRIBUTE_UNUSED, - int events, - void *opaque) +static void qemuStreamFDEvent(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; + struct qemuStreamFD *qemust = stream->privateData; virStreamEventCallback cb; void *cbopaque; virFreeCallback ff; @@ -10268,14 +10268,14 @@ static void qemuStreamMigEvent(int watch ATTRIBUTE_UNUSED, } static int -qemuStreamMigAddCallback(virStreamPtr st, - int events, - virStreamEventCallback cb, - void *opaque, - virFreeCallback ff) +qemuStreamFDAddCallback(virStreamPtr st, + int events, + virStreamEventCallback cb, + void *opaque, + virFreeCallback ff) { struct qemud_driver *driver = st->conn->privateData; - struct qemuStreamMigFile *qemust = st->privateData; + struct qemuStreamFD *qemust = st->privateData; int ret = -1; if (!qemust) { @@ -10293,7 +10293,7 @@ qemuStreamMigAddCallback(virStreamPtr st, if ((qemust->watch = virEventAddHandle(qemust->fd, events, - qemuStreamMigEvent, + qemuStreamFDEvent, st, NULL)) < 0) { qemuReportError(VIR_ERR_INTERNAL_ERROR, @@ -10314,39 +10314,83 @@ cleanup: return ret; } -static void qemuStreamMigFree(struct qemuStreamMigFile *qemust) +static void qemuStreamFDFree(struct qemuStreamFD *qemust) { if (qemust->fd != -1) close(qemust->fd); VIR_FREE(qemust); } -static struct qemuStreamMigFile *qemuStreamMigOpen(virStreamPtr st, - const char *unixfile) +static struct qemuStreamFD *qemuStreamFDOpen(virStreamPtr st, + int fd) { - struct qemuStreamMigFile *qemust = NULL; - struct sockaddr_un sa_qemu; - int i = 0; - int timeout = 3; - int ret; + struct qemuStreamFD *qemust = NULL; if (VIR_ALLOC(qemust) < 0) { virReportOOMError(); return NULL; } - qemust->fd = socket(AF_UNIX, SOCK_STREAM, 0); - if (qemust->fd < 0) - goto cleanup; + if ((st->flags & VIR_STREAM_NONBLOCK) && + virSetNonBlock(fd) < 0) { + virReportSystemError(errno, "%s", + _("Unable to make stream non-blocking")); + VIR_FREE(qemust); + return NULL; + } + qemust->fd = fd; + + return qemust; +} + + +static struct qemuStreamFD *qemuStreamFDOpenDevice(virStreamPtr st, + const char *path) +{ + int fd; + struct qemuStreamFD *ret; + + fd = open(path, O_RDWR); + if (fd < 0) { + virReportSystemError(errno, + _("unable to open %s"), path); + return NULL; + } + + ret = qemuStreamFDOpen(st, fd); + if (!ret) + close(fd); + return ret; +} + + +static struct qemuStreamFD *qemuStreamFDOpenUNIX(virStreamPtr st, + const char *unixfile) +{ + struct sockaddr_un sa_qemu; + int i = 0; + int timeout = 3; + int rv; + int fd; + struct qemuStreamFD *ret = NULL; + + fd = socket(AF_UNIX, SOCK_STREAM, 0); + if (fd < 0) { + virReportSystemError(errno, "%s", + _("unable to open UNIX socket")); + return NULL; + } 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; + if (virStrcpy(sa_qemu.sun_path, unixfile, sizeof(sa_qemu.sun_path)) == NULL) { + close(fd); + return NULL; + } do { - ret = connect(qemust->fd, (struct sockaddr *)&sa_qemu, sizeof(sa_qemu)); - if (ret == 0) + rv = connect(fd, (struct sockaddr *)&sa_qemu, sizeof(sa_qemu)); + if (rv == 0) break; if (errno == ENOENT || errno == ECONNREFUSED) { @@ -10355,31 +10399,28 @@ static struct qemuStreamMigFile *qemuStreamMigOpen(virStreamPtr st, continue; } - goto cleanup; + close(fd); + return NULL; } 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; + ret = qemuStreamFDOpen(st, fd); + if (!ret) + close(fd); + return ret; } static int -qemuStreamMigClose(virStreamPtr st) +qemuStreamFDClose(virStreamPtr st) { struct qemud_driver *driver = st->conn->privateData; - struct qemuStreamMigFile *qemust = st->privateData; + struct qemuStreamFD *qemust = st->privateData; if (!qemust) return 0; qemuDriverLock(driver); - qemuStreamMigFree(qemust); + qemuStreamFDFree(qemust); st->privateData = NULL; @@ -10388,10 +10429,10 @@ qemuStreamMigClose(virStreamPtr st) return 0; } -static int qemuStreamMigWrite(virStreamPtr st, const char *bytes, size_t nbytes) +static int qemuStreamFDWrite(virStreamPtr st, const char *bytes, size_t nbytes) { struct qemud_driver *driver = st->conn->privateData; - struct qemuStreamMigFile *qemust = st->privateData; + struct qemuStreamFD *qemust = st->privateData; int ret; if (!qemust) { @@ -10420,13 +10461,48 @@ retry: return ret; } -static virStreamDriver qemuStreamMigDrv = { - .streamSend = qemuStreamMigWrite, - .streamFinish = qemuStreamMigClose, - .streamAbort = qemuStreamMigClose, - .streamAddCallback = qemuStreamMigAddCallback, - .streamUpdateCallback = qemuStreamMigUpdateCallback, - .streamRemoveCallback = qemuStreamMigRemoveCallback + +static int qemuStreamFDRead(virStreamPtr st, char *bytes, size_t nbytes) +{ + struct qemud_driver *driver = st->conn->privateData; + struct qemuStreamFD *qemust = st->privateData; + int ret; + + if (!qemust) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("stream is not open")); + return -1; + } + + qemuDriverLock(driver); + +retry: + ret = read(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 qemuStreamFDDrv = { + .streamSend = qemuStreamFDWrite, + .streamRecv = qemuStreamFDRead, + .streamFinish = qemuStreamFDClose, + .streamAbort = qemuStreamFDClose, + .streamAddCallback = qemuStreamFDAddCallback, + .streamUpdateCallback = qemuStreamFDUpdateCallback, + .streamRemoveCallback = qemuStreamFDRemoveCallback }; /* Prepare is the first step, and it runs on the destination host. @@ -10451,7 +10527,7 @@ qemudDomainMigratePrepareTunnel(virConnectPtr dconn, int internalret; char *unixfile = NULL; unsigned long long qemuCmdFlags; - struct qemuStreamMigFile *qemust = NULL; + struct qemuStreamFD *qemust = NULL; qemuDomainObjPrivatePtr priv = NULL; struct timeval now; @@ -10557,7 +10633,7 @@ qemudDomainMigratePrepareTunnel(virConnectPtr dconn, goto endjob; } - qemust = qemuStreamMigOpen(st, unixfile); + qemust = qemuStreamFDOpenUNIX(st, unixfile); if (qemust == NULL) { qemudShutdownVMDaemon(driver, vm, 0); if (!vm->persistent) { @@ -10571,7 +10647,7 @@ qemudDomainMigratePrepareTunnel(virConnectPtr dconn, goto endjob; } - st->driver = &qemuStreamMigDrv; + st->driver = &qemuStreamFDDrv; st->privateData = qemust; event = virDomainEventNewFromObj(vm, @@ -12646,6 +12722,83 @@ cleanup: return ret; } + +static int +qemuDomainOpenConsole(virDomainPtr dom, + const char *devname, + virStreamPtr st, + unsigned int flags ATTRIBUTE_UNUSED) +{ + struct qemud_driver *driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + int ret = -1; + int i; + virDomainChrDefPtr chr = NULL; + struct qemuStreamFD *qemust; + + 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; + } + + qemust = qemuStreamFDOpenDevice(st, chr->data.file.path); + if (qemust == NULL) + goto cleanup; + + st->driver = &qemuStreamFDDrv; + st->privateData = qemust; + + ret = 0; +cleanup: + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + return ret; +} + + static virDriver qemuDriver = { VIR_DRV_QEMU, "QEMU", @@ -12746,7 +12899,7 @@ static virDriver qemuDriver = { qemuDomainRevertToSnapshot, /* domainRevertToSnapshot */ qemuDomainSnapshotDelete, /* domainSnapshotDelete */ qemuDomainMonitorCommand, /* qemuDomainMonitorCommand */ - NULL, /* domainOpenConsole */ + qemuDomainOpenConsole, /* domainOpenConsole */ }; -- 1.7.2.1
On 08/17/2010 11:02 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 | 267 +++++++++++++++++++++++++++++++++++++---------- 1 files changed, 210 insertions(+), 57 deletions(-)
+static int qemuStreamFDRead(virStreamPtr st, char *bytes, size_t nbytes) +{
...
+ } else { + ret = -1; + virReportSystemError(errno, "%s", + _("cannot write to stream"));
s/write to/read from/
+ +static int +qemuDomainOpenConsole(virDomainPtr dom, + const char *devname, + virStreamPtr st, + unsigned int flags ATTRIBUTE_UNUSED)
Drop the attribute...
+{ + struct qemud_driver *driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + int ret = -1; + int i; + virDomainChrDefPtr chr = NULL; + struct qemuStreamFD *qemust;
...and add virCheckFlags(0, -1) here.
+ 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]; + }
Missing the check for vm->dev->nparallels when devname is NULL and there is no console or serial devices? ACK with those nits fixed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Thu, Aug 19, 2010 at 02:44:27PM -0600, Eric Blake wrote:
On 08/17/2010 11:02 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 | 267 +++++++++++++++++++++++++++++++++++++---------- 1 files changed, 210 insertions(+), 57 deletions(-)
+static int qemuStreamFDRead(virStreamPtr st, char *bytes, size_t nbytes) +{
...
+ } else { + ret = -1; + virReportSystemError(errno, "%s", + _("cannot write to stream"));
s/write to/read from/
+ +static int +qemuDomainOpenConsole(virDomainPtr dom, + const char *devname, + virStreamPtr st, + unsigned int flags ATTRIBUTE_UNUSED)
Drop the attribute...
+{ + struct qemud_driver *driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + int ret = -1; + int i; + virDomainChrDefPtr chr = NULL; + struct qemuStreamFD *qemust;
...and add virCheckFlags(0, -1) here.
+ 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]; + }
Missing the check for vm->dev->nparallels when devname is NULL and there is no console or serial devices?
The intended semantics are that if no alias is provided, then this will open the first <console> element. Most of the time this maps to the first (only) vm->def->console. Due to historic wierdness though, with a HVM guest this will instead sometimes point to vm->def->serials[0]. When I fix that mess, the 'else if' clause can be dropped here. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
This 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 --- tools/Makefile.am | 1 + tools/console.c | 330 ++++++++++++++++++++++++++++++++++++++++------------- tools/console.h | 2 +- tools/virsh.c | 76 ++++--------- 4 files changed, 274 insertions(+), 135 deletions(-) diff --git a/tools/Makefile.am b/tools/Makefile.am index fd05e8b..9550e25 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -32,6 +32,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..7077c5e 100644 --- a/tools/console.c +++ b/tools/console.c @@ -35,14 +35,39 @@ # include <unistd.h> # include <signal.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; + int 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 +86,192 @@ 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 = 1; + 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 = 1; + 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) { + virErrorPtr err = virGetLastError(); + con->quit = 1; + 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 = 1; + } +} + +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 = 1; + 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 = 1; + } + return; + } + if (got == 0) { + con->quit = 1; + return; + } + if (con->terminalToStream.data[con->terminalToStream.offset] == CTRL_CLOSE_BRACKET) { + con->quit = 1; + 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 = 1; + } +} + +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 = 1; + } + 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 = 1; + } +} + + +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 c0ee3ee..699d386 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; @@ -615,36 +616,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; @@ -655,38 +636,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; } @@ -696,6 +651,7 @@ cmdConsole(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom; int ret; + const char *devname; if (!vshConnectionUsability(ctl, ctl->conn)) return FALSE; @@ -703,7 +659,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; @@ -1180,7 +1138,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 { @@ -1343,7 +1301,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)); @@ -10556,6 +10514,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.1
On 08/17/2010 11:02 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 --- tools/Makefile.am | 1 + tools/console.c | 330 ++++++++++++++++++++++++++++++++++++++++------------- tools/console.h | 2 +- tools/virsh.c | 76 ++++--------- 4 files changed, 274 insertions(+), 135 deletions(-)
+ +struct virConsoleBuffer { + size_t length; + size_t offset;
s/size_t/off_t/ for offset? (but see below)
+ char *data; +}; + +typedef struct virConsole virConsole; +typedef virConsole *virConsolePtr; +struct virConsole { + virStreamPtr st; + int quit;
s/int/bool/, and adjust clients to use false/true instead of 0/1
+ + 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 +86,192 @@ 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;
Oh, never mind. size_t for offset is probably just fine after all.
+ int got; + + if (avail < 1024) { + if (VIR_REALLOC_N(con->streamToTerminal.data, + con->streamToTerminal.length + 1024) < 0) {
/me Note to self - another VIR_EXPAND_N or VIR_RESIZE_N candidate.
+ virReportOOMError(); + con->quit = 1; + 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 = 1; + 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) { + virErrorPtr err = virGetLastError(); + con->quit = 1; + 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) + {}
/me Note to self - a VIR_SHRINK_N candidate.
+ 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 = 1; + } +} + +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 = 1; + 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 = 1; + } + return; + } + if (got == 0) { + con->quit = 1; + return; + } + if (con->terminalToStream.data[con->terminalToStream.offset] == CTRL_CLOSE_BRACKET) { + con->quit = 1; + 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 = 1; + } +} + +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);
Do we want to use safewrite here? Conditional ACK, if those are deemed easy nits to fix. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Thu, Aug 19, 2010 at 03:09:45PM -0600, Eric Blake wrote:
On 08/17/2010 11:02 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
+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);
Do we want to use safewrite here?
All I/O in this is now done non-blocking, so don't actually want to block on writing the entire buffer. The code expect to see partial writes & will handle those in a suitable manner, going back to sleep in the event loop 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 08/20/2010 03:10 AM, Daniel P. Berrange wrote:
+ done = write(fd, + con->streamToTerminal.data, + con->streamToTerminal.offset);
Do we want to use safewrite here?
All I/O in this is now done non-blocking, so don't actually want to block on writing the entire buffer. The code expect to see partial writes & will handle those in a suitable manner, going back to sleep in the event loop
Good point (I was just comparing it to the old code, thinking it was straight code motion; but it was also a conversion in I/O styles). You've answered my question, so that only leaves my suggestion on making virConsole.quit a bool instead of int; ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, Aug 17, 2010 at 06:02:45PM +0100, 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
I've realized one problem with this patch. It breaks virsh console for Xen & LXC, since I've not added virDomainOpenConsole to those drivers yet. Supporting them is easy, but it would be a big cut+paste of the QEMU driver code, so I think I need to re-work this to pull out the QEMU stream code into a reusable module for other (local) drivers to share. ESX would of course need separate code, since that's a remote driver only. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
2010/8/23 Daniel P. Berrange <berrange@redhat.com>:
On Tue, Aug 17, 2010 at 06:02:45PM +0100, 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
I've realized one problem with this patch. It breaks virsh console for Xen & LXC, since I've not added virDomainOpenConsole to those drivers yet.
Supporting them is easy, but it would be a big cut+paste of the QEMU driver code, so I think I need to re-work this to pull out the QEMU stream code into a reusable module for other (local) drivers to share. ESX would of course need separate code, since that's a remote driver only.
ESX 4.0 and before can redirect a virtual serial device to file in the datastore or to a physical serial device only. One could poll for this file in the datastore via HTTPS, but that's quite ugly and I'm not sure that it would work in a reasonable way. Also this would only cover the read part, I'm not sure how to do writing that way. ESX also allows to connect two guests via a virtual null modem cable, but that would require a management guest per ESX server that runs a proxy for the virtual serial devices of other guests. Neither of this ideas sounds reasonable or even doable, so probably no virDomainOpenConsole for ESX <= 4.0. Since version 4.1 ESX can export a virtual serial device via Telnet, Telnet+SSL, TCP and TCP+SSL. This will probably allow the ESX driver to support virDomainOpenConsole for ESX >= 4.1. I already tested this manually using an SSL enabled Telnet client for the Telnet+SSL transport and gnutls-cli for the TCP+SSL transport and it works. Matthias
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.1
On 08/17/2010 11:02 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(-)
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);
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, 17 Aug 2010, Daniel P. Berrange wrote:
The 'virsh console' command has been an oddity that only works when run locally, as the same UID as the QEMU instance. This is because it directly opens /dev/pty/XXX. This introduces a formal API for accessing consoles that uses the virStreamPtr APIs. Now any app can open consoles anywhere it can connect to libvirt
This and the patches look LOVELY --- thank you We presently need to involve an admin level staffer to get onto the hosting box in question to be able to see the OOM, kernel, and other messages leaking out, and indeed to try to connect to a wedged instance. We had it happen just last week Getting network socket transport will solve a lot for us. Some authentication layer (PKI key mediated access comes to mind, similar to keyed SSH access), or ACL's to permit exposing specific consoles to specific end customers would close the loop -- Russ herrold
On 17-08-2010 19:02, Daniel P. Berrange wrote:
The 'virsh console' command has been an oddity that only works when run locally, as the same UID as the QEMU instance. This is because it directly opens /dev/pty/XXX. This introduces a formal API for accessing consoles that uses the virStreamPtr APIs. Now any app can open consoles anywhere it can connect to libvirt
I don't (right now, at least) have any comments on the patches themselves, but I can't help but wonder what other wonderful improvements you've got in your pipeline. I spent at least a couple of hours on something like this a couple of weeks ago, but had I known that you were already doing it, I wouldn't have wasted my time. So, in an effort to not duplicate efforts, perhaps everyone who's working on something reasonably big (certainly stuff like this, but also smaller change sets) could put a list up somewhere for all to see? Perhaps such a list already exists and I just don't know about it? -- Soren Hansen Ubuntu Developer http://www.ubuntu.com/
On Wed, Aug 18, 2010 at 12:53:41PM +0200, Soren Hansen wrote:
On 17-08-2010 19:02, Daniel P. Berrange wrote:
The 'virsh console' command has been an oddity that only works when run locally, as the same UID as the QEMU instance. This is because it directly opens /dev/pty/XXX. This introduces a formal API for accessing consoles that uses the virStreamPtr APIs. Now any app can open consoles anywhere it can connect to libvirt
I don't (right now, at least) have any comments on the patches themselves, but I can't help but wonder what other wonderful improvements you've got in your pipeline. I spent at least a couple of hours on something like this a couple of weeks ago, but had I known that you were already doing it, I wouldn't have wasted my time.
So, in an effort to not duplicate efforts, perhaps everyone who's working on something reasonably big (certainly stuff like this, but also smaller change sets) could put a list up somewhere for all to see? Perhaps such a list already exists and I just don't know about it?
I'm in the process of working through all the RFE bugs against libvirt to put together a semi-formal 'roadmap' that we can publish. People could then put their names against items if they intend todo them. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (5)
-
Daniel P. Berrange -
Eric Blake -
Matthias Bolte -
R P Herrold -
Soren Hansen