Re: [Qemu-devel] [PATCH 03/11] QMP: Introduce protocol print functions

[Eduardo Habkost <ehabkost@redhat.com>]

On Tue, Jun 23, 2009 at 08:45:06AM -0500, Anthony Liguori wrote:
> Luiz Capitulino wrote:
>> This change introduces the functions that will be used by the Monitor
>> to output data in the format defined by the protocol specification.
>>
>> There are four functions:
>>
>>  o monitor_printf_bad()  signals a protocol error
>>  o monitor_print_ok()    signals successful execution
>>  o monitor_printf_err()  used by commands, to signal execution errors
>>  o monitor_printf_data() used by commands, to output data
>>
>> For now monitor_print_ok() compilation is disabled to avoid breaking
>> the build as the function is not currently used. It will be enabled
>> by a later commit.
>>
>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> ---
>>  monitor.c   |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  monitor.h   |    3 ++
>>  qemu-tool.c |   12 +++++++++++
>>  3 files changed, 78 insertions(+), 0 deletions(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index 514db00..dfa777d 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -199,6 +199,69 @@ static int monitor_fprintf(FILE *stream, const char *fmt, ...)
>>      return 0;
>>  }
>>  +/*
>> + * QEMU Monitor Control print functions
>> + */
>> +
>> +/* Protocol errors */
>> +void monitor_printf_bad(Monitor *mon, const char *fmt, ...)
>> +{
>> +    if (monitor_ctrl_mode(mon)) {
>> +        va_list ap;
>> +
>> +        monitor_puts(mon, "- BAD ");
>> +
>> +        va_start(ap, fmt);
>> +        monitor_vprintf(mon, fmt, ap);
>> +        va_end(ap);
>> +    }
>> +}
>> +
>> +#if 0
>> +/* OK command completion, 'info' commands are special */
>> +static void monitor_print_ok(Monitor *mon, const char *cmd, const char *arg)
>> +{
>> +    if (!monitor_ctrl_mode(mon))
>> +        return;
>> +
>> +    monitor_printf(mon, "+ OK %s", cmd);
>> +    if (!strcmp(cmd, "info"))
>> +        monitor_printf(mon, " %s", arg);
>> +    monitor_puts(mon, " completed\n");
>> +}
>> +#endif
>> +
>> +static void monitor_ctrl_pprintf(Monitor *mon, const char *prefix,
>> +                                 const char *fmt, va_list ap)
>> +{
>> +    if (monitor_ctrl_mode(mon))
>> +        monitor_puts(mon, prefix);
>> +
>> +    monitor_vprintf(mon, fmt, ap);
>> +}
>> +
>> +/* Should be used by commands to signal errors, will add the prefix
>> + * '- ERR ' if in control mode */
>> +void monitor_printf_err(Monitor *mon, const char *fmt, ...)
>> +{
>> +    va_list ap;
>> +
>> +    va_start(ap, fmt);
>> +    monitor_ctrl_pprintf(mon, "- ERR ", fmt, ap);
>> +    va_end(ap);
>> +}
>> +
>> +/* Should be used by commands to print any output which is not
>> + * an error, will add the prefix '= ' if in control mode */
>> +void monitor_printf_data(Monitor *mon, const char *fmt, ...)
>> +{
>> +    va_list ap;
>> +
>> +    va_start(ap, fmt);
>> +    monitor_ctrl_pprintf(mon, "= ", fmt, ap);
>> +    va_end(ap);
>> +}
>> +
>>  static int compare_cmd(const char *name, const char *list)
>>  {
>>      const char *p, *pstart;
>> diff --git a/monitor.h b/monitor.h
>> index 48bc056..8b054eb 100644
>> --- a/monitor.h
>> +++ b/monitor.h
>> @@ -24,6 +24,9 @@ void monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
>>  void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap);
>>  void monitor_printf(Monitor *mon, const char *fmt, ...)
>>      __attribute__ ((__format__ (__printf__, 2, 3)));
>> +void monitor_printf_bad(Monitor *mon, const char *fmt, ...);
>> +void monitor_printf_err(Monitor *mon, const char *fmt, ...);
>> +void monitor_printf_data(Monitor *mon, const char *fmt, ...);
>>   
>
> Probably could just introduce a flag to monitor_printf().  In fact, you  
> could go kernel style and have:
>
> monitor_printf(mon, MON_BAD "bad monitor command\n");
> monitor_printf(mon, MON_DATA "some monitor data\n");
>
> With MON_DATA being explicit.
>
> If not, make sure to add the format attribute to the printfs.

I think different functions make sense because some messages may have
additional attributes (not only a text message), that may be arguments
to the function. e.g.:

void monitor_printf_event(Monitor *mon, qemu_event_t event, const char *fmt, ...);

void monitor_printf_err(Monitor *mon, int status_code, const char *fmt, ...);

(or maybe we can just call it monitor_printf_result(), being applicable
to both success and failure, depending on the status code).

-- 
Eduardo