* [Qemu-devel] [RFC][PATCH v5 01/21] Move code related to fd handlers into utility functions
2010-12-03 18:03 [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent Michael Roth
@ 2010-12-03 18:03 ` Michael Roth
2010-12-07 13:31 ` [Qemu-devel] " Jes Sorensen
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 02/21] Add qemu_set_fd_handler() wrappers to qemu-tools.c Michael Roth
` (21 subsequent siblings)
22 siblings, 1 reply; 74+ messages in thread
From: Michael Roth @ 2010-12-03 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: agl, stefanha, Jes.Sorensen, mdroth, aliguori, ryanh, abeekhof
This allows us to implement an i/o loop outside of vl.c that can
interact with objects that use qemu_set_fd_handler()
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
Makefile.objs | 2 +-
qemu-char.h | 4 ++
qemu-ioh.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
qemu-ioh.h | 34 +++++++++++++++++
vl.c | 86 ++++++++----------------------------------
5 files changed, 170 insertions(+), 71 deletions(-)
create mode 100644 qemu-ioh.c
create mode 100644 qemu-ioh.h
diff --git a/Makefile.objs b/Makefile.objs
index 23b17ce..403bbb0 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -14,7 +14,7 @@ oslib-obj-$(CONFIG_POSIX) += oslib-posix.o
# block-obj-y is code used by both qemu system emulation and qemu-img
block-obj-y = cutils.o cache-utils.o qemu-malloc.o qemu-option.o module.o
-block-obj-y += nbd.o block.o aio.o aes.o qemu-config.o
+block-obj-y += nbd.o block.o aio.o aes.o qemu-config.o qemu-ioh.o
block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
diff --git a/qemu-char.h b/qemu-char.h
index 18ad12b..9809ef6 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -6,6 +6,7 @@
#include "qemu-option.h"
#include "qemu-config.h"
#include "qobject.h"
+#include "qemu-ioh.h"
/* character device */
@@ -111,4 +112,7 @@ int qemu_set_fd_handler(int fd,
IOHandler *fd_read,
IOHandler *fd_write,
void *opaque);
+void qemu_get_fdset(int *nfds, fd_set *rfds, fd_set *wfds, fd_set *xfds);
+void qemu_process_fd_handlers(const fd_set *rfds, const fd_set *wfds,
+ const fd_set *xfds);
#endif
diff --git a/qemu-ioh.c b/qemu-ioh.c
new file mode 100644
index 0000000..cc71470
--- /dev/null
+++ b/qemu-ioh.c
@@ -0,0 +1,115 @@
+/*
+ * QEMU System Emulator
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#include "qemu-ioh.h"
+#include "qlist.h"
+
+/* XXX: fd_read_poll should be suppressed, but an API change is
+ necessary in the character devices to suppress fd_can_read(). */
+int qemu_set_fd_handler3(void *ioh_record_list,
+ int fd,
+ IOCanReadHandler *fd_read_poll,
+ IOHandler *fd_read,
+ IOHandler *fd_write,
+ void *opaque)
+{
+ QLIST_HEAD(, IOHandlerRecord) *io_handlers_ptr = ioh_record_list;
+ IOHandlerRecord *ioh;
+
+ if (!fd_read && !fd_write) {
+ QLIST_FOREACH(ioh, io_handlers_ptr, next) {
+ if (ioh->fd == fd) {
+ ioh->deleted = 1;
+ break;
+ }
+ }
+ } else {
+ QLIST_FOREACH(ioh, io_handlers_ptr, next) {
+ if (ioh->fd == fd)
+ goto found;
+ }
+ ioh = qemu_mallocz(sizeof(IOHandlerRecord));
+ QLIST_INSERT_HEAD(io_handlers_ptr, ioh, next);
+ found:
+ ioh->fd = fd;
+ ioh->fd_read_poll = fd_read_poll;
+ ioh->fd_read = fd_read;
+ ioh->fd_write = fd_write;
+ ioh->opaque = opaque;
+ ioh->deleted = 0;
+ }
+ return 0;
+}
+
+/* add entries from ioh record list to fd sets. nfds and fd sets
+ * should be cleared/reset by caller if desired. set a particular
+ * fdset to NULL to ignore fd events of that type
+ */
+void qemu_get_fdset2(void *ioh_record_list, int *nfds, fd_set *rfds,
+ fd_set *wfds, fd_set *xfds)
+{
+ QLIST_HEAD(, IOHandlerRecord) *io_handlers = ioh_record_list;
+ IOHandlerRecord *ioh;
+
+ QLIST_FOREACH(ioh, io_handlers, next) {
+ if (ioh->deleted)
+ continue;
+ if ((rfds != NULL && ioh->fd_read) &&
+ (!ioh->fd_read_poll ||
+ ioh->fd_read_poll(ioh->opaque) != 0)) {
+ FD_SET(ioh->fd, rfds);
+ if (ioh->fd > *nfds)
+ *nfds = ioh->fd;
+ }
+ if (wfds != NULL && ioh->fd_write) {
+ FD_SET(ioh->fd, wfds);
+ if (ioh->fd > *nfds)
+ *nfds = ioh->fd;
+ }
+ }
+}
+
+/* execute registered handlers for r/w events in the provided fdsets. unset
+ * handlers are cleaned up here as well
+ */
+void qemu_process_fd_handlers2(void *ioh_record_list, const fd_set *rfds,
+ const fd_set *wfds, const fd_set *xfds)
+{
+ QLIST_HEAD(, IOHandlerRecord) *io_handlers = ioh_record_list;
+ IOHandlerRecord *ioh, *pioh;
+
+ QLIST_FOREACH_SAFE(ioh, io_handlers, next, pioh) {
+ if (!ioh->deleted && ioh->fd_read && FD_ISSET(ioh->fd, rfds)) {
+ ioh->fd_read(ioh->opaque);
+ }
+ if (!ioh->deleted && ioh->fd_write && FD_ISSET(ioh->fd, wfds)) {
+ ioh->fd_write(ioh->opaque);
+ }
+
+ /* Do this last in case read/write handlers marked it for deletion */
+ if (ioh->deleted) {
+ QLIST_REMOVE(ioh, next);
+ qemu_free(ioh);
+ }
+ }
+}
diff --git a/qemu-ioh.h b/qemu-ioh.h
new file mode 100644
index 0000000..7c6e833
--- /dev/null
+++ b/qemu-ioh.h
@@ -0,0 +1,34 @@
+#ifndef QEMU_IOH_H
+#define QEMU_IOH_H
+
+#include "qemu-common.h"
+#include "qlist.h"
+
+/* common i/o loop definitions */
+
+typedef struct IOHandlerRecord {
+ int fd;
+ IOCanReadHandler *fd_read_poll;
+ IOHandler *fd_read;
+ IOHandler *fd_write;
+ int deleted;
+ void *opaque;
+ /* temporary data */
+ struct pollfd *ufd;
+ QLIST_ENTRY(IOHandlerRecord) next;
+} IOHandlerRecord;
+
+/* XXX: fd_read_poll should be suppressed, but an API change is
+ necessary in the character devices to suppress fd_can_read(). */
+int qemu_set_fd_handler3(void *io_handlers_ptr,
+ int fd,
+ IOCanReadHandler *fd_read_poll,
+ IOHandler *fd_read,
+ IOHandler *fd_write,
+ void *opaque);
+void qemu_get_fdset2(void *ioh_record_list, int *nfds, fd_set *rfds,
+ fd_set *wfds, fd_set *xfds);
+void qemu_process_fd_handlers2(void *ioh_record_list, const fd_set *rfds,
+ const fd_set *wfds, const fd_set *xfds);
+
+#endif
diff --git a/vl.c b/vl.c
index 805e11f..d7024bc 100644
--- a/vl.c
+++ b/vl.c
@@ -148,6 +148,7 @@ int main(int argc, char **argv)
#include "qemu-config.h"
#include "qemu-objects.h"
#include "qemu-options.h"
+#include "qemu-ioh.h"
#ifdef CONFIG_VIRTFS
#include "fsdev/qemu-fsdev.h"
#endif
@@ -914,18 +915,6 @@ void pcmcia_info(Monitor *mon)
/***********************************************************/
/* I/O handling */
-typedef struct IOHandlerRecord {
- int fd;
- IOCanReadHandler *fd_read_poll;
- IOHandler *fd_read;
- IOHandler *fd_write;
- int deleted;
- void *opaque;
- /* temporary data */
- struct pollfd *ufd;
- QLIST_ENTRY(IOHandlerRecord) next;
-} IOHandlerRecord;
-
static QLIST_HEAD(, IOHandlerRecord) io_handlers =
QLIST_HEAD_INITIALIZER(io_handlers);
@@ -938,31 +927,8 @@ int qemu_set_fd_handler2(int fd,
IOHandler *fd_write,
void *opaque)
{
- IOHandlerRecord *ioh;
-
- if (!fd_read && !fd_write) {
- QLIST_FOREACH(ioh, &io_handlers, next) {
- if (ioh->fd == fd) {
- ioh->deleted = 1;
- break;
- }
- }
- } else {
- QLIST_FOREACH(ioh, &io_handlers, next) {
- if (ioh->fd == fd)
- goto found;
- }
- ioh = qemu_mallocz(sizeof(IOHandlerRecord));
- QLIST_INSERT_HEAD(&io_handlers, ioh, next);
- found:
- ioh->fd = fd;
- ioh->fd_read_poll = fd_read_poll;
- ioh->fd_read = fd_read;
- ioh->fd_write = fd_write;
- ioh->opaque = opaque;
- ioh->deleted = 0;
- }
- return 0;
+ return qemu_set_fd_handler3(&io_handlers, fd, fd_read_poll, fd_read,
+ fd_write, opaque);
}
int qemu_set_fd_handler(int fd,
@@ -973,6 +939,17 @@ int qemu_set_fd_handler(int fd,
return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
}
+void qemu_get_fdset(int *nfds, fd_set *rfds, fd_set *wfds, fd_set *xfds)
+{
+ return qemu_get_fdset2(&io_handlers, nfds, rfds, wfds, xfds);
+}
+
+void qemu_process_fd_handlers(const fd_set *rfds, const fd_set *wfds,
+ const fd_set *xfds)
+{
+ return qemu_process_fd_handlers2(&io_handlers, rfds, wfds, xfds);
+}
+
/***********************************************************/
/* machine registration */
@@ -1202,7 +1179,6 @@ void qemu_system_powerdown_request(void)
void main_loop_wait(int nonblocking)
{
- IOHandlerRecord *ioh;
fd_set rfds, wfds, xfds;
int ret, nfds;
struct timeval tv;
@@ -1223,22 +1199,7 @@ void main_loop_wait(int nonblocking)
FD_ZERO(&rfds);
FD_ZERO(&wfds);
FD_ZERO(&xfds);
- QLIST_FOREACH(ioh, &io_handlers, next) {
- if (ioh->deleted)
- continue;
- if (ioh->fd_read &&
- (!ioh->fd_read_poll ||
- ioh->fd_read_poll(ioh->opaque) != 0)) {
- FD_SET(ioh->fd, &rfds);
- if (ioh->fd > nfds)
- nfds = ioh->fd;
- }
- if (ioh->fd_write) {
- FD_SET(ioh->fd, &wfds);
- if (ioh->fd > nfds)
- nfds = ioh->fd;
- }
- }
+ qemu_get_fdset(&nfds, &rfds, &wfds, &xfds);
tv.tv_sec = timeout / 1000;
tv.tv_usec = (timeout % 1000) * 1000;
@@ -1249,22 +1210,7 @@ void main_loop_wait(int nonblocking)
ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv);
qemu_mutex_lock_iothread();
if (ret > 0) {
- IOHandlerRecord *pioh;
-
- QLIST_FOREACH_SAFE(ioh, &io_handlers, next, pioh) {
- if (!ioh->deleted && ioh->fd_read && FD_ISSET(ioh->fd, &rfds)) {
- ioh->fd_read(ioh->opaque);
- }
- if (!ioh->deleted && ioh->fd_write && FD_ISSET(ioh->fd, &wfds)) {
- ioh->fd_write(ioh->opaque);
- }
-
- /* Do this last in case read/write handlers marked it for deletion */
- if (ioh->deleted) {
- QLIST_REMOVE(ioh, next);
- qemu_free(ioh);
- }
- }
+ qemu_process_fd_handlers(&rfds, &wfds, &xfds);
}
slirp_select_poll(&rfds, &wfds, &xfds, (ret < 0));
--
1.7.0.4
^ permalink raw reply related [flat|nested] 74+ messages in thread* [Qemu-devel] Re: [RFC][PATCH v5 01/21] Move code related to fd handlers into utility functions
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 01/21] Move code related to fd handlers into utility functions Michael Roth
@ 2010-12-07 13:31 ` Jes Sorensen
2010-12-07 14:48 ` Michael Roth
0 siblings, 1 reply; 74+ messages in thread
From: Jes Sorensen @ 2010-12-07 13:31 UTC (permalink / raw)
To: Michael Roth; +Cc: agl, stefanha, abeekhof, qemu-devel, aliguori, ryanh
On 12/03/10 19:03, Michael Roth wrote:
> This allows us to implement an i/o loop outside of vl.c that can
> interact with objects that use qemu_set_fd_handler()
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
This commit message really tells us nothing. Please be more specific
about what is in the commit.
> diff --git a/qemu-ioh.c b/qemu-ioh.c
> new file mode 100644
> index 0000000..cc71470
> --- /dev/null
> +++ b/qemu-ioh.c
> @@ -0,0 +1,115 @@
> +/*
> + * QEMU System Emulator
> + *
> + * Copyright (c) 2003-2008 Fabrice Bellard
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
Is this moved or new code? If the former, fine, but if it is new code,
you might want to leave your own name on the (c). I presume at least
some of the changes are (c) 2010?
> +/* XXX: fd_read_poll should be suppressed, but an API change is
> + necessary in the character devices to suppress fd_can_read(). */
XXX in the comment isn't really of much use. Please make it more
explicit, or put your name in if it is a statement you wish to make.
> +int qemu_set_fd_handler3(void *ioh_record_list,
> + int fd,
> + IOCanReadHandler *fd_read_poll,
> + IOHandler *fd_read,
> + IOHandler *fd_write,
> + void *opaque)
I am not happy with this addition of numbers to these functions, it
doesn't tell us why we have a 3 and how it differs from 2. If 3 is
really the backend for implementing 2, maybe it would be better to name
it __qemu_set_fd_handler2() and then have macros/wrappers calling into it.
Cheers,
Jes
^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [Qemu-devel] Re: [RFC][PATCH v5 01/21] Move code related to fd handlers into utility functions
2010-12-07 13:31 ` [Qemu-devel] " Jes Sorensen
@ 2010-12-07 14:48 ` Michael Roth
2010-12-07 15:02 ` Jes Sorensen
0 siblings, 1 reply; 74+ messages in thread
From: Michael Roth @ 2010-12-07 14:48 UTC (permalink / raw)
To: Jes Sorensen; +Cc: agl, stefanha, abeekhof, qemu-devel, aliguori, ryanh
On 12/07/2010 07:31 AM, Jes Sorensen wrote:
> On 12/03/10 19:03, Michael Roth wrote:
>> This allows us to implement an i/o loop outside of vl.c that can
>> interact with objects that use qemu_set_fd_handler()
>>
>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>
> This commit message really tells us nothing. Please be more specific
> about what is in the commit.
>
Currently, in qemu, the virtagent client/server functionality is driven
by vl.c:main_loop_wait(), which implements a select() loop that kicks
off handlers registered for various FDs/events via qemu_set_fd_handler().
To share the code with the agent, qemu-va.c, I re-implemented this i/o
loop to do same thing, along with vl.c:qemu_set_fd_handler*() and
friends. It was big nasty copy/paste job and I think most of the
reviewers agreed that the i/o loop code should be shared.
This commit moves the shared code into back-end utility functions that
get called by vl.c:qemu_set_fd_handler()/qemu_process_fd_handlers() and
friends for qemu, and by
qemu-tools.c:qemu_set_fd_handler()/qemu_process_fd_handlers() for tools.
So now to interact with code that uses qemu_set_fd_handler() you can
built a select() loop around these utility functions.
>> diff --git a/qemu-ioh.c b/qemu-ioh.c
>> new file mode 100644
>> index 0000000..cc71470
>> --- /dev/null
>> +++ b/qemu-ioh.c
>> @@ -0,0 +1,115 @@
>> +/*
>> + * QEMU System Emulator
>> + *
>> + * Copyright (c) 2003-2008 Fabrice Bellard
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>
> Is this moved or new code? If the former, fine, but if it is new code,
> you might want to leave your own name on the (c). I presume at least
> some of the changes are (c) 2010?
>
It's moved out from vl.c, nearly verbatim...
>> +/* XXX: fd_read_poll should be suppressed, but an API change is
>> + necessary in the character devices to suppress fd_can_read(). */
>
> XXX in the comment isn't really of much use. Please make it more
> explicit, or put your name in if it is a statement you wish to make.
>
Even down to the comments :)
>> +int qemu_set_fd_handler3(void *ioh_record_list,
>> + int fd,
>> + IOCanReadHandler *fd_read_poll,
>> + IOHandler *fd_read,
>> + IOHandler *fd_write,
>> + void *opaque)
>
> I am not happy with this addition of numbers to these functions, it
> doesn't tell us why we have a 3 and how it differs from 2. If 3 is
> really the backend for implementing 2, maybe it would be better to name
> it __qemu_set_fd_handler2() and then have macros/wrappers calling into it.
>
That was the initial plan, but qemu_set_fd_handler2() is a back-end of
sorts for qemu_set_fd_handler(), so I was just trying to be consistent
with the naming. Personally I don't have any objections one way or the
other.
> Cheers,
> Jes
>
^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [Qemu-devel] Re: [RFC][PATCH v5 01/21] Move code related to fd handlers into utility functions
2010-12-07 14:48 ` Michael Roth
@ 2010-12-07 15:02 ` Jes Sorensen
2010-12-08 9:15 ` Stefan Hajnoczi
0 siblings, 1 reply; 74+ messages in thread
From: Jes Sorensen @ 2010-12-07 15:02 UTC (permalink / raw)
To: Michael Roth; +Cc: agl, stefanha, abeekhof, qemu-devel, aliguori, ryanh
On 12/07/10 15:48, Michael Roth wrote:
> On 12/07/2010 07:31 AM, Jes Sorensen wrote:
>> On 12/03/10 19:03, Michael Roth wrote:
>>> This allows us to implement an i/o loop outside of vl.c that can
>>> interact with objects that use qemu_set_fd_handler()
>>>
>>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>>
>> This commit message really tells us nothing. Please be more specific
>> about what is in the commit.
>>
>
> Currently, in qemu, the virtagent client/server functionality is driven
> by vl.c:main_loop_wait(), which implements a select() loop that kicks
> off handlers registered for various FDs/events via qemu_set_fd_handler().
>
> To share the code with the agent, qemu-va.c, I re-implemented this i/o
> loop to do same thing, along with vl.c:qemu_set_fd_handler*() and
> friends. It was big nasty copy/paste job and I think most of the
> reviewers agreed that the i/o loop code should be shared.
>
> This commit moves the shared code into back-end utility functions that
> get called by vl.c:qemu_set_fd_handler()/qemu_process_fd_handlers() and
> friends for qemu, and by
> qemu-tools.c:qemu_set_fd_handler()/qemu_process_fd_handlers() for tools.
>
> So now to interact with code that uses qemu_set_fd_handler() you can
> built a select() loop around these utility functions.
Please put some of this in the commit message.
>> I am not happy with this addition of numbers to these functions, it
>> doesn't tell us why we have a 3 and how it differs from 2. If 3 is
>> really the backend for implementing 2, maybe it would be better to name
>> it __qemu_set_fd_handler2() and then have macros/wrappers calling into
>> it.
>
> That was the initial plan, but qemu_set_fd_handler2() is a back-end of
> sorts for qemu_set_fd_handler(), so I was just trying to be consistent
> with the naming. Personally I don't have any objections one way or the
> other.
Anything to avoid qemu_set_fd_handler17() at some point. I think using
__qemu_set_fd_handler() encourages people to modify that code rather
than copy it.
Cheers,
Jes
^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [Qemu-devel] Re: [RFC][PATCH v5 01/21] Move code related to fd handlers into utility functions
2010-12-07 15:02 ` Jes Sorensen
@ 2010-12-08 9:15 ` Stefan Hajnoczi
2010-12-08 9:17 ` Jes Sorensen
2010-12-08 14:24 ` Anthony Liguori
0 siblings, 2 replies; 74+ messages in thread
From: Stefan Hajnoczi @ 2010-12-08 9:15 UTC (permalink / raw)
To: Jes Sorensen; +Cc: agl, abeekhof, Michael Roth, qemu-devel, aliguori, ryanh
On Tue, Dec 07, 2010 at 04:02:03PM +0100, Jes Sorensen wrote:
> On 12/07/10 15:48, Michael Roth wrote:
> > On 12/07/2010 07:31 AM, Jes Sorensen wrote:
> >> On 12/03/10 19:03, Michael Roth wrote:
> >> I am not happy with this addition of numbers to these functions, it
> >> doesn't tell us why we have a 3 and how it differs from 2. If 3 is
> >> really the backend for implementing 2, maybe it would be better to name
> >> it __qemu_set_fd_handler2() and then have macros/wrappers calling into
> >> it.
> >
> > That was the initial plan, but qemu_set_fd_handler2() is a back-end of
> > sorts for qemu_set_fd_handler(), so I was just trying to be consistent
> > with the naming. Personally I don't have any objections one way or the
> > other.
>
> Anything to avoid qemu_set_fd_handler17() at some point. I think using
> __qemu_set_fd_handler() encourages people to modify that code rather
> than copy it.
I agree that qemu_set_fd_handler3() could be named something more
meaningful. Unfortunately we can't use __qemu_set_fd_handler() because
the C language standard reserves identifiers that start with double
underscore for the standard library. The Linux kernel gets away with it
because the code is freestanding but that doesn't apply to QEMU.
Stefan
^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [Qemu-devel] Re: [RFC][PATCH v5 01/21] Move code related to fd handlers into utility functions
2010-12-08 9:15 ` Stefan Hajnoczi
@ 2010-12-08 9:17 ` Jes Sorensen
2010-12-08 9:23 ` Stefan Hajnoczi
2010-12-08 14:24 ` Anthony Liguori
1 sibling, 1 reply; 74+ messages in thread
From: Jes Sorensen @ 2010-12-08 9:17 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: agl, abeekhof, Michael Roth, qemu-devel, aliguori, ryanh
On 12/08/10 10:15, Stefan Hajnoczi wrote:
> On Tue, Dec 07, 2010 at 04:02:03PM +0100, Jes Sorensen wrote:
>> Anything to avoid qemu_set_fd_handler17() at some point. I think using
>> __qemu_set_fd_handler() encourages people to modify that code rather
>> than copy it.
>
> I agree that qemu_set_fd_handler3() could be named something more
> meaningful. Unfortunately we can't use __qemu_set_fd_handler() because
> the C language standard reserves identifiers that start with double
> underscore for the standard library. The Linux kernel gets away with it
> because the code is freestanding but that doesn't apply to QEMU.
Hmmm you sure that is actually a rule, rather than a convention?
Either way, we can use 3 underscores, or leave the function static, in
which case the C library names are a non issue.
Cheers,
Jes
^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [Qemu-devel] Re: [RFC][PATCH v5 01/21] Move code related to fd handlers into utility functions
2010-12-08 9:17 ` Jes Sorensen
@ 2010-12-08 9:23 ` Stefan Hajnoczi
2010-12-08 9:29 ` Jes Sorensen
0 siblings, 1 reply; 74+ messages in thread
From: Stefan Hajnoczi @ 2010-12-08 9:23 UTC (permalink / raw)
To: Jes Sorensen; +Cc: agl, abeekhof, Michael Roth, qemu-devel, aliguori, ryanh
On Wed, Dec 08, 2010 at 10:17:50AM +0100, Jes Sorensen wrote:
> On 12/08/10 10:15, Stefan Hajnoczi wrote:
> > On Tue, Dec 07, 2010 at 04:02:03PM +0100, Jes Sorensen wrote:
> >> Anything to avoid qemu_set_fd_handler17() at some point. I think using
> >> __qemu_set_fd_handler() encourages people to modify that code rather
> >> than copy it.
> >
> > I agree that qemu_set_fd_handler3() could be named something more
> > meaningful. Unfortunately we can't use __qemu_set_fd_handler() because
> > the C language standard reserves identifiers that start with double
> > underscore for the standard library. The Linux kernel gets away with it
> > because the code is freestanding but that doesn't apply to QEMU.
>
> Hmmm you sure that is actually a rule, rather than a convention?
>
> Either way, we can use 3 underscores, or leave the function static, in
> which case the C library names are a non issue.
>From 7.1.3 Reserved identifiers:
"All identifiers that begin with an underscore and either an uppercase
letter or another underscore are always reserved for any use."
and
"All identifiers that begin with an underscore are always reserved for
use as identifiers with file scope in both the ordinary and tag name
spaces."
That includes three or more underscores too.
Stefan
^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [Qemu-devel] Re: [RFC][PATCH v5 01/21] Move code related to fd handlers into utility functions
2010-12-08 9:23 ` Stefan Hajnoczi
@ 2010-12-08 9:29 ` Jes Sorensen
0 siblings, 0 replies; 74+ messages in thread
From: Jes Sorensen @ 2010-12-08 9:29 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: agl, abeekhof, Michael Roth, qemu-devel, aliguori, ryanh
On 12/08/10 10:23, Stefan Hajnoczi wrote:
> From 7.1.3 Reserved identifiers:
>
> "All identifiers that begin with an underscore and either an uppercase
> letter or another underscore are always reserved for any use."
>
> and
>
> "All identifiers that begin with an underscore are always reserved for
> use as identifiers with file scope in both the ordinary and tag name
> spaces."
>
> That includes three or more underscores too.
Ok, I never hit problems with this, but ok we can name it
do_qemu_set_fd_handler() instead. That would go with the existing naming
conventions used in many places throughout the code.
Cheers,
Jes
^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [Qemu-devel] Re: [RFC][PATCH v5 01/21] Move code related to fd handlers into utility functions
2010-12-08 9:15 ` Stefan Hajnoczi
2010-12-08 9:17 ` Jes Sorensen
@ 2010-12-08 14:24 ` Anthony Liguori
1 sibling, 0 replies; 74+ messages in thread
From: Anthony Liguori @ 2010-12-08 14:24 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: agl, Jes Sorensen, qemu-devel, Michael Roth, ryanh, abeekhof
On 12/08/2010 03:15 AM, Stefan Hajnoczi wrote:
> On Tue, Dec 07, 2010 at 04:02:03PM +0100, Jes Sorensen wrote:
>
>> On 12/07/10 15:48, Michael Roth wrote:
>>
>>> On 12/07/2010 07:31 AM, Jes Sorensen wrote:
>>>
>>>> On 12/03/10 19:03, Michael Roth wrote:
>>>> I am not happy with this addition of numbers to these functions, it
>>>> doesn't tell us why we have a 3 and how it differs from 2. If 3 is
>>>> really the backend for implementing 2, maybe it would be better to name
>>>> it __qemu_set_fd_handler2() and then have macros/wrappers calling into
>>>> it.
>>>>
>>> That was the initial plan, but qemu_set_fd_handler2() is a back-end of
>>> sorts for qemu_set_fd_handler(), so I was just trying to be consistent
>>> with the naming. Personally I don't have any objections one way or the
>>> other.
>>>
>> Anything to avoid qemu_set_fd_handler17() at some point. I think using
>> __qemu_set_fd_handler() encourages people to modify that code rather
>> than copy it.
>>
> I agree that qemu_set_fd_handler3() could be named something more
> meaningful. Unfortunately we can't use __qemu_set_fd_handler() because
> the C language standard reserves identifiers that start with double
> underscore for the standard library. The Linux kernel gets away with it
> because the code is freestanding but that doesn't apply to QEMU.
>
fdset_set_handler()
or:
iohandler_set_handler()
Take whatever the structure is that represents the global state, convert
it's name to lower case, and use that as the prefix.
If this were C++, the namespacing would be obvious because you wouldn't
have a choice about how to design the namespace ;-)
/me ducks
Regards,
Anthony Liguori
> Stefan
>
^ permalink raw reply [flat|nested] 74+ messages in thread
* [Qemu-devel] [RFC][PATCH v5 02/21] Add qemu_set_fd_handler() wrappers to qemu-tools.c
2010-12-03 18:03 [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent Michael Roth
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 01/21] Move code related to fd handlers into utility functions Michael Roth
@ 2010-12-03 18:03 ` Michael Roth
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 03/21] virtagent: common code for managing client/server rpc jobs Michael Roth
` (20 subsequent siblings)
22 siblings, 0 replies; 74+ messages in thread
From: Michael Roth @ 2010-12-03 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: agl, stefanha, Jes.Sorensen, mdroth, aliguori, ryanh, abeekhof
This adds state information for managing fd handlers to qemu-tools.c so
that tools that build against it can implement an I/O loop for
interacting with objects that use qemu_set_fd_handler()
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
qemu-tool.c | 25 ++++++++++++++++++++++++-
1 files changed, 24 insertions(+), 1 deletions(-)
diff --git a/qemu-tool.c b/qemu-tool.c
index 392e1c9..78d3532 100644
--- a/qemu-tool.c
+++ b/qemu-tool.c
@@ -22,6 +22,8 @@
QEMUClock *rt_clock;
FILE *logfile;
+static QLIST_HEAD(, IOHandlerRecord) io_handlers =
+ QLIST_HEAD_INITIALIZER(io_handlers);
struct QEMUBH
{
@@ -103,11 +105,32 @@ void qemu_bh_delete(QEMUBH *bh)
qemu_free(bh);
}
+/* definitions to implement i/o loop for fd handlers in tools */
int qemu_set_fd_handler2(int fd,
IOCanReadHandler *fd_read_poll,
IOHandler *fd_read,
IOHandler *fd_write,
void *opaque)
{
- return 0;
+ return qemu_set_fd_handler3(&io_handlers, fd, fd_read_poll, fd_read,
+ fd_write, opaque);
+}
+
+int qemu_set_fd_handler(int fd,
+ IOHandler *fd_read,
+ IOHandler *fd_write,
+ void *opaque)
+{
+ return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
+}
+
+void qemu_get_fdset(int *nfds, fd_set *rfds, fd_set *wfds, fd_set *xfds)
+{
+ return qemu_get_fdset2(&io_handlers, nfds, rfds, wfds, xfds);
+}
+
+void qemu_process_fd_handlers(const fd_set *rfds, const fd_set *wfds,
+ const fd_set *xfds)
+{
+ return qemu_process_fd_handlers2(&io_handlers, rfds, wfds, xfds);
}
--
1.7.0.4
^ permalink raw reply related [flat|nested] 74+ messages in thread* [Qemu-devel] [RFC][PATCH v5 03/21] virtagent: common code for managing client/server rpc jobs
2010-12-03 18:03 [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent Michael Roth
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 01/21] Move code related to fd handlers into utility functions Michael Roth
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 02/21] Add qemu_set_fd_handler() wrappers to qemu-tools.c Michael Roth
@ 2010-12-03 18:03 ` Michael Roth
2010-12-06 21:54 ` [Qemu-devel] " Adam Litke
` (2 more replies)
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 04/21] virtagent: transport definitions and job callbacks Michael Roth
` (19 subsequent siblings)
22 siblings, 3 replies; 74+ messages in thread
From: Michael Roth @ 2010-12-03 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: agl, stefanha, Jes.Sorensen, mdroth, aliguori, ryanh, abeekhof
This implements a simple state machine to manage client/server rpc
jobs being multiplexed over a single channel.
A client job consists of sending an rpc request, reading an
rpc response, then making the appropriate callbacks. We allow one
client job to be processed at a time, which will make the following
state transitions:
VA_CLIENT_IDLE -> VA_CLIENT_SEND (job queued, send channel open)
VA_CLIENT_SEND -> VA_CLIENT_WAIT (request sent, awaiting response)
VA_CLIENT_WAIT -> VA_CLIENT_IDLE (response recieved, callbacks made)
A server job consists of recieving an rpc request, generating a
response, then sending the response. We expect to receive one server
request at a time due to the 1 at a time restriction for client jobs.
Server jobs make the following transitions:
VA_SERVER_IDLE -> VA_SERVER_WAIT (recieved/executed request, send
channel busy, response deferred)
VA_SERVER_IDLE -> VA_SERVER_SEND (recieved/executed request, send
channel open, sending response)
VA_SERVER_WAIT -> VA_SERVER_SEND (send channel now open, sending
response)
VA_SERVER_SEND -> VA_SERVER_IDLE (response sent)
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
virtagent-common.c | 427 ++++++++++++++++++++++++++++++++++++++++++++++++++++
virtagent-common.h | 66 ++++++++
2 files changed, 493 insertions(+), 0 deletions(-)
create mode 100644 virtagent-common.c
create mode 100644 virtagent-common.h
diff --git a/virtagent-common.c b/virtagent-common.c
new file mode 100644
index 0000000..45f9d9f
--- /dev/null
+++ b/virtagent-common.c
@@ -0,0 +1,427 @@
+/*
+ * virtagent - common host/guest RPC functions
+ *
+ * Copyright IBM Corp. 2010
+ *
+ * Authors:
+ * Adam Litke <aglitke@linux.vnet.ibm.com>
+ * Michael Roth <mdroth@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "virtagent-common.h"
+
+typedef struct VAClientJob {
+ xmlrpc_mem_block *req_data;
+ char *resp_data;
+ size_t resp_data_len;
+ VAClientCallback *cb;
+ QTAILQ_ENTRY(VAClientJob) next;
+ /* for use by QMP functions */
+ MonitorCompletion *mon_cb;
+ void *mon_data;
+} VAClientJob;
+
+typedef struct VAServerJob {
+ xmlrpc_mem_block *resp_data;
+ char *req_data;
+ size_t req_data_len;
+ void *opaque;
+ QTAILQ_ENTRY(VAServerJob) next;
+} VAServerJob;
+
+enum va_http_status {
+ VA_HTTP_STATUS_NEW,
+ VA_HTTP_STATUS_OK,
+ VA_HTTP_STATUS_ERROR,
+};
+
+typedef void (VAHTSendCallback)(enum va_http_status http_status,
+ const char *content, size_t content_len);
+typedef void (VAHTReadCallback)(enum va_http_status http_status,
+ const char *content, size_t content_len,
+ bool is_request);
+typedef struct VAHTState {
+ enum {
+ VA_SEND_START,
+ VA_SEND_HDR,
+ VA_SEND_BODY,
+ VA_READ_START,
+ VA_READ_HDR,
+ VA_READ_BODY,
+ } state;
+ char hdr[1024];
+ size_t hdr_len;
+ size_t hdr_pos;
+ char *content;
+ size_t content_len;
+ size_t content_pos;
+ VAHTSendCallback *send_cb;
+ VAHTReadCallback *read_cb;
+ bool is_request;
+} VAHTState;
+
+typedef struct VAState {
+ int fd;
+ enum va_client_state {
+ VA_CLIENT_IDLE = 0,
+ VA_CLIENT_SEND, /* sending rpc request */
+ VA_CLIENT_WAIT, /* waiting for rpc response */
+ } client_state;
+ enum va_server_state {
+ VA_SERVER_IDLE = 0,
+ VA_SERVER_WAIT, /* waiting to send rpc response */
+ VA_SERVER_SEND, /* sending rpc response */
+ } server_state;
+ VAClientData client_data;
+ VAServerData server_data;
+ int client_job_count;
+ QTAILQ_HEAD(, VAClientJob) client_jobs;
+ int server_job_count;
+ QTAILQ_HEAD(, VAServerJob) server_jobs;
+ /* for use by async send/read handlers for fd */
+ VAHTState send_state;
+ VAHTState read_state;
+} VAState;
+
+static VAState *va_state;
+
+static VAClientJob *va_current_client_job(void)
+{
+ TRACE("called");
+ return QTAILQ_FIRST(&va_state->client_jobs);
+}
+
+/***********************************************************/
+/* functions for starting/managing client/server rpc jobs */
+
+static int va_send_server_response(VAServerJob *server_job)
+{
+ VAHTState http_state;
+ TRACE("called");
+ http_state.content = XMLRPC_MEMBLOCK_CONTENTS(char, server_job->resp_data);
+ TRACE("server response: %s", http_state.content);
+ http_state.content_len = XMLRPC_MEMBLOCK_SIZE(char,
+ server_job->resp_data);
+ http_state.content_pos = 0;
+ http_state.hdr_pos = 0;
+ http_state.state = VA_SEND_START;
+ http_state.send_cb = va_server_send_cb;
+ va_http_hdr_init(&http_state, VA_HTTP_RESPONSE);
+ va_state->send_state = http_state;
+ qemu_set_fd_handler(va_state->fd, va_http_read_handler,
+ va_http_send_handler, NULL);
+ return 0;
+}
+
+static int va_send_client_request(VAClientJob *client_job)
+{
+ VAHTState http_state;
+ TRACE("called");
+ http_state.content = XMLRPC_MEMBLOCK_CONTENTS(char, client_job->req_data);
+ TRACE("client request: %s", http_state.content);
+ http_state.content_len = XMLRPC_MEMBLOCK_SIZE(char,
+ client_job->req_data);
+ http_state.content_pos = 0;
+ http_state.hdr_pos = 0;
+ http_state.state = VA_SEND_START;
+ http_state.send_cb = va_client_send_cb;
+ va_http_hdr_init(&http_state, VA_HTTP_REQUEST);
+ va_state->send_state = http_state;
+ qemu_set_fd_handler(va_state->fd, va_http_send_handler,
+ va_http_send_handler, NULL);
+ return 0;
+}
+
+/* do some sanity checks before setting client state */
+static bool va_set_client_state(enum va_client_state client_state)
+{
+ TRACE("setting client state to %d", client_state);
+ switch (client_state) {
+ case VA_CLIENT_IDLE:
+ assert(va_state->client_state == VA_CLIENT_IDLE ||
+ va_state->client_state == VA_CLIENT_WAIT);
+ break;
+ case VA_CLIENT_SEND:
+ assert(va_state->client_state == VA_CLIENT_IDLE);
+ break;
+ case VA_CLIENT_WAIT:
+ assert(va_state->client_state == VA_CLIENT_SEND);
+ break;
+ default:
+ LOG("invalid client state");
+ return false;
+ }
+ va_state->client_state = client_state;
+ return true;
+}
+
+/* do some sanity checks before setting server state */
+static bool va_set_server_state(enum va_server_state server_state)
+{
+ TRACE("setting server state to %d", server_state);
+ switch (server_state) {
+ case VA_SERVER_IDLE:
+ assert(va_state->server_state == VA_SERVER_IDLE ||
+ va_state->server_state == VA_SERVER_SEND);
+ break;
+ case VA_SERVER_WAIT:
+ assert(va_state->server_state == VA_SERVER_IDLE);
+ break;
+ case VA_SERVER_SEND:
+ assert(va_state->server_state == VA_SERVER_IDLE ||
+ va_state->server_state == VA_SERVER_WAIT);
+ break;
+ default:
+ LOG("invalid server state");
+ return false;
+ }
+ va_state->server_state = server_state;
+ return true;
+}
+
+/* xmit the next client/server job. for the client this entails sending
+ * a request to the remote server. for the server this entails sending a
+ * response to the remote client
+ *
+ * currently we only do one client job or one server job at a time. for
+ * situations where we start a client job but recieve a server job (remote
+ * rpc request) we go ahead and handle the server job before returning to
+ * handling the client job. TODO: there is potential for pipelining
+ * requests/responses for more efficient use of the channel.
+ *
+ * in all cases, we can only kick off client requests or server responses
+ * when the send side of the channel is not being used
+ */
+static int va_kick(void)
+{
+ VAServerJob *server_job;
+ VAClientJob *client_job;
+ int ret;
+
+ TRACE("called");
+
+ /* handle server jobs first */
+ if (QTAILQ_EMPTY(&va_state->server_jobs)) {
+ assert(va_set_server_state(VA_SERVER_IDLE));
+ } else {
+ TRACE("handling server job queue");
+ if (va_state->client_state == VA_CLIENT_SEND) {
+ TRACE("send channel busy, deferring till available");
+ assert(va_set_server_state(VA_SERVER_WAIT));
+ goto out;
+ }
+ TRACE("send server response");
+ server_job = QTAILQ_FIRST(&va_state->server_jobs);
+
+ /* set up the send handler for the response */
+ ret = va_send_server_response(server_job);
+ if (ret != 0) {
+ LOG("error setting up send handler for server response");
+ goto out_bad;
+ }
+ assert(va_set_server_state(VA_SERVER_SEND));
+ goto out;
+ }
+
+ /* handle client jobs if nothing to do for server */
+ if (QTAILQ_EMPTY(&va_state->client_jobs)) {
+ assert(va_set_client_state(VA_CLIENT_IDLE));
+ } else {
+ TRACE("handling client job queue");
+ if (va_state->client_state != VA_CLIENT_IDLE) {
+ TRACE("client job in progress, returning");
+ goto out;
+ }
+
+ TRACE("sending new client request");
+ client_job = QTAILQ_FIRST(&va_state->client_jobs);
+ /* set up the send handler for the request, then put it on the
+ * wait queue till response is read
+ */
+ ret = va_send_client_request(client_job);
+ if (ret != 0) {
+ LOG("error setting up sendhandler for client request");
+ goto out_bad;
+ }
+ assert(va_set_client_state(VA_CLIENT_SEND));
+ }
+
+out:
+ return 0;
+out_bad:
+ return ret;
+}
+
+/* push new client job onto queue, */
+static int va_push_client_job(VAClientJob *client_job)
+{
+ TRACE("called");
+ assert(client_job != NULL);
+ if (va_state->client_job_count >= VA_CLIENT_JOBS_MAX) {
+ LOG("client job queue limit exceeded");
+ return -ENOBUFS;
+ }
+ QTAILQ_INSERT_TAIL(&va_state->client_jobs, client_job, next);
+ va_state->client_job_count++;
+
+ return va_kick();
+}
+
+/* pop client job off queue. this should only be done when we're done with
+ * both sending the request and recieving the response
+ */
+static VAClientJob *va_pop_client_job(void)
+{
+ VAClientJob *client_job = va_current_client_job();
+ TRACE("called");
+ if (client_job != NULL) {
+ QTAILQ_REMOVE(&va_state->client_jobs, client_job, next);
+ va_state->client_job_count--;
+ assert(va_set_client_state(VA_CLIENT_IDLE));
+ }
+ return client_job;
+}
+
+/* push new server job onto the queue */
+static int va_push_server_job(VAServerJob *server_job)
+{
+ TRACE("called");
+ if (va_state->server_job_count >= VA_SERVER_JOBS_MAX) {
+ LOG("server job queue limit exceeded");
+ return -ENOBUFS;
+ }
+ QTAILQ_INSERT_TAIL(&va_state->server_jobs, server_job, next);
+ va_state->server_job_count++;
+ return va_kick();
+}
+
+/* pop server job off queue. this should only be done when we're ready to
+ * send the rpc response back to the remote client
+ */
+static VAServerJob *va_pop_server_job(void) {
+ VAServerJob *server_job = QTAILQ_FIRST(&va_state->server_jobs);
+ TRACE("called");
+ if (server_job != NULL) {
+ QTAILQ_REMOVE(&va_state->server_jobs, server_job, next);
+ va_state->server_job_count--;
+ assert(va_set_server_state(VA_SERVER_IDLE));
+ }
+
+ return server_job;
+}
+
+static VAClientJob *va_client_job_new(xmlrpc_mem_block *req_data,
+ VAClientCallback *cb,
+ MonitorCompletion *mon_cb,
+ void *mon_data)
+{
+ VAClientJob *cj = qemu_mallocz(sizeof(VAClientJob));
+ TRACE("called");
+ cj->req_data = req_data;
+ cj->cb = cb;
+ cj->mon_cb = mon_cb;
+ cj->mon_data = mon_data;
+
+ return cj;
+}
+
+static VAServerJob *va_server_job_new(xmlrpc_mem_block *resp_data)
+{
+ VAServerJob *sj = qemu_mallocz(sizeof(VAServerJob));
+ TRACE("called");
+ sj->resp_data = resp_data;
+
+ return sj;
+}
+
+/* create new client job and then put it on the queue. this can be
+ * called externally from virtagent. Since there can only be one virtagent
+ * instance we access state via an object-scoped global rather than pass
+ * it around.
+ *
+ * if this is successful virtagent will handle cleanup of req_xml after
+ * making the appropriate callbacks, otherwise callee should handle it
+ */
+int va_client_job_add(xmlrpc_mem_block *req_xml, VAClientCallback *cb,
+ MonitorCompletion *mon_cb, void *mon_data)
+{
+ int ret;
+ VAClientJob *client_job;
+ TRACE("called");
+
+ client_job = va_client_job_new(req_xml, cb, mon_cb, mon_data);
+ if (client_job == NULL) {
+ return -EINVAL;
+ }
+
+ ret = va_push_client_job(client_job);
+ if (ret != 0) {
+ LOG("error adding client to queue: %s", strerror(ret));
+ qemu_free(client_job);
+ return ret;
+ }
+
+ return 0;
+}
+
+/* create new server job and then put it on the queue in wait state
+ * this should only be called from within our read handler callback
+ */
+static int va_server_job_add(xmlrpc_mem_block *resp_xml)
+{
+ VAServerJob *server_job;
+ TRACE("called");
+
+ server_job = va_server_job_new(resp_xml);
+ assert(server_job != NULL);
+ va_push_server_job(server_job);
+ return 0;
+}
+
+
+int va_init(enum va_ctx ctx, int fd)
+{
+ VAState *s;
+ int ret;
+ bool is_host = (ctx == VA_CTX_HOST) ? true : false;
+
+ TRACE("called");
+ if (va_state) {
+ LOG("virtagent already initialized");
+ return -EPERM;
+ }
+
+ s = qemu_mallocz(sizeof(VAState));
+
+ ret = va_server_init(&s->server_data, is_host);
+ if (ret) {
+ LOG("error initializing virtagent server");
+ goto out_bad;
+ }
+ ret = va_client_init(&s->client_data);
+ if (ret) {
+ LOG("error initializing virtagent client");
+ goto out_bad;
+ }
+
+ s->client_state = VA_CLIENT_IDLE;
+ s->server_state = VA_SERVER_IDLE;
+ QTAILQ_INIT(&s->client_jobs);
+ QTAILQ_INIT(&s->server_jobs);
+ s->read_state.state = VA_READ_START;
+ s->read_state.read_cb = va_http_read_cb;
+ s->fd = fd;
+ va_state = s;
+
+ /* start listening for requests/responses */
+ qemu_set_fd_handler(va_state->fd, va_http_read_handler, NULL, NULL);
+
+ return 0;
+out_bad:
+ qemu_free(s);
+ return ret;
+}
diff --git a/virtagent-common.h b/virtagent-common.h
new file mode 100644
index 0000000..c0ada60
--- /dev/null
+++ b/virtagent-common.h
@@ -0,0 +1,66 @@
+/*
+ * virt-agent - host/guest RPC client functions
+ *
+ * Copyright IBM Corp. 2010
+ *
+ * Authors:
+ * Adam Litke <aglitke@linux.vnet.ibm.com>
+ * Michael Roth <mdroth@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+#ifndef VIRTAGENT_COMMON_H
+#define VIRTAGENT_COMMON_H
+
+#include <xmlrpc-c/base.h>
+#include <xmlrpc-c/client.h>
+#include <xmlrpc-c/server.h>
+#include "qemu-common.h"
+#include "qemu_socket.h"
+#include "monitor.h"
+#include "virtagent-server.h"
+#include "virtagent.h"
+
+#define DEBUG_VA
+
+#ifdef DEBUG_VA
+#define TRACE(msg, ...) do { \
+ fprintf(stderr, "%s:%s():L%d: " msg "\n", \
+ __FILE__, __FUNCTION__, __LINE__, ## __VA_ARGS__); \
+} while(0)
+#else
+#define TRACE(msg, ...) \
+ do { } while (0)
+#endif
+
+#define LOG(msg, ...) do { \
+ fprintf(stderr, "%s:%s(): " msg "\n", \
+ __FILE__, __FUNCTION__, ## __VA_ARGS__); \
+} while(0)
+
+#define VERSION "1.0"
+#define EOL "\r\n"
+
+#define VA_HDR_LEN_MAX 4096 /* http header limit */
+#define VA_CONTENT_LEN_MAX 2*1024*1024 /* rpc/http send limit */
+#define VA_CLIENT_JOBS_MAX 5 /* max client rpcs we can queue */
+#define VA_SERVER_JOBS_MAX 1 /* max server rpcs we can queue */
+
+enum va_ctx {
+ VA_CTX_HOST,
+ VA_CTX_GUEST,
+};
+
+enum va_job_status {
+ VA_JOB_STATUS_PENDING = 0,
+ VA_JOB_STATUS_OK,
+ VA_JOB_STATUS_ERROR,
+ VA_JOB_STATUS_CANCELLED,
+};
+
+int va_init(enum va_ctx ctx, int fd);
+int va_client_job_add(xmlrpc_mem_block *req_xml, VAClientCallback *cb,
+ MonitorCompletion *mon_cb, void *mon_data);
+#endif /* VIRTAGENT_COMMON_H */
--
1.7.0.4
^ permalink raw reply related [flat|nested] 74+ messages in thread* [Qemu-devel] Re: [RFC][PATCH v5 03/21] virtagent: common code for managing client/server rpc jobs
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 03/21] virtagent: common code for managing client/server rpc jobs Michael Roth
@ 2010-12-06 21:54 ` Adam Litke
2010-12-06 22:15 ` Michael Roth
2010-12-06 21:57 ` Adam Litke
2010-12-07 13:38 ` Jes Sorensen
2 siblings, 1 reply; 74+ messages in thread
From: Adam Litke @ 2010-12-06 21:54 UTC (permalink / raw)
To: Michael Roth
Cc: agl, stefanha, Jes.Sorensen, qemu-devel, aliguori, ryanh, abeekhof
On Fri, 2010-12-03 at 12:03 -0600, Michael Roth wrote:
> +/* create new client job and then put it on the queue. this can be
> + * called externally from virtagent. Since there can only be one virtagent
> + * instance we access state via an object-scoped global rather than pass
> + * it around.
> + *
> + * if this is successful virtagent will handle cleanup of req_xml after
> + * making the appropriate callbacks, otherwise callee should handle it
> + */
Explain please. Do you mean caller should handle it? Are you trying to
say that this function, when successful, "steals" the reference to
req_xml?
> +int va_client_job_add(xmlrpc_mem_block *req_xml, VAClientCallback *cb,
> + MonitorCompletion *mon_cb, void *mon_data)
> +{
> + int ret;
> + VAClientJob *client_job;
> + TRACE("called");
> +
> + client_job = va_client_job_new(req_xml, cb, mon_cb, mon_data);
> + if (client_job == NULL) {
> + return -EINVAL;
> + }
> +
> + ret = va_push_client_job(client_job);
> + if (ret != 0) {
> + LOG("error adding client to queue: %s", strerror(ret));
> + qemu_free(client_job);
> + return ret;
> + }
> +
> + return 0;
> +}
--
Thanks,
Adam
^ permalink raw reply [flat|nested] 74+ messages in thread* [Qemu-devel] Re: [RFC][PATCH v5 03/21] virtagent: common code for managing client/server rpc jobs
2010-12-06 21:54 ` [Qemu-devel] " Adam Litke
@ 2010-12-06 22:15 ` Michael Roth
0 siblings, 0 replies; 74+ messages in thread
From: Michael Roth @ 2010-12-06 22:15 UTC (permalink / raw)
To: Adam Litke
Cc: agl, stefanha, Jes.Sorensen, qemu-devel, aliguori, ryanh, abeekhof
On 12/06/2010 03:54 PM, Adam Litke wrote:
> On Fri, 2010-12-03 at 12:03 -0600, Michael Roth wrote:
>> +/* create new client job and then put it on the queue. this can be
>> + * called externally from virtagent. Since there can only be one virtagent
>> + * instance we access state via an object-scoped global rather than pass
>> + * it around.
>> + *
>> + * if this is successful virtagent will handle cleanup of req_xml after
>> + * making the appropriate callbacks, otherwise callee should handle it
>> + */
>
> Explain please. Do you mean caller should handle it? Are you trying to
> say that this function, when successful, "steals" the reference to
> req_xml?
>
Yup, should be caller. And yes, cleanup duty gets taken over if the call
succeeds (after transmitting the request we have no need for the
req_xml, so it didn't seem to make sense to carry it around just so the
caller can free it when it gets it's call later on)
>> +int va_client_job_add(xmlrpc_mem_block *req_xml, VAClientCallback *cb,
>> + MonitorCompletion *mon_cb, void *mon_data)
>> +{
>> + int ret;
>> + VAClientJob *client_job;
>> + TRACE("called");
>> +
>> + client_job = va_client_job_new(req_xml, cb, mon_cb, mon_data);
>> + if (client_job == NULL) {
>> + return -EINVAL;
>> + }
>> +
>> + ret = va_push_client_job(client_job);
>> + if (ret != 0) {
>> + LOG("error adding client to queue: %s", strerror(ret));
>> + qemu_free(client_job);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>
^ permalink raw reply [flat|nested] 74+ messages in thread
* [Qemu-devel] Re: [RFC][PATCH v5 03/21] virtagent: common code for managing client/server rpc jobs
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 03/21] virtagent: common code for managing client/server rpc jobs Michael Roth
2010-12-06 21:54 ` [Qemu-devel] " Adam Litke
@ 2010-12-06 21:57 ` Adam Litke
2010-12-06 22:24 ` Michael Roth
2010-12-07 13:38 ` Jes Sorensen
2 siblings, 1 reply; 74+ messages in thread
From: Adam Litke @ 2010-12-06 21:57 UTC (permalink / raw)
To: Michael Roth
Cc: agl, stefanha, Jes.Sorensen, qemu-devel, aliguori, ryanh, abeekhof
On Fri, 2010-12-03 at 12:03 -0600, Michael Roth wrote:
> +/* create new client job and then put it on the queue. this can be
> + * called externally from virtagent. Since there can only be one virtagent
> + * instance we access state via an object-scoped global rather than pass
> + * it around.
> + *
> + * if this is successful virtagent will handle cleanup of req_xml after
> + * making the appropriate callbacks, otherwise callee should handle it
> + */
Explain please. Do you mean caller should handle it? Are you trying to
say that this function, when successful, "steals" the reference to
req_xml?
> +int va_client_job_add(xmlrpc_mem_block *req_xml, VAClientCallback *cb,
> + MonitorCompletion *mon_cb, void *mon_data)
> +{
> + int ret;
> + VAClientJob *client_job;
> + TRACE("called");
> +
> + client_job = va_client_job_new(req_xml, cb, mon_cb, mon_data);
> + if (client_job == NULL) {
> + return -EINVAL;
> + }
> +
> + ret = va_push_client_job(client_job);
> + if (ret != 0) {
> + LOG("error adding client to queue: %s", strerror(ret));
> + qemu_free(client_job);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +/* create new server job and then put it on the queue in wait state
> + * this should only be called from within our read handler callback
> + */
Since this function is only 4 lines and has only one valid call site.
perhaps its better to fold it directly into the read handler callback.
> +static int va_server_job_add(xmlrpc_mem_block *resp_xml)
> +{
> + VAServerJob *server_job;
> + TRACE("called");
> +
> + server_job = va_server_job_new(resp_xml);
> + assert(server_job != NULL);
> + va_push_server_job(server_job);
> + return 0;
> +}
--
Thanks,
Adam
^ permalink raw reply [flat|nested] 74+ messages in thread* [Qemu-devel] Re: [RFC][PATCH v5 03/21] virtagent: common code for managing client/server rpc jobs
2010-12-06 21:57 ` Adam Litke
@ 2010-12-06 22:24 ` Michael Roth
0 siblings, 0 replies; 74+ messages in thread
From: Michael Roth @ 2010-12-06 22:24 UTC (permalink / raw)
To: Adam Litke
Cc: agl, stefanha, Jes.Sorensen, qemu-devel, aliguori, ryanh, abeekhof
On 12/06/2010 03:57 PM, Adam Litke wrote:
> On Fri, 2010-12-03 at 12:03 -0600, Michael Roth wrote:
>> +/* create new client job and then put it on the queue. this can be
>> + * called externally from virtagent. Since there can only be one virtagent
>> + * instance we access state via an object-scoped global rather than pass
>> + * it around.
>> + *
>> + * if this is successful virtagent will handle cleanup of req_xml after
>> + * making the appropriate callbacks, otherwise callee should handle it
>> + */
>
> Explain please. Do you mean caller should handle it? Are you trying to
> say that this function, when successful, "steals" the reference to
> req_xml?
>
>> +int va_client_job_add(xmlrpc_mem_block *req_xml, VAClientCallback *cb,
>> + MonitorCompletion *mon_cb, void *mon_data)
>> +{
>> + int ret;
>> + VAClientJob *client_job;
>> + TRACE("called");
>> +
>> + client_job = va_client_job_new(req_xml, cb, mon_cb, mon_data);
>> + if (client_job == NULL) {
>> + return -EINVAL;
>> + }
>> +
>> + ret = va_push_client_job(client_job);
>> + if (ret != 0) {
>> + LOG("error adding client to queue: %s", strerror(ret));
>> + qemu_free(client_job);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/* create new server job and then put it on the queue in wait state
>> + * this should only be called from within our read handler callback
>> + */
>
> Since this function is only 4 lines and has only one valid call site.
> perhaps its better to fold it directly into the read handler callback.
>
>> +static int va_server_job_add(xmlrpc_mem_block *resp_xml)
>> +{
>> + VAServerJob *server_job;
>> + TRACE("called");
>> +
>> + server_job = va_server_job_new(resp_xml);
>> + assert(server_job != NULL);
>> + va_push_server_job(server_job);
>> + return 0;
>> +}
>
What I was mainly shooting for was to have the entry-points for adding
client and server jobs be clear and somewhat similar. I've actually
moved the read handler callback body into
virtagent-server.c:va_do_server_rpc() since then. So client jobs get
added by hmp/qmp->virtagent:va_do_rpc()->va_push_client_job() and server
jobs by read
handler->virtagent-server.c:va_do_server_rpc()->va_push_server_job().
^ permalink raw reply [flat|nested] 74+ messages in thread
* [Qemu-devel] Re: [RFC][PATCH v5 03/21] virtagent: common code for managing client/server rpc jobs
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 03/21] virtagent: common code for managing client/server rpc jobs Michael Roth
2010-12-06 21:54 ` [Qemu-devel] " Adam Litke
2010-12-06 21:57 ` Adam Litke
@ 2010-12-07 13:38 ` Jes Sorensen
2010-12-07 15:02 ` Michael Roth
2 siblings, 1 reply; 74+ messages in thread
From: Jes Sorensen @ 2010-12-07 13:38 UTC (permalink / raw)
To: Michael Roth; +Cc: agl, stefanha, abeekhof, qemu-devel, aliguori, ryanh
On 12/03/10 19:03, Michael Roth wrote:
> This implements a simple state machine to manage client/server rpc
> jobs being multiplexed over a single channel.
>
> A client job consists of sending an rpc request, reading an
> rpc response, then making the appropriate callbacks. We allow one
> client job to be processed at a time, which will make the following
> state transitions:
>
> VA_CLIENT_IDLE -> VA_CLIENT_SEND (job queued, send channel open)
> VA_CLIENT_SEND -> VA_CLIENT_WAIT (request sent, awaiting response)
> VA_CLIENT_WAIT -> VA_CLIENT_IDLE (response recieved, callbacks made)
>
> A server job consists of recieving an rpc request, generating a
> response, then sending the response. We expect to receive one server
> request at a time due to the 1 at a time restriction for client jobs.
> Server jobs make the following transitions:
>
> VA_SERVER_IDLE -> VA_SERVER_WAIT (recieved/executed request, send
> channel busy, response deferred)
> VA_SERVER_IDLE -> VA_SERVER_SEND (recieved/executed request, send
> channel open, sending response)
> VA_SERVER_WAIT -> VA_SERVER_SEND (send channel now open, sending
> response)
> VA_SERVER_SEND -> VA_SERVER_IDLE (response sent)
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
As mentioned before, I really don't understand why this is part of QEMU,
the guest agent really should be able to run totally outside of QEMU.
> +
> +#define DEBUG_VA
> +
> +#ifdef DEBUG_VA
> +#define TRACE(msg, ...) do { \
> + fprintf(stderr, "%s:%s():L%d: " msg "\n", \
> + __FILE__, __FUNCTION__, __LINE__, ## __VA_ARGS__); \
> +} while(0)
> +#else
> +#define TRACE(msg, ...) \
> + do { } while (0)
> +#endif
> +
> +#define LOG(msg, ...) do { \
> + fprintf(stderr, "%s:%s(): " msg "\n", \
> + __FILE__, __FUNCTION__, ## __VA_ARGS__); \
> +} while(0)
This must be like the 217th copy of these functions, could you please
use some of the code that is already in the tree, and make it generic if
needed.
> +
> +#define VERSION "1.0"
> +#define EOL "\r\n"
> +
> +#define VA_HDR_LEN_MAX 4096 /* http header limit */
> +#define VA_CONTENT_LEN_MAX 2*1024*1024 /* rpc/http send limit */
> +#define VA_CLIENT_JOBS_MAX 5 /* max client rpcs we can queue */
> +#define VA_SERVER_JOBS_MAX 1 /* max server rpcs we can queue */
As mentioned last time, please make this stuff configurable and not hard
coded.
Cheers,
Jes
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Qemu-devel] Re: [RFC][PATCH v5 03/21] virtagent: common code for managing client/server rpc jobs
2010-12-07 13:38 ` Jes Sorensen
@ 2010-12-07 15:02 ` Michael Roth
0 siblings, 0 replies; 74+ messages in thread
From: Michael Roth @ 2010-12-07 15:02 UTC (permalink / raw)
To: Jes Sorensen; +Cc: agl, stefanha, abeekhof, qemu-devel, aliguori, ryanh
On 12/07/2010 07:38 AM, Jes Sorensen wrote:
> On 12/03/10 19:03, Michael Roth wrote:
>> This implements a simple state machine to manage client/server rpc
>> jobs being multiplexed over a single channel.
>>
>> A client job consists of sending an rpc request, reading an
>> rpc response, then making the appropriate callbacks. We allow one
>> client job to be processed at a time, which will make the following
>> state transitions:
>>
>> VA_CLIENT_IDLE -> VA_CLIENT_SEND (job queued, send channel open)
>> VA_CLIENT_SEND -> VA_CLIENT_WAIT (request sent, awaiting response)
>> VA_CLIENT_WAIT -> VA_CLIENT_IDLE (response recieved, callbacks made)
>>
>> A server job consists of recieving an rpc request, generating a
>> response, then sending the response. We expect to receive one server
>> request at a time due to the 1 at a time restriction for client jobs.
>> Server jobs make the following transitions:
>>
>> VA_SERVER_IDLE -> VA_SERVER_WAIT (recieved/executed request, send
>> channel busy, response deferred)
>> VA_SERVER_IDLE -> VA_SERVER_SEND (recieved/executed request, send
>> channel open, sending response)
>> VA_SERVER_WAIT -> VA_SERVER_SEND (send channel now open, sending
>> response)
>> VA_SERVER_SEND -> VA_SERVER_IDLE (response sent)
>>
>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>
> As mentioned before, I really don't understand why this is part of QEMU,
> the guest agent really should be able to run totally outside of QEMU.
>
>> +
>> +#define DEBUG_VA
>> +
>> +#ifdef DEBUG_VA
>> +#define TRACE(msg, ...) do { \
>> + fprintf(stderr, "%s:%s():L%d: " msg "\n", \
>> + __FILE__, __FUNCTION__, __LINE__, ## __VA_ARGS__); \
>> +} while(0)
>> +#else
>> +#define TRACE(msg, ...) \
>> + do { } while (0)
>> +#endif
>> +
>> +#define LOG(msg, ...) do { \
>> + fprintf(stderr, "%s:%s(): " msg "\n", \
>> + __FILE__, __FUNCTION__, ## __VA_ARGS__); \
>> +} while(0)
>
> This must be like the 217th copy of these functions, could you please
> use some of the code that is already in the tree, and make it generic if
> needed.
>
>> +
>> +#define VERSION "1.0"
>> +#define EOL "\r\n"
>> +
>> +#define VA_HDR_LEN_MAX 4096 /* http header limit */
>> +#define VA_CONTENT_LEN_MAX 2*1024*1024 /* rpc/http send limit */
>> +#define VA_CLIENT_JOBS_MAX 5 /* max client rpcs we can queue */
>> +#define VA_SERVER_JOBS_MAX 1 /* max server rpcs we can queue */
>
> As mentioned last time, please make this stuff configurable and not hard
> coded.
>
Yup, definitely on the TODO. Should be in the next round.
> Cheers,
> Jes
>
^ permalink raw reply [flat|nested] 74+ messages in thread
* [Qemu-devel] [RFC][PATCH v5 04/21] virtagent: transport definitions and job callbacks
2010-12-03 18:03 [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent Michael Roth
` (2 preceding siblings ...)
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 03/21] virtagent: common code for managing client/server rpc jobs Michael Roth
@ 2010-12-03 18:03 ` Michael Roth
2010-12-06 22:02 ` [Qemu-devel] " Adam Litke
2010-12-07 13:44 ` Jes Sorensen
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 05/21] virtagent: base client definitions Michael Roth
` (18 subsequent siblings)
22 siblings, 2 replies; 74+ messages in thread
From: Michael Roth @ 2010-12-03 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: agl, stefanha, Jes.Sorensen, mdroth, aliguori, ryanh, abeekhof
Async read/send handlers for managing RPC/HTTP request/responses. The
read handler runs continuously and calls into client or server RPC
callbacks based on the HTTP header. The send handlers are started up to
send RPC client requests/server responses.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
virtagent-common.c | 332 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 332 insertions(+), 0 deletions(-)
diff --git a/virtagent-common.c b/virtagent-common.c
index 45f9d9f..1ed2b55 100644
--- a/virtagent-common.c
+++ b/virtagent-common.c
@@ -89,6 +89,12 @@ typedef struct VAState {
static VAState *va_state;
+static bool va_set_client_state(enum va_client_state client_state);
+static VAServerJob *va_pop_server_job(void);
+static VAClientJob *va_pop_client_job(void);
+static int va_server_job_add(xmlrpc_mem_block *resp_xml);
+static int va_kick(void);
+
static VAClientJob *va_current_client_job(void)
{
TRACE("called");
@@ -96,6 +102,332 @@ static VAClientJob *va_current_client_job(void)
}
/***********************************************************/
+/* callbacks for read/send handlers */
+
+static void va_client_send_cb(enum va_http_status http_status,
+ const char *content, size_t content_len)
+{
+ VAClientJob *client_job = va_current_client_job();
+
+ TRACE("called");
+ assert(client_job != NULL);
+
+ if (http_status != VA_HTTP_STATUS_OK) {
+ /* TODO: we should reset everything at this point...guest/host will
+ * be out of whack with each other since there's no way to let the
+ * other know job failed (server or client job) if the send channel
+ * is down. But how do we induce the other side to do the same?
+ */
+ LOG("error sending http request");
+ }
+
+ /* request sent ok. free up request xml, then move to
+ * wait (for response) state
+ */
+ XMLRPC_MEMBLOCK_FREE(char, client_job->req_data);
+ assert(va_set_client_state(VA_CLIENT_WAIT));
+}
+
+static void va_server_send_cb(enum va_http_status http_status,
+ const char *content, size_t content_len)
+{
+ VAServerJob *server_job = va_pop_server_job();
+
+ TRACE("called");
+ assert(server_job != NULL);
+
+ if (http_status != VA_HTTP_STATUS_OK) {
+ /* TODO: we should reset everything at this point...guest/host will
+ * be out of whack with each other since there's no way to let the
+ * other know job failed (server or client job) if the send channel
+ * is down
+ */
+ LOG("error sending http response");
+ return;
+ }
+
+ /* response sent ok, cleanup server job and kick off the next one */
+ XMLRPC_MEMBLOCK_FREE(char, server_job->resp_data);
+ qemu_free(server_job);
+ va_kick();
+}
+
+static void va_client_read_cb(const char *content, size_t content_len)
+{
+ VAClientJob *client_job;
+
+ client_job = va_pop_client_job();
+ assert(client_job != NULL);
+TRACE("marker");
+ client_job->cb(content, content_len, client_job->mon_cb,
+ client_job->mon_data);
+ va_kick();
+}
+
+static void va_server_read_cb(const char *content, size_t content_len)
+{
+ xmlrpc_mem_block *resp_xml;
+ VAServerData *server_data = &va_state->server_data;
+ int ret;
+
+ TRACE("called");
+ resp_xml = xmlrpc_registry_process_call(&server_data->env,
+ server_data->registry,
+ NULL, content, content_len);
+ if (resp_xml == NULL) {
+ LOG("error processing RPC request");
+ goto out_bad;
+ }
+
+ ret = va_server_job_add(resp_xml);
+ if (ret != 0) {
+ LOG("error adding server job: %s", strerror(ret));
+ }
+
+ return;
+out_bad:
+ /* TODO: should reset state here */
+ return;
+}
+
+static void va_http_read_cb(enum va_http_status http_status,
+ const char *content, size_t content_len, bool is_request)
+{
+ TRACE("called");
+ if (http_status != VA_HTTP_STATUS_OK) {
+ LOG("error reading http %s", is_request ? "request" : "response");
+ content = NULL;
+ }
+
+ if (is_request) {
+ va_server_read_cb(content, content_len);
+ } else {
+ va_client_read_cb(content, content_len);
+ }
+
+ return;
+}
+
+/***********************************************************/
+/* utility functions for handling http calls */
+
+#define VA_HTTP_REQUEST 1
+#define VA_HTTP_RESPONSE 2
+
+static void va_http_hdr_init(VAHTState *s, int request_type) {
+ const char *preamble;
+
+ TRACE("called");
+ /* essentially ignored in the context of virtagent, but might as well */
+ if (request_type == VA_HTTP_REQUEST) {
+ preamble = "POST /RPC2 HTTP/1.1";
+ } else if (request_type == VA_HTTP_RESPONSE) {
+ preamble = "HTTP/1.1 200 OK";
+ } else {
+ s->hdr_len = 0;
+ return;
+ }
+ s->hdr_len = sprintf(s->hdr,
+ "%s" EOL
+ "Content-Type: text/xml" EOL
+ "Content-Length: %u" EOL EOL,
+ preamble,
+ (uint32_t)s->content_len);
+}
+
+static void va_rpc_parse_hdr(VAHTState *s)
+{
+ int i, line_pos = 0;
+ bool first_line = true;
+ char line_buf[4096];
+
+ TRACE("called");
+
+ for (i = 0; i < VA_HDR_LEN_MAX; ++i) {
+ if (s->hdr[i] != '\n') {
+ /* read line */
+ line_buf[line_pos++] = s->hdr[i];
+ } else {
+ /* process line */
+ if (first_line) {
+ s->is_request = (strncmp(line_buf, "POST", 4) == 0) ?
+ true : false;
+ first_line = false;
+ }
+ if (strncmp(line_buf, "Content-Length: ", 16) == 0) {
+ s->content_len = atoi(&line_buf[16]);
+ return;
+ }
+ line_pos = 0;
+ }
+ }
+}
+
+static int va_end_of_header(char *buf, int end_pos)
+{
+ return !strncmp(buf+(end_pos-2), "\n\r\n", 3);
+}
+
+/***********************************************************/
+/* read/send handlers */
+
+static void va_http_read_handler(void *opaque)
+{
+ VAHTState *s = &va_state->read_state;
+ enum va_http_status http_status;
+ int fd = va_state->fd;
+ int ret;
+
+ TRACE("called with opaque: %p", opaque);
+
+ switch (s->state) {
+ case VA_READ_START:
+ s->state = VA_READ_HDR;
+ case VA_READ_HDR:
+ while((ret = read(fd, s->hdr + s->hdr_pos, 1)) > 0
+ && s->hdr_pos < VA_HDR_LEN_MAX) {
+ s->hdr_pos += ret;
+ if (va_end_of_header(s->hdr, s->hdr_pos - 1)) {
+ break;
+ }
+ }
+ if (ret == -1) {
+ if (errno == EAGAIN || errno == EWOULDBLOCK || errno == EINTR) {
+ return;
+ } else {
+ LOG("error reading connection: %s", strerror(errno));
+ goto out_bad;
+ }
+ } else if (ret == 0) {
+ LOG("connected closed unexpectedly");
+ goto out_bad;
+ } else if (s->hdr_pos >= VA_HDR_LEN_MAX) {
+ LOG("http header too long");
+ goto out_bad;
+ } else {
+ s->content_len = -1;
+ va_rpc_parse_hdr(s);
+ if (s->content_len == -1) {
+ LOG("malformed http header");
+ goto out_bad;
+ } else if (s->content_len > VA_CONTENT_LEN_MAX) {
+ LOG("http content length too long");
+ goto out_bad;
+ }
+ s->content = qemu_mallocz(s->content_len);
+ s->state = VA_READ_BODY;
+ }
+ case VA_READ_BODY:
+ while(s->content_pos < s->content_len) {
+ ret = read(fd, s->content + s->content_pos,
+ s->content_len - s->content_pos);
+ if (ret == -1) {
+ if (errno == EAGAIN || errno == EWOULDBLOCK
+ || errno == EINTR) {
+ return;
+ } else {
+ LOG("error reading connection: %s", strerror(errno));
+ goto out_bad;
+ }
+ } else if (ret == 0) {
+ LOG("connection closed unexpectedly:"
+ " read %u bytes, expected %u bytes",
+ (unsigned int)s->content_pos, (unsigned int)s->content_len);
+ goto out_bad;
+ }
+ s->content_pos += ret;
+ }
+
+ http_status = VA_HTTP_STATUS_OK;
+ goto out;
+ default:
+ LOG("unknown state");
+ goto out_bad;
+ }
+
+out_bad:
+ http_status = VA_HTTP_STATUS_ERROR;
+out:
+ /* handle the response or request we just read */
+ s->read_cb(http_status, s->content, s->content_len, s->is_request);
+ /* restart read handler */
+ s->state = VA_READ_START;
+ s->hdr_pos = 0;
+ s->content_len = 0;
+ s->content_pos = 0;
+ qemu_free(s->content);
+ http_status = VA_HTTP_STATUS_NEW;
+}
+
+static void va_http_send_handler(void *opaque)
+{
+ VAHTState *s = &va_state->send_state;
+ enum va_http_status http_status;
+ int fd = va_state->fd;
+ int ret;
+
+ TRACE("called, fd: %d", fd);
+
+ switch (s->state) {
+ case VA_SEND_START:
+ s->state = VA_SEND_HDR;
+ case VA_SEND_HDR:
+ do {
+ ret = write(fd, s->hdr + s->hdr_pos, s->hdr_len - s->hdr_pos);
+ if (ret <= 0) {
+ break;
+ }
+ s->hdr_pos += ret;
+ } while (s->hdr_pos < s->hdr_len);
+ if (ret == -1) {
+ if (errno == EAGAIN || errno == EWOULDBLOCK || errno == EINTR) {
+ return;
+ } else {
+ LOG("error writing header: %s", strerror(errno));
+ goto out_bad;
+ }
+ } else if (ret == 0) {
+ LOG("connected closed unexpectedly");
+ goto out_bad;
+ } else {
+ s->state = VA_SEND_BODY;
+ }
+ case VA_SEND_BODY:
+ do {
+ ret = write(fd, s->content + s->content_pos,
+ s->content_len - s->content_pos);
+ if (ret <= 0) {
+ break;
+ }
+ s->content_pos += ret;
+ } while (s->content_pos < s->content_len);
+ if (ret == -1) {
+ if (errno == EAGAIN || errno == EWOULDBLOCK || errno == EINTR) {
+ return;
+ } else {
+ LOG("error writing content: %s", strerror(errno));
+ goto out_bad;
+ }
+ } else if (ret == 0) {
+ LOG("connected closed unexpectedly");
+ goto out_bad;
+ } else {
+ http_status = VA_HTTP_STATUS_OK;
+ goto out;
+ }
+ default:
+ LOG("unknown state");
+ goto out_bad;
+ }
+
+out_bad:
+ http_status = VA_HTTP_STATUS_ERROR;
+out:
+ s->send_cb(http_status, s->content, s->content_len);
+ qemu_set_fd_handler(fd, va_http_read_handler, NULL, NULL);
+}
+
+/***********************************************************/
/* functions for starting/managing client/server rpc jobs */
static int va_send_server_response(VAServerJob *server_job)
--
1.7.0.4
^ permalink raw reply related [flat|nested] 74+ messages in thread* [Qemu-devel] Re: [RFC][PATCH v5 04/21] virtagent: transport definitions and job callbacks
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 04/21] virtagent: transport definitions and job callbacks Michael Roth
@ 2010-12-06 22:02 ` Adam Litke
2010-12-06 22:34 ` Michael Roth
2010-12-07 13:44 ` Jes Sorensen
1 sibling, 1 reply; 74+ messages in thread
From: Adam Litke @ 2010-12-06 22:02 UTC (permalink / raw)
To: Michael Roth
Cc: agl, stefanha, Jes.Sorensen, qemu-devel, aliguori, ryanh, abeekhof
On Fri, 2010-12-03 at 12:03 -0600, Michael Roth wrote:
> +static void va_http_send_handler(void *opaque)
> +{
> + VAHTState *s = &va_state->send_state;
> + enum va_http_status http_status;
> + int fd = va_state->fd;
> + int ret;
> +
> + TRACE("called, fd: %d", fd);
> +
> + switch (s->state) {
Why is there a VA_SEND_START state when it always falls through to
VA_SEND_HDR? Is there any difference between these?
> + case VA_SEND_START:
> + s->state = VA_SEND_HDR;
> + case VA_SEND_HDR:
> + do {
> + ret = write(fd, s->hdr + s->hdr_pos, s->hdr_len - s->hdr_pos);
> + if (ret <= 0) {
> + break;
> + }
> + s->hdr_pos += ret;
> + } while (s->hdr_pos < s->hdr_len);
> + if (ret == -1) {
> + if (errno == EAGAIN || errno == EWOULDBLOCK || errno == EINTR) {
> + return;
> + } else {
> + LOG("error writing header: %s", strerror(errno));
> + goto out_bad;
> + }
> + } else if (ret == 0) {
> + LOG("connected closed unexpectedly");
> + goto out_bad;
> + } else {
> + s->state = VA_SEND_BODY;
> + }
> + case VA_SEND_BODY:
> + do {
> + ret = write(fd, s->content + s->content_pos,
> + s->content_len - s->content_pos);
> + if (ret <= 0) {
> + break;
> + }
> + s->content_pos += ret;
> + } while (s->content_pos < s->content_len);
> + if (ret == -1) {
> + if (errno == EAGAIN || errno == EWOULDBLOCK || errno == EINTR) {
> + return;
> + } else {
> + LOG("error writing content: %s", strerror(errno));
> + goto out_bad;
> + }
> + } else if (ret == 0) {
> + LOG("connected closed unexpectedly");
> + goto out_bad;
> + } else {
> + http_status = VA_HTTP_STATUS_OK;
> + goto out;
> + }
> + default:
> + LOG("unknown state");
> + goto out_bad;
> + }
> +
> +out_bad:
> + http_status = VA_HTTP_STATUS_ERROR;
> +out:
> + s->send_cb(http_status, s->content, s->content_len);
> + qemu_set_fd_handler(fd, va_http_read_handler, NULL, NULL);
> +}
--
Thanks,
Adam
^ permalink raw reply [flat|nested] 74+ messages in thread* [Qemu-devel] Re: [RFC][PATCH v5 04/21] virtagent: transport definitions and job callbacks
2010-12-06 22:02 ` [Qemu-devel] " Adam Litke
@ 2010-12-06 22:34 ` Michael Roth
0 siblings, 0 replies; 74+ messages in thread
From: Michael Roth @ 2010-12-06 22:34 UTC (permalink / raw)
To: Adam Litke
Cc: agl, stefanha, Jes.Sorensen, qemu-devel, aliguori, ryanh, abeekhof
On 12/06/2010 04:02 PM, Adam Litke wrote:
> On Fri, 2010-12-03 at 12:03 -0600, Michael Roth wrote:
>> +static void va_http_send_handler(void *opaque)
>> +{
>> + VAHTState *s =&va_state->send_state;
>> + enum va_http_status http_status;
>> + int fd = va_state->fd;
>> + int ret;
>> +
>> + TRACE("called, fd: %d", fd);
>> +
>> + switch (s->state) {
>
> Why is there a VA_SEND_START state when it always falls through to
> VA_SEND_HDR? Is there any difference between these?
>
Nope, not at the moment. I'll stick with just _HDR for now, but if we
ever need to do some initialization or anything before we start
sending/reading that's what this would be for.
>> + case VA_SEND_START:
>> + s->state = VA_SEND_HDR;
>> + case VA_SEND_HDR:
>> + do {
>> + ret = write(fd, s->hdr + s->hdr_pos, s->hdr_len - s->hdr_pos);
>> + if (ret<= 0) {
>> + break;
>> + }
>> + s->hdr_pos += ret;
>> + } while (s->hdr_pos< s->hdr_len);
>> + if (ret == -1) {
>> + if (errno == EAGAIN || errno == EWOULDBLOCK || errno == EINTR) {
>> + return;
>> + } else {
>> + LOG("error writing header: %s", strerror(errno));
>> + goto out_bad;
>> + }
>> + } else if (ret == 0) {
>> + LOG("connected closed unexpectedly");
>> + goto out_bad;
>> + } else {
>> + s->state = VA_SEND_BODY;
>> + }
>> + case VA_SEND_BODY:
>> + do {
>> + ret = write(fd, s->content + s->content_pos,
>> + s->content_len - s->content_pos);
>> + if (ret<= 0) {
>> + break;
>> + }
>> + s->content_pos += ret;
>> + } while (s->content_pos< s->content_len);
>> + if (ret == -1) {
>> + if (errno == EAGAIN || errno == EWOULDBLOCK || errno == EINTR) {
>> + return;
>> + } else {
>> + LOG("error writing content: %s", strerror(errno));
>> + goto out_bad;
>> + }
>> + } else if (ret == 0) {
>> + LOG("connected closed unexpectedly");
>> + goto out_bad;
>> + } else {
>> + http_status = VA_HTTP_STATUS_OK;
>> + goto out;
>> + }
>> + default:
>> + LOG("unknown state");
>> + goto out_bad;
>> + }
>> +
>> +out_bad:
>> + http_status = VA_HTTP_STATUS_ERROR;
>> +out:
>> + s->send_cb(http_status, s->content, s->content_len);
>> + qemu_set_fd_handler(fd, va_http_read_handler, NULL, NULL);
>> +}
>
^ permalink raw reply [flat|nested] 74+ messages in thread
* [Qemu-devel] Re: [RFC][PATCH v5 04/21] virtagent: transport definitions and job callbacks
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 04/21] virtagent: transport definitions and job callbacks Michael Roth
2010-12-06 22:02 ` [Qemu-devel] " Adam Litke
@ 2010-12-07 13:44 ` Jes Sorensen
2010-12-07 17:19 ` Michael Roth
1 sibling, 1 reply; 74+ messages in thread
From: Jes Sorensen @ 2010-12-07 13:44 UTC (permalink / raw)
To: Michael Roth; +Cc: agl, stefanha, abeekhof, qemu-devel, aliguori, ryanh
On 12/03/10 19:03, Michael Roth wrote:
> +static void va_server_read_cb(const char *content, size_t content_len)
> +{
> + xmlrpc_mem_block *resp_xml;
> + VAServerData *server_data = &va_state->server_data;
> + int ret;
> +
> + TRACE("called");
> + resp_xml = xmlrpc_registry_process_call(&server_data->env,
> + server_data->registry,
> + NULL, content, content_len);
> + if (resp_xml == NULL) {
> + LOG("error processing RPC request");
> + goto out_bad;
> + }
> +
> + ret = va_server_job_add(resp_xml);
> + if (ret != 0) {
> + LOG("error adding server job: %s", strerror(ret));
> + }
> +
> + return;
> +out_bad:
> + /* TODO: should reset state here */
> + return;
Looks like some missing error handling is needed here?
> +static void va_rpc_parse_hdr(VAHTState *s)
> +{
> + int i, line_pos = 0;
> + bool first_line = true;
> + char line_buf[4096];
In 03/21 you defined VA_HDR_LEN_MAX to 4096, here you hard code the
value .... sounds like something begging to go wrong.
> +static int va_end_of_header(char *buf, int end_pos)
> +{
> + return !strncmp(buf+(end_pos-2), "\n\r\n", 3);
> +}
Maybe I am missing something here, but it looks like you do a strncmp to
a char that is one past the end of the buffer, or? If this is
intentional, please document it.
All this http parsing code leaves the question open why you do it
manually, instead of relying on a library?
Cheers,
Jes
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Qemu-devel] Re: [RFC][PATCH v5 04/21] virtagent: transport definitions and job callbacks
2010-12-07 13:44 ` Jes Sorensen
@ 2010-12-07 17:19 ` Michael Roth
2010-12-08 19:16 ` Jes Sorensen
0 siblings, 1 reply; 74+ messages in thread
From: Michael Roth @ 2010-12-07 17:19 UTC (permalink / raw)
To: Jes Sorensen; +Cc: agl, stefanha, abeekhof, qemu-devel, aliguori, ryanh
On 12/07/2010 07:44 AM, Jes Sorensen wrote:
> On 12/03/10 19:03, Michael Roth wrote:
>> +static void va_server_read_cb(const char *content, size_t content_len)
>> +{
>> + xmlrpc_mem_block *resp_xml;
>> + VAServerData *server_data =&va_state->server_data;
>> + int ret;
>> +
>> + TRACE("called");
>> + resp_xml = xmlrpc_registry_process_call(&server_data->env,
>> + server_data->registry,
>> + NULL, content, content_len);
>> + if (resp_xml == NULL) {
>> + LOG("error processing RPC request");
>> + goto out_bad;
>> + }
>> +
>> + ret = va_server_job_add(resp_xml);
>> + if (ret != 0) {
>> + LOG("error adding server job: %s", strerror(ret));
>> + }
>> +
>> + return;
>> +out_bad:
>> + /* TODO: should reset state here */
>> + return;
>
> Looks like some missing error handling is needed here?
>
>> +static void va_rpc_parse_hdr(VAHTState *s)
>> +{
>> + int i, line_pos = 0;
>> + bool first_line = true;
>> + char line_buf[4096];
>
> In 03/21 you defined VA_HDR_LEN_MAX to 4096, here you hard code the
> value .... sounds like something begging to go wrong.
>
>> +static int va_end_of_header(char *buf, int end_pos)
>> +{
>> + return !strncmp(buf+(end_pos-2), "\n\r\n", 3);
>> +}
>
> Maybe I am missing something here, but it looks like you do a strncmp to
> a char that is one past the end of the buffer, or? If this is
> intentional, please document it.
>
buf+end_pos points to the last char we read (rather than being an offset
to the current position). So it stops comparing when it reaches
buf+end_pos (buf=0 + end_pos=2 implies 3 characters)
For some reason this confused the hell out of me when I looked over it
again as well. Alternatively I can do:
static int va_end_of_header(char *buf, int end_pos)
{
return !strncmp(buf+(end_pos-2), "\n\r\n", 3);
}
...
va_end_of_header(s->hdr, s->hdr_pos - 1)
->
static int va_end_of_header(char *buf, int cur_pos)
{
return !strncmp(buf+(cur_pos-3), "\n\r\n", 3);
}
...
va_end_of_header(s->hdr, s->hdr_pos);
It does seem easier to parse...
> All this http parsing code leaves the question open why you do it
> manually, instead of relying on a library?
>
Something like libcurl? At some point we didn't attempt to use libraries
provide by xmlrpc-c (which uses libcurl for http transport) for the
client and server. The problem there is that libcurl really wants and
tcp socket read and write from, whereas we need to support tcp/unix
sockets on the host side and isa/virtio serial ports on the guest side.
Even assuming we could hook in wrappers for these other types of
sockets/channels, there's also the added complexity since dropping
virtproxy of multiplexing HTTP/RPCs using a single stream, whereas
something like libcurl would, understandably, assume it has a dedicated
stream to read/write from. So we wouldn't really save any work or code,
unfortunately.
> Cheers,
> Jes
>
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Qemu-devel] Re: [RFC][PATCH v5 04/21] virtagent: transport definitions and job callbacks
2010-12-07 17:19 ` Michael Roth
@ 2010-12-08 19:16 ` Jes Sorensen
0 siblings, 0 replies; 74+ messages in thread
From: Jes Sorensen @ 2010-12-08 19:16 UTC (permalink / raw)
To: Michael Roth; +Cc: agl, stefanha, abeekhof, qemu-devel, aliguori, ryanh
On 12/07/10 18:19, Michael Roth wrote:
> On 12/07/2010 07:44 AM, Jes Sorensen wrote:
>>> +static int va_end_of_header(char *buf, int end_pos)
>>> +{
>>> + return !strncmp(buf+(end_pos-2), "\n\r\n", 3);
>>> +}
>>
>> Maybe I am missing something here, but it looks like you do a strncmp to
>> a char that is one past the end of the buffer, or? If this is
>> intentional, please document it.
>>
>
> buf+end_pos points to the last char we read (rather than being an offset
> to the current position). So it stops comparing when it reaches
> buf+end_pos (buf=0 + end_pos=2 implies 3 characters)
>
> For some reason this confused the hell out of me when I looked over it
> again as well. Alternatively I can do:
>
> static int va_end_of_header(char *buf, int end_pos)
> {
> return !strncmp(buf+(end_pos-2), "\n\r\n", 3);
> }
> ...
> va_end_of_header(s->hdr, s->hdr_pos - 1)
>
> ->
>
> static int va_end_of_header(char *buf, int cur_pos)
> {
> return !strncmp(buf+(cur_pos-3), "\n\r\n", 3);
> }
> ...
> va_end_of_header(s->hdr, s->hdr_pos);
>
> It does seem easier to parse...
I would prefer this, somewhat easier to parse.
>> All this http parsing code leaves the question open why you do it
>> manually, instead of relying on a library?
> Something like libcurl? At some point we didn't attempt to use libraries
> provide by xmlrpc-c (which uses libcurl for http transport) for the
> client and server. The problem there is that libcurl really wants and
> tcp socket read and write from, whereas we need to support tcp/unix
> sockets on the host side and isa/virtio serial ports on the guest side.
>
> Even assuming we could hook in wrappers for these other types of
> sockets/channels, there's also the added complexity since dropping
> virtproxy of multiplexing HTTP/RPCs using a single stream, whereas
> something like libcurl would, understandably, assume it has a dedicated
> stream to read/write from. So we wouldn't really save any work or code,
> unfortunately.
I guess I am just a little worried that we end up with errors in the
code that could have been solved by using a maintainer http library, but
if it isn't feasible I guess not.
Cheers,
Jes
^ permalink raw reply [flat|nested] 74+ messages in thread
* [Qemu-devel] [RFC][PATCH v5 05/21] virtagent: base client definitions
2010-12-03 18:03 [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent Michael Roth
` (3 preceding siblings ...)
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 04/21] virtagent: transport definitions and job callbacks Michael Roth
@ 2010-12-03 18:03 ` Michael Roth
2010-12-07 14:04 ` [Qemu-devel] " Jes Sorensen
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 06/21] virtagent: base server definitions Michael Roth
` (17 subsequent siblings)
22 siblings, 1 reply; 74+ messages in thread
From: Michael Roth @ 2010-12-03 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: agl, stefanha, Jes.Sorensen, mdroth, aliguori, ryanh, abeekhof
Functions for managing client capabilities and creating client RPC jobs.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
qerror.c | 8 +++
qerror.h | 6 +++
virtagent.c | 141 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
virtagent.h | 32 +++++++++++++
4 files changed, 187 insertions(+), 0 deletions(-)
create mode 100644 virtagent.c
create mode 100644 virtagent.h
diff --git a/qerror.c b/qerror.c
index ac2cdaf..c765a57 100644
--- a/qerror.c
+++ b/qerror.c
@@ -200,6 +200,14 @@ static const QErrorStringTable qerror_table[] = {
.error_fmt = QERR_VNC_SERVER_FAILED,
.desc = "Could not start VNC server on %(target)",
},
+ {
+ .error_fmt = QERR_RPC_FAILED,
+ .desc = "An RPC error has occurred",
+ },
+ {
+ .error_fmt = QERR_VA_FAILED,
+ .desc = "An error was reported by virtagent",
+ },
{}
};
diff --git a/qerror.h b/qerror.h
index 943a24b..059c0dc 100644
--- a/qerror.h
+++ b/qerror.h
@@ -165,4 +165,10 @@ QError *qobject_to_qerror(const QObject *obj);
#define QERR_VNC_SERVER_FAILED \
"{ 'class': 'VNCServerFailed', 'data': { 'target': %s } }"
+#define QERR_RPC_FAILED \
+ "{ 'class': 'RPCFailed', 'data': { 'code': %i, 'message': %s } }"
+
+#define QERR_VA_FAILED \
+ "{ 'class': 'VirtagentFailed', 'data': { 'code': %i, 'message': %s } }"
+
#endif /* QERROR_H */
diff --git a/virtagent.c b/virtagent.c
new file mode 100644
index 0000000..34d8545
--- /dev/null
+++ b/virtagent.c
@@ -0,0 +1,141 @@
+/*
+ * virtagent - host/guest RPC client functions
+ *
+ * Copyright IBM Corp. 2010
+ *
+ * Authors:
+ * Adam Litke <aglitke@linux.vnet.ibm.com>
+ * Michael Roth <mdroth@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu_socket.h"
+#include "virtagent-common.h"
+
+static VAClientData *va_client_data;
+
+static void va_set_capabilities(QList *qlist)
+{
+ TRACE("called");
+
+ if (va_client_data == NULL) {
+ LOG("client is uninitialized, unable to set capabilities");
+ return;
+ }
+
+ if (va_client_data->supported_methods != NULL) {
+ qobject_decref(QOBJECT(va_client_data->supported_methods));
+ va_client_data->supported_methods = NULL;
+ TRACE("capabilities reset");
+ }
+
+ if (qlist != NULL) {
+ va_client_data->supported_methods = qlist_copy(qlist);
+ TRACE("capabilities set");
+ }
+}
+
+typedef struct VACmpState {
+ const char *method;
+ bool found;
+} VACmpState;
+
+static void va_cmp_capability_iter(QObject *obj, void *opaque)
+{
+ QString *method = qobject_to_qstring(obj);
+ const char *method_str = NULL;
+ VACmpState *cmp_state = opaque;
+
+ if (method) {
+ method_str = qstring_get_str(method);
+ }
+
+ if (method_str && opaque) {
+ if (strcmp(method_str, cmp_state->method) == 0) {
+ cmp_state->found = 1;
+ }
+ }
+}
+
+static bool va_has_capability(const char *method)
+{
+ VACmpState cmp_state;
+
+ if (method == NULL) {
+ return false;
+ }
+
+ /* we can assume method introspection is available */
+ if (strcmp(method, "system.listMethods") == 0) {
+ return true;
+ }
+ /* assume hello is available to we can probe for/notify the host
+ * rpc server
+ */
+ if (strcmp(method, "va.hello") == 0) {
+ return true;
+ }
+
+ /* compare method against the last retrieved supported method list */
+ cmp_state.method = method;
+ cmp_state.found = false;
+ if (va_client_data->supported_methods) {
+ qlist_iter(va_client_data->supported_methods,
+ va_cmp_capability_iter,
+ (void *)&cmp_state);
+ }
+
+ return cmp_state.found;
+}
+
+int va_client_init(VAClientData *client_data)
+{
+ client_data->supported_methods = NULL;
+ va_client_data = client_data;
+
+ return 0;
+}
+
+static int va_rpc_has_error(xmlrpc_env *env)
+{
+ if (env->fault_occurred) {
+ qerror_report(QERR_RPC_FAILED, env->fault_code, env->fault_string);
+ return -1;
+ }
+ return 0;
+}
+
+static int va_do_rpc(xmlrpc_env *const env, const char *function,
+ xmlrpc_value *params, VAClientCallback *cb,
+ MonitorCompletion *mon_cb, void *mon_data)
+{
+ xmlrpc_mem_block *req_xml;
+ int ret;
+
+ if (!va_has_capability(function)) {
+ LOG("guest agent does not have required capability");
+ ret = -1;
+ goto out;
+ }
+
+ req_xml = XMLRPC_MEMBLOCK_NEW(char, env, 0);
+ xmlrpc_serialize_call(env, req_xml, function, params);
+ if (va_rpc_has_error(env)) {
+ ret = -EINVAL;
+ goto out_free;
+ }
+
+ ret = va_client_job_add(req_xml, cb, mon_cb, mon_data);
+ if (ret) {
+ goto out_free;
+ }
+
+ return ret;
+out_free:
+ XMLRPC_MEMBLOCK_FREE(char, req_xml);
+out:
+ return ret;
+}
diff --git a/virtagent.h b/virtagent.h
new file mode 100644
index 0000000..af36b6a
--- /dev/null
+++ b/virtagent.h
@@ -0,0 +1,32 @@
+/*
+ * virt-agent - host/guest RPC client functions
+ *
+ * Copyright IBM Corp. 2010
+ *
+ * Authors:
+ * Adam Litke <aglitke@linux.vnet.ibm.com>
+ * Michael Roth <mdroth@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef VIRTAGENT_H
+#define VIRTAGENT_H
+
+#include "monitor.h"
+
+#define GUEST_AGENT_PATH_CLIENT "/tmp/virtagent-guest-client.sock"
+#define HOST_AGENT_PATH_CLIENT "/tmp/virtagent-host-client.sock"
+#define VA_MAX_CHUNK_SIZE 4096 /* max bytes at a time for get/send file */
+
+typedef void (VAClientCallback)(const char *resp_data, size_t resp_data_len,
+ MonitorCompletion *mon_cb, void *mon_data);
+typedef struct VAClientData {
+ QList *supported_methods;
+} VAClientData;
+
+int va_client_init(VAClientData *client_data);
+
+#endif /* VIRTAGENT_H */
--
1.7.0.4
^ permalink raw reply related [flat|nested] 74+ messages in thread* [Qemu-devel] [RFC][PATCH v5 06/21] virtagent: base server definitions
2010-12-03 18:03 [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent Michael Roth
` (4 preceding siblings ...)
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 05/21] virtagent: base client definitions Michael Roth
@ 2010-12-03 18:03 ` Michael Roth
2010-12-07 14:07 ` [Qemu-devel] " Jes Sorensen
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 07/21] virtagent: add va.getfile RPC Michael Roth
` (16 subsequent siblings)
22 siblings, 1 reply; 74+ messages in thread
From: Michael Roth @ 2010-12-03 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: agl, stefanha, Jes.Sorensen, mdroth, aliguori, ryanh, abeekhof
Functions for managing server's RPC methods.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
virtagent-server.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++
virtagent-server.h | 30 +++++++++++++++++++++++++
2 files changed, 92 insertions(+), 0 deletions(-)
create mode 100644 virtagent-server.c
create mode 100644 virtagent-server.h
diff --git a/virtagent-server.c b/virtagent-server.c
new file mode 100644
index 0000000..36fbae2
--- /dev/null
+++ b/virtagent-server.c
@@ -0,0 +1,62 @@
+/*
+ * virtagent - host/guest RPC server functions
+ *
+ * Copyright IBM Corp. 2010
+ *
+ * Authors:
+ * Adam Litke <aglitke@linux.vnet.ibm.com>
+ * Michael Roth <mdroth@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+#include <syslog.h>
+#include "qemu_socket.h"
+#include "virtagent-common.h"
+
+static bool va_enable_syslog = false; /* enable syslog'ing of RPCs */
+
+#define SLOG(msg, ...) do { \
+ char msg_buf[1024]; \
+ if (!va_enable_syslog) { \
+ break; \
+ } \
+ snprintf(msg_buf, 1024, msg, ## __VA_ARGS__); \
+ syslog(LOG_INFO, "virtagent, %s", msg_buf); \
+} while(0)
+
+typedef struct RPCFunction {
+ xmlrpc_value *(*func)(xmlrpc_env *env, xmlrpc_value *param, void *unused);
+ const char *func_name;
+} RPCFunction;
+
+static RPCFunction guest_functions[] = {
+ { NULL, NULL }
+};
+static RPCFunction host_functions[] = {
+ { NULL, NULL }
+};
+
+static void va_register_functions(xmlrpc_env *env, xmlrpc_registry *registry,
+ RPCFunction *list)
+{
+ int i;
+ for (i = 0; list[i].func != NULL; ++i) {
+ TRACE("adding func: %s", list[i].func_name);
+ xmlrpc_registry_add_method(env, registry, NULL, list[i].func_name,
+ list[i].func, NULL);
+ }
+}
+
+int va_server_init(VAServerData *server_data, bool is_host)
+{
+ RPCFunction *func_list = is_host ? host_functions : guest_functions;
+
+ va_enable_syslog = !is_host; /* enable logging for guest agent */
+ xmlrpc_env_init(&server_data->env);
+ server_data->registry = xmlrpc_registry_new(&server_data->env);
+ va_register_functions(&server_data->env, server_data->registry, func_list);
+
+ return 0;
+}
diff --git a/virtagent-server.h b/virtagent-server.h
new file mode 100644
index 0000000..dc1870e
--- /dev/null
+++ b/virtagent-server.h
@@ -0,0 +1,30 @@
+/*
+ * virt-agent - host/guest RPC daemon functions
+ *
+ * Copyright IBM Corp. 2010
+ *
+ * Authors:
+ * Michael Roth <mdroth@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include <xmlrpc-c/base.h>
+#include <xmlrpc-c/server.h>
+
+#define GUEST_AGENT_SERVICE_ID "virtagent"
+#define GUEST_AGENT_PATH "/tmp/virtagent-guest.sock"
+#define HOST_AGENT_SERVICE_ID "virtagent-host"
+#define HOST_AGENT_PATH "/tmp/virtagent-host.sock"
+#define VA_GETFILE_MAX 1 << 30
+#define VA_FILEBUF_LEN 16384
+#define VA_DMESG_LEN 16384
+
+typedef struct VAServerData {
+ xmlrpc_env env;
+ xmlrpc_registry *registry;
+} VAServerData;
+
+int va_server_init(VAServerData *server_data, bool is_host);
--
1.7.0.4
^ permalink raw reply related [flat|nested] 74+ messages in thread* [Qemu-devel] [RFC][PATCH v5 07/21] virtagent: add va.getfile RPC
2010-12-03 18:03 [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent Michael Roth
` (5 preceding siblings ...)
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 06/21] virtagent: base server definitions Michael Roth
@ 2010-12-03 18:03 ` Michael Roth
2010-12-06 22:06 ` [Qemu-devel] " Adam Litke
2010-12-07 14:18 ` Jes Sorensen
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 08/21] virtagent: add agent_viewfile qmp/hmp command Michael Roth
` (15 subsequent siblings)
22 siblings, 2 replies; 74+ messages in thread
From: Michael Roth @ 2010-12-03 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: agl, stefanha, Jes.Sorensen, mdroth, aliguori, ryanh, abeekhof
Add RPC to retrieve a guest file. This interface is intended
for smaller reads like peeking at logs and /proc and such.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
virtagent-server.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 59 insertions(+), 0 deletions(-)
diff --git a/virtagent-server.c b/virtagent-server.c
index 36fbae2..a430b58 100644
--- a/virtagent-server.c
+++ b/virtagent-server.c
@@ -26,12 +26,71 @@ static bool va_enable_syslog = false; /* enable syslog'ing of RPCs */
syslog(LOG_INFO, "virtagent, %s", msg_buf); \
} while(0)
+/* RPC functions common to guest/host daemons */
+
+/* va_getfile(): return file contents
+ * rpc return values:
+ * - base64-encoded file contents
+ */
+static xmlrpc_value *va_getfile(xmlrpc_env *env,
+ xmlrpc_value *param,
+ void *user_data)
+{
+ const char *path;
+ char *file_contents = NULL;
+ char buf[VA_FILEBUF_LEN];
+ int fd, ret, count = 0;
+ xmlrpc_value *result = NULL;
+
+ /* parse argument array */
+ xmlrpc_decompose_value(env, param, "(s)", &path);
+ if (env->fault_occurred) {
+ return NULL;
+ }
+
+ SLOG("va_getfile(), path:%s", path);
+
+ fd = open(path, O_RDONLY);
+ if (fd == -1) {
+ LOG("open failed: %s", strerror(errno));
+ xmlrpc_faultf(env, "open failed: %s", strerror(errno));
+ return NULL;
+ }
+
+ while ((ret = read(fd, buf, VA_FILEBUF_LEN)) > 0) {
+ file_contents = qemu_realloc(file_contents, count + VA_FILEBUF_LEN);
+ memcpy(file_contents + count, buf, ret);
+ count += ret;
+ if (count > VA_GETFILE_MAX) {
+ xmlrpc_faultf(env, "max file size (%d bytes) exceeded",
+ VA_GETFILE_MAX);
+ goto EXIT_CLOSE_BAD;
+ }
+ }
+ if (ret == -1) {
+ LOG("read failed: %s", strerror(errno));
+ xmlrpc_faultf(env, "read failed: %s", strerror(errno));
+ goto EXIT_CLOSE_BAD;
+ }
+
+ result = xmlrpc_build_value(env, "6", file_contents, count);
+
+EXIT_CLOSE_BAD:
+ if (file_contents) {
+ qemu_free(file_contents);
+ }
+ close(fd);
+ return result;
+}
+
typedef struct RPCFunction {
xmlrpc_value *(*func)(xmlrpc_env *env, xmlrpc_value *param, void *unused);
const char *func_name;
} RPCFunction;
static RPCFunction guest_functions[] = {
+ { .func = va_getfile,
+ .func_name = "va.getfile" },
{ NULL, NULL }
};
static RPCFunction host_functions[] = {
--
1.7.0.4
^ permalink raw reply related [flat|nested] 74+ messages in thread* [Qemu-devel] Re: [RFC][PATCH v5 07/21] virtagent: add va.getfile RPC
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 07/21] virtagent: add va.getfile RPC Michael Roth
@ 2010-12-06 22:06 ` Adam Litke
2010-12-06 23:23 ` Michael Roth
2010-12-07 14:18 ` Jes Sorensen
1 sibling, 1 reply; 74+ messages in thread
From: Adam Litke @ 2010-12-06 22:06 UTC (permalink / raw)
To: Michael Roth
Cc: agl, stefanha, Jes.Sorensen, qemu-devel, aliguori, ryanh, abeekhof
On Fri, 2010-12-03 at 12:03 -0600, Michael Roth wrote:
> Add RPC to retrieve a guest file. This interface is intended
> for smaller reads like peeking at logs and /proc and such.
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> virtagent-server.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 59 insertions(+), 0 deletions(-)
>
> diff --git a/virtagent-server.c b/virtagent-server.c
> index 36fbae2..a430b58 100644
> --- a/virtagent-server.c
> +++ b/virtagent-server.c
> @@ -26,12 +26,71 @@ static bool va_enable_syslog = false; /* enable syslog'ing of RPCs */
> syslog(LOG_INFO, "virtagent, %s", msg_buf); \
> } while(0)
>
> +/* RPC functions common to guest/host daemons */
> +
> +/* va_getfile(): return file contents
> + * rpc return values:
> + * - base64-encoded file contents
> + */
> +static xmlrpc_value *va_getfile(xmlrpc_env *env,
> + xmlrpc_value *param,
> + void *user_data)
> +{
> + const char *path;
> + char *file_contents = NULL;
> + char buf[VA_FILEBUF_LEN];
> + int fd, ret, count = 0;
> + xmlrpc_value *result = NULL;
> +
> + /* parse argument array */
> + xmlrpc_decompose_value(env, param, "(s)", &path);
> + if (env->fault_occurred) {
> + return NULL;
> + }
> +
> + SLOG("va_getfile(), path:%s", path);
I would log all calls (even those that would fail
xmlrpc_decompose_value(). You might catch someone trying to do something
tricky.
> +
> + fd = open(path, O_RDONLY);
> + if (fd == -1) {
> + LOG("open failed: %s", strerror(errno));
> + xmlrpc_faultf(env, "open failed: %s", strerror(errno));
> + return NULL;
> + }
> +
> + while ((ret = read(fd, buf, VA_FILEBUF_LEN)) > 0) {
> + file_contents = qemu_realloc(file_contents, count + VA_FILEBUF_LEN);
> + memcpy(file_contents + count, buf, ret);
> + count += ret;
> + if (count > VA_GETFILE_MAX) {
> + xmlrpc_faultf(env, "max file size (%d bytes) exceeded",
> + VA_GETFILE_MAX);
> + goto EXIT_CLOSE_BAD;
> + }
> + }
> + if (ret == -1) {
> + LOG("read failed: %s", strerror(errno));
> + xmlrpc_faultf(env, "read failed: %s", strerror(errno));
> + goto EXIT_CLOSE_BAD;
> + }
> +
> + result = xmlrpc_build_value(env, "6", file_contents, count);
> +
> +EXIT_CLOSE_BAD:
> + if (file_contents) {
> + qemu_free(file_contents);
> + }
> + close(fd);
> + return result;
> +}
> +
> typedef struct RPCFunction {
> xmlrpc_value *(*func)(xmlrpc_env *env, xmlrpc_value *param, void *unused);
> const char *func_name;
> } RPCFunction;
>
> static RPCFunction guest_functions[] = {
> + { .func = va_getfile,
> + .func_name = "va.getfile" },
> { NULL, NULL }
> };
> static RPCFunction host_functions[] = {
--
Thanks,
Adam
^ permalink raw reply [flat|nested] 74+ messages in thread* [Qemu-devel] Re: [RFC][PATCH v5 07/21] virtagent: add va.getfile RPC
2010-12-06 22:06 ` [Qemu-devel] " Adam Litke
@ 2010-12-06 23:23 ` Michael Roth
0 siblings, 0 replies; 74+ messages in thread
From: Michael Roth @ 2010-12-06 23:23 UTC (permalink / raw)
To: Adam Litke
Cc: agl, stefanha, Jes.Sorensen, qemu-devel, aliguori, ryanh, abeekhof
On 12/06/2010 04:06 PM, Adam Litke wrote:
> On Fri, 2010-12-03 at 12:03 -0600, Michael Roth wrote:
>> Add RPC to retrieve a guest file. This interface is intended
>> for smaller reads like peeking at logs and /proc and such.
>>
>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>> ---
>> virtagent-server.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 59 insertions(+), 0 deletions(-)
>>
>> diff --git a/virtagent-server.c b/virtagent-server.c
>> index 36fbae2..a430b58 100644
>> --- a/virtagent-server.c
>> +++ b/virtagent-server.c
>> @@ -26,12 +26,71 @@ static bool va_enable_syslog = false; /* enable syslog'ing of RPCs */
>> syslog(LOG_INFO, "virtagent, %s", msg_buf); \
>> } while(0)
>>
>> +/* RPC functions common to guest/host daemons */
>> +
>> +/* va_getfile(): return file contents
>> + * rpc return values:
>> + * - base64-encoded file contents
>> + */
>> +static xmlrpc_value *va_getfile(xmlrpc_env *env,
>> + xmlrpc_value *param,
>> + void *user_data)
>> +{
>> + const char *path;
>> + char *file_contents = NULL;
>> + char buf[VA_FILEBUF_LEN];
>> + int fd, ret, count = 0;
>> + xmlrpc_value *result = NULL;
>> +
>> + /* parse argument array */
>> + xmlrpc_decompose_value(env, param, "(s)",&path);
>> + if (env->fault_occurred) {
>> + return NULL;
>> + }
>> +
>> + SLOG("va_getfile(), path:%s", path);
>
> I would log all calls (even those that would fail
> xmlrpc_decompose_value(). You might catch someone trying to do something
> tricky.
>
Agreed, I'll fix this up for the next round
>> +
>> + fd = open(path, O_RDONLY);
>> + if (fd == -1) {
>> + LOG("open failed: %s", strerror(errno));
>> + xmlrpc_faultf(env, "open failed: %s", strerror(errno));
>> + return NULL;
>> + }
>> +
>> + while ((ret = read(fd, buf, VA_FILEBUF_LEN))> 0) {
>> + file_contents = qemu_realloc(file_contents, count + VA_FILEBUF_LEN);
>> + memcpy(file_contents + count, buf, ret);
>> + count += ret;
>> + if (count> VA_GETFILE_MAX) {
>> + xmlrpc_faultf(env, "max file size (%d bytes) exceeded",
>> + VA_GETFILE_MAX);
>> + goto EXIT_CLOSE_BAD;
>> + }
>> + }
>> + if (ret == -1) {
>> + LOG("read failed: %s", strerror(errno));
>> + xmlrpc_faultf(env, "read failed: %s", strerror(errno));
>> + goto EXIT_CLOSE_BAD;
>> + }
>> +
>> + result = xmlrpc_build_value(env, "6", file_contents, count);
>> +
>> +EXIT_CLOSE_BAD:
>> + if (file_contents) {
>> + qemu_free(file_contents);
>> + }
>> + close(fd);
>> + return result;
>> +}
>> +
>> typedef struct RPCFunction {
>> xmlrpc_value *(*func)(xmlrpc_env *env, xmlrpc_value *param, void *unused);
>> const char *func_name;
>> } RPCFunction;
>>
>> static RPCFunction guest_functions[] = {
>> + { .func = va_getfile,
>> + .func_name = "va.getfile" },
>> { NULL, NULL }
>> };
>> static RPCFunction host_functions[] = {
>
^ permalink raw reply [flat|nested] 74+ messages in thread
* [Qemu-devel] Re: [RFC][PATCH v5 07/21] virtagent: add va.getfile RPC
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 07/21] virtagent: add va.getfile RPC Michael Roth
2010-12-06 22:06 ` [Qemu-devel] " Adam Litke
@ 2010-12-07 14:18 ` Jes Sorensen
2010-12-07 16:00 ` Adam Litke
1 sibling, 1 reply; 74+ messages in thread
From: Jes Sorensen @ 2010-12-07 14:18 UTC (permalink / raw)
To: Michael Roth; +Cc: agl, stefanha, abeekhof, qemu-devel, aliguori, ryanh
On 12/03/10 19:03, Michael Roth wrote:
> Add RPC to retrieve a guest file. This interface is intended
> for smaller reads like peeking at logs and /proc and such.
I think you need to redesign your approach here..... see below.
In 06/21 you had:
+#define VA_GETFILE_MAX 1 << 30
> + while ((ret = read(fd, buf, VA_FILEBUF_LEN)) > 0) {
> + file_contents = qemu_realloc(file_contents, count + VA_FILEBUF_LEN);
> + memcpy(file_contents + count, buf, ret);
UH OH!
realloc will do a malloc and a memcpy of the data, this is going to turn
into a really nasty malloc memcpy loop if someone tries to transfer a
large file using this method. You could end up with almost 4GB of
parallel allocations for a guest that might have been configured as a
1GB guest. This would allow the guest to effectively blow the expected
memory consumption out of the water. It's not exactly going to be fast
either :(
Maybe use a tmp file, and write data out to that as you receive it to
avoid the malloc ballooning.
Jes
^ permalink raw reply [flat|nested] 74+ messages in thread* [Qemu-devel] Re: [RFC][PATCH v5 07/21] virtagent: add va.getfile RPC
2010-12-07 14:18 ` Jes Sorensen
@ 2010-12-07 16:00 ` Adam Litke
2010-12-08 19:19 ` Jes Sorensen
0 siblings, 1 reply; 74+ messages in thread
From: Adam Litke @ 2010-12-07 16:00 UTC (permalink / raw)
To: Jes Sorensen
Cc: agl, stefanha, abeekhof, Michael Roth, qemu-devel, aliguori, ryanh
Hi Jes, you raise some good points and pitfalls with the current getfile
approach. I've been thinking about an alternative and am wondering what
you (and others) think...
First off, I think we should switch to a copyfile() API that allows us
to avoid presenting the file contents to the user. Neither the human
monitor nor the control monitor are designed to be file pagers. Let the
user decide how to consume the data once it has been transferred. Now
we don't need to care if the file is binary or text.
The virtagent RPC protocol is bi-directional and supports asynchronous
events. We can use these to implement a better copyfile RPC that can
transfer larger files without wasting memory. The host issues a
copyfile(<guest-path>, <host-path>) RPC. The immediate result of this
call will indicate whether the guest is able to initiate the transfer.
The guest will generate a series of events (<offset>, <size>, <payload>)
until the entire contents has been transferred. The host and guest
could negotiate the chunk size if necessary. Once the transfer is
complete, the guest sends a final event to indicate this (<file-size>,
0).
This interface could be integrated into the monitor with a pair of
commands (va_copyfile and info va_copyfile), the former used to initiate
transfers and the latter to check on the status.
Thoughts on this?
On Tue, 2010-12-07 at 15:18 +0100, Jes Sorensen wrote:
> On 12/03/10 19:03, Michael Roth wrote:
> > Add RPC to retrieve a guest file. This interface is intended
> > for smaller reads like peeking at logs and /proc and such.
>
> I think you need to redesign your approach here..... see below.
>
> In 06/21 you had:
>
> +#define VA_GETFILE_MAX 1 << 30
>
> > + while ((ret = read(fd, buf, VA_FILEBUF_LEN)) > 0) {
> > + file_contents = qemu_realloc(file_contents, count + VA_FILEBUF_LEN);
> > + memcpy(file_contents + count, buf, ret);
>
> UH OH!
>
> realloc will do a malloc and a memcpy of the data, this is going to turn
> into a really nasty malloc memcpy loop if someone tries to transfer a
> large file using this method. You could end up with almost 4GB of
> parallel allocations for a guest that might have been configured as a
> 1GB guest. This would allow the guest to effectively blow the expected
> memory consumption out of the water. It's not exactly going to be fast
> either :(
>
> Maybe use a tmp file, and write data out to that as you receive it to
> avoid the malloc ballooning.
>
> Jes
--
Thanks,
Adam
^ permalink raw reply [flat|nested] 74+ messages in thread* [Qemu-devel] Re: [RFC][PATCH v5 07/21] virtagent: add va.getfile RPC
2010-12-07 16:00 ` Adam Litke
@ 2010-12-08 19:19 ` Jes Sorensen
2010-12-09 14:40 ` Adam Litke
0 siblings, 1 reply; 74+ messages in thread
From: Jes Sorensen @ 2010-12-08 19:19 UTC (permalink / raw)
To: Adam Litke
Cc: agl, stefanha, abeekhof, Michael Roth, qemu-devel, aliguori, ryanh
On 12/07/10 17:00, Adam Litke wrote:
> Hi Jes, you raise some good points and pitfalls with the current getfile
> approach. I've been thinking about an alternative and am wondering what
> you (and others) think...
>
> First off, I think we should switch to a copyfile() API that allows us
> to avoid presenting the file contents to the user. Neither the human
> monitor nor the control monitor are designed to be file pagers. Let the
> user decide how to consume the data once it has been transferred. Now
> we don't need to care if the file is binary or text.
>
> The virtagent RPC protocol is bi-directional and supports asynchronous
> events. We can use these to implement a better copyfile RPC that can
> transfer larger files without wasting memory. The host issues a
> copyfile(<guest-path>, <host-path>) RPC. The immediate result of this
> call will indicate whether the guest is able to initiate the transfer.
> The guest will generate a series of events (<offset>, <size>, <payload>)
> until the entire contents has been transferred. The host and guest
> could negotiate the chunk size if necessary. Once the transfer is
> complete, the guest sends a final event to indicate this (<file-size>,
> 0).
>
> This interface could be integrated into the monitor with a pair of
> commands (va_copyfile and info va_copyfile), the former used to initiate
> transfers and the latter to check on the status.
>
> Thoughts on this?
Hi Adam,
This sounds a lot safer than the current approach. Intuitively I would
think it should be the host controlling the copy, but otherwise it
sounds good. Or is there a reason why the guest should control it?
I think it is vital that we do it in a way where a copy cannot blow
QEMU's memory consumption out of the water, but the approach you suggest
seems to take care of that.
Cheers,
Jes
^ permalink raw reply [flat|nested] 74+ messages in thread
* [Qemu-devel] Re: [RFC][PATCH v5 07/21] virtagent: add va.getfile RPC
2010-12-08 19:19 ` Jes Sorensen
@ 2010-12-09 14:40 ` Adam Litke
2010-12-09 21:04 ` Michael Roth
0 siblings, 1 reply; 74+ messages in thread
From: Adam Litke @ 2010-12-09 14:40 UTC (permalink / raw)
To: Jes Sorensen
Cc: agl, stefanha, abeekhof, Michael Roth, qemu-devel, aliguori, ryanh
On Wed, 2010-12-08 at 20:19 +0100, Jes Sorensen wrote:
> On 12/07/10 17:00, Adam Litke wrote:
> > Hi Jes, you raise some good points and pitfalls with the current getfile
> > approach. I've been thinking about an alternative and am wondering what
> > you (and others) think...
> >
> > First off, I think we should switch to a copyfile() API that allows us
> > to avoid presenting the file contents to the user. Neither the human
> > monitor nor the control monitor are designed to be file pagers. Let the
> > user decide how to consume the data once it has been transferred. Now
> > we don't need to care if the file is binary or text.
> >
> > The virtagent RPC protocol is bi-directional and supports asynchronous
> > events. We can use these to implement a better copyfile RPC that can
> > transfer larger files without wasting memory. The host issues a
> > copyfile(<guest-path>, <host-path>) RPC. The immediate result of this
> > call will indicate whether the guest is able to initiate the transfer.
> > The guest will generate a series of events (<offset>, <size>, <payload>)
> > until the entire contents has been transferred. The host and guest
> > could negotiate the chunk size if necessary. Once the transfer is
> > complete, the guest sends a final event to indicate this (<file-size>,
> > 0).
> >
> > This interface could be integrated into the monitor with a pair of
> > commands (va_copyfile and info va_copyfile), the former used to initiate
> > transfers and the latter to check on the status.
> >
> > Thoughts on this?
>
> Hi Adam,
>
> This sounds a lot safer than the current approach. Intuitively I would
> think it should be the host controlling the copy, but otherwise it
> sounds good. Or is there a reason why the guest should control it?
Actually, a host-controlled interface would be both simpler to implement
(on both ends) and would concentrate control in the host (which is what
we probably want anyway).
> I think it is vital that we do it in a way where a copy cannot blow
> QEMU's memory consumption out of the water, but the approach you suggest
> seems to take care of that.
>
> Cheers,
> Jes
>
--
Thanks,
Adam
^ permalink raw reply [flat|nested] 74+ messages in thread
* [Qemu-devel] Re: [RFC][PATCH v5 07/21] virtagent: add va.getfile RPC
2010-12-09 14:40 ` Adam Litke
@ 2010-12-09 21:04 ` Michael Roth
2010-12-10 6:38 ` Jes Sorensen
0 siblings, 1 reply; 74+ messages in thread
From: Michael Roth @ 2010-12-09 21:04 UTC (permalink / raw)
To: Adam Litke
Cc: agl, stefanha, abeekhof, qemu-devel, aliguori, ryanh, Jes Sorensen
On 12/09/2010 08:40 AM, Adam Litke wrote:
> On Wed, 2010-12-08 at 20:19 +0100, Jes Sorensen wrote:
>> On 12/07/10 17:00, Adam Litke wrote:
>>> Hi Jes, you raise some good points and pitfalls with the current getfile
>>> approach. I've been thinking about an alternative and am wondering what
>>> you (and others) think...
>>>
>>> First off, I think we should switch to a copyfile() API that allows us
>>> to avoid presenting the file contents to the user. Neither the human
>>> monitor nor the control monitor are designed to be file pagers. Let the
>>> user decide how to consume the data once it has been transferred. Now
>>> we don't need to care if the file is binary or text.
>>>
>>> The virtagent RPC protocol is bi-directional and supports asynchronous
>>> events. We can use these to implement a better copyfile RPC that can
>>> transfer larger files without wasting memory. The host issues a
>>> copyfile(<guest-path>,<host-path>) RPC. The immediate result of this
>>> call will indicate whether the guest is able to initiate the transfer.
>>> The guest will generate a series of events (<offset>,<size>,<payload>)
>>> until the entire contents has been transferred. The host and guest
>>> could negotiate the chunk size if necessary. Once the transfer is
>>> complete, the guest sends a final event to indicate this (<file-size>,
>>> 0).
>>>
>>> This interface could be integrated into the monitor with a pair of
>>> commands (va_copyfile and info va_copyfile), the former used to initiate
>>> transfers and the latter to check on the status.
>>>
>>> Thoughts on this?
>>
>> Hi Adam,
>>
>> This sounds a lot safer than the current approach. Intuitively I would
>> think it should be the host controlling the copy, but otherwise it
>> sounds good. Or is there a reason why the guest should control it?
>
> Actually, a host-controlled interface would be both simpler to implement
> (on both ends) and would concentrate control in the host (which is what
> we probably want anyway).
>
I also like the host-controlled mechanism. I think the difficulty is
about the same either way though. Both would require an FD to be kept
open, and some mechanism to read/seek in chunks and then send it back.
I don't think this should replace the underlying RPC for viewfile
however. I'd much rather it be an additional RPC, and we just put strict
limits on the size of files viewfile/getfile can handle and make it
clear that it is intended for quickly viewing text. I'll rename the
getfile RPC to viewfile to make this a bit clearer as well.
>> I think it is vital that we do it in a way where a copy cannot blow
>> QEMU's memory consumption out of the water, but the approach you suggest
>> seems to take care of that.
>>
>> Cheers,
>> Jes
>>
>
^ permalink raw reply [flat|nested] 74+ messages in thread
* [Qemu-devel] Re: [RFC][PATCH v5 07/21] virtagent: add va.getfile RPC
2010-12-09 21:04 ` Michael Roth
@ 2010-12-10 6:38 ` Jes Sorensen
0 siblings, 0 replies; 74+ messages in thread
From: Jes Sorensen @ 2010-12-10 6:38 UTC (permalink / raw)
To: Michael Roth
Cc: agl, stefanha, abeekhof, qemu-devel, aliguori, ryanh, Adam Litke
On 12/09/10 22:04, Michael Roth wrote:
> On 12/09/2010 08:40 AM, Adam Litke wrote:
>> Actually, a host-controlled interface would be both simpler to implement
>> (on both ends) and would concentrate control in the host (which is what
>> we probably want anyway).
>
> I also like the host-controlled mechanism. I think the difficulty is
> about the same either way though. Both would require an FD to be kept
> open, and some mechanism to read/seek in chunks and then send it back.
> I don't think this should replace the underlying RPC for viewfile
> however. I'd much rather it be an additional RPC, and we just put strict
> limits on the size of files viewfile/getfile can handle and make it
> clear that it is intended for quickly viewing text. I'll rename the
> getfile RPC to viewfile to make this a bit clearer as well.
I really think the viewfile stuff needs to go away, it is a rats trap
for things that can go wrong. It would really only be useful from the
human monitor, whereas any other application will be happy to implement
it at the higher level.
Having it in the human monitor you risk messing it up by viewing a
binary file and you end up having to kill you guest to fix it.
Putting artificial limits on it means someone will eventually try to
change those.
Lets keep this at the higher level, where it IMHO belongs.
Cheers,
Jes
^ permalink raw reply [flat|nested] 74+ messages in thread
* [Qemu-devel] [RFC][PATCH v5 08/21] virtagent: add agent_viewfile qmp/hmp command
2010-12-03 18:03 [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent Michael Roth
` (6 preceding siblings ...)
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 07/21] virtagent: add va.getfile RPC Michael Roth
@ 2010-12-03 18:03 ` Michael Roth
2010-12-06 22:08 ` [Qemu-devel] " Adam Litke
2010-12-07 14:26 ` Jes Sorensen
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 09/21] virtagent: add va.getdmesg RPC Michael Roth
` (14 subsequent siblings)
22 siblings, 2 replies; 74+ messages in thread
From: Michael Roth @ 2010-12-03 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: agl, stefanha, Jes.Sorensen, mdroth, aliguori, ryanh, abeekhof
Utilize the getfile RPC to provide a means to view text files in the
guest. Getfile can handle binary files as well but we don't advertise
that here due to the special handling requiring to store it and provide
it back to the user (base64 encoding it for instance). Hence the
otherwise confusing "viewfile" as opposed to "getfile".
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
hmp-commands.hx | 16 +++++++++
monitor.c | 1 +
qmp-commands.hx | 33 +++++++++++++++++++
virtagent.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
virtagent.h | 3 ++
5 files changed, 149 insertions(+), 0 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index e5585ba..423c752 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1212,6 +1212,22 @@ show available trace events and their state
ETEXI
#endif
+ {
+ .name = "agent_viewfile",
+ .args_type = "filepath:s",
+ .params = "filepath",
+ .help = "Echo a file from the guest filesystem",
+ .user_print = do_agent_viewfile_print,
+ .mhandler.cmd_async = do_agent_viewfile,
+ .flags = MONITOR_CMD_ASYNC,
+ },
+
+STEXI
+@item agent_viewfile @var{filepath}
+@findex agent_viewfile
+Echo the file identified by @var{filepath} on the guest filesystem
+ETEXI
+
STEXI
@end table
ETEXI
diff --git a/monitor.c b/monitor.c
index 8cee35d..145895d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -56,6 +56,7 @@
#include "json-parser.h"
#include "osdep.h"
#include "exec-all.h"
+#include "virtagent.h"
#ifdef CONFIG_SIMPLE_TRACE
#include "trace.h"
#endif
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 793cf1c..efa2137 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -738,6 +738,39 @@ Example:
EQMP
{
+ .name = "agent_viewfile",
+ .args_type = "filepath:s",
+ .params = "filepath",
+ .help = "Echo a file from the guest filesystem",
+ .user_print = monitor_user_noop,
+ .mhandler.cmd_async = do_agent_viewfile,
+ .flags = MONITOR_CMD_ASYNC,
+ },
+
+STEXI
+@item agent_viewfile @var{filepath}
+@findex agent_viewfile
+Echo the file identified by @var{filepath} on the guest filesystem
+ETEXI
+SQMP
+agent_viewfile
+--------
+
+Echo the file identified by @var{filepath} from the guest filesystem.
+
+Arguments:
+
+- "filepath": Full guest path of the desired file
+
+Example:
+
+-> { "execute": "agent_viewfile",
+ "arguments": { "filepath": "/sys/kernel/kexec_loaded" } }
+<- { "return": { "contents": "0" } }
+
+EQMP
+
+ {
.name = "qmp_capabilities",
.args_type = "",
.params = "",
diff --git a/virtagent.c b/virtagent.c
index 34d8545..4a4dc8a 100644
--- a/virtagent.c
+++ b/virtagent.c
@@ -139,3 +139,99 @@ out_free:
out:
return ret;
}
+
+/* QMP/HMP RPC client functions */
+
+void do_agent_viewfile_print(Monitor *mon, const QObject *data)
+{
+ QDict *qdict;
+ const char *contents = NULL;
+ int i;
+
+ qdict = qobject_to_qdict(data);
+ if (!qdict_haskey(qdict, "contents")) {
+ return;
+ }
+
+ contents = qdict_get_str(qdict, "contents");
+ if (contents != NULL) {
+ /* monitor_printf truncates so do it in chunks. also, file_contents
+ * may not be null-termed at proper location so explicitly calc
+ * last chunk sizes */
+ for (i = 0; i < strlen(contents); i += 1024) {
+ monitor_printf(mon, "%.1024s", contents + i);
+ }
+ }
+ monitor_printf(mon, "\n");
+}
+
+static void do_agent_viewfile_cb(const char *resp_data,
+ size_t resp_data_len,
+ MonitorCompletion *mon_cb,
+ void *mon_data)
+{
+ xmlrpc_value *resp = NULL;
+ char *file_contents = NULL;
+ size_t file_size;
+ int ret;
+ xmlrpc_env env;
+ QDict *qdict = qdict_new();
+
+ if (resp_data == NULL) {
+ LOG("error handling RPC request");
+ goto out_no_resp;
+ }
+
+ xmlrpc_env_init(&env);
+ resp = xmlrpc_parse_response(&env, resp_data, resp_data_len);
+ if (va_rpc_has_error(&env)) {
+ ret = -1;
+ goto out_no_resp;
+ }
+
+ xmlrpc_parse_value(&env, resp, "6", &file_contents, &file_size);
+ if (va_rpc_has_error(&env)) {
+ ret = -1;
+ goto out;
+ }
+
+ if (file_contents != NULL) {
+ qdict_put(qdict, "contents",
+ qstring_from_substr(file_contents, 0, file_size-1));
+ }
+
+out:
+ xmlrpc_DECREF(resp);
+out_no_resp:
+ if (mon_cb) {
+ mon_cb(mon_data, QOBJECT(qdict));
+ }
+ qobject_decref(QOBJECT(qdict));
+}
+
+/*
+ * do_agent_viewfile(): View a text file in the guest
+ */
+int do_agent_viewfile(Monitor *mon, const QDict *mon_params,
+ MonitorCompletion cb, void *opaque)
+{
+ xmlrpc_env env;
+ xmlrpc_value *params;
+ const char *filepath;
+ int ret;
+
+ filepath = qdict_get_str(mon_params, "filepath");
+ xmlrpc_env_init(&env);
+ params = xmlrpc_build_value(&env, "(s)", filepath);
+ if (va_rpc_has_error(&env)) {
+ return -1;
+ }
+
+ ret = va_do_rpc(&env, "va.getfile", params, do_agent_viewfile_cb, cb,
+ opaque);
+ if (ret) {
+ qerror_report(QERR_VA_FAILED, ret, strerror(ret));
+ }
+ xmlrpc_DECREF(params);
+ return ret;
+}
diff --git a/virtagent.h b/virtagent.h
index af36b6a..5960616 100644
--- a/virtagent.h
+++ b/virtagent.h
@@ -28,5 +28,8 @@ typedef struct VAClientData {
} VAClientData;
int va_client_init(VAClientData *client_data);
+void do_agent_viewfile_print(Monitor *mon, const QObject *qobject);
+int do_agent_viewfile(Monitor *mon, const QDict *mon_params,
+ MonitorCompletion cb, void *opaque);
#endif /* VIRTAGENT_H */
--
1.7.0.4
^ permalink raw reply related [flat|nested] 74+ messages in thread* [Qemu-devel] Re: [RFC][PATCH v5 08/21] virtagent: add agent_viewfile qmp/hmp command
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 08/21] virtagent: add agent_viewfile qmp/hmp command Michael Roth
@ 2010-12-06 22:08 ` Adam Litke
2010-12-06 23:20 ` Michael Roth
2010-12-07 14:26 ` Jes Sorensen
1 sibling, 1 reply; 74+ messages in thread
From: Adam Litke @ 2010-12-06 22:08 UTC (permalink / raw)
To: Michael Roth
Cc: agl, stefanha, Jes.Sorensen, qemu-devel, aliguori, ryanh, abeekhof
On Fri, 2010-12-03 at 12:03 -0600, Michael Roth wrote:
> Utilize the getfile RPC to provide a means to view text files in the
> guest. Getfile can handle binary files as well but we don't advertise
> that here due to the special handling requiring to store it and provide
> it back to the user (base64 encoding it for instance). Hence the
> otherwise confusing "viewfile" as opposed to "getfile".
What happens to the monitor if you use this to view a binary file?
Retrieving binary files progmatically using the QMP interface is a valid
use case right? If so, I don't think it makes sense to introduce
confusion by renaming the rpc function from getfile to viewfile when we
are just exposing the getfile interface.
--
Thanks,
Adam
^ permalink raw reply [flat|nested] 74+ messages in thread
* [Qemu-devel] Re: [RFC][PATCH v5 08/21] virtagent: add agent_viewfile qmp/hmp command
2010-12-06 22:08 ` [Qemu-devel] " Adam Litke
@ 2010-12-06 23:20 ` Michael Roth
2010-12-07 14:09 ` Michael Roth
0 siblings, 1 reply; 74+ messages in thread
From: Michael Roth @ 2010-12-06 23:20 UTC (permalink / raw)
To: Adam Litke
Cc: agl, stefanha, Jes.Sorensen, qemu-devel, aliguori, ryanh, abeekhof
On 12/06/2010 04:08 PM, Adam Litke wrote:
> On Fri, 2010-12-03 at 12:03 -0600, Michael Roth wrote:
>> Utilize the getfile RPC to provide a means to view text files in the
>> guest. Getfile can handle binary files as well but we don't advertise
>> that here due to the special handling requiring to store it and provide
>> it back to the user (base64 encoding it for instance). Hence the
>> otherwise confusing "viewfile" as opposed to "getfile".
>
> What happens to the monitor if you use this to view a binary file?
At the very least we probably get a lot of truncated files from the
binary->string conversion via monitor_printf(). Im not sure how the
qobject/json layer would deal with things.
> Retrieving binary files progmatically using the QMP interface is a valid
> use case right?
For getfile (the RPC), but not for viewfile (HMP/QMP). It's doable, but
we'd *have to* pass this data to the user as base64-encoded data at the
QMP level. At the HMP level I think we're good either way, since we
could just base64 decode in the print function.
So in the case of QMP we'd be pushing complexity to the user in exchange
for not having a seperate plain-text-only interface.
Either way seems reasonable, but I'd been planning on adding a seperate
`agent_copyfile <remote_path> <local_path>` command for dealing with
binary data, and making viewfile quick and easy for plain text (both for
HMP and QMP).
^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [Qemu-devel] Re: [RFC][PATCH v5 08/21] virtagent: add agent_viewfile qmp/hmp command
2010-12-06 23:20 ` Michael Roth
@ 2010-12-07 14:09 ` Michael Roth
0 siblings, 0 replies; 74+ messages in thread
From: Michael Roth @ 2010-12-07 14:09 UTC (permalink / raw)
To: Adam Litke
Cc: agl, stefanha, Jes.Sorensen, qemu-devel, aliguori, ryanh, abeekhof
On 12/06/2010 05:20 PM, Michael Roth wrote:
> On 12/06/2010 04:08 PM, Adam Litke wrote:
>> On Fri, 2010-12-03 at 12:03 -0600, Michael Roth wrote:
>>> Utilize the getfile RPC to provide a means to view text files in the
>>> guest. Getfile can handle binary files as well but we don't advertise
>>> that here due to the special handling requiring to store it and provide
>>> it back to the user (base64 encoding it for instance). Hence the
>>> otherwise confusing "viewfile" as opposed to "getfile".
>>
>> What happens to the monitor if you use this to view a binary file?
>
> At the very least we probably get a lot of truncated files from the
> binary->string conversion via monitor_printf(). Im not sure how the
> qobject/json layer would deal with things.
>
>> Retrieving binary files progmatically using the QMP interface is a valid
>> use case right?
>
> For getfile (the RPC), but not for viewfile (HMP/QMP). It's doable, but
> we'd *have to* pass this data to the user as base64-encoded data at the
> QMP level. At the HMP level I think we're good either way, since we
> could just base64 decode in the print function.
>
> So in the case of QMP we'd be pushing complexity to the user in exchange
> for not having a seperate plain-text-only interface.
>
> Either way seems reasonable, but I'd been planning on adding a seperate
> `agent_copyfile <remote_path> <local_path>` command for dealing with
> binary data, and making viewfile quick and easy for plain text (both for
> HMP and QMP).
>
Although, agent_copyfile doesn't seem like the right approach looking at
things like future libvirt integration. So we will most likely end up
with a QMP command that passes base64-encoded binary data to the
end-user for binary data, which we can provide a pretty-printing HMP
function to decode. We'd need to take care to differentiate the HMP
command from the QMP one however, else we'd have users tempted to do
something like:
echo "agent_getfile /remotepath/rand.bin" | socat stdin
unix-connect:monitor.sock > /localpath/rand.bin
to avoid having to decode the data. Would documenting the HMP
counterpart as being reliable only for plain-text be sufficient? Or
Should be have QMP:agent_getfile() and HMP:agent_viewfile()?
^ permalink raw reply [flat|nested] 74+ messages in thread
* [Qemu-devel] Re: [RFC][PATCH v5 08/21] virtagent: add agent_viewfile qmp/hmp command
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 08/21] virtagent: add agent_viewfile qmp/hmp command Michael Roth
2010-12-06 22:08 ` [Qemu-devel] " Adam Litke
@ 2010-12-07 14:26 ` Jes Sorensen
2010-12-09 21:12 ` Michael Roth
1 sibling, 1 reply; 74+ messages in thread
From: Jes Sorensen @ 2010-12-07 14:26 UTC (permalink / raw)
To: Michael Roth; +Cc: agl, stefanha, abeekhof, qemu-devel, aliguori, ryanh
On 12/03/10 19:03, Michael Roth wrote:
> Utilize the getfile RPC to provide a means to view text files in the
> guest. Getfile can handle binary files as well but we don't advertise
> that here due to the special handling requiring to store it and provide
> it back to the user (base64 encoding it for instance). Hence the
> otherwise confusing "viewfile" as opposed to "getfile".
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> hmp-commands.hx | 16 +++++++++
> monitor.c | 1 +
> qmp-commands.hx | 33 +++++++++++++++++++
> virtagent.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> virtagent.h | 3 ++
> 5 files changed, 149 insertions(+), 0 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index e5585ba..423c752 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1212,6 +1212,22 @@ show available trace events and their state
> ETEXI
> #endif
>
> + {
> + .name = "agent_viewfile",
> + .args_type = "filepath:s",
> + .params = "filepath",
> + .help = "Echo a file from the guest filesystem",
> + .user_print = do_agent_viewfile_print,
> + .mhandler.cmd_async = do_agent_viewfile,
> + .flags = MONITOR_CMD_ASYNC,
> + },
> +
> +STEXI
> +@item agent_viewfile @var{filepath}
> +@findex agent_viewfile
> +Echo the file identified by @var{filepath} on the guest filesystem
> +ETEXI
> +
> STEXI
> @end table
> ETEXI
> diff --git a/monitor.c b/monitor.c
> index 8cee35d..145895d 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -56,6 +56,7 @@
> #include "json-parser.h"
> #include "osdep.h"
> #include "exec-all.h"
> +#include "virtagent.h"
> #ifdef CONFIG_SIMPLE_TRACE
> #include "trace.h"
> #endif
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 793cf1c..efa2137 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -738,6 +738,39 @@ Example:
> EQMP
>
> {
> + .name = "agent_viewfile",
> + .args_type = "filepath:s",
> + .params = "filepath",
> + .help = "Echo a file from the guest filesystem",
> + .user_print = monitor_user_noop,
> + .mhandler.cmd_async = do_agent_viewfile,
> + .flags = MONITOR_CMD_ASYNC,
> + },
> +
> +STEXI
> +@item agent_viewfile @var{filepath}
> +@findex agent_viewfile
> +Echo the file identified by @var{filepath} on the guest filesystem
> +ETEXI
> +SQMP
> +agent_viewfile
> +--------
> +
> +Echo the file identified by @var{filepath} from the guest filesystem.
> +
> +Arguments:
> +
> +- "filepath": Full guest path of the desired file
> +
> +Example:
> +
> +-> { "execute": "agent_viewfile",
> + "arguments": { "filepath": "/sys/kernel/kexec_loaded" } }
> +<- { "return": { "contents": "0" } }
> +
> +EQMP
> +
> + {
> .name = "qmp_capabilities",
> .args_type = "",
> .params = "",
> diff --git a/virtagent.c b/virtagent.c
> index 34d8545..4a4dc8a 100644
> --- a/virtagent.c
> +++ b/virtagent.c
> @@ -139,3 +139,99 @@ out_free:
> out:
> return ret;
> }
> +
> +/* QMP/HMP RPC client functions */
> +
> +void do_agent_viewfile_print(Monitor *mon, const QObject *data)
> +{
> + QDict *qdict;
> + const char *contents = NULL;
> + int i;
> +
> + qdict = qobject_to_qdict(data);
> + if (!qdict_haskey(qdict, "contents")) {
> + return;
> + }
> +
> + contents = qdict_get_str(qdict, "contents");
> + if (contents != NULL) {
> + /* monitor_printf truncates so do it in chunks. also, file_contents
> + * may not be null-termed at proper location so explicitly calc
> + * last chunk sizes */
> + for (i = 0; i < strlen(contents); i += 1024) {
> + monitor_printf(mon, "%.1024s", contents + i);
> + }
> + }
> + monitor_printf(mon, "\n");
> +}
> +
> +static void do_agent_viewfile_cb(const char *resp_data,
> + size_t resp_data_len,
> + MonitorCompletion *mon_cb,
> + void *mon_data)
> +{
> + xmlrpc_value *resp = NULL;
> + char *file_contents = NULL;
> + size_t file_size;
> + int ret;
> + xmlrpc_env env;
> + QDict *qdict = qdict_new();
> +
> + if (resp_data == NULL) {
> + LOG("error handling RPC request");
> + goto out_no_resp;
> + }
> +
> + xmlrpc_env_init(&env);
> + resp = xmlrpc_parse_response(&env, resp_data, resp_data_len);
> + if (va_rpc_has_error(&env)) {
> + ret = -1;
> + goto out_no_resp;
> + }
> +
> + xmlrpc_parse_value(&env, resp, "6", &file_contents, &file_size);
> + if (va_rpc_has_error(&env)) {
> + ret = -1;
> + goto out;
I believe this suffers from the same architectural problem I mentioned
in my comment to 07/21 - you don't restrict the file size, so it could
blow up the QEMU process on the host trying to view the wrong file.
I really think it is a bad idea to put this kind of command into the
monitor.
Jes
^ permalink raw reply [flat|nested] 74+ messages in thread* [Qemu-devel] Re: [RFC][PATCH v5 08/21] virtagent: add agent_viewfile qmp/hmp command
2010-12-07 14:26 ` Jes Sorensen
@ 2010-12-09 21:12 ` Michael Roth
2010-12-10 6:43 ` Jes Sorensen
0 siblings, 1 reply; 74+ messages in thread
From: Michael Roth @ 2010-12-09 21:12 UTC (permalink / raw)
To: Jes Sorensen; +Cc: agl, stefanha, abeekhof, qemu-devel, aliguori, ryanh
On 12/07/2010 08:26 AM, Jes Sorensen wrote:
> On 12/03/10 19:03, Michael Roth wrote:
>> Utilize the getfile RPC to provide a means to view text files in the
>> guest. Getfile can handle binary files as well but we don't advertise
>> that here due to the special handling requiring to store it and provide
>> it back to the user (base64 encoding it for instance). Hence the
>> otherwise confusing "viewfile" as opposed to "getfile".
>>
>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>> ---
>> hmp-commands.hx | 16 +++++++++
>> monitor.c | 1 +
>> qmp-commands.hx | 33 +++++++++++++++++++
>> virtagent.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> virtagent.h | 3 ++
>> 5 files changed, 149 insertions(+), 0 deletions(-)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index e5585ba..423c752 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -1212,6 +1212,22 @@ show available trace events and their state
>> ETEXI
>> #endif
>>
>> + {
>> + .name = "agent_viewfile",
>> + .args_type = "filepath:s",
>> + .params = "filepath",
>> + .help = "Echo a file from the guest filesystem",
>> + .user_print = do_agent_viewfile_print,
>> + .mhandler.cmd_async = do_agent_viewfile,
>> + .flags = MONITOR_CMD_ASYNC,
>> + },
>> +
>> +STEXI
>> +@item agent_viewfile @var{filepath}
>> +@findex agent_viewfile
>> +Echo the file identified by @var{filepath} on the guest filesystem
>> +ETEXI
>> +
>> STEXI
>> @end table
>> ETEXI
>> diff --git a/monitor.c b/monitor.c
>> index 8cee35d..145895d 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -56,6 +56,7 @@
>> #include "json-parser.h"
>> #include "osdep.h"
>> #include "exec-all.h"
>> +#include "virtagent.h"
>> #ifdef CONFIG_SIMPLE_TRACE
>> #include "trace.h"
>> #endif
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 793cf1c..efa2137 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -738,6 +738,39 @@ Example:
>> EQMP
>>
>> {
>> + .name = "agent_viewfile",
>> + .args_type = "filepath:s",
>> + .params = "filepath",
>> + .help = "Echo a file from the guest filesystem",
>> + .user_print = monitor_user_noop,
>> + .mhandler.cmd_async = do_agent_viewfile,
>> + .flags = MONITOR_CMD_ASYNC,
>> + },
>> +
>> +STEXI
>> +@item agent_viewfile @var{filepath}
>> +@findex agent_viewfile
>> +Echo the file identified by @var{filepath} on the guest filesystem
>> +ETEXI
>> +SQMP
>> +agent_viewfile
>> +--------
>> +
>> +Echo the file identified by @var{filepath} from the guest filesystem.
>> +
>> +Arguments:
>> +
>> +- "filepath": Full guest path of the desired file
>> +
>> +Example:
>> +
>> +-> { "execute": "agent_viewfile",
>> + "arguments": { "filepath": "/sys/kernel/kexec_loaded" } }
>> +<- { "return": { "contents": "0" } }
>> +
>> +EQMP
>> +
>> + {
>> .name = "qmp_capabilities",
>> .args_type = "",
>> .params = "",
>> diff --git a/virtagent.c b/virtagent.c
>> index 34d8545..4a4dc8a 100644
>> --- a/virtagent.c
>> +++ b/virtagent.c
>> @@ -139,3 +139,99 @@ out_free:
>> out:
>> return ret;
>> }
>> +
>> +/* QMP/HMP RPC client functions */
>> +
>> +void do_agent_viewfile_print(Monitor *mon, const QObject *data)
>> +{
>> + QDict *qdict;
>> + const char *contents = NULL;
>> + int i;
>> +
>> + qdict = qobject_to_qdict(data);
>> + if (!qdict_haskey(qdict, "contents")) {
>> + return;
>> + }
>> +
>> + contents = qdict_get_str(qdict, "contents");
>> + if (contents != NULL) {
>> + /* monitor_printf truncates so do it in chunks. also, file_contents
>> + * may not be null-termed at proper location so explicitly calc
>> + * last chunk sizes */
>> + for (i = 0; i< strlen(contents); i += 1024) {
>> + monitor_printf(mon, "%.1024s", contents + i);
>> + }
>> + }
>> + monitor_printf(mon, "\n");
>> +}
>> +
>> +static void do_agent_viewfile_cb(const char *resp_data,
>> + size_t resp_data_len,
>> + MonitorCompletion *mon_cb,
>> + void *mon_data)
>> +{
>> + xmlrpc_value *resp = NULL;
>> + char *file_contents = NULL;
>> + size_t file_size;
>> + int ret;
>> + xmlrpc_env env;
>> + QDict *qdict = qdict_new();
>> +
>> + if (resp_data == NULL) {
>> + LOG("error handling RPC request");
>> + goto out_no_resp;
>> + }
>> +
>> + xmlrpc_env_init(&env);
>> + resp = xmlrpc_parse_response(&env, resp_data, resp_data_len);
>> + if (va_rpc_has_error(&env)) {
>> + ret = -1;
>> + goto out_no_resp;
>> + }
>> +
>> + xmlrpc_parse_value(&env, resp, "6",&file_contents,&file_size);
>> + if (va_rpc_has_error(&env)) {
>> + ret = -1;
>> + goto out;
>
> I believe this suffers from the same architectural problem I mentioned
> in my comment to 07/21 - you don't restrict the file size, so it could
> blow up the QEMU process on the host trying to view the wrong file.
It's restricted on the guest side:
virtagent-server.c:va_getfile():
while ((ret = read(fd, buf, VA_FILEBUF_LEN)) > 0) {
file_contents = qemu_realloc(file_contents, count +
VA_FILEBUF_LEN);
memcpy(file_contents + count, buf, ret);
count += ret;
if (count > VA_GETFILE_MAX) {
xmlrpc_faultf(env, "max file size (%d bytes) exceeded",
VA_GETFILE_MAX);
goto EXIT_CLOSE_BAD;
}
}
There are additional limits at the transport layer well to deal with a
potentially malicious/buggy agent as well:
virtagent-common.c:va_http_read_handler():
} else if (s->content_len > VA_CONTENT_LEN_MAX) {
LOG("http content length too long");
goto out_bad;
}
And configurable limits enforced by the xmlrpc-c library on the host and
guest side, which I haven't been explicitly setting or keeping
consistent with the other various limits. I'll address this for the next
round.
>
> I really think it is a bad idea to put this kind of command into the
> monitor.
>
> Jes
>
^ permalink raw reply [flat|nested] 74+ messages in thread* [Qemu-devel] Re: [RFC][PATCH v5 08/21] virtagent: add agent_viewfile qmp/hmp command
2010-12-09 21:12 ` Michael Roth
@ 2010-12-10 6:43 ` Jes Sorensen
2010-12-10 17:09 ` Michael Roth
0 siblings, 1 reply; 74+ messages in thread
From: Jes Sorensen @ 2010-12-10 6:43 UTC (permalink / raw)
To: Michael Roth; +Cc: agl, stefanha, abeekhof, qemu-devel, aliguori, ryanh
On 12/09/10 22:12, Michael Roth wrote:
> On 12/07/2010 08:26 AM, Jes Sorensen wrote:
>> I believe this suffers from the same architectural problem I mentioned
>> in my comment to 07/21 - you don't restrict the file size, so it could
>> blow up the QEMU process on the host trying to view the wrong file.
>
> It's restricted on the guest side:
>
> virtagent-server.c:va_getfile():
>
> while ((ret = read(fd, buf, VA_FILEBUF_LEN)) > 0) {
> file_contents = qemu_realloc(file_contents, count +
> VA_FILEBUF_LEN);
> memcpy(file_contents + count, buf, ret);
> count += ret;
> if (count > VA_GETFILE_MAX) {
> xmlrpc_faultf(env, "max file size (%d bytes) exceeded",
> VA_GETFILE_MAX);
> goto EXIT_CLOSE_BAD;
> }
> }
You cannot rely on the guest controlling this. You really have to treat
any guest as hostile and keep control and security in the host,
otherwise a hacked guest could end up attacking the host by blowing up
the host's QEMU process.
Cheers,
Jes
^ permalink raw reply [flat|nested] 74+ messages in thread* [Qemu-devel] Re: [RFC][PATCH v5 08/21] virtagent: add agent_viewfile qmp/hmp command
2010-12-10 6:43 ` Jes Sorensen
@ 2010-12-10 17:09 ` Michael Roth
2010-12-13 8:29 ` Jes Sorensen
0 siblings, 1 reply; 74+ messages in thread
From: Michael Roth @ 2010-12-10 17:09 UTC (permalink / raw)
To: Jes Sorensen; +Cc: agl, stefanha, abeekhof, qemu-devel, aliguori, ryanh
On 12/10/2010 12:43 AM, Jes Sorensen wrote:
> On 12/09/10 22:12, Michael Roth wrote:
>> On 12/07/2010 08:26 AM, Jes Sorensen wrote:
>>> I believe this suffers from the same architectural problem I mentioned
>>> in my comment to 07/21 - you don't restrict the file size, so it could
>>> blow up the QEMU process on the host trying to view the wrong file.
>>
>> It's restricted on the guest side:
>>
>> virtagent-server.c:va_getfile():
>>
>> while ((ret = read(fd, buf, VA_FILEBUF_LEN))> 0) {
>> file_contents = qemu_realloc(file_contents, count +
>> VA_FILEBUF_LEN);
>> memcpy(file_contents + count, buf, ret);
>> count += ret;
>> if (count> VA_GETFILE_MAX) {
>> xmlrpc_faultf(env, "max file size (%d bytes) exceeded",
>> VA_GETFILE_MAX);
>> goto EXIT_CLOSE_BAD;
>> }
>> }
>
> You cannot rely on the guest controlling this. You really have to treat
> any guest as hostile and keep control and security in the host,
> otherwise a hacked guest could end up attacking the host by blowing up
> the host's QEMU process.
Definetely agree on this, I mentioned some other checks here at the
transport and host xmlrpc level that would also limit this possibility:
> There are additional limits at the transport layer well to deal with a
> potentially malicious/buggy agent as well:
>
> virtagent-common.c:va_http_read_handler():
>
> } else if (s->content_len > VA_CONTENT_LEN_MAX) {
> LOG("http content length too long");
> goto out_bad;
> }
I think with strictly enforced size limits the major liability for
viewfile is, as you mentioned, users using it to view binary data or
carefully crafted files that can mess up or fool users/shells/programs
interpreting monitor output.
But plain-text does not include escape sequences, so it's completely
reasonable that we'd scrape them. And I'm not sure if a "(qemu)" in the
text is a potential liability. Would there be any other issues to consider?
If we can guard against those things, do you agree it wouldn't be an
inherently dangerous interface? State-full, asynchronous RPCs like
copyfile and exec are not really something I'd planned for the initial
release. I think they'll take some time to get right, and a simple
low-risk interface to cover what I'm fairly sure is the most common use
case seems reasonable.
>
> Cheers,
> Jes
^ permalink raw reply [flat|nested] 74+ messages in thread* [Qemu-devel] Re: [RFC][PATCH v5 08/21] virtagent: add agent_viewfile qmp/hmp command
2010-12-10 17:09 ` Michael Roth
@ 2010-12-13 8:29 ` Jes Sorensen
0 siblings, 0 replies; 74+ messages in thread
From: Jes Sorensen @ 2010-12-13 8:29 UTC (permalink / raw)
To: Michael Roth; +Cc: agl, stefanha, abeekhof, qemu-devel, aliguori, ryanh
On 12/10/10 18:09, Michael Roth wrote:
> I think with strictly enforced size limits the major liability for
> viewfile is, as you mentioned, users using it to view binary data or
> carefully crafted files that can mess up or fool users/shells/programs
> interpreting monitor output.
>
> But plain-text does not include escape sequences, so it's completely
> reasonable that we'd scrape them. And I'm not sure if a "(qemu)" in the
> text is a potential liability. Would there be any other issues to consider?
>
> If we can guard against those things, do you agree it wouldn't be an
> inherently dangerous interface? State-full, asynchronous RPCs like
> copyfile and exec are not really something I'd planned for the initial
> release. I think they'll take some time to get right, and a simple
> low-risk interface to cover what I'm fairly sure is the most common use
> case seems reasonable.
I am still wary of relying on strict limit enforcement. It is the sort
of thing that will eventually change without us noticing and we end up
with a security hole.
IMHO QEMU should not try to do these sorts of things, instead it should
provide the transport and control services. I don't think file viewing
belongs in QEMU at all. I would be a lot more comfortable if this was
implemented as a standalone monitor interface that connected to QEMU's
QMP interface. I could then use QMP to perform actions like copying the
file to /tmp and if viewing the file caused the monitor to lock up, we
wouldn't lose the guest. This could indeed be the start of an external
monitor :)
Cheers,
Jes
^ permalink raw reply [flat|nested] 74+ messages in thread
* [Qemu-devel] [RFC][PATCH v5 09/21] virtagent: add va.getdmesg RPC
2010-12-03 18:03 [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent Michael Roth
` (7 preceding siblings ...)
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 08/21] virtagent: add agent_viewfile qmp/hmp command Michael Roth
@ 2010-12-03 18:03 ` Michael Roth
2010-12-06 22:25 ` [Qemu-devel] " Adam Litke
2010-12-07 14:37 ` Jes Sorensen
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 10/21] virtagent: add agent_viewdmesg qmp/hmp commands Michael Roth
` (13 subsequent siblings)
22 siblings, 2 replies; 74+ messages in thread
From: Michael Roth @ 2010-12-03 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: agl, stefanha, Jes.Sorensen, mdroth, aliguori, ryanh, abeekhof
Add RPC to view guest dmesg output.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
virtagent-server.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 46 insertions(+), 0 deletions(-)
diff --git a/virtagent-server.c b/virtagent-server.c
index a430b58..aac8f70 100644
--- a/virtagent-server.c
+++ b/virtagent-server.c
@@ -83,6 +83,50 @@ EXIT_CLOSE_BAD:
return result;
}
+/* va_getdmesg(): return dmesg output
+ * rpc return values:
+ * - dmesg output as a string
+ */
+static xmlrpc_value *va_getdmesg(xmlrpc_env *env,
+ xmlrpc_value *param,
+ void *user_data)
+{
+ char *dmesg_buf = NULL, cmd[256];
+ int ret;
+ xmlrpc_value *result = NULL;
+ FILE *pipe;
+
+ SLOG("va_getdmesg()");
+
+ dmesg_buf = qemu_mallocz(VA_DMESG_LEN + 2048);
+ sprintf(cmd, "dmesg -s %d", VA_DMESG_LEN);
+
+ pipe = popen(cmd, "r");
+ if (pipe == NULL) {
+ LOG("popen failed: %s", strerror(errno));
+ xmlrpc_faultf(env, "popen failed: %s", strerror(errno));
+ goto EXIT_NOCLOSE;
+ }
+
+ ret = fread(dmesg_buf, sizeof(char), VA_DMESG_LEN, pipe);
+ if (!ferror(pipe)) {
+ dmesg_buf[ret] = '\0';
+ TRACE("dmesg:\n%s", dmesg_buf);
+ result = xmlrpc_build_value(env, "s", dmesg_buf);
+ } else {
+ LOG("fread failed");
+ xmlrpc_faultf(env, "popen failed: %s", strerror(errno));
+ }
+
+ pclose(pipe);
+EXIT_NOCLOSE:
+ if (dmesg_buf) {
+ qemu_free(dmesg_buf);
+ }
+
+ return result;
+}
+
typedef struct RPCFunction {
xmlrpc_value *(*func)(xmlrpc_env *env, xmlrpc_value *param, void *unused);
const char *func_name;
@@ -91,6 +135,8 @@ typedef struct RPCFunction {
static RPCFunction guest_functions[] = {
{ .func = va_getfile,
.func_name = "va.getfile" },
+ { .func = va_getdmesg,
+ .func_name = "va.getdmesg" },
{ NULL, NULL }
};
static RPCFunction host_functions[] = {
--
1.7.0.4
^ permalink raw reply related [flat|nested] 74+ messages in thread* [Qemu-devel] Re: [RFC][PATCH v5 09/21] virtagent: add va.getdmesg RPC
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 09/21] virtagent: add va.getdmesg RPC Michael Roth
@ 2010-12-06 22:25 ` Adam Litke
2010-12-07 14:37 ` Jes Sorensen
1 sibling, 0 replies; 74+ messages in thread
From: Adam Litke @ 2010-12-06 22:25 UTC (permalink / raw)
To: Michael Roth
Cc: agl, stefanha, Jes.Sorensen, qemu-devel, aliguori, ryanh, abeekhof
On Fri, 2010-12-03 at 12:03 -0600, Michael Roth wrote:
> +/* va_getdmesg(): return dmesg output
> + * rpc return values:
> + * - dmesg output as a string
> + */
> +static xmlrpc_value *va_getdmesg(xmlrpc_env *env,
> + xmlrpc_value *param,
> + void *user_data)
> +{
> + char *dmesg_buf = NULL, cmd[256];
> + int ret;
> + xmlrpc_value *result = NULL;
> + FILE *pipe;
> +
> + SLOG("va_getdmesg()");
> +
> + dmesg_buf = qemu_mallocz(VA_DMESG_LEN + 2048);
> + sprintf(cmd, "dmesg -s %d", VA_DMESG_LEN);
> +
> + pipe = popen(cmd, "r");
> + if (pipe == NULL) {
> + LOG("popen failed: %s", strerror(errno));
> + xmlrpc_faultf(env, "popen failed: %s", strerror(errno));
> + goto EXIT_NOCLOSE;
> + }
> +
> + ret = fread(dmesg_buf, sizeof(char), VA_DMESG_LEN, pipe);
> + if (!ferror(pipe)) {
> + dmesg_buf[ret] = '\0';
> + TRACE("dmesg:\n%s", dmesg_buf);
> + result = xmlrpc_build_value(env, "s", dmesg_buf);
> + } else {
> + LOG("fread failed");
> + xmlrpc_faultf(env, "popen failed: %s", strerror(errno));
> + }
> +
> + pclose(pipe);
> +EXIT_NOCLOSE:
I think goto labels are supposed to be lower-case.
--
Thanks,
Adam
^ permalink raw reply [flat|nested] 74+ messages in thread* [Qemu-devel] Re: [RFC][PATCH v5 09/21] virtagent: add va.getdmesg RPC
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 09/21] virtagent: add va.getdmesg RPC Michael Roth
2010-12-06 22:25 ` [Qemu-devel] " Adam Litke
@ 2010-12-07 14:37 ` Jes Sorensen
2010-12-07 17:32 ` Michael Roth
1 sibling, 1 reply; 74+ messages in thread
From: Jes Sorensen @ 2010-12-07 14:37 UTC (permalink / raw)
To: Michael Roth; +Cc: agl, stefanha, abeekhof, qemu-devel, aliguori, ryanh
On 12/03/10 19:03, Michael Roth wrote:
> Add RPC to view guest dmesg output.
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> virtagent-server.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 46 insertions(+), 0 deletions(-)
>
> diff --git a/virtagent-server.c b/virtagent-server.c
> index a430b58..aac8f70 100644
> --- a/virtagent-server.c
> +++ b/virtagent-server.c
> @@ -83,6 +83,50 @@ EXIT_CLOSE_BAD:
> return result;
> }
>
> +/* va_getdmesg(): return dmesg output
> + * rpc return values:
> + * - dmesg output as a string
> + */
> +static xmlrpc_value *va_getdmesg(xmlrpc_env *env,
> + xmlrpc_value *param,
> + void *user_data)
> +{
> + char *dmesg_buf = NULL, cmd[256];
> + int ret;
> + xmlrpc_value *result = NULL;
> + FILE *pipe;
> +
> + SLOG("va_getdmesg()");
> +
> + dmesg_buf = qemu_mallocz(VA_DMESG_LEN + 2048);
> + sprintf(cmd, "dmesg -s %d", VA_DMESG_LEN);
What happens if the guest's dmesg buffer is larger than your hardcoded
value?
Jes
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Qemu-devel] Re: [RFC][PATCH v5 09/21] virtagent: add va.getdmesg RPC
2010-12-07 14:37 ` Jes Sorensen
@ 2010-12-07 17:32 ` Michael Roth
2010-12-08 19:22 ` Jes Sorensen
0 siblings, 1 reply; 74+ messages in thread
From: Michael Roth @ 2010-12-07 17:32 UTC (permalink / raw)
To: Jes Sorensen; +Cc: agl, stefanha, abeekhof, qemu-devel, aliguori, ryanh
On 12/07/2010 08:37 AM, Jes Sorensen wrote:
> On 12/03/10 19:03, Michael Roth wrote:
>> Add RPC to view guest dmesg output.
>>
>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>> ---
>> virtagent-server.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 46 insertions(+), 0 deletions(-)
>>
>> diff --git a/virtagent-server.c b/virtagent-server.c
>> index a430b58..aac8f70 100644
>> --- a/virtagent-server.c
>> +++ b/virtagent-server.c
>> @@ -83,6 +83,50 @@ EXIT_CLOSE_BAD:
>> return result;
>> }
>>
>> +/* va_getdmesg(): return dmesg output
>> + * rpc return values:
>> + * - dmesg output as a string
>> + */
>> +static xmlrpc_value *va_getdmesg(xmlrpc_env *env,
>> + xmlrpc_value *param,
>> + void *user_data)
>> +{
>> + char *dmesg_buf = NULL, cmd[256];
>> + int ret;
>> + xmlrpc_value *result = NULL;
>> + FILE *pipe;
>> +
>> + SLOG("va_getdmesg()");
>> +
>> + dmesg_buf = qemu_mallocz(VA_DMESG_LEN + 2048);
>> + sprintf(cmd, "dmesg -s %d", VA_DMESG_LEN);
>
> What happens if the guest's dmesg buffer is larger than your hardcoded
> value?
It'll end up getting truncated by the fread() later:
ret = fread(dmesg_buf, sizeof(char), VA_DMESG_LEN, pipe);
That's where the dmesg -s VA_DMESG_LEN comes into play, it should size
things such that we can buffer up till the end of the dmesg output.
This param is kind of quirky though, size doesn't seem to have an affect
for anything below 4KB, but if we stick with VA_DMESG_LEN >= 4KB this
should cover us, unless it's a distro-specific. But it should blow
anything up, at least.
>
> Jes
>
>
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Qemu-devel] Re: [RFC][PATCH v5 09/21] virtagent: add va.getdmesg RPC
2010-12-07 17:32 ` Michael Roth
@ 2010-12-08 19:22 ` Jes Sorensen
2010-12-09 21:15 ` Michael Roth
0 siblings, 1 reply; 74+ messages in thread
From: Jes Sorensen @ 2010-12-08 19:22 UTC (permalink / raw)
To: Michael Roth; +Cc: agl, stefanha, abeekhof, qemu-devel, aliguori, ryanh
On 12/07/10 18:32, Michael Roth wrote:
> On 12/07/2010 08:37 AM, Jes Sorensen wrote:
>> On 12/03/10 19:03, Michael Roth wrote:
>>> +static xmlrpc_value *va_getdmesg(xmlrpc_env *env,
>>> + xmlrpc_value *param,
>>> + void *user_data)
>>> +{
>>> + char *dmesg_buf = NULL, cmd[256];
>>> + int ret;
>>> + xmlrpc_value *result = NULL;
>>> + FILE *pipe;
>>> +
>>> + SLOG("va_getdmesg()");
>>> +
>>> + dmesg_buf = qemu_mallocz(VA_DMESG_LEN + 2048);
>>> + sprintf(cmd, "dmesg -s %d", VA_DMESG_LEN);
>>
>> What happens if the guest's dmesg buffer is larger than your hardcoded
>> value?
>
> It'll end up getting truncated by the fread() later:
>
> ret = fread(dmesg_buf, sizeof(char), VA_DMESG_LEN, pipe);
>
> That's where the dmesg -s VA_DMESG_LEN comes into play, it should size
> things such that we can buffer up till the end of the dmesg output.
>
> This param is kind of quirky though, size doesn't seem to have an affect
> for anything below 4KB, but if we stick with VA_DMESG_LEN >= 4KB this
> should cover us, unless it's a distro-specific. But it should blow
> anything up, at least.
I am wary of these hard coded constants. Isn't there a way to set the
kernel's dmesg buffer size, or is that only a compile time option?
Cheers,
Jes
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Qemu-devel] Re: [RFC][PATCH v5 09/21] virtagent: add va.getdmesg RPC
2010-12-08 19:22 ` Jes Sorensen
@ 2010-12-09 21:15 ` Michael Roth
2010-12-10 6:46 ` Jes Sorensen
0 siblings, 1 reply; 74+ messages in thread
From: Michael Roth @ 2010-12-09 21:15 UTC (permalink / raw)
To: Jes Sorensen; +Cc: agl, stefanha, abeekhof, qemu-devel, aliguori, ryanh
On 12/08/2010 01:22 PM, Jes Sorensen wrote:
> On 12/07/10 18:32, Michael Roth wrote:
>> On 12/07/2010 08:37 AM, Jes Sorensen wrote:
>>> On 12/03/10 19:03, Michael Roth wrote:
>>>> +static xmlrpc_value *va_getdmesg(xmlrpc_env *env,
>>>> + xmlrpc_value *param,
>>>> + void *user_data)
>>>> +{
>>>> + char *dmesg_buf = NULL, cmd[256];
>>>> + int ret;
>>>> + xmlrpc_value *result = NULL;
>>>> + FILE *pipe;
>>>> +
>>>> + SLOG("va_getdmesg()");
>>>> +
>>>> + dmesg_buf = qemu_mallocz(VA_DMESG_LEN + 2048);
>>>> + sprintf(cmd, "dmesg -s %d", VA_DMESG_LEN);
>>>
>>> What happens if the guest's dmesg buffer is larger than your hardcoded
>>> value?
>>
>> It'll end up getting truncated by the fread() later:
>>
>> ret = fread(dmesg_buf, sizeof(char), VA_DMESG_LEN, pipe);
>>
>> That's where the dmesg -s VA_DMESG_LEN comes into play, it should size
>> things such that we can buffer up till the end of the dmesg output.
>>
>> This param is kind of quirky though, size doesn't seem to have an affect
>> for anything below 4KB, but if we stick with VA_DMESG_LEN>= 4KB this
>> should cover us, unless it's a distro-specific. But it should blow
>> anything up, at least.
>
> I am wary of these hard coded constants. Isn't there a way to set the
> kernel's dmesg buffer size, or is that only a compile time option?
>
From what I can tell it's a compile-time option. I originally had
dmesg_len as a param the host could pass to the guest, but it has no
effect if the buffer is smaller, which might cause unnecessary confusion.
> Cheers,
> Jes
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Qemu-devel] Re: [RFC][PATCH v5 09/21] virtagent: add va.getdmesg RPC
2010-12-09 21:15 ` Michael Roth
@ 2010-12-10 6:46 ` Jes Sorensen
0 siblings, 0 replies; 74+ messages in thread
From: Jes Sorensen @ 2010-12-10 6:46 UTC (permalink / raw)
To: Michael Roth; +Cc: agl, stefanha, abeekhof, qemu-devel, aliguori, ryanh
On 12/09/10 22:15, Michael Roth wrote:
> On 12/08/2010 01:22 PM, Jes Sorensen wrote:
>>> This param is kind of quirky though, size doesn't seem to have an affect
>>> for anything below 4KB, but if we stick with VA_DMESG_LEN>= 4KB this
>>> should cover us, unless it's a distro-specific. But it should blow
>>> anything up, at least.
>>
>> I am wary of these hard coded constants. Isn't there a way to set the
>> kernel's dmesg buffer size, or is that only a compile time option?
>>
>
> From what I can tell it's a compile-time option. I originally had
> dmesg_len as a param the host could pass to the guest, but it has no
> effect if the buffer is smaller, which might cause unnecessary confusion.
You are correct! I checked, there were people talking about a
configurable option but so far it is not in place.
I would still rather have this go via a file transfer to the host, and
put the output in a tmpfile.
I am still against adding viewfile commands to the monitor though. You
get a bad control character in the string and your console is messed up.
Hit CTRL-c and you lost your guest.
Cheers,
Jes
^ permalink raw reply [flat|nested] 74+ messages in thread
* [Qemu-devel] [RFC][PATCH v5 10/21] virtagent: add agent_viewdmesg qmp/hmp commands
2010-12-03 18:03 [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent Michael Roth
` (8 preceding siblings ...)
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 09/21] virtagent: add va.getdmesg RPC Michael Roth
@ 2010-12-03 18:03 ` Michael Roth
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 11/21] virtagent: add va.shutdown RPC Michael Roth
` (12 subsequent siblings)
22 siblings, 0 replies; 74+ messages in thread
From: Michael Roth @ 2010-12-03 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: agl, stefanha, Jes.Sorensen, mdroth, aliguori, ryanh, abeekhof
Add commands to view guest dmesg output. Currently it is a 16K buffer.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
hmp-commands.hx | 16 +++++++++
qmp-commands.hx | 35 +++++++++++++++++++++
virtagent.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
virtagent.h | 3 ++
4 files changed, 146 insertions(+), 0 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 423c752..5b9db62 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1228,6 +1228,22 @@ STEXI
Echo the file identified by @var{filepath} on the guest filesystem
ETEXI
+ {
+ .name = "agent_viewdmesg",
+ .args_type = "",
+ .params = "",
+ .help = "View guest dmesg output",
+ .user_print = do_agent_viewdmesg_print,
+ .mhandler.cmd_async = do_agent_viewdmesg,
+ .flags = MONITOR_CMD_ASYNC,
+ },
+
+STEXI
+@item agent_viewdmesg
+@findex agent_viewdmesg
+View guest dmesg output
+ETEXI
+
STEXI
@end table
ETEXI
diff --git a/qmp-commands.hx b/qmp-commands.hx
index efa2137..dc319b7 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -771,6 +771,41 @@ Example:
EQMP
{
+ .name = "agent_viewdmesg",
+ .args_type = "",
+ .params = "",
+ .help = "View guest dmesg output",
+ .user_print = do_agent_viewdmesg_print,
+ .mhandler.cmd_async = do_agent_viewdmesg,
+ .flags = MONITOR_CMD_ASYNC,
+ },
+
+STEXI
+@item agent_viewdmesg
+@findex agent_viewdmesg
+View guest dmesg output
+ETEXI
+SQMP
+agent_viewdmesg
+--------
+
+View guest dmesg output
+
+Arguments:
+
+(none)
+
+Example:
+
+-> { "execute": "agent_viewdmesg" }
+<- { "return": {
+ "contents": "[353487.942215] usb 1-4: USB disconnect, address 9\n..."
+ }
+ }
+
+EQMP
+
+ {
.name = "qmp_capabilities",
.args_type = "",
.params = "",
diff --git a/virtagent.c b/virtagent.c
index 4a4dc8a..bacd95e 100644
--- a/virtagent.c
+++ b/virtagent.c
@@ -235,3 +235,95 @@ int do_agent_viewfile(Monitor *mon, const QDict *mon_params,
xmlrpc_DECREF(params);
return ret;
}
+
+void do_agent_viewdmesg_print(Monitor *mon, const QObject *data)
+{
+ QDict *qdict;
+ const char *contents = NULL;
+ int i;
+
+ qdict = qobject_to_qdict(data);
+ if (!qdict_haskey(qdict, "contents")) {
+ goto out;
+ }
+
+ contents = qdict_get_str(qdict, "contents");
+ if (contents != NULL) {
+ /* monitor_printf truncates so do it in chunks. also, file_contents
+ * may not be null-termed at proper location so explicitly calc
+ * last chunk sizes */
+ for (i = 0; i < strlen(contents); i += 1024) {
+ monitor_printf(mon, "%.1024s", contents + i);
+ }
+ }
+
+out:
+ monitor_printf(mon, "\n");
+}
+
+static void do_agent_viewdmesg_cb(const char *resp_data,
+ size_t resp_data_len,
+ MonitorCompletion *mon_cb,
+ void *mon_data)
+{
+ xmlrpc_value *resp = NULL;
+ char *dmesg = NULL;
+ int ret;
+ xmlrpc_env env;
+ QDict *qdict = qdict_new();
+
+ if (resp_data == NULL) {
+ LOG("error handling RPC request");
+ goto out_no_resp;
+ }
+
+ xmlrpc_env_init(&env);
+ resp = xmlrpc_parse_response(&env, resp_data, resp_data_len);
+ if (va_rpc_has_error(&env)) {
+ ret = -1;
+ goto out_no_resp;
+ }
+
+ xmlrpc_parse_value(&env, resp, "s", &dmesg);
+ if (va_rpc_has_error(&env)) {
+ ret = -1;
+ goto out;
+ }
+
+ if (dmesg != NULL) {
+ qdict_put(qdict, "contents", qstring_from_str(dmesg));
+ }
+
+out:
+ xmlrpc_DECREF(resp);
+out_no_resp:
+ if (mon_cb) {
+ mon_cb(mon_data, QOBJECT(qdict));
+ }
+}
+
+/*
+ * do_agent_viewdmesg(): View guest dmesg output
+ */
+int do_agent_viewdmesg(Monitor *mon, const QDict *mon_params,
+ MonitorCompletion cb, void *opaque)
+{
+ xmlrpc_env env;
+ xmlrpc_value *params;
+ int ret;
+
+ xmlrpc_env_init(&env);
+
+ params = xmlrpc_build_value(&env, "()");
+ if (va_rpc_has_error(&env)) {
+ return -1;
+ }
+
+ ret = va_do_rpc(&env, "va.getdmesg", params, do_agent_viewdmesg_cb, cb,
+ opaque);
+ if (ret) {
+ qerror_report(QERR_VA_FAILED, ret, strerror(ret));
+ }
+ xmlrpc_DECREF(params);
+ return ret;
+}
diff --git a/virtagent.h b/virtagent.h
index 5960616..7f9899f 100644
--- a/virtagent.h
+++ b/virtagent.h
@@ -31,5 +31,8 @@ int va_client_init(VAClientData *client_data);
void do_agent_viewfile_print(Monitor *mon, const QObject *qobject);
int do_agent_viewfile(Monitor *mon, const QDict *mon_params,
MonitorCompletion cb, void *opaque);
+void do_agent_viewdmesg_print(Monitor *mon, const QObject *qobject);
+int do_agent_viewdmesg(Monitor *mon, const QDict *mon_params,
+ MonitorCompletion cb, void *opaque);
#endif /* VIRTAGENT_H */
--
1.7.0.4
^ permalink raw reply related [flat|nested] 74+ messages in thread* [Qemu-devel] [RFC][PATCH v5 11/21] virtagent: add va.shutdown RPC
2010-12-03 18:03 [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent Michael Roth
` (9 preceding siblings ...)
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 10/21] virtagent: add agent_viewdmesg qmp/hmp commands Michael Roth
@ 2010-12-03 18:03 ` Michael Roth
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 12/21] virtagent: add agent_shutdown qmp/hmp commands Michael Roth
` (11 subsequent siblings)
22 siblings, 0 replies; 74+ messages in thread
From: Michael Roth @ 2010-12-03 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: agl, stefanha, Jes.Sorensen, mdroth, aliguori, ryanh, abeekhof
RPC to initiate guest reboot/halt/powerdown
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
virtagent-server.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 58 insertions(+), 0 deletions(-)
diff --git a/virtagent-server.c b/virtagent-server.c
index aac8f70..453cf0d 100644
--- a/virtagent-server.c
+++ b/virtagent-server.c
@@ -127,6 +127,62 @@ EXIT_NOCLOSE:
return result;
}
+/* va_shutdown(): initiate guest shutdown
+ * rpc return values: none
+ */
+static xmlrpc_value *va_shutdown(xmlrpc_env *env,
+ xmlrpc_value *param,
+ void *user_data)
+{
+ int ret;
+ const char *shutdown_type, *shutdown_flag;
+ xmlrpc_value *result = xmlrpc_build_value(env, "s", "dummy");
+
+ TRACE("called");
+ xmlrpc_decompose_value(env, param, "(s)", &shutdown_type);
+ if (env->fault_occurred) {
+ goto out_bad;
+ }
+
+ if (strcmp(shutdown_type, "halt") == 0) {
+ shutdown_flag = "-H";
+ } else if (strcmp(shutdown_type, "powerdown") == 0) {
+ shutdown_flag = "-P";
+ } else if (strcmp(shutdown_type, "reboot") == 0) {
+ shutdown_flag = "-r";
+ } else {
+ xmlrpc_faultf(env, "invalid shutdown type: %s", shutdown_type);
+ goto out_bad;
+ }
+
+ SLOG("va_shutdown(), shutdown_type:%s", shutdown_type);
+
+ ret = fork();
+ if (ret == 0) {
+ /* child, start the shutdown */
+ setsid();
+ fclose(stdin);
+ fclose(stdout);
+ fclose(stderr);
+
+ sleep(5);
+ ret = execl("/sbin/shutdown", "shutdown", shutdown_flag, "+0",
+ "hypervisor initiated shutdown", (char*)NULL);
+ if (ret < 0) {
+ LOG("execl() failed: %s", strerror(errno));
+ exit(1);
+ }
+ TRACE("shouldn't be here");
+ exit(0);
+ } else if (ret < 0) {
+ xmlrpc_faultf(env, "fork() failed: %s", strerror(errno));
+ }
+
+ return result;
+out_bad:
+ return NULL;
+}
+
typedef struct RPCFunction {
xmlrpc_value *(*func)(xmlrpc_env *env, xmlrpc_value *param, void *unused);
const char *func_name;
@@ -137,6 +193,8 @@ static RPCFunction guest_functions[] = {
.func_name = "va.getfile" },
{ .func = va_getdmesg,
.func_name = "va.getdmesg" },
+ { .func = va_shutdown,
+ .func_name = "va.shutdown" },
{ NULL, NULL }
};
static RPCFunction host_functions[] = {
--
1.7.0.4
^ permalink raw reply related [flat|nested] 74+ messages in thread* [Qemu-devel] [RFC][PATCH v5 12/21] virtagent: add agent_shutdown qmp/hmp commands
2010-12-03 18:03 [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent Michael Roth
` (10 preceding siblings ...)
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 11/21] virtagent: add va.shutdown RPC Michael Roth
@ 2010-12-03 18:03 ` Michael Roth
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 13/21] virtagent: add va.ping RPC Michael Roth
` (10 subsequent siblings)
22 siblings, 0 replies; 74+ messages in thread
From: Michael Roth @ 2010-12-03 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: agl, stefanha, Jes.Sorensen, mdroth, aliguori, ryanh, abeekhof
Provide monitor command to initiate guest reboot/halt/powerdown
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
hmp-commands.hx | 16 ++++++++++++++
qmp-commands.hx | 32 +++++++++++++++++++++++++++++
virtagent.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
virtagent.h | 2 +
4 files changed, 109 insertions(+), 0 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 5b9db62..3250e41 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1244,6 +1244,22 @@ STEXI
View guest dmesg output
ETEXI
+ {
+ .name = "agent_shutdown",
+ .args_type = "shutdown_type:s",
+ .params = "[reboot|shutdown|poweroff]",
+ .help = "Shutdown/reboot a guest locally",
+ .user_print = monitor_user_noop,
+ .mhandler.cmd_async = do_agent_shutdown,
+ .flags = MONITOR_CMD_ASYNC,
+ },
+
+STEXI
+@item agent_shutdown
+@findex agent_shutdown
+Shutdown/reboot a guest locally
+ETEXI
+
STEXI
@end table
ETEXI
diff --git a/qmp-commands.hx b/qmp-commands.hx
index dc319b7..0f983cc 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -806,6 +806,38 @@ Example:
EQMP
{
+ .name = "agent_shutdown",
+ .args_type = "shutdown_type:s",
+ .params = "[reboot|shutdown|poweroff]",
+ .help = "Shutdown/reboot the guest locally",
+ .user_print = monitor_user_noop,
+ .mhandler.cmd_async = do_agent_shutdown,
+ .flags = MONITOR_CMD_ASYNC,
+ },
+
+STEXI
+@item agent_shutdown
+@findex agent_shutdown
+Shutdown/reboot the guest locally
+ETEXI
+SQMP
+agent_shutdown
+--------
+
+Shutdown the guest locally
+
+Arguments:
+
+(none)
+
+Example:
+
+-> { "execute": "agent_shutdown" }
+<- { "return": {} }
+
+EQMP
+
+ {
.name = "qmp_capabilities",
.args_type = "",
.params = "",
diff --git a/virtagent.c b/virtagent.c
index bacd95e..1f66787 100644
--- a/virtagent.c
+++ b/virtagent.c
@@ -327,3 +327,62 @@ int do_agent_viewdmesg(Monitor *mon, const QDict *mon_params,
xmlrpc_DECREF(params);
return ret;
}
+
+static void do_agent_shutdown_cb(const char *resp_data,
+ size_t resp_data_len,
+ MonitorCompletion *mon_cb,
+ void *mon_data)
+{
+ xmlrpc_value *resp = NULL;
+ xmlrpc_env env;
+
+ TRACE("called");
+
+ if (resp_data == NULL) {
+ LOG("error handling RPC request");
+ goto out_no_resp;
+ }
+
+ xmlrpc_env_init(&env);
+ resp = xmlrpc_parse_response(&env, resp_data, resp_data_len);
+ if (va_rpc_has_error(&env)) {
+ LOG("RPC Failed (%i): %s\n", env.fault_code,
+ env.fault_string);
+ goto out_no_resp;
+ }
+
+ xmlrpc_DECREF(resp);
+out_no_resp:
+ if (mon_cb) {
+ mon_cb(mon_data, NULL);
+ }
+}
+
+/*
+ * do_agent_shutdown(): Shutdown a guest
+ */
+int do_agent_shutdown(Monitor *mon, const QDict *mon_params,
+ MonitorCompletion cb, void *opaque)
+{
+ xmlrpc_env env;
+ xmlrpc_value *params;
+ const char *shutdown_type;
+ int ret;
+
+ TRACE("called");
+
+ xmlrpc_env_init(&env);
+ shutdown_type = qdict_get_str(mon_params, "shutdown_type");
+ params = xmlrpc_build_value(&env, "(s)", shutdown_type);
+ if (va_rpc_has_error(&env)) {
+ return -1;
+ }
+
+ ret = va_do_rpc(&env, "va.shutdown", params, do_agent_shutdown_cb, cb,
+ opaque);
+ if (ret) {
+ qerror_report(QERR_VA_FAILED, ret, strerror(ret));
+ }
+ xmlrpc_DECREF(params);
+ return ret;
+}
diff --git a/virtagent.h b/virtagent.h
index 7f9899f..60af1d8 100644
--- a/virtagent.h
+++ b/virtagent.h
@@ -34,5 +34,7 @@ int do_agent_viewfile(Monitor *mon, const QDict *mon_params,
void do_agent_viewdmesg_print(Monitor *mon, const QObject *qobject);
int do_agent_viewdmesg(Monitor *mon, const QDict *mon_params,
MonitorCompletion cb, void *opaque);
+int do_agent_shutdown(Monitor *mon, const QDict *mon_params,
+ MonitorCompletion cb, void *opaque);
#endif /* VIRTAGENT_H */
--
1.7.0.4
^ permalink raw reply related [flat|nested] 74+ messages in thread* [Qemu-devel] [RFC][PATCH v5 13/21] virtagent: add va.ping RPC
2010-12-03 18:03 [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent Michael Roth
` (11 preceding siblings ...)
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 12/21] virtagent: add agent_shutdown qmp/hmp commands Michael Roth
@ 2010-12-03 18:03 ` Michael Roth
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 14/21] virtagent: add agent_ping qmp/hmp commands Michael Roth
` (9 subsequent siblings)
22 siblings, 0 replies; 74+ messages in thread
From: Michael Roth @ 2010-12-03 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: agl, stefanha, Jes.Sorensen, mdroth, aliguori, ryanh, abeekhof
Do-nothing RPC that can be used to "ping" the RPC server
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
virtagent-server.c | 17 +++++++++++++++++
1 files changed, 17 insertions(+), 0 deletions(-)
diff --git a/virtagent-server.c b/virtagent-server.c
index 453cf0d..4a8ce7d 100644
--- a/virtagent-server.c
+++ b/virtagent-server.c
@@ -183,6 +183,19 @@ out_bad:
return NULL;
}
+/* va_ping(): respond to client. response without error in env
+ * variable indicates successful response
+ * rpc return values: none
+ */
+static xmlrpc_value *va_ping(xmlrpc_env *env,
+ xmlrpc_value *param,
+ void *user_data)
+{
+ xmlrpc_value *result = xmlrpc_build_value(env, "s", "dummy");
+ SLOG("va_ping()");
+ return result;
+}
+
typedef struct RPCFunction {
xmlrpc_value *(*func)(xmlrpc_env *env, xmlrpc_value *param, void *unused);
const char *func_name;
@@ -195,9 +208,13 @@ static RPCFunction guest_functions[] = {
.func_name = "va.getdmesg" },
{ .func = va_shutdown,
.func_name = "va.shutdown" },
+ { .func = va_ping,
+ .func_name = "va.ping" },
{ NULL, NULL }
};
static RPCFunction host_functions[] = {
+ { .func = va_ping,
+ .func_name = "va.ping" },
{ NULL, NULL }
};
--
1.7.0.4
^ permalink raw reply related [flat|nested] 74+ messages in thread* [Qemu-devel] [RFC][PATCH v5 14/21] virtagent: add agent_ping qmp/hmp commands
2010-12-03 18:03 [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent Michael Roth
` (12 preceding siblings ...)
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 13/21] virtagent: add va.ping RPC Michael Roth
@ 2010-12-03 18:03 ` Michael Roth
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 15/21] virtagent: add agent_capabilities " Michael Roth
` (8 subsequent siblings)
22 siblings, 0 replies; 74+ messages in thread
From: Michael Roth @ 2010-12-03 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: agl, stefanha, Jes.Sorensen, mdroth, aliguori, ryanh, abeekhof
Monitor command to ping the RPC server.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
hmp-commands.hx | 16 ++++++++++++
qmp-commands.hx | 32 +++++++++++++++++++++++
virtagent.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
virtagent.h | 3 ++
4 files changed, 125 insertions(+), 0 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 3250e41..d3f642f 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1260,6 +1260,22 @@ STEXI
Shutdown/reboot a guest locally
ETEXI
+ {
+ .name = "agent_ping",
+ .args_type = "",
+ .params = "",
+ .help = "Ping a guest",
+ .user_print = do_agent_ping_print,
+ .mhandler.cmd_async = do_agent_ping,
+ .flags = MONITOR_CMD_ASYNC,
+ },
+
+STEXI
+@item agent_ping
+@findex agent_ping
+Ping a guest
+ETEXI
+
STEXI
@end table
ETEXI
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 0f983cc..1e798f5 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -838,6 +838,38 @@ Example:
EQMP
{
+ .name = "agent_ping",
+ .args_type = "",
+ .params = "",
+ .help = "Ping a guest",
+ .user_print = do_agent_ping_print,
+ .mhandler.cmd_async = do_agent_ping,
+ .flags = MONITOR_CMD_ASYNC,
+ },
+
+STEXI
+@item agent_ping
+@findex agent_ping
+Ping a guest
+ETEXI
+SQMP
+agent_ping
+--------
+
+Ping a guest
+
+Arguments:
+
+(none)
+
+Example:
+
+-> { "execute": "agent_ping" }
+<- { "return": { "response":"ok" } }
+
+EQMP
+
+ {
.name = "qmp_capabilities",
.args_type = "",
.params = "",
diff --git a/virtagent.c b/virtagent.c
index 1f66787..93329e3 100644
--- a/virtagent.c
+++ b/virtagent.c
@@ -386,3 +386,77 @@ int do_agent_shutdown(Monitor *mon, const QDict *mon_params,
xmlrpc_DECREF(params);
return ret;
}
+
+void do_agent_ping_print(Monitor *mon, const QObject *data)
+{
+ QDict *qdict;
+ const char *response;
+
+ TRACE("called");
+
+ qdict = qobject_to_qdict(data);
+ response = qdict_get_str(qdict, "response");
+ if (qdict_haskey(qdict, "response")) {
+ monitor_printf(mon, "%s", response);
+ }
+
+ monitor_printf(mon, "\n");
+}
+
+static void do_agent_ping_cb(const char *resp_data,
+ size_t resp_data_len,
+ MonitorCompletion *mon_cb,
+ void *mon_data)
+{
+ xmlrpc_value *resp = NULL;
+ xmlrpc_env env;
+ QDict *qdict = qdict_new();
+
+ TRACE("called");
+
+ if (resp_data == NULL) {
+ LOG("error handling RPC request");
+ qdict_put(qdict, "response", qstring_from_str("error"));
+ goto out_no_resp;
+ }
+
+ xmlrpc_env_init(&env);
+ resp = xmlrpc_parse_response(&env, resp_data, resp_data_len);
+ if (va_rpc_has_error(&env)) {
+ qdict_put(qdict, "response", qstring_from_str("error"));
+ goto out_no_resp;
+ }
+ qdict_put(qdict, "response", qstring_from_str("ok"));
+
+ xmlrpc_DECREF(resp);
+out_no_resp:
+ if (mon_cb) {
+ mon_cb(mon_data, QOBJECT(qdict));
+ }
+ qobject_decref(QOBJECT(qdict));
+}
+
+/*
+ * do_agent_ping(): Ping a guest
+ */
+int do_agent_ping(Monitor *mon, const QDict *mon_params,
+ MonitorCompletion cb, void *opaque)
+{
+ xmlrpc_env env;
+ xmlrpc_value *params;
+ int ret;
+
+ xmlrpc_env_init(&env);
+
+ params = xmlrpc_build_value(&env, "(n)");
+ if (va_rpc_has_error(&env)) {
+ return -1;
+ }
+
+ ret = va_do_rpc(&env, "va.ping", params, do_agent_ping_cb, cb, opaque);
+ if (ret) {
+ qerror_report(QERR_VA_FAILED, ret, strerror(ret));
+ }
+ xmlrpc_DECREF(params);
+ return ret;
+}
diff --git a/virtagent.h b/virtagent.h
index 60af1d8..f420c8b 100644
--- a/virtagent.h
+++ b/virtagent.h
@@ -36,5 +36,8 @@ int do_agent_viewdmesg(Monitor *mon, const QDict *mon_params,
MonitorCompletion cb, void *opaque);
int do_agent_shutdown(Monitor *mon, const QDict *mon_params,
MonitorCompletion cb, void *opaque);
+void do_agent_ping_print(Monitor *mon, const QObject *qobject);
+int do_agent_ping(Monitor *mon, const QDict *mon_params,
+ MonitorCompletion cb, void *opaque);
#endif /* VIRTAGENT_H */
--
1.7.0.4
^ permalink raw reply related [flat|nested] 74+ messages in thread* [Qemu-devel] [RFC][PATCH v5 15/21] virtagent: add agent_capabilities qmp/hmp commands
2010-12-03 18:03 [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent Michael Roth
` (13 preceding siblings ...)
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 14/21] virtagent: add agent_ping qmp/hmp commands Michael Roth
@ 2010-12-03 18:03 ` Michael Roth
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 16/21] virtagent: add client capabilities init function Michael Roth
` (7 subsequent siblings)
22 siblings, 0 replies; 74+ messages in thread
From: Michael Roth @ 2010-12-03 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: agl, stefanha, Jes.Sorensen, mdroth, aliguori, ryanh, abeekhof
Call guest agent's built-in introspection functions to get a list of
supported RPCs, and re-negotiate guest agent capabilities to determine
what agent_* commands are supported.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
hmp-commands.hx | 16 +++++++++
qmp-commands.hx | 32 ++++++++++++++++++
virtagent.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
virtagent.h | 3 ++
4 files changed, 149 insertions(+), 0 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index d3f642f..d68c1ba 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1276,6 +1276,22 @@ STEXI
Ping a guest
ETEXI
+ {
+ .name = "agent_capabilities",
+ .args_type = "",
+ .params = "",
+ .help = "Fetch and re-negotiate guest agent capabilities",
+ .user_print = do_agent_capabilities_print,
+ .mhandler.cmd_async = do_agent_capabilities,
+ .flags = MONITOR_CMD_ASYNC,
+ },
+
+STEXI
+@item agent_capabilities
+@findex agent_capabilities
+Fetch and re-negotiate guest agent capabilties
+ETEXI
+
STEXI
@end table
ETEXI
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 1e798f5..4ae0890 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -870,6 +870,38 @@ Example:
EQMP
{
+ .name = "agent_capabilities",
+ .args_type = "",
+ .params = "",
+ .help = "Fetch and re-negotiate guest agent capabilities",
+ .user_print = do_agent_capabilities_print,
+ .mhandler.cmd_async = do_agent_capabilities,
+ .flags = MONITOR_CMD_ASYNC,
+ },
+
+STEXI
+@item agent_capabilities
+@findex agent_capabilities
+Fetch and re-negotiate guest agent capabilties
+ETEXI
+SQMP
+agent_capabilities
+--------
+
+Fetch and re-negotiate guest agent capabilities
+
+Arguments:
+
+(none)
+
+Example:
+
+-> { "execute": "agent_capabilities" }
+<- { "return":["va.shutdown", "va.getdmesg", "va.getfile", ... ] }
+
+EQMP
+
+ {
.name = "qmp_capabilities",
.args_type = "",
.params = "",
diff --git a/virtagent.c b/virtagent.c
index 93329e3..269bbcd 100644
--- a/virtagent.c
+++ b/virtagent.c
@@ -460,3 +460,101 @@ int do_agent_ping(Monitor *mon, const QDict *mon_params,
xmlrpc_DECREF(params);
return ret;
}
+
+static void va_print_capability_iter(QObject *obj, void *opaque)
+{
+ Monitor *mon = opaque;
+ QString *method = qobject_to_qstring(obj);
+ const char *method_str;
+
+ if (method) {
+ method_str = qstring_get_str(method);
+ monitor_printf(mon, "%s\n", method_str);
+ }
+}
+
+void do_agent_capabilities_print(Monitor *mon, const QObject *data)
+{
+ QList *qlist;
+
+ TRACE("called");
+
+ monitor_printf(mon, "the following RPC methods are supported by the guest agent:\n");
+ qlist = qobject_to_qlist(data);
+ qlist_iter(qlist, va_print_capability_iter, mon);
+}
+
+static void do_agent_capabilities_cb(const char *resp_data,
+ size_t resp_data_len,
+ MonitorCompletion *mon_cb,
+ void *mon_data)
+{
+ xmlrpc_value *resp = NULL;
+ xmlrpc_value *cur_val = NULL;
+ const char *cur_method = NULL;
+ xmlrpc_env env;
+ QList *qlist = qlist_new();
+ int i;
+
+ TRACE("called");
+
+ if (resp_data == NULL) {
+ LOG("error handling RPC request");
+ goto out_no_resp;
+ }
+
+ TRACE("resp = %s\n", resp_data);
+
+ xmlrpc_env_init(&env);
+ resp = xmlrpc_parse_response(&env, resp_data, resp_data_len);
+ if (va_rpc_has_error(&env)) {
+ goto out_no_resp;
+ }
+
+ /* extract the list of supported RPCs */
+ for (i = 0; i < xmlrpc_array_size(&env, resp); i++) {
+ xmlrpc_array_read_item(&env, resp, i, &cur_val);
+ xmlrpc_read_string(&env, cur_val, &cur_method);
+ if (cur_method) {
+ TRACE("cur_method: %s", cur_method);
+ qlist_append_obj(qlist, QOBJECT(qstring_from_str(cur_method)));
+ }
+ xmlrpc_DECREF(cur_val);
+ }
+
+ /* set our client capabilities accordingly */
+ va_set_capabilities(qlist);
+
+ xmlrpc_DECREF(resp);
+out_no_resp:
+ if (mon_cb) {
+ mon_cb(mon_data, QOBJECT(qlist));
+ }
+ qobject_decref(QOBJECT(qlist));
+}
+
+/*
+ * do_agent_capabilities(): Fetch/re-negotiate guest agent capabilities
+ */
+int do_agent_capabilities(Monitor *mon, const QDict *mon_params,
+ MonitorCompletion cb, void *opaque)
+{
+ xmlrpc_env env;
+ xmlrpc_value *params;
+ int ret;
+
+ xmlrpc_env_init(&env);
+
+ params = xmlrpc_build_value(&env, "()");
+ if (va_rpc_has_error(&env)) {
+ return -1;
+ }
+
+ ret = va_do_rpc(&env, "system.listMethods", params,
+ do_agent_capabilities_cb, cb, opaque);
+ if (ret) {
+ qerror_report(QERR_VA_FAILED, ret, strerror(ret));
+ }
+ xmlrpc_DECREF(params);
+ return ret;
+}
diff --git a/virtagent.h b/virtagent.h
index f420c8b..8443c6f 100644
--- a/virtagent.h
+++ b/virtagent.h
@@ -39,5 +39,8 @@ int do_agent_shutdown(Monitor *mon, const QDict *mon_params,
void do_agent_ping_print(Monitor *mon, const QObject *qobject);
int do_agent_ping(Monitor *mon, const QDict *mon_params,
MonitorCompletion cb, void *opaque);
+void do_agent_capabilities_print(Monitor *mon, const QObject *qobject);
+int do_agent_capabilities(Monitor *mon, const QDict *mon_params,
+ MonitorCompletion cb, void *opaque);
#endif /* VIRTAGENT_H */
--
1.7.0.4
^ permalink raw reply related [flat|nested] 74+ messages in thread* [Qemu-devel] [RFC][PATCH v5 16/21] virtagent: add client capabilities init function
2010-12-03 18:03 [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent Michael Roth
` (14 preceding siblings ...)
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 15/21] virtagent: add agent_capabilities " Michael Roth
@ 2010-12-03 18:03 ` Michael Roth
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 17/21] virtagent: add va.hello RPC Michael Roth
` (6 subsequent siblings)
22 siblings, 0 replies; 74+ messages in thread
From: Michael Roth @ 2010-12-03 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: agl, stefanha, Jes.Sorensen, mdroth, aliguori, ryanh, abeekhof
Non-monitor version of agent_capabilities monitor function. This is
called by the local RPC server when it gets a "hello" from the guest
agent to re-negotiate guest agent capabilities.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
virtagent.c | 18 ++++++++++++++++++
virtagent.h | 1 +
2 files changed, 19 insertions(+), 0 deletions(-)
diff --git a/virtagent.c b/virtagent.c
index 269bbcd..887ad02 100644
--- a/virtagent.c
+++ b/virtagent.c
@@ -558,3 +558,21 @@ int do_agent_capabilities(Monitor *mon, const QDict *mon_params,
xmlrpc_DECREF(params);
return ret;
}
+
+/* non-HMP/QMP RPC client functions */
+
+int va_client_init_capabilities(void)
+{
+ xmlrpc_env env;
+ xmlrpc_value *params;
+
+ xmlrpc_env_init(&env);
+
+ params = xmlrpc_build_value(&env, "()");
+ if (va_rpc_has_error(&env)) {
+ return -1;
+ }
+
+ return va_do_rpc(&env, "system.listMethods", params,
+ do_agent_capabilities_cb, NULL, NULL);
+}
diff --git a/virtagent.h b/virtagent.h
index 8443c6f..440fe6d 100644
--- a/virtagent.h
+++ b/virtagent.h
@@ -42,5 +42,6 @@ int do_agent_ping(Monitor *mon, const QDict *mon_params,
void do_agent_capabilities_print(Monitor *mon, const QObject *qobject);
int do_agent_capabilities(Monitor *mon, const QDict *mon_params,
MonitorCompletion cb, void *opaque);
+int va_client_init_capabilities(void);
#endif /* VIRTAGENT_H */
--
1.7.0.4
^ permalink raw reply related [flat|nested] 74+ messages in thread* [Qemu-devel] [RFC][PATCH v5 17/21] virtagent: add va.hello RPC
2010-12-03 18:03 [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent Michael Roth
` (15 preceding siblings ...)
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 16/21] virtagent: add client capabilities init function Michael Roth
@ 2010-12-03 18:03 ` Michael Roth
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 18/21] virtagent: add "hello" notification function for guest agent Michael Roth
` (5 subsequent siblings)
22 siblings, 0 replies; 74+ messages in thread
From: Michael Roth @ 2010-12-03 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: agl, stefanha, Jes.Sorensen, mdroth, aliguori, ryanh, abeekhof
This RPC tells us the guest agent is up and ready, and invokes guest
agent capability negotiation
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
virtagent-server.c | 22 ++++++++++++++++++++++
1 files changed, 22 insertions(+), 0 deletions(-)
diff --git a/virtagent-server.c b/virtagent-server.c
index 4a8ce7d..7a1b560 100644
--- a/virtagent-server.c
+++ b/virtagent-server.c
@@ -196,6 +196,26 @@ static xmlrpc_value *va_ping(xmlrpc_env *env,
return result;
}
+/* va_hello(): handle client startup notification
+ * rpc return values: none
+ */
+
+static xmlrpc_value *va_hello(xmlrpc_env *env,
+ xmlrpc_value *param,
+ void *user_data)
+{
+ xmlrpc_value *result;
+ int ret;
+ TRACE("called");
+ SLOG("va_hello()");
+ result = xmlrpc_build_value(env, "s", "dummy");
+ ret = va_client_init_capabilities();
+ if (ret < 0) {
+ LOG("error setting initializing client capabilities");
+ }
+ return result;
+}
+
typedef struct RPCFunction {
xmlrpc_value *(*func)(xmlrpc_env *env, xmlrpc_value *param, void *unused);
const char *func_name;
@@ -215,6 +235,8 @@ static RPCFunction guest_functions[] = {
static RPCFunction host_functions[] = {
{ .func = va_ping,
.func_name = "va.ping" },
+ { .func = va_hello,
+ .func_name = "va.hello" },
{ NULL, NULL }
};
--
1.7.0.4
^ permalink raw reply related [flat|nested] 74+ messages in thread* [Qemu-devel] [RFC][PATCH v5 18/21] virtagent: add "hello" notification function for guest agent
2010-12-03 18:03 [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent Michael Roth
` (16 preceding siblings ...)
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 17/21] virtagent: add va.hello RPC Michael Roth
@ 2010-12-03 18:03 ` Michael Roth
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 19/21] virtagent: add virtagent guest daemon Michael Roth
` (4 subsequent siblings)
22 siblings, 0 replies; 74+ messages in thread
From: Michael Roth @ 2010-12-03 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: agl, stefanha, Jes.Sorensen, mdroth, aliguori, ryanh, abeekhof
This tells the host RPC server (QEMU) that we're up and running
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
virtagent.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
virtagent.h | 1 +
2 files changed, 48 insertions(+), 0 deletions(-)
diff --git a/virtagent.c b/virtagent.c
index 887ad02..e120068 100644
--- a/virtagent.c
+++ b/virtagent.c
@@ -576,3 +576,50 @@ int va_client_init_capabilities(void)
return va_do_rpc(&env, "system.listMethods", params,
do_agent_capabilities_cb, NULL, NULL);
}
+
+static void va_send_hello_cb(const char *resp_data,
+ size_t resp_data_len,
+ MonitorCompletion *mon_cb,
+ void *mon_data)
+{
+ xmlrpc_value *resp = NULL;
+ xmlrpc_env env;
+
+ TRACE("called");
+
+ if (resp_data == NULL) {
+ LOG("error handling RPC request");
+ return;
+ }
+
+ xmlrpc_env_init(&env);
+ resp = xmlrpc_parse_response(&env, resp_data, resp_data_len);
+ if (va_rpc_has_error(&env)) {
+ LOG("error parsing RPC response");
+ return;
+ }
+
+ xmlrpc_DECREF(resp);
+}
+
+int va_send_hello(void)
+{
+ xmlrpc_env env;
+ xmlrpc_value *params;
+ int ret;
+
+ TRACE("called");
+
+ xmlrpc_env_init(&env);
+ params = xmlrpc_build_value(&env, "()");
+ if (va_rpc_has_error(&env)) {
+ return -1;
+ }
+
+ ret = va_do_rpc(&env, "va.hello", params, va_send_hello_cb, NULL, NULL);
+ if (ret) {
+ qerror_report(QERR_VA_FAILED, ret, strerror(ret));
+ }
+ xmlrpc_DECREF(params);
+ return ret;
+}
diff --git a/virtagent.h b/virtagent.h
index 440fe6d..398b211 100644
--- a/virtagent.h
+++ b/virtagent.h
@@ -43,5 +43,6 @@ void do_agent_capabilities_print(Monitor *mon, const QObject *qobject);
int do_agent_capabilities(Monitor *mon, const QDict *mon_params,
MonitorCompletion cb, void *opaque);
int va_client_init_capabilities(void);
+int va_send_hello(void);
#endif /* VIRTAGENT_H */
--
1.7.0.4
^ permalink raw reply related [flat|nested] 74+ messages in thread* [Qemu-devel] [RFC][PATCH v5 19/21] virtagent: add virtagent guest daemon
2010-12-03 18:03 [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent Michael Roth
` (17 preceding siblings ...)
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 18/21] virtagent: add "hello" notification function for guest agent Michael Roth
@ 2010-12-03 18:03 ` Michael Roth
2010-12-06 22:26 ` [Qemu-devel] " Adam Litke
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 20/21] virtagent: integrate virtagent server/client via chardev Michael Roth
` (3 subsequent siblings)
22 siblings, 1 reply; 74+ messages in thread
From: Michael Roth @ 2010-12-03 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: agl, stefanha, Jes.Sorensen, mdroth, aliguori, ryanh, abeekhof
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
Makefile | 4 +-
qemu-va.c | 327 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 330 insertions(+), 1 deletions(-)
create mode 100644 qemu-va.c
diff --git a/Makefile b/Makefile
index 4e120a2..556c853 100644
--- a/Makefile
+++ b/Makefile
@@ -150,7 +150,7 @@ version-obj-$(CONFIG_WIN32) += version.o
######################################################################
qemu-img.o: qemu-img-cmds.h
-qemu-img.o qemu-tool.o qemu-nbd.o qemu-io.o: $(GENERATED_HEADERS)
+qemu-img.o qemu-tool.o qemu-nbd.o qemu-io.o qemu-va.o: $(GENERATED_HEADERS)
qemu-img$(EXESUF): qemu-img.o qemu-tool.o qemu-error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) qemu-timer-common.o
@@ -158,6 +158,8 @@ qemu-nbd$(EXESUF): qemu-nbd.o qemu-tool.o qemu-error.o $(oslib-obj-y) $(trace-ob
qemu-io$(EXESUF): qemu-io.o cmd.o qemu-tool.o qemu-error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) qemu-timer-common.o
+qemu-va$(EXESUF): qemu-va.o virtagent.o virtagent-server.o virtagent-common.o qemu-tool.o qemu-error.o qemu-sockets.c $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) qemu-timer-common.o
+
qemu-img-cmds.h: $(SRC_PATH)/qemu-img-cmds.hx
$(call quiet-command,sh $(SRC_PATH)/hxtool -h < $< > $@," GEN $@")
diff --git a/qemu-va.c b/qemu-va.c
new file mode 100644
index 0000000..1035b50
--- /dev/null
+++ b/qemu-va.c
@@ -0,0 +1,327 @@
+/*
+ * virtagent - QEMU guest agent
+ *
+ * Copyright IBM Corp. 2010
+ *
+ * Authors:
+ * Michael Roth <mdroth@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ * QEMU System Emulator
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include <getopt.h>
+#include <err.h>
+#include "qemu-option.h"
+#include "qemu_socket.h"
+#include "qemu-ioh.h"
+#include "virtagent-common.h"
+
+static bool verbose_enabled;
+#define DEBUG_ENABLED
+
+#ifdef DEBUG_ENABLED
+#define DEBUG(msg, ...) do { \
+ fprintf(stderr, "%s:%s():L%d: " msg "\n", \
+ __FILE__, __FUNCTION__, __LINE__, ## __VA_ARGS__); \
+} while(0)
+#else
+#define DEBUG(msg, ...) do {} while (0)
+#endif
+
+#define INFO(msg, ...) do { \
+ if (!verbose_enabled) { \
+ break; \
+ } \
+ warnx(msg, ## __VA_ARGS__); \
+} while(0)
+
+QemuOptsList va_opts = {
+ .name = "vaargs",
+ .head = QTAILQ_HEAD_INITIALIZER(va_opts.head),
+ .desc = {
+ {
+ .name = "channel_method",
+ .type = QEMU_OPT_STRING,
+ },{
+ .name = "path",
+ .type = QEMU_OPT_STRING,
+ },{
+ .name = "host",
+ .type = QEMU_OPT_STRING,
+ },{
+ .name = "port",
+ .type = QEMU_OPT_STRING,
+ },{
+ .name = "ipv4",
+ .type = QEMU_OPT_BOOL,
+ },{
+ .name = "ipv6",
+ .type = QEMU_OPT_BOOL,
+ },
+ { /* end of list */ }
+ },
+};
+
+/* parse channel options */
+static int va_parse(QemuOpts *opts, const char *str)
+{
+ char channel_method[32];
+ char *addr;
+ char port[33];
+ int pos, ret;
+
+ /* parse connection type */
+ ret = sscanf(str,"%32[^:]:%n",channel_method,&pos);
+ if (ret != 1) {
+ LOG("error parsing channel method");
+ return -EINVAL;
+ }
+ qemu_opt_set(opts, "channel_method", channel_method);
+ str += pos;
+ pos = 0;
+
+ /* parse path/addr and port */
+ if (str[0] == '[') {
+ /* ipv6 formatted */
+ ret = sscanf(str,"[%a[^]:]]:%32[^:]%n",&addr,port,&pos);
+ qemu_opt_set(opts, "ipv6", "on");
+ } else {
+ ret = sscanf(str,"%a[^:]:%32[^:]%n",&addr,port,&pos);
+ qemu_opt_set(opts, "ipv4", "on");
+ }
+
+ if (ret != 2) {
+ LOG("error parsing path/addr/port");
+ return -EINVAL;
+ } else if (port[0] == '-') {
+ /* no port given, assume unix path */
+ qemu_opt_set(opts, "path", addr);
+ } else {
+ qemu_opt_set(opts, "host", addr);
+ qemu_opt_set(opts, "port", port);
+ qemu_free(addr);
+ }
+ str += pos;
+ pos = 0;
+
+ return 0;
+}
+
+/* mirror qemu I/O loop for standalone daemon */
+static void main_loop_wait(int nonblocking)
+{
+ fd_set rfds, wfds, xfds;
+ int ret, nfds;
+ struct timeval tv;
+ int timeout = 100000;
+
+ if (nonblocking) {
+ timeout = 0;
+ }
+
+ /* poll any events */
+ nfds = -1;
+ FD_ZERO(&rfds);
+ FD_ZERO(&wfds);
+ FD_ZERO(&xfds);
+ qemu_get_fdset(&nfds, &rfds, &wfds, &xfds);
+
+ tv.tv_sec = timeout / 1000;
+ tv.tv_usec = (timeout % 1000) * 1000;
+
+ ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv);
+
+ if (ret > 0) {
+ qemu_process_fd_handlers(&rfds, &wfds, &xfds);
+ }
+}
+
+#define VP_ARG_LEN 256
+
+typedef struct VAData {
+ QemuOpts *opts;
+ void *opaque;
+ QTAILQ_ENTRY(VAData) next;
+} VAData;
+
+static QTAILQ_HEAD(, VAData) channels;
+
+static void usage(const char *cmd)
+{
+ printf(
+"Usage: %s -c <channel_opts>\n"
+"QEMU virtagent guest agent\n"
+"\n"
+" -c, --channel channel options of the form:\n"
+" <method>:<addr>:<port>[:channel_id]\n"
+" -v, --verbose display extra debugging information\n"
+" -h, --help display this help and exit\n"
+"\n"
+" channels are used to establish a data connection between 2 end-points in\n"
+" the host or the guest (connection method specified by <method>).\n"
+" The positional parameters for channels are:\n"
+"\n"
+" <method>: one of unix-connect, unix-listen, tcp-connect, tcp-listen,\n"
+" virtserial-open\n"
+" <addr>: path of unix socket or virtserial port, or IP of host, to\n"
+" connect/bind to\n"
+" <port>: port to bind/connect to, or '-' if addr is a path\n"
+"\n"
+"Report bugs to <mdroth@linux.vnet.ibm.com>\n"
+ , cmd);
+}
+
+static int init_channels(void) {
+ VAData *channel_data;
+ const char *channel_method, *path;
+ int fd, ret;
+ bool listen;
+
+ if (QTAILQ_EMPTY(&channels)) {
+ warnx("no channel specified");
+ return -EINVAL;
+ }
+
+ channel_data = QTAILQ_FIRST(&channels);
+
+ /* TODO: add this support, optional idx param for -i/-o/-c
+ * args should suffice
+ */
+ if (QTAILQ_NEXT(channel_data, next) != NULL) {
+ warnx("multiple channels not currently supported, defaulting to first");
+ }
+
+ INFO("initializing channel...");
+ if (verbose_enabled) {
+ qemu_opts_print(channel_data->opts, NULL);
+ }
+
+ channel_method = qemu_opt_get(channel_data->opts, "channel_method");
+
+ if (strcmp(channel_method, "tcp-listen") == 0) {
+ fd = inet_listen_opts(channel_data->opts, 0);
+ listen = true;
+ } else if (strcmp(channel_method, "tcp-connect") == 0) {
+ fd = inet_connect_opts(channel_data->opts);
+ listen = false;
+ } else if (strcmp(channel_method, "unix-listen") == 0) {
+ fd = unix_listen_opts(channel_data->opts);
+ listen = true;
+ } else if (strcmp(channel_method, "unix-connect") == 0) {
+ fd = unix_connect_opts(channel_data->opts);
+ listen = false;
+ } else if (strcmp(channel_method, "virtio-serial") == 0) {
+ path = qemu_opt_get(channel_data->opts, "path");
+ fd = qemu_open(path, O_RDWR);
+ if (fd == -1) {
+ warn("error opening channel");
+ return errno;
+ }
+ ret = fcntl(fd, F_GETFL);
+ ret = fcntl(fd, F_SETFL, ret | O_ASYNC);
+ if (ret < 0) {
+ warn("error setting flags for fd");
+ return errno;
+ }
+ listen = false;
+ } else {
+ warnx("invalid channel type: %s", channel_method);
+ return -EINVAL;
+ }
+
+ if (fd == -1) {
+ warn("error opening connection");
+ return errno;
+ }
+
+ /* initialize virtagent */
+ ret = va_init(VA_CTX_GUEST, fd);
+ if (ret) {
+ errx(EXIT_FAILURE, "unable to initialize virtagent");
+ }
+
+
+ return 0;
+}
+
+int main(int argc, char **argv)
+{
+ const char *sopt = "hVvc:";
+ struct option lopt[] = {
+ { "help", 0, NULL, 'h' },
+ { "version", 0, NULL, 'V' },
+ { "verbose", 0, NULL, 'v' },
+ { "channel", 0, NULL, 'c' },
+ { NULL, 0, NULL, 0 }
+ };
+ int opt_ind = 0, ch, ret;
+ QTAILQ_INIT(&channels);
+
+ while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
+ QemuOpts *opts;
+ VAData *data;
+ switch (ch) {
+ case 'c':
+ opts = qemu_opts_create(&va_opts, NULL, 0);
+ ret = va_parse(opts, optarg);
+ if (ret) {
+ errx(EXIT_FAILURE, "error parsing arg: %s", optarg);
+ }
+ data = qemu_mallocz(sizeof(VAData));
+ data->opts = opts;
+ QTAILQ_INSERT_TAIL(&channels, data, next);
+ break;
+ case 'v':
+ verbose_enabled = 1;
+ break;
+ case 'h':
+ usage(argv[0]);
+ return 0;
+ case '?':
+ errx(EXIT_FAILURE, "Try '%s --help' for more information.",
+ argv[0]);
+ }
+ }
+
+ /* initialize communication channel and pass it to virtagent */
+ /* XXX: we only support one channel now so this should be simplified */
+ ret = init_channels();
+ if (ret) {
+ errx(EXIT_FAILURE, "error initializing communication channel");
+ }
+
+ /* tell the host the agent is running */
+ va_send_hello();
+
+ /* main i/o loop */
+ for (;;) {
+ DEBUG("entering main_loop_wait()");
+ main_loop_wait(0);
+ DEBUG("left main_loop_wait()");
+ }
+
+ return 0;
+}
--
1.7.0.4
^ permalink raw reply related [flat|nested] 74+ messages in thread* [Qemu-devel] Re: [RFC][PATCH v5 19/21] virtagent: add virtagent guest daemon
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 19/21] virtagent: add virtagent guest daemon Michael Roth
@ 2010-12-06 22:26 ` Adam Litke
0 siblings, 0 replies; 74+ messages in thread
From: Adam Litke @ 2010-12-06 22:26 UTC (permalink / raw)
To: Michael Roth
Cc: agl, stefanha, Jes.Sorensen, qemu-devel, aliguori, ryanh, abeekhof
On Fri, 2010-12-03 at 12:03 -0600, Michael Roth wrote:
> +static void usage(const char *cmd)
> +{
> + printf(
> +"Usage: %s -c <channel_opts>\n"
> +"QEMU virtagent guest agent\n"
> +"\n"
> +" -c, --channel channel options of the form:\n"
> +" <method>:<addr>:<port>[:channel_id]\n"
> +" -v, --verbose display extra debugging information\n"
> +" -h, --help display this help and exit\n"
> +"\n"
> +" channels are used to establish a data connection between 2 end-points in\n"
> +" the host or the guest (connection method specified by <method>).\n"
> +" The positional parameters for channels are:\n"
> +"\n"
> +" <method>: one of unix-connect, unix-listen, tcp-connect, tcp-listen,\n"
> +" virtserial-open\n"
The usage information doesn't agree with the parameters the program
accepts (virtserial-open -> virtio-serial)
> +" <addr>: path of unix socket or virtserial port, or IP of host, to\n"
> +" connect/bind to\n"
> +" <port>: port to bind/connect to, or '-' if addr is a path\n"
> +"\n"
> +"Report bugs to <mdroth@linux.vnet.ibm.com>\n"
> + , cmd);
> +}
--
Thanks,
Adam
^ permalink raw reply [flat|nested] 74+ messages in thread
* [Qemu-devel] [RFC][PATCH v5 20/21] virtagent: integrate virtagent server/client via chardev
2010-12-03 18:03 [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent Michael Roth
` (18 preceding siblings ...)
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 19/21] virtagent: add virtagent guest daemon Michael Roth
@ 2010-12-03 18:03 ` Michael Roth
2010-12-07 14:44 ` [Qemu-devel] " Jes Sorensen
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 21/21] virtagent: various bits to build QEMU with virtagent Michael Roth
` (2 subsequent siblings)
22 siblings, 1 reply; 74+ messages in thread
From: Michael Roth @ 2010-12-03 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: agl, stefanha, Jes.Sorensen, mdroth, aliguori, ryanh, abeekhof
This adds a new chardev, virtagent, which actually just passes back a
socket chardev after connecting to it and initializing the agent.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
qemu-char.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 49 insertions(+), 0 deletions(-)
diff --git a/qemu-char.c b/qemu-char.c
index 88997f9..5e83e09 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2399,6 +2399,54 @@ fail:
return NULL;
}
+#include "virtagent-common.h"
+
+static CharDriverState *qemu_chr_open_virtagent(QemuOpts *opts)
+{
+ CharDriverState *chr;
+ int fd, ret;
+
+ /* revert to/enforce default socket chardev options for virtagent */
+ if (qemu_opt_get(opts, "path") == NULL) {
+ qemu_opt_set(opts, "path", "/tmp/virtagent-client.sock");
+ }
+ //qemu_opt_set(opts, "id", "virtagent");
+ qemu_opt_set(opts, "server", "on");
+ qemu_opt_set(opts, "wait", "off");
+ qemu_opt_set(opts, "telnet", "off");
+
+ chr = qemu_chr_open_socket(opts);
+ if (chr == NULL) {
+ goto err;
+ }
+
+ /* connect immediately to the socket we set up.
+ * TODO: perhaps we should cut out the socket for the virtagent
+ * chardev case and use a couple pipe pairs for i/o?
+ */
+ fd = unix_connect_opts(opts);
+ if (fd == -1) {
+ fprintf(stderr, "error connecting to virtagent socket: %s",
+ strerror(errno));
+ }
+ socket_set_nonblock(fd);
+
+ /* pass fd to virtagent */
+ ret = va_init(VA_CTX_HOST, fd);
+ if (ret != 0) {
+ fprintf(stderr, "error initializing virtagent");
+ goto err;
+ }
+
+ return chr;
+
+err:
+ if (chr) {
+ qemu_free(chr);
+ }
+ return NULL;
+}
+
static const struct {
const char *name;
CharDriverState *(*open)(QemuOpts *opts);
@@ -2408,6 +2456,7 @@ static const struct {
{ .name = "udp", .open = qemu_chr_open_udp },
{ .name = "msmouse", .open = qemu_chr_open_msmouse },
{ .name = "vc", .open = text_console_init },
+ { .name = "virtagent", .open = qemu_chr_open_virtagent },
#ifdef _WIN32
{ .name = "file", .open = qemu_chr_open_win_file_out },
{ .name = "pipe", .open = qemu_chr_open_win_pipe },
--
1.7.0.4
^ permalink raw reply related [flat|nested] 74+ messages in thread* [Qemu-devel] Re: [RFC][PATCH v5 20/21] virtagent: integrate virtagent server/client via chardev
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 20/21] virtagent: integrate virtagent server/client via chardev Michael Roth
@ 2010-12-07 14:44 ` Jes Sorensen
0 siblings, 0 replies; 74+ messages in thread
From: Jes Sorensen @ 2010-12-07 14:44 UTC (permalink / raw)
To: Michael Roth; +Cc: agl, stefanha, abeekhof, qemu-devel, aliguori, ryanh
On 12/03/10 19:03, Michael Roth wrote:
> +#include "virtagent-common.h"
> +
> +static CharDriverState *qemu_chr_open_virtagent(QemuOpts *opts)
> +{
> + CharDriverState *chr;
> + int fd, ret;
> +
> + /* revert to/enforce default socket chardev options for virtagent */
> + if (qemu_opt_get(opts, "path") == NULL) {
> + qemu_opt_set(opts, "path", "/tmp/virtagent-client.sock");
> + }
More hardcoded paths, which you defined somewhere in a header already.
Again, please make it configurable.
> + //qemu_opt_set(opts, "id", "virtagent");
If it isn't needed, please remove it.
Cheers,
Jes
^ permalink raw reply [flat|nested] 74+ messages in thread
* [Qemu-devel] [RFC][PATCH v5 21/21] virtagent: various bits to build QEMU with virtagent
2010-12-03 18:03 [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent Michael Roth
` (19 preceding siblings ...)
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 20/21] virtagent: integrate virtagent server/client via chardev Michael Roth
@ 2010-12-03 18:03 ` Michael Roth
2010-12-07 10:24 ` [Qemu-devel] Re: [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent Jes Sorensen
2010-12-08 10:10 ` [Qemu-devel] " Stefan Hajnoczi
22 siblings, 0 replies; 74+ messages in thread
From: Michael Roth @ 2010-12-03 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: agl, stefanha, Jes.Sorensen, mdroth, aliguori, ryanh, abeekhof
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
Makefile.target | 2 +-
configure | 26 ++++++++++++++++++++++++++
2 files changed, 27 insertions(+), 1 deletions(-)
diff --git a/Makefile.target b/Makefile.target
index 2800f47..c549c66 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -185,7 +185,7 @@ endif #CONFIG_BSD_USER
# System emulator target
ifdef CONFIG_SOFTMMU
-obj-y = arch_init.o cpus.o monitor.o machine.o gdbstub.o balloon.o
+obj-y = arch_init.o cpus.o monitor.o machine.o gdbstub.o balloon.o virtagent.o virtagent-server.o virtagent-common.o
# virtio has to be here due to weird dependency between PCI and virtio-net.
# need to fix this properly
obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-serial-bus.o
diff --git a/configure b/configure
index 2917874..076688b 100755
--- a/configure
+++ b/configure
@@ -1264,6 +1264,31 @@ EOF
fi
##########################################
+# xmlrpc-c probe
+
+# Look for the xmlrpc-c config program
+if test -n "$cross_prefix" && has ${cross_prefix}xmlrpc-c-config; then
+ xmlrpccconfig=${cross_prefix}xmlrpc-c-config
+elif has xmlrpc-c-config; then
+ xmlrpccconfig=xmlrpc-c-config
+else
+ feature_not_found "xmlrpc-c"
+fi
+
+cat > $TMPC << EOF
+#include <xmlrpc.h>
+int main(void) { xmlrpc_env env; xmlrpc_env_init(&env); return 0; }
+EOF
+xmlrpc_cflags=`$xmlrpccconfig --cflags 2> /dev/null`
+xmlrpc_libs=`$xmlrpccconfig client server-util --libs 2> /dev/null`
+if compile_prog "$xmlrpc_cflags" "$xmlrpc_libs"; then
+ libs_softmmu="$xmlrpc_libs $libs_softmmu"
+ libs_tools="$xmlrpc_libs $libs_tools"
+else
+ feature_not_found "xmlrpc-c"
+fi
+
+##########################################
# VNC TLS detection
if test "$vnc_tls" != "no" ; then
cat > $TMPC <<EOF
@@ -2266,6 +2291,7 @@ if test "$softmmu" = yes ; then
tools="qemu-img\$(EXESUF) qemu-io\$(EXESUF) $tools"
if [ "$linux" = "yes" -o "$bsd" = "yes" -o "$solaris" = "yes" ] ; then
tools="qemu-nbd\$(EXESUF) $tools"
+ tools="qemu-va\$(EXESUF) $tools"
if [ "$check_utests" = "yes" ]; then
tools="check-qint check-qstring check-qdict check-qlist $tools"
tools="check-qfloat check-qjson $tools"
--
1.7.0.4
^ permalink raw reply related [flat|nested] 74+ messages in thread* [Qemu-devel] Re: [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent
2010-12-03 18:03 [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent Michael Roth
` (20 preceding siblings ...)
2010-12-03 18:03 ` [Qemu-devel] [RFC][PATCH v5 21/21] virtagent: various bits to build QEMU with virtagent Michael Roth
@ 2010-12-07 10:24 ` Jes Sorensen
2010-12-07 14:29 ` Michael Roth
2010-12-08 10:10 ` [Qemu-devel] " Stefan Hajnoczi
22 siblings, 1 reply; 74+ messages in thread
From: Jes Sorensen @ 2010-12-07 10:24 UTC (permalink / raw)
To: Michael Roth; +Cc: agl, stefanha, abeekhof, qemu-devel, aliguori, ryanh
On 12/03/10 19:03, Michael Roth wrote:
> These patches apply to master, and can also be obtained from:
> git://repo.or.cz/qemu/mdroth.git virtagent_v5
>
> CHANGES IN V5:
>
> - Dependency on virtproxy dropped, virtagent now handles transport and multiplexing of bi-directional RPCs internally
> - Removed duplification of qemu_set_fd_handler()-centered i/o code. Support for interacting with objects that use qemu_set_fd_handler() now available to tools via qemu-tools.c and a set of generalized utility functions
> - Fixed memory leaks in client/monitor functions
> - Various cleanups
Hi Michael,
Does this mean that virtproxy is now obsolete, or does it just mean
using virtproxy is optional?
I would still prefer to have virtagent a separate package, rather than
part of the QEMU tree though.
Thanks,
Jes
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Qemu-devel] Re: [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent
2010-12-07 10:24 ` [Qemu-devel] Re: [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent Jes Sorensen
@ 2010-12-07 14:29 ` Michael Roth
0 siblings, 0 replies; 74+ messages in thread
From: Michael Roth @ 2010-12-07 14:29 UTC (permalink / raw)
To: Jes Sorensen; +Cc: agl, stefanha, abeekhof, qemu-devel, aliguori, ryanh
On 12/07/2010 04:24 AM, Jes Sorensen wrote:
> On 12/03/10 19:03, Michael Roth wrote:
>> These patches apply to master, and can also be obtained from:
>> git://repo.or.cz/qemu/mdroth.git virtagent_v5
>>
>> CHANGES IN V5:
>>
>> - Dependency on virtproxy dropped, virtagent now handles transport and multiplexing of bi-directional RPCs internally
>> - Removed duplification of qemu_set_fd_handler()-centered i/o code. Support for interacting with objects that use qemu_set_fd_handler() now available to tools via qemu-tools.c and a set of generalized utility functions
>> - Fixed memory leaks in client/monitor functions
>> - Various cleanups
>
> Hi Michael,
>
> Does this mean that virtproxy is now obsolete, or does it just mean
> using virtproxy is optional?
>
As far as virtagent goes it is obsolete, and without the guest-side bits
of virtproxy being integrated into the guest agent I don't see it being
very useful at this point.
> I would still prefer to have virtagent a separate package, rather than
> part of the QEMU tree though.
There's a client and server in qemu, and a client and server in the
agent, and all that code is shared. So even if we were to have a
seperate tree for the agent, 90% of the code would also be sitting in
the qemu tree anyway. I wouldn't mind hosting it outside of qemu but
given what we're trying to do there's not a whole lot to be gained from it.
I agree it'd make sense if virtagent wasn't bidirectional since then
there'd be a clean separation between qemu (client) and virtagent
(server), and it would have the added benefit of enforcing
consistent/stable client/server APIs between versions, but that's not
the case here.
>
> Thanks,
> Jes
>
>
^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent
2010-12-03 18:03 [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent Michael Roth
` (21 preceding siblings ...)
2010-12-07 10:24 ` [Qemu-devel] Re: [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent Jes Sorensen
@ 2010-12-08 10:10 ` Stefan Hajnoczi
2010-12-09 20:45 ` Michael Roth
22 siblings, 1 reply; 74+ messages in thread
From: Stefan Hajnoczi @ 2010-12-08 10:10 UTC (permalink / raw)
To: Michael Roth
Cc: agl, stefanha, Jes.Sorensen, qemu-devel, aliguori, ryanh, abeekhof
On Fri, Dec 3, 2010 at 6:03 PM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> These patches apply to master, and can also be obtained from:
> git://repo.or.cz/qemu/mdroth.git virtagent_v5
Why XML-RPC and not QMP? When I skim through the patch series it
seems like much of the work being done is very similar to QMP.
What concrete use-cases are there?
* Reboot support on x86. A QMP command can invoke guest-initiated
reboot via virtagent.
* ?
Will virtagent be extensible by host administrators or end-users? For
example, can I drop in a custom command to collect statistics and
invoke it across VMs on my hosts? Do I need to recompile QEMU and/or
the virtagent daemon?
Stefan
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent
2010-12-08 10:10 ` [Qemu-devel] " Stefan Hajnoczi
@ 2010-12-09 20:45 ` Michael Roth
2010-12-09 21:03 ` Anthony Liguori
2010-12-10 10:03 ` Stefan Hajnoczi
0 siblings, 2 replies; 74+ messages in thread
From: Michael Roth @ 2010-12-09 20:45 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: agl, stefanha, Jes.Sorensen, qemu-devel, aliguori, ryanh, abeekhof
On 12/08/2010 04:10 AM, Stefan Hajnoczi wrote:
> On Fri, Dec 3, 2010 at 6:03 PM, Michael Roth<mdroth@linux.vnet.ibm.com> wrote:
>> These patches apply to master, and can also be obtained from:
>> git://repo.or.cz/qemu/mdroth.git virtagent_v5
>
> Why XML-RPC and not QMP? When I skim through the patch series it
> seems like much of the work being done is very similar to QMP.
>
It does, actually, more than I realized. In terms of data encapsulation
I don't see why we couldn't use QMP/JSON for at least the current set of
RPCs. XMLRPC does support a wider range of data types however, such as
nestable arrays and structs, and set-precision numerical types like 32
and 64-bit ints/floats, which may prove useful for future lower level
interfaces.
QMP can be fairly trivially extended for some of these things, but JSON
is a limiting factor in some cases. We can't maintain strong typing of
data across the transport for instance, since JSON only allows for a
generic number type, whereas with XMLRPC we can explicitly specify the
machine representation. But whether this really matters is hard to say
with the limited set of RPCs we have right now.
In terms of using QMP all the way through...that sounds really cool, but
with the bi-directional client/server data being multiplexed over a
single channel, and increased potential for weird states resulting from
guest/guest agent restarts, there's a need for some additional
synchronization that might end up being fairly intrusive/undesirable to
work into QMP.
> What concrete use-cases are there?
> * Reboot support on x86. A QMP command can invoke guest-initiated
> reboot via virtagent.
> * ?
>
* viewfile
The ability to do a quick peek at guest stats via, say, /proc, is a use
case that seems to be fairly generally desirable. It's what essentially
started all this work, actually. That's also why I think a
simple/limited viewfile RPC for relatively small text files is warranted
regardless of whatever approach we end up taking for handling
large/binary file transfers.
* getdmesg
Really useful for trouble-shooting things like soft-lockups.
* ping
Heartbeat monitoring has also been a fairly re-occurring approach to
identifying potential problems in our cloud, and it's not even something
we're capable of accomplishing in a production environment due to having
limited network access to the guests. Being able to do it without
relying on custom/network-based daemons would be pretty useful.
* exec (planned)
Internally and externally I've seen interest in guest-initiated
snapshots, but that would tie into exec or some other, higher-level,
RPC, which isn't yet well-defined.
* copyfile (planned)
Nothing solid, but I think it's generally desirable. Quick access to
coredumps and such would be useful. Lots of discussion on how to
implement this and I think we have some good potential approaches to
adding this soon.
* writefile (planned)
Nothing solid. But guest activation is probably a big potential use case
for this one. Managing various guest kernel params is another. Another
would be deploying custom scripts to run via exec.
> Will virtagent be extensible by host administrators or end-users? For
> example, can I drop in a custom command to collect statistics and
> invoke it across VMs on my hosts? Do I need to recompile QEMU and/or
> the virtagent daemon?
writefile + exec would probably be the best way to achieve this. I don't
think there are any plans to make the supported set of RPCs
pluggable/extendable.
>
> Stefan
^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent
2010-12-09 20:45 ` Michael Roth
@ 2010-12-09 21:03 ` Anthony Liguori
2010-12-10 9:42 ` Stefan Hajnoczi
2010-12-10 10:03 ` Stefan Hajnoczi
1 sibling, 1 reply; 74+ messages in thread
From: Anthony Liguori @ 2010-12-09 21:03 UTC (permalink / raw)
To: Michael Roth
Cc: agl, stefanha, Stefan Hajnoczi, qemu-devel, abeekhof, ryanh,
Jes.Sorensen
On 12/09/2010 02:45 PM, Michael Roth wrote:
> On 12/08/2010 04:10 AM, Stefan Hajnoczi wrote:
>> On Fri, Dec 3, 2010 at 6:03 PM, Michael
>> Roth<mdroth@linux.vnet.ibm.com> wrote:
>>> These patches apply to master, and can also be obtained from:
>>> git://repo.or.cz/qemu/mdroth.git virtagent_v5
>>
>> Why XML-RPC and not QMP? When I skim through the patch series it
>> seems like much of the work being done is very similar to QMP.
>>
>
> It does, actually, more than I realized. In terms of data
> encapsulation I don't see why we couldn't use QMP/JSON for at least
> the current set of RPCs. XMLRPC does support a wider range of data
> types however, such as nestable arrays and structs, and set-precision
> numerical types like 32 and 64-bit ints/floats, which may prove useful
> for future lower level interfaces.
1) XML-RPC is more widely supported than QMP (as there is zero support
for QMP outside of QEMU and libvirt)
2) The target of this work is for guest agents
3) QMP does not support bidirectional RPC messages.
4) The RPC mechanism is a minor part of virt-agent so ultimately, it
kind of doesn't matter. The RPC messages themselves are what's important.
Regards,
Anthony Liguori
> QMP can be fairly trivially extended for some of these things, but
> JSON is a limiting factor in some cases. We can't maintain strong
> typing of data across the transport for instance, since JSON only
> allows for a generic number type, whereas with XMLRPC we can
> explicitly specify the machine representation. But whether this really
> matters is hard to say with the limited set of RPCs we have right now.
>
> In terms of using QMP all the way through...that sounds really cool,
> but with the bi-directional client/server data being multiplexed over
> a single channel, and increased potential for weird states resulting
> from guest/guest agent restarts, there's a need for some additional
> synchronization that might end up being fairly intrusive/undesirable
> to work into QMP.
>
>> What concrete use-cases are there?
>> * Reboot support on x86. A QMP command can invoke guest-initiated
>> reboot via virtagent.
>> * ?
>>
>
> * viewfile
> The ability to do a quick peek at guest stats via, say, /proc, is a
> use case that seems to be fairly generally desirable. It's what
> essentially started all this work, actually. That's also why I think a
> simple/limited viewfile RPC for relatively small text files is
> warranted regardless of whatever approach we end up taking for
> handling large/binary file transfers.
>
> * getdmesg
> Really useful for trouble-shooting things like soft-lockups.
>
> * ping
> Heartbeat monitoring has also been a fairly re-occurring approach to
> identifying potential problems in our cloud, and it's not even
> something we're capable of accomplishing in a production environment
> due to having limited network access to the guests. Being able to do
> it without relying on custom/network-based daemons would be pretty
> useful.
>
> * exec (planned)
> Internally and externally I've seen interest in guest-initiated
> snapshots, but that would tie into exec or some other, higher-level,
> RPC, which isn't yet well-defined.
>
> * copyfile (planned)
> Nothing solid, but I think it's generally desirable. Quick access to
> coredumps and such would be useful. Lots of discussion on how to
> implement this and I think we have some good potential approaches to
> adding this soon.
>
> * writefile (planned)
> Nothing solid. But guest activation is probably a big potential use
> case for this one. Managing various guest kernel params is another.
> Another would be deploying custom scripts to run via exec.
>
>> Will virtagent be extensible by host administrators or end-users? For
>> example, can I drop in a custom command to collect statistics and
>> invoke it across VMs on my hosts? Do I need to recompile QEMU and/or
>> the virtagent daemon?
>
> writefile + exec would probably be the best way to achieve this. I
> don't think there are any plans to make the supported set of RPCs
> pluggable/extendable.
>
>>
>> Stefan
>
^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent
2010-12-09 21:03 ` Anthony Liguori
@ 2010-12-10 9:42 ` Stefan Hajnoczi
0 siblings, 0 replies; 74+ messages in thread
From: Stefan Hajnoczi @ 2010-12-10 9:42 UTC (permalink / raw)
To: Anthony Liguori
Cc: agl, stefanha, Jes.Sorensen, Michael Roth, qemu-devel, ryanh, abeekhof
On Thu, Dec 9, 2010 at 9:03 PM, Anthony Liguori
<aliguori@linux.vnet.ibm.com> wrote:
> On 12/09/2010 02:45 PM, Michael Roth wrote:
>>
>> On 12/08/2010 04:10 AM, Stefan Hajnoczi wrote:
>>>
>>> On Fri, Dec 3, 2010 at 6:03 PM, Michael Roth<mdroth@linux.vnet.ibm.com>
>>> wrote:
>>>>
>>>> These patches apply to master, and can also be obtained from:
>>>> git://repo.or.cz/qemu/mdroth.git virtagent_v5
>>>
>>> Why XML-RPC and not QMP? When I skim through the patch series it
>>> seems like much of the work being done is very similar to QMP.
>>>
>>
>> It does, actually, more than I realized. In terms of data encapsulation I
>> don't see why we couldn't use QMP/JSON for at least the current set of RPCs.
>> XMLRPC does support a wider range of data types however, such as nestable
>> arrays and structs, and set-precision numerical types like 32 and 64-bit
>> ints/floats, which may prove useful for future lower level interfaces.
>
> 1) XML-RPC is more widely supported than QMP (as there is zero support for
> QMP outside of QEMU and libvirt)
True, but the current design doesn't lend itself to custom agents at
the XML-RPC level. Therefore using a standard protocol doesn't win
anything - it would have been more useful for QMP actually.
> 2) The target of this work is for guest agents
Not sure what this means.
> 3) QMP does not support bidirectional RPC messages.
Right. QMP has commands, responses, and asynchronous events so the
client and server are two different roles (asymmetric).
I hadn't though through the details and this suggests shoehorning QMP
in as the agent protocol would be ugly.
> 4) The RPC mechanism is a minor part of virt-agent so ultimately, it kind of
> doesn't matter. The RPC messages themselves are what's important.
Getting the design and features of virtagent right is key. But we
still need to maintain the code, that's why I ask about reusing what
we've got.
Stefan
^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [Qemu-devel] [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent
2010-12-09 20:45 ` Michael Roth
2010-12-09 21:03 ` Anthony Liguori
@ 2010-12-10 10:03 ` Stefan Hajnoczi
1 sibling, 0 replies; 74+ messages in thread
From: Stefan Hajnoczi @ 2010-12-10 10:03 UTC (permalink / raw)
To: Michael Roth
Cc: agl, stefanha, Jes.Sorensen, qemu-devel, aliguori, ryanh, abeekhof
On Thu, Dec 9, 2010 at 8:45 PM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> On 12/08/2010 04:10 AM, Stefan Hajnoczi wrote:
>> What concrete use-cases are there?
>> * Reboot support on x86. A QMP command can invoke guest-initiated
>> reboot via virtagent.
>> * ?
>>
>
> * viewfile
> The ability to do a quick peek at guest stats via, say, /proc, is a use case
> that seems to be fairly generally desirable. It's what essentially started
> all this work, actually. That's also why I think a simple/limited viewfile
> RPC for relatively small text files is warranted regardless of whatever
> approach we end up taking for handling large/binary file transfers.
>
> * getdmesg
> Really useful for trouble-shooting things like soft-lockups.
>
> * ping
> Heartbeat monitoring has also been a fairly re-occurring approach to
> identifying potential problems in our cloud, and it's not even something
> we're capable of accomplishing in a production environment due to having
> limited network access to the guests. Being able to do it without relying on
> custom/network-based daemons would be pretty useful.
>
> * exec (planned)
> Internally and externally I've seen interest in guest-initiated snapshots,
> but that would tie into exec or some other, higher-level, RPC, which isn't
> yet well-defined.
>
> * copyfile (planned)
> Nothing solid, but I think it's generally desirable. Quick access to
> coredumps and such would be useful. Lots of discussion on how to implement
> this and I think we have some good potential approaches to adding this soon.
>
> * writefile (planned)
> Nothing solid. But guest activation is probably a big potential use case for
> this one. Managing various guest kernel params is another. Another would be
> deploying custom scripts to run via exec.
Perhaps "getfile" and "putfile" as names?
>> Will virtagent be extensible by host administrators or end-users? For
>> example, can I drop in a custom command to collect statistics and
>> invoke it across VMs on my hosts? Do I need to recompile QEMU and/or
>> the virtagent daemon?
>
> writefile + exec would probably be the best way to achieve this. I don't
> think there are any plans to make the supported set of RPCs
> pluggable/extendable.
Thanks for explaining this, I think this is an important aspect of
virtagent. Users will want to take advantage of an always-available
host<->guest management transport. There should be a clear scope
defined for virtagent.
For example users might include backup, monitoring, and inventory
(making sure installed software is up-to-date) software.
I imagine just how flexible and easy to use will be controversial
because some people may feel users should use this channel into the VM
carefully and only in situations where other remote access methods are
not suitable (e.g. ssh, etc).
If there is a flexible interface from the start then new use-cases can
be implemented without modifying QEMU/virtagent source code and
updating guests.
Stefan
^ permalink raw reply [flat|nested] 74+ messages in thread