* [Qemu-devel] [PATCH v2 01/17] qapi: fix error propagation
2012-06-13 8:22 [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing Laszlo Ersek
@ 2012-06-13 8:22 ` Laszlo Ersek
2012-07-13 16:38 ` Luiz Capitulino
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 02/17] qapi: generate C types for fixed-width integers Laszlo Ersek
` (17 subsequent siblings)
18 siblings, 1 reply; 38+ messages in thread
From: Laszlo Ersek @ 2012-06-13 8:22 UTC (permalink / raw)
To: qemu-devel, lersek
From: Paolo Bonzini <pbonzini@redhat.com>
Don't overwrite / leak previously set errors.
Don't try to end a container that could not be started.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
error.h | 4 +-
error.c | 4 +-
qapi/qapi-visit-core.c | 10 +--
tests/test-qmp-input-visitor.c | 24 +++++---
docs/qapi-code-gen.txt | 2 +
scripts/qapi-visit.py | 129 +++++++++++++++++++++++----------------
6 files changed, 102 insertions(+), 71 deletions(-)
diff --git a/error.h b/error.h
index 45ff6c1..6898f84 100644
--- a/error.h
+++ b/error.h
@@ -24,7 +24,7 @@ typedef struct Error Error;
/**
* Set an indirect pointer to an error given a printf-style format parameter.
* Currently, qerror.h defines these error formats. This function is not
- * meant to be used outside of QEMU.
+ * meant to be used outside of QEMU. Errors after the first are discarded.
*/
void error_set(Error **err, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
@@ -57,7 +57,7 @@ void error_set_field(Error *err, const char *field, const char *value);
/**
* Propagate an error to an indirect pointer to an error. This function will
* always transfer ownership of the error reference and handles the case where
- * dst_err is NULL correctly.
+ * dst_err is NULL correctly. Errors after the first are discarded.
*/
void error_propagate(Error **dst_err, Error *local_err);
diff --git a/error.c b/error.c
index a52b771..0177972 100644
--- a/error.c
+++ b/error.c
@@ -29,7 +29,7 @@ void error_set(Error **errp, const char *fmt, ...)
Error *err;
va_list ap;
- if (errp == NULL) {
+ if (errp == NULL || *errp != NULL) {
return;
}
@@ -132,7 +132,7 @@ bool error_is_type(Error *err, const char *fmt)
void error_propagate(Error **dst_err, Error *local_err)
{
- if (dst_err) {
+ if (dst_err && !*dst_err) {
*dst_err = local_err;
} else if (local_err) {
error_free(local_err);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index ffffbf7..0a513d2 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -39,9 +39,8 @@ void visit_start_struct(Visitor *v, void **obj, const char *kind,
void visit_end_struct(Visitor *v, Error **errp)
{
- if (!error_is_set(errp)) {
- v->end_struct(v, errp);
- }
+ assert(!error_is_set(errp));
+ v->end_struct(v, errp);
}
void visit_start_list(Visitor *v, const char *name, Error **errp)
@@ -62,9 +61,8 @@ GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp)
void visit_end_list(Visitor *v, Error **errp)
{
- if (!error_is_set(errp)) {
- v->end_list(v, errp);
- }
+ assert(!error_is_set(errp));
+ v->end_list(v, errp);
}
void visit_start_optional(Visitor *v, bool *present, const char *name,
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index c30fdc4..8f5a509 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -151,14 +151,22 @@ typedef struct TestStruct
static void visit_type_TestStruct(Visitor *v, TestStruct **obj,
const char *name, Error **errp)
{
- visit_start_struct(v, (void **)obj, "TestStruct", name, sizeof(TestStruct),
- errp);
-
- visit_type_int(v, &(*obj)->integer, "integer", errp);
- visit_type_bool(v, &(*obj)->boolean, "boolean", errp);
- visit_type_str(v, &(*obj)->string, "string", errp);
-
- visit_end_struct(v, errp);
+ Error *err = NULL;
+ if (!error_is_set(errp)) {
+ visit_start_struct(v, (void **)obj, "TestStruct", name, sizeof(TestStruct),
+ &err);
+ if (!err) {
+ visit_type_int(v, &(*obj)->integer, "integer", &err);
+ visit_type_bool(v, &(*obj)->boolean, "boolean", &err);
+ visit_type_str(v, &(*obj)->string, "string", &err);
+
+ /* Always call end_struct if start_struct succeeded. */
+ error_propagate(errp, err);
+ err = NULL;
+ visit_end_struct(v, &err);
+ }
+ error_propagate(errp, err);
+ }
}
static void test_visitor_in_struct(TestInputVisitorData *data,
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index ad11767..cccb11e 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -220,6 +220,8 @@ Example:
#endif
mdroth@illuin:~/w/qemu2.git$
+(The actual structure of the visit_type_* functions is a bit more complex
+in order to propagate errors correctly and avoid leaking memory).
=== scripts/qapi-commands.py ===
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 8d4e94a..61cf586 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -17,14 +17,37 @@ import os
import getopt
import errno
-def generate_visit_struct_body(field_prefix, members):
- ret = ""
+def generate_visit_struct_body(field_prefix, name, members):
+ ret = mcgen('''
+if (!error_is_set(errp)) {
+''')
+ push_indent()
+
if len(field_prefix):
field_prefix = field_prefix + "."
+ ret += mcgen('''
+Error **errp = &err; /* from outer scope */
+Error *err = NULL;
+visit_start_struct(m, NULL, "", "%(name)s", 0, &err);
+''',
+ name=name)
+ else:
+ ret += mcgen('''
+Error *err = NULL;
+visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
+''',
+ name=name)
+
+ ret += mcgen('''
+if (!err) {
+ assert(!obj || *obj);
+''')
+
+ push_indent()
for argname, argentry, optional, structured in parse_args(members):
if optional:
ret += mcgen('''
-visit_start_optional(m, (obj && *obj) ? &(*obj)->%(c_prefix)shas_%(c_name)s : NULL, "%(name)s", errp);
+visit_start_optional(m, obj ? &(*obj)->%(c_prefix)shas_%(c_name)s : NULL, "%(name)s", &err);
if ((*obj)->%(prefix)shas_%(c_name)s) {
''',
c_prefix=c_var(field_prefix), prefix=field_prefix,
@@ -32,17 +55,10 @@ if ((*obj)->%(prefix)shas_%(c_name)s) {
push_indent()
if structured:
- ret += mcgen('''
-visit_start_struct(m, NULL, "", "%(name)s", 0, errp);
-''',
- name=argname)
- ret += generate_visit_struct_body(field_prefix + argname, argentry)
- ret += mcgen('''
-visit_end_struct(m, errp);
-''')
+ ret += generate_visit_struct_body(field_prefix + argname, argname, argentry)
else:
ret += mcgen('''
-visit_type_%(type)s(m, (obj && *obj) ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "%(name)s", errp);
+visit_type_%(type)s(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "%(name)s", &err);
''',
c_prefix=c_var(field_prefix), prefix=field_prefix,
type=type_name(argentry), c_name=c_var(argname),
@@ -52,7 +68,19 @@ visit_type_%(type)s(m, (obj && *obj) ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "
pop_indent()
ret += mcgen('''
}
-visit_end_optional(m, errp);
+visit_end_optional(m, &err);
+''')
+
+ pop_indent()
+ pop_indent()
+ ret += mcgen('''
+ /* Always call end_struct if start_struct succeeded. */
+ error_propagate(errp, err);
+ err = NULL;
+ visit_end_struct(m, &err);
+ }
+ error_propagate(errp, err);
+}
''')
return ret
@@ -61,22 +89,14 @@ def generate_visit_struct(name, members):
void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp)
{
- if (error_is_set(errp)) {
- return;
- }
- visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), errp);
- if (obj && !*obj) {
- goto end;
- }
''',
name=name)
+
push_indent()
- ret += generate_visit_struct_body("", members)
+ ret += generate_visit_struct_body("", name, members)
pop_indent()
ret += mcgen('''
-end:
- visit_end_struct(m, errp);
}
''')
return ret
@@ -87,18 +107,22 @@ def generate_visit_list(name, members):
void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp)
{
GenericList *i, **prev = (GenericList **)obj;
+ Error *err = NULL;
- if (error_is_set(errp)) {
- return;
- }
- visit_start_list(m, name, errp);
-
- for (; (i = visit_next_list(m, prev, errp)) != NULL; prev = &i) {
- %(name)sList *native_i = (%(name)sList *)i;
- visit_type_%(name)s(m, &native_i->value, NULL, errp);
+ if (!error_is_set(errp)) {
+ visit_start_list(m, name, &err);
+ if (!err) {
+ for (; (i = visit_next_list(m, prev, &err)) != NULL; prev = &i) {
+ %(name)sList *native_i = (%(name)sList *)i;
+ visit_type_%(name)s(m, &native_i->value, NULL, &err);
+ }
+ /* Always call end_list if start_list succeeded. */
+ error_propagate(errp, err);
+ err = NULL;
+ visit_end_list(m, &err);
+ }
+ error_propagate(errp, err);
}
-
- visit_end_list(m, errp);
}
''',
name=name)
@@ -122,27 +146,21 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
{
Error *err = NULL;
- if (error_is_set(errp)) {
- return;
- }
- visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
- if (obj && !*obj) {
- goto end;
- }
- visit_type_%(name)sKind(m, &(*obj)->kind, "type", &err);
- if (err) {
- error_propagate(errp, err);
- goto end;
- }
- switch ((*obj)->kind) {
+ if (!error_is_set(errp)) {
+ visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
+ if (!err) {
+ visit_type_%(name)sKind(m, &(*obj)->kind, "type", &err);
+ }
+ if (!err) {
+ switch ((*obj)->kind) {
''',
name=name)
for key in members:
ret += mcgen('''
- case %(abbrev)s_KIND_%(enum)s:
- visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", errp);
- break;
+ case %(abbrev)s_KIND_%(enum)s:
+ visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", &err);
+ break;
''',
abbrev = de_camel_case(name).upper(),
enum = c_fun(de_camel_case(key)).upper(),
@@ -150,11 +168,16 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
c_name=c_fun(key))
ret += mcgen('''
- default:
- abort();
+ default:
+ abort();
+ }
+ }
+ /* Always call end_struct if start_struct succeeded. */
+ error_propagate(errp, err);
+ err = NULL;
+ visit_end_struct(m, &err);
}
-end:
- visit_end_struct(m, errp);
+ error_propagate(errp, err);
}
''')
--
1.7.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 01/17] qapi: fix error propagation
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 01/17] qapi: fix error propagation Laszlo Ersek
@ 2012-07-13 16:38 ` Luiz Capitulino
2012-07-13 17:30 ` Laszlo Ersek
0 siblings, 1 reply; 38+ messages in thread
From: Luiz Capitulino @ 2012-07-13 16:38 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: qemu-devel
On Wed, 13 Jun 2012 10:22:32 +0200
Laszlo Ersek <lersek@redhat.com> wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
>
> Don't overwrite / leak previously set errors.
Can you elaborate a bit more? It's not clear to me where the bug is.
More comments below.
> Don't try to end a container that could not be started.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> error.h | 4 +-
> error.c | 4 +-
> qapi/qapi-visit-core.c | 10 +--
> tests/test-qmp-input-visitor.c | 24 +++++---
> docs/qapi-code-gen.txt | 2 +
> scripts/qapi-visit.py | 129 +++++++++++++++++++++++----------------
> 6 files changed, 102 insertions(+), 71 deletions(-)
>
> diff --git a/error.h b/error.h
> index 45ff6c1..6898f84 100644
> --- a/error.h
> +++ b/error.h
> @@ -24,7 +24,7 @@ typedef struct Error Error;
> /**
> * Set an indirect pointer to an error given a printf-style format parameter.
> * Currently, qerror.h defines these error formats. This function is not
> - * meant to be used outside of QEMU.
> + * meant to be used outside of QEMU. Errors after the first are discarded.
> */
> void error_set(Error **err, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
>
> @@ -57,7 +57,7 @@ void error_set_field(Error *err, const char *field, const char *value);
> /**
> * Propagate an error to an indirect pointer to an error. This function will
> * always transfer ownership of the error reference and handles the case where
> - * dst_err is NULL correctly.
> + * dst_err is NULL correctly. Errors after the first are discarded.
> */
> void error_propagate(Error **dst_err, Error *local_err);
>
> diff --git a/error.c b/error.c
> index a52b771..0177972 100644
> --- a/error.c
> +++ b/error.c
> @@ -29,7 +29,7 @@ void error_set(Error **errp, const char *fmt, ...)
> Error *err;
> va_list ap;
>
> - if (errp == NULL) {
> + if (errp == NULL || *errp != NULL) {
I think we should use assert() here.
If the error is already set, that most probably indicates a bug in the caller, as
it's the caller's responsibility to decide which error to return.
> return;
> }
>
> @@ -132,7 +132,7 @@ bool error_is_type(Error *err, const char *fmt)
>
> void error_propagate(Error **dst_err, Error *local_err)
> {
> - if (dst_err) {
> + if (dst_err && !*dst_err) {
> *dst_err = local_err;
> } else if (local_err) {
> error_free(local_err);
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index ffffbf7..0a513d2 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -39,9 +39,8 @@ void visit_start_struct(Visitor *v, void **obj, const char *kind,
>
> void visit_end_struct(Visitor *v, Error **errp)
> {
> - if (!error_is_set(errp)) {
> - v->end_struct(v, errp);
> - }
Is this the ending of a container that could not be started? But if it couldn't
be started, then errp be will be set and we won't try to end it, no?
> + assert(!error_is_set(errp));
> + v->end_struct(v, errp);
> }
>
> void visit_start_list(Visitor *v, const char *name, Error **errp)
> @@ -62,9 +61,8 @@ GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp)
>
> void visit_end_list(Visitor *v, Error **errp)
> {
> - if (!error_is_set(errp)) {
> - v->end_list(v, errp);
> - }
> + assert(!error_is_set(errp));
> + v->end_list(v, errp);
> }
>
> void visit_start_optional(Visitor *v, bool *present, const char *name,
> diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
> index c30fdc4..8f5a509 100644
> --- a/tests/test-qmp-input-visitor.c
> +++ b/tests/test-qmp-input-visitor.c
> @@ -151,14 +151,22 @@ typedef struct TestStruct
> static void visit_type_TestStruct(Visitor *v, TestStruct **obj,
> const char *name, Error **errp)
> {
> - visit_start_struct(v, (void **)obj, "TestStruct", name, sizeof(TestStruct),
> - errp);
> -
> - visit_type_int(v, &(*obj)->integer, "integer", errp);
> - visit_type_bool(v, &(*obj)->boolean, "boolean", errp);
> - visit_type_str(v, &(*obj)->string, "string", errp);
> -
> - visit_end_struct(v, errp);
> + Error *err = NULL;
> + if (!error_is_set(errp)) {
> + visit_start_struct(v, (void **)obj, "TestStruct", name, sizeof(TestStruct),
> + &err);
> + if (!err) {
> + visit_type_int(v, &(*obj)->integer, "integer", &err);
> + visit_type_bool(v, &(*obj)->boolean, "boolean", &err);
> + visit_type_str(v, &(*obj)->string, "string", &err);
> +
> + /* Always call end_struct if start_struct succeeded. */
> + error_propagate(errp, err);
> + err = NULL;
> + visit_end_struct(v, &err);
> + }
> + error_propagate(errp, err);
> + }
> }
>
> static void test_visitor_in_struct(TestInputVisitorData *data,
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index ad11767..cccb11e 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -220,6 +220,8 @@ Example:
> #endif
> mdroth@illuin:~/w/qemu2.git$
>
> +(The actual structure of the visit_type_* functions is a bit more complex
> +in order to propagate errors correctly and avoid leaking memory).
>
> === scripts/qapi-commands.py ===
>
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 8d4e94a..61cf586 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -17,14 +17,37 @@ import os
> import getopt
> import errno
>
> -def generate_visit_struct_body(field_prefix, members):
> - ret = ""
> +def generate_visit_struct_body(field_prefix, name, members):
> + ret = mcgen('''
> +if (!error_is_set(errp)) {
> +''')
> + push_indent()
> +
> if len(field_prefix):
> field_prefix = field_prefix + "."
> + ret += mcgen('''
> +Error **errp = &err; /* from outer scope */
> +Error *err = NULL;
> +visit_start_struct(m, NULL, "", "%(name)s", 0, &err);
> +''',
> + name=name)
> + else:
> + ret += mcgen('''
> +Error *err = NULL;
> +visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
> +''',
> + name=name)
> +
> + ret += mcgen('''
> +if (!err) {
> + assert(!obj || *obj);
> +''')
> +
> + push_indent()
> for argname, argentry, optional, structured in parse_args(members):
> if optional:
> ret += mcgen('''
> -visit_start_optional(m, (obj && *obj) ? &(*obj)->%(c_prefix)shas_%(c_name)s : NULL, "%(name)s", errp);
> +visit_start_optional(m, obj ? &(*obj)->%(c_prefix)shas_%(c_name)s : NULL, "%(name)s", &err);
> if ((*obj)->%(prefix)shas_%(c_name)s) {
> ''',
> c_prefix=c_var(field_prefix), prefix=field_prefix,
> @@ -32,17 +55,10 @@ if ((*obj)->%(prefix)shas_%(c_name)s) {
> push_indent()
>
> if structured:
> - ret += mcgen('''
> -visit_start_struct(m, NULL, "", "%(name)s", 0, errp);
> -''',
> - name=argname)
> - ret += generate_visit_struct_body(field_prefix + argname, argentry)
> - ret += mcgen('''
> -visit_end_struct(m, errp);
> -''')
> + ret += generate_visit_struct_body(field_prefix + argname, argname, argentry)
> else:
> ret += mcgen('''
> -visit_type_%(type)s(m, (obj && *obj) ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "%(name)s", errp);
> +visit_type_%(type)s(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "%(name)s", &err);
> ''',
> c_prefix=c_var(field_prefix), prefix=field_prefix,
> type=type_name(argentry), c_name=c_var(argname),
> @@ -52,7 +68,19 @@ visit_type_%(type)s(m, (obj && *obj) ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "
> pop_indent()
> ret += mcgen('''
> }
> -visit_end_optional(m, errp);
> +visit_end_optional(m, &err);
> +''')
> +
> + pop_indent()
> + pop_indent()
> + ret += mcgen('''
> + /* Always call end_struct if start_struct succeeded. */
> + error_propagate(errp, err);
> + err = NULL;
> + visit_end_struct(m, &err);
> + }
> + error_propagate(errp, err);
> +}
> ''')
> return ret
>
> @@ -61,22 +89,14 @@ def generate_visit_struct(name, members):
>
> void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp)
> {
> - if (error_is_set(errp)) {
> - return;
> - }
> - visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), errp);
> - if (obj && !*obj) {
> - goto end;
> - }
> ''',
> name=name)
> +
> push_indent()
> - ret += generate_visit_struct_body("", members)
> + ret += generate_visit_struct_body("", name, members)
> pop_indent()
>
> ret += mcgen('''
> -end:
> - visit_end_struct(m, errp);
> }
> ''')
> return ret
> @@ -87,18 +107,22 @@ def generate_visit_list(name, members):
> void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp)
> {
> GenericList *i, **prev = (GenericList **)obj;
> + Error *err = NULL;
>
> - if (error_is_set(errp)) {
> - return;
> - }
> - visit_start_list(m, name, errp);
> -
> - for (; (i = visit_next_list(m, prev, errp)) != NULL; prev = &i) {
> - %(name)sList *native_i = (%(name)sList *)i;
> - visit_type_%(name)s(m, &native_i->value, NULL, errp);
> + if (!error_is_set(errp)) {
> + visit_start_list(m, name, &err);
> + if (!err) {
> + for (; (i = visit_next_list(m, prev, &err)) != NULL; prev = &i) {
> + %(name)sList *native_i = (%(name)sList *)i;
> + visit_type_%(name)s(m, &native_i->value, NULL, &err);
> + }
> + /* Always call end_list if start_list succeeded. */
> + error_propagate(errp, err);
> + err = NULL;
> + visit_end_list(m, &err);
> + }
> + error_propagate(errp, err);
> }
> -
> - visit_end_list(m, errp);
> }
> ''',
> name=name)
> @@ -122,27 +146,21 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
> {
> Error *err = NULL;
>
> - if (error_is_set(errp)) {
> - return;
> - }
> - visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
> - if (obj && !*obj) {
> - goto end;
> - }
> - visit_type_%(name)sKind(m, &(*obj)->kind, "type", &err);
> - if (err) {
> - error_propagate(errp, err);
> - goto end;
> - }
> - switch ((*obj)->kind) {
> + if (!error_is_set(errp)) {
> + visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
> + if (!err) {
> + visit_type_%(name)sKind(m, &(*obj)->kind, "type", &err);
> + }
> + if (!err) {
> + switch ((*obj)->kind) {
> ''',
> name=name)
>
> for key in members:
> ret += mcgen('''
> - case %(abbrev)s_KIND_%(enum)s:
> - visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", errp);
> - break;
> + case %(abbrev)s_KIND_%(enum)s:
> + visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", &err);
> + break;
> ''',
> abbrev = de_camel_case(name).upper(),
> enum = c_fun(de_camel_case(key)).upper(),
> @@ -150,11 +168,16 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
> c_name=c_fun(key))
>
> ret += mcgen('''
> - default:
> - abort();
> + default:
> + abort();
> + }
> + }
> + /* Always call end_struct if start_struct succeeded. */
> + error_propagate(errp, err);
> + err = NULL;
> + visit_end_struct(m, &err);
> }
> -end:
> - visit_end_struct(m, errp);
> + error_propagate(errp, err);
> }
> ''')
>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 01/17] qapi: fix error propagation
2012-07-13 16:38 ` Luiz Capitulino
@ 2012-07-13 17:30 ` Laszlo Ersek
2012-07-13 19:11 ` Paolo Bonzini
0 siblings, 1 reply; 38+ messages in thread
From: Laszlo Ersek @ 2012-07-13 17:30 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: Paolo Bonzini, qemu-devel
On 07/13/12 18:38, Luiz Capitulino wrote:
> On Wed, 13 Jun 2012 10:22:32 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
>
>> From: Paolo Bonzini <pbonzini@redhat.com>
>>
>> Don't overwrite / leak previously set errors.
>
> Can you elaborate a bit more? It's not clear to me where the bug is.
Suppose you encounter the first error on the normal path, while a bunch
of objects is being constructed / composed. You set the error
accordingly and start to unwind the stack, tearing down objects
previously composed fully or partially. While doing this, you encounter
another error. If you call error_set() or error_propagate() now, the
first error is leaked. To avoid this, you have to check.
This change saves you the checks during stack unwinding, keeps the first
error stored (which is more important than any destructor errors, since
the latter could be the direct consequence of the first error and
aborting further processing). Second and later errors attempted to be
set via error_set() are simply not formatted, while second and later
errors attempted to be propagated with error_propagate() are released
(as there are two errors and we keep only one).
See "qapi: introduce OptsVisitor", function opts_end_struct(), comment
"we should have processed all (distinct) QemuOpt instances". If we abort
processing due to some error, there may be leftover options. We
shouldn't overwrite the first (real) error with this bogus one.
The stack is unwound in this case by the generated code -- if some
deeper part of OptsVisitor sets an error, the generated code will make
sure opts_end_struct() is called the right number of times.
>
> More comments below.
>
>> Don't try to end a container that could not be started.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>> error.h | 4 +-
>> error.c | 4 +-
>> qapi/qapi-visit-core.c | 10 +--
>> tests/test-qmp-input-visitor.c | 24 +++++---
>> docs/qapi-code-gen.txt | 2 +
>> scripts/qapi-visit.py | 129 +++++++++++++++++++++++----------------
>> 6 files changed, 102 insertions(+), 71 deletions(-)
>>
>> diff --git a/error.h b/error.h
>> index 45ff6c1..6898f84 100644
>> --- a/error.h
>> +++ b/error.h
>> @@ -24,7 +24,7 @@ typedef struct Error Error;
>> /**
>> * Set an indirect pointer to an error given a printf-style format parameter.
>> * Currently, qerror.h defines these error formats. This function is not
>> - * meant to be used outside of QEMU.
>> + * meant to be used outside of QEMU. Errors after the first are discarded.
>> */
>> void error_set(Error **err, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
>>
>> @@ -57,7 +57,7 @@ void error_set_field(Error *err, const char *field, const char *value);
>> /**
>> * Propagate an error to an indirect pointer to an error. This function will
>> * always transfer ownership of the error reference and handles the case where
>> - * dst_err is NULL correctly.
>> + * dst_err is NULL correctly. Errors after the first are discarded.
>> */
>> void error_propagate(Error **dst_err, Error *local_err);
>>
>> diff --git a/error.c b/error.c
>> index a52b771..0177972 100644
>> --- a/error.c
>> +++ b/error.c
>> @@ -29,7 +29,7 @@ void error_set(Error **errp, const char *fmt, ...)
>> Error *err;
>> va_list ap;
>>
>> - if (errp == NULL) {
>> + if (errp == NULL || *errp != NULL) {
>
> I think we should use assert() here.
>
> If the error is already set, that most probably indicates a bug in the caller, as
> it's the caller's responsibility to decide which error to return.
I believe we had a good argument against this, but I can't precisely
recall (or find) it now. Paolo, do you remember? Can you please both
search your respective mailboxen for Message-ID
<4FB21B71.7030804@redhat.com>? That's where we started to discuss this.
I believe I saw some paths in the code that tripped on this leak, and
generally keeping the first error seemed like a good idea.
opts_end_struct() originally checked for any pre-existent error
explicitly, but then the check was moved to the common code.
>
>> return;
>> }
>>
>> @@ -132,7 +132,7 @@ bool error_is_type(Error *err, const char *fmt)
>>
>> void error_propagate(Error **dst_err, Error *local_err)
>> {
>> - if (dst_err) {
>> + if (dst_err && !*dst_err) {
>> *dst_err = local_err;
>> } else if (local_err) {
>> error_free(local_err);
>> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
>> index ffffbf7..0a513d2 100644
>> --- a/qapi/qapi-visit-core.c
>> +++ b/qapi/qapi-visit-core.c
>> @@ -39,9 +39,8 @@ void visit_start_struct(Visitor *v, void **obj, const char *kind,
>>
>> void visit_end_struct(Visitor *v, Error **errp)
>> {
>> - if (!error_is_set(errp)) {
>> - v->end_struct(v, errp);
>> - }
>
> Is this the ending of a container that could not be started? But if it couldn't
> be started, then errp be will be set and we won't try to end it, no?
>
>> + assert(!error_is_set(errp));
>> + v->end_struct(v, errp);
>> }
(Perhaps I'm misunderstanding.)
Exactly as you say. That's why we replace the check with an assert.
(This seems to be your last question for this patch, so I'm cutting the
rest.)
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 01/17] qapi: fix error propagation
2012-07-13 17:30 ` Laszlo Ersek
@ 2012-07-13 19:11 ` Paolo Bonzini
2012-07-13 20:11 ` Laszlo Ersek
2012-07-16 17:12 ` Luiz Capitulino
0 siblings, 2 replies; 38+ messages in thread
From: Paolo Bonzini @ 2012-07-13 19:11 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: qemu-devel, Luiz Capitulino
Il 13/07/2012 19:30, Laszlo Ersek ha scritto:
>>> >> - if (errp == NULL) {
>>> >> + if (errp == NULL || *errp != NULL) {
>> >
>> > I think we should use assert() here.
>> >
>> > If the error is already set, that most probably indicates a bug in the caller, as
>> > it's the caller's responsibility to decide which error to return.
> I believe we had a good argument against this, but I can't precisely
> recall (or find) it now. Paolo, do you remember? Can you please both
> search your respective mailboxen for Message-ID
> <4FB21B71.7030804@redhat.com>? That's where we started to discuss this.
>
> I believe I saw some paths in the code that tripped on this leak, and
> generally keeping the first error seemed like a good idea.
> opts_end_struct() originally checked for any pre-existent error
> explicitly, but then the check was moved to the common code.
The reason to do this for error_propagate was to allow this idiom:
/* Always call end_struct if start_struct succeeded. */
error_propagate(errp, err);
err = NULL;
visit_end_struct(v, &err);
error_propagate(errp, err);
I think doing it for error_set was just for symmetry and to avoid
introducing excessive complexity.
Paolo
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 01/17] qapi: fix error propagation
2012-07-13 19:11 ` Paolo Bonzini
@ 2012-07-13 20:11 ` Laszlo Ersek
2012-07-16 17:12 ` Luiz Capitulino
1 sibling, 0 replies; 38+ messages in thread
From: Laszlo Ersek @ 2012-07-13 20:11 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Luiz Capitulino
On 07/13/12 21:11, Paolo Bonzini wrote:
> Il 13/07/2012 19:30, Laszlo Ersek ha scritto:
>>>>>> - if (errp == NULL) {
>>>>>> + if (errp == NULL || *errp != NULL) {
>>>>
>>>> I think we should use assert() here.
>>>>
>>>> If the error is already set, that most probably indicates a bug in the caller, as
>>>> it's the caller's responsibility to decide which error to return.
>> I believe we had a good argument against this, but I can't precisely
>> recall (or find) it now. Paolo, do you remember? Can you please both
>> search your respective mailboxen for Message-ID
>> <4FB21B71.7030804@redhat.com>? That's where we started to discuss this.
>>
>> I believe I saw some paths in the code that tripped on this leak, and
>> generally keeping the first error seemed like a good idea.
>> opts_end_struct() originally checked for any pre-existent error
>> explicitly, but then the check was moved to the common code.
>
> The reason to do this for error_propagate was to allow this idiom:
>
> /* Always call end_struct if start_struct succeeded. */
> error_propagate(errp, err);
> err = NULL;
> visit_end_struct(v, &err);
> error_propagate(errp, err);
Right!
> I think doing it for error_set was just for symmetry and to avoid
> introducing excessive complexity.
Correct again. IIRC it was even yours truly who humbly suggested that.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 01/17] qapi: fix error propagation
2012-07-13 19:11 ` Paolo Bonzini
2012-07-13 20:11 ` Laszlo Ersek
@ 2012-07-16 17:12 ` Luiz Capitulino
2012-07-16 20:31 ` Laszlo Ersek
1 sibling, 1 reply; 38+ messages in thread
From: Luiz Capitulino @ 2012-07-16 17:12 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Laszlo Ersek, qemu-devel
On Fri, 13 Jul 2012 21:11:15 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 13/07/2012 19:30, Laszlo Ersek ha scritto:
> >>> >> - if (errp == NULL) {
> >>> >> + if (errp == NULL || *errp != NULL) {
> >> >
> >> > I think we should use assert() here.
> >> >
> >> > If the error is already set, that most probably indicates a bug in the caller, as
> >> > it's the caller's responsibility to decide which error to return.
> > I believe we had a good argument against this, but I can't precisely
> > recall (or find) it now. Paolo, do you remember? Can you please both
> > search your respective mailboxen for Message-ID
> > <4FB21B71.7030804@redhat.com>? That's where we started to discuss this.
> >
> > I believe I saw some paths in the code that tripped on this leak, and
> > generally keeping the first error seemed like a good idea.
> > opts_end_struct() originally checked for any pre-existent error
> > explicitly, but then the check was moved to the common code.
>
> The reason to do this for error_propagate was to allow this idiom:
>
> /* Always call end_struct if start_struct succeeded. */
> error_propagate(errp, err);
> err = NULL;
> visit_end_struct(v, &err);
> error_propagate(errp, err);
I agree with this change for error_propagate() because it encapsulates our
rules for error propagation.
> I think doing it for error_set was just for symmetry and to avoid
> introducing excessive complexity.
We already check if the error is set in several places, and I don't think
it will add much complexity. I still think that an assert() is better.
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 01/17] qapi: fix error propagation
2012-07-16 17:12 ` Luiz Capitulino
@ 2012-07-16 20:31 ` Laszlo Ersek
2012-07-16 20:44 ` Luiz Capitulino
0 siblings, 1 reply; 38+ messages in thread
From: Laszlo Ersek @ 2012-07-16 20:31 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: Paolo Bonzini, qemu-devel
On 07/16/12 19:12, Luiz Capitulino wrote:
> On Fri, 13 Jul 2012 21:11:15 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>> I think doing it for error_set was just for symmetry and to avoid
>> introducing excessive complexity.
>
> We already check if the error is set in several places, and I don't think
> it will add much complexity. I still think that an assert() is better.
If that means that the generated traversal code takes responsibility to
call any visitor callback with a fresh error receptacle, IOW I can go
ahead and just use error_set() in OptsVisitor and any firing assert will
be blamed on the generator: fine :)
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] [PATCH v2 01/17] qapi: fix error propagation
2012-07-16 20:31 ` Laszlo Ersek
@ 2012-07-16 20:44 ` Luiz Capitulino
0 siblings, 0 replies; 38+ messages in thread
From: Luiz Capitulino @ 2012-07-16 20:44 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Paolo Bonzini, qemu-devel
On Mon, 16 Jul 2012 22:31:26 +0200
Laszlo Ersek <lersek@redhat.com> wrote:
> On 07/16/12 19:12, Luiz Capitulino wrote:
> > On Fri, 13 Jul 2012 21:11:15 +0200
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> >
> >> I think doing it for error_set was just for symmetry and to avoid
> >> introducing excessive complexity.
> >
> > We already check if the error is set in several places, and I don't think
> > it will add much complexity. I still think that an assert() is better.
>
> If that means that the generated traversal code takes responsibility to
> call any visitor callback with a fresh error receptacle, IOW I can go
> ahead and just use error_set() in OptsVisitor and any firing assert will
> be blamed on the generator: fine :)
If that means it's finding bugs then that's great. On the other hand, if it
shows only false positives and we end up having to re-work the code just to
avoid that, then I'd agree on not having an assert().
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v2 02/17] qapi: generate C types for fixed-width integers
2012-06-13 8:22 [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing Laszlo Ersek
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 01/17] qapi: fix error propagation Laszlo Ersek
@ 2012-06-13 8:22 ` Laszlo Ersek
2012-06-13 10:48 ` Paolo Bonzini
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 03/17] qapi: introduce "size" type Laszlo Ersek
` (16 subsequent siblings)
18 siblings, 1 reply; 38+ messages in thread
From: Laszlo Ersek @ 2012-06-13 8:22 UTC (permalink / raw)
To: qemu-devel, lersek
(Long line folded using parens:
<http://www.python.org/dev/peps/pep-0008/#maximum-line-length>.)
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
scripts/qapi.py | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index e062336..1292476 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -159,6 +159,10 @@ def c_type(name):
return 'char *'
elif name == 'int':
return 'int64_t'
+ elif (name == 'int8' or name == 'int16' or name == 'int32' or
+ name == 'int64' or name == 'uint8' or name == 'uint16' or
+ name == 'uint32' or name == 'uint64'):
+ return name + '_t'
elif name == 'bool':
return 'bool'
elif name == 'number':
--
1.7.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 02/17] qapi: generate C types for fixed-width integers
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 02/17] qapi: generate C types for fixed-width integers Laszlo Ersek
@ 2012-06-13 10:48 ` Paolo Bonzini
0 siblings, 0 replies; 38+ messages in thread
From: Paolo Bonzini @ 2012-06-13 10:48 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: qemu-devel
Il 13/06/2012 10:22, Laszlo Ersek ha scritto:
> (Long line folded using parens:
> <http://www.python.org/dev/peps/pep-0008/#maximum-line-length>.)
>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> scripts/qapi.py | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index e062336..1292476 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -159,6 +159,10 @@ def c_type(name):
> return 'char *'
> elif name == 'int':
> return 'int64_t'
> + elif (name == 'int8' or name == 'int16' or name == 'int32' or
> + name == 'int64' or name == 'uint8' or name == 'uint16' or
> + name == 'uint32' or name == 'uint64'):
> + return name + '_t'
> elif name == 'bool':
> return 'bool'
> elif name == 'number':
>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v2 03/17] qapi: introduce "size" type
2012-06-13 8:22 [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing Laszlo Ersek
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 01/17] qapi: fix error propagation Laszlo Ersek
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 02/17] qapi: generate C types for fixed-width integers Laszlo Ersek
@ 2012-06-13 8:22 ` Laszlo Ersek
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 04/17] expose QemuOpt and QemuOpts struct definitions to interested parties Laszlo Ersek
` (15 subsequent siblings)
18 siblings, 0 replies; 38+ messages in thread
From: Laszlo Ersek @ 2012-06-13 8:22 UTC (permalink / raw)
To: qemu-devel, lersek
v1->v2:
- fall back to uint64 rather than int
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
qapi/qapi-visit-core.h | 3 +++
qapi/qapi-visit-core.c | 7 +++++++
scripts/qapi.py | 2 ++
3 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/qapi/qapi-visit-core.h b/qapi/qapi-visit-core.h
index a19d70c..60aceda 100644
--- a/qapi/qapi-visit-core.h
+++ b/qapi/qapi-visit-core.h
@@ -60,6 +60,8 @@ struct Visitor
void (*type_int16)(Visitor *v, int16_t *obj, const char *name, Error **errp);
void (*type_int32)(Visitor *v, int32_t *obj, const char *name, Error **errp);
void (*type_int64)(Visitor *v, int64_t *obj, const char *name, Error **errp);
+ /* visit_type_size() falls back to (*type_uint64)() if type_size is unset */
+ void (*type_size)(Visitor *v, uint64_t *obj, const char *name, Error **errp);
};
void visit_start_handle(Visitor *v, void **obj, const char *kind,
@@ -85,6 +87,7 @@ void visit_type_int8(Visitor *v, int8_t *obj, const char *name, Error **errp);
void visit_type_int16(Visitor *v, int16_t *obj, const char *name, Error **errp);
void visit_type_int32(Visitor *v, int32_t *obj, const char *name, Error **errp);
void visit_type_int64(Visitor *v, int64_t *obj, const char *name, Error **errp);
+void visit_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp);
void visit_type_bool(Visitor *v, bool *obj, const char *name, Error **errp);
void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp);
void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 0a513d2..811cb2c 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -234,6 +234,13 @@ void visit_type_int64(Visitor *v, int64_t *obj, const char *name, Error **errp)
}
}
+void visit_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp)
+{
+ if (!error_is_set(errp)) {
+ (v->type_size ? v->type_size : v->type_uint64)(v, obj, name, errp);
+ }
+}
+
void visit_type_bool(Visitor *v, bool *obj, const char *name, Error **errp)
{
if (!error_is_set(errp)) {
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 1292476..8082af3 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -163,6 +163,8 @@ def c_type(name):
name == 'int64' or name == 'uint8' or name == 'uint16' or
name == 'uint32' or name == 'uint64'):
return name + '_t'
+ elif name == 'size':
+ return 'uint64_t'
elif name == 'bool':
return 'bool'
elif name == 'number':
--
1.7.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v2 04/17] expose QemuOpt and QemuOpts struct definitions to interested parties
2012-06-13 8:22 [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing Laszlo Ersek
` (2 preceding siblings ...)
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 03/17] qapi: introduce "size" type Laszlo Ersek
@ 2012-06-13 8:22 ` Laszlo Ersek
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 05/17] qapi: introduce OptsVisitor Laszlo Ersek
` (14 subsequent siblings)
18 siblings, 0 replies; 38+ messages in thread
From: Laszlo Ersek @ 2012-06-13 8:22 UTC (permalink / raw)
To: qemu-devel, lersek
The only clients should be the existent "qemu-option.c", and the upcoming
"qapi/opts-visitor.c".
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
qemu-option-internal.h | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
qemu-option.c | 24 +--------------------
2 files changed, 54 insertions(+), 23 deletions(-)
create mode 100644 qemu-option-internal.h
diff --git a/qemu-option-internal.h b/qemu-option-internal.h
new file mode 100644
index 0000000..19fdc1c
--- /dev/null
+++ b/qemu-option-internal.h
@@ -0,0 +1,53 @@
+/*
+ * Commandline option parsing functions
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ * Copyright (c) 2009 Kevin Wolf <kwolf@redhat.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef QEMU_OPTIONS_INTERNAL_H
+#define QEMU_OPTIONS_INTERNAL_H
+
+#include "qemu-option.h"
+
+struct QemuOpt {
+ const char *name;
+ const char *str;
+
+ const QemuOptDesc *desc;
+ union {
+ bool boolean;
+ uint64_t uint;
+ } value;
+
+ QemuOpts *opts;
+ QTAILQ_ENTRY(QemuOpt) next;
+};
+
+struct QemuOpts {
+ char *id;
+ QemuOptsList *list;
+ Location loc;
+ QTAILQ_HEAD(QemuOptHead, QemuOpt) head;
+ QTAILQ_ENTRY(QemuOpts) next;
+};
+
+#endif
diff --git a/qemu-option.c b/qemu-option.c
index bb3886c..8334190 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -29,9 +29,9 @@
#include "qemu-common.h"
#include "qemu-error.h"
#include "qemu-objects.h"
-#include "qemu-option.h"
#include "error.h"
#include "qerror.h"
+#include "qemu-option-internal.h"
/*
* Extracts the name of an option from the parameter string (p points at the
@@ -511,28 +511,6 @@ void print_option_help(QEMUOptionParameter *list)
/* ------------------------------------------------------------------ */
-struct QemuOpt {
- const char *name;
- const char *str;
-
- const QemuOptDesc *desc;
- union {
- bool boolean;
- uint64_t uint;
- } value;
-
- QemuOpts *opts;
- QTAILQ_ENTRY(QemuOpt) next;
-};
-
-struct QemuOpts {
- char *id;
- QemuOptsList *list;
- Location loc;
- QTAILQ_HEAD(QemuOptHead, QemuOpt) head;
- QTAILQ_ENTRY(QemuOpts) next;
-};
-
static QemuOpt *qemu_opt_find(QemuOpts *opts, const char *name)
{
QemuOpt *opt;
--
1.7.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v2 05/17] qapi: introduce OptsVisitor
2012-06-13 8:22 [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing Laszlo Ersek
` (3 preceding siblings ...)
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 04/17] expose QemuOpt and QemuOpts struct definitions to interested parties Laszlo Ersek
@ 2012-06-13 8:22 ` Laszlo Ersek
2012-06-13 10:50 ` Paolo Bonzini
2012-07-13 16:51 ` Luiz Capitulino
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 06/17] qapi schema: remove trailing whitespace Laszlo Ersek
` (13 subsequent siblings)
18 siblings, 2 replies; 38+ messages in thread
From: Laszlo Ersek @ 2012-06-13 8:22 UTC (permalink / raw)
To: qemu-devel, lersek
This visitor supports parsing
-option [type=]discriminator[,optarg1=val1][,optarg2=val2][,...]
style QemuOpts objects into "native" C structures. After defining the type
tree in the qapi schema (see below), a root type traversal with this
visitor linked to the underlying QemuOpts object will build the "native" C
representation of the option.
The type tree in the schema, corresponding to an option with a
discriminator, must have the following structure:
struct
scalar member for non-discriminated optarg 1 [*]
list for repeating non-discriminated optarg 2 [*]
wrapper struct
single scalar member
union
struct for discriminator case 1
scalar member for optarg 3 [*]
list for repeating optarg 4 [*]
wrapper struct
single scalar member
scalar member for optarg 5 [*]
struct for discriminator case 2
...
The "type" optarg name is fixed for the discriminator role. Its schema
representation is "union of structures", and each discriminator value must
correspond to a member name in the union.
If the option takes no "type" descriminator, then the type subtree rooted
at the union must be absent from the schema (including the union itself).
Optarg values can be of scalar types str / bool / integers / size.
Members marked with [*] may be defined as optional in the schema,
describing an optional optarg.
Repeating an optarg is supported; its schema representation must be "list
of structure with single mandatory scalar member". If an optarg is not
described as repeating in the schema (ie. it is defined as a scalar field
instead of a list), its last occurrence will take effect. Ordering between
differently named optargs is not preserved.
A mandatory list (or an optional one which is reported to be available),
corresponding to a repeating optarg, has at least one element after
successful parsing.
v1->v2:
- Update opts_type_size() prototype to uint64_t.
- Add opts_type_uint64() for options needing the full uint64_t range.
(Internals could be extracted to "cutils.c".)
- Allow negative values in opts_type_int().
- Rebase to nested Makefiles.
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
qapi/opts-visitor.h | 31 ++++
qapi/opts-visitor.c | 401 +++++++++++++++++++++++++++++++++++++++++++++++++++
qapi/Makefile.objs | 2 +-
3 files changed, 433 insertions(+), 1 deletions(-)
create mode 100644 qapi/opts-visitor.h
create mode 100644 qapi/opts-visitor.c
diff --git a/qapi/opts-visitor.h b/qapi/opts-visitor.h
new file mode 100644
index 0000000..ea1a395
--- /dev/null
+++ b/qapi/opts-visitor.h
@@ -0,0 +1,31 @@
+/*
+ * Options Visitor
+ *
+ * Copyright Red Hat, Inc. 2012
+ *
+ * Author: Laszlo Ersek <lersek@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#ifndef OPTS_VISITOR_H
+#define OPTS_VISITOR_H
+
+#include "qapi-visit-core.h"
+#include "qemu-option.h"
+
+typedef struct OptsVisitor OptsVisitor;
+
+/* Contrarily to qemu-option.c::parse_option_number(), OptsVisitor's "int"
+ * parser relies on strtoll() instead of strtoull(). Consequences:
+ * - string representations of negative numbers yield negative values,
+ * - values below INT64_MIN or LLONG_MIN are rejected,
+ * - values above INT64_MAX or LLONG_MAX are rejected.
+ */
+OptsVisitor *opts_visitor_new(const QemuOpts *opts);
+void opts_visitor_cleanup(OptsVisitor *nv);
+Visitor *opts_get_visitor(OptsVisitor *nv);
+
+#endif
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
new file mode 100644
index 0000000..9187c86
--- /dev/null
+++ b/qapi/opts-visitor.c
@@ -0,0 +1,401 @@
+/*
+ * Options Visitor
+ *
+ * Copyright Red Hat, Inc. 2012
+ *
+ * Author: Laszlo Ersek <lersek@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include "opts-visitor.h"
+#include "qemu-queue.h"
+#include "qemu-option-internal.h"
+#include "qapi-visit-impl.h"
+
+
+struct OptsVisitor
+{
+ Visitor visitor;
+
+ /* Ownership remains with opts_visitor_new()'s caller. */
+ const QemuOpts *opts_root;
+
+ unsigned depth;
+
+ /* Non-null iff depth is positive. Each key is a QemuOpt name. Each value
+ * is a non-empty GQueue, enumerating all QemuOpt occurrences with that
+ * name. */
+ GHashTable *unprocessed_opts;
+
+ /* The list currently being traversed with opts_start_list() /
+ * opts_next_list(). The list must have a struct element type in the
+ * schema, with a single mandatory scalar member. */
+ GQueue *repeated_opts;
+ bool repeated_opts_first;
+};
+
+
+static void
+destroy_list(gpointer list)
+{
+ g_queue_free(list);
+}
+
+
+static void
+opts_start_struct(Visitor *v, void **obj, const char *kind,
+ const char *name, size_t size, Error **errp)
+{
+ OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+ const QemuOpt *opt;
+
+ *obj = g_malloc0(size);
+ if (ov->depth++ > 0) {
+ return;
+ }
+
+ ov->unprocessed_opts = g_hash_table_new_full(&g_str_hash, &g_str_equal,
+ NULL, &destroy_list);
+ QTAILQ_FOREACH(opt, &ov->opts_root->head, next) {
+ GQueue *list;
+
+ list = g_hash_table_lookup(ov->unprocessed_opts, opt->name);
+ if (list == NULL) {
+ list = g_queue_new();
+
+ /* GHashTable will never try to free the keys -- we supplied NULL
+ * as "key_destroy_func" above. Thus cast away key const-ness in
+ * order to suppress gcc's warning. */
+ g_hash_table_insert(ov->unprocessed_opts, (gpointer)opt->name,
+ list);
+ }
+
+ /* Similarly, destroy_list() doesn't call g_queue_free_full(). */
+ g_queue_push_tail(list, (gpointer)opt);
+ }
+}
+
+
+static gboolean
+ghr_true(gpointer ign_key, gpointer ign_value, gpointer ign_user_data)
+{
+ return TRUE;
+}
+
+
+static void
+opts_end_struct(Visitor *v, Error **errp)
+{
+ OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+ GQueue *any;
+
+ if (--ov->depth > 0) {
+ return;
+ }
+
+ /* we should have processed all (distinct) QemuOpt instances */
+ any = g_hash_table_find(ov->unprocessed_opts, &ghr_true, NULL);
+ if (any) {
+ const QemuOpt *first;
+
+ first = g_queue_peek_head(any);
+ error_set(errp, QERR_INVALID_PARAMETER, first->name);
+ }
+ g_hash_table_destroy(ov->unprocessed_opts);
+ ov->unprocessed_opts = NULL;
+}
+
+
+static GQueue *
+lookup_distinct(const OptsVisitor *ov, const char *name, Error **errp)
+{
+ GQueue *list;
+
+ list = g_hash_table_lookup(ov->unprocessed_opts, name);
+ if (!list) {
+ error_set(errp, QERR_MISSING_PARAMETER, name);
+ }
+ return list;
+}
+
+
+static void
+opts_start_list(Visitor *v, const char *name, Error **errp)
+{
+ OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+
+ /* we can't traverse a list in a list */
+ assert(ov->repeated_opts == NULL);
+ ov->repeated_opts = lookup_distinct(ov, name, errp);
+ ov->repeated_opts_first = (ov->repeated_opts != NULL);
+}
+
+
+static GenericList *
+opts_next_list(Visitor *v, GenericList **list, Error **errp)
+{
+ OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+ GenericList **link;
+
+ if (ov->repeated_opts_first) {
+ ov->repeated_opts_first = false;
+ link = list;
+ } else {
+ const QemuOpt *opt;
+
+ opt = g_queue_pop_head(ov->repeated_opts);
+ if (g_queue_is_empty(ov->repeated_opts)) {
+ g_hash_table_remove(ov->unprocessed_opts, opt->name);
+ return NULL;
+ }
+ link = &(*list)->next;
+ }
+
+ *link = g_malloc0(sizeof **link);
+ return *link;
+}
+
+
+static void
+opts_end_list(Visitor *v, Error **errp)
+{
+ OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+
+ ov->repeated_opts = NULL;
+}
+
+
+static const QemuOpt *
+lookup_scalar(const OptsVisitor *ov, const char *name, Error **errp)
+{
+ if (ov->repeated_opts == NULL) {
+ GQueue *list;
+
+ /* the last occurrence of any QemuOpt takes effect when queried by name
+ */
+ list = lookup_distinct(ov, name, errp);
+ return list ? g_queue_peek_tail(list) : NULL;
+ }
+ return g_queue_peek_head(ov->repeated_opts);
+}
+
+
+static void
+processed(OptsVisitor *ov, const char *name)
+{
+ if (ov->repeated_opts == NULL) {
+ g_hash_table_remove(ov->unprocessed_opts, name);
+ }
+}
+
+
+static void
+opts_type_str(Visitor *v, char **obj, const char *name, Error **errp)
+{
+ OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+ const QemuOpt *opt;
+
+ opt = lookup_scalar(ov, name, errp);
+ if (!opt) {
+ return;
+ }
+ *obj = g_strdup(opt->str ? opt->str : "");
+ processed(ov, name);
+}
+
+
+/* mimics qemu-option.c::parse_option_bool() */
+static void
+opts_type_bool(Visitor *v, bool *obj, const char *name, Error **errp)
+{
+ OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+ const QemuOpt *opt;
+
+ opt = lookup_scalar(ov, name, errp);
+ if (!opt) {
+ return;
+ }
+
+ if (opt->str) {
+ if (strcmp(opt->str, "on") == 0 ||
+ strcmp(opt->str, "yes") == 0 ||
+ strcmp(opt->str, "y") == 0) {
+ *obj = true;
+ } else if (strcmp(opt->str, "off") == 0 ||
+ strcmp(opt->str, "no") == 0 ||
+ strcmp(opt->str, "n") == 0) {
+ *obj = false;
+ } else {
+ error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
+ "on|yes|y|off|no|n");
+ return;
+ }
+ } else {
+ *obj = true;
+ }
+
+ processed(ov, name);
+}
+
+
+static void
+opts_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp)
+{
+ OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+ const QemuOpt *opt;
+ const char *str;
+ long long val;
+ char *endptr;
+
+ opt = lookup_scalar(ov, name, errp);
+ if (!opt) {
+ return;
+ }
+ str = opt->str ? opt->str : "";
+
+ errno = 0;
+ val = strtoll(str, &endptr, 0);
+ if (*str != '\0' && *endptr == '\0' && errno == 0 && INT64_MIN <= val &&
+ val <= INT64_MAX) {
+ *obj = val;
+ processed(ov, name);
+ return;
+ }
+ error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
+ "a non-negative int64 value");
+}
+
+
+static void
+opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp)
+{
+ OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+ const QemuOpt *opt;
+ const char *str;
+
+ opt = lookup_scalar(ov, name, errp);
+ if (!opt) {
+ return;
+ }
+
+ str = opt->str;
+ if (str != NULL) {
+ while (isspace((unsigned char)*str)) {
+ ++str;
+ }
+
+ if (*str != '-' && *str != '\0') {
+ unsigned long long val;
+ char *endptr;
+
+ /* non-empty, non-negative subject sequence */
+ errno = 0;
+ val = strtoull(str, &endptr, 0);
+ if (*endptr == '\0' && errno == 0 && val <= UINT64_MAX) {
+ *obj = val;
+ processed(ov, name);
+ return;
+ }
+ }
+ }
+ error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
+ "an uint64 value");
+}
+
+
+static void
+opts_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp)
+{
+ OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+ const QemuOpt *opt;
+ int64_t val;
+ char *endptr;
+
+ opt = lookup_scalar(ov, name, errp);
+ if (!opt) {
+ return;
+ }
+
+ val = strtosz_suffix(opt->str ? opt->str : "", &endptr,
+ STRTOSZ_DEFSUFFIX_B);
+ if (val != -1 && *endptr == '\0') {
+ *obj = val;
+ processed(ov, name);
+ return;
+ }
+ error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
+ "a size value representible as a non-negative int64");
+}
+
+
+static void
+opts_start_optional(Visitor *v, bool *present, const char *name,
+ Error **errp)
+{
+ OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+
+ /* we only support a single mandatory scalar field in a list node */
+ assert(ov->repeated_opts == NULL);
+ *present = (lookup_distinct(ov, name, NULL) != NULL);
+}
+
+
+OptsVisitor *
+opts_visitor_new(const QemuOpts *opts)
+{
+ OptsVisitor *ov;
+
+ ov = g_malloc0(sizeof *ov);
+
+ ov->visitor.start_struct = &opts_start_struct;
+ ov->visitor.end_struct = &opts_end_struct;
+
+ ov->visitor.start_list = &opts_start_list;
+ ov->visitor.next_list = &opts_next_list;
+ ov->visitor.end_list = &opts_end_list;
+
+ /* input_type_enum() covers both "normal" enums and union discriminators.
+ * The union discriminator field is always generated as "type"; it should
+ * match the "type" QemuOpt child of any QemuOpts.
+ *
+ * input_type_enum() will remove the looked-up key from the
+ * "unprocessed_opts" hash even if the lookup fails, because the removal is
+ * done earlier in opts_type_str(). This should be harmless.
+ */
+ ov->visitor.type_enum = &input_type_enum;
+
+ ov->visitor.type_int = &opts_type_int;
+ ov->visitor.type_uint64 = &opts_type_uint64;
+ ov->visitor.type_size = &opts_type_size;
+ ov->visitor.type_bool = &opts_type_bool;
+ ov->visitor.type_str = &opts_type_str;
+
+ /* type_number() is not filled in, but this is not the first visitor to
+ * skip some mandatory methods... */
+
+ ov->visitor.start_optional = &opts_start_optional;
+
+ ov->opts_root = opts;
+
+ return ov;
+}
+
+
+void
+opts_visitor_cleanup(OptsVisitor *ov)
+{
+ if (ov->unprocessed_opts != NULL) {
+ g_hash_table_destroy(ov->unprocessed_opts);
+ }
+ memset(ov, '\0', sizeof *ov);
+}
+
+
+Visitor *
+opts_get_visitor(OptsVisitor *ov)
+{
+ return &ov->visitor;
+}
diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs
index d0b0c16..5f5846e 100644
--- a/qapi/Makefile.objs
+++ b/qapi/Makefile.objs
@@ -1,3 +1,3 @@
qapi-obj-y = qapi-visit-core.o qapi-dealloc-visitor.o qmp-input-visitor.o
qapi-obj-y += qmp-output-visitor.o qmp-registry.o qmp-dispatch.o
-qapi-obj-y += string-input-visitor.o string-output-visitor.o
+qapi-obj-y += string-input-visitor.o string-output-visitor.o opts-visitor.o
--
1.7.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 05/17] qapi: introduce OptsVisitor
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 05/17] qapi: introduce OptsVisitor Laszlo Ersek
@ 2012-06-13 10:50 ` Paolo Bonzini
2012-06-13 14:03 ` Laszlo Ersek
2012-07-13 16:51 ` Luiz Capitulino
1 sibling, 1 reply; 38+ messages in thread
From: Paolo Bonzini @ 2012-06-13 10:50 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: qemu-devel
Il 13/06/2012 10:22, Laszlo Ersek ha scritto:
> +static void
> +opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp)
> +{
> + OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
> + const QemuOpt *opt;
> + const char *str;
> +
> + opt = lookup_scalar(ov, name, errp);
> + if (!opt) {
> + return;
> + }
> +
> + str = opt->str;
> + if (str != NULL) {
> + while (isspace((unsigned char)*str)) {
> + ++str;
> + }
> +
> + if (*str != '-' && *str != '\0') {
> + unsigned long long val;
> + char *endptr;
> +
> + /* non-empty, non-negative subject sequence */
> + errno = 0;
> + val = strtoull(str, &endptr, 0);
> + if (*endptr == '\0' && errno == 0 && val <= UINT64_MAX) {
I would have expected a warning from GCC here, but obviously that's not
the case?
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 05/17] qapi: introduce OptsVisitor
2012-06-13 10:50 ` Paolo Bonzini
@ 2012-06-13 14:03 ` Laszlo Ersek
0 siblings, 0 replies; 38+ messages in thread
From: Laszlo Ersek @ 2012-06-13 14:03 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
On 06/13/12 12:50, Paolo Bonzini wrote:
> Il 13/06/2012 10:22, Laszlo Ersek ha scritto:
>> +static void
>> +opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp)
>> +{
>> + OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
>> + const QemuOpt *opt;
>> + const char *str;
>> +
>> + opt = lookup_scalar(ov, name, errp);
>> + if (!opt) {
>> + return;
>> + }
>> +
>> + str = opt->str;
>> + if (str != NULL) {
>> + while (isspace((unsigned char)*str)) {
>> + ++str;
>> + }
>> +
>> + if (*str != '-' && *str != '\0') {
>> + unsigned long long val;
>> + char *endptr;
>> +
>> + /* non-empty, non-negative subject sequence */
>> + errno = 0;
>> + val = strtoull(str, &endptr, 0);
>> + if (*endptr == '\0' && errno == 0 && val <= UINT64_MAX) {
>
> I would have expected a warning from GCC here, but obviously that's not
> the case?
Right, same surprise here. Maybe gcc has seen the light and it realizes
now "long long" can be wider than 64 bits, theoretically.
Thanks for the review!
Laszlo
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] [PATCH v2 05/17] qapi: introduce OptsVisitor
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 05/17] qapi: introduce OptsVisitor Laszlo Ersek
2012-06-13 10:50 ` Paolo Bonzini
@ 2012-07-13 16:51 ` Luiz Capitulino
2012-07-13 22:48 ` Laszlo Ersek
1 sibling, 1 reply; 38+ messages in thread
From: Luiz Capitulino @ 2012-07-13 16:51 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: qemu-devel
On Wed, 13 Jun 2012 10:22:36 +0200
Laszlo Ersek <lersek@redhat.com> wrote:
> This visitor supports parsing
>
> -option [type=]discriminator[,optarg1=val1][,optarg2=val2][,...]
>
> style QemuOpts objects into "native" C structures. After defining the type
> tree in the qapi schema (see below), a root type traversal with this
> visitor linked to the underlying QemuOpts object will build the "native" C
> representation of the option.
>
> The type tree in the schema, corresponding to an option with a
> discriminator, must have the following structure:
>
> struct
> scalar member for non-discriminated optarg 1 [*]
> list for repeating non-discriminated optarg 2 [*]
> wrapper struct
> single scalar member
> union
> struct for discriminator case 1
> scalar member for optarg 3 [*]
> list for repeating optarg 4 [*]
> wrapper struct
> single scalar member
> scalar member for optarg 5 [*]
> struct for discriminator case 2
> ...
>
> The "type" optarg name is fixed for the discriminator role. Its schema
> representation is "union of structures", and each discriminator value must
> correspond to a member name in the union.
>
> If the option takes no "type" descriminator, then the type subtree rooted
> at the union must be absent from the schema (including the union itself).
>
> Optarg values can be of scalar types str / bool / integers / size.
>
> Members marked with [*] may be defined as optional in the schema,
> describing an optional optarg.
>
> Repeating an optarg is supported;
I see that the current code supports this too, but why? Something like this
should fail:
-netdev type=tap,vhost=on,vhost=off,id=guest1,script=qemu-ifup-switch
More comments below.
> its schema representation must be "list
> of structure with single mandatory scalar member". If an optarg is not
> described as repeating in the schema (ie. it is defined as a scalar field
> instead of a list), its last occurrence will take effect. Ordering between
> differently named optargs is not preserved.
>
> A mandatory list (or an optional one which is reported to be available),
> corresponding to a repeating optarg, has at least one element after
> successful parsing.
>
> v1->v2:
> - Update opts_type_size() prototype to uint64_t.
> - Add opts_type_uint64() for options needing the full uint64_t range.
> (Internals could be extracted to "cutils.c".)
> - Allow negative values in opts_type_int().
> - Rebase to nested Makefiles.
>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> qapi/opts-visitor.h | 31 ++++
> qapi/opts-visitor.c | 401 +++++++++++++++++++++++++++++++++++++++++++++++++++
> qapi/Makefile.objs | 2 +-
> 3 files changed, 433 insertions(+), 1 deletions(-)
> create mode 100644 qapi/opts-visitor.h
> create mode 100644 qapi/opts-visitor.c
>
> diff --git a/qapi/opts-visitor.h b/qapi/opts-visitor.h
> new file mode 100644
> index 0000000..ea1a395
> --- /dev/null
> +++ b/qapi/opts-visitor.h
> @@ -0,0 +1,31 @@
> +/*
> + * Options Visitor
> + *
> + * Copyright Red Hat, Inc. 2012
> + *
> + * Author: Laszlo Ersek <lersek@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */
> +
> +#ifndef OPTS_VISITOR_H
> +#define OPTS_VISITOR_H
> +
> +#include "qapi-visit-core.h"
> +#include "qemu-option.h"
> +
> +typedef struct OptsVisitor OptsVisitor;
> +
> +/* Contrarily to qemu-option.c::parse_option_number(), OptsVisitor's "int"
> + * parser relies on strtoll() instead of strtoull(). Consequences:
> + * - string representations of negative numbers yield negative values,
> + * - values below INT64_MIN or LLONG_MIN are rejected,
> + * - values above INT64_MAX or LLONG_MAX are rejected.
> + */
> +OptsVisitor *opts_visitor_new(const QemuOpts *opts);
> +void opts_visitor_cleanup(OptsVisitor *nv);
> +Visitor *opts_get_visitor(OptsVisitor *nv);
> +
> +#endif
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> new file mode 100644
> index 0000000..9187c86
> --- /dev/null
> +++ b/qapi/opts-visitor.c
> @@ -0,0 +1,401 @@
> +/*
> + * Options Visitor
> + *
> + * Copyright Red Hat, Inc. 2012
> + *
> + * Author: Laszlo Ersek <lersek@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */
> +
> +#include "opts-visitor.h"
> +#include "qemu-queue.h"
> +#include "qemu-option-internal.h"
> +#include "qapi-visit-impl.h"
> +
> +
> +struct OptsVisitor
> +{
> + Visitor visitor;
> +
> + /* Ownership remains with opts_visitor_new()'s caller. */
> + const QemuOpts *opts_root;
> +
> + unsigned depth;
> +
> + /* Non-null iff depth is positive. Each key is a QemuOpt name. Each value
> + * is a non-empty GQueue, enumerating all QemuOpt occurrences with that
> + * name. */
> + GHashTable *unprocessed_opts;
> +
> + /* The list currently being traversed with opts_start_list() /
> + * opts_next_list(). The list must have a struct element type in the
> + * schema, with a single mandatory scalar member. */
> + GQueue *repeated_opts;
> + bool repeated_opts_first;
> +};
> +
> +
> +static void
> +destroy_list(gpointer list)
> +{
> + g_queue_free(list);
> +}
> +
> +
> +static void
> +opts_start_struct(Visitor *v, void **obj, const char *kind,
> + const char *name, size_t size, Error **errp)
> +{
> + OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
> + const QemuOpt *opt;
> +
> + *obj = g_malloc0(size);
> + if (ov->depth++ > 0) {
> + return;
> + }
> +
> + ov->unprocessed_opts = g_hash_table_new_full(&g_str_hash, &g_str_equal,
> + NULL, &destroy_list);
> + QTAILQ_FOREACH(opt, &ov->opts_root->head, next) {
> + GQueue *list;
> +
> + list = g_hash_table_lookup(ov->unprocessed_opts, opt->name);
> + if (list == NULL) {
> + list = g_queue_new();
> +
> + /* GHashTable will never try to free the keys -- we supplied NULL
> + * as "key_destroy_func" above. Thus cast away key const-ness in
> + * order to suppress gcc's warning. */
> + g_hash_table_insert(ov->unprocessed_opts, (gpointer)opt->name,
> + list);
> + }
> +
> + /* Similarly, destroy_list() doesn't call g_queue_free_full(). */
> + g_queue_push_tail(list, (gpointer)opt);
> + }
> +}
This doesn't insert the opts id into the hash, so opts_type_str() will fail
to find the id when the generated code visits it here:
void visit_type_Netdev(Visitor *m, Netdev ** obj, const char *name, Error **errp)
{
if (!error_is_set(errp)) {
Error *err = NULL;
visit_start_struct(m, (void **)obj, "Netdev", name, sizeof(Netdev), &err);
if (!err) {
assert(!obj || *obj);
visit_type_str(m, obj ? &(*obj)->id : NULL, "id", &err); <---------
[...]
Also, you're using a queue to support the repeating of optargs, right? I think
this could be simplified if we just don't support that.
> +
> +
> +static gboolean
> +ghr_true(gpointer ign_key, gpointer ign_value, gpointer ign_user_data)
> +{
> + return TRUE;
> +}
> +
> +
> +static void
> +opts_end_struct(Visitor *v, Error **errp)
> +{
> + OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
> + GQueue *any;
> +
> + if (--ov->depth > 0) {
> + return;
> + }
> +
> + /* we should have processed all (distinct) QemuOpt instances */
> + any = g_hash_table_find(ov->unprocessed_opts, &ghr_true, NULL);
> + if (any) {
> + const QemuOpt *first;
> +
> + first = g_queue_peek_head(any);
> + error_set(errp, QERR_INVALID_PARAMETER, first->name);
> + }
> + g_hash_table_destroy(ov->unprocessed_opts);
> + ov->unprocessed_opts = NULL;
> +}
> +
> +
> +static GQueue *
> +lookup_distinct(const OptsVisitor *ov, const char *name, Error **errp)
> +{
> + GQueue *list;
> +
> + list = g_hash_table_lookup(ov->unprocessed_opts, name);
> + if (!list) {
> + error_set(errp, QERR_MISSING_PARAMETER, name);
> + }
> + return list;
> +}
> +
> +
> +static void
> +opts_start_list(Visitor *v, const char *name, Error **errp)
> +{
> + OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
> +
> + /* we can't traverse a list in a list */
> + assert(ov->repeated_opts == NULL);
> + ov->repeated_opts = lookup_distinct(ov, name, errp);
> + ov->repeated_opts_first = (ov->repeated_opts != NULL);
> +}
> +
> +
> +static GenericList *
> +opts_next_list(Visitor *v, GenericList **list, Error **errp)
> +{
> + OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
> + GenericList **link;
> +
> + if (ov->repeated_opts_first) {
> + ov->repeated_opts_first = false;
> + link = list;
> + } else {
> + const QemuOpt *opt;
> +
> + opt = g_queue_pop_head(ov->repeated_opts);
> + if (g_queue_is_empty(ov->repeated_opts)) {
> + g_hash_table_remove(ov->unprocessed_opts, opt->name);
> + return NULL;
> + }
> + link = &(*list)->next;
> + }
> +
> + *link = g_malloc0(sizeof **link);
> + return *link;
> +}
> +
> +
> +static void
> +opts_end_list(Visitor *v, Error **errp)
> +{
> + OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
> +
> + ov->repeated_opts = NULL;
> +}
> +
> +
> +static const QemuOpt *
> +lookup_scalar(const OptsVisitor *ov, const char *name, Error **errp)
> +{
> + if (ov->repeated_opts == NULL) {
> + GQueue *list;
> +
> + /* the last occurrence of any QemuOpt takes effect when queried by name
> + */
> + list = lookup_distinct(ov, name, errp);
> + return list ? g_queue_peek_tail(list) : NULL;
> + }
> + return g_queue_peek_head(ov->repeated_opts);
> +}
> +
> +
> +static void
> +processed(OptsVisitor *ov, const char *name)
> +{
> + if (ov->repeated_opts == NULL) {
> + g_hash_table_remove(ov->unprocessed_opts, name);
> + }
> +}
> +
> +
> +static void
> +opts_type_str(Visitor *v, char **obj, const char *name, Error **errp)
> +{
> + OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
> + const QemuOpt *opt;
> +
> + opt = lookup_scalar(ov, name, errp);
> + if (!opt) {
> + return;
> + }
> + *obj = g_strdup(opt->str ? opt->str : "");
> + processed(ov, name);
> +}
> +
> +
> +/* mimics qemu-option.c::parse_option_bool() */
> +static void
> +opts_type_bool(Visitor *v, bool *obj, const char *name, Error **errp)
> +{
> + OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
> + const QemuOpt *opt;
> +
> + opt = lookup_scalar(ov, name, errp);
> + if (!opt) {
> + return;
> + }
> +
> + if (opt->str) {
> + if (strcmp(opt->str, "on") == 0 ||
> + strcmp(opt->str, "yes") == 0 ||
> + strcmp(opt->str, "y") == 0) {
> + *obj = true;
> + } else if (strcmp(opt->str, "off") == 0 ||
> + strcmp(opt->str, "no") == 0 ||
> + strcmp(opt->str, "n") == 0) {
> + *obj = false;
The current code only accepts 'on' or 'off', no reason to change that.
> + } else {
> + error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
> + "on|yes|y|off|no|n");
> + return;
> + }
> + } else {
> + *obj = true;
> + }
> +
> + processed(ov, name);
> +}
> +
> +
> +static void
> +opts_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp)
> +{
> + OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
> + const QemuOpt *opt;
> + const char *str;
> + long long val;
> + char *endptr;
> +
> + opt = lookup_scalar(ov, name, errp);
> + if (!opt) {
> + return;
> + }
> + str = opt->str ? opt->str : "";
> +
> + errno = 0;
> + val = strtoll(str, &endptr, 0);
> + if (*str != '\0' && *endptr == '\0' && errno == 0 && INT64_MIN <= val &&
> + val <= INT64_MAX) {
> + *obj = val;
> + processed(ov, name);
> + return;
> + }
> + error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
> + "a non-negative int64 value");
> +}
> +
> +
> +static void
> +opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp)
> +{
> + OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
> + const QemuOpt *opt;
> + const char *str;
> +
> + opt = lookup_scalar(ov, name, errp);
> + if (!opt) {
> + return;
> + }
> +
> + str = opt->str;
> + if (str != NULL) {
> + while (isspace((unsigned char)*str)) {
> + ++str;
> + }
> +
> + if (*str != '-' && *str != '\0') {
> + unsigned long long val;
> + char *endptr;
> +
> + /* non-empty, non-negative subject sequence */
> + errno = 0;
> + val = strtoull(str, &endptr, 0);
> + if (*endptr == '\0' && errno == 0 && val <= UINT64_MAX) {
> + *obj = val;
> + processed(ov, name);
> + return;
> + }
> + }
> + }
> + error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
> + "an uint64 value");
> +}
> +
> +
> +static void
> +opts_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp)
> +{
> + OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
> + const QemuOpt *opt;
> + int64_t val;
> + char *endptr;
> +
> + opt = lookup_scalar(ov, name, errp);
> + if (!opt) {
> + return;
> + }
> +
> + val = strtosz_suffix(opt->str ? opt->str : "", &endptr,
> + STRTOSZ_DEFSUFFIX_B);
> + if (val != -1 && *endptr == '\0') {
> + *obj = val;
> + processed(ov, name);
> + return;
> + }
> + error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
> + "a size value representible as a non-negative int64");
> +}
> +
> +
> +static void
> +opts_start_optional(Visitor *v, bool *present, const char *name,
> + Error **errp)
> +{
> + OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
> +
> + /* we only support a single mandatory scalar field in a list node */
> + assert(ov->repeated_opts == NULL);
> + *present = (lookup_distinct(ov, name, NULL) != NULL);
> +}
> +
> +
> +OptsVisitor *
> +opts_visitor_new(const QemuOpts *opts)
> +{
> + OptsVisitor *ov;
> +
> + ov = g_malloc0(sizeof *ov);
> +
> + ov->visitor.start_struct = &opts_start_struct;
> + ov->visitor.end_struct = &opts_end_struct;
> +
> + ov->visitor.start_list = &opts_start_list;
> + ov->visitor.next_list = &opts_next_list;
> + ov->visitor.end_list = &opts_end_list;
> +
> + /* input_type_enum() covers both "normal" enums and union discriminators.
> + * The union discriminator field is always generated as "type"; it should
> + * match the "type" QemuOpt child of any QemuOpts.
> + *
> + * input_type_enum() will remove the looked-up key from the
> + * "unprocessed_opts" hash even if the lookup fails, because the removal is
> + * done earlier in opts_type_str(). This should be harmless.
> + */
> + ov->visitor.type_enum = &input_type_enum;
> +
> + ov->visitor.type_int = &opts_type_int;
> + ov->visitor.type_uint64 = &opts_type_uint64;
> + ov->visitor.type_size = &opts_type_size;
> + ov->visitor.type_bool = &opts_type_bool;
> + ov->visitor.type_str = &opts_type_str;
> +
> + /* type_number() is not filled in, but this is not the first visitor to
> + * skip some mandatory methods... */
> +
> + ov->visitor.start_optional = &opts_start_optional;
> +
> + ov->opts_root = opts;
> +
> + return ov;
> +}
> +
> +
> +void
> +opts_visitor_cleanup(OptsVisitor *ov)
> +{
> + if (ov->unprocessed_opts != NULL) {
> + g_hash_table_destroy(ov->unprocessed_opts);
> + }
> + memset(ov, '\0', sizeof *ov);
> +}
> +
> +
> +Visitor *
> +opts_get_visitor(OptsVisitor *ov)
> +{
> + return &ov->visitor;
> +}
> diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs
> index d0b0c16..5f5846e 100644
> --- a/qapi/Makefile.objs
> +++ b/qapi/Makefile.objs
> @@ -1,3 +1,3 @@
> qapi-obj-y = qapi-visit-core.o qapi-dealloc-visitor.o qmp-input-visitor.o
> qapi-obj-y += qmp-output-visitor.o qmp-registry.o qmp-dispatch.o
> -qapi-obj-y += string-input-visitor.o string-output-visitor.o
> +qapi-obj-y += string-input-visitor.o string-output-visitor.o opts-visitor.o
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 05/17] qapi: introduce OptsVisitor
2012-07-13 16:51 ` Luiz Capitulino
@ 2012-07-13 22:48 ` Laszlo Ersek
2012-07-13 23:09 ` Laszlo Ersek
2012-07-16 17:30 ` Luiz Capitulino
0 siblings, 2 replies; 38+ messages in thread
From: Laszlo Ersek @ 2012-07-13 22:48 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: Paolo Bonzini, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 6752 bytes --]
On 07/13/12 18:51, Luiz Capitulino wrote:
> On Wed, 13 Jun 2012 10:22:36 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
>> Repeating an optarg is supported;
>
> I see that the current code supports this too, but why? Something
> like this should fail:
>
> -netdev type=tap,vhost=on,vhost=off,id=guest1,script=qemu-ifup-switch
> Also, you're using a queue to support the repeating of optargs,
> right? I think this could be simplified if we just don't support
> that.
I hate repeated options with a passion, but SLIRP's hostfwd and guestfwd
depend on repetition.
When the outermost opts_start_struct() is invoked and I shovel the
optargs into the queues, I can't yet know what's going to be used in
repeated form and what not.
If you prefer I can change lookup_scalar() as follows. For reference:
>> +static GQueue *
>> +lookup_distinct(const OptsVisitor *ov, const char *name, Error **errp)
>> +{
>> + GQueue *list;
>> +
>> + list = g_hash_table_lookup(ov->unprocessed_opts, name);
>> + if (!list) {
>> + error_set(errp, QERR_MISSING_PARAMETER, name);
>> + }
>> + return list;
>> +}
>> +static void
>> +opts_start_optional(Visitor *v, bool *present, const char *name,
>> + Error **errp)
>> +{
>> + OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
>> +
>> + /* we only support a single mandatory scalar field in a list node */
>> + assert(ov->repeated_opts == NULL);
>> + *present = (lookup_distinct(ov, name, NULL) != NULL);
>> +}
>> +static const QemuOpt *
>> +lookup_scalar(const OptsVisitor *ov, const char *name, Error **errp)
>> +{
>> + if (ov->repeated_opts == NULL) {
>> + GQueue *list;
>> +
>> + /* the last occurrence of any QemuOpt takes effect when queried by name
>> + */
>> + list = lookup_distinct(ov, name, errp;
>> + return list ? g_queue_peek_tail(list) : NULL;
We're outside of list traversal in this branch, meaning the optarg is
allowed exactly once. (Optional optargs are first handled by
opts_start_optional().) If lookup_distinct() succeeds here, then rather
than returning the last occurrence, I could check the depth of the queue
(== 1 or > 1), and set an error for > 1.
However QemuOpts definitely supports repeated optargs now (otherwise
slirp hostfwd/guestfwd wouldn't work). qemu_opt_foreach() is used for
iteration (with QTAILQ_FOREACH()), while qemu_opt_find() -- and thus its
direct callers -- rely on QTAILQ_FOREACH_REVERSE() and the first match.
Optargs of an option are apparently chained like this:
qemu_opts_parse() [qemu-option.c]
opts_parse(..., defaults=false)
opts_do_parse(..., prepend=false)
opt_set(..., prepend=false, ...)
QTAILQ_INSERT_TAIL()
"-option arg=val1,arg=val2,arg=val3" is therefore linked into the
corresponding QemuOpts instance in the same order, and qemu_opt_find()
will return "arg=val3". I also use g_queue_push_tail() and
g_queue_peek_tail(), so I think we're compatible.
>> + }
>> + return g_queue_peek_head(ov->repeated_opts);
>> +}
Continuing slightly out of order:
>> +/* mimics qemu-option.c::parse_option_bool() */
>> +static void
>> +opts_type_bool(Visitor *v, bool *obj, const char *name, Error **errp)
>> +{
>> + OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
>> + const QemuOpt *opt;
>> +
>> + opt = lookup_scalar(ov, name, errp);
>> + if (!opt) {
>> + return;
>> + }
>> +
>> + if (opt->str) {
>> + if (strcmp(opt->str, "on") == 0 ||
>> + strcmp(opt->str, "yes") == 0 ||
>> + strcmp(opt->str, "y") == 0) {
>> + *obj = true;
>> + } else if (strcmp(opt->str, "off") == 0 ||
>> + strcmp(opt->str, "no") == 0 ||
>> + strcmp(opt->str, "n") == 0) {
>> + *obj = false;
>
> The current code only accepts 'on' or 'off', no reason to change that.
>
>> + } else {
>> + error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
>> + "on|yes|y|off|no|n");
>> + return;
>> + }
>> + } else {
>> + *obj = true;
>> + }
>> +
>> + processed(ov, name);
>> +}
This function is used for "bool" generally. The following optargs were
all unified as "bool":
- slirp/restrict: originally QEMU_OPT_STRING, net_init_slirp() accepting
all of "on|yes|y|off|no|n"
- tap/vnet_hdr: originally QEMU_OPT_BOOL, parse_option_bool() accepting
"on|off".
- tap/vhost: ditto
- tap/vhostforce: ditto
So I took the union (nothing should break that used to work).
The leading comment rather means that the structure of
parse_option_bool() is followed:
- optarg values meaning "true": true
- optarg values meaning "false": false
- other optarg values: error
- no optarg value at all: true
>> +static void
>> +opts_start_struct(Visitor *v, void **obj, const char *kind,
>> + const char *name, size_t size, Error **errp)
>> +{
>> + OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
>> + const QemuOpt *opt;
>> +
>> + *obj = g_malloc0(size);
>> + if (ov->depth++ > 0) {
>> + return;
>> + }
>> +
>> + ov->unprocessed_opts = g_hash_table_new_full(&g_str_hash, &g_str_equal,
>> + NULL, &destroy_list);
>> + QTAILQ_FOREACH(opt, &ov->opts_root->head, next) {
>> + GQueue *list;
>> +
>> + list = g_hash_table_lookup(ov->unprocessed_opts, opt->name);
>> + if (list == NULL) {
>> + list = g_queue_new();
>> +
>> + /* GHashTable will never try to free the keys -- we supplied NULL
>> + * as "key_destroy_func" above. Thus cast away key const-ness in
>> + * order to suppress gcc's warning. */
>> + g_hash_table_insert(ov->unprocessed_opts, (gpointer)opt->name,
>> + list);
>> + }
>> +
>> + /* Similarly, destroy_list() doesn't call g_queue_free_full(). */
>> + g_queue_push_tail(list, (gpointer)opt);
>> + }
>> +}
>
> This doesn't insert the opts id into the hash, so opts_type_str()
> will fail to find the id when the generated code visits it here:
>
> void visit_type_Netdev(Visitor *m, Netdev ** obj, const char *name, Error **errp)
> {
> if (!error_is_set(errp)) {
> Error *err = NULL;
> visit_start_struct(m, (void **)obj, "Netdev", name, sizeof(Netdev), &err);
> if (!err) {
> assert(!obj || *obj);
> visit_type_str(m, obj ? &(*obj)->id : NULL, "id", &err); <---------
> [...]
>
*groan*
You're right. opts_do_parse() makes an exception with "id" and doesn't
call opt_set() for any occurrence of it. Would you accept the attached
fix, split up and squashed into previous parts appropriately?
Thanks!
Laszlo
[-- Attachment #2: 0001-support-NetLegacy-id-and-clean-up-QemuOpts-id-usage.patch --]
[-- Type: text/plain, Size: 4686 bytes --]
>From 6839ead5ac4a77ac82aee2fe1365e72e276aa89d Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Fri, 13 Jul 2012 23:49:09 +0200
Subject: [PATCH] support NetLegacy::id and clean up QemuOpts::id usage
NetLegacy::id is actually allowed and takes precedence over
NetLegacy::name.
Factor opts_visitor_insert() out of opts_start_struct() and call it
separately for opts_root->id if there's any.
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
net.c | 2 +-
qapi-schema.json | 5 ++++-
qapi/opts-visitor.c | 49 +++++++++++++++++++++++++++++++++++++------------
3 files changed, 42 insertions(+), 14 deletions(-)
diff --git a/net.c b/net.c
index 1612f64..dbca77b 100644
--- a/net.c
+++ b/net.c
@@ -869,7 +869,7 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp)
u.net = object;
opts = u.net->opts;
/* missing optional values have been initialized to "all bits zero" */
- name = u.net->name;
+ name = u.net->has_id ? u.net->id : u.net->name;
}
if (net_client_init_fun[opts->kind]) {
diff --git a/qapi-schema.json b/qapi-schema.json
index ed345ee..cc48127 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2116,7 +2116,9 @@
#
# @vlan: #optional vlan number
#
-# @name: #optional identifier for monitor commands
+# @id: #optional identifier for monitor commands
+#
+# @name: #optional identifier for monitor commands, ignored if @id is present
#
# @traits: device type specific properties (legacy)
#
@@ -2125,6 +2127,7 @@
{ 'type': 'NetLegacy',
'data': {
'*vlan': 'int32',
+ '*id': 'str',
'*name': 'str',
'opts': 'NetClientOptions' } }
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 9187c86..a261cf3 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -35,6 +35,12 @@ struct OptsVisitor
* schema, with a single mandatory scalar member. */
GQueue *repeated_opts;
bool repeated_opts_first;
+
+ /* If "opts_root->id" is set, reinstantiate it as a fake QemuOpt for
+ * uniformity. Only its "name" and "str" fields are set. "fake_id_opt" does
+ * not survive or escape the OptsVisitor object.
+ */
+ QemuOpt *fake_id_opt;
};
@@ -46,6 +52,27 @@ destroy_list(gpointer list)
static void
+opts_visitor_insert(GHashTable *unprocessed_opts, const QemuOpt *opt)
+{
+ GQueue *list;
+
+ list = g_hash_table_lookup(unprocessed_opts, opt->name);
+ if (list == NULL) {
+ list = g_queue_new();
+
+ /* GHashTable will never try to free the keys -- we supply NULL as
+ * "key_destroy_func" in opts_start_struct(). Thus cast away key
+ * const-ness in order to suppress gcc's warning.
+ */
+ g_hash_table_insert(unprocessed_opts, (gpointer)opt->name, list);
+ }
+
+ /* Similarly, destroy_list() doesn't call g_queue_free_full(). */
+ g_queue_push_tail(list, (gpointer)opt);
+}
+
+
+static void
opts_start_struct(Visitor *v, void **obj, const char *kind,
const char *name, size_t size, Error **errp)
{
@@ -60,21 +87,18 @@ opts_start_struct(Visitor *v, void **obj, const char *kind,
ov->unprocessed_opts = g_hash_table_new_full(&g_str_hash, &g_str_equal,
NULL, &destroy_list);
QTAILQ_FOREACH(opt, &ov->opts_root->head, next) {
- GQueue *list;
+ /* ensured by qemu-option.c::opts_do_parse() */
+ assert(strcmp(opt->name, "id") != 0);
- list = g_hash_table_lookup(ov->unprocessed_opts, opt->name);
- if (list == NULL) {
- list = g_queue_new();
+ opts_visitor_insert(ov->unprocessed_opts, opt);
+ }
- /* GHashTable will never try to free the keys -- we supplied NULL
- * as "key_destroy_func" above. Thus cast away key const-ness in
- * order to suppress gcc's warning. */
- g_hash_table_insert(ov->unprocessed_opts, (gpointer)opt->name,
- list);
- }
+ if (ov->opts_root->id != NULL) {
+ ov->fake_id_opt = g_malloc0(sizeof *ov->fake_id_opt);
- /* Similarly, destroy_list() doesn't call g_queue_free_full(). */
- g_queue_push_tail(list, (gpointer)opt);
+ ov->fake_id_opt->name = "id";
+ ov->fake_id_opt->str = ov->opts_root->id;
+ opts_visitor_insert(ov->unprocessed_opts, ov->fake_id_opt);
}
}
@@ -390,6 +414,7 @@ opts_visitor_cleanup(OptsVisitor *ov)
if (ov->unprocessed_opts != NULL) {
g_hash_table_destroy(ov->unprocessed_opts);
}
+ g_free(ov->fake_id_opt);
memset(ov, '\0', sizeof *ov);
}
--
1.7.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 05/17] qapi: introduce OptsVisitor
2012-07-13 22:48 ` Laszlo Ersek
@ 2012-07-13 23:09 ` Laszlo Ersek
2012-07-16 17:30 ` Luiz Capitulino
1 sibling, 0 replies; 38+ messages in thread
From: Laszlo Ersek @ 2012-07-13 23:09 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: Paolo Bonzini, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 366 bytes --]
On 07/14/12 00:48, Laszlo Ersek wrote:
> You're right. opts_do_parse() makes an exception with "id" and doesn't
> call opt_set() for any occurrence of it. Would you accept the attached
> fix, split up and squashed into previous parts appropriately?
Sigh. I haven't looked at this code in a month, I obviously forgot to
release memory symmetrically. Sorry.
Laszlo
[-- Attachment #2: 0002-OptsVisitor-release-fake_id_opt-symmetrically.patch --]
[-- Type: text/plain, Size: 721 bytes --]
>From 6ad7336a79f0187f4d0fba1f2f2eee64349cb137 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Sat, 14 Jul 2012 01:03:10 +0200
Subject: [PATCH 2/2] OptsVisitor: release "fake_id_opt" symmetrically
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
qapi/opts-visitor.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index a261cf3..ee6cf2b 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -130,6 +130,8 @@ opts_end_struct(Visitor *v, Error **errp)
}
g_hash_table_destroy(ov->unprocessed_opts);
ov->unprocessed_opts = NULL;
+ g_free(ov->fake_id_opt);
+ ov->fake_id_opt = NULL;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 05/17] qapi: introduce OptsVisitor
2012-07-13 22:48 ` Laszlo Ersek
2012-07-13 23:09 ` Laszlo Ersek
@ 2012-07-16 17:30 ` Luiz Capitulino
1 sibling, 0 replies; 38+ messages in thread
From: Luiz Capitulino @ 2012-07-16 17:30 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Paolo Bonzini, qemu-devel
On Sat, 14 Jul 2012 00:48:58 +0200
Laszlo Ersek <lersek@redhat.com> wrote:
> On 07/13/12 18:51, Luiz Capitulino wrote:
> > On Wed, 13 Jun 2012 10:22:36 +0200
> > Laszlo Ersek <lersek@redhat.com> wrote:
>
> >> Repeating an optarg is supported;
> >
> > I see that the current code supports this too, but why? Something
> > like this should fail:
> >
> > -netdev type=tap,vhost=on,vhost=off,id=guest1,script=qemu-ifup-switch
>
> > Also, you're using a queue to support the repeating of optargs,
> > right? I think this could be simplified if we just don't support
> > that.
>
> I hate repeated options with a passion, but SLIRP's hostfwd and guestfwd
> depend on repetition.
Oh, I see. I think it would be nicer to have a flag for this in QemuOptsList,
but that's unrelated to this series.
> > This doesn't insert the opts id into the hash, so opts_type_str()
> > will fail to find the id when the generated code visits it here:
> >
> > void visit_type_Netdev(Visitor *m, Netdev ** obj, const char *name, Error **errp)
> > {
> > if (!error_is_set(errp)) {
> > Error *err = NULL;
> > visit_start_struct(m, (void **)obj, "Netdev", name, sizeof(Netdev), &err);
> > if (!err) {
> > assert(!obj || *obj);
> > visit_type_str(m, obj ? &(*obj)->id : NULL, "id", &err); <---------
> > [...]
> >
>
> *groan*
>
> You're right. opts_do_parse() makes an exception with "id" and doesn't
> call opt_set() for any occurrence of it. Would you accept the attached
> fix, split up and squashed into previous parts appropriately?
I've glanced at it and it looked fine, but I'll wait for v3 to do a full
review again.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v2 06/17] qapi schema: remove trailing whitespace
2012-06-13 8:22 [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing Laszlo Ersek
` (4 preceding siblings ...)
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 05/17] qapi: introduce OptsVisitor Laszlo Ersek
@ 2012-06-13 8:22 ` Laszlo Ersek
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 07/17] qapi schema: add Netdev types Laszlo Ersek
` (12 subsequent siblings)
18 siblings, 0 replies; 38+ messages in thread
From: Laszlo Ersek @ 2012-06-13 8:22 UTC (permalink / raw)
To: qemu-devel, lersek
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
qapi-schema.json | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/qapi-schema.json b/qapi-schema.json
index 3b6e346..8a05b66 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -337,7 +337,7 @@
# @CPU: the index of the virtual CPU
#
# @current: this only exists for backwards compatible and should be ignored
-#
+#
# @halted: true if the virtual CPU is in the halt state. Halt usually refers
# to a processor specific low power mode.
#
@@ -680,7 +680,7 @@
# @SpiceInfo
#
# Information about the SPICE session.
-#
+#
# @enabled: true if the SPICE server is enabled, false otherwise
#
# @host: #optional The hostname the SPICE server is bound to. This depends on
@@ -1291,7 +1291,7 @@
##
{ 'command': 'human-monitor-command',
'data': {'command-line': 'str', '*cpu-index': 'int'},
- 'returns': 'str' }
+ 'returns': 'str' }
##
# @migrate_cancel
@@ -1452,7 +1452,7 @@
# @password: the new password
#
# @connected: #optional how to handle existing clients when changing the
-# password. If nothing is specified, defaults to `keep'
+# password. If nothing is specified, defaults to `keep'
# `fail' to fail the command if clients are connected
# `disconnect' to disconnect existing clients
# `keep' to maintain existing clients
@@ -1592,7 +1592,7 @@
# If the argument combination is invalid, InvalidParameterCombination
#
# Since: 1.1
-##
+##
{ 'command': 'block_set_io_throttle',
'data': { 'device': 'str', 'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int' } }
--
1.7.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v2 07/17] qapi schema: add Netdev types
2012-06-13 8:22 [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing Laszlo Ersek
` (5 preceding siblings ...)
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 06/17] qapi schema: remove trailing whitespace Laszlo Ersek
@ 2012-06-13 8:22 ` Laszlo Ersek
2012-06-13 10:50 ` Paolo Bonzini
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 08/17] hw, net: "net_client_type" -> "NetClientOptionsKind" (qapi-generated) Laszlo Ersek
` (11 subsequent siblings)
18 siblings, 1 reply; 38+ messages in thread
From: Laszlo Ersek @ 2012-06-13 8:22 UTC (permalink / raw)
To: qemu-devel, lersek
NetdevTapOptions::sndbuf and NetdevDumpOptions::len use the new "size"
type.
v1->v2:
- NetLegacy::name is optional
- NetLegacyNicOptions::vectors is of type uint32
- NetdevVdeOptions::port and ::mode are of type uint16
- NetLegacy::vlan has type int32
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
qapi-schema.json | 275 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 275 insertions(+), 0 deletions(-)
diff --git a/qapi-schema.json b/qapi-schema.json
index 8a05b66..099663d 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1862,3 +1862,278 @@
# Since: 0.14.0
##
{ 'command': 'netdev_del', 'data': {'id': 'str'} }
+
+##
+# @NetdevNoneOptions
+#
+# Use it alone to have zero network devices.
+#
+# Since 1.2
+##
+{ 'type': 'NetdevNoneOptions',
+ 'data': { } }
+
+##
+# @NetLegacyNicOptions
+#
+# Create a new Network Interface Card.
+#
+# @netdev: #optional id of -netdev to connect to
+#
+# @macaddr: #optional MAC address
+#
+# @model: #optional device model (e1000, rtl8139, virtio etc.)
+#
+# @addr: #optional PCI device address
+#
+# @vectors: #optional number of MSI-x vectors, 0 to disable MSI-X
+#
+# Since 1.2
+##
+{ 'type': 'NetLegacyNicOptions',
+ 'data': {
+ '*netdev': 'str',
+ '*macaddr': 'str',
+ '*model': 'str',
+ '*addr': 'str',
+ '*vectors': 'uint32' } }
+
+##
+# @String
+#
+# A fat type wrapping 'str', to be embedded in lists.
+#
+# Since 1.2
+##
+{ 'type': 'String',
+ 'data': {
+ 'str': 'str' } }
+
+##
+# @NetdevUserOptions
+#
+# Use the user mode network stack which requires no administrator privilege to
+# run.
+#
+# @hostname: #optional client hostname reported by the builtin DHCP server
+#
+# @restrict: #optional isolate the guest from the host
+#
+# @ip: #optional legacy parameter, use net= instead
+#
+# @net: #optional IP address and optional netmask
+#
+# @host: #optional guest-visible address of the host
+#
+# @tftp: #optional root directory of the built-in TFTP server
+#
+# @bootfile: #optional BOOTP filename, for use with tftp=
+#
+# @dhcpstart: #optional the first of the 16 IPs the built-in DHCP server can
+# assign
+#
+# @dns: #optional guest-visible address of the virtual nameserver
+#
+# @smb: #optional root directory of the built-in SMB server
+#
+# @smbserver: #optional IP address of the built-in SMB server
+#
+# @hostfwd: #optional redirect incoming TCP or UDP host connections to guest
+# endpoints
+#
+# @guestfwd: #optional forward guest TCP connections
+#
+# Since 1.2
+##
+{ 'type': 'NetdevUserOptions',
+ 'data': {
+ '*hostname': 'str',
+ '*restrict': 'bool',
+ '*ip': 'str',
+ '*net': 'str',
+ '*host': 'str',
+ '*tftp': 'str',
+ '*bootfile': 'str',
+ '*dhcpstart': 'str',
+ '*dns': 'str',
+ '*smb': 'str',
+ '*smbserver': 'str',
+ '*hostfwd': ['String'],
+ '*guestfwd': ['String'] } }
+
+##
+# @NetdevTapOptions
+#
+# Connect the host TAP network interface name to the VLAN.
+#
+# @ifname: #optional interface name
+#
+# @fd: #optional file descriptor of an already opened tap
+#
+# @script: #optional script to initialize the interface
+#
+# @downscript: #optional script to shut down the interface
+#
+# @helper: #optional command to execute to configure bridge
+#
+# @sndbuf: #optional send buffer limit. Understands [TGMKkb] suffixes.
+#
+# @vnet_hdr: #optional enable the IFF_VNET_HDR flag on the tap interface
+#
+# @vhost: #optional enable vhost-net network accelerator
+#
+# @vhostfd: #optional file descriptor of an already opened vhost net device
+#
+# @vhostforce: #optional vhost on for non-MSIX virtio guests
+#
+# Since 1.2
+##
+{ 'type': 'NetdevTapOptions',
+ 'data': {
+ '*ifname': 'str',
+ '*fd': 'str',
+ '*script': 'str',
+ '*downscript': 'str',
+ '*helper': 'str',
+ '*sndbuf': 'size',
+ '*vnet_hdr': 'bool',
+ '*vhost': 'bool',
+ '*vhostfd': 'str',
+ '*vhostforce': 'bool' } }
+
+##
+# @NetdevSocketOptions
+#
+# Connect the VLAN to a remote VLAN in another QEMU virtual machine using a TCP
+# socket connection.
+#
+# @fd: #optional file descriptor of an already opened socket
+#
+# @listen: #optional port number, and optional hostname, to listen on
+#
+# @connect: #optional port number, and optional hostname, to connect to
+#
+# @mcast: #optional UDP multicast address and port number
+#
+# @localaddr: #optional source address and port for multicast and udp packets
+#
+# @udp: #optional UDP unicast address and port number
+#
+# Since 1.2
+##
+{ 'type': 'NetdevSocketOptions',
+ 'data': {
+ '*fd': 'str',
+ '*listen': 'str',
+ '*connect': 'str',
+ '*mcast': 'str',
+ '*localaddr': 'str',
+ '*udp': 'str' } }
+
+##
+# @NetdevVdeOptions
+#
+# Connect the VLAN to a vde switch running on the host.
+#
+# @sock: #optional socket path
+#
+# @port: #optional port number
+#
+# @group: #optional group owner of socket
+#
+# @mode: #optional permissions for socket
+#
+# Since 1.2
+##
+{ 'type': 'NetdevVdeOptions',
+ 'data': {
+ '*sock': 'str',
+ '*port': 'uint16',
+ '*group': 'str',
+ '*mode': 'uint16' } }
+
+##
+# @NetdevDumpOptions
+#
+# Dump VLAN network traffic to a file.
+#
+# @len: #optional per-packet size limit (64k default). Understands [TGMKkb]
+# suffixes.
+#
+# @file: #optional dump file path (default is qemu-vlan0.pcap)
+#
+# Since 1.2
+##
+{ 'type': 'NetdevDumpOptions',
+ 'data': {
+ '*len': 'size',
+ '*file': 'str' } }
+
+##
+# @NetdevBridgeOptions
+#
+# Connect a host TAP network interface to a host bridge device.
+#
+# @br: #optional bridge name
+#
+# @helper: #optional command to execute to configure bridge
+#
+# Since 1.2
+##
+{ 'type': 'NetdevBridgeOptions',
+ 'data': {
+ '*br': 'str',
+ '*helper': 'str' } }
+
+##
+# @NetClientOptions
+#
+# A discriminated record of network device traits.
+#
+# Since 1.2
+##
+{ 'union': 'NetClientOptions',
+ 'data': {
+ 'none': 'NetdevNoneOptions',
+ 'nic': 'NetLegacyNicOptions',
+ 'user': 'NetdevUserOptions',
+ 'tap': 'NetdevTapOptions',
+ 'socket': 'NetdevSocketOptions',
+ 'vde': 'NetdevVdeOptions',
+ 'dump': 'NetdevDumpOptions',
+ 'bridge': 'NetdevBridgeOptions' } }
+
+##
+# @NetLegacy
+#
+# Captures the configuration of a network device; legacy.
+#
+# @vlan: #optional vlan number
+#
+# @name: #optional identifier for monitor commands
+#
+# @traits: device type specific properties (legacy)
+#
+# Since 1.2
+##
+{ 'type': 'NetLegacy',
+ 'data': {
+ '*vlan': 'int32',
+ '*name': 'str',
+ 'opts': 'NetClientOptions' } }
+
+##
+# @Netdev
+#
+# Captures the configuration of a network device.
+#
+# @id: identifier for monitor commands.
+#
+# @traits: device type specific properties
+#
+# Since 1.2
+##
+{ 'type': 'Netdev',
+ 'data': {
+ 'id': 'str',
+ 'opts': 'NetClientOptions' } }
--
1.7.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 07/17] qapi schema: add Netdev types
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 07/17] qapi schema: add Netdev types Laszlo Ersek
@ 2012-06-13 10:50 ` Paolo Bonzini
0 siblings, 0 replies; 38+ messages in thread
From: Paolo Bonzini @ 2012-06-13 10:50 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: qemu-devel
Il 13/06/2012 10:22, Laszlo Ersek ha scritto:
> NetdevTapOptions::sndbuf and NetdevDumpOptions::len use the new "size"
> type.
>
> v1->v2:
> - NetLegacy::name is optional
> - NetLegacyNicOptions::vectors is of type uint32
> - NetdevVdeOptions::port and ::mode are of type uint16
> - NetLegacy::vlan has type int32
>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> qapi-schema.json | 275 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 275 insertions(+), 0 deletions(-)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 8a05b66..099663d 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1862,3 +1862,278 @@
> # Since: 0.14.0
> ##
> { 'command': 'netdev_del', 'data': {'id': 'str'} }
> +
> +##
> +# @NetdevNoneOptions
> +#
> +# Use it alone to have zero network devices.
> +#
> +# Since 1.2
> +##
> +{ 'type': 'NetdevNoneOptions',
> + 'data': { } }
> +
> +##
> +# @NetLegacyNicOptions
> +#
> +# Create a new Network Interface Card.
> +#
> +# @netdev: #optional id of -netdev to connect to
> +#
> +# @macaddr: #optional MAC address
> +#
> +# @model: #optional device model (e1000, rtl8139, virtio etc.)
> +#
> +# @addr: #optional PCI device address
> +#
> +# @vectors: #optional number of MSI-x vectors, 0 to disable MSI-X
> +#
> +# Since 1.2
> +##
> +{ 'type': 'NetLegacyNicOptions',
> + 'data': {
> + '*netdev': 'str',
> + '*macaddr': 'str',
> + '*model': 'str',
> + '*addr': 'str',
> + '*vectors': 'uint32' } }
> +
> +##
> +# @String
> +#
> +# A fat type wrapping 'str', to be embedded in lists.
> +#
> +# Since 1.2
> +##
> +{ 'type': 'String',
> + 'data': {
> + 'str': 'str' } }
> +
> +##
> +# @NetdevUserOptions
> +#
> +# Use the user mode network stack which requires no administrator privilege to
> +# run.
> +#
> +# @hostname: #optional client hostname reported by the builtin DHCP server
> +#
> +# @restrict: #optional isolate the guest from the host
> +#
> +# @ip: #optional legacy parameter, use net= instead
> +#
> +# @net: #optional IP address and optional netmask
> +#
> +# @host: #optional guest-visible address of the host
> +#
> +# @tftp: #optional root directory of the built-in TFTP server
> +#
> +# @bootfile: #optional BOOTP filename, for use with tftp=
> +#
> +# @dhcpstart: #optional the first of the 16 IPs the built-in DHCP server can
> +# assign
> +#
> +# @dns: #optional guest-visible address of the virtual nameserver
> +#
> +# @smb: #optional root directory of the built-in SMB server
> +#
> +# @smbserver: #optional IP address of the built-in SMB server
> +#
> +# @hostfwd: #optional redirect incoming TCP or UDP host connections to guest
> +# endpoints
> +#
> +# @guestfwd: #optional forward guest TCP connections
> +#
> +# Since 1.2
> +##
> +{ 'type': 'NetdevUserOptions',
> + 'data': {
> + '*hostname': 'str',
> + '*restrict': 'bool',
> + '*ip': 'str',
> + '*net': 'str',
> + '*host': 'str',
> + '*tftp': 'str',
> + '*bootfile': 'str',
> + '*dhcpstart': 'str',
> + '*dns': 'str',
> + '*smb': 'str',
> + '*smbserver': 'str',
> + '*hostfwd': ['String'],
> + '*guestfwd': ['String'] } }
> +
> +##
> +# @NetdevTapOptions
> +#
> +# Connect the host TAP network interface name to the VLAN.
> +#
> +# @ifname: #optional interface name
> +#
> +# @fd: #optional file descriptor of an already opened tap
> +#
> +# @script: #optional script to initialize the interface
> +#
> +# @downscript: #optional script to shut down the interface
> +#
> +# @helper: #optional command to execute to configure bridge
> +#
> +# @sndbuf: #optional send buffer limit. Understands [TGMKkb] suffixes.
> +#
> +# @vnet_hdr: #optional enable the IFF_VNET_HDR flag on the tap interface
> +#
> +# @vhost: #optional enable vhost-net network accelerator
> +#
> +# @vhostfd: #optional file descriptor of an already opened vhost net device
> +#
> +# @vhostforce: #optional vhost on for non-MSIX virtio guests
> +#
> +# Since 1.2
> +##
> +{ 'type': 'NetdevTapOptions',
> + 'data': {
> + '*ifname': 'str',
> + '*fd': 'str',
> + '*script': 'str',
> + '*downscript': 'str',
> + '*helper': 'str',
> + '*sndbuf': 'size',
> + '*vnet_hdr': 'bool',
> + '*vhost': 'bool',
> + '*vhostfd': 'str',
> + '*vhostforce': 'bool' } }
> +
> +##
> +# @NetdevSocketOptions
> +#
> +# Connect the VLAN to a remote VLAN in another QEMU virtual machine using a TCP
> +# socket connection.
> +#
> +# @fd: #optional file descriptor of an already opened socket
> +#
> +# @listen: #optional port number, and optional hostname, to listen on
> +#
> +# @connect: #optional port number, and optional hostname, to connect to
> +#
> +# @mcast: #optional UDP multicast address and port number
> +#
> +# @localaddr: #optional source address and port for multicast and udp packets
> +#
> +# @udp: #optional UDP unicast address and port number
> +#
> +# Since 1.2
> +##
> +{ 'type': 'NetdevSocketOptions',
> + 'data': {
> + '*fd': 'str',
> + '*listen': 'str',
> + '*connect': 'str',
> + '*mcast': 'str',
> + '*localaddr': 'str',
> + '*udp': 'str' } }
> +
> +##
> +# @NetdevVdeOptions
> +#
> +# Connect the VLAN to a vde switch running on the host.
> +#
> +# @sock: #optional socket path
> +#
> +# @port: #optional port number
> +#
> +# @group: #optional group owner of socket
> +#
> +# @mode: #optional permissions for socket
> +#
> +# Since 1.2
> +##
> +{ 'type': 'NetdevVdeOptions',
> + 'data': {
> + '*sock': 'str',
> + '*port': 'uint16',
> + '*group': 'str',
> + '*mode': 'uint16' } }
> +
> +##
> +# @NetdevDumpOptions
> +#
> +# Dump VLAN network traffic to a file.
> +#
> +# @len: #optional per-packet size limit (64k default). Understands [TGMKkb]
> +# suffixes.
> +#
> +# @file: #optional dump file path (default is qemu-vlan0.pcap)
> +#
> +# Since 1.2
> +##
> +{ 'type': 'NetdevDumpOptions',
> + 'data': {
> + '*len': 'size',
> + '*file': 'str' } }
> +
> +##
> +# @NetdevBridgeOptions
> +#
> +# Connect a host TAP network interface to a host bridge device.
> +#
> +# @br: #optional bridge name
> +#
> +# @helper: #optional command to execute to configure bridge
> +#
> +# Since 1.2
> +##
> +{ 'type': 'NetdevBridgeOptions',
> + 'data': {
> + '*br': 'str',
> + '*helper': 'str' } }
> +
> +##
> +# @NetClientOptions
> +#
> +# A discriminated record of network device traits.
> +#
> +# Since 1.2
> +##
> +{ 'union': 'NetClientOptions',
> + 'data': {
> + 'none': 'NetdevNoneOptions',
> + 'nic': 'NetLegacyNicOptions',
> + 'user': 'NetdevUserOptions',
> + 'tap': 'NetdevTapOptions',
> + 'socket': 'NetdevSocketOptions',
> + 'vde': 'NetdevVdeOptions',
> + 'dump': 'NetdevDumpOptions',
> + 'bridge': 'NetdevBridgeOptions' } }
> +
> +##
> +# @NetLegacy
> +#
> +# Captures the configuration of a network device; legacy.
> +#
> +# @vlan: #optional vlan number
> +#
> +# @name: #optional identifier for monitor commands
> +#
> +# @traits: device type specific properties (legacy)
> +#
> +# Since 1.2
> +##
> +{ 'type': 'NetLegacy',
> + 'data': {
> + '*vlan': 'int32',
> + '*name': 'str',
> + 'opts': 'NetClientOptions' } }
> +
> +##
> +# @Netdev
> +#
> +# Captures the configuration of a network device.
> +#
> +# @id: identifier for monitor commands.
> +#
> +# @traits: device type specific properties
> +#
> +# Since 1.2
> +##
> +{ 'type': 'Netdev',
> + 'data': {
> + 'id': 'str',
> + 'opts': 'NetClientOptions' } }
>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v2 08/17] hw, net: "net_client_type" -> "NetClientOptionsKind" (qapi-generated)
2012-06-13 8:22 [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing Laszlo Ersek
` (6 preceding siblings ...)
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 07/17] qapi schema: add Netdev types Laszlo Ersek
@ 2012-06-13 8:22 ` Laszlo Ersek
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 09/17] convert net_client_init() to OptsVisitor Laszlo Ersek
` (10 subsequent siblings)
18 siblings, 0 replies; 38+ messages in thread
From: Laszlo Ersek @ 2012-06-13 8:22 UTC (permalink / raw)
To: qemu-devel, lersek
NET_CLIENT_TYPE_ -> NET_CLIENT_OPTIONS_KIND_
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
net.h | 16 +-------------
hw/cadence_gem.c | 2 +-
hw/dp8393x.c | 2 +-
hw/e1000.c | 2 +-
hw/eepro100.c | 2 +-
hw/etraxfs_eth.c | 2 +-
hw/lan9118.c | 2 +-
hw/lance.c | 2 +-
hw/mcf_fec.c | 2 +-
hw/milkymist-minimac2.c | 2 +-
hw/mipsnet.c | 2 +-
hw/musicpal.c | 2 +-
hw/ne2000-isa.c | 2 +-
hw/ne2000.c | 2 +-
hw/opencores_eth.c | 2 +-
hw/pcnet-pci.c | 2 +-
hw/rtl8139.c | 2 +-
hw/smc91c111.c | 2 +-
hw/spapr_llan.c | 2 +-
hw/stellaris_enet.c | 2 +-
hw/usb/dev-network.c | 2 +-
hw/vhost_net.c | 2 +-
hw/virtio-net.c | 10 ++++----
hw/xen_nic.c | 2 +-
hw/xgmac.c | 2 +-
hw/xilinx_axienet.c | 2 +-
hw/xilinx_ethlite.c | 2 +-
net.c | 50 +++++++++++++++++++++++-----------------------
net/dump.c | 2 +-
net/slirp.c | 2 +-
net/socket.c | 4 +-
net/tap-win32.c | 2 +-
net/tap.c | 16 +++++++-------
net/vde.c | 2 +-
34 files changed, 71 insertions(+), 83 deletions(-)
diff --git a/net.h b/net.h
index bdc2a06..b0b8c7a 100644
--- a/net.h
+++ b/net.h
@@ -7,6 +7,7 @@
#include "qemu-option.h"
#include "net/queue.h"
#include "vmstate.h"
+#include "qapi-types.h"
struct MACAddr {
uint8_t a[6];
@@ -29,19 +30,6 @@ typedef struct NICConf {
/* VLANs support */
-typedef enum {
- NET_CLIENT_TYPE_NONE,
- NET_CLIENT_TYPE_NIC,
- NET_CLIENT_TYPE_USER,
- NET_CLIENT_TYPE_TAP,
- NET_CLIENT_TYPE_SOCKET,
- NET_CLIENT_TYPE_VDE,
- NET_CLIENT_TYPE_DUMP,
- NET_CLIENT_TYPE_BRIDGE,
-
- NET_CLIENT_TYPE_MAX
-} net_client_type;
-
typedef void (NetPoll)(VLANClientState *, bool enable);
typedef int (NetCanReceive)(VLANClientState *);
typedef ssize_t (NetReceive)(VLANClientState *, const uint8_t *, size_t);
@@ -50,7 +38,7 @@ typedef void (NetCleanup) (VLANClientState *);
typedef void (LinkStatusChanged)(VLANClientState *);
typedef struct NetClientInfo {
- net_client_type type;
+ NetClientOptionsKind type;
size_t size;
NetReceive *receive;
NetReceive *receive_raw;
diff --git a/hw/cadence_gem.c b/hw/cadence_gem.c
index e2140ae..f184620 100644
--- a/hw/cadence_gem.c
+++ b/hw/cadence_gem.c
@@ -1161,7 +1161,7 @@ static void gem_set_link(VLANClientState *nc)
}
static NetClientInfo net_gem_info = {
- .type = NET_CLIENT_TYPE_NIC,
+ .type = NET_CLIENT_OPTIONS_KIND_NIC,
.size = sizeof(NICState),
.can_receive = gem_can_receive,
.receive = gem_receive,
diff --git a/hw/dp8393x.c b/hw/dp8393x.c
index 017d074..756d630 100644
--- a/hw/dp8393x.c
+++ b/hw/dp8393x.c
@@ -872,7 +872,7 @@ static void nic_cleanup(VLANClientState *nc)
}
static NetClientInfo net_dp83932_info = {
- .type = NET_CLIENT_TYPE_NIC,
+ .type = NET_CLIENT_OPTIONS_KIND_NIC,
.size = sizeof(NICState),
.can_receive = nic_can_receive,
.receive = nic_receive,
diff --git a/hw/e1000.c b/hw/e1000.c
index 4573f13..ad24298 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -1206,7 +1206,7 @@ pci_e1000_uninit(PCIDevice *dev)
}
static NetClientInfo net_e1000_info = {
- .type = NET_CLIENT_TYPE_NIC,
+ .type = NET_CLIENT_OPTIONS_KIND_NIC,
.size = sizeof(NICState),
.can_receive = e1000_can_receive,
.receive = e1000_receive,
diff --git a/hw/eepro100.c b/hw/eepro100.c
index 6279ae3..f343685 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -1845,7 +1845,7 @@ static int pci_nic_uninit(PCIDevice *pci_dev)
}
static NetClientInfo net_eepro100_info = {
- .type = NET_CLIENT_TYPE_NIC,
+ .type = NET_CLIENT_OPTIONS_KIND_NIC,
.size = sizeof(NICState),
.can_receive = nic_can_receive,
.receive = nic_receive,
diff --git a/hw/etraxfs_eth.c b/hw/etraxfs_eth.c
index 16a0637..45fb40c 100644
--- a/hw/etraxfs_eth.c
+++ b/hw/etraxfs_eth.c
@@ -579,7 +579,7 @@ static void eth_cleanup(VLANClientState *nc)
}
static NetClientInfo net_etraxfs_info = {
- .type = NET_CLIENT_TYPE_NIC,
+ .type = NET_CLIENT_OPTIONS_KIND_NIC,
.size = sizeof(NICState),
.can_receive = eth_can_receive,
.receive = eth_receive,
diff --git a/hw/lan9118.c b/hw/lan9118.c
index 7b4fe87..40fb765 100644
--- a/hw/lan9118.c
+++ b/hw/lan9118.c
@@ -1310,7 +1310,7 @@ static void lan9118_cleanup(VLANClientState *nc)
}
static NetClientInfo net_lan9118_info = {
- .type = NET_CLIENT_TYPE_NIC,
+ .type = NET_CLIENT_OPTIONS_KIND_NIC,
.size = sizeof(NICState),
.can_receive = lan9118_can_receive,
.receive = lan9118_receive,
diff --git a/hw/lance.c b/hw/lance.c
index ce3d46c..91c0e16 100644
--- a/hw/lance.c
+++ b/hw/lance.c
@@ -93,7 +93,7 @@ static void lance_cleanup(VLANClientState *nc)
}
static NetClientInfo net_lance_info = {
- .type = NET_CLIENT_TYPE_NIC,
+ .type = NET_CLIENT_OPTIONS_KIND_NIC,
.size = sizeof(NICState),
.can_receive = pcnet_can_receive,
.receive = pcnet_receive,
diff --git a/hw/mcf_fec.c b/hw/mcf_fec.c
index ae37bef..4ab4ff5 100644
--- a/hw/mcf_fec.c
+++ b/hw/mcf_fec.c
@@ -450,7 +450,7 @@ static void mcf_fec_cleanup(VLANClientState *nc)
}
static NetClientInfo net_mcf_fec_info = {
- .type = NET_CLIENT_TYPE_NIC,
+ .type = NET_CLIENT_OPTIONS_KIND_NIC,
.size = sizeof(NICState),
.can_receive = mcf_fec_can_receive,
.receive = mcf_fec_receive,
diff --git a/hw/milkymist-minimac2.c b/hw/milkymist-minimac2.c
index 70bf336..3924b83 100644
--- a/hw/milkymist-minimac2.c
+++ b/hw/milkymist-minimac2.c
@@ -448,7 +448,7 @@ static void milkymist_minimac2_reset(DeviceState *d)
}
static NetClientInfo net_milkymist_minimac2_info = {
- .type = NET_CLIENT_TYPE_NIC,
+ .type = NET_CLIENT_OPTIONS_KIND_NIC,
.size = sizeof(NICState),
.can_receive = minimac2_can_rx,
.receive = minimac2_rx,
diff --git a/hw/mipsnet.c b/hw/mipsnet.c
index 3107246..3385be7 100644
--- a/hw/mipsnet.c
+++ b/hw/mipsnet.c
@@ -217,7 +217,7 @@ static void mipsnet_cleanup(VLANClientState *nc)
}
static NetClientInfo net_mipsnet_info = {
- .type = NET_CLIENT_TYPE_NIC,
+ .type = NET_CLIENT_OPTIONS_KIND_NIC,
.size = sizeof(NICState),
.can_receive = mipsnet_can_receive,
.receive = mipsnet_receive,
diff --git a/hw/musicpal.c b/hw/musicpal.c
index f14f20d..448897f 100644
--- a/hw/musicpal.c
+++ b/hw/musicpal.c
@@ -374,7 +374,7 @@ static void eth_cleanup(VLANClientState *nc)
}
static NetClientInfo net_mv88w8618_info = {
- .type = NET_CLIENT_TYPE_NIC,
+ .type = NET_CLIENT_OPTIONS_KIND_NIC,
.size = sizeof(NICState),
.can_receive = eth_can_receive,
.receive = eth_receive,
diff --git a/hw/ne2000-isa.c b/hw/ne2000-isa.c
index a4a783a..99ed965 100644
--- a/hw/ne2000-isa.c
+++ b/hw/ne2000-isa.c
@@ -44,7 +44,7 @@ static void isa_ne2000_cleanup(VLANClientState *nc)
}
static NetClientInfo net_ne2000_isa_info = {
- .type = NET_CLIENT_TYPE_NIC,
+ .type = NET_CLIENT_OPTIONS_KIND_NIC,
.size = sizeof(NICState),
.can_receive = ne2000_can_receive,
.receive = ne2000_receive,
diff --git a/hw/ne2000.c b/hw/ne2000.c
index d02e60c..760ed29 100644
--- a/hw/ne2000.c
+++ b/hw/ne2000.c
@@ -711,7 +711,7 @@ static void ne2000_cleanup(VLANClientState *nc)
}
static NetClientInfo net_ne2000_info = {
- .type = NET_CLIENT_TYPE_NIC,
+ .type = NET_CLIENT_OPTIONS_KIND_NIC,
.size = sizeof(NICState),
.can_receive = ne2000_can_receive,
.receive = ne2000_receive,
diff --git a/hw/opencores_eth.c b/hw/opencores_eth.c
index 350f731..f4498d4 100644
--- a/hw/opencores_eth.c
+++ b/hw/opencores_eth.c
@@ -467,7 +467,7 @@ static void open_eth_cleanup(VLANClientState *nc)
}
static NetClientInfo net_open_eth_info = {
- .type = NET_CLIENT_TYPE_NIC,
+ .type = NET_CLIENT_OPTIONS_KIND_NIC,
.size = sizeof(NICState),
.can_receive = open_eth_can_receive,
.receive = open_eth_receive,
diff --git a/hw/pcnet-pci.c b/hw/pcnet-pci.c
index 34d73aa..931fedd 100644
--- a/hw/pcnet-pci.c
+++ b/hw/pcnet-pci.c
@@ -284,7 +284,7 @@ static int pci_pcnet_uninit(PCIDevice *dev)
}
static NetClientInfo net_pci_pcnet_info = {
- .type = NET_CLIENT_TYPE_NIC,
+ .type = NET_CLIENT_OPTIONS_KIND_NIC,
.size = sizeof(NICState),
.can_receive = pcnet_can_receive,
.receive = pcnet_receive,
diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index eb22d04..aae2de4 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -3453,7 +3453,7 @@ static int pci_rtl8139_uninit(PCIDevice *dev)
}
static NetClientInfo net_rtl8139_info = {
- .type = NET_CLIENT_TYPE_NIC,
+ .type = NET_CLIENT_OPTIONS_KIND_NIC,
.size = sizeof(NICState),
.can_receive = rtl8139_can_receive,
.receive = rtl8139_receive,
diff --git a/hw/smc91c111.c b/hw/smc91c111.c
index 1a5213f..451ede0 100644
--- a/hw/smc91c111.c
+++ b/hw/smc91c111.c
@@ -736,7 +736,7 @@ static void smc91c111_cleanup(VLANClientState *nc)
}
static NetClientInfo net_smc91c111_info = {
- .type = NET_CLIENT_TYPE_NIC,
+ .type = NET_CLIENT_OPTIONS_KIND_NIC,
.size = sizeof(NICState),
.can_receive = smc91c111_can_receive,
.receive = smc91c111_receive,
diff --git a/hw/spapr_llan.c b/hw/spapr_llan.c
index 8313043..9a1ac4f 100644
--- a/hw/spapr_llan.c
+++ b/hw/spapr_llan.c
@@ -176,7 +176,7 @@ static ssize_t spapr_vlan_receive(VLANClientState *nc, const uint8_t *buf,
}
static NetClientInfo net_spapr_vlan_info = {
- .type = NET_CLIENT_TYPE_NIC,
+ .type = NET_CLIENT_OPTIONS_KIND_NIC,
.size = sizeof(NICState),
.can_receive = spapr_vlan_can_receive,
.receive = spapr_vlan_receive,
diff --git a/hw/stellaris_enet.c b/hw/stellaris_enet.c
index fbe99cb..b593cd0 100644
--- a/hw/stellaris_enet.c
+++ b/hw/stellaris_enet.c
@@ -393,7 +393,7 @@ static void stellaris_enet_cleanup(VLANClientState *nc)
}
static NetClientInfo net_stellaris_enet_info = {
- .type = NET_CLIENT_TYPE_NIC,
+ .type = NET_CLIENT_OPTIONS_KIND_NIC,
.size = sizeof(NICState),
.can_receive = stellaris_enet_can_receive,
.receive = stellaris_enet_receive,
diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
index 5d2f098..f40c349 100644
--- a/hw/usb/dev-network.c
+++ b/hw/usb/dev-network.c
@@ -1313,7 +1313,7 @@ static void usb_net_handle_destroy(USBDevice *dev)
}
static NetClientInfo net_usbnet_info = {
- .type = NET_CLIENT_TYPE_NIC,
+ .type = NET_CLIENT_OPTIONS_KIND_NIC,
.size = sizeof(NICState),
.can_receive = usbnet_can_receive,
.receive = usbnet_receive,
diff --git a/hw/vhost_net.c b/hw/vhost_net.c
index f672e9d..75f8211 100644
--- a/hw/vhost_net.c
+++ b/hw/vhost_net.c
@@ -83,7 +83,7 @@ void vhost_net_ack_features(struct vhost_net *net, unsigned features)
static int vhost_net_get_fd(VLANClientState *backend)
{
switch (backend->info->type) {
- case NET_CLIENT_TYPE_TAP:
+ case NET_CLIENT_OPTIONS_KIND_TAP:
return tap_get_fd(backend);
default:
fprintf(stderr, "vhost-net requires tap backend\n");
diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 3f190d4..5d5ead4 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -108,7 +108,7 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
if (!n->nic->nc.peer) {
return;
}
- if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
+ if (n->nic->nc.peer->info->type != NET_CLIENT_OPTIONS_KIND_TAP) {
return;
}
@@ -205,7 +205,7 @@ static int peer_has_vnet_hdr(VirtIONet *n)
if (!n->nic->nc.peer)
return 0;
- if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP)
+ if (n->nic->nc.peer->info->type != NET_CLIENT_OPTIONS_KIND_TAP)
return 0;
n->has_vnet_hdr = tap_has_vnet_hdr(n->nic->nc.peer);
@@ -249,7 +249,7 @@ static uint32_t virtio_net_get_features(VirtIODevice *vdev, uint32_t features)
}
if (!n->nic->nc.peer ||
- n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
+ n->nic->nc.peer->info->type != NET_CLIENT_OPTIONS_KIND_TAP) {
return features;
}
if (!tap_get_vhost_net(n->nic->nc.peer)) {
@@ -288,7 +288,7 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
(features >> VIRTIO_NET_F_GUEST_UFO) & 1);
}
if (!n->nic->nc.peer ||
- n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
+ n->nic->nc.peer->info->type != NET_CLIENT_OPTIONS_KIND_TAP) {
return;
}
if (!tap_get_vhost_net(n->nic->nc.peer)) {
@@ -988,7 +988,7 @@ static void virtio_net_cleanup(VLANClientState *nc)
}
static NetClientInfo net_virtio_info = {
- .type = NET_CLIENT_TYPE_NIC,
+ .type = NET_CLIENT_OPTIONS_KIND_NIC,
.size = sizeof(NICState),
.can_receive = virtio_net_can_receive,
.receive = virtio_net_receive,
diff --git a/hw/xen_nic.c b/hw/xen_nic.c
index 9a59bda..3866743 100644
--- a/hw/xen_nic.c
+++ b/hw/xen_nic.c
@@ -304,7 +304,7 @@ static ssize_t net_rx_packet(VLANClientState *nc, const uint8_t *buf, size_t siz
/* ------------------------------------------------------------- */
static NetClientInfo net_xen_info = {
- .type = NET_CLIENT_TYPE_NIC,
+ .type = NET_CLIENT_OPTIONS_KIND_NIC,
.size = sizeof(NICState),
.can_receive = net_rx_ok,
.receive = net_rx_packet,
diff --git a/hw/xgmac.c b/hw/xgmac.c
index dd4bdc4..e539681 100644
--- a/hw/xgmac.c
+++ b/hw/xgmac.c
@@ -371,7 +371,7 @@ static void eth_cleanup(VLANClientState *nc)
}
static NetClientInfo net_xgmac_enet_info = {
- .type = NET_CLIENT_TYPE_NIC,
+ .type = NET_CLIENT_OPTIONS_KIND_NIC,
.size = sizeof(NICState),
.can_receive = eth_can_rx,
.receive = eth_rx,
diff --git a/hw/xilinx_axienet.c b/hw/xilinx_axienet.c
index 7526273..993a79f 100644
--- a/hw/xilinx_axienet.c
+++ b/hw/xilinx_axienet.c
@@ -832,7 +832,7 @@ axienet_stream_push(void *opaque, uint8_t *buf, size_t size, uint32_t *hdr)
}
static NetClientInfo net_xilinx_enet_info = {
- .type = NET_CLIENT_TYPE_NIC,
+ .type = NET_CLIENT_OPTIONS_KIND_NIC,
.size = sizeof(NICState),
.can_receive = eth_can_rx,
.receive = eth_rx,
diff --git a/hw/xilinx_ethlite.c b/hw/xilinx_ethlite.c
index 857b33d..1560a20 100644
--- a/hw/xilinx_ethlite.c
+++ b/hw/xilinx_ethlite.c
@@ -202,7 +202,7 @@ static void eth_cleanup(VLANClientState *nc)
}
static NetClientInfo net_xilinx_ethlite_info = {
- .type = NET_CLIENT_TYPE_NIC,
+ .type = NET_CLIENT_OPTIONS_KIND_NIC,
.size = sizeof(NICState),
.can_receive = eth_can_rx,
.receive = eth_rx,
diff --git a/net.c b/net.c
index 4aa416c..9c8cd42 100644
--- a/net.c
+++ b/net.c
@@ -239,7 +239,7 @@ NICState *qemu_new_nic(NetClientInfo *info,
VLANClientState *nc;
NICState *nic;
- assert(info->type == NET_CLIENT_TYPE_NIC);
+ assert(info->type == NET_CLIENT_OPTIONS_KIND_NIC);
assert(info->size >= sizeof(NICState));
nc = qemu_new_net_client(info, conf->vlan, conf->peer, model, name);
@@ -282,7 +282,7 @@ static void qemu_free_vlan_client(VLANClientState *vc)
void qemu_del_vlan_client(VLANClientState *vc)
{
/* If there is a peer NIC, delete and cleanup client, but do not free. */
- if (!vc->vlan && vc->peer && vc->peer->info->type == NET_CLIENT_TYPE_NIC) {
+ if (!vc->vlan && vc->peer && vc->peer->info->type == NET_CLIENT_OPTIONS_KIND_NIC) {
NICState *nic = DO_UPCAST(NICState, nc, vc->peer);
if (nic->peer_deleted) {
return;
@@ -298,7 +298,7 @@ void qemu_del_vlan_client(VLANClientState *vc)
}
/* If this is a peer NIC and peer has already been deleted, free it now. */
- if (!vc->vlan && vc->peer && vc->info->type == NET_CLIENT_TYPE_NIC) {
+ if (!vc->vlan && vc->peer && vc->info->type == NET_CLIENT_OPTIONS_KIND_NIC) {
NICState *nic = DO_UPCAST(NICState, nc, vc);
if (nic->peer_deleted) {
qemu_free_vlan_client(vc->peer);
@@ -341,14 +341,14 @@ void qemu_foreach_nic(qemu_nic_foreach func, void *opaque)
VLANState *vlan;
QTAILQ_FOREACH(nc, &non_vlan_clients, next) {
- if (nc->info->type == NET_CLIENT_TYPE_NIC) {
+ if (nc->info->type == NET_CLIENT_OPTIONS_KIND_NIC) {
func(DO_UPCAST(NICState, nc, nc), opaque);
}
}
QTAILQ_FOREACH(vlan, &vlans, next) {
QTAILQ_FOREACH(nc, &vlan->clients, next) {
- if (nc->info->type == NET_CLIENT_TYPE_NIC) {
+ if (nc->info->type == NET_CLIENT_OPTIONS_KIND_NIC) {
func(DO_UPCAST(NICState, nc, nc), opaque);
}
}
@@ -664,7 +664,7 @@ VLANClientState *qemu_find_netdev(const char *id)
VLANClientState *vc;
QTAILQ_FOREACH(vc, &non_vlan_clients, next) {
- if (vc->info->type == NET_CLIENT_TYPE_NIC)
+ if (vc->info->type == NET_CLIENT_OPTIONS_KIND_NIC)
continue;
if (!strcmp(vc->name, id)) {
return vc;
@@ -828,15 +828,15 @@ static const struct {
const char *type;
net_client_init_func init;
QemuOptDesc desc[NET_MAX_DESC];
-} net_client_types[NET_CLIENT_TYPE_MAX] = {
- [NET_CLIENT_TYPE_NONE] = {
+} net_client_types[NET_CLIENT_OPTIONS_KIND_MAX] = {
+ [NET_CLIENT_OPTIONS_KIND_NONE] = {
.type = "none",
.desc = {
NET_COMMON_PARAMS_DESC,
{ /* end of list */ }
},
},
- [NET_CLIENT_TYPE_NIC] = {
+ [NET_CLIENT_OPTIONS_KIND_NIC] = {
.type = "nic",
.init = net_init_nic,
.desc = {
@@ -867,7 +867,7 @@ static const struct {
},
},
#ifdef CONFIG_SLIRP
- [NET_CLIENT_TYPE_USER] = {
+ [NET_CLIENT_OPTIONS_KIND_USER] = {
.type = "user",
.init = net_init_slirp,
.desc = {
@@ -929,7 +929,7 @@ static const struct {
},
},
#endif
- [NET_CLIENT_TYPE_TAP] = {
+ [NET_CLIENT_OPTIONS_KIND_TAP] = {
.type = "tap",
.init = net_init_tap,
.desc = {
@@ -983,7 +983,7 @@ static const struct {
{ /* end of list */ }
},
},
- [NET_CLIENT_TYPE_SOCKET] = {
+ [NET_CLIENT_OPTIONS_KIND_SOCKET] = {
.type = "socket",
.init = net_init_socket,
.desc = {
@@ -1017,7 +1017,7 @@ static const struct {
},
},
#ifdef CONFIG_VDE
- [NET_CLIENT_TYPE_VDE] = {
+ [NET_CLIENT_OPTIONS_KIND_VDE] = {
.type = "vde",
.init = net_init_vde,
.desc = {
@@ -1043,7 +1043,7 @@ static const struct {
},
},
#endif
- [NET_CLIENT_TYPE_DUMP] = {
+ [NET_CLIENT_OPTIONS_KIND_DUMP] = {
.type = "dump",
.init = net_init_dump,
.desc = {
@@ -1061,7 +1061,7 @@ static const struct {
},
},
#ifdef CONFIG_NET_BRIDGE
- [NET_CLIENT_TYPE_BRIDGE] = {
+ [NET_CLIENT_OPTIONS_KIND_BRIDGE] = {
.type = "bridge",
.init = net_init_bridge,
.desc = {
@@ -1129,7 +1129,7 @@ int net_client_init(QemuOpts *opts, int is_netdev, Error **errp)
name = qemu_opt_get(opts, "name");
}
- for (i = 0; i < NET_CLIENT_TYPE_MAX; i++) {
+ for (i = 0; i < NET_CLIENT_OPTIONS_KIND_MAX; i++) {
if (net_client_types[i].type != NULL &&
!strcmp(net_client_types[i].type, type)) {
Error *local_err = NULL;
@@ -1293,7 +1293,7 @@ void do_info_network(Monitor *mon)
{
VLANState *vlan;
VLANClientState *vc, *peer;
- net_client_type type;
+ NetClientOptionsKind type;
QTAILQ_FOREACH(vlan, &vlans, next) {
monitor_printf(mon, "VLAN %d devices:\n", vlan->id);
@@ -1307,11 +1307,11 @@ void do_info_network(Monitor *mon)
QTAILQ_FOREACH(vc, &non_vlan_clients, next) {
peer = vc->peer;
type = vc->info->type;
- if (!peer || type == NET_CLIENT_TYPE_NIC) {
+ if (!peer || type == NET_CLIENT_OPTIONS_KIND_NIC) {
monitor_printf(mon, " ");
print_net_client(mon, vc);
} /* else it's a netdev connected to a NIC, printed with the NIC */
- if (peer && type == NET_CLIENT_TYPE_NIC) {
+ if (peer && type == NET_CLIENT_OPTIONS_KIND_NIC) {
monitor_printf(mon, " \\ ");
print_net_client(mon, peer);
}
@@ -1399,13 +1399,13 @@ void net_check_clients(void)
QTAILQ_FOREACH(vc, &vlan->clients, next) {
switch (vc->info->type) {
- case NET_CLIENT_TYPE_NIC:
+ case NET_CLIENT_OPTIONS_KIND_NIC:
has_nic = 1;
break;
- case NET_CLIENT_TYPE_USER:
- case NET_CLIENT_TYPE_TAP:
- case NET_CLIENT_TYPE_SOCKET:
- case NET_CLIENT_TYPE_VDE:
+ case NET_CLIENT_OPTIONS_KIND_USER:
+ case NET_CLIENT_OPTIONS_KIND_TAP:
+ case NET_CLIENT_OPTIONS_KIND_SOCKET:
+ case NET_CLIENT_OPTIONS_KIND_VDE:
has_host_dev = 1;
break;
default: ;
@@ -1421,7 +1421,7 @@ void net_check_clients(void)
QTAILQ_FOREACH(vc, &non_vlan_clients, next) {
if (!vc->peer) {
fprintf(stderr, "Warning: %s %s has no peer\n",
- vc->info->type == NET_CLIENT_TYPE_NIC ? "nic" : "netdev",
+ vc->info->type == NET_CLIENT_OPTIONS_KIND_NIC ? "nic" : "netdev",
vc->name);
}
}
diff --git a/net/dump.c b/net/dump.c
index f835c51..2124b9a 100644
--- a/net/dump.c
+++ b/net/dump.c
@@ -93,7 +93,7 @@ static void dump_cleanup(VLANClientState *nc)
}
static NetClientInfo net_dump_info = {
- .type = NET_CLIENT_TYPE_DUMP,
+ .type = NET_CLIENT_OPTIONS_KIND_DUMP,
.size = sizeof(DumpState),
.receive = dump_receive,
.cleanup = dump_cleanup,
diff --git a/net/slirp.c b/net/slirp.c
index 37b6ccf..9076d99 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -128,7 +128,7 @@ static void net_slirp_cleanup(VLANClientState *nc)
}
static NetClientInfo net_slirp_info = {
- .type = NET_CLIENT_TYPE_USER,
+ .type = NET_CLIENT_OPTIONS_KIND_USER,
.size = sizeof(SlirpState),
.receive = net_slirp_receive,
.cleanup = net_slirp_cleanup,
diff --git a/net/socket.c b/net/socket.c
index fcd0a3c..30536ef 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -239,7 +239,7 @@ static void net_socket_cleanup(VLANClientState *nc)
}
static NetClientInfo net_dgram_socket_info = {
- .type = NET_CLIENT_TYPE_SOCKET,
+ .type = NET_CLIENT_OPTIONS_KIND_SOCKET,
.size = sizeof(NetSocketState),
.receive = net_socket_receive_dgram,
.cleanup = net_socket_cleanup,
@@ -317,7 +317,7 @@ static void net_socket_connect(void *opaque)
}
static NetClientInfo net_socket_info = {
- .type = NET_CLIENT_TYPE_SOCKET,
+ .type = NET_CLIENT_OPTIONS_KIND_SOCKET,
.size = sizeof(NetSocketState),
.receive = net_socket_receive,
.cleanup = net_socket_cleanup,
diff --git a/net/tap-win32.c b/net/tap-win32.c
index a801a55..f7b6129 100644
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -667,7 +667,7 @@ static void tap_win32_send(void *opaque)
}
static NetClientInfo net_tap_win32_info = {
- .type = NET_CLIENT_TYPE_TAP,
+ .type = NET_CLIENT_OPTIONS_KIND_TAP,
.size = sizeof(TAPState),
.receive = tap_receive,
.cleanup = tap_cleanup,
diff --git a/net/tap.c b/net/tap.c
index 5ac4ba3..4cba566 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -217,7 +217,7 @@ int tap_has_ufo(VLANClientState *nc)
{
TAPState *s = DO_UPCAST(TAPState, nc, nc);
- assert(nc->info->type == NET_CLIENT_TYPE_TAP);
+ assert(nc->info->type == NET_CLIENT_OPTIONS_KIND_TAP);
return s->has_ufo;
}
@@ -226,7 +226,7 @@ int tap_has_vnet_hdr(VLANClientState *nc)
{
TAPState *s = DO_UPCAST(TAPState, nc, nc);
- assert(nc->info->type == NET_CLIENT_TYPE_TAP);
+ assert(nc->info->type == NET_CLIENT_OPTIONS_KIND_TAP);
return !!s->host_vnet_hdr_len;
}
@@ -235,7 +235,7 @@ int tap_has_vnet_hdr_len(VLANClientState *nc, int len)
{
TAPState *s = DO_UPCAST(TAPState, nc, nc);
- assert(nc->info->type == NET_CLIENT_TYPE_TAP);
+ assert(nc->info->type == NET_CLIENT_OPTIONS_KIND_TAP);
return tap_probe_vnet_hdr_len(s->fd, len);
}
@@ -244,7 +244,7 @@ void tap_set_vnet_hdr_len(VLANClientState *nc, int len)
{
TAPState *s = DO_UPCAST(TAPState, nc, nc);
- assert(nc->info->type == NET_CLIENT_TYPE_TAP);
+ assert(nc->info->type == NET_CLIENT_OPTIONS_KIND_TAP);
assert(len == sizeof(struct virtio_net_hdr_mrg_rxbuf) ||
len == sizeof(struct virtio_net_hdr));
@@ -258,7 +258,7 @@ void tap_using_vnet_hdr(VLANClientState *nc, int using_vnet_hdr)
using_vnet_hdr = using_vnet_hdr != 0;
- assert(nc->info->type == NET_CLIENT_TYPE_TAP);
+ assert(nc->info->type == NET_CLIENT_OPTIONS_KIND_TAP);
assert(!!s->host_vnet_hdr_len == using_vnet_hdr);
s->using_vnet_hdr = using_vnet_hdr;
@@ -305,14 +305,14 @@ static void tap_poll(VLANClientState *nc, bool enable)
int tap_get_fd(VLANClientState *nc)
{
TAPState *s = DO_UPCAST(TAPState, nc, nc);
- assert(nc->info->type == NET_CLIENT_TYPE_TAP);
+ assert(nc->info->type == NET_CLIENT_OPTIONS_KIND_TAP);
return s->fd;
}
/* fd support */
static NetClientInfo net_tap_info = {
- .type = NET_CLIENT_TYPE_TAP,
+ .type = NET_CLIENT_OPTIONS_KIND_TAP,
.size = sizeof(TAPState),
.receive = tap_receive,
.receive_raw = tap_receive_raw,
@@ -710,6 +710,6 @@ int net_init_tap(QemuOpts *opts, const char *name, VLANState *vlan)
VHostNetState *tap_get_vhost_net(VLANClientState *nc)
{
TAPState *s = DO_UPCAST(TAPState, nc, nc);
- assert(nc->info->type == NET_CLIENT_TYPE_TAP);
+ assert(nc->info->type == NET_CLIENT_OPTIONS_KIND_TAP);
return s->vhost_net;
}
diff --git a/net/vde.c b/net/vde.c
index 6b9d452..0e8bf23 100644
--- a/net/vde.c
+++ b/net/vde.c
@@ -69,7 +69,7 @@ static void vde_cleanup(VLANClientState *nc)
}
static NetClientInfo net_vde_info = {
- .type = NET_CLIENT_TYPE_VDE,
+ .type = NET_CLIENT_OPTIONS_KIND_VDE,
.size = sizeof(VDEState),
.receive = vde_receive,
.cleanup = vde_cleanup,
--
1.7.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v2 09/17] convert net_client_init() to OptsVisitor
2012-06-13 8:22 [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing Laszlo Ersek
` (7 preceding siblings ...)
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 08/17] hw, net: "net_client_type" -> "NetClientOptionsKind" (qapi-generated) Laszlo Ersek
@ 2012-06-13 8:22 ` Laszlo Ersek
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 10/17] convert net_init_nic() to NetClientOptions Laszlo Ersek
` (9 subsequent siblings)
18 siblings, 0 replies; 38+ messages in thread
From: Laszlo Ersek @ 2012-06-13 8:22 UTC (permalink / raw)
To: qemu-devel, lersek
The net_client_init() prototype is kept intact.
Based on "is_netdev", the QemuOpts-rooted QemuOpt-list is parsed as a
Netdev or a NetLegacy. The original meat of net_client_init() is moved to
and simplified in net_client_init1():
Fields not common between -net and -netdev are clearly separated. Getting
the name for the init functions is cleaner: Netdev::id is mandatory, and
all init functions handle a NULL NetLegacy::name. NetLegacy::vlan
explicitly depends on -net (see below).
Verifying the "type=" option for -netdev can be turned into a switch.
Format validation with qemu_opts_validate() can be removed because the
visitor covers it. Relatedly, the "net_client_types" array is reduced to
an array of init functions that can be directly indexed by opts->kind.
(Help text is available in the schema JSON.)
The outermost negation in the condition around qemu_find_vlan() was
flattened, because it expresses the dependent code's requirements more
clearly.
VLAN lookup is avoided if there's no init function to pass the VLAN to.
Whenever the value of type=... is needed, we substitute
NetClientOptionsKind_lookup[kind].
The individual init functions are not converted yet, thus the original
QemuOpts instance is passed transparently.
v1->v2:
- NetLegacy::name is optional. Tracked it through all init functions: they
all handle a NULL name. Updated commit message accordingly.
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
net/dump.h | 4 +-
net/slirp.h | 4 +-
net/socket.h | 4 +-
net/tap.h | 7 +-
net/vde.h | 4 +-
net.c | 429 ++++++++++++------------------------------------------
net/dump.c | 3 +-
net/slirp.c | 3 +-
net/socket.c | 3 +-
net/tap-win32.c | 3 +-
net/tap.c | 6 +-
net/vde.c | 3 +-
12 files changed, 127 insertions(+), 346 deletions(-)
diff --git a/net/dump.h b/net/dump.h
index 2b5d9ba..85ac00b 100644
--- a/net/dump.h
+++ b/net/dump.h
@@ -26,7 +26,9 @@
#include "net.h"
#include "qemu-common.h"
+#include "qapi-types.h"
-int net_init_dump(QemuOpts *opts, const char *name, VLANState *vlan);
+int net_init_dump(QemuOpts *opts, const NetClientOptions *new_opts,
+ const char *name, VLANState *vlan);
#endif /* QEMU_NET_DUMP_H */
diff --git a/net/slirp.h b/net/slirp.h
index 53fe95d..ef13a65 100644
--- a/net/slirp.h
+++ b/net/slirp.h
@@ -27,10 +27,12 @@
#include "qemu-common.h"
#include "qdict.h"
#include "qemu-option.h"
+#include "qapi-types.h"
#ifdef CONFIG_SLIRP
-int net_init_slirp(QemuOpts *opts, const char *name, VLANState *vlan);
+int net_init_slirp(QemuOpts *opts, const NetClientOptions *new_opts,
+ const char *name, VLANState *vlan);
void net_slirp_hostfwd_add(Monitor *mon, const QDict *qdict);
void net_slirp_hostfwd_remove(Monitor *mon, const QDict *qdict);
diff --git a/net/socket.h b/net/socket.h
index e1fe959..e44d26e 100644
--- a/net/socket.h
+++ b/net/socket.h
@@ -26,7 +26,9 @@
#include "net.h"
#include "qemu-common.h"
+#include "qapi-types.h"
-int net_init_socket(QemuOpts *opts, const char *name, VLANState *vlan);
+int net_init_socket(QemuOpts *opts, const NetClientOptions *new_opts,
+ const char *name, VLANState *vlan);
#endif /* QEMU_NET_SOCKET_H */
diff --git a/net/tap.h b/net/tap.h
index b2a9450..44e31ce 100644
--- a/net/tap.h
+++ b/net/tap.h
@@ -28,11 +28,13 @@
#include "qemu-common.h"
#include "qemu-option.h"
+#include "qapi-types.h"
#define DEFAULT_NETWORK_SCRIPT "/etc/qemu-ifup"
#define DEFAULT_NETWORK_DOWN_SCRIPT "/etc/qemu-ifdown"
-int net_init_tap(QemuOpts *opts, const char *name, VLANState *vlan);
+int net_init_tap(QemuOpts *opts, const NetClientOptions *new_opts,
+ const char *name, VLANState *vlan);
int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required);
@@ -57,6 +59,7 @@ int tap_get_fd(VLANClientState *vc);
struct vhost_net;
struct vhost_net *tap_get_vhost_net(VLANClientState *vc);
-int net_init_bridge(QemuOpts *opts, const char *name, VLANState *vlan);
+int net_init_bridge(QemuOpts *opts, const NetClientOptions *new_opts,
+ const char *name, VLANState *vlan);
#endif /* QEMU_NET_TAP_H */
diff --git a/net/vde.h b/net/vde.h
index 732e575..5fc17f9 100644
--- a/net/vde.h
+++ b/net/vde.h
@@ -26,10 +26,12 @@
#include "qemu-common.h"
#include "qemu-option.h"
+#include "qapi-types.h"
#ifdef CONFIG_VDE
-int net_init_vde(QemuOpts *opts, const char *name, VLANState *vlan);
+int net_init_vde(QemuOpts *opts, const NetClientOptions *new_opts,
+ const char *name, VLANState *vlan);
#endif /* CONFIG_VDE */
diff --git a/net.c b/net.c
index 9c8cd42..2cf40a0 100644
--- a/net.c
+++ b/net.c
@@ -37,6 +37,9 @@
#include "qmp-commands.h"
#include "hw/qdev.h"
#include "iov.h"
+#include "qapi-visit.h"
+#include "qapi/opts-visitor.h"
+#include "qapi/qapi-dealloc-visitor.h"
/* Net bridge is currently not supported for W32. */
#if !defined(_WIN32)
@@ -745,7 +748,8 @@ int net_handle_fd_param(Monitor *mon, const char *param)
return fd;
}
-static int net_init_nic(QemuOpts *opts, const char *name, VLANState *vlan)
+static int net_init_nic(QemuOpts *opts, const NetClientOptions *new_opts,
+ const char *name, VLANState *vlan)
{
int idx;
NICInfo *nd;
@@ -802,371 +806,130 @@ static int net_init_nic(QemuOpts *opts, const char *name, VLANState *vlan)
return idx;
}
-#define NET_COMMON_PARAMS_DESC \
- { \
- .name = "type", \
- .type = QEMU_OPT_STRING, \
- .help = "net client type (nic, tap etc.)", \
- }, { \
- .name = "vlan", \
- .type = QEMU_OPT_NUMBER, \
- .help = "vlan number", \
- }, { \
- .name = "name", \
- .type = QEMU_OPT_STRING, \
- .help = "identifier for monitor commands", \
- }
-
-typedef int (*net_client_init_func)(QemuOpts *opts,
- const char *name,
- VLANState *vlan);
-
-/* magic number, but compiler will warn if too small */
-#define NET_MAX_DESC 20
-
-static const struct {
- const char *type;
- net_client_init_func init;
- QemuOptDesc desc[NET_MAX_DESC];
-} net_client_types[NET_CLIENT_OPTIONS_KIND_MAX] = {
- [NET_CLIENT_OPTIONS_KIND_NONE] = {
- .type = "none",
- .desc = {
- NET_COMMON_PARAMS_DESC,
- { /* end of list */ }
- },
- },
- [NET_CLIENT_OPTIONS_KIND_NIC] = {
- .type = "nic",
- .init = net_init_nic,
- .desc = {
- NET_COMMON_PARAMS_DESC,
- {
- .name = "netdev",
- .type = QEMU_OPT_STRING,
- .help = "id of -netdev to connect to",
- },
- {
- .name = "macaddr",
- .type = QEMU_OPT_STRING,
- .help = "MAC address",
- }, {
- .name = "model",
- .type = QEMU_OPT_STRING,
- .help = "device model (e1000, rtl8139, virtio etc.)",
- }, {
- .name = "addr",
- .type = QEMU_OPT_STRING,
- .help = "PCI device address",
- }, {
- .name = "vectors",
- .type = QEMU_OPT_NUMBER,
- .help = "number of MSI-x vectors, 0 to disable MSI-X",
- },
- { /* end of list */ }
- },
- },
+
+static int (* const net_client_init_fun[NET_CLIENT_OPTIONS_KIND_MAX])(
+ QemuOpts *old_opts,
+ const NetClientOptions *new_opts,
+ const char *name,
+ VLANState *vlan) = {
+ [NET_CLIENT_OPTIONS_KIND_NIC] = net_init_nic,
#ifdef CONFIG_SLIRP
- [NET_CLIENT_OPTIONS_KIND_USER] = {
- .type = "user",
- .init = net_init_slirp,
- .desc = {
- NET_COMMON_PARAMS_DESC,
- {
- .name = "hostname",
- .type = QEMU_OPT_STRING,
- .help = "client hostname reported by the builtin DHCP server",
- }, {
- .name = "restrict",
- .type = QEMU_OPT_STRING,
- .help = "isolate the guest from the host (y|yes|n|no)",
- }, {
- .name = "ip",
- .type = QEMU_OPT_STRING,
- .help = "legacy parameter, use net= instead",
- }, {
- .name = "net",
- .type = QEMU_OPT_STRING,
- .help = "IP address and optional netmask",
- }, {
- .name = "host",
- .type = QEMU_OPT_STRING,
- .help = "guest-visible address of the host",
- }, {
- .name = "tftp",
- .type = QEMU_OPT_STRING,
- .help = "root directory of the built-in TFTP server",
- }, {
- .name = "bootfile",
- .type = QEMU_OPT_STRING,
- .help = "BOOTP filename, for use with tftp=",
- }, {
- .name = "dhcpstart",
- .type = QEMU_OPT_STRING,
- .help = "the first of the 16 IPs the built-in DHCP server can assign",
- }, {
- .name = "dns",
- .type = QEMU_OPT_STRING,
- .help = "guest-visible address of the virtual nameserver",
- }, {
- .name = "smb",
- .type = QEMU_OPT_STRING,
- .help = "root directory of the built-in SMB server",
- }, {
- .name = "smbserver",
- .type = QEMU_OPT_STRING,
- .help = "IP address of the built-in SMB server",
- }, {
- .name = "hostfwd",
- .type = QEMU_OPT_STRING,
- .help = "guest port number to forward incoming TCP or UDP connections",
- }, {
- .name = "guestfwd",
- .type = QEMU_OPT_STRING,
- .help = "IP address and port to forward guest TCP connections",
- },
- { /* end of list */ }
- },
- },
-#endif
- [NET_CLIENT_OPTIONS_KIND_TAP] = {
- .type = "tap",
- .init = net_init_tap,
- .desc = {
- NET_COMMON_PARAMS_DESC,
- {
- .name = "ifname",
- .type = QEMU_OPT_STRING,
- .help = "interface name",
- },
-#ifndef _WIN32
- {
- .name = "fd",
- .type = QEMU_OPT_STRING,
- .help = "file descriptor of an already opened tap",
- }, {
- .name = "script",
- .type = QEMU_OPT_STRING,
- .help = "script to initialize the interface",
- }, {
- .name = "downscript",
- .type = QEMU_OPT_STRING,
- .help = "script to shut down the interface",
- }, {
-#ifdef CONFIG_NET_BRIDGE
- .name = "helper",
- .type = QEMU_OPT_STRING,
- .help = "command to execute to configure bridge",
- }, {
+ [NET_CLIENT_OPTIONS_KIND_USER] = net_init_slirp,
#endif
- .name = "sndbuf",
- .type = QEMU_OPT_SIZE,
- .help = "send buffer limit"
- }, {
- .name = "vnet_hdr",
- .type = QEMU_OPT_BOOL,
- .help = "enable the IFF_VNET_HDR flag on the tap interface"
- }, {
- .name = "vhost",
- .type = QEMU_OPT_BOOL,
- .help = "enable vhost-net network accelerator",
- }, {
- .name = "vhostfd",
- .type = QEMU_OPT_STRING,
- .help = "file descriptor of an already opened vhost net device",
- }, {
- .name = "vhostforce",
- .type = QEMU_OPT_BOOL,
- .help = "force vhost on for non-MSIX virtio guests",
- },
-#endif /* _WIN32 */
- { /* end of list */ }
- },
- },
- [NET_CLIENT_OPTIONS_KIND_SOCKET] = {
- .type = "socket",
- .init = net_init_socket,
- .desc = {
- NET_COMMON_PARAMS_DESC,
- {
- .name = "fd",
- .type = QEMU_OPT_STRING,
- .help = "file descriptor of an already opened socket",
- }, {
- .name = "listen",
- .type = QEMU_OPT_STRING,
- .help = "port number, and optional hostname, to listen on",
- }, {
- .name = "connect",
- .type = QEMU_OPT_STRING,
- .help = "port number, and optional hostname, to connect to",
- }, {
- .name = "mcast",
- .type = QEMU_OPT_STRING,
- .help = "UDP multicast address and port number",
- }, {
- .name = "localaddr",
- .type = QEMU_OPT_STRING,
- .help = "source address and port for multicast and udp packets",
- }, {
- .name = "udp",
- .type = QEMU_OPT_STRING,
- .help = "UDP unicast address and port number",
- },
- { /* end of list */ }
- },
- },
+ [NET_CLIENT_OPTIONS_KIND_TAP] = net_init_tap,
+ [NET_CLIENT_OPTIONS_KIND_SOCKET] = net_init_socket,
#ifdef CONFIG_VDE
- [NET_CLIENT_OPTIONS_KIND_VDE] = {
- .type = "vde",
- .init = net_init_vde,
- .desc = {
- NET_COMMON_PARAMS_DESC,
- {
- .name = "sock",
- .type = QEMU_OPT_STRING,
- .help = "socket path",
- }, {
- .name = "port",
- .type = QEMU_OPT_NUMBER,
- .help = "port number",
- }, {
- .name = "group",
- .type = QEMU_OPT_STRING,
- .help = "group owner of socket",
- }, {
- .name = "mode",
- .type = QEMU_OPT_NUMBER,
- .help = "permissions for socket",
- },
- { /* end of list */ }
- },
- },
+ [NET_CLIENT_OPTIONS_KIND_VDE] = net_init_vde,
#endif
- [NET_CLIENT_OPTIONS_KIND_DUMP] = {
- .type = "dump",
- .init = net_init_dump,
- .desc = {
- NET_COMMON_PARAMS_DESC,
- {
- .name = "len",
- .type = QEMU_OPT_SIZE,
- .help = "per-packet size limit (64k default)",
- }, {
- .name = "file",
- .type = QEMU_OPT_STRING,
- .help = "dump file path (default is qemu-vlan0.pcap)",
- },
- { /* end of list */ }
- },
- },
+ [NET_CLIENT_OPTIONS_KIND_DUMP] = net_init_dump,
#ifdef CONFIG_NET_BRIDGE
- [NET_CLIENT_OPTIONS_KIND_BRIDGE] = {
- .type = "bridge",
- .init = net_init_bridge,
- .desc = {
- NET_COMMON_PARAMS_DESC,
- {
- .name = "br",
- .type = QEMU_OPT_STRING,
- .help = "bridge name",
- }, {
- .name = "helper",
- .type = QEMU_OPT_STRING,
- .help = "command to execute to configure bridge",
- },
- { /* end of list */ }
- },
- },
-#endif /* CONFIG_NET_BRIDGE */
+ [NET_CLIENT_OPTIONS_KIND_BRIDGE] = net_init_bridge,
+#endif
};
-int net_client_init(QemuOpts *opts, int is_netdev, Error **errp)
+
+static int net_client_init1(const void *object, int is_netdev,
+ QemuOpts *old_opts, Error **errp)
{
+ union {
+ const Netdev *netdev;
+ const NetLegacy *net;
+ } u;
+ const NetClientOptions *opts;
const char *name;
- const char *type;
- int i;
-
- type = qemu_opt_get(opts, "type");
- if (!type) {
- error_set(errp, QERR_MISSING_PARAMETER, "type");
- return -1;
- }
if (is_netdev) {
- if (strcmp(type, "tap") != 0 &&
-#ifdef CONFIG_NET_BRIDGE
- strcmp(type, "bridge") != 0 &&
-#endif
+ u.netdev = object;
+ opts = u.netdev->opts;
+ name = u.netdev->id;
+
+ switch (opts->kind) {
#ifdef CONFIG_SLIRP
- strcmp(type, "user") != 0 &&
+ case NET_CLIENT_OPTIONS_KIND_USER:
#endif
+ case NET_CLIENT_OPTIONS_KIND_TAP:
+ case NET_CLIENT_OPTIONS_KIND_SOCKET:
#ifdef CONFIG_VDE
- strcmp(type, "vde") != 0 &&
+ case NET_CLIENT_OPTIONS_KIND_VDE:
+#endif
+#ifdef CONFIG_NET_BRIDGE
+ case NET_CLIENT_OPTIONS_KIND_BRIDGE:
#endif
- strcmp(type, "socket") != 0) {
+ break;
+
+ default:
error_set(errp, QERR_INVALID_PARAMETER_VALUE, "type",
"a netdev backend type");
return -1;
}
+ } else {
+ u.net = object;
+ opts = u.net->opts;
+ /* missing optional values have been initialized to "all bits zero" */
+ name = u.net->name;
+ }
- if (qemu_opt_get(opts, "vlan")) {
- error_set(errp, QERR_INVALID_PARAMETER, "vlan");
- return -1;
- }
- if (qemu_opt_get(opts, "name")) {
- error_set(errp, QERR_INVALID_PARAMETER, "name");
- return -1;
+ if (net_client_init_fun[opts->kind]) {
+ VLANState *vlan = NULL;
+
+ /* Do not add to a vlan if it's a -netdev or a nic with a netdev=
+ * parameter. */
+ if (!is_netdev &&
+ (opts->kind != NET_CLIENT_OPTIONS_KIND_NIC ||
+ !opts->nic->has_netdev)) {
+ vlan = qemu_find_vlan(u.net->has_vlan ? u.net->vlan : 0, true);
}
- if (!qemu_opts_id(opts)) {
- error_set(errp, QERR_MISSING_PARAMETER, "id");
+
+ if (net_client_init_fun[opts->kind](old_opts, opts, name, vlan) < 0) {
+ /* TODO push error reporting into init() methods */
+ error_set(errp, QERR_DEVICE_INIT_FAILED,
+ NetClientOptionsKind_lookup[opts->kind]);
return -1;
}
}
+ return 0;
+}
+
- name = qemu_opts_id(opts);
- if (!name) {
- name = qemu_opt_get(opts, "name");
+static void net_visit(Visitor *v, int is_netdev, void **object, Error **errp)
+{
+ if (is_netdev) {
+ visit_type_Netdev(v, (Netdev **)object, NULL, errp);
+ } else {
+ visit_type_NetLegacy(v, (NetLegacy **)object, NULL, errp);
}
+}
- for (i = 0; i < NET_CLIENT_OPTIONS_KIND_MAX; i++) {
- if (net_client_types[i].type != NULL &&
- !strcmp(net_client_types[i].type, type)) {
- Error *local_err = NULL;
- VLANState *vlan = NULL;
- int ret;
- qemu_opts_validate(opts, &net_client_types[i].desc[0], &local_err);
- if (error_is_set(&local_err)) {
- error_propagate(errp, local_err);
- return -1;
- }
+int net_client_init(QemuOpts *opts, int is_netdev, Error **errp)
+{
+ void *object = NULL;
+ Error *err = NULL;
+ int ret = -1;
- /* Do not add to a vlan if it's a -netdev or a nic with a
- * netdev= parameter. */
- if (!(is_netdev ||
- (strcmp(type, "nic") == 0 && qemu_opt_get(opts, "netdev")))) {
- vlan = qemu_find_vlan(qemu_opt_get_number(opts, "vlan", 0), 1);
- }
+ {
+ OptsVisitor *ov = opts_visitor_new(opts);
- ret = 0;
- if (net_client_types[i].init) {
- ret = net_client_types[i].init(opts, name, vlan);
- if (ret < 0) {
- /* TODO push error reporting into init() methods */
- error_set(errp, QERR_DEVICE_INIT_FAILED, type);
- return -1;
- }
- }
- return ret;
- }
+ net_visit(opts_get_visitor(ov), is_netdev, &object, &err);
+ opts_visitor_cleanup(ov);
}
- error_set(errp, QERR_INVALID_PARAMETER_VALUE, "type",
- "a network client type");
- return -1;
+ if (!err) {
+ ret = net_client_init1(object, is_netdev, opts, &err);
+ }
+
+ if (object) {
+ QapiDeallocVisitor *dv = qapi_dealloc_visitor_new();
+
+ net_visit(qapi_dealloc_get_visitor(dv), is_netdev, &object, NULL);
+ qapi_dealloc_visitor_cleanup(dv);
+ }
+
+ error_propagate(errp, err);
+ return ret;
}
+
static int net_host_check_device(const char *device)
{
int i;
@@ -1286,7 +1049,7 @@ void qmp_netdev_del(const char *id, Error **errp)
static void print_net_client(Monitor *mon, VLANClientState *vc)
{
monitor_printf(mon, "%s: type=%s,%s\n", vc->name,
- net_client_types[vc->info->type].type, vc->info_str);
+ NetClientOptionsKind_lookup[vc->info->type], vc->info_str);
}
void do_info_network(Monitor *mon)
diff --git a/net/dump.c b/net/dump.c
index 2124b9a..27e9528 100644
--- a/net/dump.c
+++ b/net/dump.c
@@ -144,7 +144,8 @@ static int net_dump_init(VLANState *vlan, const char *device,
return 0;
}
-int net_init_dump(QemuOpts *opts, const char *name, VLANState *vlan)
+int net_init_dump(QemuOpts *opts, const NetClientOptions *new_opts,
+ const char *name, VLANState *vlan)
{
int len;
const char *file;
diff --git a/net/slirp.c b/net/slirp.c
index 9076d99..9b925b7 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -676,7 +676,8 @@ static int net_init_slirp_configs(const char *name, const char *value, void *opa
return 0;
}
-int net_init_slirp(QemuOpts *opts, const char *name, VLANState *vlan)
+int net_init_slirp(QemuOpts *opts, const NetClientOptions *new_opts,
+ const char *name, VLANState *vlan)
{
struct slirp_config_str *config;
const char *vhost;
diff --git a/net/socket.c b/net/socket.c
index 30536ef..563447d 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -586,7 +586,8 @@ static int net_socket_udp_init(VLANState *vlan,
return 0;
}
-int net_init_socket(QemuOpts *opts, const char *name, VLANState *vlan)
+int net_init_socket(QemuOpts *opts, const NetClientOptions *new_opts,
+ const char *name, VLANState *vlan)
{
if (qemu_opt_get(opts, "fd")) {
int fd;
diff --git a/net/tap-win32.c b/net/tap-win32.c
index f7b6129..b738f45 100644
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -699,7 +699,8 @@ static int tap_win32_init(VLANState *vlan, const char *model,
return 0;
}
-int net_init_tap(QemuOpts *opts, const char *name, VLANState *vlan)
+int net_init_tap(QemuOpts *opts, const NetClientOptions *new_opts,
+ const char *name, VLANState *vlan)
{
const char *ifname;
diff --git a/net/tap.c b/net/tap.c
index 4cba566..6d57d6c 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -512,7 +512,8 @@ static int net_bridge_run_helper(const char *helper, const char *bridge)
return -1;
}
-int net_init_bridge(QemuOpts *opts, const char *name, VLANState *vlan)
+int net_init_bridge(QemuOpts *opts, const NetClientOptions *new_opts,
+ const char *name, VLANState *vlan)
{
TAPState *s;
int fd, vnet_hdr;
@@ -582,7 +583,8 @@ static int net_tap_init(QemuOpts *opts, int *vnet_hdr)
return fd;
}
-int net_init_tap(QemuOpts *opts, const char *name, VLANState *vlan)
+int net_init_tap(QemuOpts *opts, const NetClientOptions *new_opts,
+ const char *name, VLANState *vlan)
{
TAPState *s;
int fd, vnet_hdr = 0;
diff --git a/net/vde.c b/net/vde.c
index 0e8bf23..8e60f68 100644
--- a/net/vde.c
+++ b/net/vde.c
@@ -110,7 +110,8 @@ static int net_vde_init(VLANState *vlan, const char *model,
return 0;
}
-int net_init_vde(QemuOpts *opts, const char *name, VLANState *vlan)
+int net_init_vde(QemuOpts *opts, const NetClientOptions *new_opts,
+ const char *name, VLANState *vlan)
{
const char *sock;
const char *group;
--
1.7.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v2 10/17] convert net_init_nic() to NetClientOptions
2012-06-13 8:22 [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing Laszlo Ersek
` (8 preceding siblings ...)
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 09/17] convert net_client_init() to OptsVisitor Laszlo Ersek
@ 2012-06-13 8:22 ` Laszlo Ersek
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 11/17] convert net_init_dump() " Laszlo Ersek
` (8 subsequent siblings)
18 siblings, 0 replies; 38+ messages in thread
From: Laszlo Ersek @ 2012-06-13 8:22 UTC (permalink / raw)
To: qemu-devel, lersek
v1->v2:
- NetLegacyNicOptions::vectors is of type uint32
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
net.c | 39 ++++++++++++++++++++++-----------------
1 files changed, 22 insertions(+), 17 deletions(-)
diff --git a/net.c b/net.c
index 2cf40a0..886961c 100644
--- a/net.c
+++ b/net.c
@@ -748,12 +748,15 @@ int net_handle_fd_param(Monitor *mon, const char *param)
return fd;
}
-static int net_init_nic(QemuOpts *opts, const NetClientOptions *new_opts,
+static int net_init_nic(QemuOpts *old_opts, const NetClientOptions *opts,
const char *name, VLANState *vlan)
{
int idx;
NICInfo *nd;
- const char *netdev;
+ const NetLegacyNicOptions *nic;
+
+ assert(opts->kind == NET_CLIENT_OPTIONS_KIND_NIC);
+ nic = opts->nic;
idx = nic_get_free_idx();
if (idx == -1 || nb_nics >= MAX_NICS) {
@@ -765,10 +768,10 @@ static int net_init_nic(QemuOpts *opts, const NetClientOptions *new_opts,
memset(nd, 0, sizeof(*nd));
- if ((netdev = qemu_opt_get(opts, "netdev"))) {
- nd->netdev = qemu_find_netdev(netdev);
+ if (nic->has_netdev) {
+ nd->netdev = qemu_find_netdev(nic->netdev);
if (!nd->netdev) {
- error_report("netdev '%s' not found", netdev);
+ error_report("netdev '%s' not found", nic->netdev);
return -1;
}
} else {
@@ -778,26 +781,28 @@ static int net_init_nic(QemuOpts *opts, const NetClientOptions *new_opts,
if (name) {
nd->name = g_strdup(name);
}
- if (qemu_opt_get(opts, "model")) {
- nd->model = g_strdup(qemu_opt_get(opts, "model"));
+ if (nic->has_model) {
+ nd->model = g_strdup(nic->model);
}
- if (qemu_opt_get(opts, "addr")) {
- nd->devaddr = g_strdup(qemu_opt_get(opts, "addr"));
+ if (nic->has_addr) {
+ nd->devaddr = g_strdup(nic->addr);
}
- if (qemu_opt_get(opts, "macaddr") &&
- net_parse_macaddr(nd->macaddr.a, qemu_opt_get(opts, "macaddr")) < 0) {
+ if (nic->has_macaddr &&
+ net_parse_macaddr(nd->macaddr.a, nic->macaddr) < 0) {
error_report("invalid syntax for ethernet address");
return -1;
}
qemu_macaddr_default_if_unset(&nd->macaddr);
- nd->nvectors = qemu_opt_get_number(opts, "vectors",
- DEV_NVECTORS_UNSPECIFIED);
- if (nd->nvectors != DEV_NVECTORS_UNSPECIFIED &&
- (nd->nvectors < 0 || nd->nvectors > 0x7ffffff)) {
- error_report("invalid # of vectors: %d", nd->nvectors);
- return -1;
+ if (nic->has_vectors) {
+ if (nic->vectors > 0x7ffffff) {
+ error_report("invalid # of vectors: %"PRIu32, nic->vectors);
+ return -1;
+ }
+ nd->nvectors = nic->vectors;
+ } else {
+ nd->nvectors = DEV_NVECTORS_UNSPECIFIED;
}
nd->used = 1;
--
1.7.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v2 11/17] convert net_init_dump() to NetClientOptions
2012-06-13 8:22 [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing Laszlo Ersek
` (9 preceding siblings ...)
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 10/17] convert net_init_nic() to NetClientOptions Laszlo Ersek
@ 2012-06-13 8:22 ` Laszlo Ersek
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 12/17] convert net_init_slirp() " Laszlo Ersek
` (7 subsequent siblings)
18 siblings, 0 replies; 38+ messages in thread
From: Laszlo Ersek @ 2012-06-13 8:22 UTC (permalink / raw)
To: qemu-devel, lersek
v1->v2:
- NetdevDumpOptions::len is of type 'size', whose C type was changed to
uint64_t. Adapt the printf() format specifier macro.
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
net/dump.c | 21 +++++++++++++++++----
1 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/net/dump.c b/net/dump.c
index 27e9528..f3d2fa9 100644
--- a/net/dump.c
+++ b/net/dump.c
@@ -144,22 +144,35 @@ static int net_dump_init(VLANState *vlan, const char *device,
return 0;
}
-int net_init_dump(QemuOpts *opts, const NetClientOptions *new_opts,
+int net_init_dump(QemuOpts *old_opts, const NetClientOptions *opts,
const char *name, VLANState *vlan)
{
int len;
const char *file;
char def_file[128];
+ const NetdevDumpOptions *dump;
+
+ assert(opts->kind == NET_CLIENT_OPTIONS_KIND_DUMP);
+ dump = opts->dump;
assert(vlan);
- file = qemu_opt_get(opts, "file");
- if (!file) {
+ if (dump->has_file) {
+ file = dump->file;
+ } else {
snprintf(def_file, sizeof(def_file), "qemu-vlan%d.pcap", vlan->id);
file = def_file;
}
- len = qemu_opt_get_size(opts, "len", 65536);
+ if (dump->has_len) {
+ if (dump->len > INT_MAX) {
+ error_report("invalid length: %"PRIu64, dump->len);
+ return -1;
+ }
+ len = dump->len;
+ } else {
+ len = 65536;
+ }
return net_dump_init(vlan, "dump", name, file, len);
}
--
1.7.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v2 12/17] convert net_init_slirp() to NetClientOptions
2012-06-13 8:22 [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing Laszlo Ersek
` (10 preceding siblings ...)
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 11/17] convert net_init_dump() " Laszlo Ersek
@ 2012-06-13 8:22 ` Laszlo Ersek
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 13/17] convert net_init_socket() " Laszlo Ersek
` (6 subsequent siblings)
18 siblings, 0 replies; 38+ messages in thread
From: Laszlo Ersek @ 2012-06-13 8:22 UTC (permalink / raw)
To: qemu-devel, lersek
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
net/slirp.c | 93 ++++++++++++++++-------------------------------------------
1 files changed, 25 insertions(+), 68 deletions(-)
diff --git a/net/slirp.c b/net/slirp.c
index 9b925b7..166304c 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -654,89 +654,46 @@ void do_info_usernet(Monitor *mon)
}
}
-static int net_init_slirp_configs(const char *name, const char *value, void *opaque)
+static void
+net_init_slirp_configs(const StringList *fwd, int flags)
{
- struct slirp_config_str *config;
-
- if (strcmp(name, "hostfwd") != 0 && strcmp(name, "guestfwd") != 0) {
- return 0;
- }
-
- config = g_malloc0(sizeof(*config));
+ while (fwd) {
+ struct slirp_config_str *config;
- pstrcpy(config->str, sizeof(config->str), value);
+ config = g_malloc0(sizeof(*config));
+ pstrcpy(config->str, sizeof(config->str), fwd->value->str);
+ config->flags = flags;
+ config->next = slirp_configs;
+ slirp_configs = config;
- if (!strcmp(name, "hostfwd")) {
- config->flags = SLIRP_CFG_HOSTFWD;
+ fwd = fwd->next;
}
-
- config->next = slirp_configs;
- slirp_configs = config;
-
- return 0;
}
-int net_init_slirp(QemuOpts *opts, const NetClientOptions *new_opts,
+int net_init_slirp(QemuOpts *old_opts, const NetClientOptions *opts,
const char *name, VLANState *vlan)
{
struct slirp_config_str *config;
- const char *vhost;
- const char *vhostname;
- const char *vdhcp_start;
- const char *vnamesrv;
- const char *tftp_export;
- const char *bootfile;
- const char *smb_export;
- const char *vsmbsrv;
- const char *restrict_opt;
- char *vnet = NULL;
- int restricted = 0;
+ char *vnet;
int ret;
+ const NetdevUserOptions *user;
- vhost = qemu_opt_get(opts, "host");
- vhostname = qemu_opt_get(opts, "hostname");
- vdhcp_start = qemu_opt_get(opts, "dhcpstart");
- vnamesrv = qemu_opt_get(opts, "dns");
- tftp_export = qemu_opt_get(opts, "tftp");
- bootfile = qemu_opt_get(opts, "bootfile");
- smb_export = qemu_opt_get(opts, "smb");
- vsmbsrv = qemu_opt_get(opts, "smbserver");
-
- restrict_opt = qemu_opt_get(opts, "restrict");
- if (restrict_opt) {
- if (!strcmp(restrict_opt, "on") ||
- !strcmp(restrict_opt, "yes") || !strcmp(restrict_opt, "y")) {
- restricted = 1;
- } else if (strcmp(restrict_opt, "off") &&
- strcmp(restrict_opt, "no") && strcmp(restrict_opt, "n")) {
- error_report("invalid option: 'restrict=%s'", restrict_opt);
- return -1;
- }
- }
-
- if (qemu_opt_get(opts, "ip")) {
- const char *ip = qemu_opt_get(opts, "ip");
- int l = strlen(ip) + strlen("/24") + 1;
+ assert(opts->kind == NET_CLIENT_OPTIONS_KIND_USER);
+ user = opts->user;
- vnet = g_malloc(l);
+ vnet = user->has_net ? g_strdup(user->net) :
+ user->has_ip ? g_strdup_printf("%s/24", user->ip) :
+ NULL;
- /* emulate legacy ip= parameter */
- pstrcpy(vnet, l, ip);
- pstrcat(vnet, l, "/24");
- }
-
- if (qemu_opt_get(opts, "net")) {
- if (vnet) {
- g_free(vnet);
- }
- vnet = g_strdup(qemu_opt_get(opts, "net"));
- }
+ /* all optional fields are initialized to "all bits zero" */
- qemu_opt_foreach(opts, net_init_slirp_configs, NULL, 0);
+ net_init_slirp_configs(user->hostfwd, SLIRP_CFG_HOSTFWD);
+ net_init_slirp_configs(user->guestfwd, 0);
- ret = net_slirp_init(vlan, "user", name, restricted, vnet, vhost,
- vhostname, tftp_export, bootfile, vdhcp_start,
- vnamesrv, smb_export, vsmbsrv);
+ ret = net_slirp_init(vlan, "user", name, user->restrict, vnet, user->host,
+ user->hostname, user->tftp, user->bootfile,
+ user->dhcpstart, user->dns, user->smb,
+ user->smbserver);
while (slirp_configs) {
config = slirp_configs;
--
1.7.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v2 13/17] convert net_init_socket() to NetClientOptions
2012-06-13 8:22 [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing Laszlo Ersek
` (11 preceding siblings ...)
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 12/17] convert net_init_slirp() " Laszlo Ersek
@ 2012-06-13 8:22 ` Laszlo Ersek
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 14/17] convert net_init_vde() " Laszlo Ersek
` (5 subsequent siblings)
18 siblings, 0 replies; 38+ messages in thread
From: Laszlo Ersek @ 2012-06-13 8:22 UTC (permalink / raw)
To: qemu-devel, lersek
I "reverse engineered" the following permissions between the -socket
sub-options:
fd listen connect mcast udp | localaddr
fd x . . . . | .
listen . x . . . | .
connect . . x . . | .
mcast . . . x . | x
udp . . . . x | x
-------------------------------------------+
localaddr . . . x x x
I transformed the code accordingly. The real fix would be to embed "fd",
"listen", "connect", "mcast" and "udp" in a separate union. However
OptsVisitor's enum parser only supports the type=XXX QemuOpt instance as
union discriminator.
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
net/socket.c | 119 +++++++++++++++++++++-------------------------------------
1 files changed, 43 insertions(+), 76 deletions(-)
diff --git a/net/socket.c b/net/socket.c
index 563447d..e3cba20 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -586,101 +586,68 @@ static int net_socket_udp_init(VLANState *vlan,
return 0;
}
-int net_init_socket(QemuOpts *opts, const NetClientOptions *new_opts,
+int net_init_socket(QemuOpts *old_opts, const NetClientOptions *opts,
const char *name, VLANState *vlan)
{
- if (qemu_opt_get(opts, "fd")) {
- int fd;
+ const NetdevSocketOptions *sock;
- if (qemu_opt_get(opts, "listen") ||
- qemu_opt_get(opts, "connect") ||
- qemu_opt_get(opts, "mcast") ||
- qemu_opt_get(opts, "localaddr")) {
- error_report("listen=, connect=, mcast= and localaddr= is invalid with fd=");
- return -1;
- }
+ assert(opts->kind == NET_CLIENT_OPTIONS_KIND_SOCKET);
+ sock = opts->socket;
- fd = net_handle_fd_param(cur_mon, qemu_opt_get(opts, "fd"));
- if (fd == -1) {
- return -1;
- }
+ if (sock->has_fd + sock->has_listen + sock->has_connect + sock->has_mcast +
+ sock->has_udp != 1) {
+ error_report("exactly one of fd=, listen=, connect=, mcast= or udp="
+ " is required");
+ return -1;
+ }
- if (!net_socket_fd_init(vlan, "socket", name, fd, 1)) {
- return -1;
- }
- } else if (qemu_opt_get(opts, "listen")) {
- const char *listen;
-
- if (qemu_opt_get(opts, "fd") ||
- qemu_opt_get(opts, "connect") ||
- qemu_opt_get(opts, "mcast") ||
- qemu_opt_get(opts, "localaddr")) {
- error_report("fd=, connect=, mcast= and localaddr= is invalid with listen=");
- return -1;
- }
+ if (sock->has_localaddr && !sock->has_mcast && !sock->has_udp) {
+ error_report("localaddr= is only valid with mcast= or udp=");
+ return -1;
+ }
- listen = qemu_opt_get(opts, "listen");
+ if (sock->has_fd) {
+ int fd;
- if (net_socket_listen_init(vlan, "socket", name, listen) == -1) {
- return -1;
- }
- } else if (qemu_opt_get(opts, "connect")) {
- const char *connect;
-
- if (qemu_opt_get(opts, "fd") ||
- qemu_opt_get(opts, "listen") ||
- qemu_opt_get(opts, "mcast") ||
- qemu_opt_get(opts, "localaddr")) {
- error_report("fd=, listen=, mcast= and localaddr= is invalid with connect=");
+ fd = net_handle_fd_param(cur_mon, sock->fd);
+ if (fd == -1 || !net_socket_fd_init(vlan, "socket", name, fd, 1)) {
return -1;
}
+ return 0;
+ }
- connect = qemu_opt_get(opts, "connect");
-
- if (net_socket_connect_init(vlan, "socket", name, connect) == -1) {
+ if (sock->has_listen) {
+ if (net_socket_listen_init(vlan, "socket", name, sock->listen) == -1) {
return -1;
}
- } else if (qemu_opt_get(opts, "mcast")) {
- const char *mcast, *localaddr;
+ return 0;
+ }
- if (qemu_opt_get(opts, "fd") ||
- qemu_opt_get(opts, "connect") ||
- qemu_opt_get(opts, "listen")) {
- error_report("fd=, connect= and listen= is invalid with mcast=");
+ if (sock->has_connect) {
+ if (net_socket_connect_init(vlan, "socket", name, sock->connect) ==
+ -1) {
return -1;
}
+ return 0;
+ }
- mcast = qemu_opt_get(opts, "mcast");
- localaddr = qemu_opt_get(opts, "localaddr");
-
- if (net_socket_mcast_init(vlan, "socket", name, mcast, localaddr) == -1) {
- return -1;
- }
- } else if (qemu_opt_get(opts, "udp")) {
- const char *udp, *localaddr;
-
- if (qemu_opt_get(opts, "fd") ||
- qemu_opt_get(opts, "connect") ||
- qemu_opt_get(opts, "listen") ||
- qemu_opt_get(opts, "mcast")) {
- error_report("fd=, connect=, listen="
- " and mcast= is invalid with udp=");
+ if (sock->has_mcast) {
+ /* if sock->localaddr is missing, it has been initialized to "all bits
+ * zero" */
+ if (net_socket_mcast_init(vlan, "socket", name, sock->mcast,
+ sock->localaddr) == -1) {
return -1;
}
+ return 0;
+ }
- udp = qemu_opt_get(opts, "udp");
- localaddr = qemu_opt_get(opts, "localaddr");
- if (localaddr == NULL) {
- error_report("localaddr= is mandatory with udp=");
- return -1;
- }
-
- if (net_socket_udp_init(vlan, "udp", name, udp, localaddr) == -1) {
- return -1;
- }
- } else {
- error_report("-socket requires fd=, listen=,"
- " connect=, mcast= or udp=");
+ assert(sock->has_udp);
+ if (!sock->has_localaddr) {
+ error_report("localaddr= is mandatory with udp=");
+ return -1;
+ }
+ if (net_socket_udp_init(vlan, "udp", name, sock->udp, sock->localaddr) ==
+ -1) {
return -1;
}
return 0;
--
1.7.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v2 14/17] convert net_init_vde() to NetClientOptions
2012-06-13 8:22 [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing Laszlo Ersek
` (12 preceding siblings ...)
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 13/17] convert net_init_socket() " Laszlo Ersek
@ 2012-06-13 8:22 ` Laszlo Ersek
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 15/17] convert net_init_tap() " Laszlo Ersek
` (4 subsequent siblings)
18 siblings, 0 replies; 38+ messages in thread
From: Laszlo Ersek @ 2012-06-13 8:22 UTC (permalink / raw)
To: qemu-devel, lersek
v1->v2:
- NetdevVdeOptions::port and ::mode are of type uint16. Remove superfluous
range checks.
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
net/vde.c | 17 +++++++----------
1 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/net/vde.c b/net/vde.c
index 8e60f68..703888c 100644
--- a/net/vde.c
+++ b/net/vde.c
@@ -110,20 +110,17 @@ static int net_vde_init(VLANState *vlan, const char *model,
return 0;
}
-int net_init_vde(QemuOpts *opts, const NetClientOptions *new_opts,
+int net_init_vde(QemuOpts *old_opts, const NetClientOptions *opts,
const char *name, VLANState *vlan)
{
- const char *sock;
- const char *group;
- int port, mode;
+ const NetdevVdeOptions *vde;
- sock = qemu_opt_get(opts, "sock");
- group = qemu_opt_get(opts, "group");
+ assert(opts->kind == NET_CLIENT_OPTIONS_KIND_VDE);
+ vde = opts->vde;
- port = qemu_opt_get_number(opts, "port", 0);
- mode = qemu_opt_get_number(opts, "mode", 0700);
-
- if (net_vde_init(vlan, "vde", name, sock, port, group, mode) == -1) {
+ /* missing optional values have been initialized to "all bits zero" */
+ if (net_vde_init(vlan, "vde", name, vde->sock, vde->port, vde->group,
+ vde->has_mode ? vde->mode : 0700) == -1) {
return -1;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v2 15/17] convert net_init_tap() to NetClientOptions
2012-06-13 8:22 [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing Laszlo Ersek
` (13 preceding siblings ...)
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 14/17] convert net_init_vde() " Laszlo Ersek
@ 2012-06-13 8:22 ` Laszlo Ersek
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 16/17] convert net_init_bridge() " Laszlo Ersek
` (3 subsequent siblings)
18 siblings, 0 replies; 38+ messages in thread
From: Laszlo Ersek @ 2012-06-13 8:22 UTC (permalink / raw)
To: qemu-devel, lersek
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
net/tap.h | 2 +-
net/tap-aix.c | 2 +-
net/tap-bsd.c | 2 +-
net/tap-haiku.c | 2 +-
net/tap-linux.c | 9 +++-
net/tap-solaris.c | 2 +-
net/tap-win32.c | 11 +++--
net/tap.c | 111 ++++++++++++++++++++++++++---------------------------
8 files changed, 71 insertions(+), 70 deletions(-)
diff --git a/net/tap.h b/net/tap.h
index 44e31ce..f092129 100644
--- a/net/tap.h
+++ b/net/tap.h
@@ -47,7 +47,7 @@ void tap_using_vnet_hdr(VLANClientState *vc, int using_vnet_hdr);
void tap_set_offload(VLANClientState *vc, int csum, int tso4, int tso6, int ecn, int ufo);
void tap_set_vnet_hdr_len(VLANClientState *vc, int len);
-int tap_set_sndbuf(int fd, QemuOpts *opts);
+int tap_set_sndbuf(int fd, const NetdevTapOptions *tap);
int tap_probe_vnet_hdr(int fd);
int tap_probe_vnet_hdr_len(int fd, int len);
int tap_probe_has_ufo(int fd);
diff --git a/net/tap-aix.c b/net/tap-aix.c
index e19aaba..f27c177 100644
--- a/net/tap-aix.c
+++ b/net/tap-aix.c
@@ -31,7 +31,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required
return -1;
}
-int tap_set_sndbuf(int fd, QemuOpts *opts)
+int tap_set_sndbuf(int fd, const NetdevTapOptions *tap)
{
return 0;
}
diff --git a/net/tap-bsd.c b/net/tap-bsd.c
index 937a94b..a3b717d 100644
--- a/net/tap-bsd.c
+++ b/net/tap-bsd.c
@@ -117,7 +117,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required
return fd;
}
-int tap_set_sndbuf(int fd, QemuOpts *opts)
+int tap_set_sndbuf(int fd, const NetdevTapOptions *tap)
{
return 0;
}
diff --git a/net/tap-haiku.c b/net/tap-haiku.c
index 91dda8e..34739d1 100644
--- a/net/tap-haiku.c
+++ b/net/tap-haiku.c
@@ -31,7 +31,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required
return -1;
}
-int tap_set_sndbuf(int fd, QemuOpts *opts)
+int tap_set_sndbuf(int fd, const NetdevTapOptions *tap)
{
return 0;
}
diff --git a/net/tap-linux.c b/net/tap-linux.c
index 41d581b..c6521be 100644
--- a/net/tap-linux.c
+++ b/net/tap-linux.c
@@ -98,16 +98,19 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required
*/
#define TAP_DEFAULT_SNDBUF 0
-int tap_set_sndbuf(int fd, QemuOpts *opts)
+int tap_set_sndbuf(int fd, const NetdevTapOptions *tap)
{
int sndbuf;
- sndbuf = qemu_opt_get_size(opts, "sndbuf", TAP_DEFAULT_SNDBUF);
+ sndbuf = !tap->has_sndbuf ? TAP_DEFAULT_SNDBUF :
+ tap->sndbuf > INT_MAX ? INT_MAX :
+ tap->sndbuf;
+
if (!sndbuf) {
sndbuf = INT_MAX;
}
- if (ioctl(fd, TUNSETSNDBUF, &sndbuf) == -1 && qemu_opt_get(opts, "sndbuf")) {
+ if (ioctl(fd, TUNSETSNDBUF, &sndbuf) == -1 && tap->has_sndbuf) {
error_report("TUNSETSNDBUF ioctl failed: %s", strerror(errno));
return -1;
}
diff --git a/net/tap-solaris.c b/net/tap-solaris.c
index cf76463..5d6ac42 100644
--- a/net/tap-solaris.c
+++ b/net/tap-solaris.c
@@ -197,7 +197,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required
return fd;
}
-int tap_set_sndbuf(int fd, QemuOpts *opts)
+int tap_set_sndbuf(int fd, const NetdevTapOptions *tap)
{
return 0;
}
diff --git a/net/tap-win32.c b/net/tap-win32.c
index b738f45..b6099cd 100644
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -699,19 +699,20 @@ static int tap_win32_init(VLANState *vlan, const char *model,
return 0;
}
-int net_init_tap(QemuOpts *opts, const NetClientOptions *new_opts,
+int net_init_tap(QemuOpts *old_opts, const NetClientOptions *opts,
const char *name, VLANState *vlan)
{
- const char *ifname;
+ const NetdevTapOptions *tap;
- ifname = qemu_opt_get(opts, "ifname");
+ assert(opts->kind == NET_CLIENT_OPTIONS_KIND_TAP);
+ tap = opts->tap;
- if (!ifname) {
+ if (!tap->has_ifname) {
error_report("tap: no interface name");
return -1;
}
- if (tap_win32_init(vlan, "tap", name, ifname) == -1) {
+ if (tap_win32_init(vlan, "tap", name, tap->ifname) == -1) {
return -1;
}
diff --git a/net/tap.c b/net/tap.c
index 6d57d6c..7501eba 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -547,29 +547,32 @@ int net_init_bridge(QemuOpts *opts, const NetClientOptions *new_opts,
return 0;
}
-static int net_tap_init(QemuOpts *opts, int *vnet_hdr)
+static int net_tap_init(const NetdevTapOptions *tap, int *vnet_hdr,
+ const char *setup_script, char *ifname,
+ size_t ifname_sz)
{
int fd, vnet_hdr_required;
- char ifname[128] = {0,};
- const char *setup_script;
- if (qemu_opt_get(opts, "ifname")) {
- pstrcpy(ifname, sizeof(ifname), qemu_opt_get(opts, "ifname"));
+ if (tap->has_ifname) {
+ pstrcpy(ifname, ifname_sz, tap->ifname);
+ } else {
+ assert(ifname_sz > 0);
+ ifname[0] = '\0';
}
- *vnet_hdr = qemu_opt_get_bool(opts, "vnet_hdr", 1);
- if (qemu_opt_get(opts, "vnet_hdr")) {
+ if (tap->has_vnet_hdr) {
+ *vnet_hdr = tap->vnet_hdr;
vnet_hdr_required = *vnet_hdr;
} else {
+ *vnet_hdr = 1;
vnet_hdr_required = 0;
}
- TFR(fd = tap_open(ifname, sizeof(ifname), vnet_hdr, vnet_hdr_required));
+ TFR(fd = tap_open(ifname, ifname_sz, vnet_hdr, vnet_hdr_required));
if (fd < 0) {
return -1;
}
- setup_script = qemu_opt_get(opts, "script");
if (setup_script &&
setup_script[0] != '\0' &&
strcmp(setup_script, "no") != 0 &&
@@ -578,30 +581,34 @@ static int net_tap_init(QemuOpts *opts, int *vnet_hdr)
return -1;
}
- qemu_opt_set(opts, "ifname", ifname);
-
return fd;
}
-int net_init_tap(QemuOpts *opts, const NetClientOptions *new_opts,
+int net_init_tap(QemuOpts *old_opts, const NetClientOptions *opts,
const char *name, VLANState *vlan)
{
- TAPState *s;
+ const NetdevTapOptions *tap;
+
int fd, vnet_hdr = 0;
const char *model;
+ TAPState *s;
+
+ /* for the no-fd, no-helper case */
+ const char *script = NULL; /* suppress wrong "uninit'd use" gcc warning */
+ char ifname[128];
+
+ assert(opts->kind == NET_CLIENT_OPTIONS_KIND_TAP);
+ tap = opts->tap;
- if (qemu_opt_get(opts, "fd")) {
- if (qemu_opt_get(opts, "ifname") ||
- qemu_opt_get(opts, "script") ||
- qemu_opt_get(opts, "downscript") ||
- qemu_opt_get(opts, "vnet_hdr") ||
- qemu_opt_get(opts, "helper")) {
+ if (tap->has_fd) {
+ if (tap->has_ifname || tap->has_script || tap->has_downscript ||
+ tap->has_vnet_hdr || tap->has_helper) {
error_report("ifname=, script=, downscript=, vnet_hdr=, "
"and helper= are invalid with fd=");
return -1;
}
- fd = net_handle_fd_param(cur_mon, qemu_opt_get(opts, "fd"));
+ fd = net_handle_fd_param(cur_mon, tap->fd);
if (fd == -1) {
return -1;
}
@@ -612,18 +619,15 @@ int net_init_tap(QemuOpts *opts, const NetClientOptions *new_opts,
model = "tap";
- } else if (qemu_opt_get(opts, "helper")) {
- if (qemu_opt_get(opts, "ifname") ||
- qemu_opt_get(opts, "script") ||
- qemu_opt_get(opts, "downscript") ||
- qemu_opt_get(opts, "vnet_hdr")) {
+ } else if (tap->has_helper) {
+ if (tap->has_ifname || tap->has_script || tap->has_downscript ||
+ tap->has_vnet_hdr) {
error_report("ifname=, script=, downscript=, and vnet_hdr= "
"are invalid with helper=");
return -1;
}
- fd = net_bridge_run_helper(qemu_opt_get(opts, "helper"),
- DEFAULT_BRIDGE_INTERFACE);
+ fd = net_bridge_run_helper(tap->helper, DEFAULT_BRIDGE_INTERFACE);
if (fd == -1) {
return -1;
}
@@ -635,15 +639,8 @@ int net_init_tap(QemuOpts *opts, const NetClientOptions *new_opts,
model = "bridge";
} else {
- if (!qemu_opt_get(opts, "script")) {
- qemu_opt_set(opts, "script", DEFAULT_NETWORK_SCRIPT);
- }
-
- if (!qemu_opt_get(opts, "downscript")) {
- qemu_opt_set(opts, "downscript", DEFAULT_NETWORK_DOWN_SCRIPT);
- }
-
- fd = net_tap_init(opts, &vnet_hdr);
+ script = tap->has_script ? tap->script : DEFAULT_NETWORK_SCRIPT;
+ fd = net_tap_init(tap, &vnet_hdr, script, ifname, sizeof ifname);
if (fd == -1) {
return -1;
}
@@ -657,25 +654,24 @@ int net_init_tap(QemuOpts *opts, const NetClientOptions *new_opts,
return -1;
}
- if (tap_set_sndbuf(s->fd, opts) < 0) {
+ if (tap_set_sndbuf(s->fd, tap) < 0) {
return -1;
}
- if (qemu_opt_get(opts, "fd")) {
+ if (tap->has_fd) {
snprintf(s->nc.info_str, sizeof(s->nc.info_str), "fd=%d", fd);
- } else if (qemu_opt_get(opts, "helper")) {
- snprintf(s->nc.info_str, sizeof(s->nc.info_str),
- "helper=%s", qemu_opt_get(opts, "helper"));
+ } else if (tap->has_helper) {
+ snprintf(s->nc.info_str, sizeof(s->nc.info_str), "helper=%s",
+ tap->helper);
} else {
- const char *ifname, *script, *downscript;
+ const char *downscript;
- ifname = qemu_opt_get(opts, "ifname");
- script = qemu_opt_get(opts, "script");
- downscript = qemu_opt_get(opts, "downscript");
+ downscript = tap->has_downscript ? tap->downscript :
+ DEFAULT_NETWORK_DOWN_SCRIPT;
snprintf(s->nc.info_str, sizeof(s->nc.info_str),
- "ifname=%s,script=%s,downscript=%s",
- ifname, script, downscript);
+ "ifname=%s,script=%s,downscript=%s", ifname, script,
+ downscript);
if (strcmp(downscript, "no") != 0) {
snprintf(s->down_script, sizeof(s->down_script), "%s", downscript);
@@ -683,25 +679,26 @@ int net_init_tap(QemuOpts *opts, const NetClientOptions *new_opts,
}
}
- if (qemu_opt_get_bool(opts, "vhost", !!qemu_opt_get(opts, "vhostfd") ||
- qemu_opt_get_bool(opts, "vhostforce", false))) {
- int vhostfd, r;
- bool force = qemu_opt_get_bool(opts, "vhostforce", false);
- if (qemu_opt_get(opts, "vhostfd")) {
- r = net_handle_fd_param(cur_mon, qemu_opt_get(opts, "vhostfd"));
- if (r == -1) {
+ if (tap->has_vhost ? tap->vhost :
+ tap->has_vhostfd || (tap->has_vhostforce && tap->vhostforce)) {
+ int vhostfd;
+
+ if (tap->has_vhostfd) {
+ vhostfd = net_handle_fd_param(cur_mon, tap->vhostfd);
+ if (vhostfd == -1) {
return -1;
}
- vhostfd = r;
} else {
vhostfd = -1;
}
- s->vhost_net = vhost_net_init(&s->nc, vhostfd, force);
+
+ s->vhost_net = vhost_net_init(&s->nc, vhostfd,
+ tap->has_vhostforce && tap->vhostforce);
if (!s->vhost_net) {
error_report("vhost-net requested but could not be initialized");
return -1;
}
- } else if (qemu_opt_get(opts, "vhostfd")) {
+ } else if (tap->has_vhostfd) {
error_report("vhostfd= is not valid without vhost");
return -1;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v2 16/17] convert net_init_bridge() to NetClientOptions
2012-06-13 8:22 [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing Laszlo Ersek
` (14 preceding siblings ...)
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 15/17] convert net_init_tap() " Laszlo Ersek
@ 2012-06-13 8:22 ` Laszlo Ersek
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 17/17] remove unused QemuOpts parameter from net init functions Laszlo Ersek
` (2 subsequent siblings)
18 siblings, 0 replies; 38+ messages in thread
From: Laszlo Ersek @ 2012-06-13 8:22 UTC (permalink / raw)
To: qemu-devel, lersek
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
net/tap.c | 23 ++++++++++++-----------
1 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index 7501eba..fdaab2b 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -512,21 +512,22 @@ static int net_bridge_run_helper(const char *helper, const char *bridge)
return -1;
}
-int net_init_bridge(QemuOpts *opts, const NetClientOptions *new_opts,
+int net_init_bridge(QemuOpts *old_opts, const NetClientOptions *opts,
const char *name, VLANState *vlan)
{
+ const NetdevBridgeOptions *bridge;
+ const char *helper, *br;
+
TAPState *s;
int fd, vnet_hdr;
- if (!qemu_opt_get(opts, "br")) {
- qemu_opt_set(opts, "br", DEFAULT_BRIDGE_INTERFACE);
- }
- if (!qemu_opt_get(opts, "helper")) {
- qemu_opt_set(opts, "helper", DEFAULT_BRIDGE_HELPER);
- }
+ assert(opts->kind == NET_CLIENT_OPTIONS_KIND_BRIDGE);
+ bridge = opts->bridge;
+
+ helper = bridge->has_helper ? bridge->helper : DEFAULT_BRIDGE_HELPER;
+ br = bridge->has_br ? bridge->br : DEFAULT_BRIDGE_INTERFACE;
- fd = net_bridge_run_helper(qemu_opt_get(opts, "helper"),
- qemu_opt_get(opts, "br"));
+ fd = net_bridge_run_helper(helper, br);
if (fd == -1) {
return -1;
}
@@ -541,8 +542,8 @@ int net_init_bridge(QemuOpts *opts, const NetClientOptions *new_opts,
return -1;
}
- snprintf(s->nc.info_str, sizeof(s->nc.info_str), "helper=%s,br=%s",
- qemu_opt_get(opts, "helper"), qemu_opt_get(opts, "br"));
+ snprintf(s->nc.info_str, sizeof(s->nc.info_str), "helper=%s,br=%s", helper,
+ br);
return 0;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v2 17/17] remove unused QemuOpts parameter from net init functions
2012-06-13 8:22 [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing Laszlo Ersek
` (15 preceding siblings ...)
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 16/17] convert net_init_bridge() " Laszlo Ersek
@ 2012-06-13 8:22 ` Laszlo Ersek
2012-06-13 10:54 ` [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing Paolo Bonzini
2012-07-13 16:46 ` Luiz Capitulino
18 siblings, 0 replies; 38+ messages in thread
From: Laszlo Ersek @ 2012-06-13 8:22 UTC (permalink / raw)
To: qemu-devel, lersek
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
net/dump.h | 5 ++---
net/slirp.h | 5 ++---
net/socket.h | 5 ++---
net/tap.h | 9 ++++-----
net/vde.h | 5 ++---
net.c | 14 ++++++--------
net/dump.c | 4 ++--
net/slirp.c | 4 ++--
net/socket.c | 4 ++--
net/tap-win32.c | 4 ++--
net/tap.c | 8 ++++----
net/vde.c | 4 ++--
12 files changed, 32 insertions(+), 39 deletions(-)
diff --git a/net/dump.h b/net/dump.h
index 85ac00b..0fa2dd7 100644
--- a/net/dump.h
+++ b/net/dump.h
@@ -25,10 +25,9 @@
#define QEMU_NET_DUMP_H
#include "net.h"
-#include "qemu-common.h"
#include "qapi-types.h"
-int net_init_dump(QemuOpts *opts, const NetClientOptions *new_opts,
- const char *name, VLANState *vlan);
+int net_init_dump(const NetClientOptions *opts, const char *name,
+ VLANState *vlan);
#endif /* QEMU_NET_DUMP_H */
diff --git a/net/slirp.h b/net/slirp.h
index ef13a65..fa7eacc 100644
--- a/net/slirp.h
+++ b/net/slirp.h
@@ -26,13 +26,12 @@
#include "qemu-common.h"
#include "qdict.h"
-#include "qemu-option.h"
#include "qapi-types.h"
#ifdef CONFIG_SLIRP
-int net_init_slirp(QemuOpts *opts, const NetClientOptions *new_opts,
- const char *name, VLANState *vlan);
+int net_init_slirp(const NetClientOptions *opts, const char *name,
+ VLANState *vlan);
void net_slirp_hostfwd_add(Monitor *mon, const QDict *qdict);
void net_slirp_hostfwd_remove(Monitor *mon, const QDict *qdict);
diff --git a/net/socket.h b/net/socket.h
index e44d26e..c4809ad 100644
--- a/net/socket.h
+++ b/net/socket.h
@@ -25,10 +25,9 @@
#define QEMU_NET_SOCKET_H
#include "net.h"
-#include "qemu-common.h"
#include "qapi-types.h"
-int net_init_socket(QemuOpts *opts, const NetClientOptions *new_opts,
- const char *name, VLANState *vlan);
+int net_init_socket(const NetClientOptions *opts, const char *name,
+ VLANState *vlan);
#endif /* QEMU_NET_SOCKET_H */
diff --git a/net/tap.h b/net/tap.h
index f092129..19dea58 100644
--- a/net/tap.h
+++ b/net/tap.h
@@ -27,14 +27,13 @@
#define QEMU_NET_TAP_H
#include "qemu-common.h"
-#include "qemu-option.h"
#include "qapi-types.h"
#define DEFAULT_NETWORK_SCRIPT "/etc/qemu-ifup"
#define DEFAULT_NETWORK_DOWN_SCRIPT "/etc/qemu-ifdown"
-int net_init_tap(QemuOpts *opts, const NetClientOptions *new_opts,
- const char *name, VLANState *vlan);
+int net_init_tap(const NetClientOptions *opts, const char *name,
+ VLANState *vlan);
int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required);
@@ -59,7 +58,7 @@ int tap_get_fd(VLANClientState *vc);
struct vhost_net;
struct vhost_net *tap_get_vhost_net(VLANClientState *vc);
-int net_init_bridge(QemuOpts *opts, const NetClientOptions *new_opts,
- const char *name, VLANState *vlan);
+int net_init_bridge(const NetClientOptions *opts, const char *name,
+ VLANState *vlan);
#endif /* QEMU_NET_TAP_H */
diff --git a/net/vde.h b/net/vde.h
index 5fc17f9..ad502ef 100644
--- a/net/vde.h
+++ b/net/vde.h
@@ -25,13 +25,12 @@
#define QEMU_NET_VDE_H
#include "qemu-common.h"
-#include "qemu-option.h"
#include "qapi-types.h"
#ifdef CONFIG_VDE
-int net_init_vde(QemuOpts *opts, const NetClientOptions *new_opts,
- const char *name, VLANState *vlan);
+int net_init_vde(const NetClientOptions *opts, const char *name,
+ VLANState *vlan);
#endif /* CONFIG_VDE */
diff --git a/net.c b/net.c
index 886961c..8804274 100644
--- a/net.c
+++ b/net.c
@@ -748,8 +748,8 @@ int net_handle_fd_param(Monitor *mon, const char *param)
return fd;
}
-static int net_init_nic(QemuOpts *old_opts, const NetClientOptions *opts,
- const char *name, VLANState *vlan)
+static int net_init_nic(const NetClientOptions *opts, const char *name,
+ VLANState *vlan)
{
int idx;
NICInfo *nd;
@@ -813,8 +813,7 @@ static int net_init_nic(QemuOpts *old_opts, const NetClientOptions *opts,
static int (* const net_client_init_fun[NET_CLIENT_OPTIONS_KIND_MAX])(
- QemuOpts *old_opts,
- const NetClientOptions *new_opts,
+ const NetClientOptions *opts,
const char *name,
VLANState *vlan) = {
[NET_CLIENT_OPTIONS_KIND_NIC] = net_init_nic,
@@ -833,8 +832,7 @@ static int (* const net_client_init_fun[NET_CLIENT_OPTIONS_KIND_MAX])(
};
-static int net_client_init1(const void *object, int is_netdev,
- QemuOpts *old_opts, Error **errp)
+static int net_client_init1(const void *object, int is_netdev, Error **errp)
{
union {
const Netdev *netdev;
@@ -885,7 +883,7 @@ static int net_client_init1(const void *object, int is_netdev,
vlan = qemu_find_vlan(u.net->has_vlan ? u.net->vlan : 0, true);
}
- if (net_client_init_fun[opts->kind](old_opts, opts, name, vlan) < 0) {
+ if (net_client_init_fun[opts->kind](opts, name, vlan) < 0) {
/* TODO push error reporting into init() methods */
error_set(errp, QERR_DEVICE_INIT_FAILED,
NetClientOptionsKind_lookup[opts->kind]);
@@ -920,7 +918,7 @@ int net_client_init(QemuOpts *opts, int is_netdev, Error **errp)
}
if (!err) {
- ret = net_client_init1(object, is_netdev, opts, &err);
+ ret = net_client_init1(object, is_netdev, &err);
}
if (object) {
diff --git a/net/dump.c b/net/dump.c
index f3d2fa9..b575430 100644
--- a/net/dump.c
+++ b/net/dump.c
@@ -144,8 +144,8 @@ static int net_dump_init(VLANState *vlan, const char *device,
return 0;
}
-int net_init_dump(QemuOpts *old_opts, const NetClientOptions *opts,
- const char *name, VLANState *vlan)
+int net_init_dump(const NetClientOptions *opts, const char *name,
+ VLANState *vlan)
{
int len;
const char *file;
diff --git a/net/slirp.c b/net/slirp.c
index 166304c..fc67f81 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -670,8 +670,8 @@ net_init_slirp_configs(const StringList *fwd, int flags)
}
}
-int net_init_slirp(QemuOpts *old_opts, const NetClientOptions *opts,
- const char *name, VLANState *vlan)
+int net_init_slirp(const NetClientOptions *opts, const char *name,
+ VLANState *vlan)
{
struct slirp_config_str *config;
char *vnet;
diff --git a/net/socket.c b/net/socket.c
index e3cba20..600c287 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -586,8 +586,8 @@ static int net_socket_udp_init(VLANState *vlan,
return 0;
}
-int net_init_socket(QemuOpts *old_opts, const NetClientOptions *opts,
- const char *name, VLANState *vlan)
+int net_init_socket(const NetClientOptions *opts, const char *name,
+ VLANState *vlan)
{
const NetdevSocketOptions *sock;
diff --git a/net/tap-win32.c b/net/tap-win32.c
index b6099cd..2328072 100644
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -699,8 +699,8 @@ static int tap_win32_init(VLANState *vlan, const char *model,
return 0;
}
-int net_init_tap(QemuOpts *old_opts, const NetClientOptions *opts,
- const char *name, VLANState *vlan)
+int net_init_tap(const NetClientOptions *opts, const char *name,
+ VLANState *vlan)
{
const NetdevTapOptions *tap;
diff --git a/net/tap.c b/net/tap.c
index fdaab2b..a996b07 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -512,8 +512,8 @@ static int net_bridge_run_helper(const char *helper, const char *bridge)
return -1;
}
-int net_init_bridge(QemuOpts *old_opts, const NetClientOptions *opts,
- const char *name, VLANState *vlan)
+int net_init_bridge(const NetClientOptions *opts, const char *name,
+ VLANState *vlan)
{
const NetdevBridgeOptions *bridge;
const char *helper, *br;
@@ -585,8 +585,8 @@ static int net_tap_init(const NetdevTapOptions *tap, int *vnet_hdr,
return fd;
}
-int net_init_tap(QemuOpts *old_opts, const NetClientOptions *opts,
- const char *name, VLANState *vlan)
+int net_init_tap(const NetClientOptions *opts, const char *name,
+ VLANState *vlan)
{
const NetdevTapOptions *tap;
diff --git a/net/vde.c b/net/vde.c
index 703888c..ee19f5c 100644
--- a/net/vde.c
+++ b/net/vde.c
@@ -110,8 +110,8 @@ static int net_vde_init(VLANState *vlan, const char *model,
return 0;
}
-int net_init_vde(QemuOpts *old_opts, const NetClientOptions *opts,
- const char *name, VLANState *vlan)
+int net_init_vde(const NetClientOptions *opts, const char *name,
+ VLANState *vlan)
{
const NetdevVdeOptions *vde;
--
1.7.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing
2012-06-13 8:22 [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing Laszlo Ersek
` (16 preceding siblings ...)
2012-06-13 8:22 ` [Qemu-devel] [PATCH v2 17/17] remove unused QemuOpts parameter from net init functions Laszlo Ersek
@ 2012-06-13 10:54 ` Paolo Bonzini
2012-07-01 13:33 ` Paolo Bonzini
2012-07-13 16:46 ` Luiz Capitulino
18 siblings, 1 reply; 38+ messages in thread
From: Paolo Bonzini @ 2012-06-13 10:54 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: qemu-devel
Il 13/06/2012 10:22, Laszlo Ersek ha scritto:
> Inspired by [1], the first half of this series attempts to implement a new
> visitor that should clean up defining and processing command line options.
> For a more detailed description, please see "[PATCH 05/17] qapi: introduce
> OptsVisitor".
>
> The second half converts -net/-netdev parsing to the new visitor.
>
> v1->v2:
> - Insert a small patch between v1 01/16 and v1 02/16 in order to generate
> C types for fixed-width integers.
> - Tighten / clean up integer types in Netdev options and in OptsVisitor.
> - NetLegacy::name is optional.
> - Changes are marked below individually and described separately.
> - (Rebase to current master.)
Looks good, thanks!
Paolo
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing
2012-06-13 10:54 ` [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing Paolo Bonzini
@ 2012-07-01 13:33 ` Paolo Bonzini
2012-07-02 13:17 ` Luiz Capitulino
0 siblings, 1 reply; 38+ messages in thread
From: Paolo Bonzini @ 2012-07-01 13:33 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: Laszlo Ersek, qemu-devel
Il 13/06/2012 12:54, Paolo Bonzini ha scritto:
> Il 13/06/2012 10:22, Laszlo Ersek ha scritto:
>> Inspired by [1], the first half of this series attempts to implement a new
>> visitor that should clean up defining and processing command line options.
>> For a more detailed description, please see "[PATCH 05/17] qapi: introduce
>> OptsVisitor".
>>
>> The second half converts -net/-netdev parsing to the new visitor.
>>
>> v1->v2:
>> - Insert a small patch between v1 01/16 and v1 02/16 in order to generate
>> C types for fixed-width integers.
>> - Tighten / clean up integer types in Netdev options and in OptsVisitor.
>> - NetLegacy::name is optional.
>> - Changes are marked below individually and described separately.
>> - (Rebase to current master.)
>
> Looks good, thanks!
Luiz, what's the state of this?
Paolo
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing
2012-07-01 13:33 ` Paolo Bonzini
@ 2012-07-02 13:17 ` Luiz Capitulino
0 siblings, 0 replies; 38+ messages in thread
From: Luiz Capitulino @ 2012-07-02 13:17 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Laszlo Ersek, qemu-devel
On Sun, 01 Jul 2012 15:33:13 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 13/06/2012 12:54, Paolo Bonzini ha scritto:
> > Il 13/06/2012 10:22, Laszlo Ersek ha scritto:
> >> Inspired by [1], the first half of this series attempts to implement a new
> >> visitor that should clean up defining and processing command line options.
> >> For a more detailed description, please see "[PATCH 05/17] qapi: introduce
> >> OptsVisitor".
> >>
> >> The second half converts -net/-netdev parsing to the new visitor.
> >>
> >> v1->v2:
> >> - Insert a small patch between v1 01/16 and v1 02/16 in order to generate
> >> C types for fixed-width integers.
> >> - Tighten / clean up integer types in Netdev options and in OptsVisitor.
> >> - NetLegacy::name is optional.
> >> - Changes are marked below individually and described separately.
> >> - (Rebase to current master.)
> >
> > Looks good, thanks!
>
> Luiz, what's the state of this?
Hmm, I've missed this one. Sorry for that.
I'll be off for a few days starting the day after tomorrow. I'll try to
review it before that.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing
2012-06-13 8:22 [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing Laszlo Ersek
` (17 preceding siblings ...)
2012-06-13 10:54 ` [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing Paolo Bonzini
@ 2012-07-13 16:46 ` Luiz Capitulino
2012-07-13 19:28 ` Laszlo Ersek
18 siblings, 1 reply; 38+ messages in thread
From: Luiz Capitulino @ 2012-07-13 16:46 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: qemu-devel
On Wed, 13 Jun 2012 10:22:31 +0200
Laszlo Ersek <lersek@redhat.com> wrote:
> Inspired by [1], the first half of this series attempts to implement a new
> visitor that should clean up defining and processing command line options.
> For a more detailed description, please see "[PATCH 05/17] qapi: introduce
> OptsVisitor".
>
> The second half converts -net/-netdev parsing to the new visitor.
The general approach looks fine to me, I've made comments to individual patches
and have two general comments:
1. This doesn't build for me:
In file included from /home/lcapitulino/work/src/qmp-unstable/net/slirp.c:24:0:
/home/lcapitulino/work/src/qmp-unstable/net/slirp.h:41:28: error: unknown type name ‘QemuOptsList’
/home/lcapitulino/work/src/qmp-unstable/net/slirp.c:741:5: error: no previous prototype for ‘net_slirp_parse_legacy’ [-Werror=missing-prototypes]
cc1: all warnings being treated as errors
make: *** [net/slirp.o] Error 1
make: *** Waiting for unfinished jobs....
2. I don't think this should go in through qmp's branch because this is more
about QemuOpts than about QMP. I suggest three alternatives:
- If you're going to go forward and convert more users, then I think
you should open your own branch, send pull requests etc
- Go through some -net three
- Ask Anthony to apply this directly
I'll, of course, review it though
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing
2012-07-13 16:46 ` Luiz Capitulino
@ 2012-07-13 19:28 ` Laszlo Ersek
0 siblings, 0 replies; 38+ messages in thread
From: Laszlo Ersek @ 2012-07-13 19:28 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: qemu-devel
On 07/13/12 18:46, Luiz Capitulino wrote:
> On Wed, 13 Jun 2012 10:22:31 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
>
>> Inspired by [1], the first half of this series attempts to implement a new
>> visitor that should clean up defining and processing command line options.
>> For a more detailed description, please see "[PATCH 05/17] qapi: introduce
>> OptsVisitor".
>>
>> The second half converts -net/-netdev parsing to the new visitor.
>
> The general approach looks fine to me, I've made comments to individual patches
> and have two general comments:
>
> 1. This doesn't build for me:
>
> In file included from /home/lcapitulino/work/src/qmp-unstable/net/slirp.c:24:0:
> /home/lcapitulino/work/src/qmp-unstable/net/slirp.h:41:28: error: unknown type name ‘QemuOptsList’
> /home/lcapitulino/work/src/qmp-unstable/net/slirp.c:741:5: error: no previous prototype for ‘net_slirp_parse_legacy’ [-Werror=missing-prototypes]
> cc1: all warnings being treated as errors
> make: *** [net/slirp.o] Error 1
> make: *** Waiting for unfinished jobs....
Okay this took some time to track down. When I posted v2, it was based
on 7677e24f in my clone. I made a mistake in 17/17, in "net/slirp.h": I
removed "qemu-option.h" after conversion was finished, because I didn't
notice net_slirp_parse_legacy() continued to depend on QemuOptsList. The
error went unnoticed because @ 7677e24f this was the relevant #include
tree, rooted at net/slirp.h:
net/slirp.h
qapi-types.h
qapi/qapi-types-core.h
monitor.h
qemu-char.h
qemu-option.h <---
block.h
qemu-aio.h
qemu-char.h
qemu-option.h <---
qemu-option.h <---
Then Paolo's patch was committed as ad608da5 ("qmp: do not include
monitor.h from qapi-types-core.h"). The above tree was cut at
"monitor.h", severing all three marked paths.
I must reinclude "qemu-option.h" and squash the change into 17/17.
>
> 2. I don't think this should go in through qmp's branch because this is more
> about QemuOpts than about QMP. I suggest three alternatives:
>
> - If you're going to go forward and convert more users, then I think
> you should open your own branch, send pull requests etc
>
> - Go through some -net three
>
> - Ask Anthony to apply this directly
>
> I'll, of course, review it though
I think I'll ask Anthony to apply v3 directly.
Thanks for the review!
Laszlo
^ permalink raw reply [flat|nested] 38+ messages in thread