[RFC,v1,08/12] qemu-char: add qmp_proxy chardev

This allows qemu to be started with guest agent support via:

qemu -chardev qmp_proxy,path=<optional>,id=qmp_proxy \
      -device ...,chardev=qmp_proxy

It is essentially a wrapper for -chardev socket, which takes the extra
step of setting required defaults and initializing the qmp proxy by
passing the path argument along.

Not sure if this is the most elegant approach, but in terms of the
command-line invocation it seems to be the most consistent way to do
it.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qemu-char.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 46 insertions(+), 0 deletions(-)

On 03/25/2011 02:47 PM, Michael Roth wrote:
> This allows qemu to be started with guest agent support via:
>
> qemu -chardev qmp_proxy,path=<optional>,id=qmp_proxy \
>        -device ...,chardev=qmp_proxy
>
> It is essentially a wrapper for -chardev socket, which takes the extra
> step of setting required defaults and initializing the qmp proxy by
> passing the path argument along.
>
> Not sure if this is the most elegant approach, but in terms of the
> command-line invocation it seems to be the most consistent way to do
> it.
>
> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
> ---
>   qemu-char.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 files changed, 46 insertions(+), 0 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index d301925..6ff7698 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2275,6 +2275,51 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
>       return NULL;
>   }
>
> +#include "qmp-proxy-core.h"
> +
> +extern QmpProxy *qmp_proxy_default;
> +
> +static CharDriverState *qemu_chr_open_qmp_proxy(QemuOpts *opts)
> +{
> +    CharDriverState *chr;
> +    QmpProxy *p;
> +    const char *path;
> +
> +    /* revert to/enforce default socket chardev options for qmp proxy */
> +    path = qemu_opt_get(opts, "path");
> +    if (path == NULL) {
> +        path = QMP_PROXY_PATH_DEFAULT;
> +        qemu_opt_set_qerr(opts, "path", path);
> +    }
> +    /* required options for qmp proxy */
> +    qemu_opt_set_qerr(opts, "server", "on");
> +    qemu_opt_set_qerr(opts, "wait", "off");
> +    qemu_opt_set_qerr(opts, "telnet", "off");

Why are these options required?

Regards,

Anthony Liguori

> +
> +    chr = qemu_chr_open_socket(opts);
> +    if (chr == NULL) {
> +        goto err;
> +    }
> +
> +    /* initialize virtagent using the socket we just set up */
> +    if (qmp_proxy_default) {
> +        fprintf(stderr, "error, multiple qmp guest proxies are not allowed\n");
> +    }
> +    p = qmp_proxy_new(path);
> +    if (p == NULL) {
> +        fprintf(stderr, "error initializing qmp guest proxy\n");
> +        goto err;
> +    }
> +    qmp_proxy_default = p;
> +
> +    return chr;
> +err:
> +    if (chr) {
> +        qemu_free(chr);
> +    }
> +    return NULL;
> +}
> +
>   /***********************************************************/
>   /* Memory chardev */
>   typedef struct {
> @@ -2495,6 +2540,7 @@ static const struct {
>       || defined(__FreeBSD_kernel__)
>       { .name = "parport",   .open = qemu_chr_open_pp },
>   #endif
> +    { .name = "qmp_proxy", .open = qemu_chr_open_qmp_proxy },
>   };
>
>   CharDriverState *qemu_chr_open_opts(QemuOpts *opts,

On 03/25/2011 04:29 PM, Anthony Liguori wrote:
> On 03/25/2011 02:47 PM, Michael Roth wrote:
>> This allows qemu to be started with guest agent support via:
>>
>> qemu -chardev qmp_proxy,path=<optional>,id=qmp_proxy \
>> -device ...,chardev=qmp_proxy
>>
>> It is essentially a wrapper for -chardev socket, which takes the extra
>> step of setting required defaults and initializing the qmp proxy by
>> passing the path argument along.
>>
>> Not sure if this is the most elegant approach, but in terms of the
>> command-line invocation it seems to be the most consistent way to do
>> it.
>>
>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>> ---
>> qemu-char.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 46 insertions(+), 0 deletions(-)
>>
>> diff --git a/qemu-char.c b/qemu-char.c
>> index d301925..6ff7698 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -2275,6 +2275,51 @@ static CharDriverState
>> *qemu_chr_open_socket(QemuOpts *opts)
>> return NULL;
>> }
>>
>> +#include "qmp-proxy-core.h"
>> +
>> +extern QmpProxy *qmp_proxy_default;
>> +
>> +static CharDriverState *qemu_chr_open_qmp_proxy(QemuOpts *opts)
>> +{
>> + CharDriverState *chr;
>> + QmpProxy *p;
>> + const char *path;
>> +
>> + /* revert to/enforce default socket chardev options for qmp proxy */
>> + path = qemu_opt_get(opts, "path");
>> + if (path == NULL) {
>> + path = QMP_PROXY_PATH_DEFAULT;
>> + qemu_opt_set_qerr(opts, "path", path);
>> + }
>> + /* required options for qmp proxy */
>> + qemu_opt_set_qerr(opts, "server", "on");
>> + qemu_opt_set_qerr(opts, "wait", "off");
>> + qemu_opt_set_qerr(opts, "telnet", "off");
>
> Why are these options required?

The qmp_proxy_new() constructor expects a path to a socket it can 
connect() to. Not sure about telnet, but the other options are required 
for this. Well...server=on at least, wait=off needs to be set as well 
because that's the only way to have the socket chardev set the listening 
socket to non-block, since qmp_proxy_new() calls connect() on it before 
we return to the main I/O loop.

Although, we probably shouldn't just silently switch out explicitly set 
options; an error would probably be more appropriate here.

>
> Regards,
>
> Anthony Liguori
>

On 03/25/2011 05:11 PM, Michael Roth wrote:
>> Why are these options required?
>
>
> The qmp_proxy_new() constructor expects a path to a socket it can 
> connect() to. Not sure about telnet, but the other options are 
> required for this. Well...server=on at least, wait=off needs to be set 
> as well because that's the only way to have the socket chardev set the 
> listening socket to non-block, since qmp_proxy_new() calls connect() 
> on it before we return to the main I/O loop.
>
> Although, we probably shouldn't just silently switch out explicitly 
> set options; an error would probably be more appropriate here.

You ought to make qmp_proxy_new() return a CharDriverState such that you 
can do:

qemu -device virtio-serial,chardev=foo -chardev guest-agent,id=foo

Regards,

Anthony Liguori

>>
>> Regards,
>>
>> Anthony Liguori
>>

On 03/28/2011 12:45 PM, Anthony Liguori wrote:
> On 03/25/2011 05:11 PM, Michael Roth wrote:
>>> Why are these options required?
>>
>>
>> The qmp_proxy_new() constructor expects a path to a socket it can
>> connect() to. Not sure about telnet, but the other options are
>> required for this. Well...server=on at least, wait=off needs to be set
>> as well because that's the only way to have the socket chardev set the
>> listening socket to non-block, since qmp_proxy_new() calls connect()
>> on it before we return to the main I/O loop.
>>
>> Although, we probably shouldn't just silently switch out explicitly
>> set options; an error would probably be more appropriate here.
>
> You ought to make qmp_proxy_new() return a CharDriverState such that you
> can do:
>
> qemu -device virtio-serial,chardev=foo -chardev guest-agent,id=foo
>

That's what the current invocation looks like, but with regard to not 
relying on an intermediate socket between QmpProxy and individual qmp 
sessions, I think the main issue is:

- We expose the proxy via a call such a qmp_proxy_send_request(QDict 
request)
- This request can be arbitrarily large (not atm, but in the future 
perhaps with RPCs sent as qmp_send_file() they may potential by large)

So if we do this directly via a new char state rather than intermediate 
sock, we'd either:

1) Send the request in full via, say, 
qmp_proxy_send_request()->qemu_chr_read()->virtio_serial_write(), and 
assume the port/device can actually buffer it all in one shot. Or,

2) Send it in smaller chunks, via something that amounts to:

     can_read_bytes = virtio_serial_guest_ready()):
     virtio_serial_write(port, request, can_read_bytes)

    If anything is left over, we need to add something QEMU's main loop 
to handle some of the remaining bytes in the next main loop iteration. 
(With the intermediate socket, we achieve this by registering a write 
handler that selects on the intermediate socket and writes in small 
chunks until the buffer is empty, the socket chardev does all the work 
of checking for the backend device to be ready and not writing more than 
it should)

So, if we do 2), which I think is the only "correct" approach out of the 
2, to take the intermediate socket fd out of the equation, we either 
need to add a custom handler to QEMU's main loop, or register deferred 
work via something like qemu_bh_schedule_idle().

So I think that's the trade-off...we can:

- do what the patches currently do and complicate the plumbing by adding 
an intermediate socket (or perhaps even just a pipe), and simplify the 
logic of handling backlogged work via a select()-driven write handler, or

- remove the intermediate socket to simplify the plumbing, but 
complicate the back-end logic involved in sending requests to the guest 
by using a BH to clear our host->guest TX buffer

If using a BH to handle this work seems reasonable, then I'd be fine 
with attempting to implement this. If using a BH seems nasty, then I 
think the current intermediate socket/fd approach is the best alternative.

> Regards,
>
> Anthony Liguori
>