[Qemu-devel] [PATCH 00/14] Convert commands to QAPI (batch 1)

[Qemu-devel] [PATCH 00/14] Convert commands to QAPI (batch 1)

[Anthony Liguori]

This is my attempt to jump-start the QAPI switch over.  All of the hard work
is already done in my qapi branch, we just need to start merging stuff.

To simplify the merge process, I've introduced a new mode to the code generator
that lets us do conversions without using the new QMP server.

Once this series is merged, anything that touchs QMP (to modify a command or
add a new command) must do it through QAPI--no exceptions.

This series also includes Dans change to the 'change' command.  I'm not thrilled
about people using this command but its better to err on the side of caution.
It's now officially deprecated with much more robust replacement commands.

[Qemu-devel] [PATCH 01/14] qerror: add qerror_report_err()

[Anthony Liguori]

This provides a bridge between Error (new error mechanism) and QError (old error
mechanism).  Errors can be propagated whereas QError cannot.

The minor evilness avoids layering violations.  Since QError should go away RSN,
it seems like a reasonable hack.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 qerror.c |   33 +++++++++++++++++++++++++++++++++
 qerror.h |    2 ++
 2 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index 3d64b80..fa647a6 100644
--- a/qerror.c
+++ b/qerror.c
@@ -478,6 +478,39 @@ void qerror_report_internal(const char *file, int linenr, const char *func,
     }
 }
 
+/* Evil... */
+struct Error
+{
+    QDict *obj;
+    const char *fmt;
+    char *msg;
+};
+
+void qerror_report_err(Error *err)
+{
+    QError *qerr;
+    int i;
+
+    qerr = qerror_new();
+    loc_save(&qerr->loc);
+    QINCREF(err->obj);
+    qerr->error = err->obj;
+
+    for (i = 0; qerror_table[i].error_fmt; i++) {
+        if (strcmp(qerror_table[i].error_fmt, err->fmt) == 0) {
+            qerr->entry = &qerror_table[i];
+            return;
+        }
+    }
+
+    if (monitor_cur_is_qmp()) {
+        monitor_set_error(cur_mon, qerr);
+    } else {
+        qerror_print(qerr);
+        QDECREF(qerr);
+    }
+}
+
 /**
  * qobject_to_qerror(): Convert a QObject into a QError
  */
diff --git a/qerror.h b/qerror.h
index 8058456..4fe24aa 100644
--- a/qerror.h
+++ b/qerror.h
@@ -15,6 +15,7 @@
 #include "qdict.h"
 #include "qstring.h"
 #include "qemu-error.h"
+#include "error.h"
 #include <stdarg.h>
 
 typedef struct QErrorStringTable {
@@ -39,6 +40,7 @@ QString *qerror_human(const QError *qerror);
 void qerror_print(QError *qerror);
 void qerror_report_internal(const char *file, int linenr, const char *func,
                             const char *fmt, ...) GCC_FMT_ATTR(4, 5);
+void qerror_report_err(Error *err);
 QString *qerror_format(const char *fmt, QDict *error);
 #define qerror_report(fmt, ...) \
     qerror_report_internal(__FILE__, __LINE__, __func__, fmt, ## __VA_ARGS__)
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 01/14] qerror: add qerror_report_err()

[Luiz Capitulino]

On Wed, 24 Aug 2011 13:42:56 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> This provides a bridge between Error (new error mechanism) and QError (old error
> mechanism).  Errors can be propagated whereas QError cannot.
> 
> The minor evilness avoids layering violations.  Since QError should go away RSN,
> it seems like a reasonable hack.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  qerror.c |   33 +++++++++++++++++++++++++++++++++
>  qerror.h |    2 ++
>  2 files changed, 35 insertions(+), 0 deletions(-)
> 
> diff --git a/qerror.c b/qerror.c
> index 3d64b80..fa647a6 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -478,6 +478,39 @@ void qerror_report_internal(const char *file, int linenr, const char *func,
>      }
>  }
>  
> +/* Evil... */
> +struct Error
> +{
> +    QDict *obj;
> +    const char *fmt;
> +    char *msg;
> +};

Given that we're in hack mode, I think I'd prefer to have struct Error in
error.h and then include it here.

> +
> +void qerror_report_err(Error *err)
> +{
> +    QError *qerr;
> +    int i;
> +
> +    qerr = qerror_new();
> +    loc_save(&qerr->loc);
> +    QINCREF(err->obj);
> +    qerr->error = err->obj;
> +
> +    for (i = 0; qerror_table[i].error_fmt; i++) {
> +        if (strcmp(qerror_table[i].error_fmt, err->fmt) == 0) {
> +            qerr->entry = &qerror_table[i];
> +            return;

You have to drop this return, else the if clause below won't be
executed. Or you could just use qerror_set_desc().

> +        }
> +    }
> +
> +    if (monitor_cur_is_qmp()) {
> +        monitor_set_error(cur_mon, qerr);
> +    } else {
> +        qerror_print(qerr);
> +        QDECREF(qerr);
> +    }
> +}
> +
>  /**
>   * qobject_to_qerror(): Convert a QObject into a QError
>   */
> diff --git a/qerror.h b/qerror.h
> index 8058456..4fe24aa 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -15,6 +15,7 @@
>  #include "qdict.h"
>  #include "qstring.h"
>  #include "qemu-error.h"
> +#include "error.h"
>  #include <stdarg.h>
>  
>  typedef struct QErrorStringTable {
> @@ -39,6 +40,7 @@ QString *qerror_human(const QError *qerror);
>  void qerror_print(QError *qerror);
>  void qerror_report_internal(const char *file, int linenr, const char *func,
>                              const char *fmt, ...) GCC_FMT_ATTR(4, 5);
> +void qerror_report_err(Error *err);
>  QString *qerror_format(const char *fmt, QDict *error);
>  #define qerror_report(fmt, ...) \
>      qerror_report_internal(__FILE__, __LINE__, __func__, fmt, ## __VA_ARGS__)

Re: [Qemu-devel] [PATCH 01/14] qerror: add qerror_report_err()

[Anthony Liguori]

On 08/24/2011 03:15 PM, Luiz Capitulino wrote:
> On Wed, 24 Aug 2011 13:42:56 -0500
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>
>> This provides a bridge between Error (new error mechanism) and QError (old error
>> mechanism).  Errors can be propagated whereas QError cannot.
>>
>> The minor evilness avoids layering violations.  Since QError should go away RSN,
>> it seems like a reasonable hack.
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>> ---
>>   qerror.c |   33 +++++++++++++++++++++++++++++++++
>>   qerror.h |    2 ++
>>   2 files changed, 35 insertions(+), 0 deletions(-)
>>
>> diff --git a/qerror.c b/qerror.c
>> index 3d64b80..fa647a6 100644
>> --- a/qerror.c
>> +++ b/qerror.c
>> @@ -478,6 +478,39 @@ void qerror_report_internal(const char *file, int linenr, const char *func,
>>       }
>>   }
>>
>> +/* Evil... */
>> +struct Error
>> +{
>> +    QDict *obj;
>> +    const char *fmt;
>> +    char *msg;
>> +};
>
> Given that we're in hack mode, I think I'd prefer to have struct Error in
> error.h and then include it here.

That adds a QObject dependency to error.h which cascades a bunch of 
dependencies.  I don't like this much either but I think it's the least 
of a few evils.

>
>> +
>> +void qerror_report_err(Error *err)
>> +{
>> +    QError *qerr;
>> +    int i;
>> +
>> +    qerr = qerror_new();
>> +    loc_save(&qerr->loc);
>> +    QINCREF(err->obj);
>> +    qerr->error = err->obj;
>> +
>> +    for (i = 0; qerror_table[i].error_fmt; i++) {
>> +        if (strcmp(qerror_table[i].error_fmt, err->fmt) == 0) {
>> +            qerr->entry =&qerror_table[i];
>> +            return;
>
> You have to drop this return, else the if clause below won't be
> executed. Or you could just use qerror_set_desc().


Thanks!

Regards,

Anthony Liguori

>> +        }
>> +    }
>> +
>> +    if (monitor_cur_is_qmp()) {
>> +        monitor_set_error(cur_mon, qerr);
>> +    } else {
>> +        qerror_print(qerr);
>> +        QDECREF(qerr);
>> +    }
>> +}
>> +
>>   /**
>>    * qobject_to_qerror(): Convert a QObject into a QError
>>    */
>> diff --git a/qerror.h b/qerror.h
>> index 8058456..4fe24aa 100644
>> --- a/qerror.h
>> +++ b/qerror.h
>> @@ -15,6 +15,7 @@
>>   #include "qdict.h"
>>   #include "qstring.h"
>>   #include "qemu-error.h"
>> +#include "error.h"
>>   #include<stdarg.h>
>>
>>   typedef struct QErrorStringTable {
>> @@ -39,6 +40,7 @@ QString *qerror_human(const QError *qerror);
>>   void qerror_print(QError *qerror);
>>   void qerror_report_internal(const char *file, int linenr, const char *func,
>>                               const char *fmt, ...) GCC_FMT_ATTR(4, 5);
>> +void qerror_report_err(Error *err);
>>   QString *qerror_format(const char *fmt, QDict *error);
>>   #define qerror_report(fmt, ...) \
>>       qerror_report_internal(__FILE__, __LINE__, __func__, fmt, ## __VA_ARGS__)
>
>

[Qemu-devel] [PATCH 02/14] qapi: add code generation support for middle mode

[Anthony Liguori]

To get the ball rolling merging QAPI, this patch introduces a "middle mode" to
the code generator.  In middle mode, the code generator generates marshalling
functions that are compatible with the current QMP server.  We absolutely need
to replace the current QMP server in order to support proper asynchronous
commands but using a middle mode provides a middle-ground that lets us start
converting commands in tree.

Note that all of the commands have been converted already in my glib branch.
Middle mode only exists until we finish merging them from my branch into the
main tree.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 qapi/qapi-types-core.h   |    3 ++
 scripts/qapi-commands.py |   79 +++++++++++++++++++++++++++++++++++++--------
 scripts/qapi-types.py    |    3 ++
 scripts/qapi.py          |    4 ++-
 4 files changed, 74 insertions(+), 15 deletions(-)

diff --git a/qapi/qapi-types-core.h b/qapi/qapi-types-core.h
index a79bc2b..27e6be0 100644
--- a/qapi/qapi-types-core.h
+++ b/qapi/qapi-types-core.h
@@ -17,4 +17,7 @@
 #include "qemu-common.h"
 #include "error.h"
 
+/* FIXME this is temporary until we remove middle mode */
+#include "monitor.h"
+
 #endif
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index bf61740..f5d402c 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -167,9 +167,10 @@ qmp_input_visitor_cleanup(mi);
     pop_indent()
     return ret.rstrip()
 
-def gen_marshal_output(name, args, ret_type):
+def gen_marshal_output(name, args, ret_type, middle_mode):
     if not ret_type:
         return ""
+
     ret = mcgen('''
 static void qmp_marshal_output_%(c_name)s(%(c_ret_type)s ret_in, QObject **ret_out, Error **errp)
 {
@@ -188,16 +189,34 @@ static void qmp_marshal_output_%(c_name)s(%(c_ret_type)s ret_in, QObject **ret_o
     qapi_dealloc_visitor_cleanup(md);
 }
 ''',
-            c_ret_type=c_type(ret_type), c_name=c_var(name), ret_type=ret_type)
+                 c_ret_type=c_type(ret_type), c_name=c_var(name),
+                 ret_type=ret_type)
 
     return ret
 
-def gen_marshal_input(name, args, ret_type):
+def gen_marshal_input_decl(name, args, ret_type, middle_mode):
+    if middle_mode:
+        return 'int qmp_marshal_input_%s(Monitor *mon, const QDict *qdict, QObject **ret)' % c_var(name)
+    else:
+        return 'static void qmp_marshal_input_%s(QDict *args, QObject **ret, Error **errp)' % c_var(name)
+
+
+
+def gen_marshal_input(name, args, ret_type, middle_mode):
+    hdr = gen_marshal_input_decl(name, args, ret_type, middle_mode)
+
     ret = mcgen('''
-static void qmp_marshal_input_%(c_name)s(QDict *args, QObject **ret, Error **errp)
+%(header)s
 {
 ''',
-                c_name=c_var(name))
+                header=hdr)
+
+    if middle_mode:
+        ret += mcgen('''
+    Error *local_err = NULL;
+    Error **errp = &local_err;
+    QDict *args = (QDict *)qdict;
+''')
 
     if ret_type:
         if c_type(ret_type).endswith("*"):
@@ -220,6 +239,10 @@ static void qmp_marshal_input_%(c_name)s(QDict *args, QObject **ret, Error **err
                      visitor_input_containers_decl=gen_visitor_input_containers_decl(args),
                      visitor_input_vars_decl=gen_visitor_input_vars_decl(args),
                      visitor_input_block=gen_visitor_input_block(args, "QOBJECT(args)"))
+    else:
+        ret += mcgen('''
+    (void)args;
+''')
 
     ret += mcgen('''
     if (error_is_set(errp)) {
@@ -234,10 +257,29 @@ out:
 ''')
     ret += mcgen('''
 %(visitor_input_block_cleanup)s
+''',
+                 visitor_input_block_cleanup=gen_visitor_input_block(args, None,
+                                                                     dealloc=True))
+
+    if middle_mode:
+        ret += mcgen('''
+                                                                    
+    if (local_err) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+        return -1;
+    }
+    return 0;
+''')
+    else:
+        ret += mcgen('''
     return;
+''')
+
+    ret += mcgen('''
 }
-''',
-                 visitor_input_block_cleanup=gen_visitor_input_block(args, None, dealloc=True))
+''')
+
     return ret
 
 def gen_registry(commands):
@@ -284,7 +326,7 @@ def gen_command_decl_prologue(header, guard, prefix=""):
 #include "error.h"
 
 ''',
-                 header=basename(h_file), guard=guardname(h_file), prefix=prefix)
+                 header=basename(header), guard=guardname(header), prefix=prefix)
     return ret
 
 def gen_command_def_prologue(prefix="", proxy=False):
@@ -317,11 +359,11 @@ def gen_command_def_prologue(prefix="", proxy=False):
                 prefix=prefix)
     if not proxy:
         ret += '#include "%sqmp-commands.h"' % prefix
-    return ret + "\n"
+    return ret + "\n\n"
 
 
 try:
-    opts, args = getopt.gnu_getopt(sys.argv[1:], "p:o:", ["prefix=", "output-dir=", "type="])
+    opts, args = getopt.gnu_getopt(sys.argv[1:], "p:o:m", ["prefix=", "output-dir=", "type=", "middle"])
 except getopt.GetoptError, err:
     print str(err)
     sys.exit(1)
@@ -331,6 +373,7 @@ prefix = ""
 dispatch_type = "sync"
 c_file = 'qmp-marshal.c'
 h_file = 'qmp-commands.h'
+middle_mode = False
 
 for o, a in opts:
     if o in ("-p", "--prefix"):
@@ -339,6 +382,8 @@ for o, a in opts:
         output_dir = a + "/"
     elif o in ("-t", "--type"):
         dispatch_type = a
+    elif o in ("-m", "--middle"):
+        middle_mode = True
 
 c_file = output_dir + prefix + c_file
 h_file = output_dir + prefix + h_file
@@ -370,14 +415,20 @@ if dispatch_type == "sync":
         ret = generate_command_decl(cmd['command'], arglist, ret_type) + "\n"
         fdecl.write(ret)
         if ret_type:
-            ret = gen_marshal_output(cmd['command'], arglist, ret_type) + "\n"
+            ret = gen_marshal_output(cmd['command'], arglist, ret_type, middle_mode) + "\n"
             fdef.write(ret)
-        ret = gen_marshal_input(cmd['command'], arglist, ret_type) + "\n"
+
+        if middle_mode:
+            fdecl.write('%s;\n' % gen_marshal_input_decl(cmd['command'], arglist, ret_type, middle_mode))
+
+        ret = gen_marshal_input(cmd['command'], arglist, ret_type, middle_mode) + "\n"
         fdef.write(ret)
 
     fdecl.write("\n#endif\n");
