* [Qemu-devel] [PATCH v2 01/10] qapi: Separate type QNull from QObject
2017-07-20 7:53 [Qemu-devel] [PATCH v2 00/10] Correct two minor QMP interface design flaws Markus Armbruster
@ 2017-07-20 7:53 ` Markus Armbruster
2017-07-20 7:53 ` [Qemu-devel] [PATCH v2 02/10] qapi: Use QNull for a more regular visit_type_null() Markus Armbruster
` (9 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Markus Armbruster @ 2017-07-20 7:53 UTC (permalink / raw)
To: qemu-devel
Cc: eblake, berrange, kwolf, mreitz, qemu-block, quintela, dgilbert
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
---
include/qapi/qmp/qobject.h | 10 +++++++---
qapi/qobject-output-visitor.c | 2 +-
qobject/json-parser.c | 2 +-
qobject/qnull.c | 8 +++++---
target/i386/cpu.c | 4 ++--
tests/check-qjson.c | 6 +++---
tests/check-qnull.c | 18 +++++++++---------
7 files changed, 28 insertions(+), 22 deletions(-)
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index b8ddbca..3543b55 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -93,11 +93,15 @@ static inline QType qobject_type(const QObject *obj)
return obj->type;
}
-extern QObject qnull_;
+typedef struct QNull {
+ QObject base;
+} QNull;
-static inline QObject *qnull(void)
+extern QNull qnull_;
+
+static inline QNull *qnull(void)
{
- qobject_incref(&qnull_);
+ QINCREF(&qnull_);
return &qnull_;
}
diff --git a/qapi/qobject-output-visitor.c b/qapi/qobject-output-visitor.c
index 70be84c..398dcb5 100644
--- a/qapi/qobject-output-visitor.c
+++ b/qapi/qobject-output-visitor.c
@@ -190,7 +190,7 @@ static void qobject_output_type_any(Visitor *v, const char *name,
static void qobject_output_type_null(Visitor *v, const char *name, Error **errp)
{
QObjectOutputVisitor *qov = to_qov(v);
- qobject_output_add_obj(qov, name, qnull());
+ qobject_output_add(qov, name, qnull());
}
/* Finish building, and return the root object.
diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index 7a417f2..724ca24 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -445,7 +445,7 @@ static QObject *parse_keyword(JSONParserContext *ctxt)
} else if (!strcmp(token->str, "false")) {
return QOBJECT(qbool_from_bool(false));
} else if (!strcmp(token->str, "null")) {
- return qnull();
+ return QOBJECT(qnull());
}
parse_error(ctxt, token, "invalid keyword '%s'", token->str);
return NULL;
diff --git a/qobject/qnull.c b/qobject/qnull.c
index c124d05..69a21d1 100644
--- a/qobject/qnull.c
+++ b/qobject/qnull.c
@@ -14,7 +14,9 @@
#include "qemu-common.h"
#include "qapi/qmp/qobject.h"
-QObject qnull_ = {
- .type = QTYPE_QNULL,
- .refcnt = 1,
+QNull qnull_ = {
+ .base = {
+ .type = QTYPE_QNULL,
+ .refcnt = 1,
+ },
};
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 0bbda76..89f5fb7 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -2442,7 +2442,7 @@ static QDict *x86_cpu_static_props(void)
d = qdict_new();
for (i = 0; props[i]; i++) {
- qdict_put_obj(d, props[i], qnull());
+ qdict_put(d, props[i], qnull());
}
for (w = 0; w < FEATURE_WORDS; w++) {
@@ -2452,7 +2452,7 @@ static QDict *x86_cpu_static_props(void)
if (!fi->feat_names[bit]) {
continue;
}
- qdict_put_obj(d, fi->feat_names[bit], qnull());
+ qdict_put(d, fi->feat_names[bit], qnull());
}
}
diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index 53f2275..a3a97b0 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -1012,7 +1012,7 @@ static void keyword_literal(void)
{
QObject *obj;
QBool *qbool;
- QObject *null;
+ QNull *null;
QString *str;
obj = qobject_from_json("true", &error_abort);
@@ -1053,10 +1053,10 @@ static void keyword_literal(void)
g_assert(qobject_type(obj) == QTYPE_QNULL);
null = qnull();
- g_assert(null == obj);
+ g_assert(QOBJECT(null) == obj);
qobject_decref(obj);
- qobject_decref(null);
+ QDECREF(null);
}
typedef struct LiteralQDictEntry LiteralQDictEntry;
diff --git a/tests/check-qnull.c b/tests/check-qnull.c
index 8dd1c96..1ab7c98 100644
--- a/tests/check-qnull.c
+++ b/tests/check-qnull.c
@@ -24,14 +24,14 @@ static void qnull_ref_test(void)
{
QObject *obj;
- g_assert(qnull_.refcnt == 1);
- obj = qnull();
+ g_assert(qnull_.base.refcnt == 1);
+ obj = QOBJECT(qnull());
g_assert(obj);
- g_assert(obj == &qnull_);
- g_assert(qnull_.refcnt == 2);
+ g_assert(obj == QOBJECT(&qnull_));
+ g_assert(qnull_.base.refcnt == 2);
g_assert(qobject_type(obj) == QTYPE_QNULL);
qobject_decref(obj);
- g_assert(qnull_.refcnt == 1);
+ g_assert(qnull_.base.refcnt == 1);
}
static void qnull_visit_test(void)
@@ -45,8 +45,8 @@ static void qnull_visit_test(void)
* depend on layering violations to check qnull_ refcnt.
*/
- g_assert(qnull_.refcnt == 1);
- obj = qnull();
+ g_assert(qnull_.base.refcnt == 1);
+ obj = QOBJECT(qnull());
v = qobject_input_visitor_new(obj);
qobject_decref(obj);
visit_type_null(v, NULL, &error_abort);
@@ -55,11 +55,11 @@ static void qnull_visit_test(void)
v = qobject_output_visitor_new(&obj);
visit_type_null(v, NULL, &error_abort);
visit_complete(v, &obj);
- g_assert(obj == &qnull_);
+ g_assert(obj == QOBJECT(&qnull_));
qobject_decref(obj);
visit_free(v);
- g_assert(qnull_.refcnt == 1);
+ g_assert(qnull_.base.refcnt == 1);
}
int main(int argc, char **argv)
--
2.7.5
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH v2 02/10] qapi: Use QNull for a more regular visit_type_null()
2017-07-20 7:53 [Qemu-devel] [PATCH v2 00/10] Correct two minor QMP interface design flaws Markus Armbruster
2017-07-20 7:53 ` [Qemu-devel] [PATCH v2 01/10] qapi: Separate type QNull from QObject Markus Armbruster
@ 2017-07-20 7:53 ` Markus Armbruster
2017-07-20 7:53 ` [Qemu-devel] [PATCH v2 03/10] qapi: Introduce a first class 'null' type Markus Armbruster
` (8 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Markus Armbruster @ 2017-07-20 7:53 UTC (permalink / raw)
To: qemu-devel
Cc: eblake, berrange, kwolf, mreitz, qemu-block, quintela, dgilbert
Make visit_type_null() take an @obj argument like its buddies. This
helps keep the next commit simple.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
---
hw/ppc/spapr_drc.c | 4 +++-
include/qapi/visitor-impl.h | 3 ++-
include/qapi/visitor.h | 8 ++++----
qapi/qapi-clone-visitor.c | 5 +++--
qapi/qapi-dealloc-visitor.c | 6 +++++-
qapi/qapi-visit-core.c | 7 ++++---
qapi/qobject-input-visitor.c | 6 +++++-
qapi/qobject-output-visitor.c | 3 ++-
qapi/string-input-visitor.c | 8 +++++++-
qapi/string-output-visitor.c | 3 ++-
qapi/trace-events | 2 +-
target/ppc/translate_init.c | 5 ++++-
tests/check-qnull.c | 9 +++++++--
tests/test-qobject-input-visitor.c | 13 +++++++++----
tests/test-qobject-output-visitor.c | 3 ++-
15 files changed, 60 insertions(+), 25 deletions(-)
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 0ffcec6..15bae5c 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -297,12 +297,14 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj);
+ QNull *null = NULL;
Error *err = NULL;
int fdt_offset_next, fdt_offset, fdt_depth;
void *fdt;
if (!drc->fdt) {
- visit_type_null(v, NULL, errp);
+ visit_type_null(v, NULL, &null, errp);
+ QDECREF(null);
return;
}
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index dcd656a..8ccb3b6 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -103,7 +103,8 @@ struct Visitor
Error **errp);
/* Must be set to visit explicit null values. */
- void (*type_null)(Visitor *v, const char *name, Error **errp);
+ void (*type_null)(Visitor *v, const char *name, QNull **obj,
+ Error **errp);
/* Must be set for input visitors to visit structs, optional otherwise.
The core takes care of the return type in the public interface. */
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 74768aa..fe9faf4 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -618,10 +618,10 @@ void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp);
* @name expresses the relationship of the null value to its parent
* container; see the general description of @name above.
*
- * Unlike all other visit_type_* functions, no obj parameter is
- * needed; rather, this is a witness that an explicit null value is
- * expected rather than any other type.
+ * @obj must be non-NULL. Input visitors set *@obj to the value;
+ * other visitors ignore *@obj.
*/
-void visit_type_null(Visitor *v, const char *name, Error **errp);
+void visit_type_null(Visitor *v, const char *name, QNull **obj,
+ Error **errp);
#endif
diff --git a/qapi/qapi-clone-visitor.c b/qapi/qapi-clone-visitor.c
index ed16d3a..d8b6279 100644
--- a/qapi/qapi-clone-visitor.c
+++ b/qapi/qapi-clone-visitor.c
@@ -127,12 +127,13 @@ static void qapi_clone_type_number(Visitor *v, const char *name, double *obj,
/* Value was already cloned by g_memdup() */
}
-static void qapi_clone_type_null(Visitor *v, const char *name, Error **errp)
+static void qapi_clone_type_null(Visitor *v, const char *name, QNull **obj,
+ Error **errp)
{
QapiCloneVisitor *qcv = to_qcv(v);
assert(qcv->depth);
- /* Nothing to do */
+ *obj = qnull();
}
static void qapi_clone_free(Visitor *v)
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index fd6f9fb..ed70a01 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -103,8 +103,12 @@ static void qapi_dealloc_type_anything(Visitor *v, const char *name,
}
}
-static void qapi_dealloc_type_null(Visitor *v, const char *name, Error **errp)
+static void qapi_dealloc_type_null(Visitor *v, const char *name,
+ QNull **obj, Error **errp)
{
+ if (obj) {
+ QDECREF(*obj);
+ }
}
static void qapi_dealloc_free(Visitor *v)
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 935a2c5..ed6d2af 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -325,10 +325,11 @@ void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp)
error_propagate(errp, err);
}
-void visit_type_null(Visitor *v, const char *name, Error **errp)
+void visit_type_null(Visitor *v, const char *name, QNull **obj,
+ Error **errp)
{
- trace_visit_type_null(v, name);
- v->type_null(v, name, errp);
+ trace_visit_type_null(v, name, obj);
+ v->type_null(v, name, obj, errp);
}
static void output_type_enum(Visitor *v, const char *name, int *obj,
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 35aff78..ee9e47d 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -587,11 +587,13 @@ static void qobject_input_type_any(Visitor *v, const char *name, QObject **obj,
*obj = qobj;
}
-static void qobject_input_type_null(Visitor *v, const char *name, Error **errp)
+static void qobject_input_type_null(Visitor *v, const char *name,
+ QNull **obj, Error **errp)
{
QObjectInputVisitor *qiv = to_qiv(v);
QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
+ *obj = NULL;
if (!qobj) {
return;
}
@@ -599,7 +601,9 @@ static void qobject_input_type_null(Visitor *v, const char *name, Error **errp)
if (qobject_type(qobj) != QTYPE_QNULL) {
error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
full_name(qiv, name), "null");
+ return;
}
+ *obj = qnull();
}
static void qobject_input_type_size_keyval(Visitor *v, const char *name,
diff --git a/qapi/qobject-output-visitor.c b/qapi/qobject-output-visitor.c
index 398dcb5..d325163 100644
--- a/qapi/qobject-output-visitor.c
+++ b/qapi/qobject-output-visitor.c
@@ -187,7 +187,8 @@ static void qobject_output_type_any(Visitor *v, const char *name,
qobject_output_add_obj(qov, name, *obj);
}
-static void qobject_output_type_null(Visitor *v, const char *name, Error **errp)
+static void qobject_output_type_null(Visitor *v, const char *name,
+ QNull **obj, Error **errp)
{
QObjectOutputVisitor *qov = to_qov(v);
qobject_output_add(qov, name, qnull());
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index 63ae115..67a0a4a 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -326,14 +326,20 @@ static void parse_type_number(Visitor *v, const char *name, double *obj,
*obj = val;
}
-static void parse_type_null(Visitor *v, const char *name, Error **errp)
+static void parse_type_null(Visitor *v, const char *name, QNull **obj,
+ Error **errp)
{
StringInputVisitor *siv = to_siv(v);
+ *obj = NULL;
+
if (!siv->string || siv->string[0]) {
error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"null");
+ return;
}
+
+ *obj = qnull();
}
static void string_input_free(Visitor *v)
diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
index af649e1..7ab6446 100644
--- a/qapi/string-output-visitor.c
+++ b/qapi/string-output-visitor.c
@@ -256,7 +256,8 @@ static void print_type_number(Visitor *v, const char *name, double *obj,
string_output_set(sov, g_strdup_printf("%f", *obj));
}
-static void print_type_null(Visitor *v, const char *name, Error **errp)
+static void print_type_null(Visitor *v, const char *name, QNull **obj,
+ Error **errp)
{
StringOutputVisitor *sov = to_sov(v);
char *out;
diff --git a/qapi/trace-events b/qapi/trace-events
index 3b57aba..9e9008a 100644
--- a/qapi/trace-events
+++ b/qapi/trace-events
@@ -31,4 +31,4 @@ visit_type_bool(void *v, const char *name, bool *obj) "v=%p name=%s obj=%p"
visit_type_str(void *v, const char *name, char **obj) "v=%p name=%s obj=%p"
visit_type_number(void *v, const char *name, double *obj) "v=%p name=%s obj=%p"
visit_type_any(void *v, const char *name, void *obj) "v=%p name=%s obj=%p"
-visit_type_null(void *v, const char *name) "v=%p name=%s"
+visit_type_null(void *v, const char *name, void *obj) "v=%p name=%s obj=%p"
diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
index b325c2c..01723bd 100644
--- a/target/ppc/translate_init.c
+++ b/target/ppc/translate_init.c
@@ -8428,11 +8428,14 @@ POWERPC_FAMILY(POWER5P)(ObjectClass *oc, void *data)
static void getset_compat_deprecated(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
+ QNull *null = NULL;
+
if (!qtest_enabled()) {
error_report("CPU 'compat' property is deprecated and has no effect; "
"use max-cpu-compat machine property instead");
}
- visit_type_null(v, name, NULL);
+ visit_type_null(v, name, &null, NULL);
+ QDECREF(null);
}
static const PropertyInfo ppc_compat_deprecated_propinfo = {
diff --git a/tests/check-qnull.c b/tests/check-qnull.c
index 1ab7c98..5c6eb0a 100644
--- a/tests/check-qnull.c
+++ b/tests/check-qnull.c
@@ -38,6 +38,7 @@ static void qnull_visit_test(void)
{
QObject *obj;
Visitor *v;
+ QNull *null;
/*
* Most tests of interactions between QObject and visitors are in
@@ -49,13 +50,17 @@ static void qnull_visit_test(void)
obj = QOBJECT(qnull());
v = qobject_input_visitor_new(obj);
qobject_decref(obj);
- visit_type_null(v, NULL, &error_abort);
+ visit_type_null(v, NULL, &null, &error_abort);
+ g_assert(obj == QOBJECT(&qnull_));
+ QDECREF(null);
visit_free(v);
+ null = NULL;
v = qobject_output_visitor_new(&obj);
- visit_type_null(v, NULL, &error_abort);
+ visit_type_null(v, NULL, &null, &error_abort);
visit_complete(v, &obj);
g_assert(obj == QOBJECT(&qnull_));
+ QDECREF(null);
qobject_decref(obj);
visit_free(v);
diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c
index 34bab8a..f98caf9 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -510,6 +510,7 @@ static void test_visitor_in_null(TestInputVisitorData *data,
{
Visitor *v;
Error *err = NULL;
+ QNull *null;
char *tmp;
/*
@@ -524,12 +525,15 @@ static void test_visitor_in_null(TestInputVisitorData *data,
v = visitor_input_test_init_full(data, false,
"{ 'a': null, 'b': '' }");
visit_start_struct(v, NULL, NULL, 0, &error_abort);
- visit_type_null(v, "a", &error_abort);
- visit_type_null(v, "b", &err);
+ visit_type_null(v, "a", &null, &error_abort);
+ g_assert(qobject_type(QOBJECT(null)) == QTYPE_QNULL);
+ QDECREF(null);
+ visit_type_null(v, "b", &null, &err);
error_free_or_abort(&err);
+ g_assert(!null);
visit_type_str(v, "c", &tmp, &err);
- g_assert(!tmp);
error_free_or_abort(&err);
+ g_assert(!tmp);
visit_check_struct(v, &error_abort);
visit_end_struct(v, NULL);
}
@@ -1087,6 +1091,7 @@ static void test_visitor_in_fail_struct_missing(TestInputVisitorData *data,
Error *err = NULL;
Visitor *v;
QObject *any;
+ QNull *null;
GenericAlternate *alt;
bool present;
int en;
@@ -1120,7 +1125,7 @@ static void test_visitor_in_fail_struct_missing(TestInputVisitorData *data,
error_free_or_abort(&err);
visit_type_any(v, "any", &any, &err);
error_free_or_abort(&err);
- visit_type_null(v, "null", &err);
+ visit_type_null(v, "null", &null, &err);
error_free_or_abort(&err);
visit_start_list(v, "sub", NULL, 0, &error_abort);
visit_start_struct(v, NULL, NULL, 0, &error_abort);
diff --git a/tests/test-qobject-output-visitor.c b/tests/test-qobject-output-visitor.c
index 749c540..8f1fcd4 100644
--- a/tests/test-qobject-output-visitor.c
+++ b/tests/test-qobject-output-visitor.c
@@ -445,11 +445,12 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
static void test_visitor_out_null(TestOutputVisitorData *data,
const void *unused)
{
+ QNull *null = NULL;
QDict *qdict;
QObject *nil;
visit_start_struct(data->ov, NULL, NULL, 0, &error_abort);
- visit_type_null(data->ov, "a", &error_abort);
+ visit_type_null(data->ov, "a", &null, &error_abort);
visit_check_struct(data->ov, &error_abort);
visit_end_struct(data->ov, NULL);
qdict = qobject_to_qdict(visitor_get(data));
--
2.7.5
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH v2 03/10] qapi: Introduce a first class 'null' type
2017-07-20 7:53 [Qemu-devel] [PATCH v2 00/10] Correct two minor QMP interface design flaws Markus Armbruster
2017-07-20 7:53 ` [Qemu-devel] [PATCH v2 01/10] qapi: Separate type QNull from QObject Markus Armbruster
2017-07-20 7:53 ` [Qemu-devel] [PATCH v2 02/10] qapi: Use QNull for a more regular visit_type_null() Markus Armbruster
@ 2017-07-20 7:53 ` Markus Armbruster
2017-07-20 7:53 ` [Qemu-devel] [PATCH v2 04/10] tests/test-qobject-input-visitor: Drop redundant test Markus Armbruster
` (7 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Markus Armbruster @ 2017-07-20 7:53 UTC (permalink / raw)
To: qemu-devel
Cc: eblake, berrange, kwolf, mreitz, qemu-block, quintela, dgilbert
I expect the 'null' type to be useful mostly for members of alternate
types.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
docs/devel/qapi-code-gen.txt | 10 ++++++----
include/qapi/qmp/qobject.h | 4 ++--
include/qemu/typedefs.h | 1 +
scripts/qapi.py | 5 ++++-
tests/qapi-schema/qapi-schema-test.json | 3 ++-
tests/qapi-schema/qapi-schema-test.out | 1 +
tests/test-qobject-input-visitor.c | 5 +++++
tests/test-qobject-output-visitor.c | 10 ++++++++++
8 files changed, 31 insertions(+), 8 deletions(-)
diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index 52e3874..9903ac4 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -282,6 +282,7 @@ The following types are predefined, and map to C as follows:
size uint64_t like uint64_t, except StringInputVisitor
accepts size suffixes
bool bool JSON true or false
+ null QNull * JSON null
any QObject * any JSON value
QType QType JSON string matching enum QType values
@@ -536,10 +537,11 @@ can only express a choice between types represented differently in
JSON. If a branch is typed as the 'bool' built-in, the alternate
accepts true and false; if it is typed as any of the various numeric
built-ins, it accepts a JSON number; if it is typed as a 'str'
-built-in or named enum type, it accepts a JSON string; and if it is
-typed as a complex type (struct or union), it accepts a JSON object.
-Two different complex types, for instance, aren't permitted, because
-both are represented as a JSON object.
+built-in or named enum type, it accepts a JSON string; if it is typed
+as the 'null' built-in, it accepts JSON null; and if it is typed as a
+complex type (struct or union), it accepts a JSON object. Two
+different complex types, for instance, aren't permitted, because both
+are represented as a JSON object.
The example alternate declaration above allows using both of the
following example objects:
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index 3543b55..eab29ed 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -93,9 +93,9 @@ static inline QType qobject_type(const QObject *obj)
return obj->type;
}
-typedef struct QNull {
+struct QNull {
QObject base;
-} QNull;
+};
extern QNull qnull_;
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 7b0d4e7..39bc835 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -89,6 +89,7 @@ typedef struct QEMUSGList QEMUSGList;
typedef struct QEMUTimer QEMUTimer;
typedef struct QEMUTimerListGroup QEMUTimerListGroup;
typedef struct QObject QObject;
+typedef struct QNull QNull;
typedef struct RAMBlock RAMBlock;
typedef struct Range Range;
typedef struct SerialState SerialState;
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 84e2eb4..8aa2775 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -20,6 +20,7 @@ import sys
from ordereddict import OrderedDict
builtin_types = {
+ 'null': 'QTYPE_QNULL',
'str': 'QTYPE_QSTRING',
'int': 'QTYPE_QNUM',
'number': 'QTYPE_QNUM',
@@ -1056,6 +1057,7 @@ class QAPISchemaType(QAPISchemaEntity):
def alternate_qtype(self):
json2qtype = {
+ 'null': 'QTYPE_QNULL',
'string': 'QTYPE_QSTRING',
'number': 'QTYPE_QNUM',
'int': 'QTYPE_QNUM',
@@ -1515,7 +1517,8 @@ class QAPISchema(object):
('uint64', 'int', 'uint64_t'),
('size', 'int', 'uint64_t'),
('bool', 'boolean', 'bool'),
- ('any', 'value', 'QObject' + pointer_suffix)]:
+ ('any', 'value', 'QObject' + pointer_suffix),
+ ('null', 'null', 'QNull' + pointer_suffix)]:
self._def_builtin_type(*t)
self.the_empty_object_type = QAPISchemaObjectType(
'q_empty', None, None, None, [], None)
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 91ffb26..c72dbd8 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -93,7 +93,8 @@
{ 'struct': 'WrapAlternate',
'data': { 'alt': 'UserDefAlternate' } }
{ 'alternate': 'UserDefAlternate',
- 'data': { 'udfu': 'UserDefFlatUnion', 'e': 'EnumOne', 'i': 'int' } }
+ 'data': { 'udfu': 'UserDefFlatUnion', 'e': 'EnumOne', 'i': 'int',
+ 'n': 'null' } }
{ 'struct': 'UserDefC',
'data': { 'string1': 'str', 'string2': 'str' } }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index b88b8aa..3b1e908 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -64,6 +64,7 @@ alternate UserDefAlternate
case udfu: UserDefFlatUnion
case e: EnumOne
case i: int
+ case n: null
object UserDefB
member intb: int optional=False
member a-b: bool optional=True
diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c
index f98caf9..f451844 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -583,6 +583,11 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
g_assert_cmpint(tmp->u.e, ==, ENUM_ONE_VALUE1);
qapi_free_UserDefAlternate(tmp);
+ v = visitor_input_test_init(data, "null");
+ visit_type_UserDefAlternate(v, NULL, &tmp, &error_abort);
+ g_assert_cmpint(tmp->type, ==, QTYPE_QNULL);
+ qapi_free_UserDefAlternate(tmp);
+
v = visitor_input_test_init(data, "{'integer':1, 'string':'str', "
"'enum1':'value1', 'boolean':true}");
visit_type_UserDefAlternate(v, NULL, &tmp, &error_abort);
diff --git a/tests/test-qobject-output-visitor.c b/tests/test-qobject-output-visitor.c
index 8f1fcd4..7eb1620 100644
--- a/tests/test-qobject-output-visitor.c
+++ b/tests/test-qobject-output-visitor.c
@@ -424,6 +424,16 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
visitor_reset(data);
tmp = g_new0(UserDefAlternate, 1);
+ tmp->type = QTYPE_QNULL;
+ tmp->u.n = qnull();
+
+ visit_type_UserDefAlternate(data->ov, NULL, &tmp, &error_abort);
+ g_assert_cmpint(qobject_type(visitor_get(data)), ==, QTYPE_QNULL);
+
+ qapi_free_UserDefAlternate(tmp);
+
+ visitor_reset(data);
+ tmp = g_new0(UserDefAlternate, 1);
tmp->type = QTYPE_QDICT;
tmp->u.udfu.integer = 1;
tmp->u.udfu.string = g_strdup("str");
--
2.7.5
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH v2 04/10] tests/test-qobject-input-visitor: Drop redundant test
2017-07-20 7:53 [Qemu-devel] [PATCH v2 00/10] Correct two minor QMP interface design flaws Markus Armbruster
` (2 preceding siblings ...)
2017-07-20 7:53 ` [Qemu-devel] [PATCH v2 03/10] qapi: Introduce a first class 'null' type Markus Armbruster
@ 2017-07-20 7:53 ` Markus Armbruster
2017-07-20 7:53 ` [Qemu-devel] [PATCH v2 05/10] block: Use JSON null instead of "" to disable backing file Markus Armbruster
` (6 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Markus Armbruster @ 2017-07-20 7:53 UTC (permalink / raw)
To: qemu-devel
Cc: eblake, berrange, kwolf, mreitz, qemu-block, quintela, dgilbert
test_visitor_in_alternate() tests UserDefAlternate with inadmissible
input. test_visitor_in_fail_alternate() does basically the same.
Drop the former, keep the latter.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
---
tests/test-qobject-input-visitor.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c
index f451844..bcf0261 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -567,7 +567,6 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
const void *unused)
{
Visitor *v;
- Error *err = NULL;
UserDefAlternate *tmp;
WrapAlternate *wrap;
@@ -599,11 +598,6 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
g_assert_cmpint(tmp->u.udfu.u.value1.has_a_b, ==, false);
qapi_free_UserDefAlternate(tmp);
- v = visitor_input_test_init(data, "false");
- visit_type_UserDefAlternate(v, NULL, &tmp, &err);
- error_free_or_abort(&err);
- qapi_free_UserDefAlternate(tmp);
-
v = visitor_input_test_init(data, "{ 'alt': 42 }");
visit_type_WrapAlternate(v, NULL, &wrap, &error_abort);
g_assert_cmpint(wrap->alt->type, ==, QTYPE_QNUM);
--
2.7.5
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH v2 05/10] block: Use JSON null instead of "" to disable backing file
2017-07-20 7:53 [Qemu-devel] [PATCH v2 00/10] Correct two minor QMP interface design flaws Markus Armbruster
` (3 preceding siblings ...)
2017-07-20 7:53 ` [Qemu-devel] [PATCH v2 04/10] tests/test-qobject-input-visitor: Drop redundant test Markus Armbruster
@ 2017-07-20 7:53 ` Markus Armbruster
2017-07-20 7:53 ` [Qemu-devel] [PATCH v2 06/10] hmp: Clean up and simplify hmp_migrate_set_parameter() Markus Armbruster
` (5 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Markus Armbruster @ 2017-07-20 7:53 UTC (permalink / raw)
To: qemu-devel
Cc: eblake, berrange, kwolf, mreitz, qemu-block, quintela, dgilbert
BlockdevRef is an alternate of BlockdevOptions (inline definition) and
str (reference to an existing block device by name). BlockdevRef
value "" is special: "no block device should be referenced." It's
actually interpreted that way in just one place: optional member
@backing of COW formats. Semantics:
* Present means "use this block device" as backing storage
* Absent means "default to the one stored in the image"
* Except "" means "don't use backing storage at all"
The first two are perfectly normal: when the parameter is absent, it
defaults to an implied value, but the value's meaning is the same.
The third one overloads the parameter with a second meaning. The
overloading is *implicit*, i.e. it's not visible in the types. Works
here, because "" is not a value block device ID.
Pressing argument values the schema accepts, but are semantically
invalid, into service to mean "do something else entirely" is not
general, as suitable invalid values need not exist. I also find it
ugly.
To clean this up, we could add a separate flag argument to suppress
@backing, or add a distinct value to @backing. This commit implements
the latter: add JSON null to the values of @backing, deprecate "".
Because we're so close to the 2.10 freeze, implement it in the
stupidest way possible: have qmp_blockdev_add() rewrite null to ""
before anything else can see the null. Works, because BlockdevRef
occurs only within arguments of blockdev-add. The proper way to do it
would be rewriting "" to null, preferably in a cleaner way, but that
requires fixing up code to work with null. Add a TODO comment for
that.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
Acked-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
blockdev.c | 14 ++++++++++++++
qapi/block-core.json | 29 ++++++++++++++++++++++-------
tests/qemu-iotests/085 | 2 +-
tests/qemu-iotests/139 | 2 +-
4 files changed, 38 insertions(+), 9 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 6469f16..02cd69b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3887,6 +3887,7 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
QObject *obj;
Visitor *v = qobject_output_visitor_new(&obj);
QDict *qdict;
+ const QDictEntry *ent;
Error *local_err = NULL;
visit_type_BlockdevOptions(v, NULL, &options, &local_err);
@@ -3900,6 +3901,19 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
qdict_flatten(qdict);
+ /*
+ * Rewrite "backing": null to "backing": ""
+ * TODO Rewrite "" to null instead, and perhaps not even here
+ */
+ for (ent = qdict_first(qdict); ent; ent = qdict_next(qdict, ent)) {
+ char *dot = strrchr(ent->key, '.');
+
+ if (!strcmp(dot ? dot + 1 : ent->key, "backing")
+ && qobject_type(ent->value) == QTYPE_QNULL) {
+ qdict_put(qdict, ent->key, qstring_new());
+ }
+ }
+
if (!qdict_get_try_str(qdict, "node-name")) {
error_setg(errp, "'node-name' must be specified for the root node");
goto fail;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index ff8e2ba..6866ae8 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2273,15 +2273,14 @@
# besides their data source and an optional backing file.
#
# @backing: reference to or definition of the backing file block
-# device (if missing, taken from the image file content). It is
-# allowed to pass an empty string here in order to disable the
-# default backing file.
+# device, null disables the backing file entirely.
+# Defaults to the backing file stored the image file.
#
# Since: 2.9
##
{ 'struct': 'BlockdevOptionsGenericCOWFormat',
'base': 'BlockdevOptionsGenericFormat',
- 'data': { '*backing': 'BlockdevRef' } }
+ 'data': { '*backing': 'BlockdevRefOrNull' } }
##
# @Qcow2OverlapCheckMode:
@@ -3120,9 +3119,7 @@
# Reference to a block device.
#
# @definition: defines a new block device inline
-# @reference: references the ID of an existing block device. An
-# empty string means that no block device should be
-# referenced.
+# @reference: references the ID of an existing block device
#
# Since: 2.9
##
@@ -3131,6 +3128,24 @@
'reference': 'str' } }
##
+# @BlockdevRefOrNull:
+#
+# Reference to a block device.
+#
+# @definition: defines a new block device inline
+# @reference: references the ID of an existing block device.
+# An empty string means that no block device should
+# be referenced. Deprecated; use null instead.
+# @null: No block device should be referenced (since 2.10)
+#
+# Since: 2.9
+##
+{ 'alternate': 'BlockdevRefOrNull',
+ 'data': { 'definition': 'BlockdevOptions',
+ 'reference': 'str',
+ 'null': 'null' } }
+
+##
# @blockdev-add:
#
# Creates a new block device. If the @id option is given at the top level, a
diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
index 71efe50..5c7668c 100755
--- a/tests/qemu-iotests/085
+++ b/tests/qemu-iotests/085
@@ -106,7 +106,7 @@ function add_snapshot_image()
snapshot_file="${TEST_DIR}/${1}-${snapshot_virt0}"
_make_test_img -u -b "${base_image}" "$size"
mv "${TEST_IMG}" "${snapshot_file}"
- do_blockdev_add "$1" "'backing': '', " "${snapshot_file}"
+ do_blockdev_add "$1" "'backing': null, " "${snapshot_file}"
}
# ${1}: unique identifier for the snapshot filename
diff --git a/tests/qemu-iotests/139 b/tests/qemu-iotests/139
index 9ff51d9..50cf40f 100644
--- a/tests/qemu-iotests/139
+++ b/tests/qemu-iotests/139
@@ -69,7 +69,7 @@ class TestBlockdevDel(iotests.QMPTestCase):
'-b', base_img, new_img, '1M')
opts = {'driver': iotests.imgfmt,
'node-name': node,
- 'backing': '',
+ 'backing': None,
'file': {'driver': 'file',
'filename': new_img}}
result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
--
2.7.5
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH v2 06/10] hmp: Clean up and simplify hmp_migrate_set_parameter()
2017-07-20 7:53 [Qemu-devel] [PATCH v2 00/10] Correct two minor QMP interface design flaws Markus Armbruster
` (4 preceding siblings ...)
2017-07-20 7:53 ` [Qemu-devel] [PATCH v2 05/10] block: Use JSON null instead of "" to disable backing file Markus Armbruster
@ 2017-07-20 7:53 ` Markus Armbruster
2017-07-20 7:53 ` [Qemu-devel] [PATCH v2 07/10] migration: Clean up around tls_creds, tls_hostname Markus Armbruster
` (4 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Markus Armbruster @ 2017-07-20 7:53 UTC (permalink / raw)
To: qemu-devel
Cc: eblake, berrange, kwolf, mreitz, qemu-block, quintela, dgilbert
The bulk of hmp_migrate_set_parameter()'s code sets one member of
MigrationParameters according to the command's arguments. It uses a
string visitor for integer and boolean members, but not for string and
size members. It calls visit_type_bool() right away, but delays
visit_type_int() some. The delaying requires a flag variable and a
bit of trickery: we set all integer members instead of just the one we
want, and rely on the has_FOOs to mask the unwanted ones.
Clean this up as follows. Don't delay calling visit_type_int(). Use
the string visitor for strings, too. This involves extra allocations
and cleanup, but doing them is simpler and cleaner than avoiding them.
Sadly, using the string visitor for sizes isn't possible, because it
defaults to Bytes rather than Mebibytes. Add a comment explaining
that.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
hmp.c | 78 ++++++++++++++++++++++++++++---------------------------------------
1 file changed, 32 insertions(+), 46 deletions(-)
diff --git a/hmp.c b/hmp.c
index bf1de74..dae4604 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1554,90 +1554,75 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
const char *param = qdict_get_str(qdict, "parameter");
const char *valuestr = qdict_get_str(qdict, "value");
Visitor *v = string_input_visitor_new(valuestr);
+ MigrationParameters *p = g_new0(MigrationParameters, 1);
uint64_t valuebw = 0;
- int64_t valueint = 0;
- bool valuebool = false;
Error *err = NULL;
- bool use_int_value = false;
int i, ret;
for (i = 0; i < MIGRATION_PARAMETER__MAX; i++) {
if (strcmp(param, MigrationParameter_lookup[i]) == 0) {
- MigrationParameters p = { 0 };
switch (i) {
case MIGRATION_PARAMETER_COMPRESS_LEVEL:
- p.has_compress_level = true;
- use_int_value = true;
+ p->has_compress_level = true;
+ visit_type_int(v, param, &p->compress_level, &err);
break;
case MIGRATION_PARAMETER_COMPRESS_THREADS:
- p.has_compress_threads = true;
- use_int_value = true;
+ p->has_compress_threads = true;
+ visit_type_int(v, param, &p->compress_threads, &err);
break;
case MIGRATION_PARAMETER_DECOMPRESS_THREADS:
- p.has_decompress_threads = true;
- use_int_value = true;
+ p->has_decompress_threads = true;
+ visit_type_int(v, param, &p->decompress_threads, &err);
break;
case MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL:
- p.has_cpu_throttle_initial = true;
- use_int_value = true;
+ p->has_cpu_throttle_initial = true;
+ visit_type_int(v, param, &p->cpu_throttle_initial, &err);
break;
case MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT:
- p.has_cpu_throttle_increment = true;
- use_int_value = true;
+ p->has_cpu_throttle_increment = true;
+ visit_type_int(v, param, &p->cpu_throttle_increment, &err);
break;
case MIGRATION_PARAMETER_TLS_CREDS:
- p.has_tls_creds = true;
- p.tls_creds = (char *) valuestr;
+ p->has_tls_creds = true;
+ visit_type_str(v, param, &p->tls_creds, &err);
break;
case MIGRATION_PARAMETER_TLS_HOSTNAME:
- p.has_tls_hostname = true;
- p.tls_hostname = (char *) valuestr;
+ p->has_tls_hostname = true;
+ visit_type_str(v, param, &p->tls_hostname, &err);
break;
case MIGRATION_PARAMETER_MAX_BANDWIDTH:
- p.has_max_bandwidth = true;
+ p->has_max_bandwidth = true;
+ /*
+ * Can't use visit_type_size() here, because it
+ * defaults to Bytes rather than Mebibytes.
+ */
ret = qemu_strtosz_MiB(valuestr, NULL, &valuebw);
if (ret < 0 || valuebw > INT64_MAX
|| (size_t)valuebw != valuebw) {
error_setg(&err, "Invalid size %s", valuestr);
- goto cleanup;
+ break;
}
- p.max_bandwidth = valuebw;
+ p->max_bandwidth = valuebw;
break;
case MIGRATION_PARAMETER_DOWNTIME_LIMIT:
- p.has_downtime_limit = true;
- use_int_value = true;
+ p->has_downtime_limit = true;
+ visit_type_int(v, param, &p->downtime_limit, &err);
break;
case MIGRATION_PARAMETER_X_CHECKPOINT_DELAY:
- p.has_x_checkpoint_delay = true;
- use_int_value = true;
+ p->has_x_checkpoint_delay = true;
+ visit_type_int(v, param, &p->x_checkpoint_delay, &err);
break;
case MIGRATION_PARAMETER_BLOCK_INCREMENTAL:
- p.has_block_incremental = true;
- visit_type_bool(v, param, &valuebool, &err);
- if (err) {
- goto cleanup;
- }
- p.block_incremental = valuebool;
+ p->has_block_incremental = true;
+ visit_type_bool(v, param, &p->block_incremental, &err);
break;
}
- if (use_int_value) {
- visit_type_int(v, param, &valueint, &err);
- if (err) {
- goto cleanup;
- }
- /* Set all integers; only one has_FOO will be set, and
- * the code ignores the remaining values */
- p.compress_level = valueint;
- p.compress_threads = valueint;
- p.decompress_threads = valueint;
- p.cpu_throttle_initial = valueint;
- p.cpu_throttle_increment = valueint;
- p.downtime_limit = valueint;
- p.x_checkpoint_delay = valueint;
+ if (err) {
+ goto cleanup;
}
- qmp_migrate_set_parameters(&p, &err);
+ qmp_migrate_set_parameters(p, &err);
break;
}
}
@@ -1647,6 +1632,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
}
cleanup:
+ qapi_free_MigrationParameters(p);
visit_free(v);
if (err) {
error_report_err(err);
--
2.7.5
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH v2 07/10] migration: Clean up around tls_creds, tls_hostname
2017-07-20 7:53 [Qemu-devel] [PATCH v2 00/10] Correct two minor QMP interface design flaws Markus Armbruster
` (5 preceding siblings ...)
2017-07-20 7:53 ` [Qemu-devel] [PATCH v2 06/10] hmp: Clean up and simplify hmp_migrate_set_parameter() Markus Armbruster
@ 2017-07-20 7:53 ` Markus Armbruster
2017-07-20 7:53 ` [Qemu-devel] [PATCH v2 08/10] migration: Add TODO comments on duplication of QAPI_CLONE() Markus Armbruster
` (3 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Markus Armbruster @ 2017-07-20 7:53 UTC (permalink / raw)
To: qemu-devel
Cc: eblake, berrange, kwolf, mreitz, qemu-block, quintela, dgilbert
Optional MigrationParameters members tls_creds and tls_hostname can't
actually be absent outside qmp_migrate_set_parameters() since commit
4af245d (v2.9.0).
Note that commit 4af245d reverted the part of commit de63ab6 (v2.8.0)
that made tls_creds and tls_hostname absent instead of "" in the value
of query-migrate-parameters, even though commit de63ab6 called that a
mistake. What a mess.
Drop the redundant tests for presence, and update documentation.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
hmp.c | 6 ++++--
migration/migration.c | 4 ++--
qapi-schema.json | 11 +++++------
3 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/hmp.c b/hmp.c
index dae4604..d7b4727 100644
--- a/hmp.c
+++ b/hmp.c
@@ -313,12 +313,14 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
monitor_printf(mon, "%s: %" PRId64 "\n",
MigrationParameter_lookup[MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT],
params->cpu_throttle_increment);
+ assert(params->has_tls_creds);
monitor_printf(mon, "%s: '%s'\n",
MigrationParameter_lookup[MIGRATION_PARAMETER_TLS_CREDS],
- params->has_tls_creds ? params->tls_creds : "");
+ params->tls_creds);
+ assert(params->has_tls_hostname);
monitor_printf(mon, "%s: '%s'\n",
MigrationParameter_lookup[MIGRATION_PARAMETER_TLS_HOSTNAME],
- params->has_tls_hostname ? params->tls_hostname : "");
+ params->tls_hostname);
assert(params->has_max_bandwidth);
monitor_printf(mon, "%s: %" PRId64 " bytes/second\n",
MigrationParameter_lookup[MIGRATION_PARAMETER_MAX_BANDWIDTH],
diff --git a/migration/migration.c b/migration/migration.c
index 7615391..88b9277 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -444,9 +444,9 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
params->cpu_throttle_initial = s->parameters.cpu_throttle_initial;
params->has_cpu_throttle_increment = true;
params->cpu_throttle_increment = s->parameters.cpu_throttle_increment;
- params->has_tls_creds = !!s->parameters.tls_creds;
+ params->has_tls_creds = true;
params->tls_creds = g_strdup(s->parameters.tls_creds);
- params->has_tls_hostname = !!s->parameters.tls_hostname;
+ params->has_tls_hostname = true;
params->tls_hostname = g_strdup(s->parameters.tls_hostname);
params->has_max_bandwidth = true;
params->max_bandwidth = s->parameters.max_bandwidth;
diff --git a/qapi-schema.json b/qapi-schema.json
index 8b015be..62e7d07 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1054,9 +1054,7 @@
# @MigrationParameters:
#
# Optional members can be omitted on input ('migrate-set-parameters')
-# but most members will always be present on output
-# ('query-migrate-parameters'), with the exception of tls-creds and
-# tls-hostname.
+# but members will always be present on output.
#
# @compress-level: compression level
#
@@ -1077,10 +1075,10 @@
# channel. On the outgoing side of the migration, the credentials
# must be for a 'client' endpoint, while for the incoming side the
# credentials must be for a 'server' endpoint. Setting this
-# will enable TLS for all migrations. The default is unset,
-# resulting in unsecured migration at the QEMU level. (Since 2.7)
+# to a non-empty string enables TLS for all migrations.
# An empty string means that QEMU will use plain text mode for
-# migration, rather than TLS (Since 2.9)
+# migration, rather than TLS (Since 2.7)
+# Note: 2.8 reports this by omitting tls-creds instead.
#
# @tls-hostname: hostname of the target host for the migration. This
# is required when using x509 based TLS credentials and the
@@ -1090,6 +1088,7 @@
# certificate identity can be validated. (Since 2.7)
# An empty string means that QEMU will use the hostname
# associated with the migration URI, if any. (Since 2.9)
+# Note: 2.8 reports this by omitting tls-hostname instead.
#
# @max-bandwidth: to set maximum speed for migration. maximum speed in
# bytes per second. (Since 2.8)
--
2.7.5
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH v2 08/10] migration: Add TODO comments on duplication of QAPI_CLONE()
2017-07-20 7:53 [Qemu-devel] [PATCH v2 00/10] Correct two minor QMP interface design flaws Markus Armbruster
` (6 preceding siblings ...)
2017-07-20 7:53 ` [Qemu-devel] [PATCH v2 07/10] migration: Clean up around tls_creds, tls_hostname Markus Armbruster
@ 2017-07-20 7:53 ` Markus Armbruster
2017-07-20 7:53 ` [Qemu-devel] [PATCH v2 09/10] migration: Unshare MigrationParameters struct for now Markus Armbruster
` (2 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Markus Armbruster @ 2017-07-20 7:53 UTC (permalink / raw)
To: qemu-devel
Cc: eblake, berrange, kwolf, mreitz, qemu-block, quintela, dgilbert
qmp_query_migrate_parameters() and qmp_migrate_set_parameters()
effectively duplicate QAPI_CLONE() inline. Add suitable TODO
comments.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
migration/migration.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/migration/migration.c b/migration/migration.c
index 88b9277..6b4d17f 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -433,6 +433,7 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
MigrationParameters *params;
MigrationState *s = migrate_get_current();
+ /* TODO use QAPI_CLONE() instead of duplicating it inline */
params = g_malloc0(sizeof(*params));
params->has_compress_level = true;
params->compress_level = s->parameters.compress_level;
@@ -745,6 +746,8 @@ static void migrate_params_apply(MigrationParameters *params)
{
MigrationState *s = migrate_get_current();
+ /* TODO use QAPI_CLONE() instead of duplicating it inline */
+
if (params->has_compress_level) {
s->parameters.compress_level = params->compress_level;
}
--
2.7.5
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH v2 09/10] migration: Unshare MigrationParameters struct for now
2017-07-20 7:53 [Qemu-devel] [PATCH v2 00/10] Correct two minor QMP interface design flaws Markus Armbruster
` (7 preceding siblings ...)
2017-07-20 7:53 ` [Qemu-devel] [PATCH v2 08/10] migration: Add TODO comments on duplication of QAPI_CLONE() Markus Armbruster
@ 2017-07-20 7:53 ` Markus Armbruster
2017-07-20 7:53 ` [Qemu-devel] [PATCH v2 10/10] migration: Use JSON null instead of "" to reset parameter to default Markus Armbruster
2017-08-16 21:34 ` [Qemu-devel] [Qemu-block] [PATCH v2 00/10] Correct two minor QMP interface design flaws John Snow
10 siblings, 0 replies; 15+ messages in thread
From: Markus Armbruster @ 2017-07-20 7:53 UTC (permalink / raw)
To: qemu-devel
Cc: eblake, berrange, kwolf, mreitz, qemu-block, quintela, dgilbert
Commit de63ab6 "migrate: Share common MigrationParameters struct"
reused MigrationParameters for the arguments of
migrate-set-parameters, with the following rationale:
It is rather verbose, and slightly error-prone, to repeat
the same set of parameters for input (migrate-set-parameters)
as for output (query-migrate-parameters), where the only
difference is whether the members are optional. We can just
document that the optional members will always be present
on output, and then share a common struct between both
commands. The next patch can then reduce the amount of
code needed on input.
I need to unshare them to correct a design flaw in a stupid, but
minimally invasive way, in the next commit. We can restore the
sharing when we redo that patch in a less stupid way. Add a suitable
TODO comment.
Note that I revert only the sharing part of commit de63ab6, not the
part that made the members of query-migrate-parameters' result
optional. The schema (and thus introspection) remains inaccurate for
query-migrate-parameters. If we decide not to restore the sharing, we
should revert that part, too.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
hmp.c | 4 +--
migration/migration.c | 66 ++++++++++++++++++++++++++++++++++++---
qapi-schema.json | 85 ++++++++++++++++++++++++++++++++++++++++++++++-----
3 files changed, 140 insertions(+), 15 deletions(-)
diff --git a/hmp.c b/hmp.c
index d7b4727..4b6b114 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1556,7 +1556,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
const char *param = qdict_get_str(qdict, "parameter");
const char *valuestr = qdict_get_str(qdict, "value");
Visitor *v = string_input_visitor_new(valuestr);
- MigrationParameters *p = g_new0(MigrationParameters, 1);
+ MigrateSetParameters *p = g_new0(MigrateSetParameters, 1);
uint64_t valuebw = 0;
Error *err = NULL;
int i, ret;
@@ -1634,7 +1634,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
}
cleanup:
- qapi_free_MigrationParameters(p);
+ qapi_free_MigrateSetParameters(p);
visit_free(v);
if (err) {
error_report_err(err);
diff --git a/migration/migration.c b/migration/migration.c
index 6b4d17f..205b801 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -742,7 +742,59 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
return true;
}
-static void migrate_params_apply(MigrationParameters *params)
+static void migrate_params_test_apply(MigrateSetParameters *params,
+ MigrationParameters *dest)
+{
+ *dest = migrate_get_current()->parameters;
+
+ /* TODO use QAPI_CLONE() instead of duplicating it inline */
+
+ if (params->has_compress_level) {
+ dest->compress_level = params->compress_level;
+ }
+
+ if (params->has_compress_threads) {
+ dest->compress_threads = params->compress_threads;
+ }
+
+ if (params->has_decompress_threads) {
+ dest->decompress_threads = params->decompress_threads;
+ }
+
+ if (params->has_cpu_throttle_initial) {
+ dest->cpu_throttle_initial = params->cpu_throttle_initial;
+ }
+
+ if (params->has_cpu_throttle_increment) {
+ dest->cpu_throttle_increment = params->cpu_throttle_increment;
+ }
+
+ if (params->has_tls_creds) {
+ dest->tls_creds = g_strdup(params->tls_creds);
+ }
+
+ if (params->has_tls_hostname) {
+ dest->tls_hostname = g_strdup(params->tls_hostname);
+ }
+
+ if (params->has_max_bandwidth) {
+ dest->max_bandwidth = params->max_bandwidth;
+ }
+
+ if (params->has_downtime_limit) {
+ dest->downtime_limit = params->downtime_limit;
+ }
+
+ if (params->has_x_checkpoint_delay) {
+ dest->x_checkpoint_delay = params->x_checkpoint_delay;
+ }
+
+ if (params->has_block_incremental) {
+ dest->block_incremental = params->block_incremental;
+ }
+}
+
+static void migrate_params_apply(MigrateSetParameters *params)
{
MigrationState *s = migrate_get_current();
@@ -802,9 +854,13 @@ static void migrate_params_apply(MigrationParameters *params)
}
}
-void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
+void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
{
- if (!migrate_params_check(params, errp)) {
+ MigrationParameters tmp;
+
+ migrate_params_test_apply(params, &tmp);
+
+ if (!migrate_params_check(&tmp, errp)) {
/* Invalid parameter */
return;
}
@@ -1240,7 +1296,7 @@ int64_t qmp_query_migrate_cache_size(Error **errp)
void qmp_migrate_set_speed(int64_t value, Error **errp)
{
- MigrationParameters p = {
+ MigrateSetParameters p = {
.has_max_bandwidth = true,
.max_bandwidth = value,
};
@@ -1260,7 +1316,7 @@ void qmp_migrate_set_downtime(double value, Error **errp)
value *= 1000; /* Convert to milliseconds */
value = MAX(0, MIN(INT64_MAX, value));
- MigrationParameters p = {
+ MigrateSetParameters p = {
.has_downtime_limit = true,
.downtime_limit = value,
};
diff --git a/qapi-schema.json b/qapi-schema.json
index 62e7d07..9ea45d2 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1035,6 +1035,77 @@
'downtime-limit', 'x-checkpoint-delay', 'block-incremental' ] }
##
+# @MigrateSetParameters:
+#
+# @compress-level: compression level
+#
+# @compress-threads: compression thread count
+#
+# @decompress-threads: decompression thread count
+#
+# @cpu-throttle-initial: Initial percentage of time guest cpus are
+# throttled when migration auto-converge is activated.
+# The default value is 20. (Since 2.7)
+#
+# @cpu-throttle-increment: throttle percentage increase each time
+# auto-converge detects that migration is not making
+# progress. The default value is 10. (Since 2.7)
+#
+# @tls-creds: ID of the 'tls-creds' object that provides credentials
+# for establishing a TLS connection over the migration data
+# channel. On the outgoing side of the migration, the credentials
+# must be for a 'client' endpoint, while for the incoming side the
+# credentials must be for a 'server' endpoint. Setting this
+# to a non-empty string enables TLS for all migrations.
+# An empty string means that QEMU will use plain text mode for
+# migration, rather than TLS (Since 2.9)
+# Previously (since 2.7), this was reported by omitting
+# tls-creds instead.
+#
+# @tls-hostname: hostname of the target host for the migration. This
+# is required when using x509 based TLS credentials and the
+# migration URI does not already include a hostname. For
+# example if using fd: or exec: based migration, the
+# hostname must be provided so that the server's x509
+# certificate identity can be validated. (Since 2.7)
+# An empty string means that QEMU will use the hostname
+# associated with the migration URI, if any. (Since 2.9)
+# Previously (since 2.7), this was reported by omitting
+# tls-hostname instead.
+#
+# @max-bandwidth: to set maximum speed for migration. maximum speed in
+# bytes per second. (Since 2.8)
+#
+# @downtime-limit: set maximum tolerated downtime for migration. maximum
+# downtime in milliseconds (Since 2.8)
+#
+# @x-checkpoint-delay: the delay time between two COLO checkpoints. (Since 2.8)
+#
+# @block-incremental: Affects how much storage is migrated when the
+# block migration capability is enabled. When false, the entire
+# storage backing chain is migrated into a flattened image at
+# the destination; when true, only the active qcow2 layer is
+# migrated and the destination must already have access to the
+# same backing chain as was used on the source. (since 2.10)
+#
+# Since: 2.4
+##
+# TODO either fuse back into MigrationParameters, or make
+# MigrationParameters members mandatory
+{ 'struct': 'MigrateSetParameters',
+ 'data': { '*compress-level': 'int',
+ '*compress-threads': 'int',
+ '*decompress-threads': 'int',
+ '*cpu-throttle-initial': 'int',
+ '*cpu-throttle-increment': 'int',
+ '*tls-creds': 'str',
+ '*tls-hostname': 'str',
+ '*max-bandwidth': 'int',
+ '*downtime-limit': 'int',
+ '*x-checkpoint-delay': 'int',
+ '*block-incremental': 'bool' } }
+
+##
# @migrate-set-parameters:
#
# Set various migration parameters.
@@ -1048,13 +1119,12 @@
#
##
{ 'command': 'migrate-set-parameters', 'boxed': true,
- 'data': 'MigrationParameters' }
+ 'data': 'MigrateSetParameters' }
##
# @MigrationParameters:
#
-# Optional members can be omitted on input ('migrate-set-parameters')
-# but members will always be present on output.
+# The optional members aren't actually optional.
#
# @compress-level: compression level
#
@@ -1063,19 +1133,18 @@
# @decompress-threads: decompression thread count
#
# @cpu-throttle-initial: Initial percentage of time guest cpus are
-# throttledwhen migration auto-converge is activated.
-# The default value is 20. (Since 2.7)
+# throttled when migration auto-converge is activated.
+# (Since 2.7)
#
# @cpu-throttle-increment: throttle percentage increase each time
# auto-converge detects that migration is not making
-# progress. The default value is 10. (Since 2.7)
+# progress. (Since 2.7)
#
# @tls-creds: ID of the 'tls-creds' object that provides credentials
# for establishing a TLS connection over the migration data
# channel. On the outgoing side of the migration, the credentials
# must be for a 'client' endpoint, while for the incoming side the
-# credentials must be for a 'server' endpoint. Setting this
-# to a non-empty string enables TLS for all migrations.
+# credentials must be for a 'server' endpoint.
# An empty string means that QEMU will use plain text mode for
# migration, rather than TLS (Since 2.7)
# Note: 2.8 reports this by omitting tls-creds instead.
--
2.7.5
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH v2 10/10] migration: Use JSON null instead of "" to reset parameter to default
2017-07-20 7:53 [Qemu-devel] [PATCH v2 00/10] Correct two minor QMP interface design flaws Markus Armbruster
` (8 preceding siblings ...)
2017-07-20 7:53 ` [Qemu-devel] [PATCH v2 09/10] migration: Unshare MigrationParameters struct for now Markus Armbruster
@ 2017-07-20 7:53 ` Markus Armbruster
2017-08-16 21:34 ` [Qemu-devel] [Qemu-block] [PATCH v2 00/10] Correct two minor QMP interface design flaws John Snow
10 siblings, 0 replies; 15+ messages in thread
From: Markus Armbruster @ 2017-07-20 7:53 UTC (permalink / raw)
To: qemu-devel
Cc: eblake, berrange, kwolf, mreitz, qemu-block, quintela, dgilbert
migrate-set-parameters sets migration parameters according to is
arguments like this:
* Present means "set the parameter to this value"
* Absent means "leave the parameter unchanged"
* Except for parameters tls_creds and tls_hostname, "" means "reset
the parameter to its default value
The first two are perfectly normal: presence of the parameter makes
the command do something.
The third one overloads the parameter with a second meaning. The
overloading is *implicit*, i.e. it's not visible in the types. Works
here, because "" is neither a valid TLS credentials ID, nor a valid
host name.
Pressing argument values the schema accepts, but are semantically
invalid, into service to mean "reset to default" is not general, as
suitable invalid values need not exist. I also find it ugly.
To clean this up, we could add a separate flag argument to ask for
"reset to default", or add a distinct value to @tls_creds and
@tls_hostname. This commit implements the latter: add JSON null to
the values of @tls_creds and @tls_hostname, deprecate "".
Because we're so close to the 2.10 freeze, implement it in the
stupidest way possible: have qmp_migrate_set_parameters() rewrite null
to "" before anything else can see the null. The proper way to do it
would be rewriting "" to null, but that requires fixing up code to
work with null. Add TODO comments for that.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
hmp.c | 8 ++++++--
migration/migration.c | 27 +++++++++++++++++++++++----
qapi-schema.json | 20 ++++++++++++++++++--
3 files changed, 47 insertions(+), 8 deletions(-)
diff --git a/hmp.c b/hmp.c
index 4b6b114..fd80dce 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1586,11 +1586,15 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
break;
case MIGRATION_PARAMETER_TLS_CREDS:
p->has_tls_creds = true;
- visit_type_str(v, param, &p->tls_creds, &err);
+ p->tls_creds = g_new0(StrOrNull, 1);
+ p->tls_creds->type = QTYPE_QSTRING;
+ visit_type_str(v, param, &p->tls_creds->u.s, &err);
break;
case MIGRATION_PARAMETER_TLS_HOSTNAME:
p->has_tls_hostname = true;
- visit_type_str(v, param, &p->tls_hostname, &err);
+ p->tls_hostname = g_new0(StrOrNull, 1);
+ p->tls_hostname->type = QTYPE_QSTRING;
+ visit_type_str(v, param, &p->tls_hostname->u.s, &err);
break;
case MIGRATION_PARAMETER_MAX_BANDWIDTH:
p->has_max_bandwidth = true;
diff --git a/migration/migration.c b/migration/migration.c
index 205b801..085c32c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -770,11 +770,13 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
}
if (params->has_tls_creds) {
- dest->tls_creds = g_strdup(params->tls_creds);
+ assert(params->tls_creds->type == QTYPE_QSTRING);
+ dest->tls_creds = g_strdup(params->tls_creds->u.s);
}
if (params->has_tls_hostname) {
- dest->tls_hostname = g_strdup(params->tls_hostname);
+ assert(params->tls_hostname->type == QTYPE_QSTRING);
+ dest->tls_hostname = g_strdup(params->tls_hostname->u.s);
}
if (params->has_max_bandwidth) {
@@ -822,12 +824,14 @@ static void migrate_params_apply(MigrateSetParameters *params)
if (params->has_tls_creds) {
g_free(s->parameters.tls_creds);
- s->parameters.tls_creds = g_strdup(params->tls_creds);
+ assert(params->tls_creds->type == QTYPE_QSTRING);
+ s->parameters.tls_creds = g_strdup(params->tls_creds->u.s);
}
if (params->has_tls_hostname) {
g_free(s->parameters.tls_hostname);
- s->parameters.tls_hostname = g_strdup(params->tls_hostname);
+ assert(params->tls_hostname->type == QTYPE_QSTRING);
+ s->parameters.tls_hostname = g_strdup(params->tls_hostname->u.s);
}
if (params->has_max_bandwidth) {
@@ -858,6 +862,21 @@ void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
{
MigrationParameters tmp;
+ /* TODO Rewrite "" to null instead */
+ if (params->has_tls_creds
+ && params->tls_creds->type == QTYPE_QNULL) {
+ QDECREF(params->tls_creds->u.n);
+ params->tls_creds->type = QTYPE_QSTRING;
+ params->tls_creds->u.s = strdup("");
+ }
+ /* TODO Rewrite "" to null instead */
+ if (params->has_tls_hostname
+ && params->tls_hostname->type == QTYPE_QNULL) {
+ QDECREF(params->tls_hostname->u.n);
+ params->tls_hostname->type = QTYPE_QSTRING;
+ params->tls_hostname->u.s = strdup("");
+ }
+
migrate_params_test_apply(params, &tmp);
if (!migrate_params_check(&tmp, errp)) {
diff --git a/qapi-schema.json b/qapi-schema.json
index 9ea45d2..df0a568 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -116,6 +116,22 @@
{ 'command': 'qmp_capabilities' }
##
+# @StrOrNull:
+#
+# This is a string value or the explicit lack of a string (null
+# pointer in C). Intended for cases when 'optional absent' already
+# has a different meaning.
+#
+# @s: the string value
+# @n: no string value
+#
+# Since: 2.10
+##
+{ 'alternate': 'StrOrNull',
+ 'data': { 's': 'str',
+ 'n': 'null' } }
+
+##
# @LostTickPolicy:
#
# Policy for handling lost ticks in timer devices.
@@ -1098,8 +1114,8 @@
'*decompress-threads': 'int',
'*cpu-throttle-initial': 'int',
'*cpu-throttle-increment': 'int',
- '*tls-creds': 'str',
- '*tls-hostname': 'str',
+ '*tls-creds': 'StrOrNull',
+ '*tls-hostname': 'StrOrNull',
'*max-bandwidth': 'int',
'*downtime-limit': 'int',
'*x-checkpoint-delay': 'int',
--
2.7.5
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [Qemu-block] [PATCH v2 00/10] Correct two minor QMP interface design flaws
2017-07-20 7:53 [Qemu-devel] [PATCH v2 00/10] Correct two minor QMP interface design flaws Markus Armbruster
` (9 preceding siblings ...)
2017-07-20 7:53 ` [Qemu-devel] [PATCH v2 10/10] migration: Use JSON null instead of "" to reset parameter to default Markus Armbruster
@ 2017-08-16 21:34 ` John Snow
2017-08-16 21:41 ` Eric Blake
2017-08-17 4:53 ` Markus Armbruster
10 siblings, 2 replies; 15+ messages in thread
From: John Snow @ 2017-08-16 21:34 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: kwolf, berrange, qemu-block, quintela, dgilbert, mreitz
Looks like this series got no replies (maybe a failure of my mail
filtering?) but it has since been merged, so purely for my own selfish
purposes;
Merged upstream as:
01fa55982692fb51a16049b63b571651a1053989 migration: Use JSON null
instead of "" to reset parameter to default
1bda8b3c6950f74482ba19e8529db72b511ba977 migration: Unshare
MigrationParameters struct for now
e87fae4c488ff8a10b921311a63f16b94031c611 migration: Add TODO comments on
duplication of QAPI_CLONE()
8cc99dcdc264bc896926e43e7576c7b7ab633d70 migration: Clean up around
tls_creds, tls_hostname
7e91e82044f8d23acfb9949a2cdd667a6b239acd hmp: Clean up and simplify
hmp_migrate_set_parameter()
c42e8742f527476839bcc5f91c3d2ea456ca6a45 block: Use JSON null instead of
"" to disable backing file
06f80154b2ada1d58ac504e98ff6e943b069b96c
tests/test-qobject-input-visitor: Drop redundant test
4d2d5c41a9e8ee201cda8be8701f7f9fc92e71aa qapi: Introduce a first class
'null' type
d2f95f4d482374485234790a6fc3cca29ebb7355 qapi: Use QNull for a more
regular visit_type_null()
006ca09f3027d86346fce707e9295975c6558f42 qapi: Separate type QNull from
QObject
--js
On 07/20/2017 03:53 AM, Markus Armbruster wrote:
> blockdev-add and migrate-set-parameters overload empty strings to mean
> something entirely different. See my memo "qapi: Stop abusing
> "special" values for something entirely different" for details.
>
> Message-ID: <87379zhrhn.fsf@dusky.pond.sub.org>
> https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg04526.html
>
> This series deprecates these usages of "" in favour of JSON null.
> Because we're so close to the 2.10 freeze, the implementation is
> intentionally stupid: rewrite null to "" at first opportunity. The
> proper way to do it would be rewriting "" to null, but that requires
> fixing up code to work with null. There are TODO comments for that.
> I'm willing to take care of them in the next development cycle.
>
Did this get dropped in favor of a more comprehensive 2.11 fix?
> v2, because my pull request clashed with Juan's:
> * Rebased, non-trivial conflicts in migration/migration.c
> * PATCH 02: Commit message typo [Eric]
> Fix ppc [patchew]
> * PATCH 03: qapi-code-gen.txt update [Eric]
> * PATCH 09: Conflict resolution, somewhat ugly
> * PATCH 10: Doc improvement [Eric]
> Conflict resolution
>
> Markus Armbruster (10):
> qapi: Separate type QNull from QObject
> qapi: Use QNull for a more regular visit_type_null()
> qapi: Introduce a first class 'null' type
> tests/test-qobject-input-visitor: Drop redundant test
> block: Use JSON null instead of "" to disable backing file
> hmp: Clean up and simplify hmp_migrate_set_parameter()
> migration: Clean up around tls_creds, tls_hostname
> migration: Add TODO comments on duplication of QAPI_CLONE()
> migration: Unshare MigrationParameters struct for now
> migration: Use JSON null instead of "" to reset parameter to default
>
> blockdev.c | 14 +++++
> docs/devel/qapi-code-gen.txt | 10 +--
> hmp.c | 88 ++++++++++++--------------
> hw/ppc/spapr_drc.c | 4 +-
> include/qapi/qmp/qobject.h | 10 ++-
> include/qapi/visitor-impl.h | 3 +-
> include/qapi/visitor.h | 8 +--
> include/qemu/typedefs.h | 1 +
> migration/migration.c | 96 +++++++++++++++++++++++++---
> qapi-schema.json | 108 ++++++++++++++++++++++++++++----
> qapi/block-core.json | 29 ++++++---
> qapi/qapi-clone-visitor.c | 5 +-
> qapi/qapi-dealloc-visitor.c | 6 +-
> qapi/qapi-visit-core.c | 7 ++-
> qapi/qobject-input-visitor.c | 6 +-
> qapi/qobject-output-visitor.c | 5 +-
> qapi/string-input-visitor.c | 8 ++-
> qapi/string-output-visitor.c | 3 +-
> qapi/trace-events | 2 +-
> qobject/json-parser.c | 2 +-
> qobject/qnull.c | 8 ++-
> scripts/qapi.py | 5 +-
> target/i386/cpu.c | 4 +-
> target/ppc/translate_init.c | 5 +-
> tests/check-qjson.c | 6 +-
> tests/check-qnull.c | 27 ++++----
> tests/qapi-schema/qapi-schema-test.json | 3 +-
> tests/qapi-schema/qapi-schema-test.out | 1 +
> tests/qemu-iotests/085 | 2 +-
> tests/qemu-iotests/139 | 2 +-
> tests/test-qobject-input-visitor.c | 24 ++++---
> tests/test-qobject-output-visitor.c | 13 +++-
> 32 files changed, 378 insertions(+), 137 deletions(-)
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [Qemu-block] [PATCH v2 00/10] Correct two minor QMP interface design flaws
2017-08-16 21:34 ` [Qemu-devel] [Qemu-block] [PATCH v2 00/10] Correct two minor QMP interface design flaws John Snow
@ 2017-08-16 21:41 ` Eric Blake
2017-08-16 21:42 ` John Snow
2017-08-17 4:53 ` Markus Armbruster
1 sibling, 1 reply; 15+ messages in thread
From: Eric Blake @ 2017-08-16 21:41 UTC (permalink / raw)
To: John Snow, Markus Armbruster, qemu-devel
Cc: kwolf, qemu-block, quintela, dgilbert, mreitz
[-- Attachment #1: Type: text/plain, Size: 506 bytes --]
On 08/16/2017 04:34 PM, John Snow wrote:
> Looks like this series got no replies (maybe a failure of my mail
> filtering?) but it has since been merged, so purely for my own selfish
> purposes;
No replies to v2 due to the fact that the differences from v1 were
trivial enough to keep R-b from that spin (it was mostly conflict
resolution that required the respin).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 00/10] Correct two minor QMP interface design flaws
2017-08-16 21:41 ` Eric Blake
@ 2017-08-16 21:42 ` John Snow
0 siblings, 0 replies; 15+ messages in thread
From: John Snow @ 2017-08-16 21:42 UTC (permalink / raw)
To: Eric Blake, Markus Armbruster, qemu-devel
Cc: kwolf, qemu-block, quintela, dgilbert, mreitz
On 08/16/2017 05:41 PM, Eric Blake wrote:
> On 08/16/2017 04:34 PM, John Snow wrote:
>> Looks like this series got no replies (maybe a failure of my mail
>> filtering?) but it has since been merged, so purely for my own selfish
>> purposes;
>
> No replies to v2 due to the fact that the differences from v1 were
> trivial enough to keep R-b from that spin (it was mostly conflict
> resolution that required the respin).
>
That's fine; just very visibly marking it as "done" for the purposes of
the list, archives, tooling, etc.
--js
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 00/10] Correct two minor QMP interface design flaws
2017-08-16 21:34 ` [Qemu-devel] [Qemu-block] [PATCH v2 00/10] Correct two minor QMP interface design flaws John Snow
2017-08-16 21:41 ` Eric Blake
@ 2017-08-17 4:53 ` Markus Armbruster
1 sibling, 0 replies; 15+ messages in thread
From: Markus Armbruster @ 2017-08-17 4:53 UTC (permalink / raw)
To: John Snow; +Cc: qemu-devel, kwolf, qemu-block, quintela, dgilbert, mreitz
John Snow <jsnow@redhat.com> writes:
[...]
> On 07/20/2017 03:53 AM, Markus Armbruster wrote:
>> blockdev-add and migrate-set-parameters overload empty strings to mean
>> something entirely different. See my memo "qapi: Stop abusing
>> "special" values for something entirely different" for details.
>>
>> Message-ID: <87379zhrhn.fsf@dusky.pond.sub.org>
>> https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg04526.html
>>
>> This series deprecates these usages of "" in favour of JSON null.
>> Because we're so close to the 2.10 freeze, the implementation is
>> intentionally stupid: rewrite null to "" at first opportunity. The
>> proper way to do it would be rewriting "" to null, but that requires
>> fixing up code to work with null. There are TODO comments for that.
>> I'm willing to take care of them in the next development cycle.
>>
>
> Did this get dropped in favor of a more comprehensive 2.11 fix?
This is the stupid solution for 2.10. TODO comments in PATCH 05+08-10
mark the spots that could use cleanup. I hope to get to them in 2.11.
[...]
^ permalink raw reply [flat|nested] 15+ messages in thread