12:02 p.m.
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
12:02 p.m.
New subject: [libvirt] [PATCH 1/7] Change calling conventions in remote driver client internals
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
2:50 p.m.
New subject: [libvirt] [PATCH 1/7] Change calling conventions in remote driver client internals
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4 a.m.
New subject: [libvirt] [PATCH 1/7] Change calling conventions in remote driver client internals
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 :|
12:02 p.m.
New subject: [libvirt] [PATCH 2/7] Support callbacks on virStream APIs in remote driver client
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
3:14 p.m.
New subject: [libvirt] [PATCH 2/7] Support callbacks on virStream APIs in remote driver client
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4:07 a.m.
New subject: [libvirt] [PATCH 2/7] Support callbacks on virStream APIs in remote driver client
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 :|
9:07 a.m.
New subject: [libvirt] [PATCH 2/7] Support callbacks on virStream APIs in remote driver client
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:02 p.m.
New subject: [libvirt] [PATCH 3/7] Introduce a virDomainOpenConsole API
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
3:21 p.m.
New subject: [libvirt] [PATCH 3/7] Introduce a virDomainOpenConsole API
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
7:20 a.m.
New subject: [libvirt] [PATCH 3/7] Introduce a virDomainOpenConsole API
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/
12:02 p.m.
New subject: [libvirt] [PATCH 4/7] Remote driver client and server for virDomainOpenConsole
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
3:29 p.m.
New subject: [libvirt] [PATCH 4/7] Remote driver client and server for virDomainOpenConsole
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
9:47 a.m.
New subject: [libvirt] [PATCH 4/7] Remote driver client and server for virDomainOpenConsole
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 :|
12:02 p.m.
New subject: [libvirt] [PATCH 5/7] Support virDomainOpenConsole with QEMU
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
3:44 p.m.
New subject: [libvirt] [PATCH 5/7] Support virDomainOpenConsole with QEMU
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
8:39 a.m.
New subject: [libvirt] [PATCH 5/7] Support virDomainOpenConsole with QEMU
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 :|
12:02 p.m.
New subject: [libvirt] [PATCH 6/7] Re-write virsh console to use streams
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
4:09 p.m.
New subject: [libvirt] [PATCH 6/7] Re-write virsh console to use streams
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4:10 a.m.
New subject: [libvirt] [PATCH 6/7] Re-write virsh console to use streams
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 :|
9:10 a.m.
New subject: [libvirt] [PATCH 6/7] Re-write virsh console to use streams
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
8:53 a.m.
New subject: [libvirt] [PATCH 6/7] Re-write virsh console to use streams
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 :|
10:43 a.m.
New subject: [libvirt] [PATCH 6/7] Re-write virsh console to use streams
2010/8/23 Daniel P. Berrange <berrange(a)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
12:02 p.m.
New subject: [libvirt] [PATCH 7/7] Fix busy-wait loop on closed file descriptor
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
4:14 p.m.
New subject: [libvirt] [PATCH 7/7] Fix busy-wait loop on closed file descriptor
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
1:31 p.m.
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
5:53 a.m.
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/
4:13 a.m.
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 :|
5203
days inactive
5211
days old
27 comments
5 participants
participants (5)
-
Daniel P. Berrange -
Eric Blake -
Matthias Bolte -
R P Herrold -
Soren Hansen