* [Qemu-devel] [PATCH 01/17] qapi-types/visit.py: Pass whole expr dict for structs
2013-09-20 11:54 [Qemu-devel] [PATCH 00/17] blockdev-add QMP command Kevin Wolf
@ 2013-09-20 11:54 ` Kevin Wolf
2013-09-20 13:32 ` Max Reitz
2013-09-20 14:51 ` Eric Blake
2013-09-20 11:54 ` [Qemu-devel] [PATCH 02/17] qapi-types/visit.py: Inheritance " Kevin Wolf
` (15 subsequent siblings)
16 siblings, 2 replies; 56+ messages in thread
From: Kevin Wolf @ 2013-09-20 11:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, mreitz
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
scripts/qapi-types.py | 11 ++++++++---
scripts/qapi-visit.py | 8 ++++++--
2 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 5222463..566fe5e 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -71,7 +71,7 @@ def generate_struct_fields(members):
c_name=c_var(argname))
if structured:
push_indent()
- ret += generate_struct("", argname, argentry)
+ ret += generate_struct({ "field": argname, "data": argentry})
pop_indent()
else:
ret += mcgen('''
@@ -81,7 +81,12 @@ def generate_struct_fields(members):
return ret
-def generate_struct(structname, fieldname, members):
+def generate_struct(expr):
+
+ structname = expr.get('type', "")
+ fieldname = expr.get('field', "")
+ members = expr['data']
+
ret = mcgen('''
struct %(name)s
{
@@ -417,7 +422,7 @@ if do_builtins:
for expr in exprs:
ret = "\n"
if expr.has_key('type'):
- ret += generate_struct(expr['type'], "", expr['data']) + "\n"
+ ret += generate_struct(expr) + "\n"
ret += generate_type_cleanup_decl(expr['type'] + "List")
fdef.write(generate_type_cleanup(expr['type'] + "List") + "\n")
ret += generate_type_cleanup_decl(expr['type'])
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 597cca4..1e44004 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -120,7 +120,11 @@ if (!err) {
''')
return ret
-def generate_visit_struct(name, members):
+def generate_visit_struct(expr):
+
+ name = expr['type']
+ members = expr['data']
+
ret = generate_visit_struct_fields(name, "", "", members)
ret += mcgen('''
@@ -472,7 +476,7 @@ if do_builtins:
for expr in exprs:
if expr.has_key('type'):
- ret = generate_visit_struct(expr['type'], expr['data'])
+ ret = generate_visit_struct(expr)
ret += generate_visit_list(expr['type'], expr['data'])
fdef.write(ret)
--
1.8.1.4
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH 01/17] qapi-types/visit.py: Pass whole expr dict for structs
2013-09-20 11:54 ` [Qemu-devel] [PATCH 01/17] qapi-types/visit.py: Pass whole expr dict for structs Kevin Wolf
@ 2013-09-20 13:32 ` Max Reitz
2013-09-20 14:51 ` Eric Blake
1 sibling, 0 replies; 56+ messages in thread
From: Max Reitz @ 2013-09-20 13:32 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, stefanha, armbru
On 2013-09-20 13:54, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> scripts/qapi-types.py | 11 ++++++++---
> scripts/qapi-visit.py | 8 ++++++--
> 2 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 5222463..566fe5e 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -71,7 +71,7 @@ def generate_struct_fields(members):
> c_name=c_var(argname))
> if structured:
> push_indent()
> - ret += generate_struct("", argname, argentry)
> + ret += generate_struct({ "field": argname, "data": argentry})
> pop_indent()
> else:
> ret += mcgen('''
> @@ -81,7 +81,12 @@ def generate_struct_fields(members):
>
> return ret
>
> -def generate_struct(structname, fieldname, members):
> +def generate_struct(expr):
> +
> + structname = expr.get('type', "")
> + fieldname = expr.get('field', "")
> + members = expr['data']
> +
> ret = mcgen('''
> struct %(name)s
> {
> @@ -417,7 +422,7 @@ if do_builtins:
> for expr in exprs:
> ret = "\n"
> if expr.has_key('type'):
> - ret += generate_struct(expr['type'], "", expr['data']) + "\n"
> + ret += generate_struct(expr) + "\n"
This change passes expr['filter'] to generate_struct, if that field is
set (in contrast to before, where an empty string was passed). This
doesn't really make a difference, since defining the "field" key now
results in linking errors (multiple definitions of that variable) where
it was just ignored before (however, the behavior was undefined anyway).
I just thought I'd say it. ;)
Max
> ret += generate_type_cleanup_decl(expr['type'] + "List")
> fdef.write(generate_type_cleanup(expr['type'] + "List") + "\n")
> ret += generate_type_cleanup_decl(expr['type'])
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 597cca4..1e44004 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -120,7 +120,11 @@ if (!err) {
> ''')
> return ret
>
> -def generate_visit_struct(name, members):
> +def generate_visit_struct(expr):
> +
> + name = expr['type']
> + members = expr['data']
> +
> ret = generate_visit_struct_fields(name, "", "", members)
>
> ret += mcgen('''
> @@ -472,7 +476,7 @@ if do_builtins:
>
> for expr in exprs:
> if expr.has_key('type'):
> - ret = generate_visit_struct(expr['type'], expr['data'])
> + ret = generate_visit_struct(expr)
> ret += generate_visit_list(expr['type'], expr['data'])
> fdef.write(ret)
>
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH 01/17] qapi-types/visit.py: Pass whole expr dict for structs
2013-09-20 11:54 ` [Qemu-devel] [PATCH 01/17] qapi-types/visit.py: Pass whole expr dict for structs Kevin Wolf
2013-09-20 13:32 ` Max Reitz
@ 2013-09-20 14:51 ` Eric Blake
1 sibling, 0 replies; 56+ messages in thread
From: Eric Blake @ 2013-09-20 14:51 UTC (permalink / raw)
To: Kevin Wolf; +Cc: mreitz, qemu-devel, stefanha, armbru
[-- Attachment #1: Type: text/plain, Size: 393 bytes --]
On 09/20/2013 05:54 AM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> scripts/qapi-types.py | 11 ++++++++---
> scripts/qapi-visit.py | 8 ++++++--
> 2 files changed, 14 insertions(+), 5 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH 02/17] qapi-types/visit.py: Inheritance for structs
2013-09-20 11:54 [Qemu-devel] [PATCH 00/17] blockdev-add QMP command Kevin Wolf
2013-09-20 11:54 ` [Qemu-devel] [PATCH 01/17] qapi-types/visit.py: Pass whole expr dict for structs Kevin Wolf
@ 2013-09-20 11:54 ` Kevin Wolf
2013-09-20 13:33 ` Max Reitz
2013-09-20 14:58 ` Eric Blake
2013-09-20 11:54 ` [Qemu-devel] [PATCH 03/17] blockdev: Introduce DriveInfo.enable_auto_del Kevin Wolf
` (14 subsequent siblings)
16 siblings, 2 replies; 56+ messages in thread
From: Kevin Wolf @ 2013-09-20 11:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, mreitz
This introduces a new 'base' key for struct definitions that refers to
another struct type. On the JSON level, the fields of the base type are
included directly into the same namespace as the fields of the defined
type, like with unions. On the C level, a pointer to a struct of the
base type is included.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
docs/qapi-code-gen.txt | 17 +++++++++++++++++
scripts/qapi-types.py | 4 ++++
scripts/qapi-visit.py | 18 ++++++++++++++++--
3 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 0ce045c..2b872f1 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -53,6 +53,23 @@ The use of '*' as a prefix to the name means the member is optional. Optional
members should always be added to the end of the dictionary to preserve
backwards compatibility.
+
+A complex type definition can specify another complex type as its base.
+In this case, the fields of the base type are included as top-level fields
+of the new complex type's dictionary in the QMP wire format. An example
+definition is:
+
+ { 'type': 'BlockdevOptionsGenericFormat', 'data': { 'file': 'str' } }
+ { 'type': 'BlockdevOptionsGenericCOWFormat',
+ 'base': 'BlockdevOptionsGenericFormat',
+ 'data': { '*backing': 'str' } }
+
+An example BlockdevOptionsGenericCOWFormat object on the wire could use
+both fields like this:
+
+ { "file": "/some/place/my-image"
+ "backing": "/some/place/my-backing-file" } }
+
=== Enumeration types ===
An enumeration type is a dictionary containing a single key whose value is a
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 566fe5e..4a1652b 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -86,6 +86,7 @@ def generate_struct(expr):
structname = expr.get('type', "")
fieldname = expr.get('field', "")
members = expr['data']
+ base = expr.get('base')
ret = mcgen('''
struct %(name)s
@@ -93,6 +94,9 @@ struct %(name)s
''',
name=structname)
+ if base:
+ ret += generate_struct_fields({'base': base})
+
ret += generate_struct_fields(members)
if len(fieldname):
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 1e44004..90cedd7 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -17,7 +17,7 @@ import os
import getopt
import errno
-def generate_visit_struct_fields(name, field_prefix, fn_prefix, members):
+def generate_visit_struct_fields(name, field_prefix, fn_prefix, members, base = None):
substructs = []
ret = ''
full_name = name if not fn_prefix else "%s_%s" % (name, fn_prefix)
@@ -42,6 +42,19 @@ static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s ** obj, Error *
name=name, full_name=full_name)
push_indent()
+ if base:
+ ret += mcgen('''
+visit_start_implicit_struct(m, (void**) &(*obj)->%(c_name)s, sizeof(%(type)s), &err);
+if (!err) {
+ visit_type_%(type)s_fields(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, &err);
+ error_propagate(errp, err);
+ err = NULL;
+ visit_end_implicit_struct(m, &err);
+}
+''',
+ c_prefix=c_var(field_prefix),
+ type=type_name(base), c_name=c_var('base'))
+
for argname, argentry, optional, structured in parse_args(members):
if optional:
ret += mcgen('''
@@ -124,8 +137,9 @@ def generate_visit_struct(expr):
name = expr['type']
members = expr['data']
+ base = expr.get('base')
- ret = generate_visit_struct_fields(name, "", "", members)
+ ret = generate_visit_struct_fields(name, "", "", members, base)
ret += mcgen('''
--
1.8.1.4
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH 02/17] qapi-types/visit.py: Inheritance for structs
2013-09-20 11:54 ` [Qemu-devel] [PATCH 02/17] qapi-types/visit.py: Inheritance " Kevin Wolf
@ 2013-09-20 13:33 ` Max Reitz
2013-09-20 14:19 ` Kevin Wolf
2013-09-20 14:58 ` Eric Blake
1 sibling, 1 reply; 56+ messages in thread
From: Max Reitz @ 2013-09-20 13:33 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, stefanha, armbru
On 2013-09-20 13:54, Kevin Wolf wrote:
> This introduces a new 'base' key for struct definitions that refers to
> another struct type. On the JSON level, the fields of the base type are
> included directly into the same namespace as the fields of the defined
> type, like with unions. On the C level, a pointer to a struct of the
> base type is included.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> docs/qapi-code-gen.txt | 17 +++++++++++++++++
> scripts/qapi-types.py | 4 ++++
> scripts/qapi-visit.py | 18 ++++++++++++++++--
> 3 files changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index 0ce045c..2b872f1 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -53,6 +53,23 @@ The use of '*' as a prefix to the name means the member is optional. Optional
> members should always be added to the end of the dictionary to preserve
> backwards compatibility.
>
> +
> +A complex type definition can specify another complex type as its base.
> +In this case, the fields of the base type are included as top-level fields
> +of the new complex type's dictionary in the QMP wire format. An example
> +definition is:
> +
> + { 'type': 'BlockdevOptionsGenericFormat', 'data': { 'file': 'str' } }
> + { 'type': 'BlockdevOptionsGenericCOWFormat',
> + 'base': 'BlockdevOptionsGenericFormat',
> + 'data': { '*backing': 'str' } }
> +
> +An example BlockdevOptionsGenericCOWFormat object on the wire could use
> +both fields like this:
> +
> + { "file": "/some/place/my-image"
> + "backing": "/some/place/my-backing-file" } }
> +
> === Enumeration types ===
>
> An enumeration type is a dictionary containing a single key whose value is a
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 566fe5e..4a1652b 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -86,6 +86,7 @@ def generate_struct(expr):
> structname = expr.get('type', "")
> fieldname = expr.get('field', "")
> members = expr['data']
> + base = expr.get('base')
>
> ret = mcgen('''
> struct %(name)s
> @@ -93,6 +94,9 @@ struct %(name)s
> ''',
> name=structname)
>
> + if base:
> + ret += generate_struct_fields({'base': base})
> +
> ret += generate_struct_fields(members)
>
> if len(fieldname):
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 1e44004..90cedd7 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -17,7 +17,7 @@ import os
> import getopt
> import errno
>
> -def generate_visit_struct_fields(name, field_prefix, fn_prefix, members):
> +def generate_visit_struct_fields(name, field_prefix, fn_prefix, members, base = None):
> substructs = []
> ret = ''
> full_name = name if not fn_prefix else "%s_%s" % (name, fn_prefix)
> @@ -42,6 +42,19 @@ static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s ** obj, Error *
> name=name, full_name=full_name)
> push_indent()
>
> + if base:
> + ret += mcgen('''
> +visit_start_implicit_struct(m, (void**) &(*obj)->%(c_name)s, sizeof(%(type)s), &err);
Why do you just dereference obj here (implying it will never be NULL)...
> +if (!err) {
> + visit_type_%(type)s_fields(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, &err);
...but then you're checking whether it is NULL before dereferencing?
Max
> + error_propagate(errp, err);
> + err = NULL;
> + visit_end_implicit_struct(m, &err);
> +}
> +''',
> + c_prefix=c_var(field_prefix),
> + type=type_name(base), c_name=c_var('base'))
> +
> for argname, argentry, optional, structured in parse_args(members):
> if optional:
> ret += mcgen('''
> @@ -124,8 +137,9 @@ def generate_visit_struct(expr):
>
> name = expr['type']
> members = expr['data']
> + base = expr.get('base')
>
> - ret = generate_visit_struct_fields(name, "", "", members)
> + ret = generate_visit_struct_fields(name, "", "", members, base)
>
> ret += mcgen('''
>
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH 02/17] qapi-types/visit.py: Inheritance for structs
2013-09-20 13:33 ` Max Reitz
@ 2013-09-20 14:19 ` Kevin Wolf
0 siblings, 0 replies; 56+ messages in thread
From: Kevin Wolf @ 2013-09-20 14:19 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel, stefanha, armbru
Am 20.09.2013 um 15:33 hat Max Reitz geschrieben:
> On 2013-09-20 13:54, Kevin Wolf wrote:
> >This introduces a new 'base' key for struct definitions that refers to
> >another struct type. On the JSON level, the fields of the base type are
> >included directly into the same namespace as the fields of the defined
> >type, like with unions. On the C level, a pointer to a struct of the
> >base type is included.
> >
> >Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> >index 1e44004..90cedd7 100644
> >--- a/scripts/qapi-visit.py
> >+++ b/scripts/qapi-visit.py
> >@@ -17,7 +17,7 @@ import os
> > import getopt
> > import errno
> >-def generate_visit_struct_fields(name, field_prefix, fn_prefix, members):
> >+def generate_visit_struct_fields(name, field_prefix, fn_prefix, members, base = None):
> > substructs = []
> > ret = ''
> > full_name = name if not fn_prefix else "%s_%s" % (name, fn_prefix)
> >@@ -42,6 +42,19 @@ static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s ** obj, Error *
> > name=name, full_name=full_name)
> > push_indent()
> >+ if base:
> >+ ret += mcgen('''
> >+visit_start_implicit_struct(m, (void**) &(*obj)->%(c_name)s, sizeof(%(type)s), &err);
> Why do you just dereference obj here (implying it will never be NULL)...
>
> >+if (!err) {
> >+ visit_type_%(type)s_fields(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, &err);
> ...but then you're checking whether it is NULL before dereferencing?
Because I'm clearly not understanding what I'm doing here... I can
change the first line into this:
visit_start_implicit_struct(m, obj ? (void**) &(*obj)->%(c_name)s : NULL, sizeof(%(type)s), &err);
I'm not sure what cases it would fix, but the surrounding code looks
like it's doing the check, so it can't hurt. visit_start_implicit_struct
seems to be able to handle NULL pointers.
Kevin
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [Qemu-devel] [PATCH 02/17] qapi-types/visit.py: Inheritance for structs
2013-09-20 11:54 ` [Qemu-devel] [PATCH 02/17] qapi-types/visit.py: Inheritance " Kevin Wolf
2013-09-20 13:33 ` Max Reitz
@ 2013-09-20 14:58 ` Eric Blake
1 sibling, 0 replies; 56+ messages in thread
From: Eric Blake @ 2013-09-20 14:58 UTC (permalink / raw)
To: Kevin Wolf; +Cc: mreitz, qemu-devel, stefanha, armbru
[-- Attachment #1: Type: text/plain, Size: 979 bytes --]
On 09/20/2013 05:54 AM, Kevin Wolf wrote:
> This introduces a new 'base' key for struct definitions that refers to
> another struct type. On the JSON level, the fields of the base type are
> included directly into the same namespace as the fields of the defined
> type, like with unions. On the C level, a pointer to a struct of the
> base type is included.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> docs/qapi-code-gen.txt | 17 +++++++++++++++++
> scripts/qapi-types.py | 4 ++++
> scripts/qapi-visit.py | 18 ++++++++++++++++--
> 3 files changed, 37 insertions(+), 2 deletions(-)
> +
> +An example BlockdevOptionsGenericCOWFormat object on the wire could use
> +both fields like this:
> +
> + { "file": "/some/place/my-image"
Missing a comma.
> + "backing": "/some/place/my-backing-file" } }
Mismatch - too many }.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH 03/17] blockdev: Introduce DriveInfo.enable_auto_del
2013-09-20 11:54 [Qemu-devel] [PATCH 00/17] blockdev-add QMP command Kevin Wolf
2013-09-20 11:54 ` [Qemu-devel] [PATCH 01/17] qapi-types/visit.py: Pass whole expr dict for structs Kevin Wolf
2013-09-20 11:54 ` [Qemu-devel] [PATCH 02/17] qapi-types/visit.py: Inheritance " Kevin Wolf
@ 2013-09-20 11:54 ` Kevin Wolf
2013-09-20 15:03 ` Eric Blake
2013-09-30 5:05 ` Wenchao Xia
2013-09-20 11:54 ` [Qemu-devel] [PATCH 04/17] blockdev: 'blockdev-add' QMP command Kevin Wolf
` (13 subsequent siblings)
16 siblings, 2 replies; 56+ messages in thread
From: Kevin Wolf @ 2013-09-20 11:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, mreitz
BlockDriverStates shouldn't be affected by an unplugged guest device,
except if created with the legacy -drive command line option or the
drive_add HMP command.
Make the automatic deletion as well as cancelling of jobs conditional on
an enable_auto_del boolean that is only set in drive_init().
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
blockdev.c | 17 ++++++++++++++++-
include/sysemu/blockdev.h | 1 +
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/blockdev.c b/blockdev.c
index 80605a2..977dc94 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -89,6 +89,10 @@ void blockdev_mark_auto_del(BlockDriverState *bs)
{
DriveInfo *dinfo = drive_get_by_blockdev(bs);
+ if (dinfo && !dinfo->enable_auto_del) {
+ return;
+ }
+
if (bs->job) {
block_job_cancel(bs->job);
}
@@ -750,6 +754,7 @@ static void qemu_opt_rename(QemuOpts *opts, const char *from, const char *to)
DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
{
const char *value;
+ DriveInfo *dinfo;
/* Change legacy command line options into QMP ones */
qemu_opt_rename(all_opts, "iops", "throttling.iops-total");
@@ -798,7 +803,17 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
qemu_opt_unset(all_opts, "cache");
}
- return blockdev_init(all_opts, block_default_type);
+ /* Actual block device init: Functionality shared with blockdev-add */
+ dinfo = blockdev_init(all_opts, block_default_type);
+ if (dinfo == NULL) {
+ goto fail;
+ }
+
+ /* Set legacy DriveInfo fields */
+ dinfo->enable_auto_del = true;
+
+fail:
+ return dinfo;
}
void do_commit(Monitor *mon, const QDict *qdict)
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 804ec88..1082091 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -37,6 +37,7 @@ struct DriveInfo {
int bus;
int unit;
int auto_del; /* see blockdev_mark_auto_del() */
+ bool enable_auto_del; /* Only for legacy drive_init() */
int media_cd;
int cyls, heads, secs, trans;
QemuOpts *opts;
--
1.8.1.4
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH 03/17] blockdev: Introduce DriveInfo.enable_auto_del
2013-09-20 11:54 ` [Qemu-devel] [PATCH 03/17] blockdev: Introduce DriveInfo.enable_auto_del Kevin Wolf
@ 2013-09-20 15:03 ` Eric Blake
2013-09-20 15:12 ` Kevin Wolf
2013-09-30 5:05 ` Wenchao Xia
1 sibling, 1 reply; 56+ messages in thread
From: Eric Blake @ 2013-09-20 15:03 UTC (permalink / raw)
To: Kevin Wolf; +Cc: mreitz, qemu-devel, stefanha, armbru
[-- Attachment #1: Type: text/plain, Size: 860 bytes --]
On 09/20/2013 05:54 AM, Kevin Wolf wrote:
> BlockDriverStates shouldn't be affected by an unplugged guest device,
> except if created with the legacy -drive command line option or the
> drive_add HMP command.
>
> Make the automatic deletion as well as cancelling of jobs conditional on
> an enable_auto_del boolean that is only set in drive_init().
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> +++ b/include/sysemu/blockdev.h
> @@ -37,6 +37,7 @@ struct DriveInfo {
> int bus;
> int unit;
> int auto_del; /* see blockdev_mark_auto_del() */
> + bool enable_auto_del; /* Only for legacy drive_init() */
auto_del is 0-initialized, and only ever assigned to 1. Should it also
be bool?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH 03/17] blockdev: Introduce DriveInfo.enable_auto_del
2013-09-20 15:03 ` Eric Blake
@ 2013-09-20 15:12 ` Kevin Wolf
2013-09-20 15:25 ` Eric Blake
0 siblings, 1 reply; 56+ messages in thread
From: Kevin Wolf @ 2013-09-20 15:12 UTC (permalink / raw)
To: Eric Blake; +Cc: mreitz, qemu-devel, stefanha, armbru
Am 20.09.2013 um 17:03 hat Eric Blake geschrieben:
> On 09/20/2013 05:54 AM, Kevin Wolf wrote:
> > BlockDriverStates shouldn't be affected by an unplugged guest device,
> > except if created with the legacy -drive command line option or the
> > drive_add HMP command.
> >
> > Make the automatic deletion as well as cancelling of jobs conditional on
> > an enable_auto_del boolean that is only set in drive_init().
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
>
> > +++ b/include/sysemu/blockdev.h
> > @@ -37,6 +37,7 @@ struct DriveInfo {
> > int bus;
> > int unit;
> > int auto_del; /* see blockdev_mark_auto_del() */
> > + bool enable_auto_del; /* Only for legacy drive_init() */
>
> auto_del is 0-initialized, and only ever assigned to 1. Should it also
> be bool?
Probably. A lot of qemu code uses int for boolean values. I'm trying to
get rid of them in the block layer in the long run, but so far I haven't
aggressively converted fields that I don't touch otherwise.
Kevin
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH 03/17] blockdev: Introduce DriveInfo.enable_auto_del
2013-09-20 15:12 ` Kevin Wolf
@ 2013-09-20 15:25 ` Eric Blake
0 siblings, 0 replies; 56+ messages in thread
From: Eric Blake @ 2013-09-20 15:25 UTC (permalink / raw)
To: Kevin Wolf; +Cc: mreitz, qemu-devel, stefanha, armbru
[-- Attachment #1: Type: text/plain, Size: 1332 bytes --]
On 09/20/2013 09:12 AM, Kevin Wolf wrote:
> Am 20.09.2013 um 17:03 hat Eric Blake geschrieben:
>> On 09/20/2013 05:54 AM, Kevin Wolf wrote:
>>> BlockDriverStates shouldn't be affected by an unplugged guest device,
>>> except if created with the legacy -drive command line option or the
>>> drive_add HMP command.
>>>
>>> Make the automatic deletion as well as cancelling of jobs conditional on
>>> an enable_auto_del boolean that is only set in drive_init().
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>
>>> +++ b/include/sysemu/blockdev.h
>>> @@ -37,6 +37,7 @@ struct DriveInfo {
>>> int bus;
>>> int unit;
>>> int auto_del; /* see blockdev_mark_auto_del() */
>>> + bool enable_auto_del; /* Only for legacy drive_init() */
>>
>> auto_del is 0-initialized, and only ever assigned to 1. Should it also
>> be bool?
>
> Probably. A lot of qemu code uses int for boolean values. I'm trying to
> get rid of them in the block layer in the long run, but so far I haven't
> aggressively converted fields that I don't touch otherwise.
At any rate, that's an independent question; I forgot to mention:
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [Qemu-devel] [PATCH 03/17] blockdev: Introduce DriveInfo.enable_auto_del
2013-09-20 11:54 ` [Qemu-devel] [PATCH 03/17] blockdev: Introduce DriveInfo.enable_auto_del Kevin Wolf
2013-09-20 15:03 ` Eric Blake
@ 2013-09-30 5:05 ` Wenchao Xia
1 sibling, 0 replies; 56+ messages in thread
From: Wenchao Xia @ 2013-09-30 5:05 UTC (permalink / raw)
To: Kevin Wolf; +Cc: mreitz, qemu-devel, stefanha, armbru
Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH 04/17] blockdev: 'blockdev-add' QMP command
2013-09-20 11:54 [Qemu-devel] [PATCH 00/17] blockdev-add QMP command Kevin Wolf
` (2 preceding siblings ...)
2013-09-20 11:54 ` [Qemu-devel] [PATCH 03/17] blockdev: Introduce DriveInfo.enable_auto_del Kevin Wolf
@ 2013-09-20 11:54 ` Kevin Wolf
2013-09-20 13:34 ` Max Reitz
` (4 more replies)
2013-09-20 11:54 ` [Qemu-devel] [PATCH 05/17] blockdev: Separate ID generation from DriveInfo creation Kevin Wolf
` (12 subsequent siblings)
16 siblings, 5 replies; 56+ messages in thread
From: Kevin Wolf @ 2013-09-20 11:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, mreitz
For examples see the changes to qmp-commands.hx.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
blockdev.c | 57 ++++++++++++
qapi-schema.json | 270 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
qmp-commands.hx | 59 ++++++++++++
3 files changed, 386 insertions(+)
diff --git a/blockdev.c b/blockdev.c
index 977dc94..c4297d8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -38,6 +38,8 @@
#include "qemu/option.h"
#include "qemu/config-file.h"
#include "qapi/qmp/types.h"
+#include "qapi-visit.h"
+#include "qapi/qmp-output-visitor.h"
#include "sysemu/sysemu.h"
#include "block/block_int.h"
#include "qmp-commands.h"
@@ -2066,6 +2068,61 @@ void qmp_block_job_complete(const char *device, Error **errp)
block_job_complete(job, errp);
}
+void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
+{
+ QmpOutputVisitor *ov = qmp_output_visitor_new();
+ QObject *obj;
+ QDict *qdict;
+ DriveInfo *dinfo;
+ Error *local_err = NULL;
+
+ /* Require an ID in the top level */
+ if (!options->has_id) {
+ error_setg(errp, "Block device needs an ID");
+ return;
+ }
+
+ /* TODO Sort it out in raw-posix and drive_init: Reject aio=native with
+ * cache.direct=false instead of silently switching to aio=threads, except
+ * if called from drive_init.
+ *
+ * For now, simply forbidding the combination for all drivers will do. */
+ if (options->has_aio && options->aio == BLOCKDEV_A_I_O_OPTIONS_NATIVE) {
+ bool direct = options->cache->has_direct && options->cache->direct;
+ if (!options->has_cache && !direct) {
+ error_setg(errp, "aio=native requires cache.direct=true");
+ goto fail;
+ }
+ }
+
+ visit_type_BlockdevOptions(qmp_output_get_visitor(ov),
+ &options, NULL, &local_err);
+ if (error_is_set(&local_err)) {
+ error_propagate(errp, local_err);
+ goto fail;
+ }
+
+ obj = qmp_output_get_qobject(ov);
+ qdict = qobject_to_qdict(obj);
+
+ qdict_flatten(qdict);
+
+ QemuOpts *opts = qemu_opts_from_qdict(&qemu_drive_opts, qdict, &local_err);
+ if (error_is_set(&local_err)) {
+ error_propagate(errp, local_err);
+ goto fail;
+ } else {
+ dinfo = blockdev_init(opts, IF_NONE);
+ if (!dinfo) {
+ error_setg(errp, "Could not open image");
+ goto fail;
+ }
+ }
+
+fail:
+ qmp_output_visitor_cleanup(ov);
+}
+
static void do_qmp_query_block_jobs_one(void *opaque, BlockDriverState *bs)
{
BlockJobInfoList **prev = opaque;
diff --git a/qapi-schema.json b/qapi-schema.json
index 145eca8..7e8ce60 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3902,3 +3902,273 @@
##
{ 'command': 'query-rx-filter', 'data': { '*name': 'str' },
'returns': ['RxFilterInfo'] }
+
+
+##
+# @BlockdevDiscardOptions
+#
+# Determines how to handle discard requests.
+#
+# @ignore: Ignore the request
+# @unmap: Forward as an unmap request
+#
+# Since: 1.7
+##
+{ 'enum': 'BlockdevDiscardOptions',
+ 'data': [ 'ignore', 'unmap' ] }
+
+##
+# @BlockdevAIOOptions
+#
+# Selects the AIO backend to handle I/O requests
+#
+# @threads: Use qemu's thread pool
+# @native: Use native AIO backend (only Linux and Windows)
+#
+# Since: 1.7
+##
+{ 'enum': 'BlockdevAIOOptions',
+ 'data': [ 'threads', 'native' ] }
+
+##
+# @BlockdevCacheOptions
+#
+# Includes cache-related options for block devices
+#
+# @writeback: #optional enables writeback mode for any caches (default: true)
+# @direct: #optional enables use of O_DIRECT (bypass the host page cache;
+# default: false)
+# @no-flush: #optional ignore any flush requests for the device (default:
+# false)
+#
+# Since: 1.7
+##
+{ 'type': 'BlockdevCacheOptions',
+ 'data': { '*writeback': 'bool',
+ '*direct': 'bool',
+ '*no-flush': 'bool' } }
+
+##
+# @BlockdevThrottlingOptions
+#
+# Includes block device options related to I/O throttling. Leaving an option out
+# means the same as assigning 0 and applies no throttling.
+#
+# @bps-total: #optional limit total bytes per second
+# @bps-read: #optional limit read bytes per second
+# @bps-write: #optional limit written bytes per second
+# @iops-total: #optional limit total I/O operations per second
+# @iops-read: #optional limit read operations per second
+# @iops-write: #optional limit write operations per second
+#
+# Since: 1.7
+##
+{ 'type': 'BlockdevThrottlingOptions',
+ 'data': { '*bps-total': 'int',
+ '*bps-read': 'int',
+ '*bps-write': 'int',
+ '*iops-total': 'int',
+ '*iops-read': 'int',
+ '*iops-write': 'int' } }
+
+##
+# @BlockdevOptionsBase
+#
+# Options that are available for all block devices, independent of the block
+# driver.
+#
+# @driver: block driver name
+# @id: #optional id by which the new block device can be referred to.
+# This is a required option on the top level of blockdev-add, and
+# currently not allowed on any other level.
+# @discard: #optional discard-related options (default: ignore)
+# @cache: #optional cache-related options
+# @aio: #optional AIO backend (default: threads)
+# @rerror: #optional how to handle read errors on the device
+# (default: report)
+# @werror: #optional how to handle write errors on the device
+# (default: enospc)
+# @throttling: #optional I/O throttling related options
+# @read-only: #optional whether the block device should be read-only
+# (default: false)
+#
+# Since: 1.7
+##
+{ 'type': 'BlockdevOptionsBase',
+ 'data': { 'driver': 'str',
+ '*id': 'str',
+ '*discard': 'BlockdevDiscardOptions',
+ '*cache': 'BlockdevCacheOptions',
+ '*aio': 'BlockdevAIOOptions',
+ '*rerror': 'BlockdevOnError',
+ '*werror': 'BlockdevOnError',
+ '*throttling': 'BlockdevThrottlingOptions',
+ '*read-only': 'bool' } }
+
+##
+# @BlockdevOptionsFile
+#
+# Driver specific block device options for the file backend and similar
+# protocols.
+#
+# @filename: path to the image file
+#
+# Since: 1.7
+##
+{ 'type': 'BlockdevOptionsFile',
+ 'data': { 'filename': 'str' } }
+
+##
+# @BlockdevOptionsVVFAT
+#
+# Driver specific block device options for the vvfat protocol.
+#
+# @dir: directory to be exported as FAT image
+# @fat-type: #optional FAT type: 12, 16 or 32
+# @floppy: #optional whether to export a floppy imae (true) or partitioned
+# hard disk (false; default)
+# @rw: #optional whether to allow write operations (default: false)
+#
+# Since: 1.7
+##
+{ 'type': 'BlockdevOptionsVVFAT',
+ 'data': { 'dir': 'str', '*fat-type': 'int', '*floppy': 'bool',
+ '*rw': 'bool' } }
+
+##
+# @BlockdevOptionsGenericFormat
+#
+# Driver specific block device options for image format that have no option
+# besides their data source.
+#
+# @file: reference to or definition of the data source block device
+#
+# Since: 1.7
+##
+{ 'type': 'BlockdevOptionsGenericFormat',
+ 'data': { 'file': 'BlockdevRef' } }
+
+##
+# @BlockdevOptionsGenericCOWFormat
+#
+# Driver specific block device options for image format that have no option
+# besides their data source and an optional backing file.
+#
+# @file: reference to or definition of the data source block device
+# @backing: #optional reference to or definition of the backing file block
+# device (if missing, taken from the image file content). It is
+# allowed to pass an empty string here in order to disable the
+# default backing file.
+# @copy-on-read: #optional whether to enable copy on read for the device
+# (default: false). Copy on read can only be used if the
+# image is not read-only.
+#
+# Since: 1.7
+##
+{ 'type': 'BlockdevOptionsGenericCOWFormat',
+ 'base': 'BlockdevOptionsGenericFormat',
+ 'data': { '*backing': 'BlockdevRef',
+ '*copy-on-read': 'bool' } }
+##
+# @BlockdevOptionsQcow2
+#
+# Driver specific block device options for qcow2.
+#
+# @file: reference to or definition of the data source block device
+#
+# @backing: #optional reference to or definition of the backing file block
+# device (if missing, taken from the image file content)
+#
+# @lazy-refcounts: #optional whether to enable the lazy refcounts feature
+# (default is taken from the image file)
+#
+# @pass-discard-request: #optional whether discard requests to the qcow2 device
+# should be forwarded to the data source
+#
+# @pass-discard-snapshot: #optional whether discard requests for the data source
+# should be issued when a snapshot operation (e.g.
+# deleting a snapshot) frees clusters in the qcow2 file
+#
+# @pass-discard-other: #optional whether discard requests for the data source
+# should be issued on other occasions where a cluster gets
+# freed
+#
+# Since: 1.7
+##
+{ 'type': 'BlockdevOptionsQcow2',
+ 'base': 'BlockdevOptionsGenericCOWFormat',
+ 'data': { '*lazy-refcounts': 'bool',
+ '*pass-discard-request': 'bool',
+ '*pass-discard-snapshot': 'bool',
+ '*pass-discard-other': 'bool' } }
+
+##
+# @BlockdevOptions
+#
+# Options for creating a block device.
+#
+# Since: 1.7
+##
+{ 'union': 'BlockdevOptions',
+ 'base': 'BlockdevOptionsBase',
+ 'discriminator': 'driver',
+ 'data': {
+ 'file': 'BlockdevOptionsFile',
+ 'http': 'BlockdevOptionsFile',
+ 'https': 'BlockdevOptionsFile',
+ 'ftp': 'BlockdevOptionsFile',
+ 'ftps': 'BlockdevOptionsFile',
+ 'tftp': 'BlockdevOptionsFile',
+# TODO gluster: Wait for structured options
+# TODO iscsi: Wait for structured options
+# TODO nbd: Should take InetSocketAddress for 'host'?
+# TODO rbd: Wait for structured options
+# TODO sheepdog: Wait for structured options
+# TODO ssh: Should take InetSocketAddress for 'host'?
+ 'vvfat': 'BlockdevOptionsVVFAT',
+
+# TODO blkdebug: Wait for structured options
+# TODO blkverify: Wait for structured options
+
+ 'bochs': 'BlockdevOptionsGenericFormat',
+ 'cloop': 'BlockdevOptionsGenericFormat',
+ 'cow': 'BlockdevOptionsGenericCOWFormat',
+ 'dmg': 'BlockdevOptionsGenericFormat',
+ 'parallels': 'BlockdevOptionsGenericFormat',
+ 'qcow': 'BlockdevOptionsGenericCOWFormat',
+ 'qcow2': 'BlockdevOptionsQcow2',
+ 'qed': 'BlockdevOptionsGenericCOWFormat',
+ 'raw': 'BlockdevOptionsGenericFormat',
+ 'vdi': 'BlockdevOptionsGenericFormat',
+ 'vhdx': 'BlockdevOptionsGenericFormat',
+ 'vmdk': 'BlockdevOptionsGenericCOWFormat',
+ 'vpc': 'BlockdevOptionsGenericFormat'
+ } }
+
+##
+# @BlockdevRef
+#
+# Reference to a block device.
+#
+# @definition: defines a new block device inline
+# @reference: references the ID of an existing block device. An
+# empty string means that no block device should be
+# referenced.
+#
+# Since: 1.7
+##
+{ 'union': 'BlockdevRef',
+ 'discriminator': {},
+ 'data': { 'definition': 'BlockdevOptions',
+ 'reference': 'str' } }
+
+##
+# @blockdev-add:
+#
+# Creates a new block device.
+#
+# @options: block device options for the new device
+#
+# Since: 1.7
+##
+{ 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index b17c46e..449ecea 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3240,3 +3240,62 @@ Example:
}
EQMP
+
+ {
+ .name = "blockdev-add",
+ .args_type = "options:q",
+ .mhandler.cmd_new = qmp_marshal_input_blockdev_add,
+ },
+
+SQMP
+blockdev-add
+------------
+
+Add a block device.
+
+Arguments:
+
+- "options": block driver options
+
+Example (1):
+
+-> { "execute": "blockdev-add",
+ "arguments": { "options" : { "driver": "qcow2",
+ "file": { "driver": "file",
+ "filename": "test.qcow2" } } } }
+<- { "return": {} }
+
+Example (2):
+
+-> { "execute": "blockdev-add",
+ "arguments": {
+ "options": {
+ "driver": "qcow2",
+ "id": "my_disk",
+ "discard": "unmap",
+ "throttling": {
+ "bps-total": 1234567,
+ "iops-write": 100
+ },
+ "cache": {
+ "direct": true,
+ "writeback": true
+ },
+ "file": {
+ "driver": "file",
+ "filename": "/tmp/test.qcow2"
+ },
+ "backing": {
+ "driver": "raw",
+ "file": {
+ "driver": "file",
+ "filename": "/dev/fdset/4"
+ }
+ }
+ }
+ }
+ }
+
+<- { "return": {} }
+
+EQMP
--
1.8.1.4
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH 04/17] blockdev: 'blockdev-add' QMP command
2013-09-20 11:54 ` [Qemu-devel] [PATCH 04/17] blockdev: 'blockdev-add' QMP command Kevin Wolf
@ 2013-09-20 13:34 ` Max Reitz
2013-09-20 14:57 ` Kevin Wolf
2013-09-20 14:01 ` Benoît Canet
` (3 subsequent siblings)
4 siblings, 1 reply; 56+ messages in thread
From: Max Reitz @ 2013-09-20 13:34 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, stefanha, armbru
On 2013-09-20 13:54, Kevin Wolf wrote:
> For examples see the changes to qmp-commands.hx.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> blockdev.c | 57 ++++++++++++
> qapi-schema.json | 270 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> qmp-commands.hx | 59 ++++++++++++
> 3 files changed, 386 insertions(+)
>
> diff --git a/blockdev.c b/blockdev.c
> index 977dc94..c4297d8 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -38,6 +38,8 @@
> #include "qemu/option.h"
> #include "qemu/config-file.h"
> #include "qapi/qmp/types.h"
> +#include "qapi-visit.h"
> +#include "qapi/qmp-output-visitor.h"
> #include "sysemu/sysemu.h"
> #include "block/block_int.h"
> #include "qmp-commands.h"
> @@ -2066,6 +2068,61 @@ void qmp_block_job_complete(const char *device, Error **errp)
> block_job_complete(job, errp);
> }
>
> +void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
> +{
> + QmpOutputVisitor *ov = qmp_output_visitor_new();
> + QObject *obj;
> + QDict *qdict;
> + DriveInfo *dinfo;
> + Error *local_err = NULL;
> +
> + /* Require an ID in the top level */
> + if (!options->has_id) {
> + error_setg(errp, "Block device needs an ID");
> + return;
> + }
> +
> + /* TODO Sort it out in raw-posix and drive_init: Reject aio=native with
> + * cache.direct=false instead of silently switching to aio=threads, except
> + * if called from drive_init.
> + *
> + * For now, simply forbidding the combination for all drivers will do. */
> + if (options->has_aio && options->aio == BLOCKDEV_A_I_O_OPTIONS_NATIVE) {
> + bool direct = options->cache->has_direct && options->cache->direct;
> + if (!options->has_cache && !direct) {
> + error_setg(errp, "aio=native requires cache.direct=true");
> + goto fail;
> + }
> + }
> +
> + visit_type_BlockdevOptions(qmp_output_get_visitor(ov),
> + &options, NULL, &local_err);
> + if (error_is_set(&local_err)) {
> + error_propagate(errp, local_err);
> + goto fail;
> + }
> +
> + obj = qmp_output_get_qobject(ov);
> + qdict = qobject_to_qdict(obj);
> +
> + qdict_flatten(qdict);
> +
> + QemuOpts *opts = qemu_opts_from_qdict(&qemu_drive_opts, qdict, &local_err);
> + if (error_is_set(&local_err)) {
> + error_propagate(errp, local_err);
> + goto fail;
> + } else {
> + dinfo = blockdev_init(opts, IF_NONE);
> + if (!dinfo) {
> + error_setg(errp, "Could not open image");
> + goto fail;
> + }
> + }
> +
> +fail:
> + qmp_output_visitor_cleanup(ov);
> +}
> +
> static void do_qmp_query_block_jobs_one(void *opaque, BlockDriverState *bs)
> {
> BlockJobInfoList **prev = opaque;
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 145eca8..7e8ce60 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3902,3 +3902,273 @@
> ##
> { 'command': 'query-rx-filter', 'data': { '*name': 'str' },
> 'returns': ['RxFilterInfo'] }
> +
> +
> +##
> +# @BlockdevDiscardOptions
> +#
> +# Determines how to handle discard requests.
> +#
> +# @ignore: Ignore the request
> +# @unmap: Forward as an unmap request
> +#
> +# Since: 1.7
> +##
> +{ 'enum': 'BlockdevDiscardOptions',
> + 'data': [ 'ignore', 'unmap' ] }
> +
> +##
> +# @BlockdevAIOOptions
> +#
> +# Selects the AIO backend to handle I/O requests
> +#
> +# @threads: Use qemu's thread pool
> +# @native: Use native AIO backend (only Linux and Windows)
> +#
> +# Since: 1.7
> +##
> +{ 'enum': 'BlockdevAIOOptions',
> + 'data': [ 'threads', 'native' ] }
> +
> +##
> +# @BlockdevCacheOptions
> +#
> +# Includes cache-related options for block devices
> +#
> +# @writeback: #optional enables writeback mode for any caches (default: true)
> +# @direct: #optional enables use of O_DIRECT (bypass the host page cache;
> +# default: false)
> +# @no-flush: #optional ignore any flush requests for the device (default:
> +# false)
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevCacheOptions',
> + 'data': { '*writeback': 'bool',
> + '*direct': 'bool',
> + '*no-flush': 'bool' } }
> +
> +##
> +# @BlockdevThrottlingOptions
> +#
> +# Includes block device options related to I/O throttling. Leaving an option out
> +# means the same as assigning 0 and applies no throttling.
> +#
> +# @bps-total: #optional limit total bytes per second
> +# @bps-read: #optional limit read bytes per second
> +# @bps-write: #optional limit written bytes per second
> +# @iops-total: #optional limit total I/O operations per second
> +# @iops-read: #optional limit read operations per second
> +# @iops-write: #optional limit write operations per second
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevThrottlingOptions',
> + 'data': { '*bps-total': 'int',
> + '*bps-read': 'int',
> + '*bps-write': 'int',
> + '*iops-total': 'int',
> + '*iops-read': 'int',
> + '*iops-write': 'int' } }
Hm, what about burst throttling?
> +
> +##
> +# @BlockdevOptionsBase
> +#
> +# Options that are available for all block devices, independent of the block
> +# driver.
> +#
> +# @driver: block driver name
> +# @id: #optional id by which the new block device can be referred to.
> +# This is a required option on the top level of blockdev-add, and
> +# currently not allowed on any other level.
> +# @discard: #optional discard-related options (default: ignore)
> +# @cache: #optional cache-related options
> +# @aio: #optional AIO backend (default: threads)
> +# @rerror: #optional how to handle read errors on the device
> +# (default: report)
> +# @werror: #optional how to handle write errors on the device
> +# (default: enospc)
> +# @throttling: #optional I/O throttling related options
> +# @read-only: #optional whether the block device should be read-only
> +# (default: false)
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevOptionsBase',
> + 'data': { 'driver': 'str',
> + '*id': 'str',
> + '*discard': 'BlockdevDiscardOptions',
> + '*cache': 'BlockdevCacheOptions',
> + '*aio': 'BlockdevAIOOptions',
> + '*rerror': 'BlockdevOnError',
> + '*werror': 'BlockdevOnError',
> + '*throttling': 'BlockdevThrottlingOptions',
> + '*read-only': 'bool' } }
> +
> +##
> +# @BlockdevOptionsFile
> +#
> +# Driver specific block device options for the file backend and similar
> +# protocols.
> +#
> +# @filename: path to the image file
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevOptionsFile',
> + 'data': { 'filename': 'str' } }
> +
> +##
> +# @BlockdevOptionsVVFAT
> +#
> +# Driver specific block device options for the vvfat protocol.
> +#
> +# @dir: directory to be exported as FAT image
> +# @fat-type: #optional FAT type: 12, 16 or 32
> +# @floppy: #optional whether to export a floppy imae (true) or partitioned
s/imae/image/
Max
> +# hard disk (false; default)
> +# @rw: #optional whether to allow write operations (default: false)
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevOptionsVVFAT',
> + 'data': { 'dir': 'str', '*fat-type': 'int', '*floppy': 'bool',
> + '*rw': 'bool' } }
> +
> +##
> +# @BlockdevOptionsGenericFormat
> +#
> +# Driver specific block device options for image format that have no option
> +# besides their data source.
> +#
> +# @file: reference to or definition of the data source block device
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevOptionsGenericFormat',
> + 'data': { 'file': 'BlockdevRef' } }
> +
> +##
> +# @BlockdevOptionsGenericCOWFormat
> +#
> +# Driver specific block device options for image format that have no option
> +# besides their data source and an optional backing file.
> +#
> +# @file: reference to or definition of the data source block device
> +# @backing: #optional reference to or definition of the backing file block
> +# device (if missing, taken from the image file content). It is
> +# allowed to pass an empty string here in order to disable the
> +# default backing file.
> +# @copy-on-read: #optional whether to enable copy on read for the device
> +# (default: false). Copy on read can only be used if the
> +# image is not read-only.
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevOptionsGenericCOWFormat',
> + 'base': 'BlockdevOptionsGenericFormat',
> + 'data': { '*backing': 'BlockdevRef',
> + '*copy-on-read': 'bool' } }
> +##
> +# @BlockdevOptionsQcow2
> +#
> +# Driver specific block device options for qcow2.
> +#
> +# @file: reference to or definition of the data source block device
> +#
> +# @backing: #optional reference to or definition of the backing file block
> +# device (if missing, taken from the image file content)
> +#
> +# @lazy-refcounts: #optional whether to enable the lazy refcounts feature
> +# (default is taken from the image file)
> +#
> +# @pass-discard-request: #optional whether discard requests to the qcow2 device
> +# should be forwarded to the data source
> +#
> +# @pass-discard-snapshot: #optional whether discard requests for the data source
> +# should be issued when a snapshot operation (e.g.
> +# deleting a snapshot) frees clusters in the qcow2 file
> +#
> +# @pass-discard-other: #optional whether discard requests for the data source
> +# should be issued on other occasions where a cluster gets
> +# freed
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevOptionsQcow2',
> + 'base': 'BlockdevOptionsGenericCOWFormat',
> + 'data': { '*lazy-refcounts': 'bool',
> + '*pass-discard-request': 'bool',
> + '*pass-discard-snapshot': 'bool',
> + '*pass-discard-other': 'bool' } }
> +
> +##
> +# @BlockdevOptions
> +#
> +# Options for creating a block device.
> +#
> +# Since: 1.7
> +##
> +{ 'union': 'BlockdevOptions',
> + 'base': 'BlockdevOptionsBase',
> + 'discriminator': 'driver',
> + 'data': {
> + 'file': 'BlockdevOptionsFile',
> + 'http': 'BlockdevOptionsFile',
> + 'https': 'BlockdevOptionsFile',
> + 'ftp': 'BlockdevOptionsFile',
> + 'ftps': 'BlockdevOptionsFile',
> + 'tftp': 'BlockdevOptionsFile',
> +# TODO gluster: Wait for structured options
> +# TODO iscsi: Wait for structured options
> +# TODO nbd: Should take InetSocketAddress for 'host'?
> +# TODO rbd: Wait for structured options
> +# TODO sheepdog: Wait for structured options
> +# TODO ssh: Should take InetSocketAddress for 'host'?
> + 'vvfat': 'BlockdevOptionsVVFAT',
> +
> +# TODO blkdebug: Wait for structured options
> +# TODO blkverify: Wait for structured options
> +
> + 'bochs': 'BlockdevOptionsGenericFormat',
> + 'cloop': 'BlockdevOptionsGenericFormat',
> + 'cow': 'BlockdevOptionsGenericCOWFormat',
> + 'dmg': 'BlockdevOptionsGenericFormat',
> + 'parallels': 'BlockdevOptionsGenericFormat',
> + 'qcow': 'BlockdevOptionsGenericCOWFormat',
> + 'qcow2': 'BlockdevOptionsQcow2',
> + 'qed': 'BlockdevOptionsGenericCOWFormat',
> + 'raw': 'BlockdevOptionsGenericFormat',
> + 'vdi': 'BlockdevOptionsGenericFormat',
> + 'vhdx': 'BlockdevOptionsGenericFormat',
> + 'vmdk': 'BlockdevOptionsGenericCOWFormat',
> + 'vpc': 'BlockdevOptionsGenericFormat'
> + } }
> +
> +##
> +# @BlockdevRef
> +#
> +# Reference to a block device.
> +#
> +# @definition: defines a new block device inline
> +# @reference: references the ID of an existing block device. An
> +# empty string means that no block device should be
> +# referenced.
> +#
> +# Since: 1.7
> +##
> +{ 'union': 'BlockdevRef',
> + 'discriminator': {},
> + 'data': { 'definition': 'BlockdevOptions',
> + 'reference': 'str' } }
> +
> +##
> +# @blockdev-add:
> +#
> +# Creates a new block device.
> +#
> +# @options: block device options for the new device
> +#
> +# Since: 1.7
> +##
> +{ 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index b17c46e..449ecea 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3240,3 +3240,62 @@ Example:
> }
>
> EQMP
> +
> + {
> + .name = "blockdev-add",
> + .args_type = "options:q",
> + .mhandler.cmd_new = qmp_marshal_input_blockdev_add,
> + },
> +
> +SQMP
> +blockdev-add
> +------------
> +
> +Add a block device.
> +
> +Arguments:
> +
> +- "options": block driver options
> +
> +Example (1):
> +
> +-> { "execute": "blockdev-add",
> + "arguments": { "options" : { "driver": "qcow2",
> + "file": { "driver": "file",
> + "filename": "test.qcow2" } } } }
> +<- { "return": {} }
> +
> +Example (2):
> +
> +-> { "execute": "blockdev-add",
> + "arguments": {
> + "options": {
> + "driver": "qcow2",
> + "id": "my_disk",
> + "discard": "unmap",
> + "throttling": {
> + "bps-total": 1234567,
> + "iops-write": 100
> + },
> + "cache": {
> + "direct": true,
> + "writeback": true
> + },
> + "file": {
> + "driver": "file",
> + "filename": "/tmp/test.qcow2"
> + },
> + "backing": {
> + "driver": "raw",
> + "file": {
> + "driver": "file",
> + "filename": "/dev/fdset/4"
> + }
> + }
> + }
> + }
> + }
> +
> +<- { "return": {} }
> +
> +EQMP
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH 04/17] blockdev: 'blockdev-add' QMP command
2013-09-20 13:34 ` Max Reitz
@ 2013-09-20 14:57 ` Kevin Wolf
0 siblings, 0 replies; 56+ messages in thread
From: Kevin Wolf @ 2013-09-20 14:57 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel, stefanha, armbru
Am 20.09.2013 um 15:34 hat Max Reitz geschrieben:
> On 2013-09-20 13:54, Kevin Wolf wrote:
> >For examples see the changes to qmp-commands.hx.
> >
> >Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >+{ 'type': 'BlockdevThrottlingOptions',
> >+ 'data': { '*bps-total': 'int',
> >+ '*bps-read': 'int',
> >+ '*bps-write': 'int',
> >+ '*iops-total': 'int',
> >+ '*iops-read': 'int',
> >+ '*iops-write': 'int' } }
> Hm, what about burst throttling?
Good catch. They were introduced only after I started this patch.
But in fact, as we discussed on Monday, I/O throttling and copy on read
should be completely removed from the interface for now so that we can
move them to a filter driver as soon as the infrastructure for them is
there.
Kevin
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [Qemu-devel] [PATCH 04/17] blockdev: 'blockdev-add' QMP command
2013-09-20 11:54 ` [Qemu-devel] [PATCH 04/17] blockdev: 'blockdev-add' QMP command Kevin Wolf
2013-09-20 13:34 ` Max Reitz
@ 2013-09-20 14:01 ` Benoît Canet
2013-09-24 10:41 ` Paolo Bonzini
2013-09-20 15:22 ` Eric Blake
` (2 subsequent siblings)
4 siblings, 1 reply; 56+ messages in thread
From: Benoît Canet @ 2013-09-20 14:01 UTC (permalink / raw)
To: Kevin Wolf; +Cc: mreitz, qemu-devel, stefanha, armbru
Le Friday 20 Sep 2013 à 13:54:17 (+0200), Kevin Wolf a écrit :
> For examples see the changes to qmp-commands.hx.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> blockdev.c | 57 ++++++++++++
> qapi-schema.json | 270 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> qmp-commands.hx | 59 ++++++++++++
> 3 files changed, 386 insertions(+)
>
> diff --git a/blockdev.c b/blockdev.c
> index 977dc94..c4297d8 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -38,6 +38,8 @@
> #include "qemu/option.h"
> #include "qemu/config-file.h"
> #include "qapi/qmp/types.h"
> +#include "qapi-visit.h"
> +#include "qapi/qmp-output-visitor.h"
> #include "sysemu/sysemu.h"
> #include "block/block_int.h"
> #include "qmp-commands.h"
> @@ -2066,6 +2068,61 @@ void qmp_block_job_complete(const char *device, Error **errp)
> block_job_complete(job, errp);
> }
>
> +void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
> +{
> + QmpOutputVisitor *ov = qmp_output_visitor_new();
> + QObject *obj;
> + QDict *qdict;
> + DriveInfo *dinfo;
> + Error *local_err = NULL;
> +
> + /* Require an ID in the top level */
> + if (!options->has_id) {
> + error_setg(errp, "Block device needs an ID");
> + return;
> + }
> +
> + /* TODO Sort it out in raw-posix and drive_init: Reject aio=native with
> + * cache.direct=false instead of silently switching to aio=threads, except
> + * if called from drive_init.
> + *
> + * For now, simply forbidding the combination for all drivers will do. */
> + if (options->has_aio && options->aio == BLOCKDEV_A_I_O_OPTIONS_NATIVE) {
> + bool direct = options->cache->has_direct && options->cache->direct;
> + if (!options->has_cache && !direct) {
> + error_setg(errp, "aio=native requires cache.direct=true");
> + goto fail;
> + }
> + }
> +
> + visit_type_BlockdevOptions(qmp_output_get_visitor(ov),
> + &options, NULL, &local_err);
> + if (error_is_set(&local_err)) {
> + error_propagate(errp, local_err);
> + goto fail;
> + }
> +
> + obj = qmp_output_get_qobject(ov);
> + qdict = qobject_to_qdict(obj);
> +
> + qdict_flatten(qdict);
> +
> + QemuOpts *opts = qemu_opts_from_qdict(&qemu_drive_opts, qdict, &local_err);
> + if (error_is_set(&local_err)) {
> + error_propagate(errp, local_err);
> + goto fail;
There is nothing to execute between error_propagate and fail: maybe we could
get rid of the goto.
> + } else {
> + dinfo = blockdev_init(opts, IF_NONE);
> + if (!dinfo) {
> + error_setg(errp, "Could not open image");
> + goto fail;
Same.
> + }
> + }
> +
> +fail:
> + qmp_output_visitor_cleanup(ov);
> +}
> +
> static void do_qmp_query_block_jobs_one(void *opaque, BlockDriverState *bs)
> {
> BlockJobInfoList **prev = opaque;
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 145eca8..7e8ce60 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3902,3 +3902,273 @@
> ##
> { 'command': 'query-rx-filter', 'data': { '*name': 'str' },
> 'returns': ['RxFilterInfo'] }
> +
> +
> +##
> +# @BlockdevDiscardOptions
> +#
> +# Determines how to handle discard requests.
> +#
> +# @ignore: Ignore the request
> +# @unmap: Forward as an unmap request
> +#
> +# Since: 1.7
> +##
> +{ 'enum': 'BlockdevDiscardOptions',
> + 'data': [ 'ignore', 'unmap' ] }
> +
> +##
> +# @BlockdevAIOOptions
> +#
> +# Selects the AIO backend to handle I/O requests
> +#
> +# @threads: Use qemu's thread pool
> +# @native: Use native AIO backend (only Linux and Windows)
> +#
> +# Since: 1.7
> +##
> +{ 'enum': 'BlockdevAIOOptions',
> + 'data': [ 'threads', 'native' ] }
> +
> +##
> +# @BlockdevCacheOptions
> +#
> +# Includes cache-related options for block devices
> +#
> +# @writeback: #optional enables writeback mode for any caches (default: true)
> +# @direct: #optional enables use of O_DIRECT (bypass the host page cache;
> +# default: false)
> +# @no-flush: #optional ignore any flush requests for the device (default:
> +# false)
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevCacheOptions',
> + 'data': { '*writeback': 'bool',
> + '*direct': 'bool',
> + '*no-flush': 'bool' } }
> +
> +##
> +# @BlockdevThrottlingOptions
> +#
> +# Includes block device options related to I/O throttling. Leaving an option out
> +# means the same as assigning 0 and applies no throttling.
> +#
> +# @bps-total: #optional limit total bytes per second
> +# @bps-read: #optional limit read bytes per second
> +# @bps-write: #optional limit written bytes per second
> +# @iops-total: #optional limit total I/O operations per second
> +# @iops-read: #optional limit read operations per second
> +# @iops-write: #optional limit write operations per second
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevThrottlingOptions',
> + 'data': { '*bps-total': 'int',
> + '*bps-read': 'int',
> + '*bps-write': 'int',
> + '*iops-total': 'int',
> + '*iops-read': 'int',
> + '*iops-write': 'int' } }
> +
> +##
> +# @BlockdevOptionsBase
> +#
> +# Options that are available for all block devices, independent of the block
> +# driver.
> +#
> +# @driver: block driver name
> +# @id: #optional id by which the new block device can be referred to.
> +# This is a required option on the top level of blockdev-add, and
> +# currently not allowed on any other level.
> +# @discard: #optional discard-related options (default: ignore)
> +# @cache: #optional cache-related options
> +# @aio: #optional AIO backend (default: threads)
> +# @rerror: #optional how to handle read errors on the device
> +# (default: report)
> +# @werror: #optional how to handle write errors on the device
> +# (default: enospc)
> +# @throttling: #optional I/O throttling related options
> +# @read-only: #optional whether the block device should be read-only
> +# (default: false)
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevOptionsBase',
> + 'data': { 'driver': 'str',
> + '*id': 'str',
> + '*discard': 'BlockdevDiscardOptions',
> + '*cache': 'BlockdevCacheOptions',
> + '*aio': 'BlockdevAIOOptions',
> + '*rerror': 'BlockdevOnError',
> + '*werror': 'BlockdevOnError',
> + '*throttling': 'BlockdevThrottlingOptions',
> + '*read-only': 'bool' } }
> +
> +##
> +# @BlockdevOptionsFile
> +#
> +# Driver specific block device options for the file backend and similar
> +# protocols.
> +#
> +# @filename: path to the image file
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevOptionsFile',
> + 'data': { 'filename': 'str' } }
> +
> +##
> +# @BlockdevOptionsVVFAT
> +#
> +# Driver specific block device options for the vvfat protocol.
> +#
> +# @dir: directory to be exported as FAT image
> +# @fat-type: #optional FAT type: 12, 16 or 32
> +# @floppy: #optional whether to export a floppy imae (true) or partitioned
> +# hard disk (false; default)
> +# @rw: #optional whether to allow write operations (default: false)
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevOptionsVVFAT',
> + 'data': { 'dir': 'str', '*fat-type': 'int', '*floppy': 'bool',
> + '*rw': 'bool' } }
> +
> +##
> +# @BlockdevOptionsGenericFormat
> +#
> +# Driver specific block device options for image format that have no option
> +# besides their data source.
> +#
> +# @file: reference to or definition of the data source block device
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevOptionsGenericFormat',
> + 'data': { 'file': 'BlockdevRef' } }
> +
> +##
> +# @BlockdevOptionsGenericCOWFormat
> +#
> +# Driver specific block device options for image format that have no option
> +# besides their data source and an optional backing file.
> +#
> +# @file: reference to or definition of the data source block device
> +# @backing: #optional reference to or definition of the backing file block
> +# device (if missing, taken from the image file content). It is
> +# allowed to pass an empty string here in order to disable the
> +# default backing file.
> +# @copy-on-read: #optional whether to enable copy on read for the device
> +# (default: false). Copy on read can only be used if the
> +# image is not read-only.
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevOptionsGenericCOWFormat',
> + 'base': 'BlockdevOptionsGenericFormat',
> + 'data': { '*backing': 'BlockdevRef',
> + '*copy-on-read': 'bool' } }
> +##
> +# @BlockdevOptionsQcow2
> +#
> +# Driver specific block device options for qcow2.
> +#
> +# @file: reference to or definition of the data source block device
> +#
> +# @backing: #optional reference to or definition of the backing file block
> +# device (if missing, taken from the image file content)
> +#
> +# @lazy-refcounts: #optional whether to enable the lazy refcounts feature
> +# (default is taken from the image file)
> +#
> +# @pass-discard-request: #optional whether discard requests to the qcow2 device
> +# should be forwarded to the data source
> +#
> +# @pass-discard-snapshot: #optional whether discard requests for the data source
> +# should be issued when a snapshot operation (e.g.
> +# deleting a snapshot) frees clusters in the qcow2 file
> +#
> +# @pass-discard-other: #optional whether discard requests for the data source
> +# should be issued on other occasions where a cluster gets
> +# freed
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevOptionsQcow2',
> + 'base': 'BlockdevOptionsGenericCOWFormat',
> + 'data': { '*lazy-refcounts': 'bool',
> + '*pass-discard-request': 'bool',
> + '*pass-discard-snapshot': 'bool',
> + '*pass-discard-other': 'bool' } }
> +
> +##
> +# @BlockdevOptions
> +#
> +# Options for creating a block device.
> +#
> +# Since: 1.7
> +##
> +{ 'union': 'BlockdevOptions',
> + 'base': 'BlockdevOptionsBase',
> + 'discriminator': 'driver',
> + 'data': {
> + 'file': 'BlockdevOptionsFile',
> + 'http': 'BlockdevOptionsFile',
> + 'https': 'BlockdevOptionsFile',
> + 'ftp': 'BlockdevOptionsFile',
> + 'ftps': 'BlockdevOptionsFile',
> + 'tftp': 'BlockdevOptionsFile',
> +# TODO gluster: Wait for structured options
> +# TODO iscsi: Wait for structured options
> +# TODO nbd: Should take InetSocketAddress for 'host'?
> +# TODO rbd: Wait for structured options
> +# TODO sheepdog: Wait for structured options
> +# TODO ssh: Should take InetSocketAddress for 'host'?
> + 'vvfat': 'BlockdevOptionsVVFAT',
> +
> +# TODO blkdebug: Wait for structured options
> +# TODO blkverify: Wait for structured options
> +
> + 'bochs': 'BlockdevOptionsGenericFormat',
> + 'cloop': 'BlockdevOptionsGenericFormat',
> + 'cow': 'BlockdevOptionsGenericCOWFormat',
> + 'dmg': 'BlockdevOptionsGenericFormat',
> + 'parallels': 'BlockdevOptionsGenericFormat',
> + 'qcow': 'BlockdevOptionsGenericCOWFormat',
> + 'qcow2': 'BlockdevOptionsQcow2',
> + 'qed': 'BlockdevOptionsGenericCOWFormat',
> + 'raw': 'BlockdevOptionsGenericFormat',
> + 'vdi': 'BlockdevOptionsGenericFormat',
> + 'vhdx': 'BlockdevOptionsGenericFormat',
> + 'vmdk': 'BlockdevOptionsGenericCOWFormat',
> + 'vpc': 'BlockdevOptionsGenericFormat'
> + } }
> +
> +##
> +# @BlockdevRef
> +#
> +# Reference to a block device.
> +#
> +# @definition: defines a new block device inline
> +# @reference: references the ID of an existing block device. An
> +# empty string means that no block device should be
> +# referenced.
> +#
> +# Since: 1.7
> +##
> +{ 'union': 'BlockdevRef',
> + 'discriminator': {},
> + 'data': { 'definition': 'BlockdevOptions',
> + 'reference': 'str' } }
> +
> +##
> +# @blockdev-add:
> +#
> +# Creates a new block device.
> +#
> +# @options: block device options for the new device
> +#
> +# Since: 1.7
> +##
> +{ 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index b17c46e..449ecea 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3240,3 +3240,62 @@ Example:
> }
>
> EQMP
> +
> + {
> + .name = "blockdev-add",
> + .args_type = "options:q",
> + .mhandler.cmd_new = qmp_marshal_input_blockdev_add,
> + },
> +
> +SQMP
> +blockdev-add
> +------------
> +
> +Add a block device.
> +
> +Arguments:
> +
> +- "options": block driver options
> +
> +Example (1):
> +
> +-> { "execute": "blockdev-add",
> + "arguments": { "options" : { "driver": "qcow2",
> + "file": { "driver": "file",
> + "filename": "test.qcow2" } } } }
> +<- { "return": {} }
> +
> +Example (2):
> +
> +-> { "execute": "blockdev-add",
> + "arguments": {
> + "options": {
> + "driver": "qcow2",
> + "id": "my_disk",
> + "discard": "unmap",
> + "throttling": {
> + "bps-total": 1234567,
> + "iops-write": 100
> + },
> + "cache": {
> + "direct": true,
> + "writeback": true
> + },
> + "file": {
> + "driver": "file",
> + "filename": "/tmp/test.qcow2"
> + },
> + "backing": {
> + "driver": "raw",
> + "file": {
> + "driver": "file",
> + "filename": "/dev/fdset/4"
> + }
> + }
> + }
> + }
> + }
> +
> +<- { "return": {} }
> +
> +EQMP
> --
> 1.8.1.4
>
>
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH 04/17] blockdev: 'blockdev-add' QMP command
2013-09-20 14:01 ` Benoît Canet
@ 2013-09-24 10:41 ` Paolo Bonzini
2013-09-24 11:10 ` Kevin Wolf
0 siblings, 1 reply; 56+ messages in thread
From: Paolo Bonzini @ 2013-09-24 10:41 UTC (permalink / raw)
To: Benoît Canet; +Cc: Kevin Wolf, armbru, qemu-devel, stefanha, mreitz
Il 20/09/2013 16:01, Benoît Canet ha scritto:
>> > + if (error_is_set(&local_err)) {
>> > + error_propagate(errp, local_err);
>> > + goto fail;
> There is nothing to execute between error_propagate and fail: maybe we could
> get rid of the goto.
Or perhaps of the "else".
Paolo
>> > + } else {
>> > + dinfo = blockdev_init(opts, IF_NONE);
>> > + if (!dinfo) {
>> > + error_setg(errp, "Could not open image");
>> > + goto fail;
> Same.
>> > + }
>> > + }
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH 04/17] blockdev: 'blockdev-add' QMP command
2013-09-24 10:41 ` Paolo Bonzini
@ 2013-09-24 11:10 ` Kevin Wolf
0 siblings, 0 replies; 56+ messages in thread
From: Kevin Wolf @ 2013-09-24 11:10 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Benoît Canet, armbru, qemu-devel, stefanha, mreitz
Am 24.09.2013 um 12:41 hat Paolo Bonzini geschrieben:
> Il 20/09/2013 16:01, Benoît Canet ha scritto:
> >> > + if (error_is_set(&local_err)) {
> >> > + error_propagate(errp, local_err);
> >> > + goto fail;
> > There is nothing to execute between error_propagate and fail: maybe we could
> > get rid of the goto.
>
> Or perhaps of the "else".
Indeed, that looks a bit strange now... It doesn't survive until the end
of the series anyway, but I can remove it here. (The else branch has a
useless goto as well, but I'd prefer to leave it there in case new code
is added later - it's easy to miss when a goto becomes necessary.)
Kevin
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [Qemu-devel] [PATCH 04/17] blockdev: 'blockdev-add' QMP command
2013-09-20 11:54 ` [Qemu-devel] [PATCH 04/17] blockdev: 'blockdev-add' QMP command Kevin Wolf
2013-09-20 13:34 ` Max Reitz
2013-09-20 14:01 ` Benoît Canet
@ 2013-09-20 15:22 ` Eric Blake
2013-09-20 15:34 ` Kevin Wolf
2013-09-24 5:18 ` Fam Zheng
2013-09-24 10:39 ` Paolo Bonzini
4 siblings, 1 reply; 56+ messages in thread
From: Eric Blake @ 2013-09-20 15:22 UTC (permalink / raw)
To: Kevin Wolf; +Cc: mreitz, qemu-devel, stefanha, armbru
[-- Attachment #1: Type: text/plain, Size: 3639 bytes --]
On 09/20/2013 05:54 AM, Kevin Wolf wrote:
> For examples see the changes to qmp-commands.hx.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> blockdev.c | 57 ++++++++++++
> qapi-schema.json | 270 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> qmp-commands.hx | 59 ++++++++++++
> 3 files changed, 386 insertions(+)
>
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevOptionsBase',
> + 'data': { 'driver': 'str',
> + '*id': 'str',
> + '*discard': 'BlockdevDiscardOptions',
> + '*cache': 'BlockdevCacheOptions',
> + '*aio': 'BlockdevAIOOptions',
Is the double space intentional? Harmless, but inconsistent.
> + '*rerror': 'BlockdevOnError',
> + '*werror': 'BlockdevOnError',
> + '*throttling': 'BlockdevThrottlingOptions',
> + '*read-only': 'bool' } }
...
> +##
> +# @BlockdevOptionsVVFAT
> +#
> +# Driver specific block device options for the vvfat protocol.
> +#
> +# @dir: directory to be exported as FAT image
> +# @fat-type: #optional FAT type: 12, 16 or 32
> +# @floppy: #optional whether to export a floppy imae (true) or partitioned
> +# hard disk (false; default)
> +# @rw: #optional whether to allow write operations (default: false)
Why do we have 'read-only' in base, and 'rw' in vvfat? It feels like
the vvfat option is redundant.
> +##
> +# @BlockdevOptionsGenericCOWFormat
> +#
> +# Driver specific block device options for image format that have no option
> +# besides their data source and an optional backing file.
> +#
> +# @file: reference to or definition of the data source block device
Do you need to document this field...
> +# @backing: #optional reference to or definition of the backing file block
> +# device (if missing, taken from the image file content). It is
> +# allowed to pass an empty string here in order to disable the
> +# default backing file.
> +# @copy-on-read: #optional whether to enable copy on read for the device
> +# (default: false). Copy on read can only be used if the
> +# image is not read-only.
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevOptionsGenericCOWFormat',
> + 'base': 'BlockdevOptionsGenericFormat',
> + 'data': { '*backing': 'BlockdevRef',
> + '*copy-on-read': 'bool' } }
...given that it is only present by inheritence?
> +##
> +# @BlockdevOptionsQcow2
> +#
> +# Driver specific block device options for qcow2.
> +#
> +# @file: reference to or definition of the data source block device
> +#
> +# @backing: #optional reference to or definition of the backing file block
> +# device (if missing, taken from the image file content)
Same question. If you DO document inherited fields, you missed
@copy-on-read; if you DON'T document inherited fields, these two aren't
necessary (instead, you could just have some statement about: "In
addition to the fields documented in BlockdevOptionsGenericCOWFormat,
this struct includes:")
> +##
> +# @blockdev-add:
> +#
> +# Creates a new block device.
> +#
> +# @options: block device options for the new device
> +#
> +# Since: 1.7
> +##
> +{ 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
So deceptively simple :) All the real work is in the rest of the
(cool!) enhancements to the qapi type generator.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH 04/17] blockdev: 'blockdev-add' QMP command
2013-09-20 15:22 ` Eric Blake
@ 2013-09-20 15:34 ` Kevin Wolf
0 siblings, 0 replies; 56+ messages in thread
From: Kevin Wolf @ 2013-09-20 15:34 UTC (permalink / raw)
To: Eric Blake; +Cc: mreitz, qemu-devel, stefanha, armbru
Am 20.09.2013 um 17:22 hat Eric Blake geschrieben:
> On 09/20/2013 05:54 AM, Kevin Wolf wrote:
> > For examples see the changes to qmp-commands.hx.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > blockdev.c | 57 ++++++++++++
> > qapi-schema.json | 270 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > qmp-commands.hx | 59 ++++++++++++
> > 3 files changed, 386 insertions(+)
> >
>
>
> > +# Since: 1.7
> > +##
> > +{ 'type': 'BlockdevOptionsBase',
> > + 'data': { 'driver': 'str',
> > + '*id': 'str',
> > + '*discard': 'BlockdevDiscardOptions',
> > + '*cache': 'BlockdevCacheOptions',
> > + '*aio': 'BlockdevAIOOptions',
>
> Is the double space intentional? Harmless, but inconsistent.
No, I'll fix it.
> > + '*rerror': 'BlockdevOnError',
> > + '*werror': 'BlockdevOnError',
> > + '*throttling': 'BlockdevThrottlingOptions',
> > + '*read-only': 'bool' } }
> ...
>
> > +##
> > +# @BlockdevOptionsVVFAT
> > +#
> > +# Driver specific block device options for the vvfat protocol.
> > +#
> > +# @dir: directory to be exported as FAT image
> > +# @fat-type: #optional FAT type: 12, 16 or 32
> > +# @floppy: #optional whether to export a floppy imae (true) or partitioned
> > +# hard disk (false; default)
> > +# @rw: #optional whether to allow write operations (default: false)
>
> Why do we have 'read-only' in base, and 'rw' in vvfat? It feels like
> the vvfat option is redundant.
I guess it is kind of redundant. The reason for it is that it's always
been there, encoded in the filename as "fat:rw:..." - and the reason for
that is that read-write vvfat is even more unreliable than read-only
vvfat. So we have a read-only=false default in base, and a
read-only=true default in vvfat.
I'm not sure if changing this without breaking the command line
is possible; but if it is, it involves vvfat-specific magic in
drive_init(), which this simply isn't worth.
> > +##
> > +# @BlockdevOptionsGenericCOWFormat
> > +#
> > +# Driver specific block device options for image format that have no option
> > +# besides their data source and an optional backing file.
> > +#
> > +# @file: reference to or definition of the data source block device
>
> Do you need to document this field...
>
> > +# @backing: #optional reference to or definition of the backing file block
> > +# device (if missing, taken from the image file content). It is
> > +# allowed to pass an empty string here in order to disable the
> > +# default backing file.
> > +# @copy-on-read: #optional whether to enable copy on read for the device
> > +# (default: false). Copy on read can only be used if the
> > +# image is not read-only.
> > +#
> > +# Since: 1.7
> > +##
> > +{ 'type': 'BlockdevOptionsGenericCOWFormat',
> > + 'base': 'BlockdevOptionsGenericFormat',
> > + 'data': { '*backing': 'BlockdevRef',
> > + '*copy-on-read': 'bool' } }
>
> ...given that it is only present by inheritence?
>
> > +##
> > +# @BlockdevOptionsQcow2
> > +#
> > +# Driver specific block device options for qcow2.
> > +#
> > +# @file: reference to or definition of the data source block device
> > +#
> > +# @backing: #optional reference to or definition of the backing file block
> > +# device (if missing, taken from the image file content)
>
> Same question. If you DO document inherited fields, you missed
> @copy-on-read; if you DON'T document inherited fields, these two aren't
> necessary (instead, you could just have some statement about: "In
> addition to the fields documented in BlockdevOptionsGenericCOWFormat,
> this struct includes:")
Nope, I don't think they are necessary. I just wasn't careful enough
when I added the inheritance. I'll drop them.
Kevin
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [Qemu-devel] [PATCH 04/17] blockdev: 'blockdev-add' QMP command
2013-09-20 11:54 ` [Qemu-devel] [PATCH 04/17] blockdev: 'blockdev-add' QMP command Kevin Wolf
` (2 preceding siblings ...)
2013-09-20 15:22 ` Eric Blake
@ 2013-09-24 5:18 ` Fam Zheng
2013-09-24 8:01 ` Kevin Wolf
2013-09-24 10:39 ` Paolo Bonzini
4 siblings, 1 reply; 56+ messages in thread
From: Fam Zheng @ 2013-09-24 5:18 UTC (permalink / raw)
To: Kevin Wolf; +Cc: mreitz, qemu-devel, stefanha, armbru
On Fri, 09/20 13:54, Kevin Wolf wrote:
> For examples see the changes to qmp-commands.hx.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> blockdev.c | 57 ++++++++++++
> qapi-schema.json | 270 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> qmp-commands.hx | 59 ++++++++++++
> 3 files changed, 386 insertions(+)
>
> diff --git a/blockdev.c b/blockdev.c
> index 977dc94..c4297d8 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -38,6 +38,8 @@
> #include "qemu/option.h"
> #include "qemu/config-file.h"
> #include "qapi/qmp/types.h"
> +#include "qapi-visit.h"
> +#include "qapi/qmp-output-visitor.h"
> #include "sysemu/sysemu.h"
> #include "block/block_int.h"
> #include "qmp-commands.h"
> @@ -2066,6 +2068,61 @@ void qmp_block_job_complete(const char *device, Error **errp)
> block_job_complete(job, errp);
> }
>
> +void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
> +{
> + QmpOutputVisitor *ov = qmp_output_visitor_new();
> + QObject *obj;
> + QDict *qdict;
> + DriveInfo *dinfo;
> + Error *local_err = NULL;
> +
> + /* Require an ID in the top level */
> + if (!options->has_id) {
> + error_setg(errp, "Block device needs an ID");
> + return;
> + }
> +
> + /* TODO Sort it out in raw-posix and drive_init: Reject aio=native with
> + * cache.direct=false instead of silently switching to aio=threads, except
> + * if called from drive_init.
> + *
> + * For now, simply forbidding the combination for all drivers will do. */
> + if (options->has_aio && options->aio == BLOCKDEV_A_I_O_OPTIONS_NATIVE) {
> + bool direct = options->cache->has_direct && options->cache->direct;
> + if (!options->has_cache && !direct) {
> + error_setg(errp, "aio=native requires cache.direct=true");
> + goto fail;
> + }
> + }
> +
> + visit_type_BlockdevOptions(qmp_output_get_visitor(ov),
> + &options, NULL, &local_err);
> + if (error_is_set(&local_err)) {
> + error_propagate(errp, local_err);
> + goto fail;
> + }
> +
> + obj = qmp_output_get_qobject(ov);
> + qdict = qobject_to_qdict(obj);
> +
> + qdict_flatten(qdict);
> +
> + QemuOpts *opts = qemu_opts_from_qdict(&qemu_drive_opts, qdict, &local_err);
> + if (error_is_set(&local_err)) {
> + error_propagate(errp, local_err);
> + goto fail;
> + } else {
> + dinfo = blockdev_init(opts, IF_NONE);
> + if (!dinfo) {
> + error_setg(errp, "Could not open image");
> + goto fail;
> + }
> + }
> +
> +fail:
> + qmp_output_visitor_cleanup(ov);
> +}
> +
> static void do_qmp_query_block_jobs_one(void *opaque, BlockDriverState *bs)
> {
> BlockJobInfoList **prev = opaque;
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 145eca8..7e8ce60 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3902,3 +3902,273 @@
> ##
> { 'command': 'query-rx-filter', 'data': { '*name': 'str' },
> 'returns': ['RxFilterInfo'] }
> +
> +
> +##
> +# @BlockdevDiscardOptions
> +#
> +# Determines how to handle discard requests.
> +#
> +# @ignore: Ignore the request
> +# @unmap: Forward as an unmap request
> +#
> +# Since: 1.7
> +##
> +{ 'enum': 'BlockdevDiscardOptions',
> + 'data': [ 'ignore', 'unmap' ] }
> +
> +##
> +# @BlockdevAIOOptions
> +#
> +# Selects the AIO backend to handle I/O requests
> +#
> +# @threads: Use qemu's thread pool
> +# @native: Use native AIO backend (only Linux and Windows)
> +#
> +# Since: 1.7
> +##
> +{ 'enum': 'BlockdevAIOOptions',
> + 'data': [ 'threads', 'native' ] }
> +
> +##
> +# @BlockdevCacheOptions
> +#
> +# Includes cache-related options for block devices
> +#
> +# @writeback: #optional enables writeback mode for any caches (default: true)
> +# @direct: #optional enables use of O_DIRECT (bypass the host page cache;
> +# default: false)
> +# @no-flush: #optional ignore any flush requests for the device (default:
> +# false)
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevCacheOptions',
> + 'data': { '*writeback': 'bool',
> + '*direct': 'bool',
> + '*no-flush': 'bool' } }
> +
> +##
> +# @BlockdevThrottlingOptions
> +#
> +# Includes block device options related to I/O throttling. Leaving an option out
> +# means the same as assigning 0 and applies no throttling.
> +#
> +# @bps-total: #optional limit total bytes per second
> +# @bps-read: #optional limit read bytes per second
> +# @bps-write: #optional limit written bytes per second
> +# @iops-total: #optional limit total I/O operations per second
> +# @iops-read: #optional limit read operations per second
> +# @iops-write: #optional limit write operations per second
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevThrottlingOptions',
> + 'data': { '*bps-total': 'int',
> + '*bps-read': 'int',
> + '*bps-write': 'int',
> + '*iops-total': 'int',
> + '*iops-read': 'int',
> + '*iops-write': 'int' } }
> +
> +##
> +# @BlockdevOptionsBase
> +#
> +# Options that are available for all block devices, independent of the block
> +# driver.
> +#
> +# @driver: block driver name
> +# @id: #optional id by which the new block device can be referred to.
> +# This is a required option on the top level of blockdev-add, and
> +# currently not allowed on any other level.
> +# @discard: #optional discard-related options (default: ignore)
> +# @cache: #optional cache-related options
> +# @aio: #optional AIO backend (default: threads)
> +# @rerror: #optional how to handle read errors on the device
> +# (default: report)
> +# @werror: #optional how to handle write errors on the device
> +# (default: enospc)
> +# @throttling: #optional I/O throttling related options
> +# @read-only: #optional whether the block device should be read-only
> +# (default: false)
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevOptionsBase',
> + 'data': { 'driver': 'str',
> + '*id': 'str',
> + '*discard': 'BlockdevDiscardOptions',
> + '*cache': 'BlockdevCacheOptions',
> + '*aio': 'BlockdevAIOOptions',
> + '*rerror': 'BlockdevOnError',
> + '*werror': 'BlockdevOnError',
> + '*throttling': 'BlockdevThrottlingOptions',
> + '*read-only': 'bool' } }
> +
> +##
> +# @BlockdevOptionsFile
> +#
> +# Driver specific block device options for the file backend and similar
> +# protocols.
> +#
> +# @filename: path to the image file
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevOptionsFile',
> + 'data': { 'filename': 'str' } }
> +
> +##
> +# @BlockdevOptionsVVFAT
> +#
> +# Driver specific block device options for the vvfat protocol.
> +#
> +# @dir: directory to be exported as FAT image
> +# @fat-type: #optional FAT type: 12, 16 or 32
> +# @floppy: #optional whether to export a floppy imae (true) or partitioned
> +# hard disk (false; default)
> +# @rw: #optional whether to allow write operations (default: false)
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevOptionsVVFAT',
> + 'data': { 'dir': 'str', '*fat-type': 'int', '*floppy': 'bool',
> + '*rw': 'bool' } }
> +
> +##
> +# @BlockdevOptionsGenericFormat
> +#
> +# Driver specific block device options for image format that have no option
> +# besides their data source.
> +#
> +# @file: reference to or definition of the data source block device
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevOptionsGenericFormat',
> + 'data': { 'file': 'BlockdevRef' } }
> +
> +##
> +# @BlockdevOptionsGenericCOWFormat
> +#
> +# Driver specific block device options for image format that have no option
> +# besides their data source and an optional backing file.
> +#
> +# @file: reference to or definition of the data source block device
> +# @backing: #optional reference to or definition of the backing file block
> +# device (if missing, taken from the image file content). It is
> +# allowed to pass an empty string here in order to disable the
> +# default backing file.
> +# @copy-on-read: #optional whether to enable copy on read for the device
> +# (default: false). Copy on read can only be used if the
> +# image is not read-only.
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevOptionsGenericCOWFormat',
> + 'base': 'BlockdevOptionsGenericFormat',
> + 'data': { '*backing': 'BlockdevRef',
> + '*copy-on-read': 'bool' } }
> +##
> +# @BlockdevOptionsQcow2
> +#
> +# Driver specific block device options for qcow2.
> +#
> +# @file: reference to or definition of the data source block device
> +#
> +# @backing: #optional reference to or definition of the backing file block
> +# device (if missing, taken from the image file content)
> +#
> +# @lazy-refcounts: #optional whether to enable the lazy refcounts feature
> +# (default is taken from the image file)
> +#
> +# @pass-discard-request: #optional whether discard requests to the qcow2 device
> +# should be forwarded to the data source
> +#
> +# @pass-discard-snapshot: #optional whether discard requests for the data source
> +# should be issued when a snapshot operation (e.g.
> +# deleting a snapshot) frees clusters in the qcow2 file
> +#
> +# @pass-discard-other: #optional whether discard requests for the data source
> +# should be issued on other occasions where a cluster gets
> +# freed
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevOptionsQcow2',
> + 'base': 'BlockdevOptionsGenericCOWFormat',
> + 'data': { '*lazy-refcounts': 'bool',
> + '*pass-discard-request': 'bool',
> + '*pass-discard-snapshot': 'bool',
> + '*pass-discard-other': 'bool' } }
> +
> +##
> +# @BlockdevOptions
> +#
> +# Options for creating a block device.
> +#
> +# Since: 1.7
> +##
> +{ 'union': 'BlockdevOptions',
> + 'base': 'BlockdevOptionsBase',
> + 'discriminator': 'driver',
> + 'data': {
> + 'file': 'BlockdevOptionsFile',
> + 'http': 'BlockdevOptionsFile',
> + 'https': 'BlockdevOptionsFile',
> + 'ftp': 'BlockdevOptionsFile',
> + 'ftps': 'BlockdevOptionsFile',
> + 'tftp': 'BlockdevOptionsFile',
> +# TODO gluster: Wait for structured options
> +# TODO iscsi: Wait for structured options
> +# TODO nbd: Should take InetSocketAddress for 'host'?
> +# TODO rbd: Wait for structured options
> +# TODO sheepdog: Wait for structured options
> +# TODO ssh: Should take InetSocketAddress for 'host'?
> + 'vvfat': 'BlockdevOptionsVVFAT',
> +
> +# TODO blkdebug: Wait for structured options
> +# TODO blkverify: Wait for structured options
> +
> + 'bochs': 'BlockdevOptionsGenericFormat',
> + 'cloop': 'BlockdevOptionsGenericFormat',
> + 'cow': 'BlockdevOptionsGenericCOWFormat',
> + 'dmg': 'BlockdevOptionsGenericFormat',
> + 'parallels': 'BlockdevOptionsGenericFormat',
> + 'qcow': 'BlockdevOptionsGenericCOWFormat',
> + 'qcow2': 'BlockdevOptionsQcow2',
> + 'qed': 'BlockdevOptionsGenericCOWFormat',
> + 'raw': 'BlockdevOptionsGenericFormat',
> + 'vdi': 'BlockdevOptionsGenericFormat',
> + 'vhdx': 'BlockdevOptionsGenericFormat',
> + 'vmdk': 'BlockdevOptionsGenericCOWFormat',
> + 'vpc': 'BlockdevOptionsGenericFormat'
> + } }
> +
> +##
> +# @BlockdevRef
> +#
> +# Reference to a block device.
> +#
> +# @definition: defines a new block device inline
> +# @reference: references the ID of an existing block device. An
> +# empty string means that no block device should be
> +# referenced.
> +#
> +# Since: 1.7
> +##
> +{ 'union': 'BlockdevRef',
> + 'discriminator': {},
> + 'data': { 'definition': 'BlockdevOptions',
> + 'reference': 'str' } }
> +
> +##
> +# @blockdev-add:
> +#
> +# Creates a new block device.
> +#
> +# @options: block device options for the new device
> +#
> +# Since: 1.7
> +##
> +{ 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index b17c46e..449ecea 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3240,3 +3240,62 @@ Example:
> }
>
> EQMP
> +
> + {
> + .name = "blockdev-add",
> + .args_type = "options:q",
> + .mhandler.cmd_new = qmp_marshal_input_blockdev_add,
> + },
> +
> +SQMP
> +blockdev-add
> +------------
> +
> +Add a block device.
> +
> +Arguments:
> +
> +- "options": block driver options
> +
> +Example (1):
> +
> +-> { "execute": "blockdev-add",
> + "arguments": { "options" : { "driver": "qcow2",
> + "file": { "driver": "file",
> + "filename": "test.qcow2" } } } }
> +<- { "return": {} }
> +
> +Example (2):
> +
> +-> { "execute": "blockdev-add",
> + "arguments": {
> + "options": {
> + "driver": "qcow2",
> + "id": "my_disk",
> + "discard": "unmap",
> + "throttling": {
> + "bps-total": 1234567,
> + "iops-write": 100
> + },
> + "cache": {
> + "direct": true,
> + "writeback": true
> + },
> + "file": {
> + "driver": "file",
> + "filename": "/tmp/test.qcow2"
> + },
> + "backing": {
> + "driver": "raw",
> + "file": {
> + "driver": "file",
> + "filename": "/dev/fdset/4"
> + }
> + }
How will BDS reference be used? Like this?
"backing": "ide0-hd0",
[As I tested this is not supported with this series, the option "backing" is
not parsed in bdrv_open(). Asking because image fleecing needs this, is it
already in your todo list?]
Thanks,
Fam
> + }
> + }
> + }
> +
> +<- { "return": {} }
> +
> +EQMP
> --
> 1.8.1.4
>
>
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH 04/17] blockdev: 'blockdev-add' QMP command
2013-09-24 5:18 ` Fam Zheng
@ 2013-09-24 8:01 ` Kevin Wolf
0 siblings, 0 replies; 56+ messages in thread
From: Kevin Wolf @ 2013-09-24 8:01 UTC (permalink / raw)
To: Fam Zheng; +Cc: mreitz, qemu-devel, stefanha, armbru
Am 24.09.2013 um 07:18 hat Fam Zheng geschrieben:
> On Fri, 09/20 13:54, Kevin Wolf wrote:
> > For examples see the changes to qmp-commands.hx.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > blockdev.c | 57 ++++++++++++
> > qapi-schema.json | 270 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > qmp-commands.hx | 59 ++++++++++++
> > 3 files changed, 386 insertions(+)
> >
> > diff --git a/blockdev.c b/blockdev.c
> > index 977dc94..c4297d8 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -38,6 +38,8 @@
> > #include "qemu/option.h"
> > #include "qemu/config-file.h"
> > #include "qapi/qmp/types.h"
> > +#include "qapi-visit.h"
> > +#include "qapi/qmp-output-visitor.h"
> > #include "sysemu/sysemu.h"
> > #include "block/block_int.h"
> > #include "qmp-commands.h"
> > @@ -2066,6 +2068,61 @@ void qmp_block_job_complete(const char *device, Error **errp)
> > block_job_complete(job, errp);
> > }
> >
> > +void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
> > +{
> > + QmpOutputVisitor *ov = qmp_output_visitor_new();
> > + QObject *obj;
> > + QDict *qdict;
> > + DriveInfo *dinfo;
> > + Error *local_err = NULL;
> > +
> > + /* Require an ID in the top level */
> > + if (!options->has_id) {
> > + error_setg(errp, "Block device needs an ID");
> > + return;
> > + }
> > +
> > + /* TODO Sort it out in raw-posix and drive_init: Reject aio=native with
> > + * cache.direct=false instead of silently switching to aio=threads, except
> > + * if called from drive_init.
> > + *
> > + * For now, simply forbidding the combination for all drivers will do. */
> > + if (options->has_aio && options->aio == BLOCKDEV_A_I_O_OPTIONS_NATIVE) {
> > + bool direct = options->cache->has_direct && options->cache->direct;
> > + if (!options->has_cache && !direct) {
> > + error_setg(errp, "aio=native requires cache.direct=true");
> > + goto fail;
> > + }
> > + }
> > +
> > + visit_type_BlockdevOptions(qmp_output_get_visitor(ov),
> > + &options, NULL, &local_err);
> > + if (error_is_set(&local_err)) {
> > + error_propagate(errp, local_err);
> > + goto fail;
> > + }
> > +
> > + obj = qmp_output_get_qobject(ov);
> > + qdict = qobject_to_qdict(obj);
> > +
> > + qdict_flatten(qdict);
> > +
> > + QemuOpts *opts = qemu_opts_from_qdict(&qemu_drive_opts, qdict, &local_err);
> > + if (error_is_set(&local_err)) {
> > + error_propagate(errp, local_err);
> > + goto fail;
> > + } else {
> > + dinfo = blockdev_init(opts, IF_NONE);
> > + if (!dinfo) {
> > + error_setg(errp, "Could not open image");
> > + goto fail;
> > + }
> > + }
> > +
> > +fail:
> > + qmp_output_visitor_cleanup(ov);
> > +}
> > +
> > static void do_qmp_query_block_jobs_one(void *opaque, BlockDriverState *bs)
> > {
> > BlockJobInfoList **prev = opaque;
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 145eca8..7e8ce60 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -3902,3 +3902,273 @@
> > ##
> > { 'command': 'query-rx-filter', 'data': { '*name': 'str' },
> > 'returns': ['RxFilterInfo'] }
> > +
> > +
> > +##
> > +# @BlockdevDiscardOptions
> > +#
> > +# Determines how to handle discard requests.
> > +#
> > +# @ignore: Ignore the request
> > +# @unmap: Forward as an unmap request
> > +#
> > +# Since: 1.7
> > +##
> > +{ 'enum': 'BlockdevDiscardOptions',
> > + 'data': [ 'ignore', 'unmap' ] }
> > +
> > +##
> > +# @BlockdevAIOOptions
> > +#
> > +# Selects the AIO backend to handle I/O requests
> > +#
> > +# @threads: Use qemu's thread pool
> > +# @native: Use native AIO backend (only Linux and Windows)
> > +#
> > +# Since: 1.7
> > +##
> > +{ 'enum': 'BlockdevAIOOptions',
> > + 'data': [ 'threads', 'native' ] }
> > +
> > +##
> > +# @BlockdevCacheOptions
> > +#
> > +# Includes cache-related options for block devices
> > +#
> > +# @writeback: #optional enables writeback mode for any caches (default: true)
> > +# @direct: #optional enables use of O_DIRECT (bypass the host page cache;
> > +# default: false)
> > +# @no-flush: #optional ignore any flush requests for the device (default:
> > +# false)
> > +#
> > +# Since: 1.7
> > +##
> > +{ 'type': 'BlockdevCacheOptions',
> > + 'data': { '*writeback': 'bool',
> > + '*direct': 'bool',
> > + '*no-flush': 'bool' } }
> > +
> > +##
> > +# @BlockdevThrottlingOptions
> > +#
> > +# Includes block device options related to I/O throttling. Leaving an option out
> > +# means the same as assigning 0 and applies no throttling.
> > +#
> > +# @bps-total: #optional limit total bytes per second
> > +# @bps-read: #optional limit read bytes per second
> > +# @bps-write: #optional limit written bytes per second
> > +# @iops-total: #optional limit total I/O operations per second
> > +# @iops-read: #optional limit read operations per second
> > +# @iops-write: #optional limit write operations per second
> > +#
> > +# Since: 1.7
> > +##
> > +{ 'type': 'BlockdevThrottlingOptions',
> > + 'data': { '*bps-total': 'int',
> > + '*bps-read': 'int',
> > + '*bps-write': 'int',
> > + '*iops-total': 'int',
> > + '*iops-read': 'int',
> > + '*iops-write': 'int' } }
> > +
> > +##
> > +# @BlockdevOptionsBase
> > +#
> > +# Options that are available for all block devices, independent of the block
> > +# driver.
> > +#
> > +# @driver: block driver name
> > +# @id: #optional id by which the new block device can be referred to.
> > +# This is a required option on the top level of blockdev-add, and
> > +# currently not allowed on any other level.
> > +# @discard: #optional discard-related options (default: ignore)
> > +# @cache: #optional cache-related options
> > +# @aio: #optional AIO backend (default: threads)
> > +# @rerror: #optional how to handle read errors on the device
> > +# (default: report)
> > +# @werror: #optional how to handle write errors on the device
> > +# (default: enospc)
> > +# @throttling: #optional I/O throttling related options
> > +# @read-only: #optional whether the block device should be read-only
> > +# (default: false)
> > +#
> > +# Since: 1.7
> > +##
> > +{ 'type': 'BlockdevOptionsBase',
> > + 'data': { 'driver': 'str',
> > + '*id': 'str',
> > + '*discard': 'BlockdevDiscardOptions',
> > + '*cache': 'BlockdevCacheOptions',
> > + '*aio': 'BlockdevAIOOptions',
> > + '*rerror': 'BlockdevOnError',
> > + '*werror': 'BlockdevOnError',
> > + '*throttling': 'BlockdevThrottlingOptions',
> > + '*read-only': 'bool' } }
> > +
> > +##
> > +# @BlockdevOptionsFile
> > +#
> > +# Driver specific block device options for the file backend and similar
> > +# protocols.
> > +#
> > +# @filename: path to the image file
> > +#
> > +# Since: 1.7
> > +##
> > +{ 'type': 'BlockdevOptionsFile',
> > + 'data': { 'filename': 'str' } }
> > +
> > +##
> > +# @BlockdevOptionsVVFAT
> > +#
> > +# Driver specific block device options for the vvfat protocol.
> > +#
> > +# @dir: directory to be exported as FAT image
> > +# @fat-type: #optional FAT type: 12, 16 or 32
> > +# @floppy: #optional whether to export a floppy imae (true) or partitioned
> > +# hard disk (false; default)
> > +# @rw: #optional whether to allow write operations (default: false)
> > +#
> > +# Since: 1.7
> > +##
> > +{ 'type': 'BlockdevOptionsVVFAT',
> > + 'data': { 'dir': 'str', '*fat-type': 'int', '*floppy': 'bool',
> > + '*rw': 'bool' } }
> > +
> > +##
> > +# @BlockdevOptionsGenericFormat
> > +#
> > +# Driver specific block device options for image format that have no option
> > +# besides their data source.
> > +#
> > +# @file: reference to or definition of the data source block device
> > +#
> > +# Since: 1.7
> > +##
> > +{ 'type': 'BlockdevOptionsGenericFormat',
> > + 'data': { 'file': 'BlockdevRef' } }
> > +
> > +##
> > +# @BlockdevOptionsGenericCOWFormat
> > +#
> > +# Driver specific block device options for image format that have no option
> > +# besides their data source and an optional backing file.
> > +#
> > +# @file: reference to or definition of the data source block device
> > +# @backing: #optional reference to or definition of the backing file block
> > +# device (if missing, taken from the image file content). It is
> > +# allowed to pass an empty string here in order to disable the
> > +# default backing file.
> > +# @copy-on-read: #optional whether to enable copy on read for the device
> > +# (default: false). Copy on read can only be used if the
> > +# image is not read-only.
> > +#
> > +# Since: 1.7
> > +##
> > +{ 'type': 'BlockdevOptionsGenericCOWFormat',
> > + 'base': 'BlockdevOptionsGenericFormat',
> > + 'data': { '*backing': 'BlockdevRef',
> > + '*copy-on-read': 'bool' } }
> > +##
> > +# @BlockdevOptionsQcow2
> > +#
> > +# Driver specific block device options for qcow2.
> > +#
> > +# @file: reference to or definition of the data source block device
> > +#
> > +# @backing: #optional reference to or definition of the backing file block
> > +# device (if missing, taken from the image file content)
> > +#
> > +# @lazy-refcounts: #optional whether to enable the lazy refcounts feature
> > +# (default is taken from the image file)
> > +#
> > +# @pass-discard-request: #optional whether discard requests to the qcow2 device
> > +# should be forwarded to the data source
> > +#
> > +# @pass-discard-snapshot: #optional whether discard requests for the data source
> > +# should be issued when a snapshot operation (e.g.
> > +# deleting a snapshot) frees clusters in the qcow2 file
> > +#
> > +# @pass-discard-other: #optional whether discard requests for the data source
> > +# should be issued on other occasions where a cluster gets
> > +# freed
> > +#
> > +# Since: 1.7
> > +##
> > +{ 'type': 'BlockdevOptionsQcow2',
> > + 'base': 'BlockdevOptionsGenericCOWFormat',
> > + 'data': { '*lazy-refcounts': 'bool',
> > + '*pass-discard-request': 'bool',
> > + '*pass-discard-snapshot': 'bool',
> > + '*pass-discard-other': 'bool' } }
> > +
> > +##
> > +# @BlockdevOptions
> > +#
> > +# Options for creating a block device.
> > +#
> > +# Since: 1.7
> > +##
> > +{ 'union': 'BlockdevOptions',
> > + 'base': 'BlockdevOptionsBase',
> > + 'discriminator': 'driver',
> > + 'data': {
> > + 'file': 'BlockdevOptionsFile',
> > + 'http': 'BlockdevOptionsFile',
> > + 'https': 'BlockdevOptionsFile',
> > + 'ftp': 'BlockdevOptionsFile',
> > + 'ftps': 'BlockdevOptionsFile',
> > + 'tftp': 'BlockdevOptionsFile',
> > +# TODO gluster: Wait for structured options
> > +# TODO iscsi: Wait for structured options
> > +# TODO nbd: Should take InetSocketAddress for 'host'?
> > +# TODO rbd: Wait for structured options
> > +# TODO sheepdog: Wait for structured options
> > +# TODO ssh: Should take InetSocketAddress for 'host'?
> > + 'vvfat': 'BlockdevOptionsVVFAT',
> > +
> > +# TODO blkdebug: Wait for structured options
> > +# TODO blkverify: Wait for structured options
> > +
> > + 'bochs': 'BlockdevOptionsGenericFormat',
> > + 'cloop': 'BlockdevOptionsGenericFormat',
> > + 'cow': 'BlockdevOptionsGenericCOWFormat',
> > + 'dmg': 'BlockdevOptionsGenericFormat',
> > + 'parallels': 'BlockdevOptionsGenericFormat',
> > + 'qcow': 'BlockdevOptionsGenericCOWFormat',
> > + 'qcow2': 'BlockdevOptionsQcow2',
> > + 'qed': 'BlockdevOptionsGenericCOWFormat',
> > + 'raw': 'BlockdevOptionsGenericFormat',
> > + 'vdi': 'BlockdevOptionsGenericFormat',
> > + 'vhdx': 'BlockdevOptionsGenericFormat',
> > + 'vmdk': 'BlockdevOptionsGenericCOWFormat',
> > + 'vpc': 'BlockdevOptionsGenericFormat'
> > + } }
> > +
> > +##
> > +# @BlockdevRef
> > +#
> > +# Reference to a block device.
> > +#
> > +# @definition: defines a new block device inline
> > +# @reference: references the ID of an existing block device. An
> > +# empty string means that no block device should be
> > +# referenced.
> > +#
> > +# Since: 1.7
> > +##
> > +{ 'union': 'BlockdevRef',
> > + 'discriminator': {},
> > + 'data': { 'definition': 'BlockdevOptions',
> > + 'reference': 'str' } }
> > +
> > +##
> > +# @blockdev-add:
> > +#
> > +# Creates a new block device.
> > +#
> > +# @options: block device options for the new device
> > +#
> > +# Since: 1.7
> > +##
> > +{ 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index b17c46e..449ecea 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -3240,3 +3240,62 @@ Example:
> > }
> >
> > EQMP
> > +
> > + {
> > + .name = "blockdev-add",
> > + .args_type = "options:q",
> > + .mhandler.cmd_new = qmp_marshal_input_blockdev_add,
> > + },
> > +
> > +SQMP
> > +blockdev-add
> > +------------
> > +
> > +Add a block device.
> > +
> > +Arguments:
> > +
> > +- "options": block driver options
> > +
> > +Example (1):
> > +
> > +-> { "execute": "blockdev-add",
> > + "arguments": { "options" : { "driver": "qcow2",
> > + "file": { "driver": "file",
> > + "filename": "test.qcow2" } } } }
> > +<- { "return": {} }
> > +
> > +Example (2):
> > +
> > +-> { "execute": "blockdev-add",
> > + "arguments": {
> > + "options": {
> > + "driver": "qcow2",
> > + "id": "my_disk",
> > + "discard": "unmap",
> > + "throttling": {
> > + "bps-total": 1234567,
> > + "iops-write": 100
> > + },
> > + "cache": {
> > + "direct": true,
> > + "writeback": true
> > + },
> > + "file": {
> > + "driver": "file",
> > + "filename": "/tmp/test.qcow2"
> > + },
> > + "backing": {
> > + "driver": "raw",
> > + "file": {
> > + "driver": "file",
> > + "filename": "/dev/fdset/4"
> > + }
> > + }
>
> How will BDS reference be used? Like this?
>
> "backing": "ide0-hd0",
>
> [As I tested this is not supported with this series, the option "backing" is
> not parsed in bdrv_open(). Asking because image fleecing needs this, is it
> already in your todo list?]
Yes, this is what the syntax will be (and is already described in the
schema, even though it's not actually implemented yet).
It's in my todo list, but it won't come immediately after this series.
Leaving other todo items aside, one dependency that is missing for it is
that we make sure that having a named BDS in the middle of a tree
doesn't (allow users to) break things.
Kevin
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [Qemu-devel] [PATCH 04/17] blockdev: 'blockdev-add' QMP command
2013-09-20 11:54 ` [Qemu-devel] [PATCH 04/17] blockdev: 'blockdev-add' QMP command Kevin Wolf
` (3 preceding siblings ...)
2013-09-24 5:18 ` Fam Zheng
@ 2013-09-24 10:39 ` Paolo Bonzini
4 siblings, 0 replies; 56+ messages in thread
From: Paolo Bonzini @ 2013-09-24 10:39 UTC (permalink / raw)
To: Kevin Wolf; +Cc: mreitz, qemu-devel, stefanha, armbru
Il 20/09/2013 13:54, Kevin Wolf ha scritto:
> + * For now, simply forbidding the combination for all drivers will do. */
> + if (options->has_aio && options->aio == BLOCKDEV_A_I_O_OPTIONS_NATIVE) {
Let's call it BlockdevAioOptions instead?
Paolo
> + bool direct = options->cache->has_direct && options->cache->direct;
> + if (!options->has_cache && !direct) {
> + error_setg(errp, "aio=native requires cache.direct=true");
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH 05/17] blockdev: Separate ID generation from DriveInfo creation
2013-09-20 11:54 [Qemu-devel] [PATCH 00/17] blockdev-add QMP command Kevin Wolf
` (3 preceding siblings ...)
2013-09-20 11:54 ` [Qemu-devel] [PATCH 04/17] blockdev: 'blockdev-add' QMP command Kevin Wolf
@ 2013-09-20 11:54 ` Kevin Wolf
2013-09-23 9:01 ` Max Reitz
` (2 more replies)
2013-09-20 11:54 ` [Qemu-devel] [PATCH 06/17] blockdev: Pass QDict to blockdev_init() Kevin Wolf
` (11 subsequent siblings)
16 siblings, 3 replies; 56+ messages in thread
From: Kevin Wolf @ 2013-09-20 11:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, mreitz
blockdev-add shouldn't automatically generate IDs, but will keep most of
the DriveInfo creation code.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
blockdev.c | 32 +++++++++++++++++---------------
include/qemu/option.h | 1 +
util/qemu-option.c | 6 ++++++
3 files changed, 24 insertions(+), 15 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index c4297d8..e3cff31 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -604,23 +604,25 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
return NULL;
}
- /* init */
-
- dinfo = g_malloc0(sizeof(*dinfo));
- if ((buf = qemu_opts_id(opts)) != NULL) {
- dinfo->id = g_strdup(buf);
- } else {
- /* no id supplied -> create one */
- dinfo->id = g_malloc0(32);
- if (type == IF_IDE || type == IF_SCSI)
+ /* no id supplied -> create one */
+ if (qemu_opts_id(opts) == NULL) {
+ char *new_id;
+ if (type == IF_IDE || type == IF_SCSI) {
mediastr = (media == MEDIA_CDROM) ? "-cd" : "-hd";
- if (max_devs)
- snprintf(dinfo->id, 32, "%s%i%s%i",
- if_name[type], bus_id, mediastr, unit_id);
- else
- snprintf(dinfo->id, 32, "%s%s%i",
- if_name[type], mediastr, unit_id);
+ }
+ if (max_devs) {
+ new_id = g_strdup_printf("%s%i%s%i", if_name[type], bus_id,
+ mediastr, unit_id);
+ } else {
+ new_id = g_strdup_printf("%s%s%i", if_name[type],
+ mediastr, unit_id);
+ }
+ qemu_opts_set_id(opts, new_id);
}
+
+ /* init */
+ dinfo = g_malloc0(sizeof(*dinfo));
+ dinfo->id = g_strdup(qemu_opts_id(opts));
dinfo->bdrv = bdrv_new(dinfo->id);
dinfo->bdrv->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
dinfo->bdrv->read_only = ro;
diff --git a/include/qemu/option.h b/include/qemu/option.h
index 63db4cc..5c0c6dd 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -142,6 +142,7 @@ void qemu_opts_loc_restore(QemuOpts *opts);
int qemu_opts_set(QemuOptsList *list, const char *id,
const char *name, const char *value);
const char *qemu_opts_id(QemuOpts *opts);
+void qemu_opts_set_id(QemuOpts *opts, char *id);
void qemu_opts_del(QemuOpts *opts);
void qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc, Error **errp);
int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname);
diff --git a/util/qemu-option.c b/util/qemu-option.c
index e0844a9..efcb5dc 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -834,6 +834,12 @@ const char *qemu_opts_id(QemuOpts *opts)
return opts->id;
}
+/* The id string will be g_free()d by qemu_opts_del */
+void qemu_opts_set_id(QemuOpts *opts, char *id)
+{
+ opts->id = id;
+}
+
void qemu_opts_del(QemuOpts *opts)
{
QemuOpt *opt;
--
1.8.1.4
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH 05/17] blockdev: Separate ID generation from DriveInfo creation
2013-09-20 11:54 ` [Qemu-devel] [PATCH 05/17] blockdev: Separate ID generation from DriveInfo creation Kevin Wolf
@ 2013-09-23 9:01 ` Max Reitz
2013-09-23 15:45 ` Eric Blake
2013-09-30 5:24 ` Wenchao Xia
2 siblings, 0 replies; 56+ messages in thread
From: Max Reitz @ 2013-09-23 9:01 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, stefanha, armbru
On 2013-09-20 13:54, Kevin Wolf wrote:
> blockdev-add shouldn't automatically generate IDs, but will keep most of
> the DriveInfo creation code.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> blockdev.c | 32 +++++++++++++++++---------------
> include/qemu/option.h | 1 +
> util/qemu-option.c | 6 ++++++
> 3 files changed, 24 insertions(+), 15 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [Qemu-devel] [PATCH 05/17] blockdev: Separate ID generation from DriveInfo creation
2013-09-20 11:54 ` [Qemu-devel] [PATCH 05/17] blockdev: Separate ID generation from DriveInfo creation Kevin Wolf
2013-09-23 9:01 ` Max Reitz
@ 2013-09-23 15:45 ` Eric Blake
2013-09-30 5:24 ` Wenchao Xia
2 siblings, 0 replies; 56+ messages in thread
From: Eric Blake @ 2013-09-23 15:45 UTC (permalink / raw)
To: Kevin Wolf; +Cc: mreitz, qemu-devel, stefanha, armbru
[-- Attachment #1: Type: text/plain, Size: 562 bytes --]
On 09/20/2013 05:54 AM, Kevin Wolf wrote:
> blockdev-add shouldn't automatically generate IDs, but will keep most of
> the DriveInfo creation code.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> blockdev.c | 32 +++++++++++++++++---------------
> include/qemu/option.h | 1 +
> util/qemu-option.c | 6 ++++++
> 3 files changed, 24 insertions(+), 15 deletions(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [Qemu-devel] [PATCH 05/17] blockdev: Separate ID generation from DriveInfo creation
2013-09-20 11:54 ` [Qemu-devel] [PATCH 05/17] blockdev: Separate ID generation from DriveInfo creation Kevin Wolf
2013-09-23 9:01 ` Max Reitz
2013-09-23 15:45 ` Eric Blake
@ 2013-09-30 5:24 ` Wenchao Xia
2 siblings, 0 replies; 56+ messages in thread
From: Wenchao Xia @ 2013-09-30 5:24 UTC (permalink / raw)
To: Kevin Wolf; +Cc: mreitz, qemu-devel, stefanha, armbru
Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH 06/17] blockdev: Pass QDict to blockdev_init()
2013-09-20 11:54 [Qemu-devel] [PATCH 00/17] blockdev-add QMP command Kevin Wolf
` (4 preceding siblings ...)
2013-09-20 11:54 ` [Qemu-devel] [PATCH 05/17] blockdev: Separate ID generation from DriveInfo creation Kevin Wolf
@ 2013-09-20 11:54 ` Kevin Wolf
2013-09-20 14:11 ` Benoît Canet
2013-09-25 6:25 ` Fam Zheng
2013-09-20 11:54 ` [Qemu-devel] [PATCH 07/17] blockdev: Move parsing of 'media' option to drive_init Kevin Wolf
` (10 subsequent siblings)
16 siblings, 2 replies; 56+ messages in thread
From: Kevin Wolf @ 2013-09-20 11:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, mreitz
Working on a QDict instead of a QemuOpts that accepts anything is more
in line with bdrv_open(). A QDict is what qmp_blockdev_add() already has
anyway, so this saves additional conversions. And last, but not least,
it allows later patches to easily extract legacy options into a
separate, typed QemuOpts for drive_init() (the untyped QemuOpts that
drive_init already has doesn't allow access to numbers, only strings,
and is therefore useless without conversion).
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
blockdev.c | 33 +++++++++++++++------------------
1 file changed, 15 insertions(+), 18 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index e3cff31..3a1444c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -302,7 +302,8 @@ static bool check_throttle_config(ThrottleConfig *cfg, Error **errp)
return true;
}
-static DriveInfo *blockdev_init(QemuOpts *all_opts,
+/* Takes the ownership of bs_opts */
+static DriveInfo *blockdev_init(QDict *bs_opts,
BlockInterfaceType block_default_type)
{
const char *buf;
@@ -326,7 +327,6 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
int ret;
Error *error = NULL;
QemuOpts *opts;
- QDict *bs_opts;
const char *id;
bool has_driver_specific_opts;
BlockDriver *drv = NULL;
@@ -334,9 +334,9 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
translation = BIOS_ATA_TRANSLATION_AUTO;
media = MEDIA_DISK;
- /* Check common options by copying from all_opts to opts, all other options
- * are stored in bs_opts. */
- id = qemu_opts_id(all_opts);
+ /* Check common options by copying from bs_opts to opts, all other options
+ * stay in bs_opts for processing by bdrv_open(). */
+ id = qdict_get_try_str(bs_opts, "id");
opts = qemu_opts_create(&qemu_common_drive_opts, id, 1, &error);
if (error_is_set(&error)) {
qerror_report_err(error);
@@ -344,8 +344,6 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
return NULL;
}
- bs_opts = qdict_new();
- qemu_opts_to_qdict(all_opts, bs_opts);
qemu_opts_absorb_qdict(opts, bs_opts, &error);
if (error_is_set(&error)) {
qerror_report_err(error);
@@ -634,7 +632,6 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
dinfo->heads = heads;
dinfo->secs = secs;
dinfo->trans = translation;
- dinfo->opts = all_opts;
dinfo->refcount = 1;
if (serial != NULL) {
dinfo->serial = g_strdup(serial);
@@ -759,6 +756,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
{
const char *value;
DriveInfo *dinfo;
+ QDict *bs_opts;
/* Change legacy command line options into QMP ones */
qemu_opt_rename(all_opts, "iops", "throttling.iops-total");
@@ -807,14 +805,19 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
qemu_opt_unset(all_opts, "cache");
}
+ /* Get a QDict for processing the options */
+ bs_opts = qdict_new();
+ qemu_opts_to_qdict(all_opts, bs_opts);
+
/* Actual block device init: Functionality shared with blockdev-add */
- dinfo = blockdev_init(all_opts, block_default_type);
+ dinfo = blockdev_init(bs_opts, block_default_type);
if (dinfo == NULL) {
goto fail;
}
/* Set legacy DriveInfo fields */
dinfo->enable_auto_del = true;
+ dinfo->opts = all_opts;
fail:
return dinfo;
@@ -2109,16 +2112,10 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
qdict_flatten(qdict);
- QemuOpts *opts = qemu_opts_from_qdict(&qemu_drive_opts, qdict, &local_err);
- if (error_is_set(&local_err)) {
- error_propagate(errp, local_err);
+ dinfo = blockdev_init(qdict, IF_NONE);
+ if (!dinfo) {
+ error_setg(errp, "Could not open image");
goto fail;
- } else {
- dinfo = blockdev_init(opts, IF_NONE);
- if (!dinfo) {
- error_setg(errp, "Could not open image");
- goto fail;
- }
}
fail:
--
1.8.1.4
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH 06/17] blockdev: Pass QDict to blockdev_init()
2013-09-20 11:54 ` [Qemu-devel] [PATCH 06/17] blockdev: Pass QDict to blockdev_init() Kevin Wolf
@ 2013-09-20 14:11 ` Benoît Canet
2013-09-25 6:25 ` Fam Zheng
1 sibling, 0 replies; 56+ messages in thread
From: Benoît Canet @ 2013-09-20 14:11 UTC (permalink / raw)
To: Kevin Wolf; +Cc: mreitz, qemu-devel, stefanha, armbru
Le Friday 20 Sep 2013 à 13:54:19 (+0200), Kevin Wolf a écrit :
> Working on a QDict instead of a QemuOpts that accepts anything is more
> in line with bdrv_open(). A QDict is what qmp_blockdev_add() already has
> anyway, so this saves additional conversions. And last, but not least,
> it allows later patches to easily extract legacy options into a
> separate, typed QemuOpts for drive_init() (the untyped QemuOpts that
> drive_init already has doesn't allow access to numbers, only strings,
> and is therefore useless without conversion).
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> blockdev.c | 33 +++++++++++++++------------------
> 1 file changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index e3cff31..3a1444c 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -302,7 +302,8 @@ static bool check_throttle_config(ThrottleConfig *cfg, Error **errp)
> return true;
> }
>
> -static DriveInfo *blockdev_init(QemuOpts *all_opts,
> +/* Takes the ownership of bs_opts */
> +static DriveInfo *blockdev_init(QDict *bs_opts,
> BlockInterfaceType block_default_type)
> {
> const char *buf;
> @@ -326,7 +327,6 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
> int ret;
> Error *error = NULL;
> QemuOpts *opts;
> - QDict *bs_opts;
> const char *id;
> bool has_driver_specific_opts;
> BlockDriver *drv = NULL;
> @@ -334,9 +334,9 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
> translation = BIOS_ATA_TRANSLATION_AUTO;
> media = MEDIA_DISK;
>
> - /* Check common options by copying from all_opts to opts, all other options
> - * are stored in bs_opts. */
> - id = qemu_opts_id(all_opts);
> + /* Check common options by copying from bs_opts to opts, all other options
> + * stay in bs_opts for processing by bdrv_open(). */
> + id = qdict_get_try_str(bs_opts, "id");
> opts = qemu_opts_create(&qemu_common_drive_opts, id, 1, &error);
> if (error_is_set(&error)) {
> qerror_report_err(error);
> @@ -344,8 +344,6 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
> return NULL;
> }
>
> - bs_opts = qdict_new();
> - qemu_opts_to_qdict(all_opts, bs_opts);
> qemu_opts_absorb_qdict(opts, bs_opts, &error);
> if (error_is_set(&error)) {
> qerror_report_err(error);
> @@ -634,7 +632,6 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
> dinfo->heads = heads;
> dinfo->secs = secs;
> dinfo->trans = translation;
> - dinfo->opts = all_opts;
> dinfo->refcount = 1;
> if (serial != NULL) {
> dinfo->serial = g_strdup(serial);
> @@ -759,6 +756,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> {
> const char *value;
> DriveInfo *dinfo;
> + QDict *bs_opts;
>
> /* Change legacy command line options into QMP ones */
> qemu_opt_rename(all_opts, "iops", "throttling.iops-total");
> @@ -807,14 +805,19 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> qemu_opt_unset(all_opts, "cache");
> }
>
> + /* Get a QDict for processing the options */
> + bs_opts = qdict_new();
> + qemu_opts_to_qdict(all_opts, bs_opts);
> +
> /* Actual block device init: Functionality shared with blockdev-add */
> - dinfo = blockdev_init(all_opts, block_default_type);
> + dinfo = blockdev_init(bs_opts, block_default_type);
> if (dinfo == NULL) {
> goto fail;
> }
>
> /* Set legacy DriveInfo fields */
> dinfo->enable_auto_del = true;
> + dinfo->opts = all_opts;
>
> fail:
> return dinfo;
> @@ -2109,16 +2112,10 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
>
> qdict_flatten(qdict);
>
> - QemuOpts *opts = qemu_opts_from_qdict(&qemu_drive_opts, qdict, &local_err);
> - if (error_is_set(&local_err)) {
> - error_propagate(errp, local_err);
> + dinfo = blockdev_init(qdict, IF_NONE);
> + if (!dinfo) {
> + error_setg(errp, "Could not open image");
> goto fail;
> - } else {
> - dinfo = blockdev_init(opts, IF_NONE);
> - if (!dinfo) {
> - error_setg(errp, "Could not open image");
> - goto fail;
> - }
> }
>
> fail:
> --
> 1.8.1.4
>
>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH 06/17] blockdev: Pass QDict to blockdev_init()
2013-09-20 11:54 ` [Qemu-devel] [PATCH 06/17] blockdev: Pass QDict to blockdev_init() Kevin Wolf
2013-09-20 14:11 ` Benoît Canet
@ 2013-09-25 6:25 ` Fam Zheng
1 sibling, 0 replies; 56+ messages in thread
From: Fam Zheng @ 2013-09-25 6:25 UTC (permalink / raw)
To: Kevin Wolf; +Cc: mreitz, qemu-devel, stefanha, armbru
On Fri, 09/20 13:54, Kevin Wolf wrote:
> Working on a QDict instead of a QemuOpts that accepts anything is more
> in line with bdrv_open(). A QDict is what qmp_blockdev_add() already has
> anyway, so this saves additional conversions. And last, but not least,
> it allows later patches to easily extract legacy options into a
> separate, typed QemuOpts for drive_init() (the untyped QemuOpts that
> drive_init already has doesn't allow access to numbers, only strings,
> and is therefore useless without conversion).
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> blockdev.c | 33 +++++++++++++++------------------
> 1 file changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index e3cff31..3a1444c 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -302,7 +302,8 @@ static bool check_throttle_config(ThrottleConfig *cfg, Error **errp)
> return true;
> }
>
> -static DriveInfo *blockdev_init(QemuOpts *all_opts,
> +/* Takes the ownership of bs_opts */
> +static DriveInfo *blockdev_init(QDict *bs_opts,
> BlockInterfaceType block_default_type)
> {
> const char *buf;
> @@ -326,7 +327,6 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
> int ret;
> Error *error = NULL;
> QemuOpts *opts;
> - QDict *bs_opts;
> const char *id;
> bool has_driver_specific_opts;
> BlockDriver *drv = NULL;
> @@ -334,9 +334,9 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
> translation = BIOS_ATA_TRANSLATION_AUTO;
> media = MEDIA_DISK;
>
> - /* Check common options by copying from all_opts to opts, all other options
> - * are stored in bs_opts. */
> - id = qemu_opts_id(all_opts);
> + /* Check common options by copying from bs_opts to opts, all other options
> + * stay in bs_opts for processing by bdrv_open(). */
> + id = qdict_get_try_str(bs_opts, "id");
> opts = qemu_opts_create(&qemu_common_drive_opts, id, 1, &error);
> if (error_is_set(&error)) {
> qerror_report_err(error);
> @@ -344,8 +344,6 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
> return NULL;
> }
>
> - bs_opts = qdict_new();
> - qemu_opts_to_qdict(all_opts, bs_opts);
> qemu_opts_absorb_qdict(opts, bs_opts, &error);
> if (error_is_set(&error)) {
> qerror_report_err(error);
> @@ -634,7 +632,6 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
> dinfo->heads = heads;
> dinfo->secs = secs;
> dinfo->trans = translation;
> - dinfo->opts = all_opts;
Not setting dinfo->opts in blockdev-add will SIGSEGV drive_del:
static void drive_uninit(DriveInfo *dinfo)
{
qemu_opts_del(dinfo->opts);
...
and:
void qemu_opts_del(QemuOpts *opts)
{
QemuOpt *opt;
for (;;) {
opt = QTAILQ_FIRST(&opts->head);
...
Fam
> dinfo->refcount = 1;
> if (serial != NULL) {
> dinfo->serial = g_strdup(serial);
> @@ -759,6 +756,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> {
> const char *value;
> DriveInfo *dinfo;
> + QDict *bs_opts;
>
> /* Change legacy command line options into QMP ones */
> qemu_opt_rename(all_opts, "iops", "throttling.iops-total");
> @@ -807,14 +805,19 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> qemu_opt_unset(all_opts, "cache");
> }
>
> + /* Get a QDict for processing the options */
> + bs_opts = qdict_new();
> + qemu_opts_to_qdict(all_opts, bs_opts);
> +
> /* Actual block device init: Functionality shared with blockdev-add */
> - dinfo = blockdev_init(all_opts, block_default_type);
> + dinfo = blockdev_init(bs_opts, block_default_type);
> if (dinfo == NULL) {
> goto fail;
> }
>
> /* Set legacy DriveInfo fields */
> dinfo->enable_auto_del = true;
> + dinfo->opts = all_opts;
>
> fail:
> return dinfo;
> @@ -2109,16 +2112,10 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
>
> qdict_flatten(qdict);
>
> - QemuOpts *opts = qemu_opts_from_qdict(&qemu_drive_opts, qdict, &local_err);
> - if (error_is_set(&local_err)) {
> - error_propagate(errp, local_err);
> + dinfo = blockdev_init(qdict, IF_NONE);
> + if (!dinfo) {
> + error_setg(errp, "Could not open image");
> goto fail;
> - } else {
> - dinfo = blockdev_init(opts, IF_NONE);
> - if (!dinfo) {
> - error_setg(errp, "Could not open image");
> - goto fail;
> - }
> }
>
> fail:
> --
> 1.8.1.4
>
>
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH 07/17] blockdev: Move parsing of 'media' option to drive_init
2013-09-20 11:54 [Qemu-devel] [PATCH 00/17] blockdev-add QMP command Kevin Wolf
` (5 preceding siblings ...)
2013-09-20 11:54 ` [Qemu-devel] [PATCH 06/17] blockdev: Pass QDict to blockdev_init() Kevin Wolf
@ 2013-09-20 11:54 ` Kevin Wolf
2013-09-20 14:28 ` Benoît Canet
2013-09-20 11:54 ` [Qemu-devel] [PATCH 08/17] blockdev: Move parsing of 'if' " Kevin Wolf
` (9 subsequent siblings)
16 siblings, 1 reply; 56+ messages in thread
From: Kevin Wolf @ 2013-09-20 11:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, mreitz
This moves as much as possible of the processing of the 'media' option
to drive_init so that it can only be accessed using legacy functions,
but never with anything blockdev-add related.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
blockdev.c | 73 ++++++++++++++++++++++++++++++++++++++++++--------------------
1 file changed, 50 insertions(+), 23 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 3a1444c..5b25d7f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -302,16 +302,18 @@ static bool check_throttle_config(ThrottleConfig *cfg, Error **errp)
return true;
}
+typedef enum { MEDIA_DISK, MEDIA_CDROM } DriveMediaType;
+
/* Takes the ownership of bs_opts */
static DriveInfo *blockdev_init(QDict *bs_opts,
- BlockInterfaceType block_default_type)
+ BlockInterfaceType block_default_type,
+ DriveMediaType media)
{
const char *buf;
const char *file = NULL;
const char *serial;
const char *mediastr = "";
BlockInterfaceType type;
- enum { MEDIA_DISK, MEDIA_CDROM } media;
int bus_id, unit_id;
int cyls, heads, secs, translation;
int max_devs;
@@ -332,7 +334,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
BlockDriver *drv = NULL;
translation = BIOS_ATA_TRANSLATION_AUTO;
- media = MEDIA_DISK;
/* Check common options by copying from bs_opts to opts, all other options
* stay in bs_opts for processing by bdrv_open(). */
@@ -419,19 +420,11 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
}
}
- if ((buf = qemu_opt_get(opts, "media")) != NULL) {
- if (!strcmp(buf, "disk")) {
- media = MEDIA_DISK;
- } else if (!strcmp(buf, "cdrom")) {
- if (cyls || secs || heads) {
- error_report("CHS can't be set with media=%s", buf);
- return NULL;
- }
- media = MEDIA_CDROM;
- } else {
- error_report("'%s' invalid media", buf);
- return NULL;
- }
+ if (media == MEDIA_CDROM) {
+ if (cyls || secs || heads) {
+ error_report("CHS can't be set with media=cdrom");
+ return NULL;
+ }
}
if ((buf = qemu_opt_get(opts, "discard")) != NULL) {
@@ -752,11 +745,27 @@ static void qemu_opt_rename(QemuOpts *opts, const char *from, const char *to)
}
}
+QemuOptsList qemu_legacy_drive_opts = {
+ .name = "drive",
+ .head = QTAILQ_HEAD_INITIALIZER(qemu_legacy_drive_opts.head),
+ .desc = {
+ {
+ .name = "media",
+ .type = QEMU_OPT_STRING,
+ .help = "media type (disk, cdrom)",
+ },
+ { /* end of list */ }
+ },
+};
+
DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
{
const char *value;
- DriveInfo *dinfo;
+ DriveInfo *dinfo = NULL;
QDict *bs_opts;
+ QemuOpts *legacy_opts;
+ DriveMediaType media = MEDIA_DISK;
+ Error *local_err = NULL;
/* Change legacy command line options into QMP ones */
qemu_opt_rename(all_opts, "iops", "throttling.iops-total");
@@ -809,8 +818,29 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
bs_opts = qdict_new();
qemu_opts_to_qdict(all_opts, bs_opts);
+ legacy_opts = qemu_opts_create_nofail(&qemu_legacy_drive_opts);
+ qemu_opts_absorb_qdict(legacy_opts, bs_opts, &local_err);
+ if (error_is_set(&local_err)) {
+ qerror_report_err(local_err);
+ error_free(local_err);
+ goto fail;
+ }
+
+ /* Media type */
+ value = qemu_opt_get(legacy_opts, "media");
+ if (value) {
+ if (!strcmp(value, "disk")) {
+ media = MEDIA_DISK;
+ } else if (!strcmp(value, "cdrom")) {
+ media = MEDIA_CDROM;
+ } else {
+ error_report("'%s' invalid media", value);
+ goto fail;
+ }
+ }
+
/* Actual block device init: Functionality shared with blockdev-add */
- dinfo = blockdev_init(bs_opts, block_default_type);
+ dinfo = blockdev_init(bs_opts, block_default_type, media);
if (dinfo == NULL) {
goto fail;
}
@@ -820,6 +850,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
dinfo->opts = all_opts;
fail:
+ qemu_opts_del(legacy_opts);
return dinfo;
}
@@ -2112,7 +2143,7 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
qdict_flatten(qdict);
- dinfo = blockdev_init(qdict, IF_NONE);
+ dinfo = blockdev_init(qdict, IF_NONE, MEDIA_DISK);
if (!dinfo) {
error_setg(errp, "Could not open image");
goto fail;
@@ -2181,10 +2212,6 @@ QemuOptsList qemu_common_drive_opts = {
.type = QEMU_OPT_STRING,
.help = "chs translation (auto, lba. none)",
},{
- .name = "media",
- .type = QEMU_OPT_STRING,
- .help = "media type (disk, cdrom)",
- },{
.name = "snapshot",
.type = QEMU_OPT_BOOL,
.help = "enable/disable snapshot mode",
--
1.8.1.4
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH 07/17] blockdev: Move parsing of 'media' option to drive_init
2013-09-20 11:54 ` [Qemu-devel] [PATCH 07/17] blockdev: Move parsing of 'media' option to drive_init Kevin Wolf
@ 2013-09-20 14:28 ` Benoît Canet
0 siblings, 0 replies; 56+ messages in thread
From: Benoît Canet @ 2013-09-20 14:28 UTC (permalink / raw)
To: Kevin Wolf; +Cc: mreitz, qemu-devel, stefanha, armbru
Le Friday 20 Sep 2013 à 13:54:20 (+0200), Kevin Wolf a écrit :
> This moves as much as possible of the processing of the 'media' option
> to drive_init so that it can only be accessed using legacy functions,
> but never with anything blockdev-add related.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> blockdev.c | 73 ++++++++++++++++++++++++++++++++++++++++++--------------------
> 1 file changed, 50 insertions(+), 23 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 3a1444c..5b25d7f 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -302,16 +302,18 @@ static bool check_throttle_config(ThrottleConfig *cfg, Error **errp)
> return true;
> }
>
> +typedef enum { MEDIA_DISK, MEDIA_CDROM } DriveMediaType;
> +
> /* Takes the ownership of bs_opts */
> static DriveInfo *blockdev_init(QDict *bs_opts,
> - BlockInterfaceType block_default_type)
> + BlockInterfaceType block_default_type,
> + DriveMediaType media)
> {
> const char *buf;
> const char *file = NULL;
> const char *serial;
> const char *mediastr = "";
> BlockInterfaceType type;
> - enum { MEDIA_DISK, MEDIA_CDROM } media;
> int bus_id, unit_id;
> int cyls, heads, secs, translation;
> int max_devs;
> @@ -332,7 +334,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
> BlockDriver *drv = NULL;
>
> translation = BIOS_ATA_TRANSLATION_AUTO;
> - media = MEDIA_DISK;
>
> /* Check common options by copying from bs_opts to opts, all other options
> * stay in bs_opts for processing by bdrv_open(). */
> @@ -419,19 +420,11 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
> }
> }
>
> - if ((buf = qemu_opt_get(opts, "media")) != NULL) {
> - if (!strcmp(buf, "disk")) {
> - media = MEDIA_DISK;
> - } else if (!strcmp(buf, "cdrom")) {
> - if (cyls || secs || heads) {
> - error_report("CHS can't be set with media=%s", buf);
> - return NULL;
> - }
> - media = MEDIA_CDROM;
> - } else {
> - error_report("'%s' invalid media", buf);
> - return NULL;
> - }
> + if (media == MEDIA_CDROM) {
> + if (cyls || secs || heads) {
> + error_report("CHS can't be set with media=cdrom");
> + return NULL;
> + }
> }
>
> if ((buf = qemu_opt_get(opts, "discard")) != NULL) {
> @@ -752,11 +745,27 @@ static void qemu_opt_rename(QemuOpts *opts, const char *from, const char *to)
> }
> }
>
> +QemuOptsList qemu_legacy_drive_opts = {
> + .name = "drive",
> + .head = QTAILQ_HEAD_INITIALIZER(qemu_legacy_drive_opts.head),
> + .desc = {
> + {
> + .name = "media",
> + .type = QEMU_OPT_STRING,
> + .help = "media type (disk, cdrom)",
> + },
> + { /* end of list */ }
> + },
> +};
> +
> DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> {
> const char *value;
> - DriveInfo *dinfo;
> + DriveInfo *dinfo = NULL;
> QDict *bs_opts;
> + QemuOpts *legacy_opts;
> + DriveMediaType media = MEDIA_DISK;
> + Error *local_err = NULL;
>
> /* Change legacy command line options into QMP ones */
> qemu_opt_rename(all_opts, "iops", "throttling.iops-total");
> @@ -809,8 +818,29 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> bs_opts = qdict_new();
> qemu_opts_to_qdict(all_opts, bs_opts);
>
> + legacy_opts = qemu_opts_create_nofail(&qemu_legacy_drive_opts);
> + qemu_opts_absorb_qdict(legacy_opts, bs_opts, &local_err);
> + if (error_is_set(&local_err)) {
> + qerror_report_err(local_err);
> + error_free(local_err);
> + goto fail;
> + }
> +
> + /* Media type */
> + value = qemu_opt_get(legacy_opts, "media");
> + if (value) {
> + if (!strcmp(value, "disk")) {
> + media = MEDIA_DISK;
> + } else if (!strcmp(value, "cdrom")) {
> + media = MEDIA_CDROM;
> + } else {
> + error_report("'%s' invalid media", value);
> + goto fail;
> + }
> + }
> +
> /* Actual block device init: Functionality shared with blockdev-add */
> - dinfo = blockdev_init(bs_opts, block_default_type);
> + dinfo = blockdev_init(bs_opts, block_default_type, media);
> if (dinfo == NULL) {
> goto fail;
> }
> @@ -820,6 +850,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> dinfo->opts = all_opts;
>
> fail:
> + qemu_opts_del(legacy_opts);
> return dinfo;
> }
>
> @@ -2112,7 +2143,7 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
>
> qdict_flatten(qdict);
>
> - dinfo = blockdev_init(qdict, IF_NONE);
> + dinfo = blockdev_init(qdict, IF_NONE, MEDIA_DISK);
> if (!dinfo) {
> error_setg(errp, "Could not open image");
> goto fail;
> @@ -2181,10 +2212,6 @@ QemuOptsList qemu_common_drive_opts = {
> .type = QEMU_OPT_STRING,
> .help = "chs translation (auto, lba. none)",
> },{
> - .name = "media",
> - .type = QEMU_OPT_STRING,
> - .help = "media type (disk, cdrom)",
> - },{
> .name = "snapshot",
> .type = QEMU_OPT_BOOL,
> .help = "enable/disable snapshot mode",
> --
> 1.8.1.4
>
>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH 08/17] blockdev: Move parsing of 'if' option to drive_init
2013-09-20 11:54 [Qemu-devel] [PATCH 00/17] blockdev-add QMP command Kevin Wolf
` (6 preceding siblings ...)
2013-09-20 11:54 ` [Qemu-devel] [PATCH 07/17] blockdev: Move parsing of 'media' option to drive_init Kevin Wolf
@ 2013-09-20 11:54 ` Kevin Wolf
2013-09-20 14:47 ` Max Reitz
2013-09-20 14:50 ` Benoît Canet
2013-09-20 11:54 ` [Qemu-devel] [PATCH 09/17] blockdev: Moving parsing of geometry options " Kevin Wolf
` (8 subsequent siblings)
16 siblings, 2 replies; 56+ messages in thread
From: Kevin Wolf @ 2013-09-20 11:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, mreitz
It's always IF_NONE for blockdev-add.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
blockdev.c | 40 ++++++++++++++++++++++------------------
1 file changed, 22 insertions(+), 18 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 5b25d7f..dc3f01a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -306,14 +306,13 @@ typedef enum { MEDIA_DISK, MEDIA_CDROM } DriveMediaType;
/* Takes the ownership of bs_opts */
static DriveInfo *blockdev_init(QDict *bs_opts,
- BlockInterfaceType block_default_type,
+ BlockInterfaceType type,
DriveMediaType media)
{
const char *buf;
const char *file = NULL;
const char *serial;
const char *mediastr = "";
- BlockInterfaceType type;
int bus_id, unit_id;
int cyls, heads, secs, translation;
int max_devs;
@@ -374,17 +373,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
file = qemu_opt_get(opts, "file");
serial = qemu_opt_get(opts, "serial");
- if ((buf = qemu_opt_get(opts, "if")) != NULL) {
- for (type = 0; type < IF_COUNT && strcmp(buf, if_name[type]); type++)
- ;
- if (type == IF_COUNT) {
- error_report("unsupported bus type '%s'", buf);
- return NULL;
- }
- } else {
- type = block_default_type;
- }
-
max_devs = if_max_devs[type];
if (cyls || heads || secs) {
@@ -753,6 +741,10 @@ QemuOptsList qemu_legacy_drive_opts = {
.name = "media",
.type = QEMU_OPT_STRING,
.help = "media type (disk, cdrom)",
+ },{
+ .name = "if",
+ .type = QEMU_OPT_STRING,
+ .help = "interface (ide, scsi, sd, mtd, floppy, pflash, virtio)",
},
{ /* end of list */ }
},
@@ -765,6 +757,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
QDict *bs_opts;
QemuOpts *legacy_opts;
DriveMediaType media = MEDIA_DISK;
+ BlockInterfaceType type;
Error *local_err = NULL;
/* Change legacy command line options into QMP ones */
@@ -839,8 +832,23 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
}
}
+ /* Controller type */
+ value = qemu_opt_get(legacy_opts, "if");
+ if (value) {
+ for (type = 0;
+ type < IF_COUNT && strcmp(value, if_name[type]);
+ type++) {
+ }
+ if (type == IF_COUNT) {
+ error_report("unsupported bus type '%s'", value);
+ return NULL;
+ }
+ } else {
+ type = block_default_type;
+ }
+
/* Actual block device init: Functionality shared with blockdev-add */
- dinfo = blockdev_init(bs_opts, block_default_type, media);
+ dinfo = blockdev_init(bs_opts, type, media);
if (dinfo == NULL) {
goto fail;
}
@@ -2188,10 +2196,6 @@ QemuOptsList qemu_common_drive_opts = {
.type = QEMU_OPT_NUMBER,
.help = "unit number (i.e. lun for scsi)",
},{
- .name = "if",
- .type = QEMU_OPT_STRING,
- .help = "interface (ide, scsi, sd, mtd, floppy, pflash, virtio)",
- },{
.name = "index",
.type = QEMU_OPT_NUMBER,
.help = "index number",
--
1.8.1.4
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH 08/17] blockdev: Move parsing of 'if' option to drive_init
2013-09-20 11:54 ` [Qemu-devel] [PATCH 08/17] blockdev: Move parsing of 'if' " Kevin Wolf
@ 2013-09-20 14:47 ` Max Reitz
2013-09-20 15:04 ` Kevin Wolf
2013-09-20 14:50 ` Benoît Canet
1 sibling, 1 reply; 56+ messages in thread
From: Max Reitz @ 2013-09-20 14:47 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, stefanha, armbru
On 2013-09-20 13:54, Kevin Wolf wrote:
> It's always IF_NONE for blockdev-add.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> blockdev.c | 40 ++++++++++++++++++++++------------------
> 1 file changed, 22 insertions(+), 18 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 5b25d7f..dc3f01a 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -306,14 +306,13 @@ typedef enum { MEDIA_DISK, MEDIA_CDROM } DriveMediaType;
>
> /* Takes the ownership of bs_opts */
> static DriveInfo *blockdev_init(QDict *bs_opts,
> - BlockInterfaceType block_default_type,
> + BlockInterfaceType type,
> DriveMediaType media)
> {
> const char *buf;
> const char *file = NULL;
> const char *serial;
> const char *mediastr = "";
> - BlockInterfaceType type;
> int bus_id, unit_id;
> int cyls, heads, secs, translation;
> int max_devs;
> @@ -374,17 +373,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
> file = qemu_opt_get(opts, "file");
> serial = qemu_opt_get(opts, "serial");
>
> - if ((buf = qemu_opt_get(opts, "if")) != NULL) {
> - for (type = 0; type < IF_COUNT && strcmp(buf, if_name[type]); type++)
> - ;
> - if (type == IF_COUNT) {
> - error_report("unsupported bus type '%s'", buf);
> - return NULL;
> - }
> - } else {
> - type = block_default_type;
> - }
> -
> max_devs = if_max_devs[type];
>
> if (cyls || heads || secs) {
> @@ -753,6 +741,10 @@ QemuOptsList qemu_legacy_drive_opts = {
> .name = "media",
> .type = QEMU_OPT_STRING,
> .help = "media type (disk, cdrom)",
> + },{
> + .name = "if",
> + .type = QEMU_OPT_STRING,
> + .help = "interface (ide, scsi, sd, mtd, floppy, pflash, virtio)",
> },
> { /* end of list */ }
> },
> @@ -765,6 +757,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> QDict *bs_opts;
> QemuOpts *legacy_opts;
> DriveMediaType media = MEDIA_DISK;
> + BlockInterfaceType type;
> Error *local_err = NULL;
>
> /* Change legacy command line options into QMP ones */
> @@ -839,8 +832,23 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> }
> }
>
> + /* Controller type */
> + value = qemu_opt_get(legacy_opts, "if");
> + if (value) {
> + for (type = 0;
> + type < IF_COUNT && strcmp(value, if_name[type]);
> + type++) {
> + }
> + if (type == IF_COUNT) {
> + error_report("unsupported bus type '%s'", value);
> + return NULL;
I'd suggest "goto fail;" instead.
Max
> + }
> + } else {
> + type = block_default_type;
> + }
> +
> /* Actual block device init: Functionality shared with blockdev-add */
> - dinfo = blockdev_init(bs_opts, block_default_type, media);
> + dinfo = blockdev_init(bs_opts, type, media);
> if (dinfo == NULL) {
> goto fail;
> }
> @@ -2188,10 +2196,6 @@ QemuOptsList qemu_common_drive_opts = {
> .type = QEMU_OPT_NUMBER,
> .help = "unit number (i.e. lun for scsi)",
> },{
> - .name = "if",
> - .type = QEMU_OPT_STRING,
> - .help = "interface (ide, scsi, sd, mtd, floppy, pflash, virtio)",
> - },{
> .name = "index",
> .type = QEMU_OPT_NUMBER,
> .help = "index number",
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH 08/17] blockdev: Move parsing of 'if' option to drive_init
2013-09-20 14:47 ` Max Reitz
@ 2013-09-20 15:04 ` Kevin Wolf
0 siblings, 0 replies; 56+ messages in thread
From: Kevin Wolf @ 2013-09-20 15:04 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel, stefanha, armbru
Am 20.09.2013 um 16:47 hat Max Reitz geschrieben:
> On 2013-09-20 13:54, Kevin Wolf wrote:
> >It's always IF_NONE for blockdev-add.
> >
> >Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >@@ -839,8 +832,23 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> > }
> > }
> >+ /* Controller type */
> >+ value = qemu_opt_get(legacy_opts, "if");
> >+ if (value) {
> >+ for (type = 0;
> >+ type < IF_COUNT && strcmp(value, if_name[type]);
> >+ type++) {
> >+ }
> >+ if (type == IF_COUNT) {
> >+ error_report("unsupported bus type '%s'", value);
> >+ return NULL;
> I'd suggest "goto fail;" instead.
Yup, otherwise we leak legacy_opts. More instances of the same bug follow
throughout the series, I'll fix them all. Thanks.
Kevin
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [Qemu-devel] [PATCH 08/17] blockdev: Move parsing of 'if' option to drive_init
2013-09-20 11:54 ` [Qemu-devel] [PATCH 08/17] blockdev: Move parsing of 'if' " Kevin Wolf
2013-09-20 14:47 ` Max Reitz
@ 2013-09-20 14:50 ` Benoît Canet
1 sibling, 0 replies; 56+ messages in thread
From: Benoît Canet @ 2013-09-20 14:50 UTC (permalink / raw)
To: Kevin Wolf; +Cc: mreitz, qemu-devel, stefanha, armbru
Le Friday 20 Sep 2013 à 13:54:21 (+0200), Kevin Wolf a écrit :
> It's always IF_NONE for blockdev-add.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> blockdev.c | 40 ++++++++++++++++++++++------------------
> 1 file changed, 22 insertions(+), 18 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 5b25d7f..dc3f01a 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -306,14 +306,13 @@ typedef enum { MEDIA_DISK, MEDIA_CDROM } DriveMediaType;
>
> /* Takes the ownership of bs_opts */
> static DriveInfo *blockdev_init(QDict *bs_opts,
> - BlockInterfaceType block_default_type,
> + BlockInterfaceType type,
> DriveMediaType media)
> {
> const char *buf;
> const char *file = NULL;
> const char *serial;
> const char *mediastr = "";
> - BlockInterfaceType type;
> int bus_id, unit_id;
> int cyls, heads, secs, translation;
> int max_devs;
> @@ -374,17 +373,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
> file = qemu_opt_get(opts, "file");
> serial = qemu_opt_get(opts, "serial");
>
> - if ((buf = qemu_opt_get(opts, "if")) != NULL) {
> - for (type = 0; type < IF_COUNT && strcmp(buf, if_name[type]); type++)
> - ;
> - if (type == IF_COUNT) {
> - error_report("unsupported bus type '%s'", buf);
> - return NULL;
> - }
> - } else {
> - type = block_default_type;
> - }
> -
> max_devs = if_max_devs[type];
>
> if (cyls || heads || secs) {
> @@ -753,6 +741,10 @@ QemuOptsList qemu_legacy_drive_opts = {
> .name = "media",
> .type = QEMU_OPT_STRING,
> .help = "media type (disk, cdrom)",
> + },{
> + .name = "if",
> + .type = QEMU_OPT_STRING,
> + .help = "interface (ide, scsi, sd, mtd, floppy, pflash, virtio)",
> },
> { /* end of list */ }
> },
> @@ -765,6 +757,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> QDict *bs_opts;
> QemuOpts *legacy_opts;
> DriveMediaType media = MEDIA_DISK;
> + BlockInterfaceType type;
> Error *local_err = NULL;
>
> /* Change legacy command line options into QMP ones */
> @@ -839,8 +832,23 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> }
> }
>
> + /* Controller type */
> + value = qemu_opt_get(legacy_opts, "if");
> + if (value) {
> + for (type = 0;
> + type < IF_COUNT && strcmp(value, if_name[type]);
> + type++) {
> + }
> + if (type == IF_COUNT) {
> + error_report("unsupported bus type '%s'", value);
> + return NULL;
goto fail; here ?
> + }
> + } else {
> + type = block_default_type;
> + }
> +
> /* Actual block device init: Functionality shared with blockdev-add */
> - dinfo = blockdev_init(bs_opts, block_default_type, media);
> + dinfo = blockdev_init(bs_opts, type, media);
> if (dinfo == NULL) {
> goto fail;
> }
> @@ -2188,10 +2196,6 @@ QemuOptsList qemu_common_drive_opts = {
> .type = QEMU_OPT_NUMBER,
> .help = "unit number (i.e. lun for scsi)",
> },{
> - .name = "if",
> - .type = QEMU_OPT_STRING,
> - .help = "interface (ide, scsi, sd, mtd, floppy, pflash, virtio)",
> - },{
> .name = "index",
> .type = QEMU_OPT_NUMBER,
> .help = "index number",
> --
> 1.8.1.4
>
>
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH 09/17] blockdev: Moving parsing of geometry options to drive_init
2013-09-20 11:54 [Qemu-devel] [PATCH 00/17] blockdev-add QMP command Kevin Wolf
` (7 preceding siblings ...)
2013-09-20 11:54 ` [Qemu-devel] [PATCH 08/17] blockdev: Move parsing of 'if' " Kevin Wolf
@ 2013-09-20 11:54 ` Kevin Wolf
2013-09-20 15:04 ` Benoît Canet
2013-09-20 11:54 ` [Qemu-devel] [PATCH 10/17] blockdev: Move parsing of 'boot' option " Kevin Wolf
` (7 subsequent siblings)
16 siblings, 1 reply; 56+ messages in thread
From: Kevin Wolf @ 2013-09-20 11:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, mreitz
This moves all of the geometry options (cyls/heads/secs/trans) to
drive_init so that they can only be accessed using legacy functions, but
never with anything blockdev-add related.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
blockdev.c | 136 +++++++++++++++++++++++++++++++------------------------------
1 file changed, 69 insertions(+), 67 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index dc3f01a..ba8b6b4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -314,7 +314,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
const char *serial;
const char *mediastr = "";
int bus_id, unit_id;
- int cyls, heads, secs, translation;
int max_devs;
int index;
int ro = 0;
@@ -332,8 +331,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
bool has_driver_specific_opts;
BlockDriver *drv = NULL;
- translation = BIOS_ATA_TRANSLATION_AUTO;
-
/* Check common options by copying from bs_opts to opts, all other options
* stay in bs_opts for processing by bdrv_open(). */
id = qdict_get_try_str(bs_opts, "id");
@@ -362,10 +359,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
unit_id = qemu_opt_get_number(opts, "unit", -1);
index = qemu_opt_get_number(opts, "index", -1);
- cyls = qemu_opt_get_number(opts, "cyls", 0);
- heads = qemu_opt_get_number(opts, "heads", 0);
- secs = qemu_opt_get_number(opts, "secs", 0);
-
snapshot = qemu_opt_get_bool(opts, "snapshot", 0);
ro = qemu_opt_get_bool(opts, "read-only", 0);
copy_on_read = qemu_opt_get_bool(opts, "copy-on-read", false);
@@ -375,46 +368,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
max_devs = if_max_devs[type];
- if (cyls || heads || secs) {
- if (cyls < 1) {
- error_report("invalid physical cyls number");
- return NULL;
- }
- if (heads < 1) {
- error_report("invalid physical heads number");
- return NULL;
- }
- if (secs < 1) {
- error_report("invalid physical secs number");
- return NULL;
- }
- }
-
- if ((buf = qemu_opt_get(opts, "trans")) != NULL) {
- if (!cyls) {
- error_report("'%s' trans must be used with cyls, heads and secs",
- buf);
- return NULL;
- }
- if (!strcmp(buf, "none"))
- translation = BIOS_ATA_TRANSLATION_NONE;
- else if (!strcmp(buf, "lba"))
- translation = BIOS_ATA_TRANSLATION_LBA;
- else if (!strcmp(buf, "auto"))
- translation = BIOS_ATA_TRANSLATION_AUTO;
- else {
- error_report("'%s' invalid translation type", buf);
- return NULL;
- }
- }
-
- if (media == MEDIA_CDROM) {
- if (cyls || secs || heads) {
- error_report("CHS can't be set with media=cdrom");
- return NULL;
- }
- }
-
if ((buf = qemu_opt_get(opts, "discard")) != NULL) {
if (bdrv_parse_discard_flags(buf, &bdrv_flags) != 0) {
error_report("invalid discard option");
@@ -609,10 +562,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
dinfo->type = type;
dinfo->bus = bus_id;
dinfo->unit = unit_id;
- dinfo->cyls = cyls;
- dinfo->heads = heads;
- dinfo->secs = secs;
- dinfo->trans = translation;
dinfo->refcount = 1;
if (serial != NULL) {
dinfo->serial = g_strdup(serial);
@@ -745,6 +694,22 @@ QemuOptsList qemu_legacy_drive_opts = {
.name = "if",
.type = QEMU_OPT_STRING,
.help = "interface (ide, scsi, sd, mtd, floppy, pflash, virtio)",
+ },{
+ .name = "cyls",
+ .type = QEMU_OPT_NUMBER,
+ .help = "number of cylinders (ide disk geometry)",
+ },{
+ .name = "heads",
+ .type = QEMU_OPT_NUMBER,
+ .help = "number of heads (ide disk geometry)",
+ },{
+ .name = "secs",
+ .type = QEMU_OPT_NUMBER,
+ .help = "number of sectors (ide disk geometry)",
+ },{
+ .name = "trans",
+ .type = QEMU_OPT_STRING,
+ .help = "chs translation (auto, lba. none)",
},
{ /* end of list */ }
},
@@ -758,6 +723,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
QemuOpts *legacy_opts;
DriveMediaType media = MEDIA_DISK;
BlockInterfaceType type;
+ int cyls, heads, secs, translation;
Error *local_err = NULL;
/* Change legacy command line options into QMP ones */
@@ -847,6 +813,53 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
type = block_default_type;
}
+ /* Geometry */
+ cyls = qemu_opt_get_number(legacy_opts, "cyls", 0);
+ heads = qemu_opt_get_number(legacy_opts, "heads", 0);
+ secs = qemu_opt_get_number(legacy_opts, "secs", 0);
+
+ if (cyls || heads || secs) {
+ if (cyls < 1) {
+ error_report("invalid physical cyls number");
+ return NULL;
+ }
+ if (heads < 1) {
+ error_report("invalid physical heads number");
+ return NULL;
+ }
+ if (secs < 1) {
+ error_report("invalid physical secs number");
+ return NULL;
+ }
+ }
+
+ translation = BIOS_ATA_TRANSLATION_AUTO;
+ value = qemu_opt_get(legacy_opts, "trans");
+ if (value != NULL) {
+ if (!cyls) {
+ error_report("'%s' trans must be used with cyls, heads and secs",
+ value);
+ return NULL;
+ }
+ if (!strcmp(value, "none")) {
+ translation = BIOS_ATA_TRANSLATION_NONE;
+ } else if (!strcmp(value, "lba")) {
+ translation = BIOS_ATA_TRANSLATION_LBA;
+ } else if (!strcmp(value, "auto")) {
+ translation = BIOS_ATA_TRANSLATION_AUTO;
+ } else {
+ error_report("'%s' invalid translation type", value);
+ return NULL;
+ }
+ }
+
+ if (media == MEDIA_CDROM) {
+ if (cyls || secs || heads) {
+ error_report("CHS can't be set with media=cdrom");
+ return NULL;
+ }
+ }
+
/* Actual block device init: Functionality shared with blockdev-add */
dinfo = blockdev_init(bs_opts, type, media);
if (dinfo == NULL) {
@@ -857,6 +870,11 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
dinfo->enable_auto_del = true;
dinfo->opts = all_opts;
+ dinfo->cyls = cyls;
+ dinfo->heads = heads;
+ dinfo->secs = secs;
+ dinfo->trans = translation;
+
fail:
qemu_opts_del(legacy_opts);
return dinfo;
@@ -2200,22 +2218,6 @@ QemuOptsList qemu_common_drive_opts = {
.type = QEMU_OPT_NUMBER,
.help = "index number",
},{
- .name = "cyls",
- .type = QEMU_OPT_NUMBER,
- .help = "number of cylinders (ide disk geometry)",
- },{
- .name = "heads",
- .type = QEMU_OPT_NUMBER,
- .help = "number of heads (ide disk geometry)",
- },{
- .name = "secs",
- .type = QEMU_OPT_NUMBER,
- .help = "number of sectors (ide disk geometry)",
- },{
- .name = "trans",
- .type = QEMU_OPT_STRING,
- .help = "chs translation (auto, lba. none)",
- },{
.name = "snapshot",
.type = QEMU_OPT_BOOL,
.help = "enable/disable snapshot mode",
--
1.8.1.4
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH 09/17] blockdev: Moving parsing of geometry options to drive_init
2013-09-20 11:54 ` [Qemu-devel] [PATCH 09/17] blockdev: Moving parsing of geometry options " Kevin Wolf
@ 2013-09-20 15:04 ` Benoît Canet
0 siblings, 0 replies; 56+ messages in thread
From: Benoît Canet @ 2013-09-20 15:04 UTC (permalink / raw)
To: Kevin Wolf; +Cc: mreitz, qemu-devel, stefanha, armbru
Le Friday 20 Sep 2013 à 13:54:22 (+0200), Kevin Wolf a écrit :
> This moves all of the geometry options (cyls/heads/secs/trans) to
> drive_init so that they can only be accessed using legacy functions, but
> never with anything blockdev-add related.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> blockdev.c | 136 +++++++++++++++++++++++++++++++------------------------------
> 1 file changed, 69 insertions(+), 67 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index dc3f01a..ba8b6b4 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -314,7 +314,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
> const char *serial;
> const char *mediastr = "";
> int bus_id, unit_id;
> - int cyls, heads, secs, translation;
> int max_devs;
> int index;
> int ro = 0;
> @@ -332,8 +331,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
> bool has_driver_specific_opts;
> BlockDriver *drv = NULL;
>
> - translation = BIOS_ATA_TRANSLATION_AUTO;
> -
> /* Check common options by copying from bs_opts to opts, all other options
> * stay in bs_opts for processing by bdrv_open(). */
> id = qdict_get_try_str(bs_opts, "id");
> @@ -362,10 +359,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
> unit_id = qemu_opt_get_number(opts, "unit", -1);
> index = qemu_opt_get_number(opts, "index", -1);
>
> - cyls = qemu_opt_get_number(opts, "cyls", 0);
> - heads = qemu_opt_get_number(opts, "heads", 0);
> - secs = qemu_opt_get_number(opts, "secs", 0);
> -
> snapshot = qemu_opt_get_bool(opts, "snapshot", 0);
> ro = qemu_opt_get_bool(opts, "read-only", 0);
> copy_on_read = qemu_opt_get_bool(opts, "copy-on-read", false);
> @@ -375,46 +368,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
>
> max_devs = if_max_devs[type];
>
> - if (cyls || heads || secs) {
> - if (cyls < 1) {
> - error_report("invalid physical cyls number");
> - return NULL;
> - }
> - if (heads < 1) {
> - error_report("invalid physical heads number");
> - return NULL;
> - }
> - if (secs < 1) {
> - error_report("invalid physical secs number");
> - return NULL;
> - }
> - }
> -
> - if ((buf = qemu_opt_get(opts, "trans")) != NULL) {
> - if (!cyls) {
> - error_report("'%s' trans must be used with cyls, heads and secs",
> - buf);
> - return NULL;
> - }
> - if (!strcmp(buf, "none"))
> - translation = BIOS_ATA_TRANSLATION_NONE;
> - else if (!strcmp(buf, "lba"))
> - translation = BIOS_ATA_TRANSLATION_LBA;
> - else if (!strcmp(buf, "auto"))
> - translation = BIOS_ATA_TRANSLATION_AUTO;
> - else {
> - error_report("'%s' invalid translation type", buf);
> - return NULL;
> - }
> - }
> -
> - if (media == MEDIA_CDROM) {
> - if (cyls || secs || heads) {
> - error_report("CHS can't be set with media=cdrom");
> - return NULL;
> - }
> - }
> -
> if ((buf = qemu_opt_get(opts, "discard")) != NULL) {
> if (bdrv_parse_discard_flags(buf, &bdrv_flags) != 0) {
> error_report("invalid discard option");
> @@ -609,10 +562,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
> dinfo->type = type;
> dinfo->bus = bus_id;
> dinfo->unit = unit_id;
> - dinfo->cyls = cyls;
> - dinfo->heads = heads;
> - dinfo->secs = secs;
> - dinfo->trans = translation;
> dinfo->refcount = 1;
> if (serial != NULL) {
> dinfo->serial = g_strdup(serial);
> @@ -745,6 +694,22 @@ QemuOptsList qemu_legacy_drive_opts = {
> .name = "if",
> .type = QEMU_OPT_STRING,
> .help = "interface (ide, scsi, sd, mtd, floppy, pflash, virtio)",
> + },{
> + .name = "cyls",
> + .type = QEMU_OPT_NUMBER,
> + .help = "number of cylinders (ide disk geometry)",
> + },{
> + .name = "heads",
> + .type = QEMU_OPT_NUMBER,
> + .help = "number of heads (ide disk geometry)",
> + },{
> + .name = "secs",
> + .type = QEMU_OPT_NUMBER,
> + .help = "number of sectors (ide disk geometry)",
> + },{
> + .name = "trans",
> + .type = QEMU_OPT_STRING,
> + .help = "chs translation (auto, lba. none)",
> },
> { /* end of list */ }
> },
> @@ -758,6 +723,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> QemuOpts *legacy_opts;
> DriveMediaType media = MEDIA_DISK;
> BlockInterfaceType type;
> + int cyls, heads, secs, translation;
> Error *local_err = NULL;
>
> /* Change legacy command line options into QMP ones */
> @@ -847,6 +813,53 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> type = block_default_type;
> }
>
> + /* Geometry */
> + cyls = qemu_opt_get_number(legacy_opts, "cyls", 0);
> + heads = qemu_opt_get_number(legacy_opts, "heads", 0);
> + secs = qemu_opt_get_number(legacy_opts, "secs", 0);
> +
> + if (cyls || heads || secs) {
> + if (cyls < 1) {
> + error_report("invalid physical cyls number");
> + return NULL;
> + }
> + if (heads < 1) {
> + error_report("invalid physical heads number");
> + return NULL;
> + }
> + if (secs < 1) {
> + error_report("invalid physical secs number");
> + return NULL;
> + }
> + }
> +
> + translation = BIOS_ATA_TRANSLATION_AUTO;
> + value = qemu_opt_get(legacy_opts, "trans");
> + if (value != NULL) {
> + if (!cyls) {
> + error_report("'%s' trans must be used with cyls, heads and secs",
> + value);
> + return NULL;
> + }
> + if (!strcmp(value, "none")) {
> + translation = BIOS_ATA_TRANSLATION_NONE;
> + } else if (!strcmp(value, "lba")) {
> + translation = BIOS_ATA_TRANSLATION_LBA;
> + } else if (!strcmp(value, "auto")) {
> + translation = BIOS_ATA_TRANSLATION_AUTO;
> + } else {
> + error_report("'%s' invalid translation type", value);
> + return NULL;
> + }
> + }
> +
> + if (media == MEDIA_CDROM) {
> + if (cyls || secs || heads) {
> + error_report("CHS can't be set with media=cdrom");
> + return NULL;
> + }
> + }
> +
> /* Actual block device init: Functionality shared with blockdev-add */
> dinfo = blockdev_init(bs_opts, type, media);
> if (dinfo == NULL) {
> @@ -857,6 +870,11 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> dinfo->enable_auto_del = true;
> dinfo->opts = all_opts;
>
> + dinfo->cyls = cyls;
> + dinfo->heads = heads;
> + dinfo->secs = secs;
> + dinfo->trans = translation;
> +
> fail:
> qemu_opts_del(legacy_opts);
> return dinfo;
> @@ -2200,22 +2218,6 @@ QemuOptsList qemu_common_drive_opts = {
> .type = QEMU_OPT_NUMBER,
> .help = "index number",
> },{
> - .name = "cyls",
> - .type = QEMU_OPT_NUMBER,
> - .help = "number of cylinders (ide disk geometry)",
> - },{
> - .name = "heads",
> - .type = QEMU_OPT_NUMBER,
> - .help = "number of heads (ide disk geometry)",
> - },{
> - .name = "secs",
> - .type = QEMU_OPT_NUMBER,
> - .help = "number of sectors (ide disk geometry)",
> - },{
> - .name = "trans",
> - .type = QEMU_OPT_STRING,
> - .help = "chs translation (auto, lba. none)",
> - },{
> .name = "snapshot",
> .type = QEMU_OPT_BOOL,
> .help = "enable/disable snapshot mode",
> --
> 1.8.1.4
>
>
Aside from the "goto fail;" impacting multiple patch:
Reviewed-for-C-bugs-by: Benoit Canet <benoit@irqsave.net>
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH 10/17] blockdev: Move parsing of 'boot' option to drive_init
2013-09-20 11:54 [Qemu-devel] [PATCH 00/17] blockdev-add QMP command Kevin Wolf
` (8 preceding siblings ...)
2013-09-20 11:54 ` [Qemu-devel] [PATCH 09/17] blockdev: Moving parsing of geometry options " Kevin Wolf
@ 2013-09-20 11:54 ` Kevin Wolf
2013-09-20 15:05 ` Benoît Canet
2013-09-20 11:54 ` [Qemu-devel] [PATCH 11/17] blockdev: Move bus/unit/index processing " Kevin Wolf
` (6 subsequent siblings)
16 siblings, 1 reply; 56+ messages in thread
From: Kevin Wolf @ 2013-09-20 11:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, mreitz
It's already ignored and only prints a deprecation message. No use in
making it available in new interfaces.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
blockdev.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index ba8b6b4..33383be 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -453,12 +453,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
return NULL;
}
- if (qemu_opt_get(opts, "boot") != NULL) {
- fprintf(stderr, "qemu-kvm: boot=on|off is deprecated and will be "
- "ignored. Future versions will reject this parameter. Please "
- "update your scripts.\n");
- }
-
on_write_error = BLOCKDEV_ON_ERROR_ENOSPC;
if ((buf = qemu_opt_get(opts, "werror")) != NULL) {
if (type != IF_IDE && type != IF_SCSI && type != IF_VIRTIO && type != IF_NONE) {
@@ -710,6 +704,10 @@ QemuOptsList qemu_legacy_drive_opts = {
.name = "trans",
.type = QEMU_OPT_STRING,
.help = "chs translation (auto, lba. none)",
+ },{
+ .name = "boot",
+ .type = QEMU_OPT_BOOL,
+ .help = "(deprecated, ignored)",
},
{ /* end of list */ }
},
@@ -785,6 +783,13 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
goto fail;
}
+ /* Deprecated option boot=[on|off] */
+ if (qemu_opt_get(legacy_opts, "boot") != NULL) {
+ fprintf(stderr, "qemu-kvm: boot=on|off is deprecated and will be "
+ "ignored. Future versions will reject this parameter. Please "
+ "update your scripts.\n");
+ }
+
/* Media type */
value = qemu_opt_get(legacy_opts, "media");
if (value) {
@@ -2325,10 +2330,6 @@ QemuOptsList qemu_common_drive_opts = {
.name = "copy-on-read",
.type = QEMU_OPT_BOOL,
.help = "copy read data from backing file into image file",
- },{
- .name = "boot",
- .type = QEMU_OPT_BOOL,
- .help = "(deprecated, ignored)",
},
{ /* end of list */ }
},
--
1.8.1.4
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH 10/17] blockdev: Move parsing of 'boot' option to drive_init
2013-09-20 11:54 ` [Qemu-devel] [PATCH 10/17] blockdev: Move parsing of 'boot' option " Kevin Wolf
@ 2013-09-20 15:05 ` Benoît Canet
0 siblings, 0 replies; 56+ messages in thread
From: Benoît Canet @ 2013-09-20 15:05 UTC (permalink / raw)
To: Kevin Wolf; +Cc: mreitz, qemu-devel, stefanha, armbru
Le Friday 20 Sep 2013 à 13:54:23 (+0200), Kevin Wolf a écrit :
> It's already ignored and only prints a deprecation message. No use in
> making it available in new interfaces.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> blockdev.c | 21 +++++++++++----------
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index ba8b6b4..33383be 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -453,12 +453,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
> return NULL;
> }
>
> - if (qemu_opt_get(opts, "boot") != NULL) {
> - fprintf(stderr, "qemu-kvm: boot=on|off is deprecated and will be "
> - "ignored. Future versions will reject this parameter. Please "
> - "update your scripts.\n");
> - }
> -
> on_write_error = BLOCKDEV_ON_ERROR_ENOSPC;
> if ((buf = qemu_opt_get(opts, "werror")) != NULL) {
> if (type != IF_IDE && type != IF_SCSI && type != IF_VIRTIO && type != IF_NONE) {
> @@ -710,6 +704,10 @@ QemuOptsList qemu_legacy_drive_opts = {
> .name = "trans",
> .type = QEMU_OPT_STRING,
> .help = "chs translation (auto, lba. none)",
> + },{
> + .name = "boot",
> + .type = QEMU_OPT_BOOL,
> + .help = "(deprecated, ignored)",
> },
> { /* end of list */ }
> },
> @@ -785,6 +783,13 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> goto fail;
> }
>
> + /* Deprecated option boot=[on|off] */
> + if (qemu_opt_get(legacy_opts, "boot") != NULL) {
> + fprintf(stderr, "qemu-kvm: boot=on|off is deprecated and will be "
> + "ignored. Future versions will reject this parameter. Please "
> + "update your scripts.\n");
> + }
> +
> /* Media type */
> value = qemu_opt_get(legacy_opts, "media");
> if (value) {
> @@ -2325,10 +2330,6 @@ QemuOptsList qemu_common_drive_opts = {
> .name = "copy-on-read",
> .type = QEMU_OPT_BOOL,
> .help = "copy read data from backing file into image file",
> - },{
> - .name = "boot",
> - .type = QEMU_OPT_BOOL,
> - .help = "(deprecated, ignored)",
> },
> { /* end of list */ }
> },
> --
> 1.8.1.4
>
>
Reviewed-for-c-bugs-by: Benoit Canet <benoit@irqsave.net>
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH 11/17] blockdev: Move bus/unit/index processing to drive_init
2013-09-20 11:54 [Qemu-devel] [PATCH 00/17] blockdev-add QMP command Kevin Wolf
` (9 preceding siblings ...)
2013-09-20 11:54 ` [Qemu-devel] [PATCH 10/17] blockdev: Move parsing of 'boot' option " Kevin Wolf
@ 2013-09-20 11:54 ` Kevin Wolf
2013-09-23 9:03 ` Max Reitz
2013-09-20 11:54 ` [Qemu-devel] [PATCH 12/17] blockdev: Move virtio-blk device creation " Kevin Wolf
` (5 subsequent siblings)
16 siblings, 1 reply; 56+ messages in thread
From: Kevin Wolf @ 2013-09-20 11:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, mreitz
This requires moving the automatic ID generation at the same time, so
let's do that as well.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
blockdev.c | 157 ++++++++++++++++++++++++++++---------------------------------
1 file changed, 73 insertions(+), 84 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 33383be..26127b3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -312,10 +312,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
const char *buf;
const char *file = NULL;
const char *serial;
- const char *mediastr = "";
- int bus_id, unit_id;
- int max_devs;
- int index;
int ro = 0;
int bdrv_flags = 0;
int on_read_error, on_write_error;
@@ -355,10 +351,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
has_driver_specific_opts = !!qdict_size(bs_opts);
/* extract parameters */
- bus_id = qemu_opt_get_number(opts, "bus", 0);
- unit_id = qemu_opt_get_number(opts, "unit", -1);
- index = qemu_opt_get_number(opts, "index", -1);
-
snapshot = qemu_opt_get_bool(opts, "snapshot", 0);
ro = qemu_opt_get_bool(opts, "read-only", 0);
copy_on_read = qemu_opt_get_bool(opts, "copy-on-read", false);
@@ -366,8 +358,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
file = qemu_opt_get(opts, "file");
serial = qemu_opt_get(opts, "serial");
- max_devs = if_max_devs[type];
-
if ((buf = qemu_opt_get(opts, "discard")) != NULL) {
if (bdrv_parse_discard_flags(buf, &bdrv_flags) != 0) {
error_report("invalid discard option");
@@ -486,66 +476,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
}
}
- /* compute bus and unit according index */
-
- if (index != -1) {
- if (bus_id != 0 || unit_id != -1) {
- error_report("index cannot be used with bus and unit");
- return NULL;
- }
- bus_id = drive_index_to_bus_id(type, index);
- unit_id = drive_index_to_unit_id(type, index);
- }
-
- /* if user doesn't specify a unit_id,
- * try to find the first free
- */
-
- if (unit_id == -1) {
- unit_id = 0;
- while (drive_get(type, bus_id, unit_id) != NULL) {
- unit_id++;
- if (max_devs && unit_id >= max_devs) {
- unit_id -= max_devs;
- bus_id++;
- }
- }
- }
-
- /* check unit id */
-
- if (max_devs && unit_id >= max_devs) {
- error_report("unit %d too big (max is %d)",
- unit_id, max_devs - 1);
- return NULL;
- }
-
- /*
- * catch multiple definitions
- */
-
- if (drive_get(type, bus_id, unit_id) != NULL) {
- error_report("drive with bus=%d, unit=%d (index=%d) exists",
- bus_id, unit_id, index);
- return NULL;
- }
-
- /* no id supplied -> create one */
- if (qemu_opts_id(opts) == NULL) {
- char *new_id;
- if (type == IF_IDE || type == IF_SCSI) {
- mediastr = (media == MEDIA_CDROM) ? "-cd" : "-hd";
- }
- if (max_devs) {
- new_id = g_strdup_printf("%s%i%s%i", if_name[type], bus_id,
- mediastr, unit_id);
- } else {
- new_id = g_strdup_printf("%s%s%i", if_name[type],
- mediastr, unit_id);
- }
- qemu_opts_set_id(opts, new_id);
- }
-
/* init */
dinfo = g_malloc0(sizeof(*dinfo));
dinfo->id = g_strdup(qemu_opts_id(opts));
@@ -554,8 +484,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
dinfo->bdrv->read_only = ro;
dinfo->devaddr = devaddr;
dinfo->type = type;
- dinfo->bus = bus_id;
- dinfo->unit = unit_id;
dinfo->refcount = 1;
if (serial != NULL) {
dinfo->serial = g_strdup(serial);
@@ -681,6 +609,18 @@ QemuOptsList qemu_legacy_drive_opts = {
.head = QTAILQ_HEAD_INITIALIZER(qemu_legacy_drive_opts.head),
.desc = {
{
+ .name = "bus",
+ .type = QEMU_OPT_NUMBER,
+ .help = "bus number",
+ },{
+ .name = "unit",
+ .type = QEMU_OPT_NUMBER,
+ .help = "unit number (i.e. lun for scsi)",
+ },{
+ .name = "index",
+ .type = QEMU_OPT_NUMBER,
+ .help = "index number",
+ },{
.name = "media",
.type = QEMU_OPT_STRING,
.help = "media type (disk, cdrom)",
@@ -722,6 +662,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
DriveMediaType media = MEDIA_DISK;
BlockInterfaceType type;
int cyls, heads, secs, translation;
+ int max_devs, bus_id, unit_id, index;
Error *local_err = NULL;
/* Change legacy command line options into QMP ones */
@@ -865,6 +806,63 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
}
}
+ /* Device address specified by bus/unit or index.
+ * If none was specified, try to find the first free one. */
+ bus_id = qemu_opt_get_number(legacy_opts, "bus", 0);
+ unit_id = qemu_opt_get_number(legacy_opts, "unit", -1);
+ index = qemu_opt_get_number(legacy_opts, "index", -1);
+
+ max_devs = if_max_devs[type];
+
+ if (index != -1) {
+ if (bus_id != 0 || unit_id != -1) {
+ error_report("index cannot be used with bus and unit");
+ goto fail;
+ }
+ bus_id = drive_index_to_bus_id(type, index);
+ unit_id = drive_index_to_unit_id(type, index);
+ }
+
+ if (unit_id == -1) {
+ unit_id = 0;
+ while (drive_get(type, bus_id, unit_id) != NULL) {
+ unit_id++;
+ if (max_devs && unit_id >= max_devs) {
+ unit_id -= max_devs;
+ bus_id++;
+ }
+ }
+ }
+
+ if (max_devs && unit_id >= max_devs) {
+ error_report("unit %d too big (max is %d)", unit_id, max_devs - 1);
+ goto fail;
+ }
+
+ if (drive_get(type, bus_id, unit_id) != NULL) {
+ error_report("drive with bus=%d, unit=%d (index=%d) exists",
+ bus_id, unit_id, index);
+ goto fail;
+ }
+
+ /* no id supplied -> create one */
+ if (qemu_opts_id(all_opts) == NULL) {
+ char *new_id;
+ const char *mediastr = "";
+ if (type == IF_IDE || type == IF_SCSI) {
+ mediastr = (media == MEDIA_CDROM) ? "-cd" : "-hd";
+ }
+ if (max_devs) {
+ new_id = g_strdup_printf("%s%i%s%i", if_name[type], bus_id,
+ mediastr, unit_id);
+ } else {
+ new_id = g_strdup_printf("%s%s%i", if_name[type],
+ mediastr, unit_id);
+ }
+ qdict_put(bs_opts, "id", qstring_from_str(new_id));
+ g_free(new_id);
+ }
+
/* Actual block device init: Functionality shared with blockdev-add */
dinfo = blockdev_init(bs_opts, type, media);
if (dinfo == NULL) {
@@ -880,6 +878,9 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
dinfo->secs = secs;
dinfo->trans = translation;
+ dinfo->bus = bus_id;
+ dinfo->unit = unit_id;
+
fail:
qemu_opts_del(legacy_opts);
return dinfo;
@@ -2211,18 +2212,6 @@ QemuOptsList qemu_common_drive_opts = {
.head = QTAILQ_HEAD_INITIALIZER(qemu_common_drive_opts.head),
.desc = {
{
- .name = "bus",
- .type = QEMU_OPT_NUMBER,
- .help = "bus number",
- },{
- .name = "unit",
- .type = QEMU_OPT_NUMBER,
- .help = "unit number (i.e. lun for scsi)",
- },{
- .name = "index",
- .type = QEMU_OPT_NUMBER,
- .help = "index number",
- },{
.name = "snapshot",
.type = QEMU_OPT_BOOL,
.help = "enable/disable snapshot mode",
--
1.8.1.4
^ permalink raw reply related [flat|nested] 56+ messages in thread* [Qemu-devel] [PATCH 12/17] blockdev: Move virtio-blk device creation to drive_init
2013-09-20 11:54 [Qemu-devel] [PATCH 00/17] blockdev-add QMP command Kevin Wolf
` (10 preceding siblings ...)
2013-09-20 11:54 ` [Qemu-devel] [PATCH 11/17] blockdev: Move bus/unit/index processing " Kevin Wolf
@ 2013-09-20 11:54 ` Kevin Wolf
2013-09-23 9:04 ` Max Reitz
2013-09-20 11:54 ` [Qemu-devel] [PATCH 13/17] blockdev: Remove IF_* check for read-only blockdev_init Kevin Wolf
` (4 subsequent siblings)
16 siblings, 1 reply; 56+ messages in thread
From: Kevin Wolf @ 2013-09-20 11:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, mreitz
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
blockdev.c | 54 +++++++++++++++++++++++++++---------------------------
1 file changed, 27 insertions(+), 27 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 26127b3..bbdae31 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -315,7 +315,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
int ro = 0;
int bdrv_flags = 0;
int on_read_error, on_write_error;
- const char *devaddr;
DriveInfo *dinfo;
ThrottleConfig cfg;
int snapshot = 0;
@@ -469,20 +468,12 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
}
}
- if ((devaddr = qemu_opt_get(opts, "addr")) != NULL) {
- if (type != IF_VIRTIO) {
- error_report("addr is not supported by this bus type");
- return NULL;
- }
- }
-
/* init */
dinfo = g_malloc0(sizeof(*dinfo));
dinfo->id = g_strdup(qemu_opts_id(opts));
dinfo->bdrv = bdrv_new(dinfo->id);
dinfo->bdrv->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
dinfo->bdrv->read_only = ro;
- dinfo->devaddr = devaddr;
dinfo->type = type;
dinfo->refcount = 1;
if (serial != NULL) {
@@ -509,22 +500,8 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
case IF_FLOPPY:
case IF_PFLASH:
case IF_MTD:
- break;
case IF_VIRTIO:
- {
- /* add virtio block device */
- QemuOpts *devopts;
- devopts = qemu_opts_create_nofail(qemu_find_opts("device"));
- if (arch_type == QEMU_ARCH_S390X) {
- qemu_opt_set(devopts, "driver", "virtio-blk-s390");
- } else {
- qemu_opt_set(devopts, "driver", "virtio-blk-pci");
- }
- qemu_opt_set(devopts, "drive", dinfo->id);
- if (devaddr)
- qemu_opt_set(devopts, "addr", devaddr);
break;
- }
default:
abort();
}
@@ -648,6 +625,10 @@ QemuOptsList qemu_legacy_drive_opts = {
.name = "boot",
.type = QEMU_OPT_BOOL,
.help = "(deprecated, ignored)",
+ },{
+ .name = "addr",
+ .type = QEMU_OPT_STRING,
+ .help = "pci address (virtio only)",
},
{ /* end of list */ }
},
@@ -663,6 +644,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
BlockInterfaceType type;
int cyls, heads, secs, translation;
int max_devs, bus_id, unit_id, index;
+ const char *devaddr;
Error *local_err = NULL;
/* Change legacy command line options into QMP ones */
@@ -863,6 +845,27 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
g_free(new_id);
}
+ /* Add virtio block device */
+ devaddr = qemu_opt_get(legacy_opts, "addr");
+ if (devaddr && type != IF_VIRTIO) {
+ error_report("addr is not supported by this bus type");
+ return NULL;
+ }
+
+ if (type == IF_VIRTIO) {
+ QemuOpts *devopts;
+ devopts = qemu_opts_create_nofail(qemu_find_opts("device"));
+ if (arch_type == QEMU_ARCH_S390X) {
+ qemu_opt_set(devopts, "driver", "virtio-blk-s390");
+ } else {
+ qemu_opt_set(devopts, "driver", "virtio-blk-pci");
+ }
+ qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"));
+ if (devaddr) {
+ qemu_opt_set(devopts, "addr", devaddr);
+ }
+ }
+
/* Actual block device init: Functionality shared with blockdev-add */
dinfo = blockdev_init(bs_opts, type, media);
if (dinfo == NULL) {
@@ -880,6 +883,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
dinfo->bus = bus_id;
dinfo->unit = unit_id;
+ dinfo->devaddr = devaddr;
fail:
qemu_opts_del(legacy_opts);
@@ -2256,10 +2260,6 @@ QemuOptsList qemu_common_drive_opts = {
.type = QEMU_OPT_STRING,
.help = "write error action",
},{
- .name = "addr",
- .type = QEMU_OPT_STRING,
- .help = "pci address (virtio only)",
- },{
.name = "read-only",
.type = QEMU_OPT_BOOL,
.help = "open drive file as read-only",
--
1.8.1.4
^ permalink raw reply related [flat|nested] 56+ messages in thread* [Qemu-devel] [PATCH 13/17] blockdev: Remove IF_* check for read-only blockdev_init
2013-09-20 11:54 [Qemu-devel] [PATCH 00/17] blockdev-add QMP command Kevin Wolf
` (11 preceding siblings ...)
2013-09-20 11:54 ` [Qemu-devel] [PATCH 12/17] blockdev: Move virtio-blk device creation " Kevin Wolf
@ 2013-09-20 11:54 ` Kevin Wolf
2013-09-23 8:00 ` Max Reitz
2013-09-20 11:54 ` [Qemu-devel] [PATCH 14/17] qemu-iotests: Check autodel behaviour for device_del Kevin Wolf
` (3 subsequent siblings)
16 siblings, 1 reply; 56+ messages in thread
From: Kevin Wolf @ 2013-09-20 11:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, mreitz
IF_NONE allows read-only, which makes forbidding it in this place
for other types pretty much pointless.
Instead, make sure that all devices for which the check would have
errored out check in their init function that they don't get a read-only
BlockDriverState. This catches even cases where IF_NONE and -device is
used.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
blockdev.c | 6 ------
hw/block/m25p80.c | 5 +++++
hw/block/xen_disk.c | 6 ++++++
hw/sd/milkymist-memcard.c | 4 ++++
hw/sd/omap_mmc.c | 8 ++++++++
hw/sd/pl181.c | 4 ++++
hw/sd/pxa2xx_mmci.c | 4 ++++
hw/sd/sd.c | 5 +++++
hw/sd/sdhci.c | 4 ++++
hw/sd/ssi-sd.c | 3 +++
tests/qemu-iotests/051.out | 5 ++++-
11 files changed, 47 insertions(+), 7 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index bbdae31..ed4ba65 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -529,12 +529,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
if (media == MEDIA_CDROM) {
/* CDROM is fine for any interface, don't check. */
ro = 1;
- } else if (ro == 1) {
- if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY &&
- type != IF_NONE && type != IF_PFLASH) {
- error_report("read-only not supported by this bus type");
- goto err;
- }
}
bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 8c3b7f0..02a1544 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -624,6 +624,11 @@ static int m25p80_init(SSISlave *ss)
if (dinfo && dinfo->bdrv) {
DB_PRINT_L(0, "Binding to IF_MTD drive\n");
s->bdrv = dinfo->bdrv;
+ if (bdrv_is_read_only(s->bdrv)) {
+ fprintf(stderr, "Can't use a read-only drive");
+ return 1;
+ }
+
/* FIXME: Move to late init */
if (bdrv_read(s->bdrv, 0, s->storage, DIV_ROUND_UP(s->size,
BDRV_SECTOR_SIZE))) {
diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index f35fc59..828c9d9 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -39,6 +39,7 @@
#include "hw/xen/xen_backend.h"
#include "xen_blkif.h"
#include "sysemu/blockdev.h"
+#include "qemu/error-report.h"
/* ------------------------------------------------------------- */
@@ -829,6 +830,11 @@ static int blk_connect(struct XenDevice *xendev)
/* setup via qemu cmdline -> already setup for us */
xen_be_printf(&blkdev->xendev, 2, "get configured bdrv (cmdline setup)\n");
blkdev->bs = blkdev->dinfo->bdrv;
+ if (bdrv_is_read_only(blkdev->bs) && !readonly) {
+ error_report("Unexpected read-only drive");
+ blkdev->bs = NULL;
+ return -1;
+ }
/* blkdev->bs is not create by us, we get a reference
* so we can bdrv_unref() unconditionally */
bdrv_ref(blkdev->bs);
diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
index 42613b3..d1168c9 100644
--- a/hw/sd/milkymist-memcard.c
+++ b/hw/sd/milkymist-memcard.c
@@ -255,6 +255,10 @@ static int milkymist_memcard_init(SysBusDevice *dev)
dinfo = drive_get_next(IF_SD);
s->card = sd_init(dinfo ? dinfo->bdrv : NULL, false);
+ if (s->card == NULL) {
+ return -1;
+ }
+
s->enabled = dinfo ? bdrv_is_inserted(dinfo->bdrv) : 0;
memory_region_init_io(&s->regs_region, OBJECT(s), &memcard_mmio_ops, s,
diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
index bf5d1fb..8c6ab2e 100644
--- a/hw/sd/omap_mmc.c
+++ b/hw/sd/omap_mmc.c
@@ -593,6 +593,10 @@ struct omap_mmc_s *omap_mmc_init(hwaddr base,
/* Instantiate the storage */
s->card = sd_init(bd, false);
+ if (s->card == NULL) {
+ fprintf(stderr, "Failed to initialise SD card\n");
+ exit(1);
+ }
return s;
}
@@ -618,6 +622,10 @@ struct omap_mmc_s *omap2_mmc_init(struct omap_target_agent_s *ta,
/* Instantiate the storage */
s->card = sd_init(bd, false);
+ if (s->card == NULL) {
+ fprintf(stderr, "Failed to initialise SD card\n");
+ exit(1);
+ }
s->cdet = qemu_allocate_irqs(omap_mmc_cover_cb, s, 1)[0];
sd_set_cb(s->card, NULL, s->cdet);
diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
index 03875bf..c35896d 100644
--- a/hw/sd/pl181.c
+++ b/hw/sd/pl181.c
@@ -491,6 +491,10 @@ static int pl181_init(SysBusDevice *sbd)
qdev_init_gpio_out(dev, s->cardstatus, 2);
dinfo = drive_get_next(IF_SD);
s->card = sd_init(dinfo ? dinfo->bdrv : NULL, false);
+ if (s->card == NULL) {
+ return -1;
+ }
+
return 0;
}
diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index 90c955f..d720bc2 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -539,6 +539,10 @@ PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
/* Instantiate the actual storage */
s->card = sd_init(bd, false);
+ if (s->card == NULL) {
+ fprintf(stderr, "Failed to initialise SD card\n");
+ exit(1);
+ }
register_savevm(NULL, "pxa2xx_mmci", 0, 0,
pxa2xx_mmci_save, pxa2xx_mmci_load, s);
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 346d86f..7380f06 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -494,6 +494,11 @@ SDState *sd_init(BlockDriverState *bs, bool is_spi)
{
SDState *sd;
+ if (bdrv_is_read_only(bs)) {
+ fprintf(stderr, "sd_init: Cannot use read-only drive\n");
+ return NULL;
+ }
+
sd = (SDState *) g_malloc0(sizeof(SDState));
sd->buf = qemu_blockalign(bs, 512);
sd->spi = is_spi;
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 1483e19..87cd1de 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1166,6 +1166,10 @@ static void sdhci_initfn(Object *obj)
di = drive_get_next(IF_SD);
s->card = sd_init(di ? di->bdrv : NULL, false);
+ if (s->card == NULL) {
+ fprintf(stderr, "Failed to initialise SD card\n");
+ exit(1);
+ }
s->eject_cb = qemu_allocate_irqs(sdhci_insert_eject_cb, s, 1)[0];
s->ro_cb = qemu_allocate_irqs(sdhci_card_readonly_cb, s, 1)[0];
sd_set_cb(s->card, s->ro_cb, s->eject_cb);
diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index d47e237..1bb56c4 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -246,6 +246,9 @@ static int ssi_sd_init(SSISlave *dev)
s->mode = SSI_SD_CMD;
dinfo = drive_get_next(IF_SD);
s->sd = sd_init(dinfo ? dinfo->bdrv : NULL, true);
+ if (s->sd == NULL) {
+ return -1;
+ }
register_savevm(&dev->qdev, "ssi_sd", -1, 1, ssi_sd_save, ssi_sd_load, s);
return 0;
}
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index 88e8fa7..bee161e 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -128,7 +128,10 @@ QEMU X.Y.Z monitor - type 'help' for more information
(qemu) q^[[K^[[Dqu^[[K^[[D^[[Dqui^[[K^[[D^[[D^[[Dquit^[[K
Testing: -drive file=TEST_DIR/t.qcow2,if=ide,readonly=on
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=ide,readonly=on: read-only not supported by this bus type
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) QEMU_PROG: Can't use a read-only drive
+QEMU_PROG: Device initialization failed.
+QEMU_PROG: Initialization of device ide-hd failed
Testing: -drive file=TEST_DIR/t.qcow2,if=virtio,readonly=on
QEMU X.Y.Z monitor - type 'help' for more information
--
1.8.1.4
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH 13/17] blockdev: Remove IF_* check for read-only blockdev_init
2013-09-20 11:54 ` [Qemu-devel] [PATCH 13/17] blockdev: Remove IF_* check for read-only blockdev_init Kevin Wolf
@ 2013-09-23 8:00 ` Max Reitz
2013-09-23 8:08 ` Kevin Wolf
0 siblings, 1 reply; 56+ messages in thread
From: Max Reitz @ 2013-09-23 8:00 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, stefanha, armbru
On 2013-09-20 13:54, Kevin Wolf wrote:
> IF_NONE allows read-only, which makes forbidding it in this place
> for other types pretty much pointless.
>
> Instead, make sure that all devices for which the check would have
> errored out check in their init function that they don't get a read-only
> BlockDriverState. This catches even cases where IF_NONE and -device is
> used.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> blockdev.c | 6 ------
> hw/block/m25p80.c | 5 +++++
> hw/block/xen_disk.c | 6 ++++++
> hw/sd/milkymist-memcard.c | 4 ++++
> hw/sd/omap_mmc.c | 8 ++++++++
> hw/sd/pl181.c | 4 ++++
> hw/sd/pxa2xx_mmci.c | 4 ++++
> hw/sd/sd.c | 5 +++++
> hw/sd/sdhci.c | 4 ++++
> hw/sd/ssi-sd.c | 3 +++
> tests/qemu-iotests/051.out | 5 ++++-
> 11 files changed, 47 insertions(+), 7 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index bbdae31..ed4ba65 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -529,12 +529,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
> if (media == MEDIA_CDROM) {
> /* CDROM is fine for any interface, don't check. */
> ro = 1;
> - } else if (ro == 1) {
> - if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY &&
> - type != IF_NONE && type != IF_PFLASH) {
> - error_report("read-only not supported by this bus type");
> - goto err;
> - }
> }
>
> bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 8c3b7f0..02a1544 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -624,6 +624,11 @@ static int m25p80_init(SSISlave *ss)
> if (dinfo && dinfo->bdrv) {
> DB_PRINT_L(0, "Binding to IF_MTD drive\n");
> s->bdrv = dinfo->bdrv;
> + if (bdrv_is_read_only(s->bdrv)) {
> + fprintf(stderr, "Can't use a read-only drive");
> + return 1;
> + }
> +
> /* FIXME: Move to late init */
> if (bdrv_read(s->bdrv, 0, s->storage, DIV_ROUND_UP(s->size,
> BDRV_SECTOR_SIZE))) {
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index f35fc59..828c9d9 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -39,6 +39,7 @@
> #include "hw/xen/xen_backend.h"
> #include "xen_blkif.h"
> #include "sysemu/blockdev.h"
> +#include "qemu/error-report.h"
>
> /* ------------------------------------------------------------- */
>
> @@ -829,6 +830,11 @@ static int blk_connect(struct XenDevice *xendev)
> /* setup via qemu cmdline -> already setup for us */
> xen_be_printf(&blkdev->xendev, 2, "get configured bdrv (cmdline setup)\n");
> blkdev->bs = blkdev->dinfo->bdrv;
> + if (bdrv_is_read_only(blkdev->bs) && !readonly) {
> + error_report("Unexpected read-only drive");
> + blkdev->bs = NULL;
> + return -1;
> + }
Just out of curiosity, why do you use error_report here but
fprintf(stderr) in most other places?
Max
> /* blkdev->bs is not create by us, we get a reference
> * so we can bdrv_unref() unconditionally */
> bdrv_ref(blkdev->bs);
> diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
> index 42613b3..d1168c9 100644
> --- a/hw/sd/milkymist-memcard.c
> +++ b/hw/sd/milkymist-memcard.c
> @@ -255,6 +255,10 @@ static int milkymist_memcard_init(SysBusDevice *dev)
>
> dinfo = drive_get_next(IF_SD);
> s->card = sd_init(dinfo ? dinfo->bdrv : NULL, false);
> + if (s->card == NULL) {
> + return -1;
> + }
> +
> s->enabled = dinfo ? bdrv_is_inserted(dinfo->bdrv) : 0;
>
> memory_region_init_io(&s->regs_region, OBJECT(s), &memcard_mmio_ops, s,
> diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
> index bf5d1fb..8c6ab2e 100644
> --- a/hw/sd/omap_mmc.c
> +++ b/hw/sd/omap_mmc.c
> @@ -593,6 +593,10 @@ struct omap_mmc_s *omap_mmc_init(hwaddr base,
>
> /* Instantiate the storage */
> s->card = sd_init(bd, false);
> + if (s->card == NULL) {
> + fprintf(stderr, "Failed to initialise SD card\n");
> + exit(1);
> + }
>
> return s;
> }
> @@ -618,6 +622,10 @@ struct omap_mmc_s *omap2_mmc_init(struct omap_target_agent_s *ta,
>
> /* Instantiate the storage */
> s->card = sd_init(bd, false);
> + if (s->card == NULL) {
> + fprintf(stderr, "Failed to initialise SD card\n");
> + exit(1);
> + }
>
> s->cdet = qemu_allocate_irqs(omap_mmc_cover_cb, s, 1)[0];
> sd_set_cb(s->card, NULL, s->cdet);
> diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
> index 03875bf..c35896d 100644
> --- a/hw/sd/pl181.c
> +++ b/hw/sd/pl181.c
> @@ -491,6 +491,10 @@ static int pl181_init(SysBusDevice *sbd)
> qdev_init_gpio_out(dev, s->cardstatus, 2);
> dinfo = drive_get_next(IF_SD);
> s->card = sd_init(dinfo ? dinfo->bdrv : NULL, false);
> + if (s->card == NULL) {
> + return -1;
> + }
> +
> return 0;
> }
>
> diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
> index 90c955f..d720bc2 100644
> --- a/hw/sd/pxa2xx_mmci.c
> +++ b/hw/sd/pxa2xx_mmci.c
> @@ -539,6 +539,10 @@ PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
>
> /* Instantiate the actual storage */
> s->card = sd_init(bd, false);
> + if (s->card == NULL) {
> + fprintf(stderr, "Failed to initialise SD card\n");
> + exit(1);
> + }
>
> register_savevm(NULL, "pxa2xx_mmci", 0, 0,
> pxa2xx_mmci_save, pxa2xx_mmci_load, s);
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 346d86f..7380f06 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -494,6 +494,11 @@ SDState *sd_init(BlockDriverState *bs, bool is_spi)
> {
> SDState *sd;
>
> + if (bdrv_is_read_only(bs)) {
> + fprintf(stderr, "sd_init: Cannot use read-only drive\n");
> + return NULL;
> + }
> +
> sd = (SDState *) g_malloc0(sizeof(SDState));
> sd->buf = qemu_blockalign(bs, 512);
> sd->spi = is_spi;
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 1483e19..87cd1de 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1166,6 +1166,10 @@ static void sdhci_initfn(Object *obj)
>
> di = drive_get_next(IF_SD);
> s->card = sd_init(di ? di->bdrv : NULL, false);
> + if (s->card == NULL) {
> + fprintf(stderr, "Failed to initialise SD card\n");
> + exit(1);
> + }
> s->eject_cb = qemu_allocate_irqs(sdhci_insert_eject_cb, s, 1)[0];
> s->ro_cb = qemu_allocate_irqs(sdhci_card_readonly_cb, s, 1)[0];
> sd_set_cb(s->card, s->ro_cb, s->eject_cb);
> diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
> index d47e237..1bb56c4 100644
> --- a/hw/sd/ssi-sd.c
> +++ b/hw/sd/ssi-sd.c
> @@ -246,6 +246,9 @@ static int ssi_sd_init(SSISlave *dev)
> s->mode = SSI_SD_CMD;
> dinfo = drive_get_next(IF_SD);
> s->sd = sd_init(dinfo ? dinfo->bdrv : NULL, true);
> + if (s->sd == NULL) {
> + return -1;
> + }
> register_savevm(&dev->qdev, "ssi_sd", -1, 1, ssi_sd_save, ssi_sd_load, s);
> return 0;
> }
> diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
> index 88e8fa7..bee161e 100644
> --- a/tests/qemu-iotests/051.out
> +++ b/tests/qemu-iotests/051.out
> @@ -128,7 +128,10 @@ QEMU X.Y.Z monitor - type 'help' for more information
> (qemu) q^[[K^[[Dqu^[[K^[[D^[[Dqui^[[K^[[D^[[D^[[Dquit^[[K
>
> Testing: -drive file=TEST_DIR/t.qcow2,if=ide,readonly=on
> -QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=ide,readonly=on: read-only not supported by this bus type
> +QEMU X.Y.Z monitor - type 'help' for more information
> +(qemu) QEMU_PROG: Can't use a read-only drive
> +QEMU_PROG: Device initialization failed.
> +QEMU_PROG: Initialization of device ide-hd failed
>
> Testing: -drive file=TEST_DIR/t.qcow2,if=virtio,readonly=on
> QEMU X.Y.Z monitor - type 'help' for more information
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH 13/17] blockdev: Remove IF_* check for read-only blockdev_init
2013-09-23 8:00 ` Max Reitz
@ 2013-09-23 8:08 ` Kevin Wolf
0 siblings, 0 replies; 56+ messages in thread
From: Kevin Wolf @ 2013-09-23 8:08 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel, stefanha, armbru
Am 23.09.2013 um 10:00 hat Max Reitz geschrieben:
> On 2013-09-20 13:54, Kevin Wolf wrote:
> >IF_NONE allows read-only, which makes forbidding it in this place
> >for other types pretty much pointless.
> >
> >Instead, make sure that all devices for which the check would have
> >errored out check in their init function that they don't get a read-only
> >BlockDriverState. This catches even cases where IF_NONE and -device is
> >used.
> >
> >Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >---
> > blockdev.c | 6 ------
> > hw/block/m25p80.c | 5 +++++
> > hw/block/xen_disk.c | 6 ++++++
> > hw/sd/milkymist-memcard.c | 4 ++++
> > hw/sd/omap_mmc.c | 8 ++++++++
> > hw/sd/pl181.c | 4 ++++
> > hw/sd/pxa2xx_mmci.c | 4 ++++
> > hw/sd/sd.c | 5 +++++
> > hw/sd/sdhci.c | 4 ++++
> > hw/sd/ssi-sd.c | 3 +++
> > tests/qemu-iotests/051.out | 5 ++++-
> > 11 files changed, 47 insertions(+), 7 deletions(-)
> >
> >diff --git a/blockdev.c b/blockdev.c
> >index bbdae31..ed4ba65 100644
> >--- a/blockdev.c
> >+++ b/blockdev.c
> >@@ -529,12 +529,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
> > if (media == MEDIA_CDROM) {
> > /* CDROM is fine for any interface, don't check. */
> > ro = 1;
> >- } else if (ro == 1) {
> >- if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY &&
> >- type != IF_NONE && type != IF_PFLASH) {
> >- error_report("read-only not supported by this bus type");
> >- goto err;
> >- }
> > }
> > bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
> >diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> >index 8c3b7f0..02a1544 100644
> >--- a/hw/block/m25p80.c
> >+++ b/hw/block/m25p80.c
> >@@ -624,6 +624,11 @@ static int m25p80_init(SSISlave *ss)
> > if (dinfo && dinfo->bdrv) {
> > DB_PRINT_L(0, "Binding to IF_MTD drive\n");
> > s->bdrv = dinfo->bdrv;
> >+ if (bdrv_is_read_only(s->bdrv)) {
> >+ fprintf(stderr, "Can't use a read-only drive");
> >+ return 1;
> >+ }
> >+
> > /* FIXME: Move to late init */
> > if (bdrv_read(s->bdrv, 0, s->storage, DIV_ROUND_UP(s->size,
> > BDRV_SECTOR_SIZE))) {
> >diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> >index f35fc59..828c9d9 100644
> >--- a/hw/block/xen_disk.c
> >+++ b/hw/block/xen_disk.c
> >@@ -39,6 +39,7 @@
> > #include "hw/xen/xen_backend.h"
> > #include "xen_blkif.h"
> > #include "sysemu/blockdev.h"
> >+#include "qemu/error-report.h"
> > /* ------------------------------------------------------------- */
> >@@ -829,6 +830,11 @@ static int blk_connect(struct XenDevice *xendev)
> > /* setup via qemu cmdline -> already setup for us */
> > xen_be_printf(&blkdev->xendev, 2, "get configured bdrv (cmdline setup)\n");
> > blkdev->bs = blkdev->dinfo->bdrv;
> >+ if (bdrv_is_read_only(blkdev->bs) && !readonly) {
> >+ error_report("Unexpected read-only drive");
> >+ blkdev->bs = NULL;
> >+ return -1;
> >+ }
> Just out of curiosity, why do you use error_report here but
> fprintf(stderr) in most other places?
Variatio delectat. ;-)
I guess fprintf + exit was a common pattern elsewhere, so I used it,
whereas error_report() is my default function. But now that I look at
this file again, maybe it should really be xen_be_printf() here...
Kevin
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH 14/17] qemu-iotests: Check autodel behaviour for device_del
2013-09-20 11:54 [Qemu-devel] [PATCH 00/17] blockdev-add QMP command Kevin Wolf
` (12 preceding siblings ...)
2013-09-20 11:54 ` [Qemu-devel] [PATCH 13/17] blockdev: Remove IF_* check for read-only blockdev_init Kevin Wolf
@ 2013-09-20 11:54 ` Kevin Wolf
2013-09-23 9:06 ` Max Reitz
2013-09-20 11:54 ` [Qemu-devel] [PATCH 15/17] blockdev: Remove 'media' parameter from blockdev_init() Kevin Wolf
` (2 subsequent siblings)
16 siblings, 1 reply; 56+ messages in thread
From: Kevin Wolf @ 2013-09-20 11:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, mreitz
Block devices creates with -drive and drive_add should automatically
disappear if the guest device is unplugged. blockdev-add ones shouldn't.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
tests/qemu-iotests/064 | 133 +++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/064.out | 80 +++++++++++++++++++++++
tests/qemu-iotests/common.filter | 8 +++
tests/qemu-iotests/group | 1 +
4 files changed, 222 insertions(+)
create mode 100755 tests/qemu-iotests/064
create mode 100644 tests/qemu-iotests/064.out
diff --git a/tests/qemu-iotests/064 b/tests/qemu-iotests/064
new file mode 100755
index 0000000..79dc38b
--- /dev/null
+++ b/tests/qemu-iotests/064
@@ -0,0 +1,133 @@
+#!/bin/bash
+#
+# Test automatic deletion of BDSes created by -drive/drive_add
+#
+# Copyright (C) 2013 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=kwolf@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1 # failure is the default!
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+function do_run_qemu()
+{
+ echo Testing: "$@"
+ $QEMU -nographic -qmp stdio -serial none "$@"
+ echo
+}
+
+function run_qemu()
+{
+ do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp
+}
+
+size=128M
+
+_make_test_img $size
+
+echo
+echo === -drive/-device and device_del ===
+echo
+
+run_qemu -drive file=$TEST_IMG,format=$IMGFMT,if=none,id=disk -device virtio-blk-pci,drive=disk,id=virtio0 <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "query-block" }
+{ "execute": "device_del", "arguments": { "id": "virtio0" } }
+{ "execute": "system_reset" }
+{ "execute": "query-block" }
+{ "execute": "quit" }
+EOF
+
+echo
+echo === -drive/device_add and device_del ===
+echo
+
+run_qemu -drive file=$TEST_IMG,format=$IMGFMT,if=none,id=disk <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "query-block" }
+{ "execute": "device_add",
+ "arguments": { "driver": "virtio-blk-pci", "drive": "disk",
+ "id": "virtio0" } }
+{ "execute": "device_del", "arguments": { "id": "virtio0" } }
+{ "execute": "system_reset" }
+{ "execute": "query-block" }
+{ "execute": "quit" }
+EOF
+
+echo
+echo === drive_add/device_add and device_del ===
+echo
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "human-monitor-command",
+ "arguments": { "command-line": "drive_add 0 file=$TEST_IMG,format=$IMGFMT,if=none,id=disk" } }
+{ "execute": "query-block" }
+{ "execute": "device_add",
+ "arguments": { "driver": "virtio-blk-pci", "drive": "disk",
+ "id": "virtio0" } }
+{ "execute": "device_del", "arguments": { "id": "virtio0" } }
+{ "execute": "system_reset" }
+{ "execute": "query-block" }
+{ "execute": "quit" }
+EOF
+
+echo
+echo === blockdev_add/device_add and device_del ===
+echo
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "blockdev-add",
+ "arguments": {
+ "options": {
+ "driver": "$IMGFMT",
+ "id": "disk",
+ "file": {
+ "driver": "file",
+ "filename": "$TEST_IMG"
+ }
+ }
+ }
+ }
+{ "execute": "query-block" }
+{ "execute": "device_add",
+ "arguments": { "driver": "virtio-blk-pci", "drive": "disk",
+ "id": "virtio0" } }
+{ "execute": "device_del", "arguments": { "id": "virtio0" } }
+{ "execute": "system_reset" }
+{ "execute": "query-block" }
+{ "execute": "quit" }
+EOF
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/064.out b/tests/qemu-iotests/064.out
new file mode 100644
index 0000000..8c7a334
--- /dev/null
+++ b/tests/qemu-iotests/064.out
@@ -0,0 +1,80 @@
+QA output created by 064
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+
+=== -drive/-device and device_del ===
+
+Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,if=none,id=disk -device virtio-blk-pci,drive=disk,id=virtio0
+QMP_VERSION
+{"return": {}}
+{"return": [{"io-status": "ok", "device": "disk", "locked": false, "removable": false, "inserted": {"iops_rd": 0, "image": {"virtual-size": 134217728, "filename": "TEST_DIR/t.qcow2", "cluster-size": 65536, "format": "qcow2", "actual-size": 139264, "dirty-flag": false}, "iops_wr": 0, "ro": false, "backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0, "encrypted": false, "bps": 0, "bps_rd": 0, "file": "TEST_DIR/t.qcow2", "encryption_key_missing": false}, "type": "unknown"}, {"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}]}
+{"return": {}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_DELETED", "data": {"path": "/machine/peripheral/virtio0/virtio-backend"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_DELETED", "data": {"device": "virtio0", "path": "/machine/peripheral/virtio0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "RESET"}
+{"return": [{"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}]}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}}
+
+
+=== -drive/device_add and device_del ===
+
+Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,if=none,id=disk
+QMP_VERSION
+{"return": {}}
+{"return": [{"device": "disk", "locked": false, "removable": true, "inserted": {"iops_rd": 0, "image": {"virtual-size": 134217728, "filename": "TEST_DIR/t.qcow2", "cluster-size": 65536, "format": "qcow2", "actual-size": 139264, "dirty-flag": false}, "iops_wr": 0, "ro": false, "backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0, "encrypted": false, "bps": 0, "bps_rd": 0, "file": "TEST_DIR/t.qcow2", "encryption_key_missing": false}, "tray_open": false, "type": "unknown"}, {"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}]}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_DELETED", "data": {"path": "/machine/peripheral/virtio0/virtio-backend"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_DELETED", "data": {"device": "virtio0", "path": "/machine/peripheral/virtio0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "RESET"}
+{"return": [{"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}]}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}}
+
+
+=== drive_add/device_add and device_del ===
+
+Testing:
+QMP_VERSION
+{"return": {}}
+{"return": "OK\r\n"}
+{"return": [{"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "disk", "locked": false, "removable": true, "inserted": {"iops_rd": 0, "image": {"virtual-size": 134217728, "filename": "TEST_DIR/t.qcow2", "cluster-size": 65536, "format": "qcow2", "actual-size": 139264, "dirty-flag": false}, "iops_wr": 0, "ro": false, "backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0, "encrypted": false, "bps": 0, "bps_rd": 0, "file": "TEST_DIR/t.qcow2", "encryption_key_missing": false}, "tray_open": false, "type": "unknown"}]}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_DELETED", "data": {"path": "/machine/peripheral/virtio0/virtio-backend"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_DELETED", "data": {"device": "virtio0", "path": "/machine/peripheral/virtio0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "RESET"}
+{"return": [{"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}]}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}}
+
+
+=== blockdev_add/device_add and device_del ===
+
+Testing:
+QMP_VERSION
+{"return": {}}
+{"return": {}}
+{"return": [{"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "disk", "locked": false, "removable": true, "inserted": {"iops_rd": 0, "image": {"virtual-size": 134217728, "filename": "TEST_DIR/t.qcow2", "cluster-size": 65536, "format": "qcow2", "actual-size": 139264, "dirty-flag": false}, "iops_wr": 0, "ro": false, "backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0, "encrypted": false, "bps": 0, "bps_rd": 0, "file": "TEST_DIR/t.qcow2", "encryption_key_missing": false}, "tray_open": false, "type": "unknown"}]}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_DELETED", "data": {"path": "/machine/peripheral/virtio0/virtio-backend"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_DELETED", "data": {"device": "virtio0", "path": "/machine/peripheral/virtio0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "RESET"}
+{"return": [{"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"io-status": "ok", "device": "disk", "locked": false, "removable": true, "inserted": {"iops_rd": 0, "image": {"virtual-size": 134217728, "filename": "TEST_DIR/t.qcow2", "cluster-size": 65536, "format": "qcow2", "actual-size": 139264, "dirty-flag": false}, "iops_wr": 0, "ro": false, "backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0, "encrypted": false, "bps": 0, "bps_rd": 0, "file": "TEST_DIR/t.qcow2", "encryption_key_missing": false}, "tray_open": false, "type": "unknown"}]}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}}
+
+*** done
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 5dfda63..8e7b1a4 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -159,5 +159,13 @@ _filter_qemu()
-e 's#^QEMU [0-9]\+\.[0-9]\+\.[0-9]\+ monitor#QEMU X.Y.Z monitor#'
}
+# replace problematic QMP output like timestamps
+_filter_qmp()
+{
+ _filter_win32 | \
+ sed -e 's#\("\(micro\)\?seconds": \)[0-9]\+#\1 TIMESTAMP#g' \
+ -e 's#^{"QMP":.*}$#QMP_VERSION#'
+}
+
# make sure this script returns success
/bin/true
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 1ad02e5..9e950fd 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -69,3 +69,4 @@
061 rw auto
062 rw auto
063 rw auto
+064 rw auto
--
1.8.1.4
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [Qemu-devel] [PATCH 14/17] qemu-iotests: Check autodel behaviour for device_del
2013-09-20 11:54 ` [Qemu-devel] [PATCH 14/17] qemu-iotests: Check autodel behaviour for device_del Kevin Wolf
@ 2013-09-23 9:06 ` Max Reitz
0 siblings, 0 replies; 56+ messages in thread
From: Max Reitz @ 2013-09-23 9:06 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, stefanha, armbru
On 2013-09-20 13:54, Kevin Wolf wrote:
> Block devices creates with -drive and drive_add should automatically
> disappear if the guest device is unplugged. blockdev-add ones shouldn't.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> tests/qemu-iotests/064 | 133 +++++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/064.out | 80 +++++++++++++++++++++++
> tests/qemu-iotests/common.filter | 8 +++
> tests/qemu-iotests/group | 1 +
> 4 files changed, 222 insertions(+)
> create mode 100755 tests/qemu-iotests/064
> create mode 100644 tests/qemu-iotests/064.out
With manual CR reconstruction:
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 56+ messages in thread
* [Qemu-devel] [PATCH 15/17] blockdev: Remove 'media' parameter from blockdev_init()
2013-09-20 11:54 [Qemu-devel] [PATCH 00/17] blockdev-add QMP command Kevin Wolf
` (13 preceding siblings ...)
2013-09-20 11:54 ` [Qemu-devel] [PATCH 14/17] qemu-iotests: Check autodel behaviour for device_del Kevin Wolf
@ 2013-09-20 11:54 ` Kevin Wolf
2013-09-23 9:06 ` Max Reitz
2013-09-20 11:54 ` [Qemu-devel] [PATCH 16/17] blockdev: Don't disable COR automatically with blockdev-add Kevin Wolf
2013-09-20 11:54 ` [Qemu-devel] [PATCH 17/17] blockdev: blockdev_init() error conversion Kevin Wolf
16 siblings, 1 reply; 56+ messages in thread
From: Kevin Wolf @ 2013-09-20 11:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, mreitz
The remaining users shouldn't be there with blockdev-add and are easy to
move to drive_init().
Bonus bug fix: As a side effect, CD-ROM drives can now use block drivers
on the read-only whitelist without explicitly specifying read-only=on,
even if a format is explicitly specified.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
blockdev.c | 40 +++++++++++++++-------------------------
1 file changed, 15 insertions(+), 25 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index ed4ba65..afdeb34 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -306,8 +306,7 @@ typedef enum { MEDIA_DISK, MEDIA_CDROM } DriveMediaType;
/* Takes the ownership of bs_opts */
static DriveInfo *blockdev_init(QDict *bs_opts,
- BlockInterfaceType type,
- DriveMediaType media)
+ BlockInterfaceType type)
{
const char *buf;
const char *file = NULL;
@@ -489,22 +488,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
bdrv_set_io_limits(dinfo->bdrv, &cfg);
}
- switch(type) {
- case IF_IDE:
- case IF_SCSI:
- case IF_XEN:
- case IF_NONE:
- dinfo->media_cd = media == MEDIA_CDROM;
- break;
- case IF_SD:
- case IF_FLOPPY:
- case IF_PFLASH:
- case IF_MTD:
- case IF_VIRTIO:
- break;
- default:
- abort();
- }
if (!file || !*file) {
if (has_driver_specific_opts) {
file = NULL;
@@ -526,11 +509,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
bdrv_flags |= BDRV_O_INCOMING;
}
- if (media == MEDIA_CDROM) {
- /* CDROM is fine for any interface, don't check. */
- ro = 1;
- }
-
bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
if (ro && copy_on_read) {
@@ -714,6 +692,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
media = MEDIA_DISK;
} else if (!strcmp(value, "cdrom")) {
media = MEDIA_CDROM;
+ qdict_put(bs_opts, "read-only", qstring_from_str("on"));
} else {
error_report("'%s' invalid media", value);
goto fail;
@@ -861,7 +840,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
}
/* Actual block device init: Functionality shared with blockdev-add */
- dinfo = blockdev_init(bs_opts, type, media);
+ dinfo = blockdev_init(bs_opts, type);
if (dinfo == NULL) {
goto fail;
}
@@ -879,6 +858,17 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
dinfo->unit = unit_id;
dinfo->devaddr = devaddr;
+ switch(type) {
+ case IF_IDE:
+ case IF_SCSI:
+ case IF_XEN:
+ case IF_NONE:
+ dinfo->media_cd = media == MEDIA_CDROM;
+ break;
+ default:
+ break;
+ }
+
fail:
qemu_opts_del(legacy_opts);
return dinfo;
@@ -2173,7 +2163,7 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
qdict_flatten(qdict);
- dinfo = blockdev_init(qdict, IF_NONE, MEDIA_DISK);
+ dinfo = blockdev_init(qdict, IF_NONE);
if (!dinfo) {
error_setg(errp, "Could not open image");
goto fail;
--
1.8.1.4
^ permalink raw reply related [flat|nested] 56+ messages in thread* [Qemu-devel] [PATCH 16/17] blockdev: Don't disable COR automatically with blockdev-add
2013-09-20 11:54 [Qemu-devel] [PATCH 00/17] blockdev-add QMP command Kevin Wolf
` (14 preceding siblings ...)
2013-09-20 11:54 ` [Qemu-devel] [PATCH 15/17] blockdev: Remove 'media' parameter from blockdev_init() Kevin Wolf
@ 2013-09-20 11:54 ` Kevin Wolf
2013-09-23 9:07 ` Max Reitz
2013-09-20 11:54 ` [Qemu-devel] [PATCH 17/17] blockdev: blockdev_init() error conversion Kevin Wolf
16 siblings, 1 reply; 56+ messages in thread
From: Kevin Wolf @ 2013-09-20 11:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, mreitz
If a read-only device is configured with copy-on-read=on, the old code
only prints a warning and automatically disables copy on read. Make it
a real error for blockdev-add.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 9 +++++++--
blockdev.c | 31 +++++++++++++++++++++++++++----
2 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/block.c b/block.c
index e176c6f..adafa3d 100644
--- a/block.c
+++ b/block.c
@@ -774,8 +774,13 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
}
assert(bs->copy_on_read == 0); /* bdrv_new() and bdrv_close() make it so */
- if (!bs->read_only && (flags & BDRV_O_COPY_ON_READ)) {
- bdrv_enable_copy_on_read(bs);
+ if (flags & BDRV_O_COPY_ON_READ) {
+ if (!bs->read_only) {
+ bdrv_enable_copy_on_read(bs);
+ } else {
+ error_setg(errp, "Can't use copy-on-read on read-only device");
+ return -EINVAL;
+ }
}
if (filename != NULL) {
diff --git a/blockdev.c b/blockdev.c
index afdeb34..ef00c6d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -511,10 +511,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
- if (ro && copy_on_read) {
- error_report("warning: disabling copy_on_read on read-only drive");
- }
-
QINCREF(bs_opts);
ret = bdrv_open(dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error);
@@ -602,6 +598,18 @@ QemuOptsList qemu_legacy_drive_opts = {
.type = QEMU_OPT_STRING,
.help = "pci address (virtio only)",
},
+
+ /* Options that are passed on, but have special semantics with -drive */
+ {
+ .name = "read-only",
+ .type = QEMU_OPT_BOOL,
+ .help = "open drive file as read-only",
+ },{
+ .name = "copy-on-read",
+ .type = QEMU_OPT_BOOL,
+ .help = "copy read data from backing file into image file",
+ },
+
{ /* end of list */ }
},
};
@@ -617,6 +625,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
int cyls, heads, secs, translation;
int max_devs, bus_id, unit_id, index;
const char *devaddr;
+ bool read_only, copy_on_read;
Error *local_err = NULL;
/* Change legacy command line options into QMP ones */
@@ -699,6 +708,20 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
}
}
+ /* copy-on-read is disabled with a warning for read-only devices */
+ read_only = qemu_opt_get_bool(legacy_opts, "read-only", false);
+ copy_on_read = qemu_opt_get_bool(legacy_opts, "copy-on-read", false);
+
+ if (read_only && copy_on_read) {
+ error_report("warning: disabling copy-on-read on read-only drive");
+ copy_on_read = false;
+ }
+
+ qdict_put(bs_opts, "read-only",
+ qstring_from_str(read_only ? "on" : "off"));
+ qdict_put(bs_opts, "copy-on-read",
+ qstring_from_str(copy_on_read ? "on" :"off"));
+
/* Controller type */
value = qemu_opt_get(legacy_opts, "if");
if (value) {
--
1.8.1.4
^ permalink raw reply related [flat|nested] 56+ messages in thread* [Qemu-devel] [PATCH 17/17] blockdev: blockdev_init() error conversion
2013-09-20 11:54 [Qemu-devel] [PATCH 00/17] blockdev-add QMP command Kevin Wolf
` (15 preceding siblings ...)
2013-09-20 11:54 ` [Qemu-devel] [PATCH 16/17] blockdev: Don't disable COR automatically with blockdev-add Kevin Wolf
@ 2013-09-20 11:54 ` Kevin Wolf
2013-09-23 9:08 ` Max Reitz
16 siblings, 1 reply; 56+ messages in thread
From: Kevin Wolf @ 2013-09-20 11:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha, mreitz
This gives us meaningful error messages for the blockdev-add QMP
command.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
blockdev.c | 59 +++++++++++++++++++++++++++++++++--------------------------
1 file changed, 33 insertions(+), 26 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index ef00c6d..fd2ef7a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -269,7 +269,7 @@ static void bdrv_put_ref_bh_schedule(BlockDriverState *bs)
qemu_bh_schedule(s->bh);
}
-static int parse_block_error_action(const char *buf, bool is_read)
+static int parse_block_error_action(const char *buf, bool is_read, Error **errp)
{
if (!strcmp(buf, "ignore")) {
return BLOCKDEV_ON_ERROR_IGNORE;
@@ -280,8 +280,8 @@ static int parse_block_error_action(const char *buf, bool is_read)
} else if (!strcmp(buf, "report")) {
return BLOCKDEV_ON_ERROR_REPORT;
} else {
- error_report("'%s' invalid %s error action",
- buf, is_read ? "read" : "write");
+ error_setg(errp, "'%s' invalid %s error action",
+ buf, is_read ? "read" : "write");
return -1;
}
}
@@ -306,7 +306,8 @@ typedef enum { MEDIA_DISK, MEDIA_CDROM } DriveMediaType;
/* Takes the ownership of bs_opts */
static DriveInfo *blockdev_init(QDict *bs_opts,
- BlockInterfaceType type)
+ BlockInterfaceType type,
+ Error **errp)
{
const char *buf;
const char *file = NULL;
@@ -330,15 +331,13 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
id = qdict_get_try_str(bs_opts, "id");
opts = qemu_opts_create(&qemu_common_drive_opts, id, 1, &error);
if (error_is_set(&error)) {
- qerror_report_err(error);
- error_free(error);
+ error_propagate(errp, error);
return NULL;
}
qemu_opts_absorb_qdict(opts, bs_opts, &error);
if (error_is_set(&error)) {
- qerror_report_err(error);
- error_free(error);
+ error_propagate(errp, error);
return NULL;
}
@@ -358,7 +357,7 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
if ((buf = qemu_opt_get(opts, "discard")) != NULL) {
if (bdrv_parse_discard_flags(buf, &bdrv_flags) != 0) {
- error_report("invalid discard option");
+ error_setg(errp, "invalid discard option");
return NULL;
}
}
@@ -380,7 +379,7 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
} else if (!strcmp(buf, "threads")) {
/* this is the default */
} else {
- error_report("invalid aio option");
+ error_setg(errp, "invalid aio option");
return NULL;
}
}
@@ -397,9 +396,10 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
drv = bdrv_find_whitelisted_format(buf, ro);
if (!drv) {
if (!ro && bdrv_find_whitelisted_format(buf, !ro)) {
- error_report("'%s' can be only used as read-only device.", buf);
+ error_setg(errp, "'%s' can be only used as read-only device.",
+ buf);
} else {
- error_report("'%s' invalid format", buf);
+ error_setg(errp, "'%s' invalid format", buf);
}
return NULL;
}
@@ -436,20 +436,20 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
cfg.op_size = qemu_opt_get_number(opts, "throttling.iops-size", 0);
if (!check_throttle_config(&cfg, &error)) {
- error_report("%s", error_get_pretty(error));
- error_free(error);
+ error_propagate(errp, error);
return NULL;
}
on_write_error = BLOCKDEV_ON_ERROR_ENOSPC;
if ((buf = qemu_opt_get(opts, "werror")) != NULL) {
if (type != IF_IDE && type != IF_SCSI && type != IF_VIRTIO && type != IF_NONE) {
- error_report("werror is not supported by this bus type");
+ error_setg(errp, "werror is not supported by this bus type");
return NULL;
}
- on_write_error = parse_block_error_action(buf, 0);
- if (on_write_error < 0) {
+ on_write_error = parse_block_error_action(buf, 0, &error);
+ if (error_is_set(&error)) {
+ error_propagate(errp, error);
return NULL;
}
}
@@ -461,8 +461,9 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
return NULL;
}
- on_read_error = parse_block_error_action(buf, 1);
- if (on_read_error < 0) {
+ on_read_error = parse_block_error_action(buf, 1, &error);
+ if (error_is_set(&error)) {
+ error_propagate(errp, error);
return NULL;
}
}
@@ -515,8 +516,9 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
ret = bdrv_open(dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error);
if (ret < 0) {
- error_report("could not open disk image %s: %s",
- file ?: dinfo->id, error_get_pretty(error));
+ error_setg(errp, "could not open disk image %s: %s",
+ file ?: dinfo->id, error_get_pretty(error));
+ error_free(error);
goto err;
}
@@ -863,9 +865,15 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
}
/* Actual block device init: Functionality shared with blockdev-add */
- dinfo = blockdev_init(bs_opts, type);
+ dinfo = blockdev_init(bs_opts, type, &local_err);
if (dinfo == NULL) {
+ if (error_is_set(&local_err)) {
+ qerror_report_err(local_err);
+ error_free(local_err);
+ }
goto fail;
+ } else {
+ assert(!error_is_set(&local_err));
}
/* Set legacy DriveInfo fields */
@@ -2152,7 +2160,6 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
QmpOutputVisitor *ov = qmp_output_visitor_new();
QObject *obj;
QDict *qdict;
- DriveInfo *dinfo;
Error *local_err = NULL;
/* Require an ID in the top level */
@@ -2186,9 +2193,9 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
qdict_flatten(qdict);
- dinfo = blockdev_init(qdict, IF_NONE);
- if (!dinfo) {
- error_setg(errp, "Could not open image");
+ blockdev_init(qdict, IF_NONE, &local_err);
+ if (error_is_set(&local_err)) {
+ error_propagate(errp, local_err);
goto fail;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 56+ messages in thread