-    ret = gen_registry(commands)
-    fdef.write(ret)
+
+    if not middle_mode:
+        ret = gen_registry(commands)
+        fdef.write(ret)
 
     fdef.flush()
     fdef.close()
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index cece325..fc0f7af 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -268,3 +268,6 @@ fdecl.write('''
 
 fdecl.flush()
 fdecl.close()
+
+fdef.flush()
+fdef.close()
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 56af232..5299976 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -200,4 +200,6 @@ def basename(filename):
     return filename.split("/")[-1]
 
 def guardname(filename):
-    return filename.replace("/", "_").replace("-", "_").split(".")[0].upper()
+    if filename.startswith('./'):
+        filename = filename[2:]
+    return filename.replace("/", "_").replace("-", "_").split(".")[0].upper() + '_H'
-- 
1.7.4.1

[Qemu-devel] [PATCH 03/14] qapi: use middle mode in QMP server

[Anthony Liguori]

Use the new middle mode within the existing QMP server.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 Makefile         |   11 +++++++++++
 Makefile.objs    |    2 ++
 Makefile.target  |    6 +++---
 monitor.c        |   11 ++++++++---
 qapi-schema.json |    3 +++
 5 files changed, 27 insertions(+), 6 deletions(-)
 create mode 100644 qapi-schema.json

diff --git a/Makefile b/Makefile
index 8606849..23ee7e0 100644
--- a/Makefile
+++ b/Makefile
@@ -184,9 +184,20 @@ $(qapi-dir)/qga-qapi-types.h: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scr
 $(qapi-dir)/qga-qapi-visit.c: $(qapi-dir)/qga-qapi-visit.h
 $(qapi-dir)/qga-qapi-visit.h: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-visit.py
 	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py -o "$(qapi-dir)" -p "qga-" < $<, "  GEN   $@")
+$(qapi-dir)/qga-qmp-commands.h: $(qapi-dir)/qga-qmp-marshal.c
 $(qapi-dir)/qga-qmp-marshal.c: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-commands.py
 	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py -o "$(qapi-dir)" -p "qga-" < $<, "  GEN   $@")
 
+qapi-types.c: qapi-types.h
+qapi-types.h: $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py
+	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py -o "." < $<, "  GEN   $@")
+qapi-visit.c: qapi-visit.h
+qapi-visit.h: $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py
+	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py -o "."  < $<, "  GEN   $@")
+qmp-commands.h: qmp-marshal.c
+qmp-marshal.c: $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py
+	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py -m -o "." < $<, "  GEN   $@")
+
 test-visitor.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h) $(qapi-obj-y)
 test-visitor: test-visitor.o qfloat.o qint.o qdict.o qstring.o qlist.o qbool.o $(qapi-obj-y) error.o osdep.o $(oslib-obj-y) qjson.o json-streamer.o json-lexer.o json-parser.o qerror.o qemu-error.o qemu-tool.o $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o
 
diff --git a/Makefile.objs b/Makefile.objs
index d1f3e5d..c02431f 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -396,6 +396,8 @@ qapi-nested-y = qapi-visit-core.o qmp-input-visitor.o qmp-output-visitor.o qapi-
 qapi-nested-y += qmp-registry.o qmp-dispatch.o
 qapi-obj-y = $(addprefix qapi/, $(qapi-nested-y))
 
+common-obj-y += qmp-marshal.o qapi-visit.o qapi-types.o $(qapi-obj-y)
+
 ######################################################################
 # guest agent
 
diff --git a/Makefile.target b/Makefile.target
index e280bf6..2cd0ec5 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -375,7 +375,7 @@ obj-alpha-y += vga.o cirrus_vga.o
 
 main.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
 
-monitor.o: hmp-commands.h qmp-commands.h
+monitor.o: hmp-commands.h qmp-commands-old.h
 
 $(obj-y) $(obj-$(TARGET_BASE_ARCH)-y): $(GENERATED_HEADERS)
 
@@ -405,13 +405,13 @@ gdbstub-xml.c: $(TARGET_XML_FILES) $(SRC_PATH)/scripts/feature_to_c.sh
 hmp-commands.h: $(SRC_PATH)/hmp-commands.hx
 	$(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h < $< > $@,"  GEN   $(TARGET_DIR)$@")
 
-qmp-commands.h: $(SRC_PATH)/qmp-commands.hx
+qmp-commands-old.h: $(SRC_PATH)/qmp-commands.hx
 	$(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h < $< > $@,"  GEN   $(TARGET_DIR)$@")
 
 clean:
 	rm -f *.o *.a *~ $(PROGS) nwfpe/*.o fpu/*.o
 	rm -f *.d */*.d tcg/*.o ide/*.o 9pfs/*.o
-	rm -f hmp-commands.h qmp-commands.h gdbstub-xml.c
+	rm -f hmp-commands.h qmp-commands-old.h gdbstub-xml.c
 ifdef CONFIG_SYSTEMTAP_TRACE
 	rm -f *.stp
 endif
diff --git a/monitor.c b/monitor.c
index ada51d0..ef204c0 100644
--- a/monitor.c
+++ b/monitor.c
@@ -119,6 +119,7 @@ typedef struct mon_cmd_t {
         int  (*cmd_async)(Monitor *mon, const QDict *params,
                           MonitorCompletion *cb, void *opaque);
     } mhandler;
+    bool qapi;
     int flags;
 } mon_cmd_t;
 
@@ -3157,7 +3158,7 @@ static const mon_cmd_t info_cmds[] = {
 };
 
 static const mon_cmd_t qmp_cmds[] = {
-#include "qmp-commands.h"
+#include "qmp-commands-old.h"
     { /* NULL */ },
 };
 
@@ -5027,10 +5028,14 @@ static void qmp_call_query_cmd(Monitor *mon, const mon_cmd_t *cmd)
         if (monitor_has_error(mon)) {
             monitor_protocol_emitter(mon, NULL);
         }
-    } else {
-        cmd->mhandler.info_new(mon, &ret_data);
+    } else if (cmd->qapi) {
+        QDict *args = qdict_new();
+
+        cmd->mhandler.cmd_new(mon, args, &ret_data);
         monitor_protocol_emitter(mon, ret_data);
         qobject_decref(ret_data);
+
+        QDECREF(args);
     }
 }
 
diff --git a/qapi-schema.json b/qapi-schema.json
new file mode 100644
index 0000000..7fcefdb
--- /dev/null
+++ b/qapi-schema.json
@@ -0,0 +1,3 @@
+# -*- Mode: Python -*-
+#
+# QAPI Schema
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 03/14] qapi: use middle mode in QMP server

[Luiz Capitulino]

On Wed, 24 Aug 2011 13:42:58 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> Use the new middle mode within the existing QMP server.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  Makefile         |   11 +++++++++++
>  Makefile.objs    |    2 ++
>  Makefile.target  |    6 +++---
>  monitor.c        |   11 ++++++++---
>  qapi-schema.json |    3 +++
>  5 files changed, 27 insertions(+), 6 deletions(-)
>  create mode 100644 qapi-schema.json
> 
> diff --git a/Makefile b/Makefile
> index 8606849..23ee7e0 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -184,9 +184,20 @@ $(qapi-dir)/qga-qapi-types.h: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scr
>  $(qapi-dir)/qga-qapi-visit.c: $(qapi-dir)/qga-qapi-visit.h
>  $(qapi-dir)/qga-qapi-visit.h: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-visit.py
>  	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py -o "$(qapi-dir)" -p "qga-" < $<, "  GEN   $@")
> +$(qapi-dir)/qga-qmp-commands.h: $(qapi-dir)/qga-qmp-marshal.c
>  $(qapi-dir)/qga-qmp-marshal.c: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-commands.py
>  	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py -o "$(qapi-dir)" -p "qga-" < $<, "  GEN   $@")
>  
> +qapi-types.c: qapi-types.h
> +qapi-types.h: $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py
> +	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py -o "." < $<, "  GEN   $@")
> +qapi-visit.c: qapi-visit.h
> +qapi-visit.h: $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py
> +	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py -o "."  < $<, "  GEN   $@")
> +qmp-commands.h: qmp-marshal.c
> +qmp-marshal.c: $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py
> +	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py -m -o "." < $<, "  GEN   $@")
> +
>  test-visitor.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h) $(qapi-obj-y)
>  test-visitor: test-visitor.o qfloat.o qint.o qdict.o qstring.o qlist.o qbool.o $(qapi-obj-y) error.o osdep.o $(oslib-obj-y) qjson.o json-streamer.o json-lexer.o json-parser.o qerror.o qemu-error.o qemu-tool.o $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o
>  
> diff --git a/Makefile.objs b/Makefile.objs
> index d1f3e5d..c02431f 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -396,6 +396,8 @@ qapi-nested-y = qapi-visit-core.o qmp-input-visitor.o qmp-output-visitor.o qapi-
>  qapi-nested-y += qmp-registry.o qmp-dispatch.o
>  qapi-obj-y = $(addprefix qapi/, $(qapi-nested-y))
>  
> +common-obj-y += qmp-marshal.o qapi-visit.o qapi-types.o $(qapi-obj-y)
> +
>  ######################################################################
>  # guest agent
>  
> diff --git a/Makefile.target b/Makefile.target
> index e280bf6..2cd0ec5 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -375,7 +375,7 @@ obj-alpha-y += vga.o cirrus_vga.o
>  
>  main.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
>  
> -monitor.o: hmp-commands.h qmp-commands.h
> +monitor.o: hmp-commands.h qmp-commands-old.h
>  
>  $(obj-y) $(obj-$(TARGET_BASE_ARCH)-y): $(GENERATED_HEADERS)
>  
> @@ -405,13 +405,13 @@ gdbstub-xml.c: $(TARGET_XML_FILES) $(SRC_PATH)/scripts/feature_to_c.sh
>  hmp-commands.h: $(SRC_PATH)/hmp-commands.hx
>  	$(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h < $< > $@,"  GEN   $(TARGET_DIR)$@")
>  
> -qmp-commands.h: $(SRC_PATH)/qmp-commands.hx
> +qmp-commands-old.h: $(SRC_PATH)/qmp-commands.hx
>  	$(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h < $< > $@,"  GEN   $(TARGET_DIR)$@")
>  
>  clean:
>  	rm -f *.o *.a *~ $(PROGS) nwfpe/*.o fpu/*.o
>  	rm -f *.d */*.d tcg/*.o ide/*.o 9pfs/*.o
> -	rm -f hmp-commands.h qmp-commands.h gdbstub-xml.c
> +	rm -f hmp-commands.h qmp-commands-old.h gdbstub-xml.c
>  ifdef CONFIG_SYSTEMTAP_TRACE
>  	rm -f *.stp
>  endif
> diff --git a/monitor.c b/monitor.c
> index ada51d0..ef204c0 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -119,6 +119,7 @@ typedef struct mon_cmd_t {
>          int  (*cmd_async)(Monitor *mon, const QDict *params,
>                            MonitorCompletion *cb, void *opaque);
>      } mhandler;
> +    bool qapi;
>      int flags;
>  } mon_cmd_t;
>  
> @@ -3157,7 +3158,7 @@ static const mon_cmd_t info_cmds[] = {
>  };
>  
>  static const mon_cmd_t qmp_cmds[] = {
> -#include "qmp-commands.h"
> +#include "qmp-commands-old.h"
>      { /* NULL */ },
>  };
>  
> @@ -5027,10 +5028,14 @@ static void qmp_call_query_cmd(Monitor *mon, const mon_cmd_t *cmd)
>          if (monitor_has_error(mon)) {
>              monitor_protocol_emitter(mon, NULL);
>          }
> -    } else {
> -        cmd->mhandler.info_new(mon, &ret_data);

This breaks query- commands not converted to the QAPI yet.

> +    } else if (cmd->qapi) {
> +        QDict *args = qdict_new();
> +
> +        cmd->mhandler.cmd_new(mon, args, &ret_data);
>          monitor_protocol_emitter(mon, ret_data);
>          qobject_decref(ret_data);
> +
> +        QDECREF(args);
>      }

I think it would also be a good idea to start moving query- commands from
qmp_query_cmds[] to qmp_cmds[]. So that we have only one dispatch table.

If we do this, we don't need the hunk above. We need this:

diff --git a/monitor.c b/monitor.c
index 6a3a3d2..01e141c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -5070,12 +5070,10 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
         goto err_out;
     }
 
-    if (strstart(cmd_name, "query-", &query_cmd)) {
+    cmd = qmp_find_cmd(cmd_name);
+    if (!cmd && strstart(cmd_name, "query-", &query_cmd)) {
         cmd = qmp_find_query_cmd(query_cmd);
-    } else {
-        cmd = qmp_find_cmd(cmd_name);
     }
-
     if (!cmd) {
         qerror_report(QERR_COMMAND_NOT_FOUND, cmd_name);
         goto err_out;

and name the command appropriately in the qmp-commands.hx struct, like
"query-name".

>  }
>  
> diff --git a/qapi-schema.json b/qapi-schema.json
> new file mode 100644
> index 0000000..7fcefdb
> --- /dev/null
> +++ b/qapi-schema.json
> @@ -0,0 +1,3 @@
> +# -*- Mode: Python -*-
> +#
> +# QAPI Schema

Re: [Qemu-devel] [PATCH 03/14] qapi: use middle mode in QMP server

[Anthony Liguori]

On 08/24/2011 03:20 PM, Luiz Capitulino wrote:
> On Wed, 24 Aug 2011 13:42:58 -0500
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>
>> Use the new middle mode within the existing QMP server.
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>> ---
>>   Makefile         |   11 +++++++++++
>>   Makefile.objs    |    2 ++
>>   Makefile.target  |    6 +++---
>>   monitor.c        |   11 ++++++++---
>>   qapi-schema.json |    3 +++
>>   5 files changed, 27 insertions(+), 6 deletions(-)
>>   create mode 100644 qapi-schema.json
>>
>> diff --git a/Makefile b/Makefile
>> index 8606849..23ee7e0 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -184,9 +184,20 @@ $(qapi-dir)/qga-qapi-types.h: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scr
>>   $(qapi-dir)/qga-qapi-visit.c: $(qapi-dir)/qga-qapi-visit.h
>>   $(qapi-dir)/qga-qapi-visit.h: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-visit.py
>>   	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py -o "$(qapi-dir)" -p "qga-"<  $<, "  GEN   $@")
>> +$(qapi-dir)/qga-qmp-commands.h: $(qapi-dir)/qga-qmp-marshal.c
>>   $(qapi-dir)/qga-qmp-marshal.c: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-commands.py
>>   	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py -o "$(qapi-dir)" -p "qga-"<  $<, "  GEN   $@")
>>
>> +qapi-types.c: qapi-types.h
>> +qapi-types.h: $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py
>> +	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py -o "."<  $<, "  GEN   $@")
>> +qapi-visit.c: qapi-visit.h
>> +qapi-visit.h: $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py
>> +	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py -o "."<  $<, "  GEN   $@")
>> +qmp-commands.h: qmp-marshal.c
>> +qmp-marshal.c: $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py
>> +	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py -m -o "."<  $<, "  GEN   $@")
>> +
>>   test-visitor.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h) $(qapi-obj-y)
>>   test-visitor: test-visitor.o qfloat.o qint.o qdict.o qstring.o qlist.o qbool.o $(qapi-obj-y) error.o osdep.o $(oslib-obj-y) qjson.o json-streamer.o json-lexer.o json-parser.o qerror.o qemu-error.o qemu-tool.o $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index d1f3e5d..c02431f 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -396,6 +396,8 @@ qapi-nested-y = qapi-visit-core.o qmp-input-visitor.o qmp-output-visitor.o qapi-
>>   qapi-nested-y += qmp-registry.o qmp-dispatch.o
>>   qapi-obj-y = $(addprefix qapi/, $(qapi-nested-y))
>>
>> +common-obj-y += qmp-marshal.o qapi-visit.o qapi-types.o $(qapi-obj-y)
>> +
>>   ######################################################################
>>   # guest agent
>>
>> diff --git a/Makefile.target b/Makefile.target
>> index e280bf6..2cd0ec5 100644
>> --- a/Makefile.target
>> +++ b/Makefile.target
>> @@ -375,7 +375,7 @@ obj-alpha-y += vga.o cirrus_vga.o
>>
>>   main.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
>>
>> -monitor.o: hmp-commands.h qmp-commands.h
>> +monitor.o: hmp-commands.h qmp-commands-old.h
>>
>>   $(obj-y) $(obj-$(TARGET_BASE_ARCH)-y): $(GENERATED_HEADERS)
>>
>> @@ -405,13 +405,13 @@ gdbstub-xml.c: $(TARGET_XML_FILES) $(SRC_PATH)/scripts/feature_to_c.sh
>>   hmp-commands.h: $(SRC_PATH)/hmp-commands.hx
>>   	$(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h<  $<  >  $@,"  GEN   $(TARGET_DIR)$@")
>>
>> -qmp-commands.h: $(SRC_PATH)/qmp-commands.hx
>> +qmp-commands-old.h: $(SRC_PATH)/qmp-commands.hx
>>   	$(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h<  $<  >  $@,"  GEN   $(TARGET_DIR)$@")
>>
>>   clean:
>>   	rm -f *.o *.a *~ $(PROGS) nwfpe/*.o fpu/*.o
>>   	rm -f *.d */*.d tcg/*.o ide/*.o 9pfs/*.o
>> -	rm -f hmp-commands.h qmp-commands.h gdbstub-xml.c
>> +	rm -f hmp-commands.h qmp-commands-old.h gdbstub-xml.c
>>   ifdef CONFIG_SYSTEMTAP_TRACE
>>   	rm -f *.stp
>>   endif
>> diff --git a/monitor.c b/monitor.c
>> index ada51d0..ef204c0 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -119,6 +119,7 @@ typedef struct mon_cmd_t {
>>           int  (*cmd_async)(Monitor *mon, const QDict *params,
>>                             MonitorCompletion *cb, void *opaque);
>>       } mhandler;
>> +    bool qapi;
>>       int flags;
>>   } mon_cmd_t;
>>
>> @@ -3157,7 +3158,7 @@ static const mon_cmd_t info_cmds[] = {
>>   };
>>
>>   static const mon_cmd_t qmp_cmds[] = {
>> -#include "qmp-commands.h"
>> +#include "qmp-commands-old.h"
>>       { /* NULL */ },
>>   };
>>
>> @@ -5027,10 +5028,14 @@ static void qmp_call_query_cmd(Monitor *mon, const mon_cmd_t *cmd)
>>           if (monitor_has_error(mon)) {
>>               monitor_protocol_emitter(mon, NULL);
>>           }
>> -    } else {
>> -        cmd->mhandler.info_new(mon,&ret_data);
>
> This breaks query- commands not converted to the QAPI yet.

Doh!  Good catch.

>
>> +    } else if (cmd->qapi) {
>> +        QDict *args = qdict_new();
>> +
>> +        cmd->mhandler.cmd_new(mon, args,&ret_data);
>>           monitor_protocol_emitter(mon, ret_data);
>>           qobject_decref(ret_data);
>> +
>> +        QDECREF(args);
>>       }
>
> I think it would also be a good idea to start moving query- commands from
> qmp_query_cmds[] to qmp_cmds[]. So that we have only one dispatch table.
>
> If we do this, we don't need the hunk above. We need this:
>
> diff --git a/monitor.c b/monitor.c
> index 6a3a3d2..01e141c 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -5070,12 +5070,10 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>           goto err_out;
>       }
>
> -    if (strstart(cmd_name, "query-",&query_cmd)) {
> +    cmd = qmp_find_cmd(cmd_name);
> +    if (!cmd&&  strstart(cmd_name, "query-",&query_cmd)) {
>           cmd = qmp_find_query_cmd(query_cmd);
> -    } else {
> -        cmd = qmp_find_cmd(cmd_name);
>       }
> -
>       if (!cmd) {
>           qerror_report(QERR_COMMAND_NOT_FOUND, cmd_name);
>           goto err_out;
>
> and name the command appropriately in the qmp-commands.hx struct, like
> "query-name".

Agreed.  I'll update for v2.

Regards,

Anthony Liguori

>
>>   }
>>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> new file mode 100644
>> index 0000000..7fcefdb
>> --- /dev/null
>> +++ b/qapi-schema.json
>> @@ -0,0 +1,3 @@
>> +# -*- Mode: Python -*-
>> +#
>> +# QAPI Schema
>
>

Re: [Qemu-devel] [PATCH 03/14] qapi: use middle mode in QMP server

[Michael Roth]

On 08/24/2011 01:42 PM, Anthony Liguori wrote:
> Use the new middle mode within the existing QMP server.
>
> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
> ---
>   Makefile         |   11 +++++++++++
>   Makefile.objs    |    2 ++
>   Makefile.target  |    6 +++---
>   monitor.c        |   11 ++++++++---
>   qapi-schema.json |    3 +++
>   5 files changed, 27 insertions(+), 6 deletions(-)
>   create mode 100644 qapi-schema.json
>
> diff --git a/Makefile b/Makefile
> index 8606849..23ee7e0 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -184,9 +184,20 @@ $(qapi-dir)/qga-qapi-types.h: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scr
>   $(qapi-dir)/qga-qapi-visit.c: $(qapi-dir)/qga-qapi-visit.h
>   $(qapi-dir)/qga-qapi-visit.h: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-visit.py
>   	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py -o "$(qapi-dir)" -p "qga-"<  $<, "  GEN   $@")
> +$(qapi-dir)/qga-qmp-commands.h: $(qapi-dir)/qga-qmp-marshal.c
>   $(qapi-dir)/qga-qmp-marshal.c: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-commands.py
>   	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py -o "$(qapi-dir)" -p "qga-"<  $<, "  GEN   $@")
>
> +qapi-types.c: qapi-types.h
> +qapi-types.h: $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py
> +	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py -o "."<  $<, "  GEN   $@")
> +qapi-visit.c: qapi-visit.h
> +qapi-visit.h: $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py
> +	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py -o "."<  $<, "  GEN   $@")
> +qmp-commands.h: qmp-marshal.c
> +qmp-marshal.c: $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py
> +	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py -m -o "."<  $<, "  GEN   $@")
> +
>   test-visitor.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h) $(qapi-obj-y)
>   test-visitor: test-visitor.o qfloat.o qint.o qdict.o qstring.o qlist.o qbool.o $(qapi-obj-y) error.o osdep.o $(oslib-obj-y) qjson.o json-streamer.o json-lexer.o json-parser.o qerror.o qemu-error.o qemu-tool.o $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o
>
> diff --git a/Makefile.objs b/Makefile.objs
> index d1f3e5d..c02431f 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -396,6 +396,8 @@ qapi-nested-y = qapi-visit-core.o qmp-input-visitor.o qmp-output-visitor.o qapi-
>   qapi-nested-y += qmp-registry.o qmp-dispatch.o
>   qapi-obj-y = $(addprefix qapi/, $(qapi-nested-y))
>
> +common-obj-y += qmp-marshal.o qapi-visit.o qapi-types.o $(qapi-obj-y)
> +
>   ######################################################################
>   # guest agent
>
> diff --git a/Makefile.target b/Makefile.target
> index e280bf6..2cd0ec5 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -375,7 +375,7 @@ obj-alpha-y += vga.o cirrus_vga.o
>
>   main.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
>
> -monitor.o: hmp-commands.h qmp-commands.h
> +monitor.o: hmp-commands.h qmp-commands-old.h
>
>   $(obj-y) $(obj-$(TARGET_BASE_ARCH)-y): $(GENERATED_HEADERS)
>
> @@ -405,13 +405,13 @@ gdbstub-xml.c: $(TARGET_XML_FILES) $(SRC_PATH)/scripts/feature_to_c.sh
>   hmp-commands.h: $(SRC_PATH)/hmp-commands.hx
>   	$(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h<  $<  >  $@,"  GEN   $(TARGET_DIR)$@")
>
> -qmp-commands.h: $(SRC_PATH)/qmp-commands.hx
> +qmp-commands-old.h: $(SRC_PATH)/qmp-commands.hx
>   	$(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h<  $<  >  $@,"  GEN   $(TARGET_DIR)$@")
>
>   clean:
>   	rm -f *.o *.a *~ $(PROGS) nwfpe/*.o fpu/*.o
>   	rm -f *.d */*.d tcg/*.o ide/*.o 9pfs/*.o
> -	rm -f hmp-commands.h qmp-commands.h gdbstub-xml.c
> +	rm -f hmp-commands.h qmp-commands-old.h gdbstub-xml.c
>   ifdef CONFIG_SYSTEMTAP_TRACE
>   	rm -f *.stp
>   endif
> diff --git a/monitor.c b/monitor.c
> index ada51d0..ef204c0 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -119,6 +119,7 @@ typedef struct mon_cmd_t {
>           int  (*cmd_async)(Monitor *mon, const QDict *params,
>                             MonitorCompletion *cb, void *opaque);
>       } mhandler;
> +    bool qapi;
>       int flags;
>   } mon_cmd_t;
>
> @@ -3157,7 +3158,7 @@ static const mon_cmd_t info_cmds[] = {
>   };
>
>   static const mon_cmd_t qmp_cmds[] = {
> -#include "qmp-commands.h"
> +#include "qmp-commands-old.h"
>       { /* NULL */ },
>   };
>
> @@ -5027,10 +5028,14 @@ static void qmp_call_query_cmd(Monitor *mon, const mon_cmd_t *cmd)
>           if (monitor_has_error(mon)) {
>               monitor_protocol_emitter(mon, NULL);
>           }
> -    } else {
> -        cmd->mhandler.info_new(mon,&ret_data);
> +    } else if (cmd->qapi) {
> +        QDict *args = qdict_new();
> +
> +        cmd->mhandler.cmd_new(mon, args,&ret_data);
>           monitor_protocol_emitter(mon, ret_data);
>           qobject_decref(ret_data);
> +
> +        QDECREF(args);
>       }
>   }
>

If we instead use the new dispatch stuff in qapi/qmp-dispatch.c, and use 
that dispatch table as a fallback for qmp_find_command(), I think we 
could drop most of the code generator additions in patch #2.

We'd also be able to completely remove any old-style definitions in 
qmp-commands.hx, so once the conversion was complete we'd basically have 
an empty file.

The only downside I can think of would be that documentation for 
commands would be split into 2 locations until the conversion was complete.

> diff --git a/qapi-schema.json b/qapi-schema.json
> new file mode 100644
> index 0000000..7fcefdb
> --- /dev/null
> +++ b/qapi-schema.json
> @@ -0,0 +1,3 @@
> +# -*- Mode: Python -*-
> +#
> +# QAPI Schema

Re: [Qemu-devel] [PATCH 03/14] qapi: use middle mode in QMP server

[Luiz Capitulino]

On Thu, 25 Aug 2011 11:24:04 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On 08/24/2011 01:42 PM, Anthony Liguori wrote:
> > Use the new middle mode within the existing QMP server.
> >
> > Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
> > ---
> >   Makefile         |   11 +++++++++++
> >   Makefile.objs    |    2 ++
> >   Makefile.target  |    6 +++---
> >   monitor.c        |   11 ++++++++---
> >   qapi-schema.json |    3 +++
> >   5 files changed, 27 insertions(+), 6 deletions(-)
> >   create mode 100644 qapi-schema.json
> >
> > diff --git a/Makefile b/Makefile
> > index 8606849..23ee7e0 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -184,9 +184,20 @@ $(qapi-dir)/qga-qapi-types.h: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scr
> >   $(qapi-dir)/qga-qapi-visit.c: $(qapi-dir)/qga-qapi-visit.h
> >   $(qapi-dir)/qga-qapi-visit.h: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-visit.py
> >   	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py -o "$(qapi-dir)" -p "qga-"<  $<, "  GEN   $@")
> > +$(qapi-dir)/qga-qmp-commands.h: $(qapi-dir)/qga-qmp-marshal.c
> >   $(qapi-dir)/qga-qmp-marshal.c: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-commands.py
> >   	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py -o "$(qapi-dir)" -p "qga-"<  $<, "  GEN   $@")
> >
> > +qapi-types.c: qapi-types.h
> > +qapi-types.h: $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py
> > +	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py -o "."<  $<, "  GEN   $@")
> > +qapi-visit.c: qapi-visit.h
> > +qapi-visit.h: $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py
> > +	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py -o "."<  $<, "  GEN   $@")
> > +qmp-commands.h: qmp-marshal.c
> > +qmp-marshal.c: $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py
> > +	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py -m -o "."<  $<, "  GEN   $@")
> > +
> >   test-visitor.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h) $(qapi-obj-y)
> >   test-visitor: test-visitor.o qfloat.o qint.o qdict.o qstring.o qlist.o qbool.o $(qapi-obj-y) error.o osdep.o $(oslib-obj-y) qjson.o json-streamer.o json-lexer.o json-parser.o qerror.o qemu-error.o qemu-tool.o $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o
> >
> > diff --git a/Makefile.objs b/Makefile.objs
> > index d1f3e5d..c02431f 100644
> > --- a/Makefile.objs
> > +++ b/Makefile.objs
> > @@ -396,6 +396,8 @@ qapi-nested-y = qapi-visit-core.o qmp-input-visitor.o qmp-output-visitor.o qapi-
> >   qapi-nested-y += qmp-registry.o qmp-dispatch.o
> >   qapi-obj-y = $(addprefix qapi/, $(qapi-nested-y))
> >
> > +common-obj-y += qmp-marshal.o qapi-visit.o qapi-types.o $(qapi-obj-y)
> > +
> >   ######################################################################
> >   # guest agent
> >
> > diff --git a/Makefile.target b/Makefile.target
> > index e280bf6..2cd0ec5 100644
> > --- a/Makefile.target
> > +++ b/Makefile.target
> > @@ -375,7 +375,7 @@ obj-alpha-y += vga.o cirrus_vga.o
> >
> >   main.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
> >
> > -monitor.o: hmp-commands.h qmp-commands.h
> > +monitor.o: hmp-commands.h qmp-commands-old.h
> >
> >   $(obj-y) $(obj-$(TARGET_BASE_ARCH)-y): $(GENERATED_HEADERS)
> >
> > @@ -405,13 +405,13 @@ gdbstub-xml.c: $(TARGET_XML_FILES) $(SRC_PATH)/scripts/feature_to_c.sh
> >   hmp-commands.h: $(SRC_PATH)/hmp-commands.hx
> >   	$(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h<  $<  >  $@,"  GEN   $(TARGET_DIR)$@")
> >
> > -qmp-commands.h: $(SRC_PATH)/qmp-commands.hx
> > +qmp-commands-old.h: $(SRC_PATH)/qmp-commands.hx
> >   	$(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h<  $<  >  $@,"  GEN   $(TARGET_DIR)$@")
> >
> >   clean:
> >   	rm -f *.o *.a *~ $(PROGS) nwfpe/*.o fpu/*.o
> >   	rm -f *.d */*.d tcg/*.o ide/*.o 9pfs/*.o
> > -	rm -f hmp-commands.h qmp-commands.h gdbstub-xml.c
> > +	rm -f hmp-commands.h qmp-commands-old.h gdbstub-xml.c
> >   ifdef CONFIG_SYSTEMTAP_TRACE
> >   	rm -f *.stp
> >   endif
> > diff --git a/monitor.c b/monitor.c
> > index ada51d0..ef204c0 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -119,6 +119,7 @@ typedef struct mon_cmd_t {
> >           int  (*cmd_async)(Monitor *mon, const QDict *params,
> >                             MonitorCompletion *cb, void *opaque);
> >       } mhandler;
> > +    bool qapi;
> >       int flags;
> >   } mon_cmd_t;
> >
> > @@ -3157,7 +3158,7 @@ static const mon_cmd_t info_cmds[] = {
> >   };
> >
> >   static const mon_cmd_t qmp_cmds[] = {
> > -#include "qmp-commands.h"
> > +#include "qmp-commands-old.h"
> >       { /* NULL */ },
> >   };
> >
> > @@ -5027,10 +5028,14 @@ static void qmp_call_query_cmd(Monitor *mon, const mon_cmd_t *cmd)
> >           if (monitor_has_error(mon)) {
> >               monitor_protocol_emitter(mon, NULL);
> >           }
> > -    } else {
> > -        cmd->mhandler.info_new(mon,&ret_data);
> > +    } else if (cmd->qapi) {
> > +        QDict *args = qdict_new();
> > +
> > +        cmd->mhandler.cmd_new(mon, args,&ret_data);
> >           monitor_protocol_emitter(mon, ret_data);
> >           qobject_decref(ret_data);
> > +
> > +        QDECREF(args);
> >       }
> >   }
> >
> 
> If we instead use the new dispatch stuff in qapi/qmp-dispatch.c, and use 
> that dispatch table as a fallback for qmp_find_command(), I think we 
> could drop most of the code generator additions in patch #2.

Seems like a good idea.

> We'd also be able to completely remove any old-style definitions in 
> qmp-commands.hx, so once the conversion was complete we'd basically have 
> an empty file.
> 
> The only downside I can think of would be that documentation for 
> commands would be split into 2 locations until the conversion was complete.

Yes, I've pointed this out too, but thinking again about this, maybe we
can live with it for a while as we're going to do a full conversion for
the next version anyway.

> 
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > new file mode 100644
> > index 0000000..7fcefdb
> > --- /dev/null
> > +++ b/qapi-schema.json
> > @@ -0,0 +1,3 @@
> > +# -*- Mode: Python -*-
> > +#
> > +# QAPI Schema
> 

Re: [Qemu-devel] [PATCH 03/14] qapi: use middle mode in QMP server

[Anthony Liguori]

On 08/25/2011 11:24 AM, Michael Roth wrote:
> On 08/24/2011 01:42 PM, Anthony Liguori wrote:
>> Use the new middle mode within the existing QMP server.
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>> ---
>> Makefile | 11 +++++++++++
>> Makefile.objs | 2 ++
>> Makefile.target | 6 +++---
>> monitor.c | 11 ++++++++---
>> qapi-schema.json | 3 +++
>> 5 files changed, 27 insertions(+), 6 deletions(-)
>> create mode 100644 qapi-schema.json
>>
>> diff --git a/Makefile b/Makefile
>> index 8606849..23ee7e0 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -184,9 +184,20 @@ $(qapi-dir)/qga-qapi-types.h:
>> $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scr
>> $(qapi-dir)/qga-qapi-visit.c: $(qapi-dir)/qga-qapi-visit.h
>> $(qapi-dir)/qga-qapi-visit.h: $(SRC_PATH)/qapi-schema-guest.json
>> $(SRC_PATH)/scripts/qapi-visit.py
>> $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py -o
>> "$(qapi-dir)" -p "qga-"< $<, " GEN $@")
>> +$(qapi-dir)/qga-qmp-commands.h: $(qapi-dir)/qga-qmp-marshal.c
>> $(qapi-dir)/qga-qmp-marshal.c: $(SRC_PATH)/qapi-schema-guest.json
>> $(SRC_PATH)/scripts/qapi-commands.py
>> $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py -o
>> "$(qapi-dir)" -p "qga-"< $<, " GEN $@")
>>
>> +qapi-types.c: qapi-types.h
>> +qapi-types.h: $(SRC_PATH)/qapi-schema.json
>> $(SRC_PATH)/scripts/qapi-types.py
>> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py -o
>> "."< $<, " GEN $@")
>> +qapi-visit.c: qapi-visit.h
>> +qapi-visit.h: $(SRC_PATH)/qapi-schema.json
>> $(SRC_PATH)/scripts/qapi-visit.py
>> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py -o
>> "."< $<, " GEN $@")
>> +qmp-commands.h: qmp-marshal.c
>> +qmp-marshal.c: $(SRC_PATH)/qapi-schema.json
>> $(SRC_PATH)/scripts/qapi-commands.py
>> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py
>> -m -o "."< $<, " GEN $@")
>> +
>> test-visitor.o: $(addprefix $(qapi-dir)/, test-qapi-types.c
>> test-qapi-types.h test-qapi-visit.c test-qapi-visit.h) $(qapi-obj-y)
>> test-visitor: test-visitor.o qfloat.o qint.o qdict.o qstring.o qlist.o
>> qbool.o $(qapi-obj-y) error.o osdep.o $(oslib-obj-y) qjson.o
>> json-streamer.o json-lexer.o json-parser.o qerror.o qemu-error.o
>> qemu-tool.o $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index d1f3e5d..c02431f 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -396,6 +396,8 @@ qapi-nested-y = qapi-visit-core.o
>> qmp-input-visitor.o qmp-output-visitor.o qapi-
>> qapi-nested-y += qmp-registry.o qmp-dispatch.o
>> qapi-obj-y = $(addprefix qapi/, $(qapi-nested-y))
>>
>> +common-obj-y += qmp-marshal.o qapi-visit.o qapi-types.o $(qapi-obj-y)
>> +
>> ######################################################################
>> # guest agent
>>
>> diff --git a/Makefile.target b/Makefile.target
>> index e280bf6..2cd0ec5 100644
>> --- a/Makefile.target
>> +++ b/Makefile.target
>> @@ -375,7 +375,7 @@ obj-alpha-y += vga.o cirrus_vga.o
>>
>> main.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
>>
>> -monitor.o: hmp-commands.h qmp-commands.h
>> +monitor.o: hmp-commands.h qmp-commands-old.h
>>
>> $(obj-y) $(obj-$(TARGET_BASE_ARCH)-y): $(GENERATED_HEADERS)
>>
>> @@ -405,13 +405,13 @@ gdbstub-xml.c: $(TARGET_XML_FILES)
>> $(SRC_PATH)/scripts/feature_to_c.sh
>> hmp-commands.h: $(SRC_PATH)/hmp-commands.hx
>> $(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h< $< > $@," GEN
>> $(TARGET_DIR)$@")
>>
>> -qmp-commands.h: $(SRC_PATH)/qmp-commands.hx
>> +qmp-commands-old.h: $(SRC_PATH)/qmp-commands.hx
>> $(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h< $< > $@," GEN
>> $(TARGET_DIR)$@")
>>
>> clean:
>> rm -f *.o *.a *~ $(PROGS) nwfpe/*.o fpu/*.o
>> rm -f *.d */*.d tcg/*.o ide/*.o 9pfs/*.o
>> - rm -f hmp-commands.h qmp-commands.h gdbstub-xml.c
>> + rm -f hmp-commands.h qmp-commands-old.h gdbstub-xml.c
>> ifdef CONFIG_SYSTEMTAP_TRACE
>> rm -f *.stp
>> endif
>> diff --git a/monitor.c b/monitor.c
>> index ada51d0..ef204c0 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -119,6 +119,7 @@ typedef struct mon_cmd_t {
>> int (*cmd_async)(Monitor *mon, const QDict *params,
>> MonitorCompletion *cb, void *opaque);
>> } mhandler;
>> + bool qapi;
>> int flags;
>> } mon_cmd_t;
>>
>> @@ -3157,7 +3158,7 @@ static const mon_cmd_t info_cmds[] = {
>> };
>>
>> static const mon_cmd_t qmp_cmds[] = {
>> -#include "qmp-commands.h"
>> +#include "qmp-commands-old.h"
>> { /* NULL */ },
>> };
>>
>> @@ -5027,10 +5028,14 @@ static void qmp_call_query_cmd(Monitor *mon,
>> const mon_cmd_t *cmd)
>> if (monitor_has_error(mon)) {
>> monitor_protocol_emitter(mon, NULL);
>> }
>> - } else {
>> - cmd->mhandler.info_new(mon,&ret_data);
>> + } else if (cmd->qapi) {
>> + QDict *args = qdict_new();
>> +
>> + cmd->mhandler.cmd_new(mon, args,&ret_data);
>> monitor_protocol_emitter(mon, ret_data);
>> qobject_decref(ret_data);
>> +
>> + QDECREF(args);
>> }
>> }
>>
>
> If we instead use the new dispatch stuff in qapi/qmp-dispatch.c, and use
> that dispatch table as a fallback for qmp_find_command(), I think we
> could drop most of the code generator additions in patch #2.
>
> We'd also be able to completely remove any old-style definitions in
> qmp-commands.hx, so once the conversion was complete we'd basically have
> an empty file.
>
> The only downside I can think of would be that documentation for
> commands would be split into 2 locations until the conversion was complete.

This ended up being a bit harder than I initially thought so I'd prefer 
to delay this to another series so we can start converting commands to 
QAPI ASAP.

Regards,

Anthony Liguori

>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> new file mode 100644
>> index 0000000..7fcefdb
>> --- /dev/null
>> +++ b/qapi-schema.json
>> @@ -0,0 +1,3 @@
>> +# -*- Mode: Python -*-
>> +#
>> +# QAPI Schema
>
>

Re: [Qemu-devel] [PATCH 03/14] qapi: use middle mode in QMP server

[Luiz Capitulino]

On Fri, 02 Sep 2011 11:00:50 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 08/25/2011 11:24 AM, Michael Roth wrote:
> > On 08/24/2011 01:42 PM, Anthony Liguori wrote:
> >> Use the new middle mode within the existing QMP server.
> >>
> >> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
> >> ---
> >> Makefile | 11 +++++++++++
> >> Makefile.objs | 2 ++
> >> Makefile.target | 6 +++---
> >> monitor.c | 11 ++++++++---
> >> qapi-schema.json | 3 +++
> >> 5 files changed, 27 insertions(+), 6 deletions(-)
> >> create mode 100644 qapi-schema.json
> >>
> >> diff --git a/Makefile b/Makefile
> >> index 8606849..23ee7e0 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -184,9 +184,20 @@ $(qapi-dir)/qga-qapi-types.h:
> >> $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scr
> >> $(qapi-dir)/qga-qapi-visit.c: $(qapi-dir)/qga-qapi-visit.h
> >> $(qapi-dir)/qga-qapi-visit.h: $(SRC_PATH)/qapi-schema-guest.json
> >> $(SRC_PATH)/scripts/qapi-visit.py
> >> $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py -o
> >> "$(qapi-dir)" -p "qga-"< $<, " GEN $@")
> >> +$(qapi-dir)/qga-qmp-commands.h: $(qapi-dir)/qga-qmp-marshal.c
> >> $(qapi-dir)/qga-qmp-marshal.c: $(SRC_PATH)/qapi-schema-guest.json
> >> $(SRC_PATH)/scripts/qapi-commands.py
> >> $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py -o
> >> "$(qapi-dir)" -p "qga-"< $<, " GEN $@")
> >>
> >> +qapi-types.c: qapi-types.h
> >> +qapi-types.h: $(SRC_PATH)/qapi-schema.json
> >> $(SRC_PATH)/scripts/qapi-types.py
> >> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py -o
> >> "."< $<, " GEN $@")
> >> +qapi-visit.c: qapi-visit.h
> >> +qapi-visit.h: $(SRC_PATH)/qapi-schema.json
> >> $(SRC_PATH)/scripts/qapi-visit.py
> >> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py -o
> >> "."< $<, " GEN $@")
> >> +qmp-commands.h: qmp-marshal.c
> >> +qmp-marshal.c: $(SRC_PATH)/qapi-schema.json
> >> $(SRC_PATH)/scripts/qapi-commands.py
> >> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py
> >> -m -o "."< $<, " GEN $@")
> >> +
> >> test-visitor.o: $(addprefix $(qapi-dir)/, test-qapi-types.c
> >> test-qapi-types.h test-qapi-visit.c test-qapi-visit.h) $(qapi-obj-y)
> >> test-visitor: test-visitor.o qfloat.o qint.o qdict.o qstring.o qlist.o
> >> qbool.o $(qapi-obj-y) error.o osdep.o $(oslib-obj-y) qjson.o
> >> json-streamer.o json-lexer.o json-parser.o qerror.o qemu-error.o
> >> qemu-tool.o $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o
> >>
> >> diff --git a/Makefile.objs b/Makefile.objs
> >> index d1f3e5d..c02431f 100644
> >> --- a/Makefile.objs
> >> +++ b/Makefile.objs
> >> @@ -396,6 +396,8 @@ qapi-nested-y = qapi-visit-core.o
> >> qmp-input-visitor.o qmp-output-visitor.o qapi-
> >> qapi-nested-y += qmp-registry.o qmp-dispatch.o
> >> qapi-obj-y = $(addprefix qapi/, $(qapi-nested-y))
> >>
> >> +common-obj-y += qmp-marshal.o qapi-visit.o qapi-types.o $(qapi-obj-y)
> >> +
> >> ######################################################################
> >> # guest agent
> >>
> >> diff --git a/Makefile.target b/Makefile.target
> >> index e280bf6..2cd0ec5 100644
> >> --- a/Makefile.target
> >> +++ b/Makefile.target
> >> @@ -375,7 +375,7 @@ obj-alpha-y += vga.o cirrus_vga.o
> >>
> >> main.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
> >>
> >> -monitor.o: hmp-commands.h qmp-commands.h
> >> +monitor.o: hmp-commands.h qmp-commands-old.h
> >>
> >> $(obj-y) $(obj-$(TARGET_BASE_ARCH)-y): $(GENERATED_HEADERS)
> >>
> >> @@ -405,13 +405,13 @@ gdbstub-xml.c: $(TARGET_XML_FILES)
> >> $(SRC_PATH)/scripts/feature_to_c.sh
> >> hmp-commands.h: $(SRC_PATH)/hmp-commands.hx
> >> $(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h< $< > $@," GEN
> >> $(TARGET_DIR)$@")
> >>
> >> -qmp-commands.h: $(SRC_PATH)/qmp-commands.hx
> >> +qmp-commands-old.h: $(SRC_PATH)/qmp-commands.hx
> >> $(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h< $< > $@," GEN
> >> $(TARGET_DIR)$@")
> >>
> >> clean:
> >> rm -f *.o *.a *~ $(PROGS) nwfpe/*.o fpu/*.o
> >> rm -f *.d */*.d tcg/*.o ide/*.o 9pfs/*.o
> >> - rm -f hmp-commands.h qmp-commands.h gdbstub-xml.c
> >> + rm -f hmp-commands.h qmp-commands-old.h gdbstub-xml.c
> >> ifdef CONFIG_SYSTEMTAP_TRACE
> >> rm -f *.stp
> >> endif
> >> diff --git a/monitor.c b/monitor.c
> >> index ada51d0..ef204c0 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -119,6 +119,7 @@ typedef struct mon_cmd_t {
> >> int (*cmd_async)(Monitor *mon, const QDict *params,
> >> MonitorCompletion *cb, void *opaque);
> >> } mhandler;
> >> + bool qapi;
> >> int flags;
> >> } mon_cmd_t;
> >>
> >> @@ -3157,7 +3158,7 @@ static const mon_cmd_t info_cmds[] = {
> >> };
> >>
> >> static const mon_cmd_t qmp_cmds[] = {
> >> -#include "qmp-commands.h"
> >> +#include "qmp-commands-old.h"
> >> { /* NULL */ },
> >> };
> >>
> >> @@ -5027,10 +5028,14 @@ static void qmp_call_query_cmd(Monitor *mon,
> >> const mon_cmd_t *cmd)
> >> if (monitor_has_error(mon)) {
> >> monitor_protocol_emitter(mon, NULL);
> >> }
> >> - } else {
> >> - cmd->mhandler.info_new(mon,&ret_data);
> >> + } else if (cmd->qapi) {
> >> + QDict *args = qdict_new();
> >> +
> >> + cmd->mhandler.cmd_new(mon, args,&ret_data);
> >> monitor_protocol_emitter(mon, ret_data);
> >> qobject_decref(ret_data);
> >> +
> >> + QDECREF(args);
> >> }
> >> }
> >>
> >
> > If we instead use the new dispatch stuff in qapi/qmp-dispatch.c, and use
> > that dispatch table as a fallback for qmp_find_command(), I think we
> > could drop most of the code generator additions in patch #2.
> >
> > We'd also be able to completely remove any old-style definitions in
> > qmp-commands.hx, so once the conversion was complete we'd basically have
> > an empty file.
> >
> > The only downside I can think of would be that documentation for
> > commands would be split into 2 locations until the conversion was complete.
> 
> This ended up being a bit harder than I initially thought so I'd prefer 
> to delay this to another series so we can start converting commands to 
> QAPI ASAP.

Fine with me.

Re: [Qemu-devel] [PATCH 03/14] qapi: use middle mode in QMP server

[Michael Roth]

On 09/02/2011 11:09 AM, Luiz Capitulino wrote:
> On Fri, 02 Sep 2011 11:00:50 -0500
> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>
>> On 08/25/2011 11:24 AM, Michael Roth wrote:
>>> On 08/24/2011 01:42 PM, Anthony Liguori wrote:
>>>> Use the new middle mode within the existing QMP server.
>>>>
>>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>>> ---
>>>> Makefile | 11 +++++++++++
>>>> Makefile.objs | 2 ++
>>>> Makefile.target | 6 +++---
>>>> monitor.c | 11 ++++++++---
>>>> qapi-schema.json | 3 +++
>>>> 5 files changed, 27 insertions(+), 6 deletions(-)
>>>> create mode 100644 qapi-schema.json
>>>>
>>>> diff --git a/Makefile b/Makefile
>>>> index 8606849..23ee7e0 100644
>>>> --- a/Makefile
>>>> +++ b/Makefile
>>>> @@ -184,9 +184,20 @@ $(qapi-dir)/qga-qapi-types.h:
>>>> $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scr
>>>> $(qapi-dir)/qga-qapi-visit.c: $(qapi-dir)/qga-qapi-visit.h
>>>> $(qapi-dir)/qga-qapi-visit.h: $(SRC_PATH)/qapi-schema-guest.json
>>>> $(SRC_PATH)/scripts/qapi-visit.py
>>>> $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py -o
>>>> "$(qapi-dir)" -p "qga-"<  $<, " GEN $@")
>>>> +$(qapi-dir)/qga-qmp-commands.h: $(qapi-dir)/qga-qmp-marshal.c
>>>> $(qapi-dir)/qga-qmp-marshal.c: $(SRC_PATH)/qapi-schema-guest.json
>>>> $(SRC_PATH)/scripts/qapi-commands.py
>>>> $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py -o
>>>> "$(qapi-dir)" -p "qga-"<  $<, " GEN $@")
>>>>
>>>> +qapi-types.c: qapi-types.h
>>>> +qapi-types.h: $(SRC_PATH)/qapi-schema.json
>>>> $(SRC_PATH)/scripts/qapi-types.py
>>>> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py -o
>>>> "."<  $<, " GEN $@")
>>>> +qapi-visit.c: qapi-visit.h
>>>> +qapi-visit.h: $(SRC_PATH)/qapi-schema.json
>>>> $(SRC_PATH)/scripts/qapi-visit.py
>>>> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py -o
>>>> "."<  $<, " GEN $@")
>>>> +qmp-commands.h: qmp-marshal.c
>>>> +qmp-marshal.c: $(SRC_PATH)/qapi-schema.json
>>>> $(SRC_PATH)/scripts/qapi-commands.py
>>>> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py
>>>> -m -o "."<  $<, " GEN $@")
>>>> +
>>>> test-visitor.o: $(addprefix $(qapi-dir)/, test-qapi-types.c
>>>> test-qapi-types.h test-qapi-visit.c test-qapi-visit.h) $(qapi-obj-y)
>>>> test-visitor: test-visitor.o qfloat.o qint.o qdict.o qstring.o qlist.o
>>>> qbool.o $(qapi-obj-y) error.o osdep.o $(oslib-obj-y) qjson.o
>>>> json-streamer.o json-lexer.o json-parser.o qerror.o qemu-error.o
>>>> qemu-tool.o $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o
>>>>
>>>> diff --git a/Makefile.objs b/Makefile.objs
>>>> index d1f3e5d..c02431f 100644
>>>> --- a/Makefile.objs
>>>> +++ b/Makefile.objs
>>>> @@ -396,6 +396,8 @@ qapi-nested-y = qapi-visit-core.o
>>>> qmp-input-visitor.o qmp-output-visitor.o qapi-
>>>> qapi-nested-y += qmp-registry.o qmp-dispatch.o
>>>> qapi-obj-y = $(addprefix qapi/, $(qapi-nested-y))
>>>>
>>>> +common-obj-y += qmp-marshal.o qapi-visit.o qapi-types.o $(qapi-obj-y)
>>>> +
>>>> ######################################################################
>>>> # guest agent
>>>>
>>>> diff --git a/Makefile.target b/Makefile.target
>>>> index e280bf6..2cd0ec5 100644
>>>> --- a/Makefile.target
>>>> +++ b/Makefile.target
>>>> @@ -375,7 +375,7 @@ obj-alpha-y += vga.o cirrus_vga.o
>>>>
>>>> main.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
>>>>
>>>> -monitor.o: hmp-commands.h qmp-commands.h
>>>> +monitor.o: hmp-commands.h qmp-commands-old.h
>>>>
>>>> $(obj-y) $(obj-$(TARGET_BASE_ARCH)-y): $(GENERATED_HEADERS)
>>>>
>>>> @@ -405,13 +405,13 @@ gdbstub-xml.c: $(TARGET_XML_FILES)
>>>> $(SRC_PATH)/scripts/feature_to_c.sh
>>>> hmp-commands.h: $(SRC_PATH)/hmp-commands.hx
>>>> $(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h<  $<  >  $@," GEN
>>>> $(TARGET_DIR)$@")
>>>>
>>>> -qmp-commands.h: $(SRC_PATH)/qmp-commands.hx
>>>> +qmp-commands-old.h: $(SRC_PATH)/qmp-commands.hx
>>>> $(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h<  $<  >  $@," GEN
>>>> $(TARGET_DIR)$@")
>>>>
>>>> clean:
>>>> rm -f *.o *.a *~ $(PROGS) nwfpe/*.o fpu/*.o
>>>> rm -f *.d */*.d tcg/*.o ide/*.o 9pfs/*.o
>>>> - rm -f hmp-commands.h qmp-commands.h gdbstub-xml.c
>>>> + rm -f hmp-commands.h qmp-commands-old.h gdbstub-xml.c
>>>> ifdef CONFIG_SYSTEMTAP_TRACE
>>>> rm -f *.stp
>>>> endif
>>>> diff --git a/monitor.c b/monitor.c
>>>> index ada51d0..ef204c0 100644
>>>> --- a/monitor.c
>>>> +++ b/monitor.c
>>>> @@ -119,6 +119,7 @@ typedef struct mon_cmd_t {
>>>> int (*cmd_async)(Monitor *mon, const QDict *params,
>>>> MonitorCompletion *cb, void *opaque);
>>>> } mhandler;
>>>> + bool qapi;
>>>> int flags;
>>>> } mon_cmd_t;
>>>>
>>>> @@ -3157,7 +3158,7 @@ static const mon_cmd_t info_cmds[] = {
>>>> };
>>>>
>>>> static const mon_cmd_t qmp_cmds[] = {
>>>> -#include "qmp-commands.h"
>>>> +#include "qmp-commands-old.h"
>>>> { /* NULL */ },
>>>> };
>>>>
>>>> @@ -5027,10 +5028,14 @@ static void qmp_call_query_cmd(Monitor *mon,
>>>> const mon_cmd_t *cmd)
>>>> if (monitor_has_error(mon)) {
>>>> monitor_protocol_emitter(mon, NULL);
>>>> }
>>>> - } else {
>>>> - cmd->mhandler.info_new(mon,&ret_data);
>>>> + } else if (cmd->qapi) {
>>>> + QDict *args = qdict_new();
>>>> +
>>>> + cmd->mhandler.cmd_new(mon, args,&ret_data);
>>>> monitor_protocol_emitter(mon, ret_data);
>>>> qobject_decref(ret_data);
>>>> +
>>>> + QDECREF(args);
>>>> }
>>>> }
>>>>
>>>
>>> If we instead use the new dispatch stuff in qapi/qmp-dispatch.c, and use
>>> that dispatch table as a fallback for qmp_find_command(), I think we
>>> could drop most of the code generator additions in patch #2.
>>>
>>> We'd also be able to completely remove any old-style definitions in
>>> qmp-commands.hx, so once the conversion was complete we'd basically have
>>> an empty file.
>>>
>>> The only downside I can think of would be that documentation for
>>> commands would be split into 2 locations until the conversion was complete.
>>
>> This ended up being a bit harder than I initially thought so I'd prefer
>> to delay this to another series so we can start converting commands to
>> QAPI ASAP.
>
> Fine with me.

Same. Curious what the complications are though, I wouldn't mind looking 
at it.

Re: [Qemu-devel] [PATCH 03/14] qapi: use middle mode in QMP server

[Anthony Liguori]

On 09/02/2011 11:31 AM, Michael Roth wrote:

>>> This ended up being a bit harder than I initially thought so I'd prefer
>>> to delay this to another series so we can start converting commands to
>>> QAPI ASAP.
>>
>> Fine with me.
>
> Same. Curious what the complications are though, I wouldn't mind looking
> at it.

The current dispatch and error checking logic in the QMP server scares 
me.  It wasn't immediately obvious if just adding another else clause in 
the dispatch logic was enough.

I could be wrong though.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 03/14] qapi: use middle mode in QMP server

[Luiz Capitulino]

On Fri, 02 Sep 2011 11:45:28 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> On 09/02/2011 11:31 AM, Michael Roth wrote:
> 
> >>> This ended up being a bit harder than I initially thought so I'd prefer
> >>> to delay this to another series so we can start converting commands to
> >>> QAPI ASAP.
> >>
> >> Fine with me.
> >
> > Same. Curious what the complications are though, I wouldn't mind looking
> > at it.
> 
> The current dispatch and error checking logic in the QMP server scares 
> me.  It wasn't immediately obvious if just adding another else clause in 
> the dispatch logic was enough.

It inherited HMP's distinction between "info" and "regular" commands plus
the arg_type thing.

IMO, the best thing to do is to let it alone for now, do the QAPI conversion
and then drop it in favor on the new server.

[Qemu-devel] [PATCH 04/14] qapi: convert query-name

[Anthony Liguori]

A simple example conversion 'info name'.  This also adds the new files for
QMP and HMP.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 Makefile.objs    |    1 +
 hmp.c            |   26 ++++++++++++++++++++++++++
 hmp.h            |   22 ++++++++++++++++++++++
 monitor.c        |   27 +++++----------------------
 qapi-schema.json |   23 +++++++++++++++++++++++
 qmp.c            |   28 ++++++++++++++++++++++++++++
 6 files changed, 105 insertions(+), 22 deletions(-)
 create mode 100644 hmp.c
 create mode 100644 hmp.h
 create mode 100644 qmp.c

diff --git a/Makefile.objs b/Makefile.objs
index c02431f..570dda7 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -397,6 +397,7 @@ qapi-nested-y += qmp-registry.o qmp-dispatch.o
 qapi-obj-y = $(addprefix qapi/, $(qapi-nested-y))
 
 common-obj-y += qmp-marshal.o qapi-visit.o qapi-types.o $(qapi-obj-y)
+common-obj-y += qmp.o hmp.o
 
 ######################################################################
 # guest agent
diff --git a/hmp.c b/hmp.c
new file mode 100644
index 0000000..47e1ff7
--- /dev/null
+++ b/hmp.c
@@ -0,0 +1,26 @@
+/*
+ * Human Monitor Interface
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#include "hmp.h"
+#include "qmp-commands.h"
+
+void hmp_info_name(Monitor *mon)
+{
+    NameInfo *info;
+
+    info = qmp_query_name(NULL);
+    if (info->has_name) {
+        monitor_printf(mon, "%s\n", info->name);
+    }
+    qapi_free_NameInfo(info);
+}
diff --git a/hmp.h b/hmp.h
new file mode 100644
index 0000000..5fe73f1
--- /dev/null
+++ b/hmp.h
@@ -0,0 +1,22 @@
+/*
+ * Human Monitor Interface
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef HMP_H
+#define HMP_H
+
+#include "qemu-common.h"
+#include "qapi-types.h"
+
+void hmp_info_name(Monitor *mon);
+
+#endif
diff --git a/monitor.c b/monitor.c
index ef204c0..6a3a3d2 100644
--- a/monitor.c
+++ b/monitor.c
@@ -61,6 +61,8 @@
 #include "trace.h"
 #endif
 #include "ui/qemu-spice.h"
+#include "qmp-commands.h"
+#include "hmp.h"
 
 //#define DEBUG
 //#define DEBUG_COMPLETION
@@ -757,24 +759,6 @@ static void do_info_version(Monitor *mon, QObject **ret_data)
         'micro': %d }, 'package': %s }", major, minor, micro, QEMU_PKGVERSION);
 }
 
-static void do_info_name_print(Monitor *mon, const QObject *data)
-{
-    QDict *qdict;
-
-    qdict = qobject_to_qdict(data);
-    if (qdict_size(qdict) == 0) {
-        return;
-    }
-
-    monitor_printf(mon, "%s\n", qdict_get_str(qdict, "name"));
-}
-
-static void do_info_name(Monitor *mon, QObject **ret_data)
-{
-    *ret_data = qemu_name ? qobject_from_jsonf("{'name': %s }", qemu_name) :
-                            qobject_from_jsonf("{}");
-}
-
 static QObject *get_cmd_dict(const char *name)
 {
     const char *p;
@@ -3069,8 +3053,7 @@ static const mon_cmd_t info_cmds[] = {
         .args_type  = "",
         .params     = "",
         .help       = "show the current VM name",
-        .user_print = do_info_name_print,
-        .mhandler.info_new = do_info_name,
+        .mhandler.info = hmp_info_name,
     },
     {
         .name       = "uuid",
@@ -3266,8 +3249,8 @@ static const mon_cmd_t qmp_query_cmds[] = {
         .args_type  = "",
         .params     = "",
         .help       = "show the current VM name",
-        .user_print = do_info_name_print,
-        .mhandler.info_new = do_info_name,
+        .mhandler.cmd_new = qmp_marshal_input_query_name,
+        .qapi       = true,
     },
     {
         .name       = "uuid",
diff --git a/qapi-schema.json b/qapi-schema.json
index 7fcefdb..654409b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1,3 +1,26 @@
 # -*- Mode: Python -*-
 #
 # QAPI Schema
+
+##
+# @NameInfo:
+#
+# Guest name information.
+#
+# @name: #optional The name of the guest
+#
+# Since 0.14.0
+##
+{ 'type': 'NameInfo', 'data': {'*name': 'str'} }
+
+##
+# @query-name:
+#
+# Return the name information of a guest.
+#
+# Returns: @NameInfo of the guest
+#
+# Since 0.14.0
+##
+{ 'command': 'query-name', 'returns': 'NameInfo' }
+
diff --git a/qmp.c b/qmp.c
new file mode 100644
index 0000000..8aa9c66
--- /dev/null
+++ b/qmp.c
@@ -0,0 +1,28 @@
+/*
+ * QEMU Management Protocol
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu-common.h"
+#include "sysemu.h"
+#include "qmp-commands.h"
+
+NameInfo *qmp_query_name(Error **errp)
+{
+    NameInfo *info = g_malloc0(sizeof(*info));
+
+    if (qemu_name) {
+        info->has_name = true;
+        info->name = g_strdup(qemu_name);
+    }
+
+    return info;
+}
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 04/14] qapi: convert query-name

[Luiz Capitulino]

On Wed, 24 Aug 2011 13:42:59 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> A simple example conversion 'info name'.  This also adds the new files for
> QMP and HMP.

The QAPI makes QMP commands real good!

(more comments below).

> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  Makefile.objs    |    1 +
>  hmp.c            |   26 ++++++++++++++++++++++++++
>  hmp.h            |   22 ++++++++++++++++++++++
>  monitor.c        |   27 +++++----------------------
>  qapi-schema.json |   23 +++++++++++++++++++++++
>  qmp.c            |   28 ++++++++++++++++++++++++++++
>  6 files changed, 105 insertions(+), 22 deletions(-)
>  create mode 100644 hmp.c
>  create mode 100644 hmp.h
>  create mode 100644 qmp.c
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index c02431f..570dda7 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -397,6 +397,7 @@ qapi-nested-y += qmp-registry.o qmp-dispatch.o
>  qapi-obj-y = $(addprefix qapi/, $(qapi-nested-y))
>  
>  common-obj-y += qmp-marshal.o qapi-visit.o qapi-types.o $(qapi-obj-y)
> +common-obj-y += qmp.o hmp.o
>  
>  ######################################################################
>  # guest agent
> diff --git a/hmp.c b/hmp.c
> new file mode 100644
> index 0000000..47e1ff7
> --- /dev/null
> +++ b/hmp.c
> @@ -0,0 +1,26 @@
> +/*
> + * Human Monitor Interface
> + *
> + * Copyright IBM, Corp. 2011
> + *
> + * Authors:
> + *  Anthony Liguori   <aliguori@us.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "hmp.h"
> +#include "qmp-commands.h"
> +
> +void hmp_info_name(Monitor *mon)
> +{
> +    NameInfo *info;
> +
> +    info = qmp_query_name(NULL);
> +    if (info->has_name) {
> +        monitor_printf(mon, "%s\n", info->name);
> +    }
> +    qapi_free_NameInfo(info);
> +}
> diff --git a/hmp.h b/hmp.h
> new file mode 100644
> index 0000000..5fe73f1
> --- /dev/null
> +++ b/hmp.h
> @@ -0,0 +1,22 @@
> +/*
> + * Human Monitor Interface
> + *
> + * Copyright IBM, Corp. 2011
> + *
> + * Authors:
> + *  Anthony Liguori   <aliguori@us.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +
> +#ifndef HMP_H
> +#define HMP_H
> +
> +#include "qemu-common.h"
> +#include "qapi-types.h"
> +
> +void hmp_info_name(Monitor *mon);
> +
> +#endif
> diff --git a/monitor.c b/monitor.c
> index ef204c0..6a3a3d2 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -61,6 +61,8 @@
>  #include "trace.h"
>  #endif
>  #include "ui/qemu-spice.h"
> +#include "qmp-commands.h"
> +#include "hmp.h"
>  
>  //#define DEBUG
>  //#define DEBUG_COMPLETION
> @@ -757,24 +759,6 @@ static void do_info_version(Monitor *mon, QObject **ret_data)
>          'micro': %d }, 'package': %s }", major, minor, micro, QEMU_PKGVERSION);
>  }
>  
> -static void do_info_name_print(Monitor *mon, const QObject *data)
> -{
> -    QDict *qdict;
> -
> -    qdict = qobject_to_qdict(data);
> -    if (qdict_size(qdict) == 0) {
> -        return;
> -    }
> -
> -    monitor_printf(mon, "%s\n", qdict_get_str(qdict, "name"));
> -}
> -
> -static void do_info_name(Monitor *mon, QObject **ret_data)
> -{
> -    *ret_data = qemu_name ? qobject_from_jsonf("{'name': %s }", qemu_name) :
> -                            qobject_from_jsonf("{}");
> -}
> -
>  static QObject *get_cmd_dict(const char *name)
>  {
>      const char *p;
> @@ -3069,8 +3053,7 @@ static const mon_cmd_t info_cmds[] = {
>          .args_type  = "",
>          .params     = "",
>          .help       = "show the current VM name",
> -        .user_print = do_info_name_print,
> -        .mhandler.info_new = do_info_name,
> +        .mhandler.info = hmp_info_name,
>      },
>      {
>          .name       = "uuid",
> @@ -3266,8 +3249,8 @@ static const mon_cmd_t qmp_query_cmds[] = {
>          .args_type  = "",
>          .params     = "",
>          .help       = "show the current VM name",
> -        .user_print = do_info_name_print,
> -        .mhandler.info_new = do_info_name,
> +        .mhandler.cmd_new = qmp_marshal_input_query_name,
> +        .qapi       = true,
>      },
>      {
>          .name       = "uuid",
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 7fcefdb..654409b 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1,3 +1,26 @@
>  # -*- Mode: Python -*-
>  #
>  # QAPI Schema
> +
> +##
> +# @NameInfo:
> +#
> +# Guest name information.
> +#
> +# @name: #optional The name of the guest

Isn't it the VM name?

> +#
> +# Since 0.14.0
> +##
> +{ 'type': 'NameInfo', 'data': {'*name': 'str'} }

Is the type name ('NameInfo' in this case) going to be public in libqmp? If yes,
Isn't VmNameInfo better?

> +
> +##
> +# @query-name:
> +#
> +# Return the name information of a guest.
> +#
> +# Returns: @NameInfo of the guest
> +#
> +# Since 0.14.0
> +##
> +{ 'command': 'query-name', 'returns': 'NameInfo' }
> +
> diff --git a/qmp.c b/qmp.c
> new file mode 100644
> index 0000000..8aa9c66
> --- /dev/null
> +++ b/qmp.c
> @@ -0,0 +1,28 @@
> +/*
> + * QEMU Management Protocol
> + *
> + * Copyright IBM, Corp. 2011
> + *
> + * Authors:
> + *  Anthony Liguori   <aliguori@us.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu-common.h"
> +#include "sysemu.h"
> +#include "qmp-commands.h"
> +
> +NameInfo *qmp_query_name(Error **errp)
> +{
> +    NameInfo *info = g_malloc0(sizeof(*info));
> +
> +    if (qemu_name) {
> +        info->has_name = true;
> +        info->name = g_strdup(qemu_name);
> +    }

Isn't it better to keep using qemu_malloc() & friends but change its
current implementation to use the glib malloc functions?



> +
> +    return info;
> +}

Re: [Qemu-devel] [PATCH 04/14] qapi: convert query-name

[Anthony Liguori]

On 08/24/2011 03:28 PM, Luiz Capitulino wrote:
> On Wed, 24 Aug 2011 13:42:59 -0500
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>
>> A simple example conversion 'info name'.  This also adds the new files for
>> QMP and HMP.
>
> The QAPI makes QMP commands real good!
>
> (more comments below).

Thanks :-)

>
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>> ---
>>   Makefile.objs    |    1 +
>>   hmp.c            |   26 ++++++++++++++++++++++++++
>>   hmp.h            |   22 ++++++++++++++++++++++
>>   monitor.c        |   27 +++++----------------------
>>   qapi-schema.json |   23 +++++++++++++++++++++++
>>   qmp.c            |   28 ++++++++++++++++++++++++++++
>>   6 files changed, 105 insertions(+), 22 deletions(-)
>>   create mode 100644 hmp.c
>>   create mode 100644 hmp.h
>>   create mode 100644 qmp.c
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index c02431f..570dda7 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -397,6 +397,7 @@ qapi-nested-y += qmp-registry.o qmp-dispatch.o
>>   qapi-obj-y = $(addprefix qapi/, $(qapi-nested-y))
>>
>>   common-obj-y += qmp-marshal.o qapi-visit.o qapi-types.o $(qapi-obj-y)
>> +common-obj-y += qmp.o hmp.o
>>
>>   ######################################################################
>>   # guest agent
>> diff --git a/hmp.c b/hmp.c
>> new file mode 100644
>> index 0000000..47e1ff7
>> --- /dev/null
>> +++ b/hmp.c
>> @@ -0,0 +1,26 @@
>> +/*
>> + * Human Monitor Interface
>> + *
>> + * Copyright IBM, Corp. 2011
>> + *
>> + * Authors:
>> + *  Anthony Liguori<aliguori@us.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
>> + * the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#include "hmp.h"
>> +#include "qmp-commands.h"
>> +
>> +void hmp_info_name(Monitor *mon)
>> +{
>> +    NameInfo *info;
>> +
>> +    info = qmp_query_name(NULL);
>> +    if (info->has_name) {
>> +        monitor_printf(mon, "%s\n", info->name);
>> +    }
>> +    qapi_free_NameInfo(info);
>> +}
>> diff --git a/hmp.h b/hmp.h
>> new file mode 100644
>> index 0000000..5fe73f1
>> --- /dev/null
>> +++ b/hmp.h
>> @@ -0,0 +1,22 @@
>> +/*
>> + * Human Monitor Interface
>> + *
>> + * Copyright IBM, Corp. 2011
>> + *
>> + * Authors:
>> + *  Anthony Liguori<aliguori@us.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
>> + * the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#ifndef HMP_H
>> +#define HMP_H
>> +
>> +#include "qemu-common.h"
>> +#include "qapi-types.h"
>> +
>> +void hmp_info_name(Monitor *mon);
>> +
>> +#endif
>> diff --git a/monitor.c b/monitor.c
>> index ef204c0..6a3a3d2 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -61,6 +61,8 @@
>>   #include "trace.h"
>>   #endif
>>   #include "ui/qemu-spice.h"
>> +#include "qmp-commands.h"
>> +#include "hmp.h"
>>
>>   //#define DEBUG
>>   //#define DEBUG_COMPLETION
>> @@ -757,24 +759,6 @@ static void do_info_version(Monitor *mon, QObject **ret_data)
>>           'micro': %d }, 'package': %s }", major, minor, micro, QEMU_PKGVERSION);
>>   }
>>
>> -static void do_info_name_print(Monitor *mon, const QObject *data)
>> -{
>> -    QDict *qdict;
>> -
>> -    qdict = qobject_to_qdict(data);
>> -    if (qdict_size(qdict) == 0) {
>> -        return;
>> -    }
>> -
>> -    monitor_printf(mon, "%s\n", qdict_get_str(qdict, "name"));
>> -}
>> -
>> -static void do_info_name(Monitor *mon, QObject **ret_data)
>> -{
>> -    *ret_data = qemu_name ? qobject_from_jsonf("{'name': %s }", qemu_name) :
>> -                            qobject_from_jsonf("{}");
>> -}
>> -
>>   static QObject *get_cmd_dict(const char *name)
>>   {
>>       const char *p;
>> @@ -3069,8 +3053,7 @@ static const mon_cmd_t info_cmds[] = {
>>           .args_type  = "",
>>           .params     = "",
>>           .help       = "show the current VM name",
>> -        .user_print = do_info_name_print,
>> -        .mhandler.info_new = do_info_name,
>> +        .mhandler.info = hmp_info_name,
>>       },
>>       {
>>           .name       = "uuid",
>> @@ -3266,8 +3249,8 @@ static const mon_cmd_t qmp_query_cmds[] = {
>>           .args_type  = "",
>>           .params     = "",
>>           .help       = "show the current VM name",
>> -        .user_print = do_info_name_print,
>> -        .mhandler.info_new = do_info_name,
>> +        .mhandler.cmd_new = qmp_marshal_input_query_name,
>> +        .qapi       = true,
>>       },
>>       {
>>           .name       = "uuid",
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 7fcefdb..654409b 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -1,3 +1,26 @@
>>   # -*- Mode: Python -*-
>>   #
>>   # QAPI Schema
>> +
>> +##
>> +# @NameInfo:
>> +#
>> +# Guest name information.
>> +#
>> +# @name: #optional The name of the guest
>
> Isn't it the VM name?

VM and guest are synonyms, no?  I think guest sounds a bit more polite :-)

>> +#
>> +# Since 0.14.0
>> +##
>> +{ 'type': 'NameInfo', 'data': {'*name': 'str'} }
>
> Is the type name ('NameInfo' in this case) going to be public in libqmp? If yes,
> Isn't VmNameInfo better?

For libqmp, it would be exposed probably as QmpNameInfo.

But for the most part, the names are mechanically derived from the 
commands for better or worse.

>> +
>> +##
>> +# @query-name:
>> +#
>> +# Return the name information of a guest.
>> +#
>> +# Returns: @NameInfo of the guest
>> +#
>> +# Since 0.14.0
>> +##
>> +{ 'command': 'query-name', 'returns': 'NameInfo' }
>> +
>> diff --git a/qmp.c b/qmp.c
>> new file mode 100644
>> index 0000000..8aa9c66
>> --- /dev/null
>> +++ b/qmp.c
>> @@ -0,0 +1,28 @@
>> +/*
>> + * QEMU Management Protocol
>> + *
>> + * Copyright IBM, Corp. 2011
>> + *
>> + * Authors:
>> + *  Anthony Liguori<aliguori@us.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
>> + * the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#include "qemu-common.h"
>> +#include "sysemu.h"
>> +#include "qmp-commands.h"
>> +
>> +NameInfo *qmp_query_name(Error **errp)
>> +{
>> +    NameInfo *info = g_malloc0(sizeof(*info));
>> +
>> +    if (qemu_name) {
>> +        info->has_name = true;
>> +        info->name = g_strdup(qemu_name);
>> +    }
>
> Isn't it better to keep using qemu_malloc()&  friends but change its
> current implementation to use the glib malloc functions?

You're behind on qemu-devel, qemu_malloc() is no more :-)

Regards,

Anthony Liguori

>
>
>> +
>> +    return info;
>> +}
>
>

Re: [Qemu-devel] [PATCH 04/14] qapi: convert query-name

[Luiz Capitulino]

On Wed, 24 Aug 2011 15:41:43 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 08/24/2011 03:28 PM, Luiz Capitulino wrote:
> > On Wed, 24 Aug 2011 13:42:59 -0500
> > Anthony Liguori<aliguori@us.ibm.com>  wrote:
> >
> >> A simple example conversion 'info name'.  This also adds the new files for
> >> QMP and HMP.
> >
> > The QAPI makes QMP commands real good!
> >
> > (more comments below).
> 
> Thanks :-)
> 
> >
> >>
> >> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
> >> ---
> >>   Makefile.objs    |    1 +
> >>   hmp.c            |   26 ++++++++++++++++++++++++++
> >>   hmp.h            |   22 ++++++++++++++++++++++
> >>   monitor.c        |   27 +++++----------------------
> >>   qapi-schema.json |   23 +++++++++++++++++++++++
> >>   qmp.c            |   28 ++++++++++++++++++++++++++++
> >>   6 files changed, 105 insertions(+), 22 deletions(-)
> >>   create mode 100644 hmp.c
> >>   create mode 100644 hmp.h
> >>   create mode 100644 qmp.c
> >>
> >> diff --git a/Makefile.objs b/Makefile.objs
> >> index c02431f..570dda7 100644
> >> --- a/Makefile.objs
> >> +++ b/Makefile.objs
> >> @@ -397,6 +397,7 @@ qapi-nested-y += qmp-registry.o qmp-dispatch.o
> >>   qapi-obj-y = $(addprefix qapi/, $(qapi-nested-y))
> >>
> >>   common-obj-y += qmp-marshal.o qapi-visit.o qapi-types.o $(qapi-obj-y)
> >> +common-obj-y += qmp.o hmp.o
> >>
> >>   ######################################################################
> >>   # guest agent
> >> diff --git a/hmp.c b/hmp.c
> >> new file mode 100644
> >> index 0000000..47e1ff7
> >> --- /dev/null
> >> +++ b/hmp.c
> >> @@ -0,0 +1,26 @@
> >> +/*
> >> + * Human Monitor Interface
> >> + *
> >> + * Copyright IBM, Corp. 2011
> >> + *
> >> + * Authors:
> >> + *  Anthony Liguori<aliguori@us.ibm.com>
> >> + *
> >> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> >> + * the COPYING file in the top-level directory.
> >> + *
> >> + */
> >> +
> >> +#include "hmp.h"
> >> +#include "qmp-commands.h"
> >> +
> >> +void hmp_info_name(Monitor *mon)
> >> +{
> >> +    NameInfo *info;
> >> +
> >> +    info = qmp_query_name(NULL);
> >> +    if (info->has_name) {
> >> +        monitor_printf(mon, "%s\n", info->name);
> >> +    }
> >> +    qapi_free_NameInfo(info);
> >> +}
> >> diff --git a/hmp.h b/hmp.h
> >> new file mode 100644
> >> index 0000000..5fe73f1
> >> --- /dev/null
> >> +++ b/hmp.h
> >> @@ -0,0 +1,22 @@
> >> +/*
> >> + * Human Monitor Interface
> >> + *
> >> + * Copyright IBM, Corp. 2011
> >> + *
> >> + * Authors:
> >> + *  Anthony Liguori<aliguori@us.ibm.com>
> >> + *
> >> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> >> + * the COPYING file in the top-level directory.
> >> + *
> >> + */
> >> +
> >> +#ifndef HMP_H
> >> +#define HMP_H
> >> +
> >> +#include "qemu-common.h"
> >> +#include "qapi-types.h"
> >> +
> >> +void hmp_info_name(Monitor *mon);
> >> +
> >> +#endif
> >> diff --git a/monitor.c b/monitor.c
> >> index ef204c0..6a3a3d2 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -61,6 +61,8 @@
> >>   #include "trace.h"
> >>   #endif
> >>   #include "ui/qemu-spice.h"
> >> +#include "qmp-commands.h"
> >> +#include "hmp.h"
> >>
> >>   //#define DEBUG
> >>   //#define DEBUG_COMPLETION
> >> @@ -757,24 +759,6 @@ static void do_info_version(Monitor *mon, QObject **ret_data)
> >>           'micro': %d }, 'package': %s }", major, minor, micro, QEMU_PKGVERSION);
> >>   }
> >>
> >> -static void do_info_name_print(Monitor *mon, const QObject *data)
> >> -{
> >> -    QDict *qdict;
> >> -
> >> -    qdict = qobject_to_qdict(data);
> >> -    if (qdict_size(qdict) == 0) {
> >> -        return;
> >> -    }
> >> -
> >> -    monitor_printf(mon, "%s\n", qdict_get_str(qdict, "name"));
> >> -}
> >> -
> >> -static void do_info_name(Monitor *mon, QObject **ret_data)
> >> -{
> >> -    *ret_data = qemu_name ? qobject_from_jsonf("{'name': %s }", qemu_name) :
> >> -                            qobject_from_jsonf("{}");
> >> -}
> >> -
> >>   static QObject *get_cmd_dict(const char *name)
> >>   {
> >>       const char *p;
> >> @@ -3069,8 +3053,7 @@ static const mon_cmd_t info_cmds[] = {
> >>           .args_type  = "",
> >>           .params     = "",
> >>           .help       = "show the current VM name",
> >> -        .user_print = do_info_name_print,
> >> -        .mhandler.info_new = do_info_name,
> >> +        .mhandler.info = hmp_info_name,
> >>       },
> >>       {
> >>           .name       = "uuid",
> >> @@ -3266,8 +3249,8 @@ static const mon_cmd_t qmp_query_cmds[] = {
> >>           .args_type  = "",
> >>           .params     = "",
> >>           .help       = "show the current VM name",
> >> -        .user_print = do_info_name_print,
> >> -        .mhandler.info_new = do_info_name,
> >> +        .mhandler.cmd_new = qmp_marshal_input_query_name,
> >> +        .qapi       = true,
> >>       },
> >>       {
> >>           .name       = "uuid",
> >> diff --git a/qapi-schema.json b/qapi-schema.json
> >> index 7fcefdb..654409b 100644
> >> --- a/qapi-schema.json
> >> +++ b/qapi-schema.json
> >> @@ -1,3 +1,26 @@
> >>   # -*- Mode: Python -*-
> >>   #
> >>   # QAPI Schema
> >> +
> >> +##
> >> +# @NameInfo:
> >> +#
> >> +# Guest name information.
> >> +#
> >> +# @name: #optional The name of the guest
> >
> > Isn't it the VM name?
> 
> VM and guest are synonyms, no?  I think guest sounds a bit more polite :-)

Then we have to fix it in the info command help text too.

> 
> >> +#
> >> +# Since 0.14.0
> >> +##
> >> +{ 'type': 'NameInfo', 'data': {'*name': 'str'} }
> >
> > Is the type name ('NameInfo' in this case) going to be public in libqmp? If yes,
> > Isn't VmNameInfo better?
> 
> For libqmp, it would be exposed probably as QmpNameInfo.
> 
> But for the most part, the names are mechanically derived from the 
> commands for better or worse.

Ok.

> 
> >> +
> >> +##
> >> +# @query-name:
> >> +#
> >> +# Return the name information of a guest.
> >> +#
> >> +# Returns: @NameInfo of the guest
> >> +#
> >> +# Since 0.14.0
> >> +##
> >> +{ 'command': 'query-name', 'returns': 'NameInfo' }
> >> +
> >> diff --git a/qmp.c b/qmp.c
> >> new file mode 100644
> >> index 0000000..8aa9c66
> >> --- /dev/null
> >> +++ b/qmp.c
> >> @@ -0,0 +1,28 @@
> >> +/*
> >> + * QEMU Management Protocol
> >> + *
> >> + * Copyright IBM, Corp. 2011
> >> + *
> >> + * Authors:
> >> + *  Anthony Liguori<aliguori@us.ibm.com>
> >> + *
> >> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> >> + * the COPYING file in the top-level directory.
> >> + *
> >> + */
> >> +
> >> +#include "qemu-common.h"
> >> +#include "sysemu.h"
> >> +#include "qmp-commands.h"
> >> +
> >> +NameInfo *qmp_query_name(Error **errp)
> >> +{
> >> +    NameInfo *info = g_malloc0(sizeof(*info));
> >> +
> >> +    if (qemu_name) {
> >> +        info->has_name = true;
> >> +        info->name = g_strdup(qemu_name);
> >> +    }
> >
> > Isn't it better to keep using qemu_malloc()&  friends but change its
> > current implementation to use the glib malloc functions?
> 
> You're behind on qemu-devel, qemu_malloc() is no more :-)

What a shocking news :)

> 
> Regards,
> 
> Anthony Liguori
> 
> >
> >
> >> +
> >> +    return info;
> >> +}
> >
> >
> 

[Qemu-devel] [PATCH 05/14] block: add unsafe_probe

[Anthony Liguori]

Mark formats that cannot be probed with 100% reliability (just raw).  This is
necessary to implement change-blockdev.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 block/raw.c |    2 ++
 block_int.h |    2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/block/raw.c b/block/raw.c
index 555db4f..5c74bd6 100644
--- a/block/raw.c
+++ b/block/raw.c
@@ -145,6 +145,8 @@ static BlockDriver bdrv_raw = {
     .bdrv_create        = raw_create,
     .create_options     = raw_create_options,
     .bdrv_has_zero_init = raw_has_zero_init,
+
+    .unsafe_probe       = true,
 };
 
 static void bdrv_raw_init(void)
diff --git a/block_int.h b/block_int.h
index f6d02b3..db9ba4b 100644
--- a/block_int.h
+++ b/block_int.h
@@ -145,6 +145,8 @@ struct BlockDriver {
      */
     int (*bdrv_has_zero_init)(BlockDriverState *bs);
 
+    bool unsafe_probe;
+
     QLIST_ENTRY(BlockDriver) list;
 };
 
-- 
1.7.4.1

[Qemu-devel] [PATCH 06/14] monitor: expose readline state

[Anthony Liguori]

HMP is now implemented in terms of QMP.  The monitor has a bunch of logic to
deal with HMP right now like readline support.  Export it from the monitor so
we can consume it in hmp.c.

In short time, hmp.c will take over all of the readline bits.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 monitor.c |   11 ++++++++---
 monitor.h |    5 +++++
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/monitor.c b/monitor.c
index 6a3a3d2..5595565 100644
--- a/monitor.c
+++ b/monitor.c
@@ -221,7 +221,7 @@ int monitor_cur_is_qmp(void)
     return cur_mon && monitor_ctrl_mode(cur_mon);
 }
 
-static void monitor_read_command(Monitor *mon, int show_prompt)
+void monitor_read_command(Monitor *mon, int show_prompt)
 {
     if (!mon->rs)
         return;
@@ -231,8 +231,8 @@ static void monitor_read_command(Monitor *mon, int show_prompt)
         readline_show_prompt(mon->rs);
 }
 
-static int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
-                                 void *opaque)
+int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
+                          void *opaque)
 {
     if (monitor_ctrl_mode(mon)) {
         qerror_report(QERR_MISSING_PARAMETER, "password");
@@ -5301,6 +5301,11 @@ static void bdrv_password_cb(Monitor *mon, const char *password, void *opaque)
     monitor_read_command(mon, 1);
 }
 
+ReadLineState *monitor_get_rs(Monitor *mon)
+{
+    return mon->rs;
+}
+
 int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
                                 BlockDriverCompletionFunc *completion_cb,
                                 void *opaque)
diff --git a/monitor.h b/monitor.h
index 4f2d328..6b2ef77 100644
--- a/monitor.h
+++ b/monitor.h
@@ -6,6 +6,7 @@
 #include "qerror.h"
 #include "qdict.h"
 #include "block.h"
+#include "readline.h"
 
 extern Monitor *cur_mon;
 extern Monitor *default_mon;
@@ -61,5 +62,9 @@ void monitor_flush(Monitor *mon);
 typedef void (MonitorCompletion)(void *opaque, QObject *ret_data);
 
 void monitor_set_error(Monitor *mon, QError *qerror);
+void monitor_read_command(Monitor *mon, int show_prompt);
+ReadLineState *monitor_get_rs(Monitor *mon);
+int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
+                          void *opaque);
 
 #endif /* !MONITOR_H */
-- 
1.7.4.1

[Qemu-devel] [PATCH 07/14] qerror: add additional parameter to QERR_DEVICE_ENCRYPTED

[Anthony Liguori]

Changing an encrypted block device requires a flow of:

 1) change device
 2) receive error from change
 3) block_passwd

DeviceEncrypted receives the device name but with a block backend with multiple
backing files, any one of the backing files could be encrypted.  From a UI
perspective, you have no way of knowing which file is encrypted without parsing
the qcow2 files :-/

Add the actual encrypted filename to the error message so that a UI can display
that information to the user.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 monitor.c |    3 ++-
 qerror.h  |    2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/monitor.c b/monitor.c
index 5595565..5cb36cd 100644
--- a/monitor.c
+++ b/monitor.c
@@ -5319,7 +5319,8 @@ int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
     }
 
     if (monitor_ctrl_mode(mon)) {
-        qerror_report(QERR_DEVICE_ENCRYPTED, bdrv_get_device_name(bs));
+        qerror_report(QERR_DEVICE_ENCRYPTED, bdrv_get_device_name(bs),
+                      bdrv_get_encrypted_filename(bs));
         return -1;
     }
 
diff --git a/qerror.h b/qerror.h
index 4fe24aa..3bbff5c 100644
--- a/qerror.h
+++ b/qerror.h
@@ -64,7 +64,7 @@ QError *qobject_to_qerror(const QObject *obj);
     "{ 'class': 'CommandNotFound', 'data': { 'name': %s } }"
 
 #define QERR_DEVICE_ENCRYPTED \
-    "{ 'class': 'DeviceEncrypted', 'data': { 'device': %s } }"
+    "{ 'class': 'DeviceEncrypted', 'data': { 'device': %s, 'filename': %s} }"
 
 #define QERR_DEVICE_INIT_FAILED \
     "{ 'class': 'DeviceInitFailed', 'data': { 'device': %s } }"
-- 
1.7.4.1

[Qemu-devel] [PATCH 08/14] qapi: convert eject (qmp and hmp) to QAPI

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 blockdev.c       |   22 +++++++++++-----------
 blockdev.h       |    1 -
 hmp-commands.hx  |    3 +--
 hmp.c            |   14 ++++++++++++++
 hmp.h            |    1 +
 qapi-schema.json |   25 +++++++++++++++++++++++++
 qmp-commands.hx  |    3 +--
 7 files changed, 53 insertions(+), 16 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index d272659..6b7fc41 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -16,6 +16,7 @@
 #include "sysemu.h"
 #include "hw/qdev.h"
 #include "block_int.h"
+#include "qmp-commands.h"
 
 static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives);
 
@@ -644,32 +645,31 @@ out:
     return ret;
 }
 
-static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
+static int eject_device(BlockDriverState *bs, int force, Error **errp)
 {
     if (!bdrv_is_removable(bs)) {
-        qerror_report(QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs));
+        error_set(errp, QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs));
         return -1;
     }
     if (!force && bdrv_is_locked(bs)) {
-        qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
+        error_set(errp, QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
         return -1;
     }
     bdrv_close(bs);
     return 0;
 }
 
-int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data)
+void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
 {
     BlockDriverState *bs;
-    int force = qdict_get_try_bool(qdict, "force", 0);
-    const char *filename = qdict_get_str(qdict, "device");
 
-    bs = bdrv_find(filename);
+    bs = bdrv_find(device);
     if (!bs) {
-        qerror_report(QERR_DEVICE_NOT_FOUND, filename);
-        return -1;
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
     }
-    return eject_device(mon, bs, force);
+
+    eject_device(bs, force, errp);
 }
 
 int do_block_set_passwd(Monitor *mon, const QDict *qdict,
@@ -715,7 +715,7 @@ int do_change_block(Monitor *mon, const char *device,
             return -1;
         }
     }
-    if (eject_device(mon, bs, 0) < 0) {
+    if (eject_device(bs, 0, NULL) < 0) {
         return -1;
     }
     bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
diff --git a/blockdev.h b/blockdev.h
index 3587786..badbf01 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -58,7 +58,6 @@ DriveInfo *drive_init(QemuOpts *arg, int default_to_scsi);
 DriveInfo *add_init_drive(const char *opts);
 
 void do_commit(Monitor *mon, const QDict *qdict);
-int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int do_block_set_passwd(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int do_change_block(Monitor *mon, const char *device,
                     const char *filename, const char *fmt);
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 0ccfb28..bcb789b 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -76,8 +76,7 @@ ETEXI
         .args_type  = "force:-f,device:B",
         .params     = "[-f] device",
         .help       = "eject a removable medium (use -f to force it)",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_eject,
+        .mhandler.cmd = hmp_eject,
     },
 
 STEXI
diff --git a/hmp.c b/hmp.c
index 47e1ff7..36eb5b9 100644
--- a/hmp.c
+++ b/hmp.c
@@ -24,3 +24,17 @@ void hmp_info_name(Monitor *mon)
     }
     qapi_free_NameInfo(info);
 }
+
+void hmp_eject(Monitor *mon, const QDict *qdict)
+{
+    int force = qdict_get_try_bool(qdict, "force", 0);
+    const char *device = qdict_get_str(qdict, "device");
+    Error *err = NULL;
+
+    qmp_eject(device, true, force, &err);
+    if (err) {
+        monitor_printf(mon, "eject: %s\n", error_get_pretty(err));
+        error_free(err);
+    }
+}
+
diff --git a/hmp.h b/hmp.h
index 5fe73f1..6a552c1 100644
--- a/hmp.h
+++ b/hmp.h
@@ -18,5 +18,6 @@
 #include "qapi-types.h"
 
 void hmp_info_name(Monitor *mon);
+void hmp_eject(Monitor *mon, const QDict *args);
 
 #endif
diff --git a/qapi-schema.json b/qapi-schema.json
index 654409b..934ea81 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -24,3 +24,28 @@
 ##
 { 'command': 'query-name', 'returns': 'NameInfo' }
 
+##
+# @eject:
+#
+# Ejects a device from a removable drive.
+#
+# @device:  The name of the device
+#
+# @force:   @optional If true, eject regardless of whether the drive is locked.
+#           If not specified, the default value is false.
+#
+# Returns:  Nothing on success
+#           If @device is not a valid block device, DeviceNotFound
+#           If @device is not removable and @force is false, DeviceNotRemovable
+#           If @force is false and @device is locked, DeviceLocked
+#
+# Notes:    If the @force flag is used, the backing file will be closed
+#           regardless of whether the device is removable.  This may result in
+#           a badly broken guest.
+#
+#           Ejecting a device with no media results in success
+#
+# Since: 0.14.0
+##
+{ 'command': 'eject', 'data': {'device': 'str', '*force': 'bool'} }
+
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 03f67da..81d1800 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -89,8 +89,7 @@ EQMP
         .args_type  = "force:-f,device:B",
         .params     = "[-f] device",
         .help       = "eject a removable medium (use -f to force it)",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_eject,
+        .mhandler.cmd_new = qmp_marshal_input_eject,
     },
 
 SQMP
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 08/14] qapi: convert eject (qmp and hmp) to QAPI

[Luiz Capitulino]

On Wed, 24 Aug 2011 13:43:03 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  blockdev.c       |   22 +++++++++++-----------
>  blockdev.h       |    1 -
>  hmp-commands.hx  |    3 +--
>  hmp.c            |   14 ++++++++++++++
>  hmp.h            |    1 +
>  qapi-schema.json |   25 +++++++++++++++++++++++++
>  qmp-commands.hx  |    3 +--
>  7 files changed, 53 insertions(+), 16 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index d272659..6b7fc41 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -16,6 +16,7 @@
>  #include "sysemu.h"
>  #include "hw/qdev.h"
>  #include "block_int.h"
> +#include "qmp-commands.h"
>  
>  static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives);
>  
> @@ -644,32 +645,31 @@ out:
>      return ret;
>  }
>  
> -static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
> +static int eject_device(BlockDriverState *bs, int force, Error **errp)
>  {
>      if (!bdrv_is_removable(bs)) {
> -        qerror_report(QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs));
> +        error_set(errp, QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs));
>          return -1;
>      }
>      if (!force && bdrv_is_locked(bs)) {
> -        qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
> +        error_set(errp, QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
>          return -1;
>      }
>      bdrv_close(bs);
>      return 0;
>  }
>  
> -int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
>  {
>      BlockDriverState *bs;
> -    int force = qdict_get_try_bool(qdict, "force", 0);
> -    const char *filename = qdict_get_str(qdict, "device");
>  
> -    bs = bdrv_find(filename);
> +    bs = bdrv_find(device);
>      if (!bs) {
> -        qerror_report(QERR_DEVICE_NOT_FOUND, filename);
> -        return -1;
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
>      }
> -    return eject_device(mon, bs, force);
> +
> +    eject_device(bs, force, errp);
>  }
>  
>  int do_block_set_passwd(Monitor *mon, const QDict *qdict,
> @@ -715,7 +715,7 @@ int do_change_block(Monitor *mon, const char *device,
>              return -1;
>          }
>      }
> -    if (eject_device(mon, bs, 0) < 0) {
> +    if (eject_device(bs, 0, NULL) < 0) {

This will make the change command return an undefined error for errors
caught in eject_device(). I believe this is fixed in patch 13/14? If yes,
the it's probably a good thing to note it in the commit log.

>          return -1;
>      }
>      bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
> diff --git a/blockdev.h b/blockdev.h
> index 3587786..badbf01 100644
> --- a/blockdev.h
> +++ b/blockdev.h
> @@ -58,7 +58,6 @@ DriveInfo *drive_init(QemuOpts *arg, int default_to_scsi);
>  DriveInfo *add_init_drive(const char *opts);
>  
>  void do_commit(Monitor *mon, const QDict *qdict);
> -int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  int do_block_set_passwd(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  int do_change_block(Monitor *mon, const char *device,
>                      const char *filename, const char *fmt);
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 0ccfb28..bcb789b 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -76,8 +76,7 @@ ETEXI
>          .args_type  = "force:-f,device:B",
>          .params     = "[-f] device",
>          .help       = "eject a removable medium (use -f to force it)",
> -        .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = do_eject,
> +        .mhandler.cmd = hmp_eject,
>      },
>  
>  STEXI
> diff --git a/hmp.c b/hmp.c
> index 47e1ff7..36eb5b9 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -24,3 +24,17 @@ void hmp_info_name(Monitor *mon)
>      }
>      qapi_free_NameInfo(info);
>  }
> +
> +void hmp_eject(Monitor *mon, const QDict *qdict)
> +{
> +    int force = qdict_get_try_bool(qdict, "force", 0);
> +    const char *device = qdict_get_str(qdict, "device");
> +    Error *err = NULL;
> +
> +    qmp_eject(device, true, force, &err);
> +    if (err) {
> +        monitor_printf(mon, "eject: %s\n", error_get_pretty(err));
> +        error_free(err);
> +    }
> +}
> +
> diff --git a/hmp.h b/hmp.h
> index 5fe73f1..6a552c1 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -18,5 +18,6 @@
>  #include "qapi-types.h"
>  
>  void hmp_info_name(Monitor *mon);
> +void hmp_eject(Monitor *mon, const QDict *args);
>  
>  #endif
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 654409b..934ea81 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -24,3 +24,28 @@
>  ##
>  { 'command': 'query-name', 'returns': 'NameInfo' }
>  
> +##
> +# @eject:
> +#
> +# Ejects a device from a removable drive.
> +#
> +# @device:  The name of the device
> +#
> +# @force:   @optional If true, eject regardless of whether the drive is locked.
> +#           If not specified, the default value is false.
> +#
> +# Returns:  Nothing on success
> +#           If @device is not a valid block device, DeviceNotFound
> +#           If @device is not removable and @force is false, DeviceNotRemovable
> +#           If @force is false and @device is locked, DeviceLocked
> +#
> +# Notes:    If the @force flag is used, the backing file will be closed
> +#           regardless of whether the device is removable.  This may result in
> +#           a badly broken guest.
> +#
> +#           Ejecting a device with no media results in success
> +#
> +# Since: 0.14.0
> +##

We're duplicating the documentation, as it also exists in qmp-commands.hx.

Should we drop it from there? If we do, we'll have to update the script
that generates QMP/qmp-commands.txt.

> +{ 'command': 'eject', 'data': {'device': 'str', '*force': 'bool'} }
> +
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 03f67da..81d1800 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -89,8 +89,7 @@ EQMP
>          .args_type  = "force:-f,device:B",
>          .params     = "[-f] device",
>          .help       = "eject a removable medium (use -f to force it)",
> -        .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = do_eject,
> +        .mhandler.cmd_new = qmp_marshal_input_eject,
>      },
>  
>  SQMP

Re: [Qemu-devel] [PATCH 08/14] qapi: convert eject (qmp and hmp) to QAPI

[Kevin Wolf]

Am 24.08.2011 20:43, schrieb Anthony Liguori:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  blockdev.c       |   22 +++++++++++-----------
>  blockdev.h       |    1 -
>  hmp-commands.hx  |    3 +--
>  hmp.c            |   14 ++++++++++++++
>  hmp.h            |    1 +
>  qapi-schema.json |   25 +++++++++++++++++++++++++
>  qmp-commands.hx  |    3 +--
>  7 files changed, 53 insertions(+), 16 deletions(-)

All of the conversion patches I've read so far add more lines than they
delete (even when you ignore changes to the schema, which is mostly new
documentation), even though I had expected code generation to do the
opposite, that is less hand-written code.

Is this expected, or are these first examples just exceptions?

> diff --git a/blockdev.c b/blockdev.c
> index d272659..6b7fc41 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -16,6 +16,7 @@
>  #include "sysemu.h"
>  #include "hw/qdev.h"
>  #include "block_int.h"
> +#include "qmp-commands.h"
>  
>  static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives);
>  
> @@ -644,32 +645,31 @@ out:
>      return ret;
>  }
>  
> -static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
> +static int eject_device(BlockDriverState *bs, int force, Error **errp)
>  {
>      if (!bdrv_is_removable(bs)) {
> -        qerror_report(QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs));
> +        error_set(errp, QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs));
>          return -1;
>      }
>      if (!force && bdrv_is_locked(bs)) {
> -        qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
> +        error_set(errp, QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
>          return -1;
>      }
>      bdrv_close(bs);
>      return 0;
>  }
>  
> -int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +void qmp_eject(const char *device, bool has_force, bool force, Error **errp)

Wow, this is ugly. :-)

I would suspect that many cases of optional arguments are like this: If
it isn't specified, the very first thing the monitor handler does is to
assign a default value (false in this case). Can't we include default
values in the schema and get the handling outside instead of an
additional has_xyz parameter that can easily be ignored by accident,
like in the code below?

>  {
>      BlockDriverState *bs;
> -    int force = qdict_get_try_bool(qdict, "force", 0);
> -    const char *filename = qdict_get_str(qdict, "device");
>  
> -    bs = bdrv_find(filename);
> +    bs = bdrv_find(device);
>      if (!bs) {
> -        qerror_report(QERR_DEVICE_NOT_FOUND, filename);
> -        return -1;
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
>      }
> -    return eject_device(mon, bs, force);
> +
> +    eject_device(bs, force, errp);
>  }

Kevin

Re: [Qemu-devel] [PATCH 08/14] qapi: convert eject (qmp and hmp) to QAPI

[Anthony Liguori]

On 08/25/2011 07:19 AM, Kevin Wolf wrote:
> Am 24.08.2011 20:43, schrieb Anthony Liguori:
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>> ---
>>   blockdev.c       |   22 +++++++++++-----------
>>   blockdev.h       |    1 -
>>   hmp-commands.hx  |    3 +--
>>   hmp.c            |   14 ++++++++++++++
>>   hmp.h            |    1 +
>>   qapi-schema.json |   25 +++++++++++++++++++++++++
>>   qmp-commands.hx  |    3 +--
>>   7 files changed, 53 insertions(+), 16 deletions(-)
>
> All of the conversion patches I've read so far add more lines than they
> delete (even when you ignore changes to the schema, which is mostly new
> documentation), even though I had expected code generation to do the
> opposite, that is less hand-written code.
>
> Is this expected, or are these first examples just exceptions?

Yes.  These are extremely simple interfaces so unmarshalling a couple 
strings by hand really isn't all that hard to do.  Plus, this series 
adds 4 new commands and also adds significantly more documentation than 
has ever existed before (in fact, that's the largest add in this patch).

The real code savings comes in for the commands that return complex data 
structures like query-vnc.  Not only do we save code, but we save a lot 
of complexity.

In the full conversion branch, I think we're generating somewhere around 
10k lines of code.  So there's a pretty significant savings.

>
>> diff --git a/blockdev.c b/blockdev.c
>> index d272659..6b7fc41 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -16,6 +16,7 @@
>>   #include "sysemu.h"
>>   #include "hw/qdev.h"
>>   #include "block_int.h"
>> +#include "qmp-commands.h"
>>
>>   static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives);
>>
>> @@ -644,32 +645,31 @@ out:
>>       return ret;
>>   }
>>
>> -static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
>> +static int eject_device(BlockDriverState *bs, int force, Error **errp)
>>   {
>>       if (!bdrv_is_removable(bs)) {
>> -        qerror_report(QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs));
>> +        error_set(errp, QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs));
>>           return -1;
>>       }
>>       if (!force&&  bdrv_is_locked(bs)) {
>> -        qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
>> +        error_set(errp, QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
>>           return -1;
>>       }
>>       bdrv_close(bs);
>>       return 0;
>>   }
>>
>> -int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data)
>> +void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
>
> Wow, this is ugly. :-)
>
> I would suspect that many cases of optional arguments are like this: If
> it isn't specified, the very first thing the monitor handler does is to
> assign a default value (false in this case). Can't we include default
> values in the schema and get the handling outside instead of an
> additional has_xyz parameter that can easily be ignored by accident,
> like in the code below?

There are quite a few commands that actually rely on tristate behavior. 
  So they'll do things like:

if (has_force) {
    if (force) {
       do_A();
    } else {
       do_B();
    }
} else {
    do_C();
}

It's not pretty, but it lets us preserve compatibility.  I think it's 
also safer for dealing with pointers because otherwise you have a mix of 
pointers that may be null and may not be null.  Having a clear 
indication of which pointers are nullable makes for safer code.

Regards,

Anthony Liguori

>
>>   {
>>       BlockDriverState *bs;
>> -    int force = qdict_get_try_bool(qdict, "force", 0);
>> -    const char *filename = qdict_get_str(qdict, "device");
>>
>> -    bs = bdrv_find(filename);
>> +    bs = bdrv_find(device);
>>       if (!bs) {
>> -        qerror_report(QERR_DEVICE_NOT_FOUND, filename);
>> -        return -1;
>> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
>> +        return;
>>       }
>> -    return eject_device(mon, bs, force);
>> +
>> +    eject_device(bs, force, errp);
>>   }
>
> Kevin
>

Re: [Qemu-devel] [PATCH 08/14] qapi: convert eject (qmp and hmp) to QAPI

[Kevin Wolf]

Am 25.08.2011 15:40, schrieb Anthony Liguori:
> On 08/25/2011 07:19 AM, Kevin Wolf wrote:
>> Am 24.08.2011 20:43, schrieb Anthony Liguori:
>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>> ---
>>>   blockdev.c       |   22 +++++++++++-----------
>>>   blockdev.h       |    1 -
>>>   hmp-commands.hx  |    3 +--
>>>   hmp.c            |   14 ++++++++++++++
>>>   hmp.h            |    1 +
>>>   qapi-schema.json |   25 +++++++++++++++++++++++++
>>>   qmp-commands.hx  |    3 +--
>>>   7 files changed, 53 insertions(+), 16 deletions(-)
>>
>> All of the conversion patches I've read so far add more lines than they
>> delete (even when you ignore changes to the schema, which is mostly new
>> documentation), even though I had expected code generation to do the
>> opposite, that is less hand-written code.
>>
>> Is this expected, or are these first examples just exceptions?
> 
> Yes.  These are extremely simple interfaces so unmarshalling a couple 
> strings by hand really isn't all that hard to do.  Plus, this series 
> adds 4 new commands and also adds significantly more documentation than 
> has ever existed before (in fact, that's the largest add in this patch).
> 
> The real code savings comes in for the commands that return complex data 
> structures like query-vnc.  Not only do we save code, but we save a lot 
> of complexity.
> 
> In the full conversion branch, I think we're generating somewhere around 
> 10k lines of code.  So there's a pretty significant savings.
> 
>>
>>> diff --git a/blockdev.c b/blockdev.c
>>> index d272659..6b7fc41 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -16,6 +16,7 @@
>>>   #include "sysemu.h"
>>>   #include "hw/qdev.h"
>>>   #include "block_int.h"
>>> +#include "qmp-commands.h"
>>>
>>>   static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives);
>>>
>>> @@ -644,32 +645,31 @@ out:
>>>       return ret;
>>>   }
>>>
>>> -static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
>>> +static int eject_device(BlockDriverState *bs, int force, Error **errp)
>>>   {
>>>       if (!bdrv_is_removable(bs)) {
>>> -        qerror_report(QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs));
>>> +        error_set(errp, QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs));
>>>           return -1;
>>>       }
>>>       if (!force&&  bdrv_is_locked(bs)) {
>>> -        qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
>>> +        error_set(errp, QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
>>>           return -1;
>>>       }
>>>       bdrv_close(bs);
>>>       return 0;
>>>   }
>>>
>>> -int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>> +void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
>>
>> Wow, this is ugly. :-)
>>
>> I would suspect that many cases of optional arguments are like this: If
>> it isn't specified, the very first thing the monitor handler does is to
>> assign a default value (false in this case). Can't we include default
>> values in the schema and get the handling outside instead of an
>> additional has_xyz parameter that can easily be ignored by accident,
>> like in the code below?
> 
> There are quite a few commands that actually rely on tristate behavior. 
>   So they'll do things like:
> 
> if (has_force) {
>     if (force) {
>        do_A();
>     } else {
>        do_B();
>     }
> } else {
>     do_C();
> }
> 
> It's not pretty, but it lets us preserve compatibility.  I think it's 
> also safer for dealing with pointers because otherwise you have a mix of 
> pointers that may be null and may not be null.  Having a clear 
> indication of which pointers are nullable makes for safer code.

I'm not saying that implementing a default value in generic (or
generated) code works for all cases. But if the schema supported default
values, we could get rid of the parameter in all simple cases (which I
would expect to be the majority); and if there is no default value in
the schema, we could still generate the has_* parameters.

Kevin

Re: [Qemu-devel] [PATCH 08/14] qapi: convert eject (qmp and hmp) to QAPI

[Avi Kivity]

On 08/25/2011 04:52 PM, Kevin Wolf wrote:
> >
> >  It's not pretty, but it lets us preserve compatibility.  I think it's
> >  also safer for dealing with pointers because otherwise you have a mix of
> >  pointers that may be null and may not be null.  Having a clear
> >  indication of which pointers are nullable makes for safer code.
>
> I'm not saying that implementing a default value in generic (or
> generated) code works for all cases. But if the schema supported default
> values, we could get rid of the parameter in all simple cases (which I
> would expect to be the majority); and if there is no default value in
> the schema, we could still generate the has_* parameters.
>

An alternative is to pass a struct by value, with a .valid bool followed 
by the actual value (which should be initialized even if not valid, just 
to be safe).

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 08/14] qapi: convert eject (qmp and hmp) to QAPI

[Anthony Liguori]

On 08/25/2011 08:52 AM, Kevin Wolf wrote:
> Am 25.08.2011 15:40, schrieb Anthony Liguori:
>> On 08/25/2011 07:19 AM, Kevin Wolf wrote:
>>> Am 24.08.2011 20:43, schrieb Anthony Liguori:
>>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>>> ---
>>>>    blockdev.c       |   22 +++++++++++-----------
>>>>    blockdev.h       |    1 -
>>>>    hmp-commands.hx  |    3 +--
>>>>    hmp.c            |   14 ++++++++++++++
>>>>    hmp.h            |    1 +
>>>>    qapi-schema.json |   25 +++++++++++++++++++++++++
>>>>    qmp-commands.hx  |    3 +--
>>>>    7 files changed, 53 insertions(+), 16 deletions(-)
>>>
>>> All of the conversion patches I've read so far add more lines than they
>>> delete (even when you ignore changes to the schema, which is mostly new
>>> documentation), even though I had expected code generation to do the
>>> opposite, that is less hand-written code.
>>>
>>> Is this expected, or are these first examples just exceptions?
>>
>> Yes.  These are extremely simple interfaces so unmarshalling a couple
>> strings by hand really isn't all that hard to do.  Plus, this series
>> adds 4 new commands and also adds significantly more documentation than
>> has ever existed before (in fact, that's the largest add in this patch).
>>
>> The real code savings comes in for the commands that return complex data
>> structures like query-vnc.  Not only do we save code, but we save a lot
>> of complexity.
>>
>> In the full conversion branch, I think we're generating somewhere around
>> 10k lines of code.  So there's a pretty significant savings.
>>
>>>
>>>> diff --git a/blockdev.c b/blockdev.c
>>>> index d272659..6b7fc41 100644
>>>> --- a/blockdev.c
>>>> +++ b/blockdev.c
>>>> @@ -16,6 +16,7 @@
>>>>    #include "sysemu.h"
>>>>    #include "hw/qdev.h"
>>>>    #include "block_int.h"
>>>> +#include "qmp-commands.h"
>>>>
>>>>    static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives);
>>>>
>>>> @@ -644,32 +645,31 @@ out:
>>>>        return ret;
>>>>    }
>>>>
>>>> -static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
>>>> +static int eject_device(BlockDriverState *bs, int force, Error **errp)
>>>>    {
>>>>        if (!bdrv_is_removable(bs)) {
>>>> -        qerror_report(QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs));
>>>> +        error_set(errp, QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs));
>>>>            return -1;
>>>>        }
>>>>        if (!force&&   bdrv_is_locked(bs)) {
>>>> -        qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
>>>> +        error_set(errp, QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
>>>>            return -1;
>>>>        }
>>>>        bdrv_close(bs);
>>>>        return 0;
>>>>    }
>>>>
>>>> -int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>>> +void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
>>>
>>> Wow, this is ugly. :-)
>>>
>>> I would suspect that many cases of optional arguments are like this: If
>>> it isn't specified, the very first thing the monitor handler does is to
>>> assign a default value (false in this case). Can't we include default
>>> values in the schema and get the handling outside instead of an
>>> additional has_xyz parameter that can easily be ignored by accident,
>>> like in the code below?
>>
>> There are quite a few commands that actually rely on tristate behavior.
>>    So they'll do things like:
>>
>> if (has_force) {
>>      if (force) {
>>         do_A();
>>      } else {
>>         do_B();
>>      }
>> } else {
>>      do_C();
>> }
>>
>> It's not pretty, but it lets us preserve compatibility.  I think it's
>> also safer for dealing with pointers because otherwise you have a mix of
>> pointers that may be null and may not be null.  Having a clear
>> indication of which pointers are nullable makes for safer code.
>
> I'm not saying that implementing a default value in generic (or
> generated) code works for all cases. But if the schema supported default
> values, we could get rid of the parameter in all simple cases (which I
> would expect to be the majority); and if there is no default value in
> the schema, we could still generate the has_* parameters.

I had thought about this but forgot to respond.

The problem is that the schema doesn't help the C API.  We're going to 
use QMP commands internally too.  So if the schema defaulted optional 
arguments, you'd end up having those default values open coded in the C 
APIs.  As ugly as it is, having an explicit not-specified flag allows 
the C API to have the same semantics as the wire protocol.

Honestly, having optional arguments in the methods was a bad move.  We 
shouldn't do that anymore.  Optional arguments should always be done via 
a structure as Avi sort of suggested in another response.

Regards,

Anthony Liguori

>
> Kevin
>

Re: [Qemu-devel] [PATCH 08/14] qapi: convert eject (qmp and hmp) to QAPI

[Kevin Wolf]

Am 02.09.2011 18:05, schrieb Anthony Liguori:
> On 08/25/2011 08:52 AM, Kevin Wolf wrote:
>> Am 25.08.2011 15:40, schrieb Anthony Liguori:
>>> On 08/25/2011 07:19 AM, Kevin Wolf wrote:
>>>> Am 24.08.2011 20:43, schrieb Anthony Liguori:
>>>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>>>> ---
>>>>>    blockdev.c       |   22 +++++++++++-----------
>>>>>    blockdev.h       |    1 -
>>>>>    hmp-commands.hx  |    3 +--
>>>>>    hmp.c            |   14 ++++++++++++++
>>>>>    hmp.h            |    1 +
>>>>>    qapi-schema.json |   25 +++++++++++++++++++++++++
>>>>>    qmp-commands.hx  |    3 +--
>>>>>    7 files changed, 53 insertions(+), 16 deletions(-)
>>>>
>>>> All of the conversion patches I've read so far add more lines than they
>>>> delete (even when you ignore changes to the schema, which is mostly new
>>>> documentation), even though I had expected code generation to do the
>>>> opposite, that is less hand-written code.
>>>>
>>>> Is this expected, or are these first examples just exceptions?
>>>
>>> Yes.  These are extremely simple interfaces so unmarshalling a couple
>>> strings by hand really isn't all that hard to do.  Plus, this series
>>> adds 4 new commands and also adds significantly more documentation than
>>> has ever existed before (in fact, that's the largest add in this patch).
>>>
>>> The real code savings comes in for the commands that return complex data
>>> structures like query-vnc.  Not only do we save code, but we save a lot
>>> of complexity.
>>>
>>> In the full conversion branch, I think we're generating somewhere around
>>> 10k lines of code.  So there's a pretty significant savings.
>>>
>>>>
>>>>> diff --git a/blockdev.c b/blockdev.c
>>>>> index d272659..6b7fc41 100644
>>>>> --- a/blockdev.c
>>>>> +++ b/blockdev.c
>>>>> @@ -16,6 +16,7 @@
>>>>>    #include "sysemu.h"
>>>>>    #include "hw/qdev.h"
>>>>>    #include "block_int.h"
>>>>> +#include "qmp-commands.h"
>>>>>
>>>>>    static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives);
>>>>>
>>>>> @@ -644,32 +645,31 @@ out:
>>>>>        return ret;
>>>>>    }
>>>>>
>>>>> -static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
>>>>> +static int eject_device(BlockDriverState *bs, int force, Error **errp)
>>>>>    {
>>>>>        if (!bdrv_is_removable(bs)) {
>>>>> -        qerror_report(QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs));
>>>>> +        error_set(errp, QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs));
>>>>>            return -1;
>>>>>        }
>>>>>        if (!force&&   bdrv_is_locked(bs)) {
>>>>> -        qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
>>>>> +        error_set(errp, QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
>>>>>            return -1;
>>>>>        }
>>>>>        bdrv_close(bs);
>>>>>        return 0;
>>>>>    }
>>>>>
>>>>> -int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>>>> +void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
>>>>
>>>> Wow, this is ugly. :-)
>>>>
>>>> I would suspect that many cases of optional arguments are like this: If
>>>> it isn't specified, the very first thing the monitor handler does is to
>>>> assign a default value (false in this case). Can't we include default
>>>> values in the schema and get the handling outside instead of an
>>>> additional has_xyz parameter that can easily be ignored by accident,
>>>> like in the code below?
>>>
>>> There are quite a few commands that actually rely on tristate behavior.
>>>    So they'll do things like:
>>>
>>> if (has_force) {
>>>      if (force) {
>>>         do_A();
>>>      } else {
>>>         do_B();
>>>      }
>>> } else {
>>>      do_C();
>>> }
>>>
>>> It's not pretty, but it lets us preserve compatibility.  I think it's
>>> also safer for dealing with pointers because otherwise you have a mix of
>>> pointers that may be null and may not be null.  Having a clear
>>> indication of which pointers are nullable makes for safer code.
>>
>> I'm not saying that implementing a default value in generic (or
>> generated) code works for all cases. But if the schema supported default
>> values, we could get rid of the parameter in all simple cases (which I
>> would expect to be the majority); and if there is no default value in
>> the schema, we could still generate the has_* parameters.
> 
> I had thought about this but forgot to respond.
> 
> The problem is that the schema doesn't help the C API.  We're going to 
> use QMP commands internally too.  So if the schema defaulted optional 
> arguments, you'd end up having those default values open coded in the C 
> APIs.  As ugly as it is, having an explicit not-specified flag allows 
> the C API to have the same semantics as the wire protocol.

Do we need it? One of the major use cases of optional arguments in the
QMP protocol is extending existing commands in newer versions. For these
cases, there is no problem with requiring all internal C callers to be
updated to actually specify the field that is optional externally.

> Honestly, having optional arguments in the methods was a bad move.  We 
> shouldn't do that anymore.  Optional arguments should always be done via 
> a structure as Avi sort of suggested in another response.

Maybe. I'm not sure if that makes the C code any nicer, though.

Kevin

[Qemu-devel] [PATCH 09/14] qapi: convert block_passwd and add set-blockdev-password

[Anthony Liguori]

block_passwd is unfortunately named so while converting block_passwd to QAPI,
introduce a more properly named alias.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 blockdev.c       |   29 +++++++++++++++--------------
 hmp-commands.hx  |    2 +-
 hmp.c            |   12 ++++++++++++
 hmp.h            |    1 +
 qapi-schema.json |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx  |   11 +++++++++--
 6 files changed, 85 insertions(+), 17 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 6b7fc41..37b2f29 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -672,28 +672,29 @@ void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
     eject_device(bs, force, errp);
 }
 
-int do_block_set_passwd(Monitor *mon, const QDict *qdict,
-                        QObject **ret_data)
+void qmp_set_blockdev_password(const char *device, const char *password,
+                               Error **err)
 {
     BlockDriverState *bs;
-    int err;
+    int ret;
 
-    bs = bdrv_find(qdict_get_str(qdict, "device"));
+    bs = bdrv_find(device);
     if (!bs) {
-        qerror_report(QERR_DEVICE_NOT_FOUND, qdict_get_str(qdict, "device"));
-        return -1;
+        error_set(err, QERR_DEVICE_NOT_FOUND, device);
+        return;
     }
 
-    err = bdrv_set_key(bs, qdict_get_str(qdict, "password"));
-    if (err == -EINVAL) {
-        qerror_report(QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs));
-        return -1;
-    } else if (err < 0) {
-        qerror_report(QERR_INVALID_PASSWORD);
-        return -1;
+    ret = bdrv_set_key(bs, password);
+    if (ret == -EINVAL) {
+        error_set(err, QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs));
+    } else if (ret < 0) {
+        error_set(err, QERR_INVALID_PASSWORD);
     }
+}
 
-    return 0;
+void qmp_block_passwd(const char *device, const char *password, Error **err)
+{
+    qmp_set_blockdev_password(device, password, err);
 }
 
 int do_change_block(Monitor *mon, const char *device,
diff --git a/hmp-commands.hx b/hmp-commands.hx
index bcb789b..2f0ffa3 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1205,7 +1205,7 @@ ETEXI
         .params     = "block_passwd device password",
         .help       = "set the password of encrypted block devices",
         .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_block_set_passwd,
+        .mhandler.cmd = hmp_block_passwd,
     },
 
 STEXI
diff --git a/hmp.c b/hmp.c
index 36eb5b9..a8ae36b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -38,3 +38,15 @@ void hmp_eject(Monitor *mon, const QDict *qdict)
     }
 }
 
+void hmp_block_passwd(Monitor *mon, const QDict *qdict)
+{
+    const char *device = qdict_get_str(qdict, "device");
+    const char *password = qdict_get_str(qdict, "password");
+    Error *err = NULL;
+
+    qmp_set_blockdev_password(device, password, &err);
+    if (err) {
+        monitor_printf(mon, "block_passwd: %s\n", error_get_pretty(err));
+        error_free(err);
+    }
+}
diff --git a/hmp.h b/hmp.h
index 6a552c1..8f72ef2 100644
--- a/hmp.h
+++ b/hmp.h
@@ -19,5 +19,6 @@
 
 void hmp_info_name(Monitor *mon);
 void hmp_eject(Monitor *mon, const QDict *args);
+void hmp_block_passwd(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/qapi-schema.json b/qapi-schema.json
index 934ea81..f159d81 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -49,3 +49,50 @@
 ##
 { 'command': 'eject', 'data': {'device': 'str', '*force': 'bool'} }
 
+##
+# @block_passwd:
+#
+# This command sets the password of a block device that has not been open
+# with a password and requires one.
+#
+# The two cases where this can happen are a block device is created through
+# QEMU's initial command line or a block device is changed through the legacy
+# @change interface.
+#
+# In the event that the block device is created through the initial command
+# line, the VM will start in the stopped state regardless of whether '-S' is
+# used.  The intention is for a management tool to query the block devices to
+# determine which ones are encrypted, set the passwords with this command, and
+# then start the guest with the @cont command.
+#
+# @device:   the name of the device to set the password on
+#
+# @password: the password to use for the device
+#
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#          If @device is not encrypted, DeviceNotEncrypted
+#          If @password is not valid for this device, InvalidPassword
+#
+# Notes:  Not all block formats support encryption and some that do are not
+#         able to validate that a password is correct.  Disk corruption may
+#         occur if an invalid password is specified.
+#
+# Since: 0.14.0
+##
+{ 'command': 'block_passwd',
+  'data': {'device': 'str', 'password': 'str'} }
+
+##
+# @set-blockdev-password:
+#
+# Alias for @block_passwd.
+#
+# @device:   see @block_passwd
+# @password: see @block_passwd
+#
+# Since: 1.0
+##
+{ 'command': 'set-blockdev-password',
+  'data': {'device': 'str', 'password': 'str'} }
+
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 81d1800..909c778 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -837,8 +837,15 @@ EQMP
         .args_type  = "device:B,password:s",
         .params     = "block_passwd device password",
         .help       = "set the password of encrypted block devices",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_block_set_passwd,
+        .mhandler.cmd_new = qmp_marshal_input_block_passwd,
+    },
+
+    {
+        .name       = "set-blockdev-password",
+        .args_type  = "device:B,password:s",
+        .params     = "block_passwd device password",
+        .help       = "set the password of encrypted block devices",
+        .mhandler.cmd_new = qmp_marshal_input_set_blockdev_password,
     },
 
 SQMP
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 09/14] qapi: convert block_passwd and add set-blockdev-password

[Kevin Wolf]

Am 24.08.2011 20:43, schrieb Anthony Liguori:
> block_passwd is unfortunately named so while converting block_passwd to QAPI,
> introduce a more properly named alias.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

I'm not sure if I like the set-blockdev-password alias, for two reasons.

The first one is that blockdev_* is supposed to be the new, clean
interface, so I wouldn't add similarly named commands without having a
look at the big picture. For example, what again was the reason for
having the password separate from other block device options?

The second one is that names like set-foo, get-foo and frob-foo make it
very hard to see what I can do with a foo. If they are called as today
(drive_add, drive_del) and ideally use a consistent prefix (unlike
drive_*, block_*, snapshot_blkdev today), you'll find all of them in the
same place in the documentation and in HMP typing blockdev_<TAB> will
show you everything you can do with a block device.

Kevin

[Qemu-devel] [PATCH 10/14] qapi: add change-vnc-password

[Anthony Liguori]

This is a new QMP only command that only changes the VNC password.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 qapi-schema.json |   13 +++++++++++++
 qmp-commands.hx  |    8 ++++++++
 qmp.c            |   11 ++++++++++-
 3 files changed, 31 insertions(+), 1 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index f159d81..3b2229f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -96,3 +96,16 @@
 { 'command': 'set-blockdev-password',
   'data': {'device': 'str', 'password': 'str'} }
 
+##
+# @change-vnc-password:
+#
+# Change the VNC server password.
+#
+# @target:  the new password to use with VNC authentication
+#
+# Since: 1.0
+#
+# Notes:  An empty password in this command will set the password to the empty
+#         string.  Existing clients are unaffected by executing this command.
+##
+{ 'command': 'change-vnc-password', 'data': {'password': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 909c778..d60f72f 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -868,6 +868,14 @@ Example:
 EQMP
 
     {
+        .name       = "change-vnc-password",
+        .args_type  = "password:s",
+        .params     = "password",
+        .help       = "set vnc password",
+        .mhandler.cmd_new = qmp_marshal_input_change_vnc_password,
+    },
+
+    {
         .name       = "set_password",
         .args_type  = "protocol:s,password:s,connected:s?",
         .params     = "protocol password action-if-connected",
diff --git a/qmp.c b/qmp.c
index 8aa9c66..f817a88 100644
--- a/qmp.c
+++ b/qmp.c
@@ -12,9 +12,11 @@
  */
 
 #include "qemu-common.h"
-#include "sysemu.h"
 #include "qmp-commands.h"
 
+#include "sysemu.h"
+#include "console.h"
+
 NameInfo *qmp_query_name(Error **errp)
 {
     NameInfo *info = g_malloc0(sizeof(*info));
@@ -26,3 +28,10 @@ NameInfo *qmp_query_name(Error **errp)
 
     return info;
 }
+
+void qmp_change_vnc_password(const char *password, Error **err)
+{
+    if (vnc_display_password(NULL, password) < 0) {
+        error_set(err, QERR_SET_PASSWD_FAILED);
+    }
+}
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 10/14] qapi: add change-vnc-password

[Gerd Hoffmann]

On 08/24/11 20:43, Anthony Liguori wrote:
> This is a new QMP only command that only changes the VNC password.

What is wrong with "set_password vnc $secret" ?

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 10/14] qapi: add change-vnc-password

[Anthony Liguori]

On 08/25/2011 04:07 AM, Gerd Hoffmann wrote:
> On 08/24/11 20:43, Anthony Liguori wrote:
>> This is a new QMP only command that only changes the VNC password.
>
> What is wrong with "set_password vnc $secret" ?

Overloading a single function to do multiple things makes the function 
hard to use.   Consider the C interface:

void qmp_set_password(QmpSession *sess, const char *protocol,
                       const char *password, bool has_connected,
                       const char *connected)

Since both protocol and connected are strings, you can get hard to debug 
errors by using the string "VNC" instead of "vnc" or "Keep" instead of 
"keep".  There's no obvious way to figure out what strings are valid for 
protocol programatically so if we added rdesktop or something like that, 
the interface would become awkward to use.

Likewise, it's non-intuitive that connected only has one valid value for 
VNC which means that for changing the VNC password, it's just extra 
typing.  Compare that to:

void qmp_change_vnc_password(QmpSession *sess, const char *password)

It's impossible to use wrong.  If we add a qmp_change_rdesktop_password, 
it's easily detectable.

Regards,

Anthony Liguori

>
> cheers,
> Gerd
>
>

Re: [Qemu-devel] [PATCH 10/14] qapi: add change-vnc-password

[Luiz Capitulino]

On Wed, 24 Aug 2011 13:43:05 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> This is a new QMP only command that only changes the VNC password.

Isn't this useful in HMP too?

> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  qapi-schema.json |   13 +++++++++++++
>  qmp-commands.hx  |    8 ++++++++
>  qmp.c            |   11 ++++++++++-
>  3 files changed, 31 insertions(+), 1 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index f159d81..3b2229f 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -96,3 +96,16 @@
>  { 'command': 'set-blockdev-password',
>    'data': {'device': 'str', 'password': 'str'} }
>  
> +##
> +# @change-vnc-password:
> +#
> +# Change the VNC server password.
> +#
> +# @target:  the new password to use with VNC authentication

The argument name is "password".

> +#
> +# Since: 1.0
> +#
> +# Notes:  An empty password in this command will set the password to the empty
> +#         string.  Existing clients are unaffected by executing this command.
> +##
> +{ 'command': 'change-vnc-password', 'data': {'password': 'str'} }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 909c778..d60f72f 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -868,6 +868,14 @@ Example:
>  EQMP
>  
>      {
> +        .name       = "change-vnc-password",
> +        .args_type  = "password:s",
> +        .params     = "password",
> +        .help       = "set vnc password",
> +        .mhandler.cmd_new = qmp_marshal_input_change_vnc_password,
> +    },
> +
> +    {
>          .name       = "set_password",
>          .args_type  = "protocol:s,password:s,connected:s?",
>          .params     = "protocol password action-if-connected",
> diff --git a/qmp.c b/qmp.c
> index 8aa9c66..f817a88 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -12,9 +12,11 @@
>   */
>  
>  #include "qemu-common.h"
> -#include "sysemu.h"
>  #include "qmp-commands.h"
>  
> +#include "sysemu.h"
> +#include "console.h"
> +
>  NameInfo *qmp_query_name(Error **errp)
>  {
>      NameInfo *info = g_malloc0(sizeof(*info));
> @@ -26,3 +28,10 @@ NameInfo *qmp_query_name(Error **errp)
>  
>      return info;
>  }
> +
> +void qmp_change_vnc_password(const char *password, Error **err)
> +{
> +    if (vnc_display_password(NULL, password) < 0) {
> +        error_set(err, QERR_SET_PASSWD_FAILED);
> +    }
> +}

Re: [Qemu-devel] [PATCH 10/14] qapi: add change-vnc-password

[Anthony Liguori]

On 08/25/2011 08:33 AM, Luiz Capitulino wrote:
> On Wed, 24 Aug 2011 13:43:05 -0500
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>
>> This is a new QMP only command that only changes the VNC password.
>
> Isn't this useful in HMP too?

I think the right design goal for HMP is to minimize the number of 
commands since a human can only remember so much.

>
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>> ---
>>   qapi-schema.json |   13 +++++++++++++
>>   qmp-commands.hx  |    8 ++++++++
>>   qmp.c            |   11 ++++++++++-
>>   3 files changed, 31 insertions(+), 1 deletions(-)
>>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index f159d81..3b2229f 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -96,3 +96,16 @@
>>   { 'command': 'set-blockdev-password',
>>     'data': {'device': 'str', 'password': 'str'} }
>>
>> +##
>> +# @change-vnc-password:
>> +#
>> +# Change the VNC server password.
>> +#
>> +# @target:  the new password to use with VNC authentication
>
> The argument name is "password".

Ack.

Regards,

Anthony Liguori

>
>> +#
>> +# Since: 1.0
>> +#
>> +# Notes:  An empty password in this command will set the password to the empty
>> +#         string.  Existing clients are unaffected by executing this command.
>> +##
>> +{ 'command': 'change-vnc-password', 'data': {'password': 'str'} }
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 909c778..d60f72f 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -868,6 +868,14 @@ Example:
>>   EQMP
>>
>>       {
>> +        .name       = "change-vnc-password",
>> +        .args_type  = "password:s",
>> +        .params     = "password",
>> +        .help       = "set vnc password",
>> +        .mhandler.cmd_new = qmp_marshal_input_change_vnc_password,
>> +    },
>> +
>> +    {
>>           .name       = "set_password",
>>           .args_type  = "protocol:s,password:s,connected:s?",
>>           .params     = "protocol password action-if-connected",
>> diff --git a/qmp.c b/qmp.c
>> index 8aa9c66..f817a88 100644
>> --- a/qmp.c
>> +++ b/qmp.c
>> @@ -12,9 +12,11 @@
>>    */
>>
>>   #include "qemu-common.h"
>> -#include "sysemu.h"
>>   #include "qmp-commands.h"
>>
>> +#include "sysemu.h"
>> +#include "console.h"
>> +
>>   NameInfo *qmp_query_name(Error **errp)
>>   {
>>       NameInfo *info = g_malloc0(sizeof(*info));
>> @@ -26,3 +28,10 @@ NameInfo *qmp_query_name(Error **errp)
>>
>>       return info;
>>   }
>> +
>> +void qmp_change_vnc_password(const char *password, Error **err)
>> +{
>> +    if (vnc_display_password(NULL, password)<  0) {
>> +        error_set(err, QERR_SET_PASSWD_FAILED);
>> +    }
>> +}
>
>

[Qemu-devel] [PATCH 11/14] qapi: add change-vnc-listen

[Anthony Liguori]

New QMP only command to change the VNC server's listening address.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 qapi-schema.json |   14 ++++++++++++++
 qmp-commands.hx  |    8 ++++++++
 qmp.c            |    7 +++++++
 3 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 3b2229f..211200a 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -109,3 +109,17 @@
 #         string.  Existing clients are unaffected by executing this command.
 ##
 { 'command': 'change-vnc-password', 'data': {'password': 'str'} }
+
+##
+# @change-vnc-listen:
+#
+# Change the host that the VNC server listens on.
+#
+# @target:  the new server specification to listen on
+#
+# Since: 1.0
+#
+# Notes:  At this moment in time, the behavior of existing client connections
+#         when this command is executed is undefined.
+##
+{ 'command': 'change-vnc-listen', 'data': {'target': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index d60f72f..c0d0ca3 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -876,6 +876,14 @@ EQMP
     },
 
     {
+        .name       = "change-vnc-listen",
+        .args_type  = "target:s",
+        .params     = "target",
+        .help       = "set vnc listening address",
+        .mhandler.cmd_new = qmp_marshal_input_change_vnc_listen,
+    },
+
+    {
         .name       = "set_password",
         .args_type  = "protocol:s,password:s,connected:s?",
         .params     = "protocol password action-if-connected",
diff --git a/qmp.c b/qmp.c
index f817a88..73d6172 100644
--- a/qmp.c
+++ b/qmp.c
@@ -35,3 +35,10 @@ void qmp_change_vnc_password(const char *password, Error **err)
         error_set(err, QERR_SET_PASSWD_FAILED);
     }
 }
+
+void qmp_change_vnc_listen(const char *target, Error **err)
+{
+    if (vnc_display_open(NULL, target) < 0) {
+        error_set(err, QERR_VNC_SERVER_FAILED, target);
+    }
+}
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 11/14] qapi: add change-vnc-listen

[Luiz Capitulino]

On Wed, 24 Aug 2011 13:43:06 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> New QMP only command to change the VNC server's listening address.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  qapi-schema.json |   14 ++++++++++++++
>  qmp-commands.hx  |    8 ++++++++
>  qmp.c            |    7 +++++++
>  3 files changed, 29 insertions(+), 0 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 3b2229f..211200a 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -109,3 +109,17 @@
>  #         string.  Existing clients are unaffected by executing this command.
>  ##
>  { 'command': 'change-vnc-password', 'data': {'password': 'str'} }
> +
> +##
> +# @change-vnc-listen:
> +#
> +# Change the host that the VNC server listens on.
> +#
> +# @target:  the new server specification to listen on
> +#
> +# Since: 1.0
> +#
> +# Notes:  At this moment in time, the behavior of existing client connections
> +#         when this command is executed is undefined.
> +##

What's the expected behaviour wrt authentication? Right now it will disable
any authentication set for the previous address. It should at least be noted
in the documentation if that's expected.

> +{ 'command': 'change-vnc-listen', 'data': {'target': 'str'} }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index d60f72f..c0d0ca3 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -876,6 +876,14 @@ EQMP
>      },
>  
>      {
> +        .name       = "change-vnc-listen",
> +        .args_type  = "target:s",
> +        .params     = "target",
> +        .help       = "set vnc listening address",
> +        .mhandler.cmd_new = qmp_marshal_input_change_vnc_listen,
> +    },
> +
> +    {
>          .name       = "set_password",
>          .args_type  = "protocol:s,password:s,connected:s?",
>          .params     = "protocol password action-if-connected",
> diff --git a/qmp.c b/qmp.c
> index f817a88..73d6172 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -35,3 +35,10 @@ void qmp_change_vnc_password(const char *password, Error **err)
>          error_set(err, QERR_SET_PASSWD_FAILED);
>      }
>  }
> +
> +void qmp_change_vnc_listen(const char *target, Error **err)
> +{
> +    if (vnc_display_open(NULL, target) < 0) {
> +        error_set(err, QERR_VNC_SERVER_FAILED, target);
> +    }
> +}

Re: [Qemu-devel] [PATCH 11/14] qapi: add change-vnc-listen

[Anthony Liguori]

On 08/25/2011 08:32 AM, Luiz Capitulino wrote:
> On Wed, 24 Aug 2011 13:43:06 -0500
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>
>> New QMP only command to change the VNC server's listening address.
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>> ---
>>   qapi-schema.json |   14 ++++++++++++++
>>   qmp-commands.hx  |    8 ++++++++
>>   qmp.c            |    7 +++++++
>>   3 files changed, 29 insertions(+), 0 deletions(-)
>>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 3b2229f..211200a 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -109,3 +109,17 @@
>>   #         string.  Existing clients are unaffected by executing this command.
>>   ##
>>   { 'command': 'change-vnc-password', 'data': {'password': 'str'} }
>> +
>> +##
>> +# @change-vnc-listen:
>> +#
>> +# Change the host that the VNC server listens on.
>> +#
>> +# @target:  the new server specification to listen on
>> +#
>> +# Since: 1.0
>> +#
>> +# Notes:  At this moment in time, the behavior of existing client connections
>> +#         when this command is executed is undefined.
>> +##
>
> What's the expected behaviour wrt authentication? Right now it will disable
> any authentication set for the previous address. It should at least be noted
> in the documentation if that's expected.

Indeed.  Thanks.

Regards,

Anthony Liguori

>
>> +{ 'command': 'change-vnc-listen', 'data': {'target': 'str'} }
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index d60f72f..c0d0ca3 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -876,6 +876,14 @@ EQMP
>>       },
>>
>>       {
>> +        .name       = "change-vnc-listen",
>> +        .args_type  = "target:s",
>> +        .params     = "target",
>> +        .help       = "set vnc listening address",
>> +        .mhandler.cmd_new = qmp_marshal_input_change_vnc_listen,
>> +    },
>> +
>> +    {
>>           .name       = "set_password",
>>           .args_type  = "protocol:s,password:s,connected:s?",
>>           .params     = "protocol password action-if-connected",
>> diff --git a/qmp.c b/qmp.c
>> index f817a88..73d6172 100644
>> --- a/qmp.c
>> +++ b/qmp.c
>> @@ -35,3 +35,10 @@ void qmp_change_vnc_password(const char *password, Error **err)
>>           error_set(err, QERR_SET_PASSWD_FAILED);
>>       }
>>   }
>> +
>> +void qmp_change_vnc_listen(const char *target, Error **err)
>> +{
>> +    if (vnc_display_open(NULL, target)<  0) {
>> +        error_set(err, QERR_VNC_SERVER_FAILED, target);
>> +    }
>> +}
>
>

[Qemu-devel] [PATCH 12/14] qapi: introduce change-blockdev

[Anthony Liguori]

A new QMP only command to change the blockdev associated with a block device.
The semantics of change right now are just plain scary.  This command introduces
sane semantics.  For more details, see the documentation in the schema file.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 blockdev.c       |  108 +++++++++++++++++++++++++++++++++++++++++++++++++++---
 qapi-schema.json |   30 +++++++++++++++
 qmp-commands.hx  |    8 ++++
 3 files changed, 140 insertions(+), 6 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 37b2f29..c00c69d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -697,12 +697,101 @@ void qmp_block_passwd(const char *device, const char *password, Error **err)
     qmp_set_blockdev_password(device, password, err);
 }
 
+static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
+                                    int bdrv_flags, BlockDriver *drv,
+                                    const char *password, Error **errp)
+{
+    if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) {
+        error_set(errp, QERR_OPEN_FILE_FAILED, filename);
+        return;
+    }
+
+    if (bdrv_key_required(bs)) {
+        if (password) {
+            if (bdrv_set_key(bs, password) < 0) {
+                error_set(errp, QERR_INVALID_PASSWORD);
+            }
+        } else {
+            error_set(errp, QERR_DEVICE_ENCRYPTED, bdrv_get_device_name(bs),
+                      bdrv_get_encrypted_filename(bs));
+        }
+    } else if (password) {
+        error_set(errp, QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs));
+    }
+}
+
+void qmp_change_blockdev(const char *device, const char *filename,
+                         bool has_format, const char *format,
+                         bool has_password, const char *password,
+                         Error **errp)
+{
+    BlockDriverState *bs, *bs1;
+    BlockDriver *drv = NULL;
+    int bdrv_flags;
+    Error *err = NULL;
+    bool probed_raw = false;
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    if (has_format) {
+        drv = bdrv_find_whitelisted_format(format);
+        if (!drv) {
+            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
+            return;
+        }
+    }
+
+    bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
+    bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
+
+    if (!has_password) {
+        password = NULL;
+    }
+
+    /* Try to open once with a temporary block device to make sure that
+     * the disk isn't encrypted and we lack a key.  This also helps keep
+     * this operation as a transaction.  That is, if we fail, we try very
+     * hard to make sure that the state is the same as before the operation
+     * was started.
+     */
+    bs1 = bdrv_new("");
+    qmp_bdrv_open_encrypted(bs1, filename, bdrv_flags, drv, password, &err);
+    if (!has_format && bs1->drv->unsafe_probe) {
+        probed_raw = true;
+    }
+    bdrv_delete(bs1);
+
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
+    if (probed_raw) {
+        /* We will not auto probe raw files. */
+        error_set(errp, QERR_MISSING_PARAMETER, "format");
+        return;
+    }
+
+    eject_device(bs, 0, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
+    qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, password, errp);
+}
+
 int do_change_block(Monitor *mon, const char *device,
                     const char *filename, const char *fmt)
 {
     BlockDriverState *bs;
     BlockDriver *drv = NULL;
     int bdrv_flags;
+    Error *err = NULL;
 
     bs = bdrv_find(device);
     if (!bs) {
@@ -716,16 +805,23 @@ int do_change_block(Monitor *mon, const char *device,
             return -1;
         }
     }
-    if (eject_device(bs, 0, NULL) < 0) {
-        return -1;
-    }
+
     bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
     bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
-    if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) {
-        qerror_report(QERR_OPEN_FILE_FAILED, filename);
+
+    eject_device(bs, 0, &err);
+    if (err) {
+        qerror_report_err(err);
+        return -1;
+    }
+
+    qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, &err);
+    if (err) {
+        qerror_report_err(err);
         return -1;
     }
-    return monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
+
+    return 0;
 }
 
 int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
diff --git a/qapi-schema.json b/qapi-schema.json
index 211200a..139c6e3 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -123,3 +123,33 @@
 #         when this command is executed is undefined.
 ##
 { 'command': 'change-vnc-listen', 'data': {'target': 'str'} }
+
+##
+# @change-blockdev:
+#
+# This is the preferred interface for changing a block device.
+#
+# @device:   The block device name.
+#
+# @filename: The new filename for the block device.  This may contain options
+#            encoded in a format specified by @format.
+#
+# @format:   #optional The format to use open the block device
+#
+# @password: #optional The password to use if the block device is encrypted
+#
+# Returns:  Nothing on success.
+#          If @device is not a valid block device, DeviceNotFound
+#          If @format is not a valid block format, InvalidBlockFormat
+#          If @filename is encrypted and @password isn't supplied,
+#            DeviceEncrypted.  The call should be repeated with @password
+#            supplied.
+#          If @format is not specified and @filename is a format that cannot
+#            be safely probed, MissingParameter.
+#          If @filename cannot be opened, OpenFileFailed
+#
+# Since: 1.0
+##
+{ 'command': 'change-blockdev',
+  'data': {'device': 'str', 'filename': 'str', '*format': 'str',
+           '*password': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index c0d0ca3..cec7135 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -121,6 +121,14 @@ EQMP
         .mhandler.cmd_new = do_change,
     },
 
+    {
+        .name       = "change-blockdev",
+        .args_type  = "device:B,target:F,arg:s?pass:s?",
+        .params     = "device filename [format] [password]",
+        .help       = "change a removable medium, optional format",
+        .mhandler.cmd_new = qmp_marshal_input_change_blockdev,
+    },
+
 SQMP
 change
 ------
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 12/14] qapi: introduce change-blockdev

[Kevin Wolf]

Am 24.08.2011 20:43, schrieb Anthony Liguori:
> A new QMP only command to change the blockdev associated with a block device.
> The semantics of change right now are just plain scary.  This command introduces
> sane semantics.  For more details, see the documentation in the schema file.

Changes to semantics should be mentioned in the commit message, and in
more detail than just that they are sane (the documentation in the
schema file doesn't describe differences to the old command).

Just assuming that you are right about sanity, I wonder how sanity and
compatibility could possibly be achieved at once... I suspect the
semantics is clearer now in that it doesn't overload one command with
block and VNC subcommands, but I doubt that the semantics of the block
version is much better than before.

So please call the command drive_change, it's similar to drive_add and
drive_del, not to the future blockdev_add/remove.

> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  blockdev.c       |  108 +++++++++++++++++++++++++++++++++++++++++++++++++++---
>  qapi-schema.json |   30 +++++++++++++++
>  qmp-commands.hx  |    8 ++++
>  3 files changed, 140 insertions(+), 6 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 37b2f29..c00c69d 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -697,12 +697,101 @@ void qmp_block_passwd(const char *device, const char *password, Error **err)
>      qmp_set_blockdev_password(device, password, err);
>  }
>  
> +static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
> +                                    int bdrv_flags, BlockDriver *drv,
> +                                    const char *password, Error **errp)
> +{
> +    if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) {
> +        error_set(errp, QERR_OPEN_FILE_FAILED, filename);
> +        return;
> +    }
> +
> +    if (bdrv_key_required(bs)) {
> +        if (password) {
> +            if (bdrv_set_key(bs, password) < 0) {
> +                error_set(errp, QERR_INVALID_PASSWORD);
> +            }
> +        } else {
> +            error_set(errp, QERR_DEVICE_ENCRYPTED, bdrv_get_device_name(bs),
> +                      bdrv_get_encrypted_filename(bs));
> +        }
> +    } else if (password) {
> +        error_set(errp, QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs));
> +    }
> +}
> +
> +void qmp_change_blockdev(const char *device, const char *filename,
> +                         bool has_format, const char *format,
> +                         bool has_password, const char *password,
> +                         Error **errp)
> +{
> +    BlockDriverState *bs, *bs1;
> +    BlockDriver *drv = NULL;
> +    int bdrv_flags;
> +    Error *err = NULL;
> +    bool probed_raw = false;
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    if (has_format) {
> +        drv = bdrv_find_whitelisted_format(format);
> +        if (!drv) {
> +            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
> +            return;
> +        }
> +    }
> +
> +    bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
> +    bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
> +
> +    if (!has_password) {
> +        password = NULL;
> +    }
> +
> +    /* Try to open once with a temporary block device to make sure that
> +     * the disk isn't encrypted and we lack a key.  This also helps keep
> +     * this operation as a transaction.  That is, if we fail, we try very
> +     * hard to make sure that the state is the same as before the operation
> +     * was started.
> +     */
> +    bs1 = bdrv_new("");
> +    qmp_bdrv_open_encrypted(bs1, filename, bdrv_flags, drv, password, &err);
> +    if (!has_format && bs1->drv->unsafe_probe) {
> +        probed_raw = true;
> +    }
> +    bdrv_delete(bs1);
> +
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +
> +    if (probed_raw) {
> +        /* We will not auto probe raw files. */
> +        error_set(errp, QERR_MISSING_PARAMETER, "format");
> +        return;
> +    }
> +
> +    eject_device(bs, 0, &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +
> +    qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, password, errp);
> +}

Can't this share code with the old command? I don't like this duplication.

Kevin

Re: [Qemu-devel] [PATCH 12/14] qapi: introduce change-blockdev

[Anthony Liguori]

On 08/25/2011 07:46 AM, Kevin Wolf wrote:
> Am 24.08.2011 20:43, schrieb Anthony Liguori:
>> A new QMP only command to change the blockdev associated with a block device.
>> The semantics of change right now are just plain scary.  This command introduces
>> sane semantics.  For more details, see the documentation in the schema file.
>
> Changes to semantics should be mentioned in the commit message, and in
> more detail than just that they are sane (the documentation in the
> schema file doesn't describe differences to the old command).

Right, but the schema describes the old command.

There are a couple major differences with the new command vs old:

1) If you had a block device named 'vnc', the old command is broken.

2) For an encrypted device, the old change command returns an error but 
has semantically completed the operation.  The usage is:

(qemu) change ide0 foo.img
Error: device ide0 is encrypted
(qemu) block_passwd ide0 secret

Because even though an error is returned, the change operation has 
actually succeeded.  The new command has transactional semantics.  If an 
error is returned, nothing has happened.  To set passwords, an 
additional option is supported to specify the password.  So you would do:

(qemu) change-blockdev ide0 foo.img
Error: device ide0 is encrypted
(qemu) change-blockdev ide0 foo.img secret
(qemu)

In addition, change-blockdev will not allow you to perform unsafe 
probing.  You can only change to a raw device if you specify a raw format.

>
> Just assuming that you are right about sanity, I wonder how sanity and
> compatibility could possibly be achieved at once... I suspect the
> semantics is clearer now in that it doesn't overload one command with
> block and VNC subcommands, but I doubt that the semantics of the block
> version is much better than before.

I don't see how we can change the behavior of the change command.

> So please call the command drive_change, it's similar to drive_add and
> drive_del, not to the future blockdev_add/remove.

Ack.

>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>> ---
>>   blockdev.c       |  108 +++++++++++++++++++++++++++++++++++++++++++++++++++---
>>   qapi-schema.json |   30 +++++++++++++++
>>   qmp-commands.hx  |    8 ++++
>>   3 files changed, 140 insertions(+), 6 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 37b2f29..c00c69d 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -697,12 +697,101 @@ void qmp_block_passwd(const char *device, const char *password, Error **err)
>>       qmp_set_blockdev_password(device, password, err);
>>   }
>>
>> +static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
>> +                                    int bdrv_flags, BlockDriver *drv,
>> +                                    const char *password, Error **errp)
>> +{
>> +    if (bdrv_open(bs, filename, bdrv_flags, drv)<  0) {
>> +        error_set(errp, QERR_OPEN_FILE_FAILED, filename);
>> +        return;
>> +    }
>> +
>> +    if (bdrv_key_required(bs)) {
>> +        if (password) {
>> +            if (bdrv_set_key(bs, password)<  0) {
>> +                error_set(errp, QERR_INVALID_PASSWORD);
>> +            }
>> +        } else {
>> +            error_set(errp, QERR_DEVICE_ENCRYPTED, bdrv_get_device_name(bs),
>> +                      bdrv_get_encrypted_filename(bs));
>> +        }
>> +    } else if (password) {
>> +        error_set(errp, QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs));
>> +    }
>> +}
>> +
>> +void qmp_change_blockdev(const char *device, const char *filename,
>> +                         bool has_format, const char *format,
>> +                         bool has_password, const char *password,
>> +                         Error **errp)
>> +{
>> +    BlockDriverState *bs, *bs1;
>> +    BlockDriver *drv = NULL;
>> +    int bdrv_flags;
>> +    Error *err = NULL;
>> +    bool probed_raw = false;
>> +
>> +    bs = bdrv_find(device);
>> +    if (!bs) {
>> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
>> +        return;
>> +    }
>> +
>> +    if (has_format) {
>> +        drv = bdrv_find_whitelisted_format(format);
>> +        if (!drv) {
>> +            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
>> +            return;
>> +        }
>> +    }
>> +
>> +    bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
>> +    bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
>> +
>> +    if (!has_password) {
>> +        password = NULL;
>> +    }
>> +
>> +    /* Try to open once with a temporary block device to make sure that
>> +     * the disk isn't encrypted and we lack a key.  This also helps keep
>> +     * this operation as a transaction.  That is, if we fail, we try very
>> +     * hard to make sure that the state is the same as before the operation
>> +     * was started.
>> +     */
>> +    bs1 = bdrv_new("");
>> +    qmp_bdrv_open_encrypted(bs1, filename, bdrv_flags, drv, password,&err);
>> +    if (!has_format&&  bs1->drv->unsafe_probe) {
>> +        probed_raw = true;
>> +    }
>> +    bdrv_delete(bs1);
>> +
>> +    if (err) {
>> +        error_propagate(errp, err);
>> +        return;
>> +    }
>> +
>> +    if (probed_raw) {
>> +        /* We will not auto probe raw files. */
>> +        error_set(errp, QERR_MISSING_PARAMETER, "format");
>> +        return;
>> +    }
>> +
>> +    eject_device(bs, 0,&err);
>> +    if (err) {
>> +        error_propagate(errp, err);
>> +        return;
>> +    }
>> +
>> +    qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, password, errp);
>> +}
>
> Can't this share code with the old command? I don't like this duplication.

qmp_bdrv_open_encrypted() is the code that can be shared.  The next 
patch I think changes the old command to use this function.

Regards,

Anthony Liguori

> Kevin
>

Re: [Qemu-devel] [PATCH 12/14] qapi: introduce change-blockdev

[Kevin Wolf]

Am 25.08.2011 14:56, schrieb Anthony Liguori:
> On 08/25/2011 07:46 AM, Kevin Wolf wrote:
>> Am 24.08.2011 20:43, schrieb Anthony Liguori:
>>> A new QMP only command to change the blockdev associated with a block device.
>>> The semantics of change right now are just plain scary.  This command introduces
>>> sane semantics.  For more details, see the documentation in the schema file.
>>
>> Changes to semantics should be mentioned in the commit message, and in
>> more detail than just that they are sane (the documentation in the
>> schema file doesn't describe differences to the old command).
> 
> Right, but the schema describes the old command.
> 
> There are a couple major differences with the new command vs old:
> 
> 1) If you had a block device named 'vnc', the old command is broken.
> 
> 2) For an encrypted device, the old change command returns an error but 
> has semantically completed the operation.  The usage is:
> 
> (qemu) change ide0 foo.img
> Error: device ide0 is encrypted
> (qemu) block_passwd ide0 secret
> 
> Because even though an error is returned, the change operation has 
> actually succeeded.  The new command has transactional semantics.  If an 
> error is returned, nothing has happened.  To set passwords, an 
> additional option is supported to specify the password.  So you would do:
> 
> (qemu) change-blockdev ide0 foo.img
> Error: device ide0 is encrypted
> (qemu) change-blockdev ide0 foo.img secret
> (qemu)

Does it really work like this? We need to improve the HMP implementation
to ask for the password when needed instead of requiring users to put it
as plain text in their command.

> In addition, change-blockdev will not allow you to perform unsafe 
> probing.  You can only change to a raw device if you specify a raw format.

Makes sense for QMP, but I would consider it a bug for HMP. So HMP code
should deal with it and allow using raw without specifying the format.

qemu is getting worse and worse to use without a management tool. :-(

Kevin

Re: [Qemu-devel] [PATCH 12/14] qapi: introduce change-blockdev

[Anthony Liguori]

On 08/25/2011 08:47 AM, Kevin Wolf wrote:
> Am 25.08.2011 14:56, schrieb Anthony Liguori:
>> On 08/25/2011 07:46 AM, Kevin Wolf wrote:
>>> Am 24.08.2011 20:43, schrieb Anthony Liguori:
>>>> A new QMP only command to change the blockdev associated with a block device.
>>>> The semantics of change right now are just plain scary.  This command introduces
>>>> sane semantics.  For more details, see the documentation in the schema file.
>>>
>>> Changes to semantics should be mentioned in the commit message, and in
>>> more detail than just that they are sane (the documentation in the
>>> schema file doesn't describe differences to the old command).
>>
>> Right, but the schema describes the old command.
>>
>> There are a couple major differences with the new command vs old:
>>
>> 1) If you had a block device named 'vnc', the old command is broken.
>>
>> 2) For an encrypted device, the old change command returns an error but
>> has semantically completed the operation.  The usage is:
>>
>> (qemu) change ide0 foo.img
>> Error: device ide0 is encrypted
>> (qemu) block_passwd ide0 secret
>>
>> Because even though an error is returned, the change operation has
>> actually succeeded.  The new command has transactional semantics.  If an
>> error is returned, nothing has happened.  To set passwords, an
>> additional option is supported to specify the password.  So you would do:
>>
>> (qemu) change-blockdev ide0 foo.img
>> Error: device ide0 is encrypted
>> (qemu) change-blockdev ide0 foo.img secret
>> (qemu)
>
> Does it really work like this? We need to improve the HMP implementation
> to ask for the password when needed instead of requiring users to put it
> as plain text in their command.

I didn't add an HMP version of change-blockdev.  change is fine for HMP.

And change does prompt for password both for VNC and block.  The main 
reason for introducing change-blockdev because the only way I could make 
the HMP change command work and be implemented in terms of QMP was to 
use the change-blockdev command.

>
>> In addition, change-blockdev will not allow you to perform unsafe
>> probing.  You can only change to a raw device if you specify a raw format.
>
> Makes sense for QMP, but I would consider it a bug for HMP. So HMP code
> should deal with it and allow using raw without specifying the format.

Yup, that's how change behaves.

> qemu is getting worse and worse to use without a management tool. :-(

One thing that's different now is that HMP and QMP are fundamentally 
different interfaces.  I hope this gives us the flexibility to implement 
better HMP commands that we would never expose in QMP and vice versa.

As long as HMP commands are implemented in terms of QMP commands, we 
should be very open and aggressive about improving HMP usablity.

Regards,

Anthony Liguori

>
> Kevin
>

Re: [Qemu-devel] [PATCH 12/14] qapi: introduce change-blockdev

[Luiz Capitulino]

On Wed, 24 Aug 2011 13:43:07 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> A new QMP only command to change the blockdev associated with a block device.
> The semantics of change right now are just plain scary.  This command introduces
> sane semantics.  For more details, see the documentation in the schema file.

IMO, this has to be split into two commits. First you introduce the new command
and then you fix do_change_block().

Also, there's a small problem with the generated code: it has to return -1 on
errors. The monitor expects a -1 return _and_ a qerror object on errors. The
generated code in middle mode is returning 0 even on errors, which makes QMP to
emit a UndefinedError error by default.

(it's probably not worth it to discuss the merits of having two ways of
signaling errors in the monitor, as this is all going to be dropped pretty soon).

> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  blockdev.c       |  108 +++++++++++++++++++++++++++++++++++++++++++++++++++---
>  qapi-schema.json |   30 +++++++++++++++
>  qmp-commands.hx  |    8 ++++
>  3 files changed, 140 insertions(+), 6 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 37b2f29..c00c69d 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -697,12 +697,101 @@ void qmp_block_passwd(const char *device, const char *password, Error **err)
>      qmp_set_blockdev_password(device, password, err);
>  }
>  
> +static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
> +                                    int bdrv_flags, BlockDriver *drv,
> +                                    const char *password, Error **errp)
> +{
> +    if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) {
> +        error_set(errp, QERR_OPEN_FILE_FAILED, filename);
> +        return;
> +    }
> +
> +    if (bdrv_key_required(bs)) {
> +        if (password) {
> +            if (bdrv_set_key(bs, password) < 0) {
> +                error_set(errp, QERR_INVALID_PASSWORD);
> +            }
> +        } else {
> +            error_set(errp, QERR_DEVICE_ENCRYPTED, bdrv_get_device_name(bs),
> +                      bdrv_get_encrypted_filename(bs));
> +        }
> +    } else if (password) {
> +        error_set(errp, QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs));
> +    }
> +}
> +
> +void qmp_change_blockdev(const char *device, const char *filename,
> +                         bool has_format, const char *format,
> +                         bool has_password, const char *password,
> +                         Error **errp)
> +{
> +    BlockDriverState *bs, *bs1;
> +    BlockDriver *drv = NULL;
> +    int bdrv_flags;
> +    Error *err = NULL;
> +    bool probed_raw = false;
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    if (has_format) {
> +        drv = bdrv_find_whitelisted_format(format);
> +        if (!drv) {
> +            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
> +            return;
> +        }
> +    }
> +
> +    bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
> +    bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
> +
> +    if (!has_password) {
> +        password = NULL;
> +    }
> +
> +    /* Try to open once with a temporary block device to make sure that
> +     * the disk isn't encrypted and we lack a key.  This also helps keep
> +     * this operation as a transaction.  That is, if we fail, we try very
> +     * hard to make sure that the state is the same as before the operation
> +     * was started.
> +     */
> +    bs1 = bdrv_new("");
> +    qmp_bdrv_open_encrypted(bs1, filename, bdrv_flags, drv, password, &err);
> +    if (!has_format && bs1->drv->unsafe_probe) {
> +        probed_raw = true;
> +    }
> +    bdrv_delete(bs1);
> +
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +
> +    if (probed_raw) {
> +        /* We will not auto probe raw files. */
> +        error_set(errp, QERR_MISSING_PARAMETER, "format");
> +        return;
> +    }
> +
> +    eject_device(bs, 0, &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +
> +    qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, password, errp);
> +}
> +
>  int do_change_block(Monitor *mon, const char *device,
>                      const char *filename, const char *fmt)
>  {
>      BlockDriverState *bs;
>      BlockDriver *drv = NULL;
>      int bdrv_flags;
> +    Error *err = NULL;
>  
>      bs = bdrv_find(device);
>      if (!bs) {
> @@ -716,16 +805,23 @@ int do_change_block(Monitor *mon, const char *device,
>              return -1;
>          }
>      }
> -    if (eject_device(bs, 0, NULL) < 0) {
> -        return -1;
> -    }
> +
>      bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
>      bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
> -    if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) {
> -        qerror_report(QERR_OPEN_FILE_FAILED, filename);
> +
> +    eject_device(bs, 0, &err);
> +    if (err) {
> +        qerror_report_err(err);
> +        return -1;
> +    }
> +
> +    qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, &err);
> +    if (err) {
> +        qerror_report_err(err);
>          return -1;
>      }
> -    return monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
> +
> +    return 0;
>  }
>  
>  int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 211200a..139c6e3 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -123,3 +123,33 @@
>  #         when this command is executed is undefined.
>  ##
>  { 'command': 'change-vnc-listen', 'data': {'target': 'str'} }
> +
> +##
> +# @change-blockdev:
> +#
> +# This is the preferred interface for changing a block device.
> +#
> +# @device:   The block device name.
> +#
> +# @filename: The new filename for the block device.  This may contain options
> +#            encoded in a format specified by @format.
> +#
> +# @format:   #optional The format to use open the block device

We need a list (or a pointers) of valid formats.

> +#
> +# @password: #optional The password to use if the block device is encrypted
> +#
> +# Returns:  Nothing on success.
> +#          If @device is not a valid block device, DeviceNotFound
> +#          If @format is not a valid block format, InvalidBlockFormat
> +#          If @filename is encrypted and @password isn't supplied,
> +#            DeviceEncrypted.  The call should be repeated with @password
> +#            supplied.
> +#          If @format is not specified and @filename is a format that cannot
> +#            be safely probed, MissingParameter.
> +#          If @filename cannot be opened, OpenFileFailed
> +#
> +# Since: 1.0
> +##
> +{ 'command': 'change-blockdev',
> +  'data': {'device': 'str', 'filename': 'str', '*format': 'str',
> +           '*password': 'str'} }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index c0d0ca3..cec7135 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -121,6 +121,14 @@ EQMP
>          .mhandler.cmd_new = do_change,
>      },
>  
> +    {
> +        .name       = "change-blockdev",
> +        .args_type  = "device:B,target:F,arg:s?pass:s?",

The arguments names don't match the schema.

> +        .params     = "device filename [format] [password]",
> +        .help       = "change a removable medium, optional format",
> +        .mhandler.cmd_new = qmp_marshal_input_change_blockdev,
> +    },
> +
>  SQMP
>  change
>  ------

Re: [Qemu-devel] [PATCH 12/14] qapi: introduce change-blockdev

[Anthony Liguori]

On 08/25/2011 09:09 AM, Luiz Capitulino wrote:
> On Wed, 24 Aug 2011 13:43:07 -0500
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>
>> A new QMP only command to change the blockdev associated with a block device.
>> The semantics of change right now are just plain scary.  This command introduces
>> sane semantics.  For more details, see the documentation in the schema file.
>
> IMO, this has to be split into two commits. First you introduce the new command
> and then you fix do_change_block().
>
> Also, there's a small problem with the generated code: it has to return -1 on
> errors. The monitor expects a -1 return _and_ a qerror object on errors. The
> generated code in middle mode is returning 0 even on errors, which makes QMP to
> emit a UndefinedError error by default.

You mean:

     if (local_err) {
         qerror_report_err(local_err);
         error_free(local_err);
         return -1;
     }
     return 0;

That should behave exactly the right way as qerror_report_err() 
propagates sends the Error as a QError.

>> +
>> +##
>> +# @change-blockdev:
>> +#
>> +# This is the preferred interface for changing a block device.
>> +#
>> +# @device:   The block device name.
>> +#
>> +# @filename: The new filename for the block device.  This may contain options
>> +#            encoded in a format specified by @format.
>> +#
>> +# @format:   #optional The format to use open the block device
>
> We need a list (or a pointers) of valid formats.

We do, but that needs to be a command because it depends on the build 
options.

>> +#
>> +# @password: #optional The password to use if the block device is encrypted
>> +#
>> +# Returns:  Nothing on success.
>> +#          If @device is not a valid block device, DeviceNotFound
>> +#          If @format is not a valid block format, InvalidBlockFormat
>> +#          If @filename is encrypted and @password isn't supplied,
>> +#            DeviceEncrypted.  The call should be repeated with @password
>> +#            supplied.
>> +#          If @format is not specified and @filename is a format that cannot
>> +#            be safely probed, MissingParameter.
>> +#          If @filename cannot be opened, OpenFileFailed
>> +#
>> +# Since: 1.0
>> +##
>> +{ 'command': 'change-blockdev',
>> +  'data': {'device': 'str', 'filename': 'str', '*format': 'str',
>> +           '*password': 'str'} }
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index c0d0ca3..cec7135 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -121,6 +121,14 @@ EQMP
>>           .mhandler.cmd_new = do_change,
>>       },
>>
>> +    {
>> +        .name       = "change-blockdev",
>> +        .args_type  = "device:B,target:F,arg:s?pass:s?",
>
> The arguments names don't match the schema.

Do we need to set args_type for QMP?

Regards,

Anthony Liguori


>> +        .params     = "device filename [format] [password]",
>> +        .help       = "change a removable medium, optional format",
>> +        .mhandler.cmd_new = qmp_marshal_input_change_blockdev,
>> +    },
>> +
>>   SQMP
>>   change
>>   ------
>

Re: [Qemu-devel] [PATCH 12/14] qapi: introduce change-blockdev

[Luiz Capitulino]

On Thu, 25 Aug 2011 09:21:02 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> On 08/25/2011 09:09 AM, Luiz Capitulino wrote:
> > On Wed, 24 Aug 2011 13:43:07 -0500
> > Anthony Liguori<aliguori@us.ibm.com>  wrote:
> >
> >> A new QMP only command to change the blockdev associated with a block device.
> >> The semantics of change right now are just plain scary.  This command introduces
> >> sane semantics.  For more details, see the documentation in the schema file.
> >
> > IMO, this has to be split into two commits. First you introduce the new command
> > and then you fix do_change_block().
> >
> > Also, there's a small problem with the generated code: it has to return -1 on
> > errors. The monitor expects a -1 return _and_ a qerror object on errors. The
> > generated code in middle mode is returning 0 even on errors, which makes QMP to
> > emit a UndefinedError error by default.
> 
> You mean:
> 
>      if (local_err) {
>          qerror_report_err(local_err);
>          error_free(local_err);
>          return -1;
>      }
>      return 0;
> 
> That should behave exactly the right way as qerror_report_err() 
> propagates sends the Error as a QError.

Oh that's right, what's happening is that qerror_report_err() is returning
before setting qerror in the Monitor object... I reported that in my
review of patch 01/14.

> 
> >> +
> >> +##
> >> +# @change-blockdev:
> >> +#
> >> +# This is the preferred interface for changing a block device.
> >> +#
> >> +# @device:   The block device name.
> >> +#
> >> +# @filename: The new filename for the block device.  This may contain options
> >> +#            encoded in a format specified by @format.
> >> +#
> >> +# @format:   #optional The format to use open the block device
> >
> > We need a list (or a pointers) of valid formats.
> 
> We do, but that needs to be a command because it depends on the build 
> options.
> 
> >> +#
> >> +# @password: #optional The password to use if the block device is encrypted
> >> +#
> >> +# Returns:  Nothing on success.
> >> +#          If @device is not a valid block device, DeviceNotFound
> >> +#          If @format is not a valid block format, InvalidBlockFormat
> >> +#          If @filename is encrypted and @password isn't supplied,
> >> +#            DeviceEncrypted.  The call should be repeated with @password
> >> +#            supplied.
> >> +#          If @format is not specified and @filename is a format that cannot
> >> +#            be safely probed, MissingParameter.
> >> +#          If @filename cannot be opened, OpenFileFailed
> >> +#
> >> +# Since: 1.0
> >> +##
> >> +{ 'command': 'change-blockdev',
> >> +  'data': {'device': 'str', 'filename': 'str', '*format': 'str',
> >> +           '*password': 'str'} }
> >> diff --git a/qmp-commands.hx b/qmp-commands.hx
> >> index c0d0ca3..cec7135 100644
> >> --- a/qmp-commands.hx
> >> +++ b/qmp-commands.hx
> >> @@ -121,6 +121,14 @@ EQMP
> >>           .mhandler.cmd_new = do_change,
> >>       },
> >>
> >> +    {
> >> +        .name       = "change-blockdev",
> >> +        .args_type  = "device:B,target:F,arg:s?pass:s?",
> >
> > The arguments names don't match the schema.
> 
> Do we need to set args_type for QMP?

Yes.

> 
> Regards,
> 
> Anthony Liguori
> 
> 
> >> +        .params     = "device filename [format] [password]",
> >> +        .help       = "change a removable medium, optional format",
> >> +        .mhandler.cmd_new = qmp_marshal_input_change_blockdev,
> >> +    },
> >> +
> >>   SQMP
> >>   change
> >>   ------
> >
> 

[Qemu-devel] [PATCH 13/14] qapi: convert change

[Anthony Liguori]

Convert the change command to QAPI and document all of its gloriousness.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 blockdev.c       |   27 ++++++++----------
 blockdev.h       |    6 +++-
 hmp-commands.hx  |    3 +-
 hmp.c            |   59 ++++++++++++++++++++++++++++++++++++++++
 hmp.h            |    1 +
 monitor.c        |   78 +++++++----------------------------------------------
 qapi-schema.json |   38 ++++++++++++++++++++++++++
 qmp-commands.hx  |    3 +-
 8 files changed, 127 insertions(+), 88 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index c00c69d..3832118 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -785,8 +785,9 @@ void qmp_change_blockdev(const char *device, const char *filename,
     qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, password, errp);
 }
 
-int do_change_block(Monitor *mon, const char *device,
-                    const char *filename, const char *fmt)
+void deprecated_qmp_change_blockdev(const char *device, const char *filename,
+                                    bool has_format, const char *format,
+                                    Error **errp)
 {
     BlockDriverState *bs;
     BlockDriver *drv = NULL;
@@ -795,14 +796,14 @@ int do_change_block(Monitor *mon, const char *device,
 
     bs = bdrv_find(device);
     if (!bs) {
-        qerror_report(QERR_DEVICE_NOT_FOUND, device);
-        return -1;
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
     }
-    if (fmt) {
-        drv = bdrv_find_whitelisted_format(fmt);
+    if (has_format) {
+        drv = bdrv_find_whitelisted_format(format);
         if (!drv) {
-            qerror_report(QERR_INVALID_BLOCK_FORMAT, fmt);
-            return -1;
+            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
+            return;
         }
     }
 
@@ -811,17 +812,13 @@ int do_change_block(Monitor *mon, const char *device,
 
     eject_device(bs, 0, &err);
     if (err) {
-        qerror_report_err(err);
-        return -1;
+        error_propagate(errp, err);
+        return;
     }
 
     qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, &err);
-    if (err) {
-        qerror_report_err(err);
-        return -1;
-    }
 
-    return 0;
+    error_propagate(errp, err);
 }
 
 int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
diff --git a/blockdev.h b/blockdev.h
index badbf01..54b142f 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -12,6 +12,7 @@
 
 #include "block.h"
 #include "qemu-queue.h"
+#include "error.h"
 
 void blockdev_mark_auto_del(BlockDriverState *bs);
 void blockdev_auto_del(BlockDriverState *bs);
@@ -59,8 +60,9 @@ DriveInfo *add_init_drive(const char *opts);
 
 void do_commit(Monitor *mon, const QDict *qdict);
 int do_block_set_passwd(Monitor *mon, const QDict *qdict, QObject **ret_data);
-int do_change_block(Monitor *mon, const char *device,
-                    const char *filename, const char *fmt);
+void deprecated_qmp_change_blockdev(const char *device, const char *filename,
+                                    bool has_format, const char *format,
+                                    Error **errp);
 int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int do_block_resize(Monitor *mon, const QDict *qdict, QObject **ret_data);
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 2f0ffa3..700d2f1 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -108,8 +108,7 @@ ETEXI
         .args_type  = "device:B,target:F,arg:s?",
         .params     = "device filename [format]",
         .help       = "change a removable medium, optional format",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_change,
+        .mhandler.cmd = hmp_change,
     },
 
 STEXI
diff --git a/hmp.c b/hmp.c
index a8ae36b..e8bca20 100644
--- a/hmp.c
+++ b/hmp.c
@@ -50,3 +50,62 @@ void hmp_block_passwd(Monitor *mon, const QDict *qdict)
         error_free(err);
     }
 }
+
+static void cb_hmp_change_bdrv_pwd(Monitor *mon, const char *password,
+                                   void *opaque)
+{
+    Error *encryption_err = opaque;
+    Error *err = NULL;
+    const char *device;
+
+    device = error_get_field(encryption_err, "device");
+
+    qmp_block_passwd(device, password, &err);
+    if (err) {
+        monitor_printf(mon, "invalid password\n");
+        error_free(err);
+    }
+
+    error_free(encryption_err);
+
+    monitor_read_command(mon, 1);
+}
+
+static void hmp_change_read_arg(Monitor *mon, const char *password,
+                                void *opaque)
+{
+    qmp_change_vnc_password(password, NULL);
+    monitor_read_command(mon, 1);
+}
+
+void hmp_change(Monitor *mon, const QDict *qdict)
+{
+    const char *device = qdict_get_str(qdict, "device");
+    const char *target = qdict_get_str(qdict, "target");
+    const char *arg = qdict_get_try_str(qdict, "arg");
+    Error *err = NULL;
+
+    if (strcmp(device, "vnc") == 0 &&
+        (strcmp(target, "passwd") == 0 ||
+         strcmp(target, "password") == 0)) {
+        if (arg == NULL) {
+            monitor_read_password(mon, hmp_change_read_arg, NULL);
+            return;
+        }
+    }
+
+    qmp_change(device, target, !!arg, arg, &err);
+    if (error_is_type(err, QERR_DEVICE_ENCRYPTED)) {
+        monitor_printf(mon, "%s (%s) is encrypted.\n",
+                       error_get_field(err, "device"),
+                       error_get_field(err, "encrypted_filename"));
+        if (!monitor_get_rs(mon)) {
+            monitor_printf(mon,
+                           "terminal does not support password prompting\n");
+            error_free(err);
+            return;
+        }
+        readline_start(monitor_get_rs(mon), "Password: ", 1,
+                       cb_hmp_change_bdrv_pwd, err);
+    }
+}
diff --git a/hmp.h b/hmp.h
index 8f72ef2..9df6ccc 100644
--- a/hmp.h
+++ b/hmp.h
@@ -20,5 +20,6 @@
 void hmp_info_name(Monitor *mon);
 void hmp_eject(Monitor *mon, const QDict *args);
 void hmp_block_passwd(Monitor *mon, const QDict *qdict);
+void hmp_change(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/monitor.c b/monitor.c
index 5cb36cd..9801a2d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1005,78 +1005,22 @@ static int do_quit(Monitor *mon, const QDict *qdict, QObject **ret_data)
     return 0;
 }
 
-#ifdef CONFIG_VNC
-static int change_vnc_password(const char *password)
-{
-    if (!password || !password[0]) {
-        if (vnc_display_disable_login(NULL)) {
-            qerror_report(QERR_SET_PASSWD_FAILED);
-            return -1;
-        }
-        return 0;
-    }
-
-    if (vnc_display_password(NULL, password) < 0) {
-        qerror_report(QERR_SET_PASSWD_FAILED);
-        return -1;
-    }
-
-    return 0;
-}
-
-static void change_vnc_password_cb(Monitor *mon, const char *password,
-                                   void *opaque)
+void qmp_change(const char *device, const char *target,
+                bool has_arg, const char *arg, Error **err)
 {
-    change_vnc_password(password);
-    monitor_read_command(mon, 1);
-}
-
-static int do_change_vnc(Monitor *mon, const char *target, const char *arg)
-{
-    if (strcmp(target, "passwd") == 0 ||
-        strcmp(target, "password") == 0) {
-        if (arg) {
-            char password[9];
-            strncpy(password, arg, sizeof(password));
-            password[sizeof(password) - 1] = '\0';
-            return change_vnc_password(password);
+    if (strcmp(device, "vnc") == 0) {
+        if (strcmp(target, "passwd") == 0 || strcmp(target, "password") == 0) {
+            if (!has_arg || !arg[0]) {
+                vnc_display_disable_login(NULL);
+            } else {
+                qmp_change_vnc_password(arg, err);
+            }
         } else {
-            return monitor_read_password(mon, change_vnc_password_cb, NULL);
+            qmp_change_vnc_listen(target, err);
         }
     } else {
-        if (vnc_display_open(NULL, target) < 0) {
-            qerror_report(QERR_VNC_SERVER_FAILED, target);
-            return -1;
-        }
+        deprecated_qmp_change_blockdev(device, target, has_arg, arg, err);
     }
-
-    return 0;
-}
-#else
-static int do_change_vnc(Monitor *mon, const char *target, const char *arg)
-{
-    qerror_report(QERR_FEATURE_DISABLED, "vnc");
-    return -ENODEV;
-}
-#endif
-
-/**
- * do_change(): Change a removable medium, or VNC configuration
- */
-static int do_change(Monitor *mon, const QDict *qdict, QObject **ret_data)
-{
-    const char *device = qdict_get_str(qdict, "device");
-    const char *target = qdict_get_str(qdict, "target");
-    const char *arg = qdict_get_try_str(qdict, "arg");
-    int ret;
-
-    if (strcmp(device, "vnc") == 0) {
-        ret = do_change_vnc(mon, target, arg);
-    } else {
-        ret = do_change_block(mon, device, target, arg);
-    }
-
-    return ret;
 }
 
 static int set_password(Monitor *mon, const QDict *qdict, QObject **ret_data)
diff --git a/qapi-schema.json b/qapi-schema.json
index 139c6e3..f108d20 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -84,6 +84,44 @@
   'data': {'device': 'str', 'password': 'str'} }
 
 ##
+# @change:
+#
+# This command is multiple commands multiplexed together.  Generally speaking,
+# it should not be used in favor of the single purpose alternatives such as
+# @change-vnc-listen, @change-vnc-password, and @change-blockdev.
+#
+# @device: This is normally the name of a block device but it may also be 'vnc'.
+#          when it's 'vnc', then sub command depends on @target
+#
+# @target: If @device is a block device, then this is the new filename.
+#          If @device is 'vnc', then if the value 'password' selects the vnc
+#          change password command.   Otherwise, this specifies a new server URI
+#          address to listen to for VNC connections.
+#
+# @arg:    If @device is a block device, then this is an optional format to open
+#          the device with.
+#          If @device is 'vnc' and @target is 'password', this is the new VNC
+#          password to set.  If this argument is an empty string, then no future
+#          logins will be allowed.
+#
+# Returns: Nothing on success.
+#          If @device is not a valid block device, DeviceNotFound
+#          If @format is not a valid block format, InvalidBlockFormat
+#          If the new block device is encrypted, DeviceEncrypted.  Note that
+#          if this error is returned, the device has been opened successfully
+#          and an additional call to @block_passwd is required to set the
+#          device's password.  The behavior of reads and writes to the block
+#          device between when these calls are executed is undefined.
+#
+# Notes:  It is strongly recommended that this interface is not used especially
+#         for changing block devices.
+#
+# Since: 0.14.0
+##
+{ 'command': 'change',
+  'data': {'device': 'str', 'target': 'str', '*arg': 'str'} }
+
+##
 # @set-blockdev-password:
 #
 # Alias for @block_passwd.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index cec7135..89b8d00 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -117,8 +117,7 @@ EQMP
         .args_type  = "device:B,target:F,arg:s?",
         .params     = "device filename [format]",
         .help       = "change a removable medium, optional format",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_change,
+        .mhandler.cmd_new = qmp_marshal_input_change,
     },
 
     {
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 13/14] qapi: convert change

[Luiz Capitulino]

On Wed, 24 Aug 2011 13:43:08 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> Convert the change command to QAPI and document all of its gloriousness.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  blockdev.c       |   27 ++++++++----------
>  blockdev.h       |    6 +++-
>  hmp-commands.hx  |    3 +-
>  hmp.c            |   59 ++++++++++++++++++++++++++++++++++++++++
>  hmp.h            |    1 +
>  monitor.c        |   78 +++++++----------------------------------------------
>  qapi-schema.json |   38 ++++++++++++++++++++++++++
>  qmp-commands.hx  |    3 +-
>  8 files changed, 127 insertions(+), 88 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index c00c69d..3832118 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -785,8 +785,9 @@ void qmp_change_blockdev(const char *device, const char *filename,
>      qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, password, errp);
>  }
>  
> -int do_change_block(Monitor *mon, const char *device,
> -                    const char *filename, const char *fmt)
> +void deprecated_qmp_change_blockdev(const char *device, const char *filename,
> +                                    bool has_format, const char *format,
> +                                    Error **errp)
>  {
>      BlockDriverState *bs;
>      BlockDriver *drv = NULL;
> @@ -795,14 +796,14 @@ int do_change_block(Monitor *mon, const char *device,
>  
>      bs = bdrv_find(device);
>      if (!bs) {
> -        qerror_report(QERR_DEVICE_NOT_FOUND, device);
> -        return -1;
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
>      }
> -    if (fmt) {
> -        drv = bdrv_find_whitelisted_format(fmt);
> +    if (has_format) {
> +        drv = bdrv_find_whitelisted_format(format);
>          if (!drv) {
> -            qerror_report(QERR_INVALID_BLOCK_FORMAT, fmt);
> -            return -1;
> +            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
> +            return;
>          }
>      }
>  
> @@ -811,17 +812,13 @@ int do_change_block(Monitor *mon, const char *device,
>  
>      eject_device(bs, 0, &err);
>      if (err) {
> -        qerror_report_err(err);
> -        return -1;
> +        error_propagate(errp, err);
> +        return;
>      }
>  
>      qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, &err);
> -    if (err) {
> -        qerror_report_err(err);
> -        return -1;
> -    }
>  
> -    return 0;
> +    error_propagate(errp, err);
>  }
>  
>  int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
> diff --git a/blockdev.h b/blockdev.h
> index badbf01..54b142f 100644
> --- a/blockdev.h
> +++ b/blockdev.h
> @@ -12,6 +12,7 @@
>  
>  #include "block.h"
>  #include "qemu-queue.h"
> +#include "error.h"
>  
>  void blockdev_mark_auto_del(BlockDriverState *bs);
>  void blockdev_auto_del(BlockDriverState *bs);
> @@ -59,8 +60,9 @@ DriveInfo *add_init_drive(const char *opts);
>  
>  void do_commit(Monitor *mon, const QDict *qdict);
>  int do_block_set_passwd(Monitor *mon, const QDict *qdict, QObject **ret_data);
> -int do_change_block(Monitor *mon, const char *device,
> -                    const char *filename, const char *fmt);
> +void deprecated_qmp_change_blockdev(const char *device, const char *filename,
> +                                    bool has_format, const char *format,
> +                                    Error **errp);
>  int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  int do_block_resize(Monitor *mon, const QDict *qdict, QObject **ret_data);
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 2f0ffa3..700d2f1 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -108,8 +108,7 @@ ETEXI
>          .args_type  = "device:B,target:F,arg:s?",
>          .params     = "device filename [format]",
>          .help       = "change a removable medium, optional format",
> -        .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = do_change,
> +        .mhandler.cmd = hmp_change,
>      },
>  
>  STEXI
> diff --git a/hmp.c b/hmp.c
> index a8ae36b..e8bca20 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -50,3 +50,62 @@ void hmp_block_passwd(Monitor *mon, const QDict *qdict)
>          error_free(err);
>      }
>  }
> +
> +static void cb_hmp_change_bdrv_pwd(Monitor *mon, const char *password,
> +                                   void *opaque)
> +{
> +    Error *encryption_err = opaque;
> +    Error *err = NULL;
> +    const char *device;
> +
> +    device = error_get_field(encryption_err, "device");
> +
> +    qmp_block_passwd(device, password, &err);
> +    if (err) {
> +        monitor_printf(mon, "invalid password\n");
> +        error_free(err);
> +    }
> +
> +    error_free(encryption_err);
> +
> +    monitor_read_command(mon, 1);
> +}
> +
> +static void hmp_change_read_arg(Monitor *mon, const char *password,
> +                                void *opaque)
> +{
> +    qmp_change_vnc_password(password, NULL);
> +    monitor_read_command(mon, 1);
> +}
> +
> +void hmp_change(Monitor *mon, const QDict *qdict)
> +{
> +    const char *device = qdict_get_str(qdict, "device");
> +    const char *target = qdict_get_str(qdict, "target");
> +    const char *arg = qdict_get_try_str(qdict, "arg");
> +    Error *err = NULL;
> +
> +    if (strcmp(device, "vnc") == 0 &&
> +        (strcmp(target, "passwd") == 0 ||
> +         strcmp(target, "password") == 0)) {
> +        if (arg == NULL) {
> +            monitor_read_password(mon, hmp_change_read_arg, NULL);
> +            return;
> +        }
> +    }
> +
> +    qmp_change(device, target, !!arg, arg, &err);
> +    if (error_is_type(err, QERR_DEVICE_ENCRYPTED)) {

You need to do:

if (err && error_is_type(err, QERR_DEVICE_ENCRYPTED)) {

Otherwise it segfaults. I'd also add a 'assert(err);' in error_is_type().
Or return false in error_is_type() is 'err' is NULL.

> +        monitor_printf(mon, "%s (%s) is encrypted.\n",
> +                       error_get_field(err, "device"),
> +                       error_get_field(err, "encrypted_filename"));
> +        if (!monitor_get_rs(mon)) {
> +            monitor_printf(mon,
> +                           "terminal does not support password prompting\n");
> +            error_free(err);
> +            return;
> +        }
> +        readline_start(monitor_get_rs(mon), "Password: ", 1,
> +                       cb_hmp_change_bdrv_pwd, err);
> +    }
> +}
> diff --git a/hmp.h b/hmp.h
> index 8f72ef2..9df6ccc 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -20,5 +20,6 @@
>  void hmp_info_name(Monitor *mon);
>  void hmp_eject(Monitor *mon, const QDict *args);
>  void hmp_block_passwd(Monitor *mon, const QDict *qdict);
> +void hmp_change(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/monitor.c b/monitor.c
> index 5cb36cd..9801a2d 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1005,78 +1005,22 @@ static int do_quit(Monitor *mon, const QDict *qdict, QObject **ret_data)
>      return 0;
>  }
>  
> -#ifdef CONFIG_VNC
> -static int change_vnc_password(const char *password)
> -{
> -    if (!password || !password[0]) {
> -        if (vnc_display_disable_login(NULL)) {
> -            qerror_report(QERR_SET_PASSWD_FAILED);
> -            return -1;
> -        }
> -        return 0;
> -    }
> -
> -    if (vnc_display_password(NULL, password) < 0) {
> -        qerror_report(QERR_SET_PASSWD_FAILED);
> -        return -1;
> -    }
> -
> -    return 0;
> -}
> -
> -static void change_vnc_password_cb(Monitor *mon, const char *password,
> -                                   void *opaque)
> +void qmp_change(const char *device, const char *target,
> +                bool has_arg, const char *arg, Error **err)
>  {
> -    change_vnc_password(password);
> -    monitor_read_command(mon, 1);
> -}
> -
> -static int do_change_vnc(Monitor *mon, const char *target, const char *arg)
> -{
> -    if (strcmp(target, "passwd") == 0 ||
> -        strcmp(target, "password") == 0) {
> -        if (arg) {
> -            char password[9];
> -            strncpy(password, arg, sizeof(password));
> -            password[sizeof(password) - 1] = '\0';
> -            return change_vnc_password(password);
> +    if (strcmp(device, "vnc") == 0) {
> +        if (strcmp(target, "passwd") == 0 || strcmp(target, "password") == 0) {
> +            if (!has_arg || !arg[0]) {
> +                vnc_display_disable_login(NULL);
> +            } else {
> +                qmp_change_vnc_password(arg, err);
> +            }
>          } else {
> -            return monitor_read_password(mon, change_vnc_password_cb, NULL);
> +            qmp_change_vnc_listen(target, err);
>          }
>      } else {
> -        if (vnc_display_open(NULL, target) < 0) {
> -            qerror_report(QERR_VNC_SERVER_FAILED, target);
> -            return -1;
> -        }
> +        deprecated_qmp_change_blockdev(device, target, has_arg, arg, err);
>      }
> -
> -    return 0;
> -}
> -#else
> -static int do_change_vnc(Monitor *mon, const char *target, const char *arg)
> -{
> -    qerror_report(QERR_FEATURE_DISABLED, "vnc");
> -    return -ENODEV;
> -}
> -#endif
> -
> -/**
> - * do_change(): Change a removable medium, or VNC configuration
> - */
> -static int do_change(Monitor *mon, const QDict *qdict, QObject **ret_data)
> -{
> -    const char *device = qdict_get_str(qdict, "device");
> -    const char *target = qdict_get_str(qdict, "target");
> -    const char *arg = qdict_get_try_str(qdict, "arg");
> -    int ret;
> -
> -    if (strcmp(device, "vnc") == 0) {
> -        ret = do_change_vnc(mon, target, arg);
> -    } else {
> -        ret = do_change_block(mon, device, target, arg);
> -    }
> -
> -    return ret;
>  }
>  
>  static int set_password(Monitor *mon, const QDict *qdict, QObject **ret_data)
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 139c6e3..f108d20 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -84,6 +84,44 @@
>    'data': {'device': 'str', 'password': 'str'} }
>  
>  ##
> +# @change:
> +#
> +# This command is multiple commands multiplexed together.  Generally speaking,
> +# it should not be used in favor of the single purpose alternatives such as
> +# @change-vnc-listen, @change-vnc-password, and @change-blockdev.
> +#
> +# @device: This is normally the name of a block device but it may also be 'vnc'.
> +#          when it's 'vnc', then sub command depends on @target
> +#
> +# @target: If @device is a block device, then this is the new filename.
> +#          If @device is 'vnc', then if the value 'password' selects the vnc
> +#          change password command.   Otherwise, this specifies a new server URI
> +#          address to listen to for VNC connections.
> +#
> +# @arg:    If @device is a block device, then this is an optional format to open
> +#          the device with.
> +#          If @device is 'vnc' and @target is 'password', this is the new VNC
> +#          password to set.  If this argument is an empty string, then no future
> +#          logins will be allowed.
> +#
> +# Returns: Nothing on success.
> +#          If @device is not a valid block device, DeviceNotFound
> +#          If @format is not a valid block format, InvalidBlockFormat
> +#          If the new block device is encrypted, DeviceEncrypted.  Note that
> +#          if this error is returned, the device has been opened successfully
> +#          and an additional call to @block_passwd is required to set the
> +#          device's password.  The behavior of reads and writes to the block
> +#          device between when these calls are executed is undefined.
> +#
> +# Notes:  It is strongly recommended that this interface is not used especially
> +#         for changing block devices.

I'd drop the 'especially' part.

> +#
> +# Since: 0.14.0
> +##
> +{ 'command': 'change',
> +  'data': {'device': 'str', 'target': 'str', '*arg': 'str'} }
> +
> +##
>  # @set-blockdev-password:
>  #
>  # Alias for @block_passwd.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index cec7135..89b8d00 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -117,8 +117,7 @@ EQMP
>          .args_type  = "device:B,target:F,arg:s?",
>          .params     = "device filename [format]",
>          .help       = "change a removable medium, optional format",
> -        .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = do_change,
> +        .mhandler.cmd_new = qmp_marshal_input_change,
>      },
>  
>      {

[Qemu-devel] [PATCH 14/14] vnc: don't demote authentication protocol when disabling login

[Anthony Liguori]

Currently when disabling login in VNC, the password is cleared out and the
authentication protocol is forced to AUTH_VNC.  If you're using a stronger
authentication protocol, this has the effect of downgrading your security
protocol.

Fix this by only changing the authentication protocol if the current
authentication protocol is AUTH_NONE.  That ensures we're never downgrading.

Reported-by: Daniel Berrange <berrange@redhat.com>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 monitor.c |   18 ------------------
 qmp.c     |   19 +++++++++++++++++++
 ui/vnc.c  |    4 +++-
 3 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/monitor.c b/monitor.c
index 9801a2d..ad73bc5 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1005,24 +1005,6 @@ static int do_quit(Monitor *mon, const QDict *qdict, QObject **ret_data)
     return 0;
 }
 
-void qmp_change(const char *device, const char *target,
-                bool has_arg, const char *arg, Error **err)
-{
-    if (strcmp(device, "vnc") == 0) {
-        if (strcmp(target, "passwd") == 0 || strcmp(target, "password") == 0) {
-            if (!has_arg || !arg[0]) {
-                vnc_display_disable_login(NULL);
-            } else {
-                qmp_change_vnc_password(arg, err);
-            }
-        } else {
-            qmp_change_vnc_listen(target, err);
-        }
-    } else {
-        deprecated_qmp_change_blockdev(device, target, has_arg, arg, err);
-    }
-}
-
 static int set_password(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     const char *protocol  = qdict_get_str(qdict, "protocol");
diff --git a/qmp.c b/qmp.c
index 73d6172..5674adc 100644
--- a/qmp.c
+++ b/qmp.c
@@ -16,6 +16,7 @@
 
 #include "sysemu.h"
 #include "console.h"
+#include "blockdev.h"
 
 NameInfo *qmp_query_name(Error **errp)
 {
@@ -42,3 +43,21 @@ void qmp_change_vnc_listen(const char *target, Error **err)
         error_set(err, QERR_VNC_SERVER_FAILED, target);
     }
 }
+
+void qmp_change(const char *device, const char *target,
+                bool has_arg, const char *arg, Error **err)
+{
+    if (strcmp(device, "vnc") == 0) {
+        if (strcmp(target, "passwd") == 0 || strcmp(target, "password") == 0) {
+            if (!has_arg || !arg[0]) {
+                vnc_display_disable_login(NULL);
+            } else {
+                qmp_change_vnc_password(arg, err);
+            }
+        } else {
+            qmp_change_vnc_listen(target, err);
+        }
+    } else {
+        deprecated_qmp_change_blockdev(device, target, has_arg, arg, err);
+    }
+}
diff --git a/ui/vnc.c b/ui/vnc.c
index fc3a612..ecb216f 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2648,7 +2648,9 @@ int vnc_display_disable_login(DisplayState *ds)
     }
 
     vs->password = NULL;
-    vs->auth = VNC_AUTH_VNC;
+    if (vs->auth == VNC_AUTH_NONE) {
+        vs->auth = VNC_AUTH_VNC;
+    }
 
     return 0;
 }
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 14/14] vnc: don't demote authentication protocol when disabling login

[Daniel P. Berrange]

On Wed, Aug 24, 2011 at 01:43:09PM -0500, Anthony Liguori wrote:
> Currently when disabling login in VNC, the password is cleared out and the
> authentication protocol is forced to AUTH_VNC.  If you're using a stronger
> authentication protocol, this has the effect of downgrading your security
> protocol.
> 
> Fix this by only changing the authentication protocol if the current
> authentication protocol is AUTH_NONE.  That ensures we're never downgrading.
> 
> Reported-by: Daniel Berrange <berrange@redhat.com>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  monitor.c |   18 ------------------
>  qmp.c     |   19 +++++++++++++++++++
>  ui/vnc.c  |    4 +++-
>  3 files changed, 22 insertions(+), 19 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 9801a2d..ad73bc5 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1005,24 +1005,6 @@ static int do_quit(Monitor *mon, const QDict *qdict, QObject **ret_data)
>      return 0;
>  }
>  
> -void qmp_change(const char *device, const char *target,
> -                bool has_arg, const char *arg, Error **err)
> -{
> -    if (strcmp(device, "vnc") == 0) {
> -        if (strcmp(target, "passwd") == 0 || strcmp(target, "password") == 0) {
> -            if (!has_arg || !arg[0]) {
> -                vnc_display_disable_login(NULL);
> -            } else {
> -                qmp_change_vnc_password(arg, err);
> -            }
> -        } else {
> -            qmp_change_vnc_listen(target, err);
> -        }
> -    } else {
> -        deprecated_qmp_change_blockdev(device, target, has_arg, arg, err);
> -    }
> -}
> -
>  static int set_password(Monitor *mon, const QDict *qdict, QObject **ret_data)
>  {
>      const char *protocol  = qdict_get_str(qdict, "protocol");
> diff --git a/qmp.c b/qmp.c
> index 73d6172..5674adc 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -16,6 +16,7 @@
>  
>  #include "sysemu.h"
>  #include "console.h"
> +#include "blockdev.h"
>  
>  NameInfo *qmp_query_name(Error **errp)
>  {
> @@ -42,3 +43,21 @@ void qmp_change_vnc_listen(const char *target, Error **err)
>          error_set(err, QERR_VNC_SERVER_FAILED, target);
>      }
>  }
> +
> +void qmp_change(const char *device, const char *target,
> +                bool has_arg, const char *arg, Error **err)
> +{
> +    if (strcmp(device, "vnc") == 0) {
> +        if (strcmp(target, "passwd") == 0 || strcmp(target, "password") == 0) {
> +            if (!has_arg || !arg[0]) {
> +                vnc_display_disable_login(NULL);
> +            } else {
> +                qmp_change_vnc_password(arg, err);
> +            }
> +        } else {
> +            qmp_change_vnc_listen(target, err);
> +        }
> +    } else {
> +        deprecated_qmp_change_blockdev(device, target, has_arg, arg, err);
> +    }
> +}
> diff --git a/ui/vnc.c b/ui/vnc.c
> index fc3a612..ecb216f 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -2648,7 +2648,9 @@ int vnc_display_disable_login(DisplayState *ds)
>      }
>  
>      vs->password = NULL;
> -    vs->auth = VNC_AUTH_VNC;
> +    if (vs->auth == VNC_AUTH_NONE) {
> +        vs->auth = VNC_AUTH_VNC;
> +    }
>  
>      return 0;
>  }

Thanks for making this change. The same also needs to be done in the
'vnc_display_password()' method.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 14/14] vnc: don't demote authentication protocol when disabling login

[Anthony Liguori]

On 08/24/2011 03:45 PM, Daniel P. Berrange wrote:
> On Wed, Aug 24, 2011 at 01:43:09PM -0500, Anthony Liguori wrote:
>> Currently when disabling login in VNC, the password is cleared out and the
>> authentication protocol is forced to AUTH_VNC.  If you're using a stronger
>> authentication protocol, this has the effect of downgrading your security
>> protocol.
>>
>> Fix this by only changing the authentication protocol if the current
>> authentication protocol is AUTH_NONE.  That ensures we're never downgrading.
>>
>> Reported-by: Daniel Berrange<berrange@redhat.com>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>> ---
>>   monitor.c |   18 ------------------
>>   qmp.c     |   19 +++++++++++++++++++
>>   ui/vnc.c  |    4 +++-
>>   3 files changed, 22 insertions(+), 19 deletions(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index 9801a2d..ad73bc5 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -1005,24 +1005,6 @@ static int do_quit(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>       return 0;
>>   }
>>
>> -void qmp_change(const char *device, const char *target,
>> -                bool has_arg, const char *arg, Error **err)
>> -{
>> -    if (strcmp(device, "vnc") == 0) {
>> -        if (strcmp(target, "passwd") == 0 || strcmp(target, "password") == 0) {
>> -            if (!has_arg || !arg[0]) {
>> -                vnc_display_disable_login(NULL);
>> -            } else {
>> -                qmp_change_vnc_password(arg, err);
>> -            }
>> -        } else {
>> -            qmp_change_vnc_listen(target, err);
>> -        }
>> -    } else {
>> -        deprecated_qmp_change_blockdev(device, target, has_arg, arg, err);
>> -    }
>> -}
>> -
>>   static int set_password(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>   {
>>       const char *protocol  = qdict_get_str(qdict, "protocol");
>> diff --git a/qmp.c b/qmp.c
>> index 73d6172..5674adc 100644
>> --- a/qmp.c
>> +++ b/qmp.c
>> @@ -16,6 +16,7 @@
>>
>>   #include "sysemu.h"
>>   #include "console.h"
>> +#include "blockdev.h"
>>
>>   NameInfo *qmp_query_name(Error **errp)
>>   {
>> @@ -42,3 +43,21 @@ void qmp_change_vnc_listen(const char *target, Error **err)
>>           error_set(err, QERR_VNC_SERVER_FAILED, target);
>>       }
>>   }
>> +
>> +void qmp_change(const char *device, const char *target,
>> +                bool has_arg, const char *arg, Error **err)
>> +{
>> +    if (strcmp(device, "vnc") == 0) {
>> +        if (strcmp(target, "passwd") == 0 || strcmp(target, "password") == 0) {
>> +            if (!has_arg || !arg[0]) {
>> +                vnc_display_disable_login(NULL);
>> +            } else {
>> +                qmp_change_vnc_password(arg, err);
>> +            }
>> +        } else {
>> +            qmp_change_vnc_listen(target, err);
>> +        }
>> +    } else {
>> +        deprecated_qmp_change_blockdev(device, target, has_arg, arg, err);
>> +    }
>> +}
>> diff --git a/ui/vnc.c b/ui/vnc.c
>> index fc3a612..ecb216f 100644
>> --- a/ui/vnc.c
>> +++ b/ui/vnc.c
>> @@ -2648,7 +2648,9 @@ int vnc_display_disable_login(DisplayState *ds)
>>       }
>>
>>       vs->password = NULL;
>> -    vs->auth = VNC_AUTH_VNC;
>> +    if (vs->auth == VNC_AUTH_NONE) {
>> +        vs->auth = VNC_AUTH_VNC;
>> +    }
>>
>>       return 0;
>>   }
>
> Thanks for making this change. The same also needs to be done in the
> 'vnc_display_password()' method.

Ack.

Regards,

Anthony Liguori

>
> Regards,
> Daniel

Re: [Qemu-devel] [PATCH 00/14] Convert commands to QAPI (batch 1)

[Luiz Capitulino]

On Wed, 24 Aug 2011 13:42:55 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> This is my attempt to jump-start the QAPI switch over.  All of the hard work
> is already done in my qapi branch, we just need to start merging stuff.
> 
> To simplify the merge process, I've introduced a new mode to the code generator
> that lets us do conversions without using the new QMP server.

It really makes the transition easier. It should be easy & quick to merge
the stuff in your repo.

> Once this series is merged, anything that touchs QMP (to modify a command or
> add a new command) must do it through QAPI--no exceptions.

Agreed. And people could start working on good replacements in parallel
with the merging of existing commands.

> 
> This series also includes Dans change to the 'change' command.  I'm not thrilled
> about people using this command but its better to err on the side of caution.
> It's now officially deprecated with much more robust replacement commands.
>