Re: [Qemu-devel] [PATCH v3 2/2] i440fx: print an error message if user tries to enable iommu
Bandan Das writes:
> Markus Armbruster writes:
>
>> Bandan Das writes:
>>
>>> There's no indication of any sort that i440fx doesn't support
>>> "iommu=on"
>>>
>>> Reviewed-by: Eric Blake
>>> Signed-off-by: Bandan Das
>>> ---
>>> hw/pci-host/piix.c | 5 +
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>>> index 7b2fbf9..715208b 100644
>>> --- a/hw/pci-host/piix.c
>>> +++ b/hw/pci-host/piix.c
>>> @@ -34,6 +34,7 @@
>>> #include "sysemu/sysemu.h"
>>> #include "hw/i386/ioapic.h"
>>> #include "qapi/visitor.h"
>>> +#include "qemu/error-report.h"
>>>
>>> /*
>>> * I440FX chipset data sheet.
>>> @@ -301,6 +302,10 @@ static void i440fx_pcihost_realize(DeviceState *dev,
>>> Error **errp)
>>> static void i440fx_realize(PCIDevice *dev, Error **errp)
>>> {
>>> dev->config[I440FX_SMRAM] = 0x02;
>>> +
>>> +if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {
>>> +error_report("warning: i440fx doesn't support emulated iommu");
>>> +}
>>> }
>>>
>>> PCIBus *i440fx_init(const char *host_type, const char *pci_type,
>>
>> Hmm.
>>
>> If I understand things correctly, we add property "iommu" to *any*
>> machine, whether it supports it or not (see machine_initfn() in
>> hw/core/machine.c).
Many more properties there that make sense only for some machines.
>> Most machines don't support it. You add a warning to one of them.
>
> Yeah, I guess the only one that does is q35. Although, iommu should
> be theoretically applicable to all machine types, a generic "iommu"
> property common to all types makes little sense to me.
>
> One way is to remove the common property and make "iommu" a property of
> Q35 only (through a custom instance_init).
Technically an incompatible change: a bogus iommu= is no longer silently
ignored. I can think of four replies:
0. We don't care about bogus properties, so leave machine.c alone. You
can add warnings wherever you like. If you want to find out whether
the machine supports an IOMMU, use something else than QOM
introspection.
1. Backward compatibility idolatry: we must keep silently ignoring bogus
iommu=... forever.
2. Not rejecting iommu=on when the machine has none is a bug, accepting
iommu=off always is a feature. We must keep accepting iommu=off, and
reject iommu=on. To make IOMMU-support introspectable in QOM, you
could give the iommu property a silly type with only one value "off"
when the machine doesn't support it.
3. Does a compatility break make a noise when nobody's watching it?
Just drop the property for machines where it doesn't make sense.
I like 3. Without an actual QMP client interested in examining machine
capabilities via QOM introspection, I can't reject 0.
> Other is to simply make the property common to pc by moving it to pc.c
> But that still doesn't solve the problem since i440fx cannot emulate it yet.
>
>> Why to that one and not the others?
>>
>> Shouldn't we add properties only to machines where they make sense?
>> Adding them indiscrimiately defeats QOM introspection.
>
> For now, maybe we can just skip this patch. I will post another
> generic solution.
> It's just weird that qemu runs happily without complaining when iommu is
> specified with an unsupported type.
Yes, but it's just as weird for all the other machines, too :)
Making all machines warn about machine properties they don't understand
won't fly.
A generic mechanism that records when a machine uses a property and
warns about unused ones might work.
Re: [Qemu-devel] [PATCH 3/3] exec: silence hugetlbfs warning under qtest
Marc-André Lureau writes: > nack, this isn't enough to silence the warning, as vhost-user-test > uses -machine accel=tcg Pity. > On Mon, Nov 16, 2015 at 6:23 PM, wrote: >> From: Marc-André Lureau >> >> vhost-user-test prints a warning. A test should not need to run on >> hugetlbfs, let's silence the warning under qtest. Unfortunately, the >> condition can't check on qtest_enabled() or qtest_driver() since they >> are initialized later. Moving configure_accelerator() earlier is >> problematic, as the memory regions aren't yet fully set up and >> vhost-user-test fails. That leaves qtest_init(), which makes qtest_driver() valid. Could that be done earlier?
[Qemu-devel] [PATCH v12 04/36] qapi: Drop obsolete tag value collision assertions
From: Markus Armbruster
Union tag values can't clash with member names in generated C anymore
since commit e4ba22b, but QAPISchemaObjectTypeVariants.check() still
asserts they don't. Drop it.
Signed-off-by: Markus Armbruster
Message-Id: <1446559499-26984-1-git-send-email-arm...@redhat.com>
Signed-off-by: Eric Blake
---
v12: no change
v11: no change
v10: redo closer to Markus' original proposal
v9: new patch
---
scripts/qapi.py | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 687d9dc..29377d6 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1057,8 +1057,7 @@ class QAPISchemaObjectTypeVariants(object):
assert self.tag_member in seen.itervalues()
assert isinstance(self.tag_member.type, QAPISchemaEnumType)
for v in self.variants:
-vseen = dict(seen)
-v.check(schema, self.tag_member.type, vseen)
+v.check(schema, self.tag_member.type, {})
class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
--
2.4.3
[Qemu-devel] [PATCH v12 07/36] qapi: Fix up commit 7618b91's clash sanity checking change
From: Markus Armbruster
This hunk
@@ -964,6 +965,7 @@ class QAPISchemaObjectType(QAPISchemaType):
members = []
seen = {}
for m in members:
+assert c_name(m.name) not in seen
seen[m.name] = m
for m in self.local_members:
m.check(schema, members, seen)
is plainly broken.
Asserting the members inherited from base don't clash is somewhat
redundant, because self.base.check() just checked that. But it
doesn't hurt.
The idea to use c_name(m.name) instead of m.name for collision
checking is sound, because we need to catch clashes between the m.name
and between the c_name(m.name), and when two m.name clash, then their
c_name() also clash.
However, using c_name(m.name) instead of m.name in one of several
places doesn't work. See the very next line.
Keep the assertion, but drop the c_name() for now. A future commit
will bring it back.
Signed-off-by: Markus Armbruster
Message-Id: <1446559499-26984-4-git-send-email-arm...@redhat.com>
[change TABs in commit message to space]
Signed-off-by: Eric Blake
---
v12: no change
v11: no change
v10: redo closer to Markus' original proposal
v9: new patch
---
scripts/qapi.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 0bf8235..86d2adc 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -987,7 +987,7 @@ class QAPISchemaObjectType(QAPISchemaType):
members = []
seen = {}
for m in members:
-assert c_name(m.name) not in seen
+assert m.name not in seen
seen[m.name] = m
for m in self.local_members:
m.check(schema)
--
2.4.3
[Qemu-devel] [PATCH v12 10/36] qapi: Simplify QAPISchemaObjectTypeVariants.check()
From: Markus Armbruster Reduce the ugly flat union / simple union conditional by doing just the essential work here, namely setting self.tag_member. Move the rest to callers. Signed-off-by: Markus Armbruster Message-Id: <1446559499-26984-7-git-send-email-arm...@redhat.com> [rebase to earlier changes that moved tag_member.check() of alternate types, and tweak commit title and wording] Signed-off-by: Eric Blake --- v12: no change v11: don't drop change of 'if not self.tag_member' v10: redo closer to Markus' original proposal v9: new patch --- scripts/qapi.py | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/scripts/qapi.py b/scripts/qapi.py index 2a73b2b..c6cb17b 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -988,9 +988,10 @@ class QAPISchemaObjectType(QAPISchemaType): for m in self.local_members: m.check(schema) m.check_clash(seen) +self.members = seen.values() if self.variants: self.variants.check(schema, seen) -self.members = seen.values() +assert self.variants.tag_member in self.members def is_implicit(self): # See QAPISchema._make_implicit_object_type() @@ -1050,10 +1051,8 @@ class QAPISchemaObjectTypeVariants(object): self.variants = variants def check(self, schema, seen): -if self.tag_name:# flat union +if not self.tag_member:# flat union self.tag_member = seen[self.tag_name] -if seen: -assert self.tag_member in seen.itervalues() assert isinstance(self.tag_member.type, QAPISchemaEnumType) for v in self.variants: v.check(schema, self.tag_member.type) -- 2.4.3
[Qemu-devel] [PATCH v12 05/36] qapi: Simplify QAPISchemaObjectTypeMember.check()
From: Markus Armbruster
QAPISchemaObjectTypeMember.check() currently does four things:
1. Compute self.type
2. Accumulate members in all_members
Only one caller cares: QAPISchemaObjectType.check() uses it to
compute self.members. The other callers pass a throw-away
accumulator.
3. Accumulate a map from names to members in seen
Only one caller cares: QAPISchemaObjectType.check() uses it to
compute its local variable seen, for self.variants.check(), which
uses it to compute self.variants.tag_member from
self.variants.tag_name. The other callers pass a throw-away
accumulator.
4. Check for collisions
This piggybacks on 3: before adding a new entry, we assert it's new.
Only one caller cares: QAPISchemaObjectType.check() uses it to
assert non-variant members don't clash.
Simplify QAPISchemaObjectType.check(): move 2.-4. to
QAPISchemaObjectType.check(), and drop parameters all_members and
seen.
Signed-off-by: Markus Armbruster
Message-Id: <1446559499-26984-2-git-send-email-arm...@redhat.com>
[rebase to earlier changes that moved tag_member.check() of
alternate types, commit message typo fix]
Signed-off-by: Eric Blake
---
v12: no change
v11: typo fix
v10: redo closer to Markus' original proposal
v9: new patch
---
scripts/qapi.py | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 29377d6..63d39e4 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -990,7 +990,10 @@ class QAPISchemaObjectType(QAPISchemaType):
assert c_name(m.name) not in seen
seen[m.name] = m
for m in self.local_members:
-m.check(schema, members, seen)
+m.check(schema)
+assert m.name not in seen
+seen[m.name] = m
+members.append(m)
if self.variants:
self.variants.check(schema, members, seen)
self.members = members
@@ -1027,12 +1030,9 @@ class QAPISchemaObjectTypeMember(object):
self.type = None
self.optional = optional
-def check(self, schema, all_members, seen):
-assert self.name not in seen
+def check(self, schema):
self.type = schema.lookup_type(self._type_name)
assert self.type
-all_members.append(self)
-seen[self.name] = self
class QAPISchemaObjectTypeVariants(object):
@@ -1065,7 +1065,7 @@ class
QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
def check(self, schema, tag_type, seen):
-QAPISchemaObjectTypeMember.check(self, schema, [], seen)
+QAPISchemaObjectTypeMember.check(self, schema)
assert self.name in tag_type.values
# This function exists to support ugly simple union special cases
@@ -1087,7 +1087,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
self.variants = variants
def check(self, schema):
-self.variants.tag_member.check(schema, [], {})
+self.variants.tag_member.check(schema)
self.variants.check(schema, [], {})
def json_type(self):
--
2.4.3
[Qemu-devel] [PATCH v12 06/36] qapi: Clean up after previous commit
From: Markus Armbruster
QAPISchemaObjectTypeVariants.check() parameter members and
QAPISchemaObjectTypeVariant.check() parameter seen are no longer used,
drop them.
Signed-off-by: Markus Armbruster
Message-Id: <1446559499-26984-3-git-send-email-arm...@redhat.com>
[rebase to earlier changes that moved tag_member.check() of
alternate types]
Signed-off-by: Eric Blake
---
v12: fix git authorship
v11: rebase for less comment churn
v10: redo closer to Markus' original proposal
v9: new patch
---
scripts/qapi.py | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 63d39e4..0bf8235 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -995,7 +995,7 @@ class QAPISchemaObjectType(QAPISchemaType):
seen[m.name] = m
members.append(m)
if self.variants:
-self.variants.check(schema, members, seen)
+self.variants.check(schema, seen)
self.members = members
def is_implicit(self):
@@ -1050,21 +1050,21 @@ class QAPISchemaObjectTypeVariants(object):
self.tag_member = tag_member
self.variants = variants
-def check(self, schema, members, seen):
+def check(self, schema, seen):
if self.tag_name:# flat union
self.tag_member = seen[self.tag_name]
if seen:
assert self.tag_member in seen.itervalues()
assert isinstance(self.tag_member.type, QAPISchemaEnumType)
for v in self.variants:
-v.check(schema, self.tag_member.type, {})
+v.check(schema, self.tag_member.type)
class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
def __init__(self, name, typ):
QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
-def check(self, schema, tag_type, seen):
+def check(self, schema, tag_type):
QAPISchemaObjectTypeMember.check(self, schema)
assert self.name in tag_type.values
@@ -1088,7 +1088,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
def check(self, schema):
self.variants.tag_member.check(schema)
-self.variants.check(schema, [], {})
+self.variants.check(schema, {})
def json_type(self):
return 'value'
--
2.4.3
[Qemu-devel] [PATCH v12 03/36] qapi-types: Simplify gen_struct_field[s]
Simplify gen_struct_fields() back to a single iteration over a
list of fields (like it was prior to commit f87ab7f9), by moving
the generated comments to gen_object(). Then, inline
gen_struct_field() into its only caller.
Signed-off-by: Eric Blake
---
v12: no change
v11: no change
v10: no change
v9: new patch
---
scripts/qapi-types.py | 40 +++-
1 file changed, 15 insertions(+), 25 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 403768b..2f2f7df 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -36,48 +36,38 @@ struct %(c_name)s {
c_name=c_name(name), c_type=element_type.c_type())
-def gen_struct_field(member):
+def gen_struct_fields(members):
ret = ''
-
-if member.optional:
-ret += mcgen('''
+for memb in members:
+if memb.optional:
+ret += mcgen('''
bool has_%(c_name)s;
''',
- c_name=c_name(member.name))
-ret += mcgen('''
+ c_name=c_name(memb.name))
+ret += mcgen('''
%(c_type)s %(c_name)s;
''',
- c_type=member.type.c_type(), c_name=c_name(member.name))
+ c_type=memb.type.c_type(), c_name=c_name(memb.name))
return ret
-def gen_struct_fields(local_members, base):
-ret = ''
+def gen_object(name, base, members, variants):
+ret = mcgen('''
+
+struct %(c_name)s {
+''',
+c_name=c_name(name))
if base:
ret += mcgen('''
/* Members inherited from %(c_name)s: */
''',
c_name=base.c_name())
-for memb in base.members:
-ret += gen_struct_field(memb)
+ret += gen_struct_fields(base.members)
ret += mcgen('''
/* Own members: */
''')
-
-for memb in local_members:
-ret += gen_struct_field(memb)
-return ret
-
-
-def gen_object(name, base, members, variants):
-ret = mcgen('''
-
-struct %(c_name)s {
-''',
-c_name=c_name(name))
-
-ret += gen_struct_fields(members, base)
+ret += gen_struct_fields(members)
if variants:
ret += gen_variants(variants)
--
2.4.3
[Qemu-devel] [PATCH v12 14/36] qapi: Remove outdated tests related to QMP/branch collisions
Now that branches are in a separate C namespace, we can remove
the restrictions in the parser that claim a branch name would
collide with QMP, and delete the negative tests that are no
longer problematic. A separate patch can then add positive
tests to qapi-schema-test to test that any corner cases will
compile correctly.
This reverts the scripts/qapi.py portion of commit 7b2a5c2,
now that the assertions that it plugged are no longer possible.
Signed-off-by: Eric Blake
---
v12: no change
v11: no change
v10: no change
v9: no change
v8: retitle and update commit message, finish reversion of 7b2a5c2
v7: new patch (also temporarily appeared in subset B v10)
---
scripts/qapi.py| 11 ++-
tests/Makefile | 3 ---
tests/qapi-schema/flat-union-clash-branch.err | 0
tests/qapi-schema/flat-union-clash-branch.exit | 1 -
tests/qapi-schema/flat-union-clash-branch.json | 18 --
tests/qapi-schema/flat-union-clash-branch.out | 14 --
tests/qapi-schema/flat-union-clash-type.err| 1 -
tests/qapi-schema/flat-union-clash-type.exit | 1 -
tests/qapi-schema/flat-union-clash-type.json | 14 --
tests/qapi-schema/flat-union-clash-type.out| 0
tests/qapi-schema/union-clash-type.err | 1 -
tests/qapi-schema/union-clash-type.exit| 1 -
tests/qapi-schema/union-clash-type.json| 9 -
tests/qapi-schema/union-clash-type.out | 0
14 files changed, 2 insertions(+), 72 deletions(-)
delete mode 100644 tests/qapi-schema/flat-union-clash-branch.err
delete mode 100644 tests/qapi-schema/flat-union-clash-branch.exit
delete mode 100644 tests/qapi-schema/flat-union-clash-branch.json
delete mode 100644 tests/qapi-schema/flat-union-clash-branch.out
delete mode 100644 tests/qapi-schema/flat-union-clash-type.err
delete mode 100644 tests/qapi-schema/flat-union-clash-type.exit
delete mode 100644 tests/qapi-schema/flat-union-clash-type.json
delete mode 100644 tests/qapi-schema/flat-union-clash-type.out
delete mode 100644 tests/qapi-schema/union-clash-type.err
delete mode 100644 tests/qapi-schema/union-clash-type.exit
delete mode 100644 tests/qapi-schema/union-clash-type.json
delete mode 100644 tests/qapi-schema/union-clash-type.out
diff --git a/scripts/qapi.py b/scripts/qapi.py
index c6f3fce..6fc14be 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -548,8 +548,7 @@ def check_union(expr, expr_info):
base = expr.get('base')
discriminator = expr.get('discriminator')
members = expr['data']
-values = {'MAX': '(automatic)', 'KIND': '(automatic)',
- 'TYPE': '(automatic)'}
+values = {'MAX': '(automatic)'}
# Two types of unions, determined by discriminator.
@@ -607,19 +606,13 @@ def check_union(expr, expr_info):
" of branch '%s'" % key)
# If the discriminator names an enum type, then all members
-# of 'data' must also be members of the enum type, which in turn
-# must not collide with the discriminator name.
+# of 'data' must also be members of the enum type.
if enum_define:
if key not in enum_define['enum_values']:
raise QAPIExprError(expr_info,
"Discriminator value '%s' is not found in "
"enum '%s'" %
(key, enum_define["enum_name"]))
-if discriminator in enum_define['enum_values']:
-raise QAPIExprError(expr_info,
-"Discriminator name '%s' collides with "
-"enum value in '%s'" %
-(discriminator, enum_define["enum_name"]))
# Otherwise, check for conflicts in the generated enum
else:
diff --git a/tests/Makefile b/tests/Makefile
index 90c4141..1c2c8ee 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -280,9 +280,7 @@ qapi-schema += flat-union-bad-base.json
qapi-schema += flat-union-bad-discriminator.json
qapi-schema += flat-union-base-any.json
qapi-schema += flat-union-base-union.json
-qapi-schema += flat-union-clash-branch.json
qapi-schema += flat-union-clash-member.json
-qapi-schema += flat-union-clash-type.json
qapi-schema += flat-union-empty.json
qapi-schema += flat-union-inline.json
qapi-schema += flat-union-int-branch.json
@@ -344,7 +342,6 @@ qapi-schema += union-bad-branch.json
qapi-schema += union-base-no-discriminator.json
qapi-schema += union-clash-branches.json
qapi-schema += union-clash-data.json
-qapi-schema += union-clash-type.json
qapi-schema += union-empty.json
qapi-schema += union-invalid-base.json
qapi-schema += union-max.json
diff --git a/tests/qapi-schema/flat-union-clash-branch.err
b/tests/qapi-schema/flat-union-clash-branch.err
deleted file mode 100644
index e69de29..000
diff --git a/tests/qapi-schema/flat-union-clash-branch
[Qemu-devel] [PATCH v12 01/36] qapi: Track simple union tag in object.local_members
We were previously creating all unions with an empty list for
local_members. However, it will make it easier to unify struct
and union generation if we include the generated tag member in
local_members. That way, we can have a common code pattern:
visit the base (if any), visit the local members (if any), visit
the variants (if any). The local_members of a flat union
remains empty (because the discriminator is already visited as
part of the base). Then, by visiting tag_member.check() during
AlternateType.check(), we no longer need to call it during
Variants.check().
The various front end entities now exist as follows:
struct: optional base, optional local_members, no variants
simple union: no base, one-element local_members, variants with tag_member
from local_members
flat union: base, no local_members, variants with tag_member from base
alternate: no base, no local_members, variants
With the new local members, we require a bit of finesse to
avoid assertions in the clients.
No change to generated code.
Signed-off-by: Eric Blake
---
v12: no change
v11: drop comment churn
v10: no change
v9: improve commit message, add comments, tweak how alternates
check tag_member
v8: new patch
---
scripts/qapi-types.py | 5 -
scripts/qapi-visit.py | 5 -
scripts/qapi.py| 15 ++-
tests/qapi-schema/qapi-schema-test.out | 2 ++
tests/qapi-schema/union-clash-data.out | 1 +
tests/qapi-schema/union-empty.out | 1 +
6 files changed, 22 insertions(+), 7 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index b37900f..946afab 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -269,7 +269,10 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
def visit_object_type(self, name, info, base, members, variants):
self._fwdecl += gen_fwd_object_or_array(name)
if variants:
-assert not members # not implemented
+if members:
+# Members other than variants.tag_member not implemented
+assert len(members) == 1
+assert members[0] == variants.tag_member
self.decl += gen_union(name, base, variants)
else:
self.decl += gen_struct(name, base, members)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 3ef5c16..94cd113 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -364,7 +364,10 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
def visit_object_type(self, name, info, base, members, variants):
self.decl += gen_visit_decl(name)
if variants:
-assert not members # not implemented
+if members:
+# Members other than variants.tag_member not implemented
+assert len(members) == 1
+assert members[0] == variants.tag_member
self.defn += gen_visit_union(name, base, variants)
else:
self.defn += gen_visit_struct(name, base, members)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 7c50cc4..687d9dc 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -957,6 +957,9 @@ class QAPISchemaArrayType(QAPISchemaType):
class QAPISchemaObjectType(QAPISchemaType):
def __init__(self, name, info, base, local_members, variants):
+# struct has local_members, optional base, and no variants
+# flat union has base, variants, and no local_members
+# simple union has local_members, variants, and no base
QAPISchemaType.__init__(self, name, info)
assert base is None or isinstance(base, str)
for m in local_members:
@@ -1048,10 +1051,10 @@ class QAPISchemaObjectTypeVariants(object):
self.variants = variants
def check(self, schema, members, seen):
-if self.tag_name:
+if self.tag_name:# flat union
self.tag_member = seen[self.tag_name]
-else:
-self.tag_member.check(schema, members, seen)
+if seen:
+assert self.tag_member in seen.itervalues()
assert isinstance(self.tag_member.type, QAPISchemaEnumType)
for v in self.variants:
vseen = dict(seen)
@@ -1085,6 +1088,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
self.variants = variants
def check(self, schema):
+self.variants.tag_member.check(schema, [], {})
self.variants.check(schema, [], {})
def json_type(self):
@@ -1270,13 +1274,14 @@ class QAPISchema(object):
if tag_name:
variants = [self._make_variant(key, value)
for (key, value) in data.iteritems()]
+members = []
else:
variants = [self._make_simple_variant(key, value, info)
for (key, value) in data.iteritems()]
tag_member = self._make_implicit_tag(name, info, variants)
+members = [tag_member]
sel
[Qemu-devel] [PATCH v12 08/36] qapi: Eliminate QAPISchemaObjectType.check() variable members
From: Markus Armbruster
We can use seen.values() instead if we make it an OrderedDict.
Signed-off-by: Markus Armbruster
Message-Id: <1446559499-26984-5-git-send-email-arm...@redhat.com>
Signed-off-by: Eric Blake
---
v12: no change
v11: no change
v10: redo closer to Markus' original proposal
v9: new patch
---
scripts/qapi.py | 14 +-
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 86d2adc..44d08c1 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -977,26 +977,22 @@ class QAPISchemaObjectType(QAPISchemaType):
if self.members:
return
self.members = False# mark as being checked
+seen = OrderedDict()
if self._base_name:
self.base = schema.lookup_type(self._base_name)
assert isinstance(self.base, QAPISchemaObjectType)
assert not self.base.variants # not implemented
self.base.check(schema)
-members = list(self.base.members)
-else:
-members = []
-seen = {}
-for m in members:
-assert m.name not in seen
-seen[m.name] = m
+for m in self.base.members:
+assert m.name not in seen
+seen[m.name] = m
for m in self.local_members:
m.check(schema)
assert m.name not in seen
seen[m.name] = m
-members.append(m)
if self.variants:
self.variants.check(schema, seen)
-self.members = members
+self.members = seen.values()
def is_implicit(self):
# See QAPISchema._make_implicit_object_type()
--
2.4.3
[Qemu-devel] [PATCH v12 02/36] qapi-types: Consolidate gen_struct() and gen_union()
These two methods are now close enough that we can finally merge
them, relying on the fact that simple unions now provide a
reasonable local_members. Change gen_struct() to gen_object()
that handles all forms of QAPISchemaObjectType, and rename and
shrink gen_union() to gen_variants() to handle the portion of
gen_object() needed when variants are present.
gen_struct_fields() now has a single caller, so it no longer
needs an optional parameter; however, I did not choose to inline
it into the caller.
No difference to generated code.
Signed-off-by: Eric Blake
---
v12: no change
v11: no change
v10: no change
v9: rebase to earlier changes
v8: new patch
---
scripts/qapi-types.py | 37 +++--
1 file changed, 11 insertions(+), 26 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 946afab..403768b 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -51,7 +51,7 @@ def gen_struct_field(member):
return ret
-def gen_struct_fields(local_members, base=None):
+def gen_struct_fields(local_members, base):
ret = ''
if base:
@@ -70,7 +70,7 @@ def gen_struct_fields(local_members, base=None):
return ret
-def gen_struct(name, base, members):
+def gen_object(name, base, members, variants):
ret = mcgen('''
struct %(c_name)s {
@@ -79,11 +79,14 @@ struct %(c_name)s {
ret += gen_struct_fields(members, base)
+if variants:
+ret += gen_variants(variants)
+
# Make sure that all structs have at least one field; this avoids
# potential issues with attempting to malloc space for zero-length
# structs in C, and also incompatibility with C++ (where an empty
# struct is size 1).
-if not (base and base.members) and not members:
+if not (base and base.members) and not members and not variants:
ret += mcgen('''
char qapi_dummy_field_for_empty_struct;
''')
@@ -140,17 +143,7 @@ const int %(c_name)s_qtypes[QTYPE_MAX] = {
return ret
-def gen_union(name, base, variants):
-ret = mcgen('''
-
-struct %(c_name)s {
-''',
-c_name=c_name(name))
-if base:
-ret += gen_struct_fields([], base)
-else:
-ret += gen_struct_field(variants.tag_member)
-
+def gen_variants(variants):
# FIXME: What purpose does data serve, besides preventing a union that
# has a branch named 'data'? We use it in qapi-visit.py to decide
# whether to bypass the switch statement if visiting the discriminator
@@ -159,11 +152,11 @@ struct %(c_name)s {
# should not be any data leaks even without a data pointer. Or, if
# 'data' is merely added to guarantee we don't have an empty union,
# shouldn't we enforce that at .json parse time?
-ret += mcgen('''
+ret = mcgen('''
union { /* union tag is @%(c_name)s */
void *data;
''',
- c_name=c_name(variants.tag_member.name))
+c_name=c_name(variants.tag_member.name))
for var in variants.variants:
# Ugly special case for simple union TODO get rid of it
@@ -176,7 +169,6 @@ struct %(c_name)s {
ret += mcgen('''
} u;
-};
''')
return ret
@@ -268,14 +260,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
def visit_object_type(self, name, info, base, members, variants):
self._fwdecl += gen_fwd_object_or_array(name)
-if variants:
-if members:
-# Members other than variants.tag_member not implemented
-assert len(members) == 1
-assert members[0] == variants.tag_member
-self.decl += gen_union(name, base, variants)
-else:
-self.decl += gen_struct(name, base, members)
+self.decl += gen_object(name, base, members, variants)
if base:
self.decl += gen_upcast(name, base)
self._gen_type_cleanup(name)
@@ -283,7 +268,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
def visit_alternate_type(self, name, info, variants):
self._fwdecl += gen_fwd_object_or_array(name)
self._fwdefn += gen_alternate_qtypes(name, variants)
-self.decl += gen_union(name, None, variants)
+self.decl += gen_object(name, None, [variants.tag_member], variants)
self.decl += gen_alternate_qtypes_decl(name)
self._gen_type_cleanup(name)
--
2.4.3
[Qemu-devel] [PATCH v12 16/36] qapi: Detect collisions in C member names
Detect attempts to declare two object members that would result in the same C member name, by keying the 'seen' dictionary off of the C name rather than the qapi name. It also requires passing info through the check_clash() methods. This addresses a TODO and fixes the previously-broken args-name-clash test. The resulting error message demonstrates the utility of the .describe() method added previously. No change to generated code. Signed-off-by: Eric Blake --- v12: no change v11: rebase to earlier changes, use 'cname' local variable, shorter message in test file v10 (now in subset C): rebase to latest; update commit message v9 (now in subset D): rebase to earlier changes, now only one test affected v8: rebase to earlier changes v7: split out error reporting prep and member.c_name() addition v6: rebase to earlier testsuite and info improvements --- scripts/qapi.py| 31 +++ tests/qapi-schema/args-name-clash.err | 1 + tests/qapi-schema/args-name-clash.exit | 2 +- tests/qapi-schema/args-name-clash.json | 5 ++--- tests/qapi-schema/args-name-clash.out | 6 -- 5 files changed, 23 insertions(+), 22 deletions(-) diff --git a/scripts/qapi.py b/scripts/qapi.py index 79038a8..b068141 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -977,20 +977,23 @@ class QAPISchemaObjectType(QAPISchemaType): self.base = schema.lookup_type(self._base_name) assert isinstance(self.base, QAPISchemaObjectType) self.base.check(schema) -self.base.check_clash(schema, seen) +self.base.check_clash(schema, self.info, seen) for m in self.local_members: m.check(schema) -m.check_clash(seen) +m.check_clash(self.info, seen) self.members = seen.values() if self.variants: self.variants.check(schema, seen) assert self.variants.tag_member in self.members -self.variants.check_clash(schema, seen) +self.variants.check_clash(schema, self.info, seen) -def check_clash(self, schema, seen): +# Check that the members of this type do not cause duplicate JSON fields, +# and update seen to track the members seen so far. Report any errors +# on behalf of info, which is not necessarily self.info +def check_clash(self, schema, info, seen): assert not self.variants # not implemented for m in self.members: -m.check_clash(seen) +m.check_clash(info, seen) def is_implicit(self): # See QAPISchema._make_implicit_object_type() @@ -1036,10 +1039,13 @@ class QAPISchemaObjectTypeMember(object): self.type = schema.lookup_type(self._type_name) assert self.type -def check_clash(self, seen): -# TODO change key of seen from QAPI name to C name -assert self.name not in seen -seen[self.name] = self +def check_clash(self, info, seen): +cname = c_name(self.name) +if cname in seen: +raise QAPIExprError(info, +"%s collides with %s" +% (self.describe(), seen[cname].describe())) +seen[cname] = self def _pretty_owner(self): owner = self.owner @@ -1079,7 +1085,8 @@ class QAPISchemaObjectTypeVariants(object): def check(self, schema, seen): if not self.tag_member:# flat union -self.tag_member = seen[self.tag_name] +self.tag_member = seen[c_name(self.tag_name)] +assert self.tag_name == self.tag_member.name assert isinstance(self.tag_member.type, QAPISchemaEnumType) for v in self.variants: v.check(schema) @@ -1087,12 +1094,12 @@ class QAPISchemaObjectTypeVariants(object): if isinstance(v.type, QAPISchemaObjectType): v.type.check(schema) -def check_clash(self, schema, seen): +def check_clash(self, schema, info, seen): for v in self.variants: # Reset seen map for each variant, since qapi names from one # branch do not affect another branch assert isinstance(v.type, QAPISchemaObjectType) -v.type.check_clash(schema, dict(seen)) +v.type.check_clash(schema, info, dict(seen)) class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): diff --git a/tests/qapi-schema/args-name-clash.err b/tests/qapi-schema/args-name-clash.err index e69de29..d953e8d 100644 --- a/tests/qapi-schema/args-name-clash.err +++ b/tests/qapi-schema/args-name-clash.err @@ -0,0 +1 @@ +tests/qapi-schema/args-name-clash.json:4: 'a_b' (parameter of oops) collides with 'a-b' (parameter of oops) diff --git a/tests/qapi-schema/args-name-clash.exit b/tests/qapi-schema/args-name-clash.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/args-name-clash.exit +++ b/tests/qapi-schema/args-name-clash.exit @@ -1 +1 @@ -0 +1 diff --git a/tests
[Qemu-devel] [PATCH v12 09/36] qapi: Factor out QAPISchemaObjectTypeMember.check_clash()
From: Markus Armbruster While there, stick in a TODO change key of seen from QAPI name to C name. Can't do it right away, because it would fail the assertion for tests/qapi-schema/args-has-clash.json. Signed-off-by: Markus Armbruster Message-Id: <1446559499-26984-6-git-send-email-arm...@redhat.com> Signed-off-by: Eric Blake --- v12: no change v11: no change v10: redo closer to Markus' original proposal v9: new patch --- scripts/qapi.py | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/scripts/qapi.py b/scripts/qapi.py index 44d08c1..2a73b2b 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -984,12 +984,10 @@ class QAPISchemaObjectType(QAPISchemaType): assert not self.base.variants # not implemented self.base.check(schema) for m in self.base.members: -assert m.name not in seen -seen[m.name] = m +m.check_clash(seen) for m in self.local_members: m.check(schema) -assert m.name not in seen -seen[m.name] = m +m.check_clash(seen) if self.variants: self.variants.check(schema, seen) self.members = seen.values() @@ -1030,6 +1028,11 @@ class QAPISchemaObjectTypeMember(object): self.type = schema.lookup_type(self._type_name) assert self.type +def check_clash(self, seen): +# TODO change key of seen from QAPI name to C name +assert self.name not in seen +seen[self.name] = self + class QAPISchemaObjectTypeVariants(object): def __init__(self, tag_name, tag_member, variants): -- 2.4.3
[Qemu-devel] [PATCH v12 13/36] qapi: Hoist tag collision check to Variants.check()
Checking that a given QAPISchemaObjectTypeVariant.name is a member of the corresponding QAPISchemaEnumType of the owning QAPISchemaObjectTypeVariants.tag_member ensures that there are no collisions in the generated C union for those tag values (since the enum itself should have no collisions). However, ever since its introduction in f51d8c3d, this was the only additional action of of Variant.check(), beyond calling the superclass Member.check(). This forces a difference in .check() signatures, just to pass the enum type down. Simplify things by instead doing the tag name check as part of Variants.check(), at which point we can rely on inheritance instead of overriding Variant.check(). Signed-off-by: Eric Blake --- v12: improve commit message v11: don't use tag_type local variable, rebase to v.type.check() v10: new patch --- scripts/qapi.py | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/scripts/qapi.py b/scripts/qapi.py index 296b9bb..c6f3fce 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -1059,7 +1059,8 @@ class QAPISchemaObjectTypeVariants(object): self.tag_member = seen[self.tag_name] assert isinstance(self.tag_member.type, QAPISchemaEnumType) for v in self.variants: -v.check(schema, self.tag_member.type) +v.check(schema) +assert v.name in self.tag_member.type.values if isinstance(v.type, QAPISchemaObjectType): v.type.check(schema) @@ -1075,10 +1076,6 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): def __init__(self, name, typ): QAPISchemaObjectTypeMember.__init__(self, name, typ, False) -def check(self, schema, tag_type): -QAPISchemaObjectTypeMember.check(self, schema) -assert self.name in tag_type.values - # This function exists to support ugly simple union special cases # TODO get rid of them, and drop the function def simple_union_type(self): -- 2.4.3
[Qemu-devel] [PATCH v12 00/36] qapi member collision, alternate layout (post-introspection cleanups, subset D)
No pending prerequisites; based on qemu.git master Also available as a tag at this location: git fetch git://repo.or.cz/qemu/ericb.git qapi-cleanupv12d and will soon be part of my branch with the rest of the v5 series, at: http://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/qapi v12 notes: Delay 26-28 to a later subset, and instead add lots of new prereq patches that tackle some cleanups that make case-insensitive collision detection easier. Series is now slated for 2.6. 001/36:[] [--] 'qapi: Track simple union tag in object.local_members' 002/36:[] [--] 'qapi-types: Consolidate gen_struct() and gen_union()' 003/36:[] [--] 'qapi-types: Simplify gen_struct_field[s]' 004/36:[] [--] 'qapi: Drop obsolete tag value collision assertions' 005/36:[] [--] 'qapi: Simplify QAPISchemaObjectTypeMember.check()' 006/36:[] [--] 'qapi: Clean up after previous commit' 007/36:[] [--] 'qapi: Fix up commit 7618b91's clash sanity checking change' 008/36:[] [--] 'qapi: Eliminate QAPISchemaObjectType.check() variable members' 009/36:[] [--] 'qapi: Factor out QAPISchemaObjectTypeMember.check_clash()' 010/36:[] [--] 'qapi: Simplify QAPISchemaObjectTypeVariants.check()' 011/36:[down] 'qapi: Check for qapi collisions involving variant members' 012/36:[] [--] 'qapi: Factor out QAPISchemaObjectType.check_clash()' 013/36:[] [--] 'qapi: Hoist tag collision check to Variants.check()' 014/36:[] [--] 'qapi: Remove outdated tests related to QMP/branch collisions' 015/36:[] [--] 'qapi: Track owner of each object member' 016/36:[] [--] 'qapi: Detect collisions in C member names' 017/36:[down] 'qapi: Fix c_name() munging' 018/36:[down] 'qapi: Remove dead visitor code' 019/36:[down] 'blkdebug: Merge hand-rolled and qapi BlkdebugEvent enum' 020/36:[down] 'blkdebug: Avoid '.' in enum values' 021/36:[down] 'qapi: Tighten the regex on valid names' 022/36:[down] 'qapi: Don't let implicit enum MAX member collide' 023/36:[down] 'qapi: Remove dead tests for max collision' 024/36:[0018] [FC] 'cpu: Convert CpuInfo into flat union' 025/36:[down] 'qapi: Add alias for ErrorClass' 026/36:[0098] [FC] 'qapi: Change munging of CamelCase enum values' 027/36:[0049] [FC] 'qapi: Forbid case-insensitive clashes' 028/36:[down] 'qapi: Simplify QObject' 029/36:[down] 'qobject: Rename qtype_code to QType' 030/36:[down] 'qapi: Convert QType into qapi builtin enum type' 031/36:[0020] [FC] 'qapi: Simplify visiting of alternate types' 032/36:[down] 'qapi: Inline _make_implicit_tag()' 033/36:[0015] [FC] 'qapi: Fix alternates that accept 'number' but not 'int'' 034/36:[] [--] 'qapi: Add positive tests to qapi-schema-test' 035/36:[0010] [FC] 'qapi: Simplify visits of optional fields' 036/36:[down] 'qapi: Shorter visits of optional fields' v11 notes: https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg02563.html First half of v10 subset C has been applied, so consolidate the last half of that along with all of subset D (which was at v9) into one group. Address list reviews, in particular, add a new patch 21 that makes alternate layouts a lot nicer by making qtype_code a builtin qapi type; and new patches 18-19 that try to reduce confusion on the use of camel_to_upper() in c_enum_const(). Probably too late to get these into 2.5, in which case 17/28 will need tweaks to call out 2.6. v10 notes: https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg01249.html Split several patches, redo the middle patches from Markus to be back in the order they were first posted, some fallout change to my patches due to the nicer pattern of minimizing conditionals inside .check(), by instead calling .check_clash() as needed. Change data->err magic in tests to instead use a new helper error_free_or_abort(). Add a patch that would prevent qapi case-insensitive clashes. I am redoing my subset boundaries slightly: patches 23-27 of v9 (updating the alternate layout) will be delayed to subset D, and 2 other patches previously posted in subset D are now here (turning qapi clash checking into actual error messages), so the subject line of this cover letter is slightly different. Hopefully, we are converging on something that will be ready for a pull request, especially for the earlier patches of this subset. v9 notes: https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg00652.html https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg06999.html More patches added, and several reorganized. Lots of new patches from Markus, although not in the order originally proposed. The first 8 patches are fairly straightforward, and could probably be taken as-is. Patch 9 is a rewrite of v8 4/17, but in the opposite direction (document that no sorting is done, rather than attempting to sort), so it may need further fine-tuning. Patches 12-21 represents a fusion of Markus' and my attempts to rewrite v5 7/17 into a more-reviewable set of patches, and caused further churn later in the series. Patch 23 still uses tag_member.type
[Qemu-devel] [PATCH v12 12/36] qapi: Factor out QAPISchemaObjectType.check_clash()
Consolidate two common sequences of clash detection into a new QAPISchemaObjectType.check_clash() helper method. No change to generated code. Signed-off-by: Eric Blake --- v12: no change v11: don't lose isinstance check, and don't hide type.check() inside check_clash() v10: rebase on new Variants.check_clash() v9: new patch, split off from v8 7/17 --- scripts/qapi.py | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/scripts/qapi.py b/scripts/qapi.py index b2d071f..296b9bb 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -981,10 +981,8 @@ class QAPISchemaObjectType(QAPISchemaType): if self._base_name: self.base = schema.lookup_type(self._base_name) assert isinstance(self.base, QAPISchemaObjectType) -assert not self.base.variants # not implemented self.base.check(schema) -for m in self.base.members: -m.check_clash(seen) +self.base.check_clash(schema, seen) for m in self.local_members: m.check(schema) m.check_clash(seen) @@ -994,6 +992,11 @@ class QAPISchemaObjectType(QAPISchemaType): assert self.variants.tag_member in self.members self.variants.check_clash(schema, seen) +def check_clash(self, schema, seen): +assert not self.variants # not implemented +for m in self.members: +m.check_clash(seen) + def is_implicit(self): # See QAPISchema._make_implicit_object_type() return self.name[0] == ':' @@ -1064,11 +1067,8 @@ class QAPISchemaObjectTypeVariants(object): for v in self.variants: # Reset seen map for each variant, since qapi names from one # branch do not affect another branch -vseen = dict(seen) assert isinstance(v.type, QAPISchemaObjectType) -assert not v.type.variants # not implemented -for m in v.type.members: -m.check_clash(vseen) +v.type.check_clash(schema, dict(seen)) class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): -- 2.4.3
[Qemu-devel] [PATCH v12 23/36] qapi: Remove dead tests for max collision
Now that we no longer collide with an implicit _MAX enum member,
we no longer need to reject it in the ad hoc parser, and can
remove several tests that are no longer needed.
Signed-off-by: Eric Blake
---
v12: new patch
---
scripts/qapi.py| 8 +++-
tests/Makefile | 3 ---
tests/qapi-schema/enum-max-member.err | 1 -
tests/qapi-schema/enum-max-member.exit | 1 -
tests/qapi-schema/enum-max-member.json | 3 ---
tests/qapi-schema/enum-max-member.out | 0
tests/qapi-schema/event-max.err| 1 -
tests/qapi-schema/event-max.exit | 1 -
tests/qapi-schema/event-max.json | 2 --
tests/qapi-schema/event-max.out| 0
tests/qapi-schema/union-max.err| 1 -
tests/qapi-schema/union-max.exit | 1 -
tests/qapi-schema/union-max.json | 3 ---
tests/qapi-schema/union-max.out| 0
14 files changed, 3 insertions(+), 22 deletions(-)
delete mode 100644 tests/qapi-schema/enum-max-member.err
delete mode 100644 tests/qapi-schema/enum-max-member.exit
delete mode 100644 tests/qapi-schema/enum-max-member.json
delete mode 100644 tests/qapi-schema/enum-max-member.out
delete mode 100644 tests/qapi-schema/event-max.err
delete mode 100644 tests/qapi-schema/event-max.exit
delete mode 100644 tests/qapi-schema/event-max.json
delete mode 100644 tests/qapi-schema/event-max.out
delete mode 100644 tests/qapi-schema/union-max.err
delete mode 100644 tests/qapi-schema/union-max.exit
delete mode 100644 tests/qapi-schema/union-max.json
delete mode 100644 tests/qapi-schema/union-max.out
diff --git a/scripts/qapi.py b/scripts/qapi.py
index aa15adb..cd2982a 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -537,8 +537,6 @@ def check_event(expr, expr_info):
global events
name = expr['event']
-if name.upper() == 'MAX':
-raise QAPIExprError(expr_info, "Event name 'MAX' cannot be created")
events.append(name)
check_type(expr_info, "'data' for event '%s'" % name,
expr.get('data'), allow_dict=True, allow_optional=True,
@@ -550,7 +548,7 @@ def check_union(expr, expr_info):
base = expr.get('base')
discriminator = expr.get('discriminator')
members = expr['data']
-values = {'MAX': '(automatic)'}
+values = {}
# Two types of unions, determined by discriminator.
@@ -629,7 +627,7 @@ def check_union(expr, expr_info):
def check_alternate(expr, expr_info):
name = expr['alternate']
members = expr['data']
-values = {'MAX': '(automatic)'}
+values = {}
types_seen = {}
# Check every branch
@@ -662,7 +660,7 @@ def check_enum(expr, expr_info):
name = expr['enum']
members = expr.get('data')
prefix = expr.get('prefix')
-values = {'MAX': '(automatic)'}
+values = {}
if not isinstance(members, list):
raise QAPIExprError(expr_info,
diff --git a/tests/Makefile b/tests/Makefile
index b70d246..a8a3b12 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -266,14 +266,12 @@ qapi-schema += enum-bad-prefix.json
qapi-schema += enum-clash-member.json
qapi-schema += enum-dict-member.json
qapi-schema += enum-int-member.json
-qapi-schema += enum-max-member.json
qapi-schema += enum-missing-data.json
qapi-schema += enum-wrong-data.json
qapi-schema += escape-outside-string.json
qapi-schema += escape-too-big.json
qapi-schema += escape-too-short.json
qapi-schema += event-case.json
-qapi-schema += event-max.json
qapi-schema += event-nest-struct.json
qapi-schema += flat-union-array-branch.json
qapi-schema += flat-union-bad-base.json
@@ -345,7 +343,6 @@ qapi-schema += union-clash-branches.json
qapi-schema += union-clash-data.json
qapi-schema += union-empty.json
qapi-schema += union-invalid-base.json
-qapi-schema += union-max.json
qapi-schema += union-optional-branch.json
qapi-schema += union-unknown.json
qapi-schema += unknown-escape.json
diff --git a/tests/qapi-schema/enum-max-member.err
b/tests/qapi-schema/enum-max-member.err
deleted file mode 100644
index f77837f..000
--- a/tests/qapi-schema/enum-max-member.err
+++ /dev/null
@@ -1 +0,0 @@
-tests/qapi-schema/enum-max-member.json:3: Enum 'MyEnum' member 'max' clashes
with '(automatic)'
diff --git a/tests/qapi-schema/enum-max-member.exit
b/tests/qapi-schema/enum-max-member.exit
deleted file mode 100644
index d00491f..000
--- a/tests/qapi-schema/enum-max-member.exit
+++ /dev/null
@@ -1 +0,0 @@
-1
diff --git a/tests/qapi-schema/enum-max-member.json
b/tests/qapi-schema/enum-max-member.json
deleted file mode 100644
index 4bcda0b..000
--- a/tests/qapi-schema/enum-max-member.json
+++ /dev/null
@@ -1,3 +0,0 @@
-# we reject user-supplied 'max' for clashing with implicit enum end
-# TODO: should we instead munge the implicit value to avoid the clash?
-{ 'enum': 'MyEnum', 'data': [ 'max' ] }
diff --git a/tests/qapi-schema/enum-max-member.out
b/tests/qapi-schema/enum-max-member.out
deleted file mode 100644
index e69de29..000
diff --git a/tests/qapi-schema/event
[Qemu-devel] [PATCH v12 25/36] qapi: Add alias for ErrorClass
The qapi enum ErrorClass is unusual that it uses 'CamelCase' names,
contrary to our documented convention of preferring 'lower-case'.
However, this enum is entrenched in the API; we cannot change
what strings QMP outputs. Meanwhile, we want to simplify how
c_enum_const() is used to generate enum constants, by moving away
from the heuristics of camel_to_upper() to a more straightforward
c_name(N).upper() - but doing so will rename all of the ErrorClass
constants and cause churn to all client files, where the new names
are aesthetically less pleasing (ERROR_CLASS_DEVICENOTFOUND looks
like we can't make up our minds on whether to break between words).
So as always in computer science, solve the problem by some more
indirection: rename the qapi type to QapiErrorClass, and add a
new enum ErrorClass in error.h whose members are aliases of the
qapi type, but with the spelling expected elsewhere in the tree.
Then, when c_enum_const() changes the munging, we only have to
adjust the one alias spot.
Suggested by: Markus Armbruster
Signed-off-by: Eric Blake
---
v12: new patch
---
include/qapi/error.h | 14 ++
monitor.c| 2 +-
qapi/common.json | 5 +++--
qapi/qmp-dispatch.c | 2 +-
4 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/include/qapi/error.h b/include/qapi/error.h
index b2362a5..299cbf4 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -108,6 +108,20 @@
typedef struct Error Error;
/*
+ * Overall category of an error.
+ * Based on the qapi type QapiErrorClass, but reproduced here for nicer
+ * enum names.
+ */
+typedef enum ErrorClass {
+ERROR_CLASS_GENERIC_ERROR = QAPI_ERROR_CLASS_GENERIC_ERROR,
+ERROR_CLASS_COMMAND_NOT_FOUND = QAPI_ERROR_CLASS_COMMAND_NOT_FOUND,
+ERROR_CLASS_DEVICE_ENCRYPTED = QAPI_ERROR_CLASS_DEVICE_ENCRYPTED,
+ERROR_CLASS_DEVICE_NOT_ACTIVE = QAPI_ERROR_CLASS_DEVICE_NOT_ACTIVE,
+ERROR_CLASS_DEVICE_NOT_FOUND = QAPI_ERROR_CLASS_DEVICE_NOT_FOUND,
+ERROR_CLASS_KVM_MISSING_CAP = QAPI_ERROR_CLASS_KVM_MISSING_CAP,
+} ErrorClass;
+
+/*
* Get @err's human-readable error message.
*/
const char *error_get_pretty(Error *err);
diff --git a/monitor.c b/monitor.c
index 3a0df08..030e2b3 100644
--- a/monitor.c
+++ b/monitor.c
@@ -403,7 +403,7 @@ static QDict *build_qmp_error_dict(Error *err)
QObject *obj;
obj = qobject_from_jsonf("{ 'error': { 'class': %s, 'desc': %s } }",
- ErrorClass_lookup[error_get_class(err)],
+ QapiErrorClass_lookup[error_get_class(err)],
error_get_pretty(err));
return qobject_to_qdict(obj);
diff --git a/qapi/common.json b/qapi/common.json
index bad56bf..6fb40e7 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -3,7 +3,7 @@
# QAPI common definitions
##
-# @ErrorClass
+# @QapiErrorClass
#
# QEMU error classes
#
@@ -24,7 +24,8 @@
#
# Since: 1.2
##
-{ 'enum': 'ErrorClass',
+{ 'enum': 'QapiErrorClass',
+ # Keep this in sync with ErrorClass in error.h
'data': [ 'GenericError', 'CommandNotFound', 'DeviceEncrypted',
'DeviceNotActive', 'DeviceNotFound', 'KVMMissingCap' ] }
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 7bcc860..f36933d 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -114,7 +114,7 @@ static QObject *do_qmp_dispatch(QObject *request, Error
**errp)
QObject *qmp_build_error_object(Error *err)
{
return qobject_from_jsonf("{ 'class': %s, 'desc': %s }",
- ErrorClass_lookup[error_get_class(err)],
+ QapiErrorClass_lookup[error_get_class(err)],
error_get_pretty(err));
}
--
2.4.3
[Qemu-devel] [PATCH v12 15/36] qapi: Track owner of each object member
Future commits will migrate semantic checking away from parsing
and over to the various QAPISchema*.check() methods. But to
report an error message about an incorrect semantic use of a
member of an object type, it helps to know which type, command,
or event owns the member. In particular, when a member is
inherited from a base type, it is desirable to associate the
member name with the base type (and not the type calling
member.check()).
Rather than packing additional information into the seen array
passed to each member.check() (as in seen[m.name] = {'member':m,
'owner':type}), it is easier to have each member track the name
of the owner type in the first place (keeping things simpler
with the existing seen[m.name] = m). The new member.owner field
is set via a new set_owner() method, called when registering
the members and variants arrays with an object or variant type.
Track only a name, and not the actual type object, to avoid
creating a circular python reference chain.
Note that Variants.set_owner() method does not set the owner
for the tag_member field; this field is set earlier either as
part of an object's non-variant members, or explicitly by
alternates.
The source information is intended for human consumption in
error messages, and a new describe() method is added to access
the resulting information. For example, given the qapi:
{ 'command': 'foo', 'data': { 'string': 'str' } }
an implementation of visit_command() that calls
arg_type.members[0].describe()
will see "'string' (parameter of foo)".
To make the human-readable name of implicit types work without
duplicating efforts, the describe() method has to reverse the
name of implicit types, via the helper _pretty_owner().
No change to generated code.
Signed-off-by: Eric Blake
---
v12: no change
v11: set alternate tag_member owner during init, tweak comments,
keep events with '-args' instead of '-data', prefer '(parameter
of foo)'
v10 (now in subset C): rebase to latest, set alternate tag_member
owner from alternate
v9 (now in subset D): rebase to earlier changes, hoist 'role' to top
of class, split out _pretty_helper(), manage owner when tag_member
appears as part of local_members for unions
v8: don't munge implicit type names [except for event data], and
instead make describe() create nicer messages. Add set_owner(), and
use field 'role' instead of method _describe()
v7: total rewrite: rework implicit object names, assign owner
when initializing owner type rather than when creating member
python object
v6: rebase on new lazy array creation and simple union 'type'
motion; tweak commit message
---
scripts/qapi.py | 39 +--
1 file changed, 37 insertions(+), 2 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 6fc14be..79038a8 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -957,8 +957,10 @@ class QAPISchemaObjectType(QAPISchemaType):
assert base is None or isinstance(base, str)
for m in local_members:
assert isinstance(m, QAPISchemaObjectTypeMember)
-assert (variants is None or
-isinstance(variants, QAPISchemaObjectTypeVariants))
+m.set_owner(name)
+if variants is not None:
+assert isinstance(variants, QAPISchemaObjectTypeVariants)
+variants.set_owner(name)
self._base_name = base
self.base = None
self.local_members = local_members
@@ -1013,6 +1015,8 @@ class QAPISchemaObjectType(QAPISchemaType):
class QAPISchemaObjectTypeMember(object):
+role = 'member'
+
def __init__(self, name, typ, optional):
assert isinstance(name, str)
assert isinstance(typ, str)
@@ -1021,8 +1025,14 @@ class QAPISchemaObjectTypeMember(object):
self._type_name = typ
self.type = None
self.optional = optional
+self.owner = None
+
+def set_owner(self, name):
+assert not self.owner
+self.owner = name
def check(self, schema):
+assert self.owner
self.type = schema.lookup_type(self._type_name)
assert self.type
@@ -1031,6 +1041,22 @@ class QAPISchemaObjectTypeMember(object):
assert self.name not in seen
seen[self.name] = self
+def _pretty_owner(self):
+owner = self.owner
+if owner.startswith(':obj-'):
+# See QAPISchema._make_implicit_object_type() - reverse the
+# mapping there to create a nice human-readable description
+owner = owner[5:]
+if owner.endswith('-arg'):
+return '(parameter of %s)' % owner[:-4]
+else:
+assert owner.endswith('-wrapper')
+return '(branch of %s)' % owner[:-8]
+return '(%s of %s)' % (self.role, owner)
+
+def describe(self):
+return "'%s' %s" % (self.name, self._pretty_owner())
+
class QAPISchemaObjectTypeVariants(object):
def __init__(self, tag_name, tag_member, variants):
@@ -
[Qemu-devel] [PATCH v12 32/36] qapi: Inline _make_implicit_tag()
Now that alternates no longer use an implicit tag, we can
inline _make_implicit_tag() into its one caller of
_def_union_type().
No change to generated code.
Suggested-by: Markus Armbruster
Signed-off-by: Eric Blake
---
v12: new patch
---
scripts/qapi.py | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 3756d41..362afa1 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1342,11 +1342,6 @@ class QAPISchema(object):
typ, info, 'wrapper', [self._make_member('data', typ, info)])
return QAPISchemaObjectTypeVariant(case, typ)
-def _make_implicit_tag(self, type_name, info, variants):
-typ = self._make_implicit_enum_type(type_name, info,
-[v.name for v in variants])
-return QAPISchemaObjectTypeMember('type', typ, False)
-
def _def_union_type(self, expr, info):
name = expr['union']
data = expr['data']
@@ -1360,7 +1355,9 @@ class QAPISchema(object):
else:
variants = [self._make_simple_variant(key, value, info)
for (key, value) in data.iteritems()]
-tag_member = self._make_implicit_tag(name, info, variants)
+typ = self._make_implicit_enum_type(name, info,
+[v.name for v in variants])
+tag_member = QAPISchemaObjectTypeMember('type', typ, False)
members = [tag_member]
self._def_entity(
QAPISchemaObjectType(name, info, base, members,
--
2.4.3
[Qemu-devel] [PATCH v12 11/36] qapi: Check for qapi collisions involving variant members
Right now, our ad hoc parser ensures that we cannot have a
flat union that introduces any members that would clash with
non-variant members inherited from the union's base type (see
flat-union-clash-member.json). We want ObjectType.check() to
make the same check, so we can later reduce some of the ad
hoc checks.
We already have a map 'seen' of all non-variant members. We
still need to check for collisions between each variant type's
members and the non-variant ones.
To know the variant type's members, we need to call
variant.type.check(). This also detects when a type contains
itself in a variant, exactly like the existing base.check()
detects when a type contains itself as a base. (Except that
we currently forbid anything but a struct as the type of a
variant, so we can't actually trigger this type of loop yet.)
Slight complication: an alternate's variant can have arbitrary
type, but only an object type's check() may be called outside
QAPISchema.check(). We could either skip the call for variants
of alternates, or skip it for non-object types. For now, do
the latter, because it's easier.
Then we call each variant member's check_clash() with the
appropriate 'seen' map. Since members of different variants
can't clash, we have to clone a fresh seen for each variant.
Wrap this in a new helper method Variants.check_clash().
Note that cloning 'seen' inside Variants.check_clash() resembles
the one we just removed from Variants.check() in 'qapi: Drop
obsolete tag value collision assertions'; the difference here is
that we are now checking for clashes among the qapi members of
the variant type, rather than for a single clash with the variant
tag name itself.
Note that, by construction, collisions can't actually happen for
simple unions: each variant's type is a wrapper with a single
member 'data', which will never collide with the only non-variant
member 'type'.
For alternates, there's nothing for a variant object type's
members to clash with, and therefore no need to call the new
variants.check_clash().
No change to generated code.
Signed-off-by: Eric Blake
---
v12: retitle; use Markus' improved commit wording
v11: keep type.check() in check(), add comment to alternate
v10: create new Variants.check_clash() rather than piggybacking
on .check()
v9: new patch, split off from v8 7/17
---
scripts/qapi.py | 15 +++
1 file changed, 15 insertions(+)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index c6cb17b..b2d071f 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -992,6 +992,7 @@ class QAPISchemaObjectType(QAPISchemaType):
if self.variants:
self.variants.check(schema, seen)
assert self.variants.tag_member in self.members
+self.variants.check_clash(schema, seen)
def is_implicit(self):
# See QAPISchema._make_implicit_object_type()
@@ -1056,6 +1057,18 @@ class QAPISchemaObjectTypeVariants(object):
assert isinstance(self.tag_member.type, QAPISchemaEnumType)
for v in self.variants:
v.check(schema, self.tag_member.type)
+if isinstance(v.type, QAPISchemaObjectType):
+v.type.check(schema)
+
+def check_clash(self, schema, seen):
+for v in self.variants:
+# Reset seen map for each variant, since qapi names from one
+# branch do not affect another branch
+vseen = dict(seen)
+assert isinstance(v.type, QAPISchemaObjectType)
+assert not v.type.variants # not implemented
+for m in v.type.members:
+m.check_clash(vseen)
class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
@@ -1086,6 +1099,8 @@ class QAPISchemaAlternateType(QAPISchemaType):
def check(self, schema):
self.variants.tag_member.check(schema)
+# Not calling self.variants.check_clash(), because there's nothing
+# to clash with
self.variants.check(schema, {})
def json_type(self):
--
2.4.3
[Qemu-devel] [PATCH v12 33/36] qapi: Fix alternates that accept 'number' but not 'int'
The QMP input visitor allows integral values to be assigned by
promotion to a QTYPE_QFLOAT. However, when parsing an alternate,
we did not take this into account, such that an alternate that
accepts 'number' and some other type, but not 'int', would reject
integral values.
With this patch, we now have the following desirable table:
alternate has case selected for
'int' 'number'QTYPE_QINT QTYPE_QFLOAT
nono error error
no yes 'number''number'
yesno 'int' error
yes yes 'int' 'number'
While it is unlikely that we will ever use 'number' in an
alternate other than in the testsuite, it never hurts to be
more precise in what we allow.
Signed-off-by: Eric Blake
---
v12: rebase to QType cleanups
v11 (no v10): slight commit message tweak, rebase to earlier changes
v9: rebase to earlier changes
v8: no change
v7: rebase to named .u union
v6: rebase onto earlier testsuite and gen_err_check() improvements
---
include/qapi/visitor-impl.h| 2 +-
include/qapi/visitor.h | 3 ++-
qapi/qapi-visit-core.c | 4 ++--
qapi/qmp-input-visitor.c | 5 -
scripts/qapi-visit.py | 11 +++
tests/test-qmp-input-visitor.c | 16 ++--
6 files changed, 22 insertions(+), 19 deletions(-)
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 7cd1313..7419684 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -33,7 +33,7 @@ struct Visitor
void (*type_enum)(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name, Error **errp);
/* May be NULL; only needed for input visitors. */
-void (*get_next_type)(Visitor *v, QType *type,
+void (*get_next_type)(Visitor *v, QType *type, bool promote_int,
const char *name, Error **errp);
void (*type_int)(Visitor *v, int64_t *obj, const char *name, Error **errp);
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 6d25ad2..1414de1 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -43,8 +43,9 @@ void visit_optional(Visitor *v, bool *present, const char
*name,
* Determine the qtype of the item @name in the current object visit.
* For input visitors, set *@type to the correct qtype of a qapi
* alternate type; for other visitors, leave *@type unchanged.
+ * If @promote_int, treat integers as QTYPE_FLOAT.
*/
-void visit_get_next_type(Visitor *v, QType *type,
+void visit_get_next_type(Visitor *v, QType *type, bool promote_int,
const char *name, Error **errp);
void visit_type_enum(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name, Error **errp);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 850ca03..cee76bc 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -81,11 +81,11 @@ void visit_optional(Visitor *v, bool *present, const char
*name,
}
}
-void visit_get_next_type(Visitor *v, QType *type,
+void visit_get_next_type(Visitor *v, QType *type, bool promote_int,
const char *name, Error **errp)
{
if (v->get_next_type) {
-v->get_next_type(v, type, name, errp);
+v->get_next_type(v, type, promote_int, name, errp);
}
}
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index d398de7..26b7414 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -208,7 +208,7 @@ static void qmp_input_end_list(Visitor *v, Error **errp)
qmp_input_pop(qiv, errp);
}
-static void qmp_input_get_next_type(Visitor *v, QType *type,
+static void qmp_input_get_next_type(Visitor *v, QType *type, bool promote_int,
const char *name, Error **errp)
{
QmpInputVisitor *qiv = to_qiv(v);
@@ -219,6 +219,9 @@ static void qmp_input_get_next_type(Visitor *v, QType *type,
return;
}
*type = qobject_type(qobj);
+if (promote_int && *type == QTYPE_QINT) {
+*type = QTYPE_QFLOAT;
+}
}
static void qmp_input_type_int(Visitor *v, int64_t *obj, const char *name,
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index e8b53b3..035ea59 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -183,6 +183,11 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s *obj,
const char *name, Error
def gen_visit_alternate(name, variants):
+promote_int = 'true'
+for var in variants.variants:
+if var.type.alternate_qtype() == 'QTYPE_QINT':
+promote_int = 'false'
+
ret = mcgen('''
void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name,
Error **errp)
@@ -193,16 +198,14 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj,
const char *name, Error
if (err) {
goto out;
}
-visit_get_next_type(v, &(*obj)->type, name, &err);
+v
[Qemu-devel] [PATCH v12 18/36] qapi: Remove dead visitor code
Commit cbc95538 removed unused start_handle() and end_handle(), but forgot to remove their declarations. Signed-off-by: Eric Blake --- v12 (no v10-11): Split off obvious piece from v9 26/27 v9: no change v8: no change v7: no change v6: no change --- include/qapi/visitor.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h index cfc19a6..a2ad66c 100644 --- a/include/qapi/visitor.h +++ b/include/qapi/visitor.h @@ -27,9 +27,6 @@ typedef struct GenericList struct GenericList *next; } GenericList; -void visit_start_handle(Visitor *v, void **obj, const char *kind, -const char *name, Error **errp); -void visit_end_handle(Visitor *v, Error **errp); void visit_start_struct(Visitor *v, void **obj, const char *kind, const char *name, size_t size, Error **errp); void visit_end_struct(Visitor *v, Error **errp); -- 2.4.3
[Qemu-devel] [PATCH v12 21/36] qapi: Tighten the regex on valid names
We already documented that qapi names should match specific
patterns (such as starting with a letter unless it was an enum
value or a downstream extension). Tighten that from a suggestion
into a hard requirement, which frees up names beginning with a
single underscore for qapi internal usage. Also restrict things
to avoid 'a__b' or 'a--b' (that is, dash and underscore must not
occur consecutively).
Add a new test, reserved-member-underscore, to demonstrate the
tighter checking.
Signed-off-by: Eric Blake
---
v12: new patch
---
docs/qapi-code-gen.txt| 19 ++-
scripts/qapi.py | 12 +++-
tests/Makefile| 1 +
tests/qapi-schema/reserved-member-underscore.err | 1 +
tests/qapi-schema/reserved-member-underscore.exit | 1 +
tests/qapi-schema/reserved-member-underscore.json | 4
tests/qapi-schema/reserved-member-underscore.out | 0
7 files changed, 24 insertions(+), 14 deletions(-)
create mode 100644 tests/qapi-schema/reserved-member-underscore.err
create mode 100644 tests/qapi-schema/reserved-member-underscore.exit
create mode 100644 tests/qapi-schema/reserved-member-underscore.json
create mode 100644 tests/qapi-schema/reserved-member-underscore.out
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index ceb9a78..ec225d1 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -118,17 +118,18 @@ tracking optional fields.
Any name (command, event, type, field, or enum value) beginning with
"x-" is marked experimental, and may be withdrawn or changed
-incompatibly in a future release. Downstream vendors may add
-extensions; such extensions should begin with a prefix matching
+incompatibly in a future release. All names must begin with a letter,
+and contain only ASCII letters, digits, dash, and underscore, where
+dash and underscore cannot occur consecutively. There are two
+exceptions: enum values may start with a digit, and any extensions
+added by downstream vendors should start with a prefix matching
"__RFQDN_" (for the reverse-fully-qualified-domain-name of the
vendor), even if the rest of the name uses dash (example:
-__com.redhat_drive-mirror). Other than downstream extensions (with
-leading underscore and the use of dots), all names should begin with a
-letter, and contain only ASCII letters, digits, dash, and underscore.
-Names beginning with 'q_' are reserved for the generator: QMP names
-that resemble C keywords or other problematic strings will be munged
-in C to use this prefix. For example, a field named "default" in
-qapi becomes "q_default" in the generated C code.
+__com.redhat_drive-mirror). Names beginning with 'q_' are reserved
+for the generator: QMP names that resemble C keywords or other
+problematic strings will be munged in C to use this prefix. For
+example, a field named "default" in qapi becomes "q_default" in the
+generated C code.
In the rest of this document, usage lines are given for each
expression type, with literal strings written in lower case and
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 4a30bc0..e6d014b 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -353,9 +353,11 @@ def discriminator_find_enum_define(expr):
return find_enum(discriminator_type)
-# FIXME should enforce "other than downstream extensions [...], all
-# names should begin with a letter".
-valid_name = re.compile('^[a-zA-Z_][a-zA-Z0-9_.-]*$')
+# Names must be letters, numbers, -, and _. They must start with letter,
+# except for downstream extensions which must start with __RFQDN_.
+# Dots are only valid in the downstream extension prefix.
+valid_name = re.compile('^(__[a-zA-Z][a-zA-Z0-9.]*_)?'
+'[a-zA-Z][a-zA-Z0-9]*([_-][a-zA-Z0-9]+)*$')
def check_name(expr_info, source, name, allow_optional=False,
@@ -374,8 +376,8 @@ def check_name(expr_info, source, name,
allow_optional=False,
% (source, name))
# Enum members can start with a digit, because the generated C
# code always prefixes it with the enum name
-if enum_member:
-membername = '_' + membername
+if enum_member and membername[0].isdigit():
+membername = 'D' + membername
# Reserve the entire 'q_' namespace for c_name()
if not valid_name.match(membername) or \
c_name(membername, False).startswith('q_'):
diff --git a/tests/Makefile b/tests/Makefile
index 1c2c8ee..b70d246 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -320,6 +320,7 @@ qapi-schema += reserved-command-q.json
qapi-schema += reserved-member-has.json
qapi-schema += reserved-member-q.json
qapi-schema += reserved-member-u.json
+qapi-schema += reserved-member-underscore.json
qapi-schema += reserved-type-kind.json
qapi-schema += reserved-type-list.json
qapi-schema += returns-alternate.json
diff --git a/tests/qapi-schema/reserved-member-underscore.err
b/tests/qapi-schema/reserved-mem
[Qemu-devel] [PATCH v12 30/36] qapi: Convert QType into qapi builtin enum type
What's more meta than using qapi to define qapi? :)
Convert QType into a full-fledged[*] builtin qapi enum type, so
that a subsequent patch can then use it as the discriminator
type of qapi alternate types. Fortunately, the judicious use of
'prefix' in the qapi definition avoids churn to the spelling of
the enum constants.
To avoid circular definitions, we have to flip the order of
inclusion between "qobject.h" vs. "qapi-types.h". Back in commit
28770e0, we had the latter include the former, so that we could
use 'QObject *' for our implementation of 'any'. But that usage
also works with only a forward declaration, whereas the
definition of QObject requires QType to be a complete type.
[*] The type has to be builtin, rather than declared in
qapi/common.json, because we want to use it for alternates even
when common.json is not included. But since it is the first
builtin enum type, we have to add special cases to qapi-types
and qapi-visit to only emit definitions once, even when two
qapi files are being compiled into the same binary (the way we
already handled builtin list types like 'intList'). We may
need to revisit how multiple qapi files share common types,
but that's a project for another day.
Signed-off-by: Eric Blake
---
v12: split out preparatory renames, retitle patch, use info rather
than name comparison
v11: new patch
---
docs/qapi-code-gen.txt | 1 +
include/qapi/qmp/qobject.h | 15 +--
scripts/qapi-types.py| 16
scripts/qapi-visit.py| 11 +--
scripts/qapi.py | 6 ++
tests/qapi-schema/alternate-empty.out| 2 ++
tests/qapi-schema/comments.out | 2 ++
tests/qapi-schema/empty.out | 2 ++
tests/qapi-schema/event-case.out | 2 ++
tests/qapi-schema/flat-union-empty.out | 2 ++
tests/qapi-schema/ident-with-escape.out | 2 ++
tests/qapi-schema/include-relpath.out| 2 ++
tests/qapi-schema/include-repetition.out | 2 ++
tests/qapi-schema/include-simple.out | 2 ++
tests/qapi-schema/indented-expr.out | 2 ++
tests/qapi-schema/qapi-schema-test.out | 2 ++
tests/qapi-schema/union-clash-data.out | 2 ++
tests/qapi-schema/union-empty.out| 2 ++
18 files changed, 55 insertions(+), 20 deletions(-)
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 1f6cb16..4edad89 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -168,6 +168,7 @@ The following types are predefined, and map to C as follows:
accepts size suffixes
bool bool JSON true or false
any QObject * any JSON value
+ QType QType JSON string matching enum QType values
=== Includes ===
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index 0558ff3..16bf50d 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -34,20 +34,7 @@
#include
#include
-
-typedef enum {
-QTYPE_NONE,/* sentinel value, no QObject has this type code */
-QTYPE_QNULL,
-QTYPE_QINT,
-QTYPE_QSTRING,
-QTYPE_QDICT,
-QTYPE_QLIST,
-QTYPE_QFLOAT,
-QTYPE_QBOOL,
-QTYPE_MAX,
-} QType;
-
-typedef struct QObject QObject;
+#include "qapi-types.h"
struct QObject {
QType type;
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 2f2f7df..99346ad 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -112,7 +112,7 @@ extern const int %(c_name)s_qtypes[];
def gen_alternate_qtypes(name, variants):
ret = mcgen('''
-const int %(c_name)s_qtypes[QTYPE_MAX] = {
+const int %(c_name)s_qtypes[QTYPE__MAX] = {
''',
c_name=c_name(name))
@@ -233,8 +233,15 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
self.defn += gen_type_cleanup(name)
def visit_enum_type(self, name, info, values, prefix):
-self._fwdecl += gen_enum(name, values, prefix)
-self._fwdefn += gen_enum_lookup(name, values, prefix)
+# Special case for our lone builtin enum type
+# TODO use something cleaner than existence of info
+if not info:
+self._btin += gen_enum(name, values, prefix)
+if do_builtins:
+self.defn += gen_enum_lookup(name, values, prefix)
+else:
+self._fwdecl += gen_enum(name, values, prefix)
+self._fwdefn += gen_enum_lookup(name, values, prefix)
def visit_array_type(self, name, info, element_type):
if isinstance(element_type, QAPISchemaBuiltinType):
@@ -319,7 +326,8 @@ fdef.write(mcgen('''
fdecl.write(mcgen('''
#include
#include
-#include "qapi/qmp/qobject.h"
+
+typedef struct QObject QObject;
'''))
schema = QAPISchema(input_file)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 94cd113..7ceda18 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -347,8 +347,15 @@ class QAPISchemaGenVisitVisitor(QAPISchem
[Qemu-devel] [PATCH v12 17/36] qapi: Fix c_name() munging
The method c_name() is supposed to do two different actions: munge
'-' into '_', and add a 'q_' prefix to ticklish names. But it did
these steps out of order, making it possible to submit input that
is not ticklish until after munging, where the output then lacked
the desired prefix.
The failure is exposed easily if you have a compiler that recognizes
C11 keywords, and try to name a member '_Thread-local', as it would
result in trying to compile the declaration 'uint64_t _Thread_local;'
which is not valid. However, this name violates our conventions
(ultimately, want to enforce that no qapi names start with single
underscore), so the test is slightly weaker by instead testing
'wchar-t'; the declaration 'uint64_t wchar_t;' is valid in C (where
wchar_t is only a typedef) but fails with a C++ compiler (where it
is a keyword).
Fix things by reversing the order of actions within c_name().
Signed-off-by: Eric Blake
---
v12: new patch
---
scripts/qapi.py | 3 ++-
tests/qapi-schema/qapi-schema-test.json | 5 +++--
tests/qapi-schema/qapi-schema-test.out | 1 +
tests/test-qmp-commands.c | 4
4 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index b068141..4a30bc0 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1481,10 +1481,11 @@ def c_name(name, protect=True):
'not_eq', 'or', 'or_eq', 'xor', 'xor_eq'])
# namespace pollution:
polluted_words = set(['unix', 'errno'])
+name = name.translate(c_name_trans)
if protect and (name in c89_words | c99_words | c11_words | gcc_words
| cpp_words | polluted_words):
return "q_" + name
-return name.translate(c_name_trans)
+return name
eatspace = '\033EATSPACE.'
pointer_suffix = ' *' + eatspace
diff --git a/tests/qapi-schema/qapi-schema-test.json
b/tests/qapi-schema/qapi-schema-test.json
index 44638da..4b89527 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -152,12 +152,13 @@
{ 'event': 'EVENT_D',
'data': { 'a' : 'EventStructOne', 'b' : 'str', '*c': 'str', '*enum3':
'EnumOne' } }
-# test that we correctly compile downstream extensions
+# test that we correctly compile downstream extensions, as well as munge
+# ticklish names
{ 'enum': '__org.qemu_x-Enum', 'data': [ '__org.qemu_x-value' ] }
{ 'struct': '__org.qemu_x-Base',
'data': { '__org.qemu_x-member1': '__org.qemu_x-Enum' } }
{ 'struct': '__org.qemu_x-Struct', 'base': '__org.qemu_x-Base',
- 'data': { '__org.qemu_x-member2': 'str' } }
+ 'data': { '__org.qemu_x-member2': 'str', '*wchar-t': 'int' } }
{ 'union': '__org.qemu_x-Union1', 'data': { '__org.qemu_x-branch': 'str' } }
{ 'struct': '__org.qemu_x-Struct2',
'data': { 'array': ['__org.qemu_x-Union1'] } }
diff --git a/tests/qapi-schema/qapi-schema-test.out
b/tests/qapi-schema/qapi-schema-test.out
index 786024e..0724a9f 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -185,6 +185,7 @@ enum __org.qemu_x-Enum ['__org.qemu_x-value']
object __org.qemu_x-Struct
base __org.qemu_x-Base
member __org.qemu_x-member2: str optional=False
+member wchar-t: int optional=True
object __org.qemu_x-Struct2
member array: __org.qemu_x-Union1List optional=False
object __org.qemu_x-Union1
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index 888fb5f..9f35b80 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -65,6 +65,10 @@ __org_qemu_x_Union1
*qmp___org_qemu_x_command(__org_qemu_x_EnumList *a,
ret->type = ORG_QEMU_X_UNION1_KIND___ORG_QEMU_X_BRANCH;
ret->u.__org_qemu_x_branch = strdup("blah1");
+/* Also test that 'wchar-t' was munged to 'q_wchar_t' */
+if (b && b->value && !b->value->has_q_wchar_t) {
+b->value->q_wchar_t = 1;
+}
return ret;
}
--
2.4.3
[Qemu-devel] [PATCH v12 36/36] qapi: Shorter visits of optional fields
For less code, reflect the determined boolean value of an optional
visit back to the caller instead of making the caller read the
boolean after the fact.
The resulting generated code has the following diff:
|-visit_optional(v, &has_fdset_id, "fdset-id");
|-if (has_fdset_id) {
|+if (visit_optional(v, &has_fdset_id, "fdset-id")) {
| visit_type_int(v, &fdset_id, "fdset-id", &err);
| if (err) {
| goto out;
| }
| }
Signed-off-by: Eric Blake
---
v12: split error removal from 'if' compression
v11 (no v10): no change
v9: no change
v8: no change
v7: rebase to no member.c_name()
v6: rebase onto earlier testsuite and gen_err_check() improvements
---
include/qapi/visitor.h | 4 ++--
qapi/qapi-visit-core.c | 3 ++-
scripts/qapi.py| 3 +--
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 9be60d4..a14a16d 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -41,9 +41,9 @@ void visit_end_list(Visitor *v, Error **errp);
* Check if an optional member @name of an object needs visiting.
* For input visitors, set *@present according to whether the
* corresponding visit_type_*() needs calling; for other visitors,
- * leave *@present unchanged.
+ * leave *@present unchanged. Return *@present for convenience.
*/
-void visit_optional(Visitor *v, bool *present, const char *name);
+bool visit_optional(Visitor *v, bool *present, const char *name);
/**
* Determine the qtype of the item @name in the current object visit.
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index e07d6f9..6d63e40 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -73,11 +73,12 @@ void visit_end_union(Visitor *v, bool data_present, Error
**errp)
}
}
-void visit_optional(Visitor *v, bool *present, const char *name)
+bool visit_optional(Visitor *v, bool *present, const char *name)
{
if (v->optional) {
v->optional(v, present, name);
}
+return *present;
}
void visit_get_next_type(Visitor *v, QType *type, bool promote_int,
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 78b6400..ae70a2d 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1680,8 +1680,7 @@ def gen_visit_fields(members, prefix='', need_cast=False,
skiperr=False):
for memb in members:
if memb.optional:
ret += mcgen('''
-visit_optional(v, &%(prefix)shas_%(c_name)s, "%(name)s");
-if (%(prefix)shas_%(c_name)s) {
+if (visit_optional(v, &%(prefix)shas_%(c_name)s, "%(name)s")) {
''',
prefix=prefix, c_name=c_name(memb.name),
name=memb.name, errp=errparg)
--
2.4.3
[Qemu-devel] [PATCH v12 19/36] blkdebug: Merge hand-rolled and qapi BlkdebugEvent enum
No need to keep two separate enums, where editing one is likely
to forget the other. Now that we can specify a qapi enum prefix,
we don't even have to change the bulk of the uses.
get_event_by_name() could perhaps be replaced by qapi_enum_parse(),
but I left that for another day.
CC: Kevin Wolf
Signed-off-by: Eric Blake
---
v12: new patch
---
block.c | 2 +-
block/blkdebug.c | 79 +++
docs/blkdebug.txt | 7 +++--
include/block/block.h | 62 +
include/block/block_int.h | 2 +-
qapi/block-core.json | 4 ++-
6 files changed, 21 insertions(+), 135 deletions(-)
diff --git a/block.c b/block.c
index 3a7324b..9971976 100644
--- a/block.c
+++ b/block.c
@@ -2851,7 +2851,7 @@ ImageInfoSpecific
*bdrv_get_specific_info(BlockDriverState *bs)
return NULL;
}
-void bdrv_debug_event(BlockDriverState *bs, BlkDebugEvent event)
+void bdrv_debug_event(BlockDriverState *bs, BlkdebugEvent event)
{
if (!bs || !bs->drv || !bs->drv->bdrv_debug_event) {
return;
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 6860a2b..76805a6 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -35,7 +35,7 @@ typedef struct BDRVBlkdebugState {
int state;
int new_state;
-QLIST_HEAD(, BlkdebugRule) rules[BLKDBG_EVENT_MAX];
+QLIST_HEAD(, BlkdebugRule) rules[BLKDBG_MAX];
QSIMPLEQ_HEAD(, BlkdebugRule) active_rules;
QLIST_HEAD(, BlkdebugSuspendedReq) suspended_reqs;
} BDRVBlkdebugState;
@@ -63,7 +63,7 @@ enum {
};
typedef struct BlkdebugRule {
-BlkDebugEvent event;
+BlkdebugEvent event;
int action;
int state;
union {
@@ -142,69 +142,12 @@ static QemuOptsList *config_groups[] = {
NULL
};
-static const char *event_names[BLKDBG_EVENT_MAX] = {
-[BLKDBG_L1_UPDATE] = "l1_update",
-[BLKDBG_L1_GROW_ALLOC_TABLE]= "l1_grow.alloc_table",
-[BLKDBG_L1_GROW_WRITE_TABLE]= "l1_grow.write_table",
-[BLKDBG_L1_GROW_ACTIVATE_TABLE] = "l1_grow.activate_table",
-
-[BLKDBG_L2_LOAD]= "l2_load",
-[BLKDBG_L2_UPDATE] = "l2_update",
-[BLKDBG_L2_UPDATE_COMPRESSED] = "l2_update_compressed",
-[BLKDBG_L2_ALLOC_COW_READ] = "l2_alloc.cow_read",
-[BLKDBG_L2_ALLOC_WRITE] = "l2_alloc.write",
-
-[BLKDBG_READ_AIO] = "read_aio",
-[BLKDBG_READ_BACKING_AIO] = "read_backing_aio",
-[BLKDBG_READ_COMPRESSED]= "read_compressed",
-
-[BLKDBG_WRITE_AIO] = "write_aio",
-[BLKDBG_WRITE_COMPRESSED] = "write_compressed",
-
-[BLKDBG_VMSTATE_LOAD] = "vmstate_load",
-[BLKDBG_VMSTATE_SAVE] = "vmstate_save",
-
-[BLKDBG_COW_READ] = "cow_read",
-[BLKDBG_COW_WRITE] = "cow_write",
-
-[BLKDBG_REFTABLE_LOAD] = "reftable_load",
-[BLKDBG_REFTABLE_GROW] = "reftable_grow",
-[BLKDBG_REFTABLE_UPDATE]= "reftable_update",
-
-[BLKDBG_REFBLOCK_LOAD] = "refblock_load",
-[BLKDBG_REFBLOCK_UPDATE]= "refblock_update",
-[BLKDBG_REFBLOCK_UPDATE_PART] = "refblock_update_part",
-[BLKDBG_REFBLOCK_ALLOC] = "refblock_alloc",
-[BLKDBG_REFBLOCK_ALLOC_HOOKUP] = "refblock_alloc.hookup",
-[BLKDBG_REFBLOCK_ALLOC_WRITE] = "refblock_alloc.write",
-[BLKDBG_REFBLOCK_ALLOC_WRITE_BLOCKS]= "refblock_alloc.write_blocks",
-[BLKDBG_REFBLOCK_ALLOC_WRITE_TABLE] = "refblock_alloc.write_table",
-[BLKDBG_REFBLOCK_ALLOC_SWITCH_TABLE]= "refblock_alloc.switch_table",
-
-[BLKDBG_CLUSTER_ALLOC] = "cluster_alloc",
-[BLKDBG_CLUSTER_ALLOC_BYTES]= "cluster_alloc_bytes",
-[BLKDBG_CLUSTER_FREE] = "cluster_free",
-
-[BLKDBG_FLUSH_TO_OS]= "flush_to_os",
-[BLKDBG_FLUSH_TO_DISK] = "flush_to_disk",
-
-[BLKDBG_PWRITEV_RMW_HEAD] = "pwritev_rmw.head",
-[BLKDBG_PWRITEV_RMW_AFTER_HEAD] = "pwritev_rmw.after_head",
-[BLKDBG_PWRITEV_RMW_TAIL] = "pwritev_rmw.tail",
-[BLKDBG_PWRITEV_RMW_AFTER_TAIL] = "pwritev_rmw.after_tail",
-[BLKDBG_PWRITEV]= "pwritev",
-[BLKDBG_PWRITEV_ZERO] = "pwritev_zero",
-[BLKDBG_PWRITEV_DONE] = "pwritev_done",
-
-[BLKDBG_EMPTY_IMAGE_PREPARE]= "empty_image_prepare",
-};
-
-static int get_event_by_name(const char *name, BlkDebugEvent *event)
+static int get_event_by_name(const char *name, BlkdebugEvent *event)
{
int i;
-for (i = 0; i < BLKDBG_EVENT_MAX; i++) {
-if (!strcmp(event_names[i], name)) {
+for (i = 0; i < BL
[Qemu-devel] [PATCH v12 20/36] blkdebug: Avoid '.' in enum values
Our qapi conventions document that '.' should only be used in
the prefix of downstream names. BlkdebugEvent was a lone
exception to this. Changing this is not backwards compatible
to the 'blockdev-add' QMP command; however, that command is
not yet fully stable. It can also be argued that the testsuite
is the biggest user of blkdebug, and that any other user can
be taught to deal with the change by paying attention to
introspection results.
Done with:
$ for str in \
l1_grow.{alloc,write,activate}_table \
l2_alloc.{cow_read,write} \
refblock_alloc.{hookup,write,write_blocks,write_table,switch_table} \
pwritev_rmw.{head,after_head,tail,after_tail}; do
str1=$(echo "$str" | sed 's/\./\\./')
str2=$(echo "$str" | sed 's/\./_/')
git grep -l "$str1" | xargs -r sed -i "s/$str1/$str2/g"
done
followed by a manual touchup to test 77 to keep the test working.
Reported-by: Markus Armbruster
CC: Kevin Wolf
Signed-off-by: Eric Blake
---
v12: new commit. Arguably, test 77 should have failed immediately
on an unrecognized event name instead of hanging forever, but that
fix belongs in a separate patch, not part of this series.
---
qapi/block-core.json | 16
tests/qemu-iotests/026 | 18 -
tests/qemu-iotests/026.out | 80 +++---
tests/qemu-iotests/026.out.nocache | 80 +++---
tests/qemu-iotests/077 | 24 ++--
5 files changed, 109 insertions(+), 109 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 603eb60..190b4e5 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1780,19 +1780,19 @@
# Since: 2.0
##
{ 'enum': 'BlkdebugEvent', 'prefix': 'BLKDBG',
- 'data': [ 'l1_update', 'l1_grow.alloc_table', 'l1_grow.write_table',
-'l1_grow.activate_table', 'l2_load', 'l2_update',
-'l2_update_compressed', 'l2_alloc.cow_read', 'l2_alloc.write',
+ 'data': [ 'l1_update', 'l1_grow_alloc_table', 'l1_grow_write_table',
+'l1_grow_activate_table', 'l2_load', 'l2_update',
+'l2_update_compressed', 'l2_alloc_cow_read', 'l2_alloc_write',
'read_aio', 'read_backing_aio', 'read_compressed', 'write_aio',
'write_compressed', 'vmstate_load', 'vmstate_save', 'cow_read',
'cow_write', 'reftable_load', 'reftable_grow', 'reftable_update',
'refblock_load', 'refblock_update', 'refblock_update_part',
-'refblock_alloc', 'refblock_alloc.hookup', 'refblock_alloc.write',
-'refblock_alloc.write_blocks', 'refblock_alloc.write_table',
-'refblock_alloc.switch_table', 'cluster_alloc',
+'refblock_alloc', 'refblock_alloc_hookup', 'refblock_alloc_write',
+'refblock_alloc_write_blocks', 'refblock_alloc_write_table',
+'refblock_alloc_switch_table', 'cluster_alloc',
'cluster_alloc_bytes', 'cluster_free', 'flush_to_os',
-'flush_to_disk', 'pwritev_rmw.head', 'pwritev_rmw.after_head',
-'pwritev_rmw.tail', 'pwritev_rmw.after_tail', 'pwritev',
+'flush_to_disk', 'pwritev_rmw_head', 'pwritev_rmw_after_head',
+'pwritev_rmw_tail', 'pwritev_rmw_after_tail', 'pwritev',
'pwritev_zero', 'pwritev_done', 'empty_image_prepare' ] }
##
diff --git a/tests/qemu-iotests/026 b/tests/qemu-iotests/026
index 0fc3244..ba1047c 100755
--- a/tests/qemu-iotests/026
+++ b/tests/qemu-iotests/026
@@ -66,7 +66,7 @@ for event in \
\
l2_load \
l2_update \
-l2_alloc.write \
+l2_alloc_write \
\
write_aio \
\
@@ -126,11 +126,11 @@ CLUSTER_SIZE=512
for event in \
-refblock_alloc.hookup \
-refblock_alloc.write \
-refblock_alloc.write_blocks \
-refblock_alloc.write_table \
-refblock_alloc.switch_table \
+refblock_alloc_hookup \
+refblock_alloc_write \
+refblock_alloc_write_blocks \
+refblock_alloc_write_table \
+refblock_alloc_switch_table \
do
@@ -170,9 +170,9 @@ CLUSTER_SIZE=1024
for event in \
-l1_grow.alloc_table \
-l1_grow.write_table \
-l1_grow.activate_table \
+l1_grow_alloc_table \
+l1_grow_write_table \
+l1_grow_activate_table \
do
diff --git a/tests/qemu-iotests/026.out b/tests/qemu-iotests/026.out
index 5e964fb..d84d82c 100644
--- a/tests/qemu-iotests/026.out
+++ b/tests/qemu-iotests/026.out
@@ -195,24 +195,24 @@ write failed: No space left on device
This means waste of disk space, but no harm to data.
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
-Event: l2_alloc.write; errno: 5; imm: off; once: on; write
+Event: l2_alloc_write; errno: 5; imm: off; once: on; write
write failed: Input/output error
No errors were found on the image.
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
-Event: l2_alloc.write; errno: 5; imm: off; once: on; write -b
+Event: l2_alloc_write; errno: 5; imm: off; once: on; write -b
write
[Qemu-devel] [PATCH v12 24/36] cpu: Convert CpuInfo into flat union
The CpuInfo struct is used only by the 'query-cpus' output
command, so we are free to modify it by adding fields (clients
are already supposed to ignore unknown output fields), or by
changing optional members to mandatory, while still keeping
QMP wire compatibility with older versions of qemu.
When qapi type CpuInfo was originally created for 0.14, we had
no notion of a flat union, and instead just listed a bunch of
optional fields with documentation about the mutually-exclusive
choice of which instruction pointer field(s) would be provided
for a given architecture. But now that we have flat unions and
introspection, it is better to segregate off which fields will
be provided according to the actual architecture. With this in
place, we no longer need the fields to be optional, because the
choice of the new 'arch' discriminator serves that role.
This has an additional benefit: the old all-in-one struct was
the only place in the code base that had a case-sensitive
naming of members 'pc' vs. 'PC'. Separating these spellings
into different branches of the flat union will allow us to add
restrictions against future case-insensitive collisions, since
that is generally a poor interface practice.
Signed-off-by: Eric Blake
---
v12: s/2.5/2.6/, typo fix
v11: also fix qmp-commands.hx, improve commit message. If this
misses 2.5 (likely), it will need updates to call out 2.6
v10: new patch
---
cpus.c | 31 --
hmp.c| 30 +-
qapi-schema.json | 120 ++-
qmp-commands.hx | 4 ++
4 files changed, 143 insertions(+), 42 deletions(-)
diff --git a/cpus.c b/cpus.c
index 877bd70..77ba15b 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1556,22 +1556,29 @@ CpuInfoList *qmp_query_cpus(Error **errp)
info->value->qom_path = object_get_canonical_path(OBJECT(cpu));
info->value->thread_id = cpu->thread_id;
#if defined(TARGET_I386)
-info->value->has_pc = true;
-info->value->pc = env->eip + env->segs[R_CS].base;
+info->value->arch = CPU_INFO_ARCH_X86;
+info->value->u.x86 = g_new0(CpuInfoX86, 1);
+info->value->u.x86->pc = env->eip + env->segs[R_CS].base;
#elif defined(TARGET_PPC)
-info->value->has_nip = true;
-info->value->nip = env->nip;
+info->value->arch = CPU_INFO_ARCH_PPC;
+info->value->u.ppc = g_new0(CpuInfoPpc, 1);
+info->value->u.ppc->nip = env->nip;
#elif defined(TARGET_SPARC)
-info->value->has_pc = true;
-info->value->pc = env->pc;
-info->value->has_npc = true;
-info->value->npc = env->npc;
+info->value->arch = CPU_INFO_ARCH_SPARC;
+info->value->u.sparc = g_new0(CpuInfoSPARC, 1);
+info->value->u.sparc->pc = env->pc;
+info->value->u.sparc->npc = env->npc;
#elif defined(TARGET_MIPS)
-info->value->has_PC = true;
-info->value->PC = env->active_tc.PC;
+info->value->arch = CPU_INFO_ARCH_MIPS;
+info->value->u.mips = g_new0(CpuInfoMips, 1);
+info->value->u.mips->PC = env->active_tc.PC;
#elif defined(TARGET_TRICORE)
-info->value->has_PC = true;
-info->value->PC = env->PC;
+info->value->arch = CPU_INFO_ARCH_TRICORE;
+info->value->u.tricore = g_new0(CpuInfoTricore, 1);
+info->value->u.tricore->PC = env->PC;
+#else
+info->value->arch = CPU_INFO_ARCH_OTHER;
+info->value->u.other = g_new0(CpuInfoOther, 1);
#endif
/* XXX: waiting for the qapi to support GSList */
diff --git a/hmp.c b/hmp.c
index 1b402c4..c2b2c16 100644
--- a/hmp.c
+++ b/hmp.c
@@ -311,17 +311,25 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
monitor_printf(mon, "%c CPU #%" PRId64 ":", active, cpu->value->CPU);
-if (cpu->value->has_pc) {
-monitor_printf(mon, " pc=0x%016" PRIx64, cpu->value->pc);
-}
-if (cpu->value->has_nip) {
-monitor_printf(mon, " nip=0x%016" PRIx64, cpu->value->nip);
-}
-if (cpu->value->has_npc) {
-monitor_printf(mon, " npc=0x%016" PRIx64, cpu->value->npc);
-}
-if (cpu->value->has_PC) {
-monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->PC);
+switch (cpu->value->arch) {
+case CPU_INFO_ARCH_X86:
+monitor_printf(mon, " pc=0x%016" PRIx64, cpu->value->u.x86->pc);
+break;
+case CPU_INFO_ARCH_PPC:
+monitor_printf(mon, " nip=0x%016" PRIx64, cpu->value->u.ppc->nip);
+break;
+case CPU_INFO_ARCH_SPARC:
+monitor_printf(mon, " pc=0x%016" PRIx64, cpu->value->u.sparc->pc);
+monitor_printf(mon, " npc=0x%016" PRIx64,
cpu->value->u.sparc->npc);
+break;
+case CPU_INFO_ARCH_MIPS:
+monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.mips->PC);
+break;
+case CPU_INFO_ARCH_TRICORE:
+monitor_printf(mon, " PC=0x%016" PRIx64,
[Qemu-devel] [PATCH v12 26/36] qapi: Change munging of CamelCase enum values
When munging enum values, the fact that we were passing the entire
prefix + value through camel_to_upper() meant that enum values
spelled with CamelCase could be turned into CAMEL_CASE. However,
this provides a potential collision (both OneTwo and One-Two would
munge into ONE_TWO) for enum types, when the same two names are
valid side-by-side as qapi member names. By changing the generation
of enum constants to always be prefix + '_' + c_name(value,
False).upper(), and ensuring that there are no case collisions (in
the next patches), we no longer have to worry about names that
would be distinct as qapi members but collide as variant tag names,
without having to think about what munging the heuristics in
camel_to_upper() will actually perform on an enum value.
Making the change will affect enums that did not follow coding
conventions, using 'CamelCase' rather than desired 'lower-case'.
Thankfully, there are only two culprits: InputButton and ErrorClass.
We already tweaked ErrorClass to make it an alias of QapiErrorClass,
where only the alias needs changingrather than the whole tree. So
the bulk of this change is modifying INPUT_BUTTON_WHEEL_UP to the
new INPUT_BUTTON_WHEELUP (and likewise for WHEELDOWN). That part
of this commit may later need reverting if we rename the enum
constants from 'WheelUp' to 'wheel-up' as part of moving
x-input-send-event to a stable interface; but at least we have
documentation bread crumbs in place to remind us (commit 513e7cd),
and it matches the fact that SDL constants are also spelled
SDL_BUTTON_WHEELUP.
Suggested by: Markus Armbruster
Signed-off-by: Eric Blake
---
v12: rebase to simpler ErrorClass changes, document decisions on
InputButton
v11: new patch
---
hw/input/hid.c | 4 ++--
hw/input/ps2.c | 4 ++--
hw/input/virtio-input-hid.c | 4 ++--
include/qapi/error.h| 12 ++--
monitor.c | 2 +-
scripts/qapi.py | 2 +-
ui/cocoa.m | 4 ++--
ui/gtk.c| 4 ++--
ui/input-legacy.c | 4 ++--
ui/sdl.c| 4 ++--
ui/sdl2.c | 4 ++--
ui/spice-input.c| 4 ++--
ui/vnc.c| 4 ++--
13 files changed, 28 insertions(+), 28 deletions(-)
diff --git a/hw/input/hid.c b/hw/input/hid.c
index 12075c9..3221d29 100644
--- a/hw/input/hid.c
+++ b/hw/input/hid.c
@@ -139,9 +139,9 @@ static void hid_pointer_event(DeviceState *dev, QemuConsole
*src,
case INPUT_EVENT_KIND_BTN:
if (evt->u.btn->down) {
e->buttons_state |= bmap[evt->u.btn->button];
-if (evt->u.btn->button == INPUT_BUTTON_WHEEL_UP) {
+if (evt->u.btn->button == INPUT_BUTTON_WHEELUP) {
e->dz--;
-} else if (evt->u.btn->button == INPUT_BUTTON_WHEEL_DOWN) {
+} else if (evt->u.btn->button == INPUT_BUTTON_WHEELDOWN) {
e->dz++;
}
} else {
diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 9096d21..79754cd 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -405,9 +405,9 @@ static void ps2_mouse_event(DeviceState *dev, QemuConsole
*src,
case INPUT_EVENT_KIND_BTN:
if (evt->u.btn->down) {
s->mouse_buttons |= bmap[evt->u.btn->button];
-if (evt->u.btn->button == INPUT_BUTTON_WHEEL_UP) {
+if (evt->u.btn->button == INPUT_BUTTON_WHEELUP) {
s->mouse_dz--;
-} else if (evt->u.btn->button == INPUT_BUTTON_WHEEL_DOWN) {
+} else if (evt->u.btn->button == INPUT_BUTTON_WHEELDOWN) {
s->mouse_dz++;
}
} else {
diff --git a/hw/input/virtio-input-hid.c b/hw/input/virtio-input-hid.c
index 5d00a03..a78d11c 100644
--- a/hw/input/virtio-input-hid.c
+++ b/hw/input/virtio-input-hid.c
@@ -142,8 +142,8 @@ static const unsigned int keymap_button[INPUT_BUTTON__MAX]
= {
[INPUT_BUTTON_LEFT] = BTN_LEFT,
[INPUT_BUTTON_RIGHT] = BTN_RIGHT,
[INPUT_BUTTON_MIDDLE]= BTN_MIDDLE,
-[INPUT_BUTTON_WHEEL_UP] = BTN_GEAR_UP,
-[INPUT_BUTTON_WHEEL_DOWN]= BTN_GEAR_DOWN,
+[INPUT_BUTTON_WHEELUP] = BTN_GEAR_UP,
+[INPUT_BUTTON_WHEELDOWN] = BTN_GEAR_DOWN,
};
static const unsigned int axismap_rel[INPUT_AXIS__MAX] = {
diff --git a/include/qapi/error.h b/include/qapi/error.h
index 299cbf4..1480f59 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -113,12 +113,12 @@ typedef struct Error Error;
* enum names.
*/
typedef enum ErrorClass {
-ERROR_CLASS_GENERIC_ERROR = QAPI_ERROR_CLASS_GENERIC_ERROR,
-ERROR_CLASS_COMMAND_NOT_FOUND = QAPI_ERROR_CLASS_COMMAND_NOT_FOUND,
-ERROR_CLASS_DEVICE_ENCRYPTED = QAPI_ERROR_CLASS_DEVICE_ENCRYPTED,
-ERROR_CLASS_DEVICE_NOT_ACTIVE = QAPI_ERROR_CLASS_DEVICE_NOT_ACTIVE,
-ERROR_CLASS_DEVICE_NOT_FOUND = QAPI_ERROR_CLASS_DEVICE_NOT_FOUND,
-ERROR_CLASS_KVM_MISSING_CAP = QAPI_ERROR
[Qemu-devel] [PATCH v12 35/36] qapi: Simplify visits of optional fields
None of the visitor callbacks would set an error when testing
if an optional field was present; make this part of the interface
contract by eliminating the errp argument.
The resulting generated code has a nice diff:
|-visit_optional(v, &has_fdset_id, "fdset-id", &err);
|-if (err) {
|-goto out;
|-}
|-if (has_fdset_id) {
|+visit_optional(v, &has_fdset_id, "fdset-id");
|+if (has_fdset_id) {
| visit_type_int(v, &fdset_id, "fdset-id", &err);
| if (err) {
| goto out;
| }
| }
Signed-off-by: Eric Blake
---
v12: split error removal from 'if' compression
v11 (no v10): no change
v9: no change
v8: no change
v7: rebase to no member.c_name()
v6: rebase onto earlier testsuite and gen_err_check() improvements
---
include/qapi/visitor-impl.h | 5 ++---
include/qapi/visitor.h | 10 --
qapi/opts-visitor.c | 2 +-
qapi/qapi-visit-core.c | 5 ++---
qapi/qmp-input-visitor.c| 3 +--
qapi/string-input-visitor.c | 3 +--
scripts/qapi.py | 8 ++--
7 files changed, 17 insertions(+), 19 deletions(-)
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 7419684..44a21b7 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -44,9 +44,8 @@ struct Visitor
void (*type_any)(Visitor *v, QObject **obj, const char *name,
Error **errp);
-/* May be NULL */
-void (*optional)(Visitor *v, bool *present, const char *name,
- Error **errp);
+/* May be NULL; most useful for input visitors. */
+void (*optional)(Visitor *v, bool *present, const char *name);
void (*type_uint8)(Visitor *v, uint8_t *obj, const char *name, Error
**errp);
void (*type_uint16)(Visitor *v, uint16_t *obj, const char *name, Error
**errp);
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 1414de1..9be60d4 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -36,8 +36,14 @@ void visit_end_implicit_struct(Visitor *v, Error **errp);
void visit_start_list(Visitor *v, const char *name, Error **errp);
GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp);
void visit_end_list(Visitor *v, Error **errp);
-void visit_optional(Visitor *v, bool *present, const char *name,
-Error **errp);
+
+/**
+ * Check if an optional member @name of an object needs visiting.
+ * For input visitors, set *@present according to whether the
+ * corresponding visit_type_*() needs calling; for other visitors,
+ * leave *@present unchanged.
+ */
+void visit_optional(Visitor *v, bool *present, const char *name);
/**
* Determine the qtype of the item @name in the current object visit.
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index cd10392..ef5fb8b 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -488,7 +488,7 @@ opts_type_size(Visitor *v, uint64_t *obj, const char *name,
Error **errp)
static void
-opts_optional(Visitor *v, bool *present, const char *name, Error **errp)
+opts_optional(Visitor *v, bool *present, const char *name)
{
OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index cee76bc..e07d6f9 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -73,11 +73,10 @@ void visit_end_union(Visitor *v, bool data_present, Error
**errp)
}
}
-void visit_optional(Visitor *v, bool *present, const char *name,
-Error **errp)
+void visit_optional(Visitor *v, bool *present, const char *name)
{
if (v->optional) {
-v->optional(v, present, name, errp);
+v->optional(v, present, name);
}
}
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 26b7414..932b5f3 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -303,8 +303,7 @@ static void qmp_input_type_any(Visitor *v, QObject **obj,
const char *name,
*obj = qobj;
}
-static void qmp_input_optional(Visitor *v, bool *present, const char *name,
- Error **errp)
+static void qmp_input_optional(Visitor *v, bool *present, const char *name)
{
QmpInputVisitor *qiv = to_qiv(v);
QObject *qobj = qmp_input_get_object(qiv, name, true);
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index bbd6a54..dee780a 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -299,8 +299,7 @@ static void parse_type_number(Visitor *v, double *obj,
const char *name,
*obj = val;
}
-static void parse_optional(Visitor *v, bool *present, const char *name,
- Error **errp)
+static void parse_optional(Visitor *v, bool *present, const char *name)
{
StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 362afa1..78b6400 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1680,15 +1
[Qemu-devel] [PATCH v12 28/36] qapi: Simplify QObject
The QObject hierarchy is small enough, and unlikely to grow further
(since we only use it to map to JSON and already cover all JSON
types), that we can simplify things by not tracking a separate
vtable, but just inline all two elements of the vtable QType
directly into QObject. We can drop qnull_destroy_obj() in the
process.
This also has the nice benefit of moving the typename 'QType'
out of the way, so that the next patch can repurpose it for a
nicer name for 'qtype_code'.
The various objects are now 8 bytes larger on 64-bit platforms, but
hopefully this is in the noise. If size is absolutely critical due
to cache line speed effects, we could do some bit packing to
restore the previous size (qtype_code currently only requires three
distinct bits, so we could assert an assumption that architecture
ABIs will give function pointers at least an 8-byte alignment, and
that the three low-order bits of destroy are always zero; or we
could use a bitfield for refcnt, with a maximum of SIZE_MAX/8, and
given that a QObject already occupies at least 8 bytes, that won't
artificially limit us). Or we could expose the currently static
destroy methods and instead create a table in qobject.h that maps
QType to destructor. But let's save any size compression for a
followup patch, if at all.
On the other hand, by avoiding a separate vtable, there is less
indirection, so use of QObjects may be slightly more efficient.
However, as we don't have evidence pointing at either QObject size
or access speed as being a hot spot, I didn't think it was worth
benchmarking.
Suggested-by: Markus Armbruster
Signed-off-by: Eric Blake
---
v12: new patch, split off from 21/28
---
include/qapi/qmp/qobject.h | 38 +-
qobject/qbool.c| 7 +--
qobject/qdict.c| 7 +--
qobject/qfloat.c | 7 +--
qobject/qint.c | 7 +--
qobject/qlist.c| 7 +--
qobject/qnull.c| 13 ++---
qobject/qstring.c | 7 +--
8 files changed, 29 insertions(+), 64 deletions(-)
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index 4b96ed5..83ed70b 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -47,21 +47,21 @@ typedef enum {
QTYPE_MAX,
} qtype_code;
-struct QObject;
+typedef struct QObject QObject;
-typedef struct QType {
-qtype_code code;
-void (*destroy)(struct QObject *);
-} QType;
-
-typedef struct QObject {
-const QType *type;
+struct QObject {
+qtype_code type;
size_t refcnt;
-} QObject;
+void (*destroy)(QObject *);
+};
/* Get the 'base' part of an object */
#define QOBJECT(obj) (&(obj)->base)
+/* High-level interface for qobject_init() */
+#define QOBJECT_INIT(obj, type, destroy) \
+qobject_init(QOBJECT(obj), type, destroy)
+
/* High-level interface for qobject_incref() */
#define QINCREF(obj) \
qobject_incref(QOBJECT(obj))
@@ -71,9 +71,14 @@ typedef struct QObject {
qobject_decref(obj ? QOBJECT(obj) : NULL)
/* Initialize an object to default values */
-#define QOBJECT_INIT(obj, qtype_type) \
-obj->base.refcnt = 1; \
-obj->base.type = qtype_type
+static inline void qobject_init(QObject *obj, qtype_code type,
+void (*destroy)(struct QObject *))
+{
+assert(type);
+obj->refcnt = 1;
+obj->type = type;
+obj->destroy = destroy;
+}
/**
* qobject_incref(): Increment QObject's reference count
@@ -92,9 +97,8 @@ static inline void qobject_decref(QObject *obj)
{
assert(!obj || obj->refcnt);
if (obj && --obj->refcnt == 0) {
-assert(obj->type != NULL);
-assert(obj->type->destroy != NULL);
-obj->type->destroy(obj);
+assert(obj->destroy);
+obj->destroy(obj);
}
}
@@ -103,8 +107,8 @@ static inline void qobject_decref(QObject *obj)
*/
static inline qtype_code qobject_type(const QObject *obj)
{
-assert(obj->type != NULL);
-return obj->type->code;
+assert(obj->type);
+return obj->type;
}
extern QObject qnull_;
diff --git a/qobject/qbool.c b/qobject/qbool.c
index bc6535f..9f46e67 100644
--- a/qobject/qbool.c
+++ b/qobject/qbool.c
@@ -17,11 +17,6 @@
static void qbool_destroy_obj(QObject *obj);
-static const QType qbool_type = {
-.code = QTYPE_QBOOL,
-.destroy = qbool_destroy_obj,
-};
-
/**
* qbool_from_bool(): Create a new QBool from a bool
*
@@ -33,7 +28,7 @@ QBool *qbool_from_bool(bool value)
qb = g_malloc(sizeof(*qb));
qb->value = value;
-QOBJECT_INIT(qb, &qbool_type);
+QOBJECT_INIT(qb, QTYPE_QBOOL, qbool_destroy_obj);
return qb;
}
diff --git a/qobject/qdict.c b/qobject/qdict.c
index 2d67bf1..cf62269 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -21,11 +21,6 @@
static void qdict_destroy_obj(QObject *obj);
-static const QType qdict_type = {
-.code = QTYPE_QDICT,
-.destroy = qdict_destroy_obj,
-};
-
/**
* qd
[Qemu-devel] [PATCH v12 22/36] qapi: Don't let implicit enum MAX member collide
Now that we guarantee the user doesn't have any enum values
beginning with a single underscore, we can use that for our
own purposes. Renaming ENUM_MAX to ENUM__MAX makes it obvious
that the sentinel is generated.
This patch was mostly generated by applying a temporary patch:
|diff --git a/scripts/qapi.py b/scripts/qapi.py
|index e6d014b..b862ec9 100644
|--- a/scripts/qapi.py
|+++ b/scripts/qapi.py
|@@ -1570,6 +1570,7 @@ const char *const %(c_name)s_lookup[] = {
| max_index = c_enum_const(name, 'MAX', prefix)
| ret += mcgen('''
| [%(max_index)s] = NULL,
|+// %(max_index)s
| };
| ''',
|max_index=max_index)
then running:
$ cat qapi-{types,event}.c tests/test-qapi-types.c |
sed -n 's,^// \(.*\)MAX,s|\1MAX|\1_MAX|g,p' > list
$ git grep -l _MAX | xargs sed -i -f list
The only things not generated are the changes in scripts/qapi.py.
Signed-off-by: Eric Blake
---
v12: new patch
---
block/blkdebug.c | 10 +-
block/parallels.c | 4 ++--
block/qcow2.c | 2 +-
block/quorum.c | 2 +-
block/raw-posix.c | 2 +-
blockdev.c | 2 +-
docs/qapi-code-gen.txt | 4 ++--
hmp.c | 14 +++---
hw/char/escc.c | 2 +-
hw/i386/pc_piix.c | 2 +-
hw/i386/pc_q35.c | 2 +-
hw/input/hid.c | 2 +-
hw/input/ps2.c | 2 +-
hw/input/virtio-input-hid.c| 8
include/migration/migration.h | 4 ++--
migration/migration.c | 4 ++--
monitor.c | 14 +++---
net/net.c | 4 ++--
qemu-nbd.c | 2 +-
replay/replay-input.c | 8
scripts/qapi.py| 6 +++---
tests/test-qmp-output-visitor.c| 6 +++---
tests/test-string-output-visitor.c | 4 ++--
tpm.c | 10 +-
ui/cocoa.m | 2 +-
ui/console.c | 2 +-
ui/input-keymap.c | 4 ++--
ui/input-legacy.c | 6 +++---
ui/input.c | 6 +++---
ui/sdl.c | 2 +-
ui/sdl2.c | 2 +-
ui/spice-input.c | 2 +-
ui/vnc.c | 2 +-
vl.c | 18 +-
34 files changed, 83 insertions(+), 83 deletions(-)
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 76805a6..7cc30d5 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -35,7 +35,7 @@ typedef struct BDRVBlkdebugState {
int state;
int new_state;
-QLIST_HEAD(, BlkdebugRule) rules[BLKDBG_MAX];
+QLIST_HEAD(, BlkdebugRule) rules[BLKDBG__MAX];
QSIMPLEQ_HEAD(, BlkdebugRule) active_rules;
QLIST_HEAD(, BlkdebugSuspendedReq) suspended_reqs;
} BDRVBlkdebugState;
@@ -146,7 +146,7 @@ static int get_event_by_name(const char *name,
BlkdebugEvent *event)
{
int i;
-for (i = 0; i < BLKDBG_MAX; i++) {
+for (i = 0; i < BLKDBG__MAX; i++) {
if (!strcmp(BlkdebugEvent_lookup[i], name)) {
*event = i;
return 0;
@@ -506,7 +506,7 @@ static void blkdebug_close(BlockDriverState *bs)
BlkdebugRule *rule, *next;
int i;
-for (i = 0; i < BLKDBG_MAX; i++) {
+for (i = 0; i < BLKDBG__MAX; i++) {
QLIST_FOREACH_SAFE(rule, &s->rules[i], next, next) {
remove_rule(rule);
}
@@ -571,7 +571,7 @@ static void blkdebug_debug_event(BlockDriverState *bs,
BlkdebugEvent event)
struct BlkdebugRule *rule, *next;
bool injected;
-assert((int)event >= 0 && event < BLKDBG_MAX);
+assert((int)event >= 0 && event < BLKDBG__MAX);
injected = false;
s->new_state = s->state;
@@ -628,7 +628,7 @@ static int
blkdebug_debug_remove_breakpoint(BlockDriverState *bs,
BlkdebugRule *rule, *next;
int i, ret = -ENOENT;
-for (i = 0; i < BLKDBG_MAX; i++) {
+for (i = 0; i < BLKDBG__MAX; i++) {
QLIST_FOREACH_SAFE(rule, &s->rules[i], next, next) {
if (rule->action == ACTION_SUSPEND &&
!strcmp(rule->options.suspend.tag, tag)) {
diff --git a/block/parallels.c b/block/parallels.c
index 4f79293..eb93321 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -61,7 +61,7 @@ typedef struct ParallelsHeader {
typedef enum ParallelsPreallocMode {
PRL_PREALLOC_MODE_FALLOCATE = 0,
PRL_PREALLOC_MODE_TRUNCATE = 1,
-PRL_PREALLOC_MODE_MAX = 2,
+PRL_PREALLOC_MODE__MAX = 2,
} ParallelsPreallocMode;
static const char *prealloc_mode_lookup[] = {
@@ -660,7 +660,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
s->prealloc_size = MAX(s->tracks, s->prealloc_size >> BDRV_SECTOR_BITS);
buf = qemu_opt_get_del(opts, PARALLELS_OPT_PREALLOC_MODE);
[Qemu-devel] [PATCH] exec: silence hugetlbfs warning under qtest
From: Marc-André Lureau
vhost-user-test prints a warning. A test should not need to run on
hugetlbfs, let's silence the warning under qtest. The
condition can't check on qtest_enabled() since vhost-user-test actually
doesn't use qtest accel. However, qtest_driver() can be used, if
qtest_init() is called early enough. For that reason, move chardev and
qtest initialization early.
Signed-off-by: Marc-André Lureau
---
exec.c | 5 -
vl.c | 28 ++--
2 files changed, 18 insertions(+), 15 deletions(-)
diff --git a/exec.c b/exec.c
index b09f18b..acbd4a2 100644
--- a/exec.c
+++ b/exec.c
@@ -51,6 +51,7 @@
#include "qemu/main-loop.h"
#include "translate-all.h"
#include "sysemu/replay.h"
+#include "sysemu/qtest.h"
#include "exec/memory-internal.h"
#include "exec/ram_addr.h"
@@ -1196,8 +1197,10 @@ static long gethugepagesize(const char *path, Error
**errp)
return 0;
}
-if (fs.f_type != HUGETLBFS_MAGIC)
+if (!qtest_driver() &&
+fs.f_type != HUGETLBFS_MAGIC) {
fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path);
+}
return fs.f_bsize;
}
diff --git a/vl.c b/vl.c
index 7d993a5..f9c661a 100644
--- a/vl.c
+++ b/vl.c
@@ -4288,14 +4288,23 @@ int main(int argc, char **argv, char **envp)
page_size_init();
socket_init();
-if (qemu_opts_foreach(qemu_find_opts("object"),
- object_create,
- object_create_initial, NULL)) {
+if (qemu_opts_foreach(qemu_find_opts("chardev"),
+ chardev_init_func, NULL, NULL)) {
exit(1);
}
-if (qemu_opts_foreach(qemu_find_opts("chardev"),
- chardev_init_func, NULL, NULL)) {
+if (qtest_chrdev) {
+Error *local_err = NULL;
+qtest_init(qtest_chrdev, qtest_log, &local_err);
+if (local_err) {
+error_report_err(local_err);
+exit(1);
+}
+}
+
+if (qemu_opts_foreach(qemu_find_opts("object"),
+ object_create,
+ object_create_initial, NULL)) {
exit(1);
}
@@ -4325,15 +4334,6 @@ int main(int argc, char **argv, char **envp)
configure_accelerator(current_machine);
-if (qtest_chrdev) {
-Error *local_err = NULL;
-qtest_init(qtest_chrdev, qtest_log, &local_err);
-if (local_err) {
-error_report_err(local_err);
-exit(1);
-}
-}
-
machine_opts = qemu_get_machine_opts();
kernel_filename = qemu_opt_get(machine_opts, "kernel");
initrd_filename = qemu_opt_get(machine_opts, "initrd");
--
2.5.0
[Qemu-devel] [PATCH v12 27/36] qapi: Forbid case-insensitive clashes
In general, designing user interfaces that rely on case distinction is poor practice. Another benefit of enforcing a restriction against case-insensitive clashes is that we no longer have to worry about the situation of enum values that could be distinguished by case if mapped by c_name(), but which cannot be distinguished when mapped to C as ALL_CAPS by c_enum_const() [via c_name(name, False).upper()]. Thus, having the generator look for case collisions up front will prevent developers from worrying whether different munging rules for member names compared to enum values as a discriminator will cause any problems in qapi unions. We do not have to care about c_name(name, True) vs. c_name(name, False), as long as any pair of munged names being compared were munged using the same flag value to c_name(). This is because commit 9fb081e already reserved names munging to 'q_', and this patch extends the reservation to 'Q_'; and because recent patches fixed things to ensure the only thing done by the flag is adding the prefix 'q_', that the only use of '.' (which also munges to '_') is in downstream extension prefixes, and that enum munging does not add underscores to any CamelCase values. However, we DO have to care about the fact that we have a command 'stop' and an event 'STOP' (currently the only case collision of any of our .json entities). To solve that, use some tricks in the lookup map for entities. With some careful choice of keys, we can bisect the map into two collision pools (name.upper() for events, name.lower() for all else). Since we already document that no two entities can have the exact same spelling, an entity can only occur under one of the two partitions of the map. We could go further and enforce that events are named with 'ALL_CAPS' and that nothing else is named in that manner; but that can be done as a followup if desired. We could also separate commands from type names, but then we no longer have a convenient upper vs. lower partitioning allowing us to share a single dictionary. In order to keep things stable, the visit() method has to ensure that it visits entities sorted by their real name, and not by the new munged keys of the dictionary; Python's attrgetter is a lifesaver for this task. There is also the possibility that we may want to add a future extension to QMP of teaching it to be case-insensitive (the user could request command 'Quit' instead of 'quit', or could spell a struct field as 'CPU' instead of 'cpu'). But for that to be a practical extension, we cannot break backwards compatibility with any existing struct that was already relying on case sensitivity. Fortunately, after a recent patch cleaned up CpuInfo, there are no such existing qapi structs. Of course, the idea of a future extension is not as strong of a reason to make the change. At any rate, it is easier to be strict now, and relax things later if we find a reason to need case-sensitive QMP members, than it would be to wish we had the restriction in place. Add new tests args-case-clash.json and command-type-case-clash.json to demonstrate that the collision detection works. Signed-off-by: Eric Blake --- v12: add in entity case collisions (required sharing two maps), improve commit message v11: rebase to latest, don't focus so hard on future case-insensitive extensions, adjust commit message v10: new patch --- docs/qapi-code-gen.txt | 7 + scripts/qapi.py| 41 +- tests/Makefile | 2 ++ tests/qapi-schema/args-case-clash.err | 1 + tests/qapi-schema/args-case-clash.exit | 1 + tests/qapi-schema/args-case-clash.json | 4 +++ tests/qapi-schema/args-case-clash.out | 0 tests/qapi-schema/command-type-case-clash.err | 1 + tests/qapi-schema/command-type-case-clash.exit | 1 + tests/qapi-schema/command-type-case-clash.json | 3 ++ tests/qapi-schema/command-type-case-clash.out | 0 11 files changed, 53 insertions(+), 8 deletions(-) create mode 100644 tests/qapi-schema/args-case-clash.err create mode 100644 tests/qapi-schema/args-case-clash.exit create mode 100644 tests/qapi-schema/args-case-clash.json create mode 100644 tests/qapi-schema/args-case-clash.out create mode 100644 tests/qapi-schema/command-type-case-clash.err create mode 100644 tests/qapi-schema/command-type-case-clash.exit create mode 100644 tests/qapi-schema/command-type-case-clash.json create mode 100644 tests/qapi-schema/command-type-case-clash.out diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt index b01a691..1f6cb16 100644 --- a/docs/qapi-code-gen.txt +++ b/docs/qapi-code-gen.txt @@ -102,6 +102,13 @@ single-dimension array of that type; multi-dimension arrays are not directly supported (although an array of a complex struct that contains an array member is possible). +Client JSON Protocol is case-sensitive. However, the generator +rejects attempts to cr
[Qemu-devel] [PATCH v12 34/36] qapi: Add positive tests to qapi-schema-test
Add positive tests to qapi-schema-test for things that were
made possible by recent patches but which caused compile errors
due to collisions prior to that point. The focus is mainly on
collisions due to names we have reserved for qapi, even though
it is unlikely that anyone will want to abuse these names in
actual .json files.
The added tests includes:
Use of a member name ending in 'Kind' or 'List' [1, 3]
Use of a type name starting with 'has_' [1, 4]
Use of a type named 'u' [1, 5]
Use of a union branch name of 'u' [2, 5]
Use of a union branch name starting with 'has_' [2, 4]
[1] Never broken, but could break if reservations are too strict
[2] Broken prior to commit e4ba22b
[3] See reservations in commit 4dc2e69 and 255960d
[4] See reservations in commit 9fb081e
[5] See reservation in commit 5e59baf
Not worth testing here: we no longer have a collision with a
member named 'base' (commit ddf2190), with a branch named
'type' (commit e4ba22b), or with an alternate member named
'max' (previous commits); these names were more accidental
namespace pollutions than intentional reservations.
Signed-off-by: Eric Blake
---
v12: no change
v11 (no v10): drop test of 'max'
v9: reorder in series (was 9/17); fewer tests of 'base' and
'type' non-collision; fold in alternate 'max' test; update commit
message
v8: new, but collects portions of subset B v10 patches 2, 3, and
16 and subset C v7 patch 6 that were deferred to later.
It might be worth dropping or simplifying this patch, depending
on how many corner cases we actually want to test.
---
tests/qapi-schema/qapi-schema-test.json | 16
tests/qapi-schema/qapi-schema-test.out | 26 ++
2 files changed, 42 insertions(+)
diff --git a/tests/qapi-schema/qapi-schema-test.json
b/tests/qapi-schema/qapi-schema-test.json
index 4b89527..793323d 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -113,6 +113,22 @@
'sizes': ['size'],
'any': ['any'] } }
+# Even though 'u' and 'has_*' are forbidden as struct member names, they
+# should still be valid as a type or union branch name. And although
+# '*Kind' and '*List' are forbidden as type names, they should not be
+# forbidden as a member or branch name. Flat union branches do not
+# collide with base members.
+{ 'enum': 'EnumName', 'data': [ 'value1', 'has_a', 'u' ] }
+{ 'struct': 'has_a', 'data': { 'MyKind': 'int', 'MyList': ['int'],
+ 'value1': 'EnumName' } }
+{ 'union': 'u', 'data': { 'u': 'uint8', 'myKind': 'has_a',
+ 'myList': 'has_a', 'has_a': 'has_a' } }
+{ 'union': 'UnionName', 'base': 'has_a', 'discriminator': 'value1',
+ 'data': { 'value1': 'UserDefZero', 'has_a': 'UserDefZero',
+'u': 'UserDefZero' } }
+{ 'alternate': 'AltName', 'data': { 'type': 'int', 'u': 'bool',
+'myKind': 'has_a' } }
+
# testing commands
{ 'command': 'user_def_cmd', 'data': {} }
{ 'command': 'user_def_cmd1', 'data': {'ud1a': 'UserDefOne'} }
diff --git a/tests/qapi-schema/qapi-schema-test.out
b/tests/qapi-schema/qapi-schema-test.out
index 2c546b7..03c465b 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -22,6 +22,8 @@ object :obj-guest-get-time-arg
member b: int optional=True
object :obj-guest-sync-arg
member arg: any optional=False
+object :obj-has_a-wrapper
+member data: has_a optional=False
object :obj-int16List-wrapper
member data: int16List optional=False
object :obj-int32List-wrapper
@@ -46,6 +48,8 @@ object :obj-uint32List-wrapper
member data: uint32List optional=False
object :obj-uint64List-wrapper
member data: uint64List optional=False
+object :obj-uint8-wrapper
+member data: uint8 optional=False
object :obj-uint8List-wrapper
member data: uint8List optional=False
object :obj-user_def_cmd1-arg
@@ -56,6 +60,10 @@ object :obj-user_def_cmd2-arg
alternate AltIntNum
case i: int
case n: number
+alternate AltName
+case type: int
+case u: bool
+case myKind: has_a
alternate AltNumInt
case n: number
case i: int
@@ -78,6 +86,7 @@ event EVENT_D :obj-EVENT_D-arg
object Empty1
object Empty2
base Empty1
+enum EnumName ['value1', 'has_a', 'u']
enum EnumOne ['value1', 'value2', 'value3']
object EventStructOne
member struct1: UserDefOne optional=False
@@ -101,6 +110,12 @@ object TestStruct
member integer: int optional=False
member boolean: bool optional=False
member string: str optional=False
+object UnionName
+base has_a
+tag value1
+case value1: UserDefZero
+case has_a: UserDefZero
+case u: UserDefZero
object UserDefA
member boolean: bool optional=False
member a_b: int optional=True
@@ -196,6 +211,17 @@ command guest-get-time :obj-guest-get-time-arg -> int
gen=True success_response=True
command guest-sync :obj-guest-sync-arg -> any
[Qemu-devel] [PATCH v12 29/36] qobject: Rename qtype_code to QType
The name QType is more in line with our conventions for qapi
types, and matches the fact that each enum member has a prefix
of QTYPE_.
Signed-off-by: Eric Blake
---
v12: new patch split off of 21/28
---
block/qapi.c | 4 ++--
include/hw/qdev-core.h | 2 +-
include/qapi/qmp/qobject.h | 8
qobject/qdict.c| 3 +--
scripts/qapi.py| 2 +-
5 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index d20262d..4eb48fc 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -590,7 +590,7 @@ static void dump_qlist(fprintf_function func_fprintf, void
*f, int indentation,
int i = 0;
for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++) {
-qtype_code type = qobject_type(entry->value);
+QType type = qobject_type(entry->value);
bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
const char *format = composite ? "%*s[%i]:\n" : "%*s[%i]: ";
@@ -608,7 +608,7 @@ static void dump_qdict(fprintf_function func_fprintf, void
*f, int indentation,
const QDictEntry *entry;
for (entry = qdict_first(dict); entry; entry = qdict_next(dict, entry)) {
-qtype_code type = qobject_type(entry->value);
+QType type = qobject_type(entry->value);
bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
const char *format = composite ? "%*s%s:\n" : "%*s%s: ";
char key[strlen(entry->key) + 1];
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index e6dbde4..c66429a 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -239,7 +239,7 @@ struct Property {
PropertyInfo *info;
int offset;
uint8_t bitnr;
-qtype_code qtype;
+QTypeqtype;
int64_t defval;
int arrayoffset;
PropertyInfo *arrayinfo;
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index 83ed70b..0558ff3 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -45,12 +45,12 @@ typedef enum {
QTYPE_QFLOAT,
QTYPE_QBOOL,
QTYPE_MAX,
-} qtype_code;
+} QType;
typedef struct QObject QObject;
struct QObject {
-qtype_code type;
+QType type;
size_t refcnt;
void (*destroy)(QObject *);
};
@@ -71,7 +71,7 @@ struct QObject {
qobject_decref(obj ? QOBJECT(obj) : NULL)
/* Initialize an object to default values */
-static inline void qobject_init(QObject *obj, qtype_code type,
+static inline void qobject_init(QObject *obj, QType type,
void (*destroy)(struct QObject *))
{
assert(type);
@@ -105,7 +105,7 @@ static inline void qobject_decref(QObject *obj)
/**
* qobject_type(): Return the QObject's type
*/
-static inline qtype_code qobject_type(const QObject *obj)
+static inline QType qobject_type(const QObject *obj)
{
assert(obj->type);
return obj->type;
diff --git a/qobject/qdict.c b/qobject/qdict.c
index cf62269..d5e6df1 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -179,8 +179,7 @@ size_t qdict_size(const QDict *qdict)
/**
* qdict_get_obj(): Get a QObject of a specific type
*/
-static QObject *qdict_get_obj(const QDict *qdict, const char *key,
- qtype_code type)
+static QObject *qdict_get_obj(const QDict *qdict, const char *key, QType type)
{
QObject *obj;
diff --git a/scripts/qapi.py b/scripts/qapi.py
index e41dbaf..7c0ef76 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -34,7 +34,7 @@ builtin_types = {
'uint32': 'QTYPE_QINT',
'uint64': 'QTYPE_QINT',
'size': 'QTYPE_QINT',
-'any': None, # any qtype_code possible, actually
+'any': None, # any QType possible, actually
}
# Whitelist of commands allowed to return a non-dictionary
--
2.4.3
[Qemu-devel] [PATCH v12 31/36] qapi: Simplify visiting of alternate types
Previously, working with alternates required two lookup arrays
and some indirection: for type Foo, we created Foo_qtypes[]
which maps each qtype to a value of the generated FooKind enum,
then look up that value in FooKind_lookup[] like we do for other
union types.
This has a couple of subtle bugs. First, the generator was
creating a call with a parameter '(int *) &(*obj)->type' where
type is an enum type; this is unsafe if the compiler chooses
to store the enum type in a different size than int, where
assigning through the wrong size pointer can corrupt data or
cause a SIGBUS. [We still have the casting bug for our enum
visitors, but that's a topic for a different patch.]
Second, since the values of the FooKind enum start at zero, all
entries of the Foo_qtypes[] array that were not explicitly
initialized will map to the same branch of the union as the
first member of the alternate, rather than triggering a desired
failure in visit_get_next_type(). Fortunately, the bug seldom
bites; the very next thing the input visitor does is try to
parse the incoming JSON with the wrong parser, which normally
fails; the output visitor is not used with a C struct in that
state, and the dealloc visitor has nothing to clean up (so
there is no leak).
However, the second bug IS observable in one case: parsing an
integer causes unusual behavior in an alternate that contains
at least a 'number' member but no 'int' member, because the
'number' parser accepts QTYPE_QINT in addition to the expected
QTYPE_QFLOAT (that is, since 'int' is not a member, the type
QTYPE_QINT accidentally maps to FooKind 0; if this enum value
is the 'number' branch the integer parses successfully, but if
the 'number' branch is not first, some other branch tries to
parse the integer and rejects it). A later patch will worry
about fixing alternates to always parse all inputs that a
non-alternate 'number' would accept, for now this is still
marked FIXME in the updated test-qmp-input-visitor.c, to
merely point out that new undesired behavior of 'ans' matches
the existing undesired behavior of 'asn'.
This patch fixes the default-initialization bug by deleting the
indirection, and modifying get_next_type() to directly assign a
QTypeCode parameter. This in turn fixes the type-casting bug,
as we are no longer casting a pointer to enum to a questionable
size. There is no longer a need to generate an implicit FooKind
enum associated with the alternate type (since the QMP wire
format never uses the stringized counterparts of the C union
member names). Since the updated visit_get_next_type() does not
know which qtypes are expected, the generated visitor is
modified to generate an error statement if an unexpected type is
encountered.
Callers now have to know the QTYPE_* mapping when looking at the
discriminator; but so far, only the testsuite was even using the
C struct of an alternate types. I considered the possibility of
keeping the internal enum FooKind, but initialized differently
than most generated arrays, as in:
typedef enum FooKind {
FOO_KIND_A = QTYPE_QDICT,
FOO_KIND_B = QTYPE_QINT,
} FooKind;
to create nicer aliases for knowing when to use foo->a or foo->b
when inspecting foo->type; but it turned out to add too much
complexity, especially without a client.
There is a user-visible side effect to this change, but I
consider it to be an improvement. Previously,
the invalid QMP command:
{"execute":"blockdev-add", "arguments":{"options":
{"driver":"raw", "id":"a", "file":true}}}
failed with:
{"error": {"class": "GenericError",
"desc": "Invalid parameter type for 'file', expected: QDict"}}
(visit_get_next_type() succeeded, and the error comes from the
visit_type_BlockdevOptions() expecting {}; there is no mention of
the fact that a string would also work). Now it fails with:
{"error": {"class": "GenericError",
"desc": "Invalid parameter type for 'file', expected: BlockdevRef"}}
(the error when the next type doesn't match any expected types for
the overall alternate).
Signed-off-by: Eric Blake
---
v12: rebase to earlier 'max' collision avoidance, some variable renames
v11 (no v10): rebase to new QTypeCode, with fewer special cases; tweak
commit message to match
v9: rebase to earlier changes, rework commit message to mention second
bug fix; move positive test in qapi-schema-test to later patch
v8: no change
v7: rebase onto earlier changes, rework how subtype makes things work
v6: rebase onto tag_member subclass, testsuite, gen_err_check(),
and info improvements
---
docs/qapi-code-gen.txt | 3 ---
include/qapi/visitor-impl.h| 3 ++-
include/qapi/visitor.h | 8 +++-
qapi/qapi-visit-core.c | 4 ++--
qapi/qmp-input-visitor.c | 4 ++--
scripts/qapi-types.py | 34 --
scripts/qapi-visit.py | 14 +-
scripts/qapi.py| 18 +-
Re: [Qemu-devel] [PATCH] vfio/pci-quirks: Only quirk to size of PCI config space
On 11/18/15 00:41, Alex Williamson wrote: > For quirks that support the full PCIe extended config space, limit the > quirk to only the size of config space available through vfio. This > allows host systems with broken MMCONFIG regions to still make use of > these quirks without generating bad address faults trying to access > beyond the end of config space exposed through vfio. This may expose > direct access to parts of extended config space that we'd prefer not > to expose, but that's why we have an IOMMU. > > Signed-off-by: Alex Williamson > Reported-by: Ronnie Swanink > --- > hw/vfio/pci-quirks.c |6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c > index 30c68a1..e117c41 100644 > --- a/hw/vfio/pci-quirks.c > +++ b/hw/vfio/pci-quirks.c > @@ -328,7 +328,7 @@ static void vfio_probe_ati_bar4_quirk(VFIOPCIDevice > *vdev, int nr) > window->data_offset = 4; > window->nr_matches = 1; > window->matches[0].match = 0x4000; > -window->matches[0].mask = PCIE_CONFIG_SPACE_SIZE - 1; > +window->matches[0].mask = vdev->config_size - 1; > window->bar = nr; > window->addr_mem = &quirk->mem[0]; > window->data_mem = &quirk->mem[1]; > @@ -674,7 +674,7 @@ static void vfio_probe_nvidia_bar5_quirk(VFIOPCIDevice > *vdev, int nr) > window->matches[0].match = 0x1800; > window->matches[0].mask = PCI_CONFIG_SPACE_SIZE - 1; > window->matches[1].match = 0x88000; > -window->matches[1].mask = PCIE_CONFIG_SPACE_SIZE - 1; > +window->matches[1].mask = vdev->config_size - 1; > window->bar = nr; > window->addr_mem = bar5->addr_mem = &quirk->mem[0]; > window->data_mem = bar5->data_mem = &quirk->mem[1]; > @@ -765,7 +765,7 @@ static void vfio_probe_nvidia_bar0_quirk(VFIOPCIDevice > *vdev, int nr) > memory_region_init_io(mirror->mem, OBJECT(vdev), >&vfio_nvidia_mirror_quirk, mirror, >"vfio-nvidia-bar0-88000-mirror-quirk", > - PCIE_CONFIG_SPACE_SIZE); > + vdev->config_size); > memory_region_add_subregion_overlap(&vdev->bars[nr].region.mem, > mirror->offset, mirror->mem, 1); > > > $ git log -- hw/vfio/pci-quirks.c | grep Reviewed-by Okay, I guess my review won't be deemed immediately unnecessary then. :) (1) Please add to the commit message: Link: https://www.redhat.com/archives/vfio-users/2015-November/msg00192.html With that reference: Reviewed-by: Laszlo Ersek (2) Also, one question just to make sure I understand right: the last sentence of the commit message means that the left-inclusive, right-exclusive config space offset range [vdev->config_size, PCIE_CONFIG_SPACE_SIZE) is now directly available to the guest, without QEMU noticing; but, we're not worried about that, because the guest can't abuse that freedom anyway for doing arbitrary DMA. Is that right? If so, then I propose another update to the commit message (replacing the last sentence): This may grant the guest direct access to trailing parts of extended config space that we'd prefer not to expose, but that's why we have an IOMMU (the guest can't abuse said config space access for arbitrary DMA). Perhaps the above is trivial for you (assuming it is correct at all...); it may not be trivial for others. Thanks! Laszlo
Re: [Qemu-devel] [PATCH 00/14] target-i386: Implement MPX extension
On 11/17/2015 06:43 PM, Paolo Bonzini wrote: Hi Richard, it would be nice to have these patches---or at least the XSAVE support---in 2.6. I also have a PKRU implementation for TCG, but currently I'm only implementing RDPKRU/WRPKRU because I would like to build the XSAVE support on top of your patches. Sure. I'll see about updating that branch this weekend. Regarding SMM support, there are three ways to go: 1) pester Intel some more so that they disclose the format of the SMM state save area; They have done so, and relatively well. Section 34.4.1.1 of the software developer's manual (I'm looking at 325462-055, June 2015). The issue, perhaps, is that the Intel and AMD layouts are totally different. Now, given that we've been using the AMD layout with Intel emulations maybe that means that it really doesn't matter what layout we use, so long as we're self-consistent. Is there anything besides BIOS code that runs in SMM anyway? Do we have to be compatible with anything besides SeaBIOS in this area? 2) just place BNDCFGS at a random offset that is left as reserved in AMD's manual; 3) do not save BNDCFGS at all since no one uses it anyway. *shrug* I'm not a fan of 3 simply because it means that one can't experiment with it, since turning it on means either that SMM produces weird results or kernel state gets corrupted. The holes in the computation of KVM's hflags are probably harmless, but nice to have anyway. Thanks for fixing them. Are there others that I missed? Not that I saw. r~
Re: [Qemu-devel] [PATCH v12 11/36] qapi: Check for qapi collisions involving variant members
Eric Blake writes: > Right now, our ad hoc parser ensures that we cannot have a > flat union that introduces any members that would clash with > non-variant members inherited from the union's base type (see > flat-union-clash-member.json). We want ObjectType.check() to Recommend not to abbreviate QAPISchemaObjectType.check(). > make the same check, so we can later reduce some of the ad > hoc checks. > > We already have a map 'seen' of all non-variant members. We > still need to check for collisions between each variant type's > members and the non-variant ones. > > To know the variant type's members, we need to call > variant.type.check(). This also detects when a type contains > itself in a variant, exactly like the existing base.check() > detects when a type contains itself as a base. (Except that > we currently forbid anything but a struct as the type of a > variant, so we can't actually trigger this type of loop yet.) > > Slight complication: an alternate's variant can have arbitrary > type, but only an object type's check() may be called outside > QAPISchema.check(). We could either skip the call for variants > of alternates, or skip it for non-object types. For now, do > the latter, because it's easier. > > Then we call each variant member's check_clash() with the > appropriate 'seen' map. Since members of different variants > can't clash, we have to clone a fresh seen for each variant. > Wrap this in a new helper method Variants.check_clash(). Likewise: QAPISchemaObjectTypeVariants.check_clash(). > Note that cloning 'seen' inside Variants.check_clash() resembles > the one we just removed from Variants.check() in 'qapi: Drop Here, we can say .check_clash() and .check(), and leave the class to context. > obsolete tag value collision assertions'; the difference here is > that we are now checking for clashes among the qapi members of > the variant type, rather than for a single clash with the variant > tag name itself. > > Note that, by construction, collisions can't actually happen for > simple unions: each variant's type is a wrapper with a single > member 'data', which will never collide with the only non-variant > member 'type'. > > For alternates, there's nothing for a variant object type's > members to clash with, and therefore no need to call the new > variants.check_clash(). > > No change to generated code. > > Signed-off-by: Eric Blake Only commit message polishing, happy to do that in my tree.
Re: [Qemu-devel] [PATCH] configure: preserve various environment variables in config.status
On Tue, Nov 17, 2015 at 12:06:55PM -0700, Eric Blake wrote: > On 11/17/2015 10:59 AM, Daniel P. Berrange wrote: > > Suggested in > > > > https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg03298.html > > > > The config.status script is auto-generated by configure upon > > completion. The intention is that config.status can be later > > invoked by the developer to re-detect the same environment > > that configure originally used. The current config.status > > script, however, only contains a record of the command line > > arguments to configure. Various environment variables have > > an effect on what configure will find. In particular the > > PKG_CONFIG_LIBDIR & PKG_CONFIG_PATH vars will affect what > > libraries pkg-config finds. The PATH var will affect what > > toolchain binaries and -config scripts are found. The > > LD_LIBRARY_PATH var will affect what libraries are found. > > All these key env variables should be recorded in the > > config.status script. > > > > Signed-off-by: Daniel P. Berrange > > --- > > > > Open question: are there more env vars we should preserve ? > > Yes - anything that autoconf would mark as precious. See below. > > > +++ b/configure > > @@ -5925,6 +5925,24 @@ cat> # Compiler output produced by configure, useful for debugging > > # configure, is in config.log if it exists. > > EOD > > + > > +preserve_env() { > > +envname=$1 > > + > > +if test -n "${!envname}" > > Bashism, but configure is /bin/sh. This won't work on dash :( > > I think you'll have to use eval, and we'll just have to audit that > preserve_env can never be called with suspicious text where eval would > open a security hole. Ok, shouldn't be a big deal > > +then > > + echo "$envname=\"${!envname}\"" >> config.status > > Another use of the bashism. > > > + echo "export $envname" >> config.status > > +fi > > +} > > + > > +# Preserve various env variables that influence what > > +# features/build target configure will detect > > +preserve_env PATH > > +preserve_env LD_LIBRARY_PATH > > +preserve_env PKG_CONFIG_LIBDIR > > +preserve_env PKG_CONFIG_PATH > > + > > Autoconf preserves CC, CFLAGS, LDFLAGS, LIBS, CPPFLAGS, and CPP by > default. Also, PKG_CONFIG is typically preserved. If you run libvirt's > './configure --help', you'll also notice a bunch of *_CFLAGS and *_LIBS > in the precious list starting under the label "Some influential > environment variables". I'll add in env vars for all the commands like CC, CPP, MAKE, etc that QEMU's configure uses. I've tried preserving various *FLAGS vars but the problem here is that configure will modify/augment those variables while it is running, so when we get to preserve the original flags we only have the munged version. In any case recommendation is to use --extra-cflags rather than CFLAGS, so I figure its not a big deal to skip preserving CFLAGS/LDFLAGS Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Re: [Qemu-devel] [PATCH v2 2/5] sockets: remove use of QemuOpts from socket_listen
On Tue, Nov 17, 2015 at 03:22:04PM -0700, Eric Blake wrote:
> On 11/17/2015 10:00 AM, Daniel P. Berrange wrote:
> > The socket_listen method accepts a QAPI SocketAddress object
> > which it then turns into QemuOpts before calling the
> > inet_listen_opts/unix_listen_opts helper methods. By
> > converting the latter to use QAPI SocketAddress directly,
> > the QemuOpts conversion step can be eliminated
> >
> > This also fixes the problem where ipv4=off && ipv6=off
> > would be treated the same as ipv4=on && ipv6=on
> >
> > Signed-off-by: Daniel P. Berrange
> > ---
> > util/qemu-sockets.c | 144
> > +++-
> > 1 file changed, 87 insertions(+), 57 deletions(-)
> >
>
> > +++ b/util/qemu-sockets.c
> > @@ -114,36 +114,68 @@ NetworkAddressFamily inet_netfamily(int family)
> > return NETWORK_ADDRESS_FAMILY_UNKNOWN;
> > }
> >
> > -static int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
> > +/*
> > + * Matrix we're trying to apply
> > + *
> > + * ipv4 ipv6 family
> > + * - - PF_UNSPEC
> > + * - f PF_INET
> > + * - t PF_INET6
> > + * f - PF_INET6
> > + * f f
> > + * f t PF_INET6
> > + * t - PF_INET
> > + * t f PF_INET
>
> These I understand,
>
> > + * t t PF_INET6
>
> but why is this one PF_INET6 instead of PF_UNSPEC?
If you use PF_UNSPEC, then the addresses we listen on will be automatically
deteremined by results of the DNS lookup. ie if DNS only returns an IPv4
address, it won't listen on IPv6. When the user has said ipv6=on, then
they expect to get an error if it was not possible to listen on IPv6. So
we must use PF_INET6 here to ensure that error, otherwise ipv6=on & ipv4=on
would be no different from ipv6=- & ipv4=-.
>
> > + */
> > +static int inet_ai_family_from_address(InetSocketAddress *addr,
> > + Error **errp)
> > +{
> > +if (addr->has_ipv6 && addr->has_ipv4 &&
> > +!addr->ipv6 && !addr->ipv4) {
> > +error_setg(errp, "Cannot disable IPv4 and IPv6 at same time");
> > +return PF_UNSPEC;
> > +}
> > +if ((addr->has_ipv6 && addr->ipv6) || (addr->has_ipv4 && !addr->ipv4))
> > {
> > +return PF_INET6;
> > +}
> > +if ((addr->has_ipv4 && addr->ipv4) || (addr->has_ipv6 && !addr->ipv6))
> > {
> > +return PF_INET;
> > +}
> > +return PF_UNSPEC;
>
> This logic matches the matrix as listed, even if I'm not positive that
> the matrix is correct. If we want PF_UNSPEC when both v4 and v6 are
> explicitly requested (as in, pick whichever works), then I think it
> should be something like:
>
> if (addr->has_ipv6 && addr->has_ipv4 &&
> addr->ipv6 == addr->ipv4) {
> if (!addr->ipv6) {
> error_setg(errp, "cannot disable IPv4 and IPv6 at the hsame time");
> }
> return PF_UNSPEC;
> }
> if ((addr->has_ipv6 && addr->ipv6) || (addr->has_ipv4 && !addr->ipv4)) {
> return PF_INET6;
> }
> assert((addr->has_ipv4 && addr->ipv4) || (addr->has_ipv6 && !addr->ipv6));
> return PF_INET;
>
> > @@ -219,13 +251,15 @@ listen:
> > freeaddrinfo(res);
> > return -1;
> > }
> > -qemu_opt_set(opts, "host", uaddr, &error_abort);
> > -qemu_opt_set_number(opts, "port", inet_getport(e) - port_offset,
> > -&error_abort);
> > -qemu_opt_set_bool(opts, "ipv6", e->ai_family == PF_INET6,
> > - &error_abort);
> > -qemu_opt_set_bool(opts, "ipv4", e->ai_family != PF_INET6,
> > - &error_abort);
> > +if (update_addr) {
> > +g_free(saddr->host);
> > +saddr->host = g_strdup(uaddr);
> > +g_free(saddr->port);
> > +saddr->port = g_strdup_printf("%d",
> > + inet_getport(e) - port_offset);
> > +saddr->has_ipv6 = saddr->ipv6 = e->ai_family == PF_INET6;
> > +saddr->has_ipv4 = saddr->ipv4 = e->ai_family != PF_INET6;
>
> Should we handle PF_UNSPEC specifically, maybe by having the has_ipv6
> assignment based on e->ai_family != PF_INET?
The returne e->ai_family from getaddrinfo will never have a value
of PF_UNSPEC - that's an input only value. So we're OK to assume
we'll have PF_INET6 and PF_INET only. Well at least until someone
invents IPv7 but I'll let my great grandchildren deal with that
problem ;-)
Regards,
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Re: [Qemu-devel] [PATCH v12 13/36] qapi: Hoist tag collision check to Variants.check()
Eric Blake writes: > *** WARNING: THE ATTACHED DOCUMENT(S) CONTAIN MACROS *** > *** MACROS MAY CONTAIN MALICIOUS CODE *** > *** Open only if you can verify and trust the sender *** > *** Please contact info...@redhat.com if you have questions or concerns ** Looks like infosec crapped over your commit message. > Checking that a given QAPISchemaObjectTypeVariant.name is a > member of the corresponding QAPISchemaEnumType of the owning > QAPISchemaObjectTypeVariants.tag_member ensures that there are > no collisions in the generated C union for those tag values > (since the enum itself should have no collisions). > > However, ever since its introduction in f51d8c3d, this was the > only additional action of of Variant.check(), beyond calling > the superclass Member.check(). This forces a difference in > .check() signatures, just to pass the enum type down. > > Simplify things by instead doing the tag name check as part of > Variants.check(), at which point we can rely on inheritance > instead of overriding Variant.check(). > > Signed-off-by: Eric Blake Can bury the infosec turd on merge.
Re: [Qemu-devel] [PATCH v2 3/5] sockets: remove use of QemuOpts from socket_connect
On Tue, Nov 17, 2015 at 03:40:58PM -0700, Eric Blake wrote:
> On 11/17/2015 10:00 AM, Daniel P. Berrange wrote:
> > The socket_connect method accepts a QAPI SocketAddress object
> > which it then turns into QemuOpts before calling the
> > inet_connect_opts/unix_connect_opts helper methods. By
> > converting the latter to use QAPI SocketAddress directly,
> > the QemuOpts conversion step can be eliminated
> >
> > This also fixes the problem where ipv4=off && ipv6=off
> > would be treated the same as ipv4=on && ipv6=on
> >
> > Signed-off-by: Daniel P. Berrange
> > ---
> > util/qemu-sockets.c | 91
> > +
> > 1 file changed, 36 insertions(+), 55 deletions(-)
> >
>
> > ai.ai_flags = AI_CANONNAME | AI_V4MAPPED | AI_ADDRCONFIG;
> > -ai.ai_family = PF_UNSPEC;
> > +ai.ai_family = inet_ai_family_from_address(saddr, &err);
> > ai.ai_socktype = SOCK_STREAM;
> >
>
> >
> > -if (qemu_opt_get_bool(opts, "ipv4", 0)) {
> > -ai.ai_family = PF_INET;
> > -}
> > -if (qemu_opt_get_bool(opts, "ipv6", 0)) {
> > -ai.ai_family = PF_INET6;
>
> I'm using the notation you used in 2/5, where - is unspecified, f is
> explicitly false, and t is explicitly true. qemu_opt_get_bool(, 0)
> cannot tell the difference between - and f.
>
> The old code treated 4=- and 6=- as PF_UNSPEC; 4=f and 6=f as PF_UNSPEC,
> and 4=t and 6=t as PF_INET6. That doesn't quite jive with the commit
> message claiming that 4=off (is that '4=-' or '4=f'?) and 6=off was the
> same as 4=on (4=t) and 6=on.
Yeah that commit message is wrong - i'll remove that bit
Regards,
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Re: [Qemu-devel] [PATCH 00/14] target-i386: Implement MPX extension
On 18/11/2015 10:43, Richard Henderson wrote: >> 1) pester Intel some more so that they disclose the format of the SMM >> state save area; > > They have done so, and relatively well. Section 34.4.1.1 of the > software developer's manual (I'm looking at 325462-055, June 2015). Relatively well unfortunately is not enough. Unlike AMD, they do not document where the descriptor cache is, which we need to implement SMM save and restore. > The issue, perhaps, is that the Intel and AMD layouts are totally > different. Now, given that we've been using the AMD layout with Intel > emulations maybe that means that it really doesn't matter what layout we > use, so long as we're self-consistent. > > Is there anything besides BIOS code that runs in SMM anyway? Do we have > to be compatible with anything besides SeaBIOS in this area? There's OVMF, whose maintainers would really like the SMM state save area to be a superset of the documented format. They have grudgingly accepted that we used AMD's format, which is completely different. But if we used Intel's format and did not put the descriptor cache at the right place, then the next field Intel adds may overlap our descriptor cache fields; we would be back with the same problem. I would just place BNDCFGS at a random offset that is left as reserved in AMD's manual. Since we are at it, we might also find a home for EFER in the 32-bit case, because it is used when LM is 0 but NX is 1. Paolo
Re: [Qemu-devel] [PATCH v12 17/36] qapi: Fix c_name() munging
Eric Blake writes:
> *** WARNING: THE ATTACHED DOCUMENT(S) CONTAIN MACROS ***
> *** MACROS MAY CONTAIN MALICIOUS CODE ***
> *** Open only if you can verify and trust the sender ***
> *** Please contact info...@redhat.com if you have questions or concerns **
Another one.
> The method c_name() is supposed to do two different actions: munge
> '-' into '_', and add a 'q_' prefix to ticklish names. But it did
> these steps out of order, making it possible to submit input that
> is not ticklish until after munging, where the output then lacked
> the desired prefix.
>
> The failure is exposed easily if you have a compiler that recognizes
> C11 keywords, and try to name a member '_Thread-local', as it would
> result in trying to compile the declaration 'uint64_t _Thread_local;'
> which is not valid. However, this name violates our conventions
> (ultimately, want to enforce that no qapi names start with single
> underscore), so the test is slightly weaker by instead testing
> 'wchar-t'; the declaration 'uint64_t wchar_t;' is valid in C (where
> wchar_t is only a typedef) but fails with a C++ compiler (where it
> is a keyword).
Do we support compiling it with a C++ compiler? To sidestep the
question, I'd say "but would fail".
In my private opinion, the one sane way to compile C code with a C++
compiler is wrapping it in extern "C" { ... }.
> Fix things by reversing the order of actions within c_name().
>
> Signed-off-by: Eric Blake
Again, just commit message polish, can do on merge.
Re: [Qemu-devel] [PATCH v12 13/36] qapi: Hoist tag collision check to Variants.check()
On Wed, Nov 18, 2015 at 11:08:58AM +0100, Markus Armbruster wrote: > Eric Blake writes: > > > *** WARNING: THE ATTACHED DOCUMENT(S) CONTAIN MACROS *** > > *** MACROS MAY CONTAIN MALICIOUS CODE *** > > *** Open only if you can verify and trust the sender *** > > *** Please contact info...@redhat.com if you have questions or concerns ** > > Looks like infosec crapped over your commit message. I don't see that text in my copy of Eric's mail, nor in the archives https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg04028.html Wierd that it would get added to the mail you receive, but not to the mail I receive, given that we have the same incoming mail server. I guess the difference was you got CC'd directly but I got it via the mailing list, so something about that delivery path caused it not to trigger for me. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Re: [Qemu-devel] [PATCH v12 19/36] blkdebug: Merge hand-rolled and qapi BlkdebugEvent enum
Eric Blake writes:
> No need to keep two separate enums, where editing one is likely
> to forget the other. Now that we can specify a qapi enum prefix,
> we don't even have to change the bulk of the uses.
>
> get_event_by_name() could perhaps be replaced by qapi_enum_parse(),
> but I left that for another day.
>
> CC: Kevin Wolf
> Signed-off-by: Eric Blake
>
> ---
> v12: new patch
> ---
> block.c | 2 +-
> block/blkdebug.c | 79
> +++
> docs/blkdebug.txt | 7 +++--
> include/block/block.h | 62 +
> include/block/block_int.h | 2 +-
> qapi/block-core.json | 4 ++-
> 6 files changed, 21 insertions(+), 135 deletions(-)
>
> diff --git a/block.c b/block.c
> index 3a7324b..9971976 100644
> --- a/block.c
> +++ b/block.c
> @@ -2851,7 +2851,7 @@ ImageInfoSpecific
> *bdrv_get_specific_info(BlockDriverState *bs)
> return NULL;
> }
>
> -void bdrv_debug_event(BlockDriverState *bs, BlkDebugEvent event)
> +void bdrv_debug_event(BlockDriverState *bs, BlkdebugEvent event)
> {
> if (!bs || !bs->drv || !bs->drv->bdrv_debug_event) {
> return;
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 6860a2b..76805a6 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -35,7 +35,7 @@ typedef struct BDRVBlkdebugState {
> int state;
> int new_state;
>
> -QLIST_HEAD(, BlkdebugRule) rules[BLKDBG_EVENT_MAX];
> +QLIST_HEAD(, BlkdebugRule) rules[BLKDBG_MAX];
> QSIMPLEQ_HEAD(, BlkdebugRule) active_rules;
> QLIST_HEAD(, BlkdebugSuspendedReq) suspended_reqs;
> } BDRVBlkdebugState;
> @@ -63,7 +63,7 @@ enum {
> };
>
> typedef struct BlkdebugRule {
> -BlkDebugEvent event;
> +BlkdebugEvent event;
> int action;
> int state;
> union {
> @@ -142,69 +142,12 @@ static QemuOptsList *config_groups[] = {
> NULL
> };
>
> -static const char *event_names[BLKDBG_EVENT_MAX] = {
> -[BLKDBG_L1_UPDATE] = "l1_update",
> -[BLKDBG_L1_GROW_ALLOC_TABLE]= "l1_grow.alloc_table",
> -[BLKDBG_L1_GROW_WRITE_TABLE]= "l1_grow.write_table",
> -[BLKDBG_L1_GROW_ACTIVATE_TABLE] = "l1_grow.activate_table",
> -
> -[BLKDBG_L2_LOAD]= "l2_load",
> -[BLKDBG_L2_UPDATE] = "l2_update",
> -[BLKDBG_L2_UPDATE_COMPRESSED] = "l2_update_compressed",
> -[BLKDBG_L2_ALLOC_COW_READ] = "l2_alloc.cow_read",
> -[BLKDBG_L2_ALLOC_WRITE] = "l2_alloc.write",
> -
> -[BLKDBG_READ_AIO] = "read_aio",
> -[BLKDBG_READ_BACKING_AIO] = "read_backing_aio",
> -[BLKDBG_READ_COMPRESSED]= "read_compressed",
> -
> -[BLKDBG_WRITE_AIO] = "write_aio",
> -[BLKDBG_WRITE_COMPRESSED] = "write_compressed",
> -
> -[BLKDBG_VMSTATE_LOAD] = "vmstate_load",
> -[BLKDBG_VMSTATE_SAVE] = "vmstate_save",
> -
> -[BLKDBG_COW_READ] = "cow_read",
> -[BLKDBG_COW_WRITE] = "cow_write",
> -
> -[BLKDBG_REFTABLE_LOAD] = "reftable_load",
> -[BLKDBG_REFTABLE_GROW] = "reftable_grow",
> -[BLKDBG_REFTABLE_UPDATE]= "reftable_update",
> -
> -[BLKDBG_REFBLOCK_LOAD] = "refblock_load",
> -[BLKDBG_REFBLOCK_UPDATE]= "refblock_update",
> -[BLKDBG_REFBLOCK_UPDATE_PART] = "refblock_update_part",
> -[BLKDBG_REFBLOCK_ALLOC] = "refblock_alloc",
> -[BLKDBG_REFBLOCK_ALLOC_HOOKUP] = "refblock_alloc.hookup",
> -[BLKDBG_REFBLOCK_ALLOC_WRITE] = "refblock_alloc.write",
> -[BLKDBG_REFBLOCK_ALLOC_WRITE_BLOCKS]= "refblock_alloc.write_blocks",
> -[BLKDBG_REFBLOCK_ALLOC_WRITE_TABLE] = "refblock_alloc.write_table",
> -[BLKDBG_REFBLOCK_ALLOC_SWITCH_TABLE]= "refblock_alloc.switch_table",
> -
> -[BLKDBG_CLUSTER_ALLOC] = "cluster_alloc",
> -[BLKDBG_CLUSTER_ALLOC_BYTES]= "cluster_alloc_bytes",
> -[BLKDBG_CLUSTER_FREE] = "cluster_free",
> -
> -[BLKDBG_FLUSH_TO_OS]= "flush_to_os",
> -[BLKDBG_FLUSH_TO_DISK] = "flush_to_disk",
> -
> -[BLKDBG_PWRITEV_RMW_HEAD] = "pwritev_rmw.head",
> -[BLKDBG_PWRITEV_RMW_AFTER_HEAD] = "pwritev_rmw.after_head",
> -[BLKDBG_PWRITEV_RMW_TAIL] = "pwritev_rmw.tail",
> -[BLKDBG_PWRITEV_RMW_AFTER_TAIL] = "pwritev_rmw.after_tail",
> -[BLKDBG_PWRITEV]= "pwritev",
> -[BLKDBG_PWRITEV_ZERO] = "pwritev_zero",
> -[BLKDBG_PWRITEV_DONE] = "pwritev_done",
> -
> -[BLKDBG_EMPTY_IMAGE_PREPARE]= "empty_image_prepare",
> -};
> -
> -static int get_e
[Qemu-devel] [PATCH v2] configure: preserve various environment variables in config.status
The config.status script is auto-generated by configure upon completion. The intention is that config.status can be later invoked by the developer to re-detect the same environment that configure originally used. The current config.status script, however, only contains a record of the command line arguments to configure. Various environment variables have an effect on what configure will find. In particular the PKG_CONFIG_LIBDIR & PKG_CONFIG_PATH vars will affect what libraries pkg-config finds. The PATH var will affect what toolchain binaries and -config scripts are found. The LD_LIBRARY_PATH var will affect what libraries are found. Most commands have env variables that will override the name/path of the default version configure finds. All these key env variables should be recorded in the config.status script. Signed-off-by: Daniel P. Berrange --- configure | 38 ++ 1 file changed, 38 insertions(+) diff --git a/configure b/configure index d7472d7..09a503c 100755 --- a/configure +++ b/configure @@ -5925,6 +5925,44 @@ cat> config.status + echo "export $envname" >> config.status +fi +} + +# Preserve various env variables that influence what +# features/build target configure will detect +preserve_env AR +preserve_env AS +preserve_env CC +preserve_env CPP +preserve_env CXX +preserve_env INSTALL +preserve_env LD +preserve_env LD_LIBRARY_PATH +preserve_env LIBTOOL +preserve_env MAKE +preserve_env NM +preserve_env OBJCOPY +preserve_env PATH +preserve_env PKG_CONFIG +preserve_env PKG_CONFIG_LIBDIR +preserve_env PKG_CONFIG_PATH +preserve_env PYTHON +preserve_env SDL_CONFIG +preserve_env SDL2_CONFIG +preserve_env SMBD +preserve_env STRIP +preserve_env WINDRES + printf "exec" >>config.status printf " '%s'" "$0" "$@" >>config.status echo >>config.status -- 2.5.0
Re: [Qemu-devel] [PATCH 1/2] mirror: Rewrite mirror_iteration
On 17/11/2015 12:41, Fam Zheng wrote:
> +/* Wait for I/O to this cluster (from a previous iteration) to be
> + * done.*/
> +while (test_bit(next_chunk, s->in_flight_bitmap)) {
> +trace_mirror_yield_in_flight(s, next_sector, s->in_flight);
> +s->waiting_for_io = true;
> +qemu_coroutine_yield();
> +s->waiting_for_io = false;
> +}
> +
I think this could just "break" if nb_chunks > 0, like
if (test_bit(next_chunk, s->in_flight_bitmap)) {
if (nb_chunks > 0) {
break;
}
mirror_wait_for_io(s);
/* Now retry. */
} else {
hbitmap_next = hbitmap_iter_next(&s->hbi);
assert(hbitmap_next == next_sector);
nb_chunks++;
}
but it can be done later (the usage of mirror_wait_for_io is a hint :)).
There's a typo though:
> +/* Clear dirty bits before querying the block status, because
> + * calling bdrv_reset_dirty_bitmap could yield - if some blocks are
> marked
That's bdrv_get_block_status_above.
> + * dirty in this window, we need to know.
> + */
Thanks,
Paolo
Re: [Qemu-devel] [PATCH qemu] spapr: Add /system-id
On Wed, Nov 18, 2015 at 06:45:39PM +1100, Alexey Kardashevskiy wrote: > On 11/09/2015 07:47 PM, David Gibson wrote: > >On Mon, Nov 09, 2015 at 05:47:17PM +1100, Alexey Kardashevskiy wrote: > >>Section B.6.2.1 Root Node Properties of PAPR specification defines > >>a set of properties which shall be present in the device tree root, > >>one of these properties is "system-id" which "should be unique across > >>all systems and all manufacturers". Since UUID is meant to be unique, > >>it makes sense to use it as "system-id". > >> > >>This adds "system-id" property to the device tree root when not empty. > >> > >>Signed-off-by: Alexey Kardashevskiy > >>--- > >> > >>This might be expected by AIX so here is the patch. > >>I am really not sure if it makes sense to initialize property when > >>UUID is all zeroes as the requirement is "unique" and zero-uuid is > >>not. > > > >Yeah, I think it would be better to omit system-id entirely when a > >UUID hasn't been supplied. > > > so this did not go anywhere yet, did it? No. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH] migration: Add state records for migration incoming
zhanghailiang wrote: > For migration destination, sometimes we need to know its state, > and it is also useful for tracing migration incoming process. > > Here we add a new member 'state' for MigrationIncomingState, > and also use migrate_set_state() to modify its value. > We fix the first parameter of migrate_set_state(), and make it > public. > > Signed-off-by: zhanghailiang > Reviewed-by: Dr. David Alan Gilbert 1st: split the patch about the change in migrate_set_state prototype, and the rest. Once there, if we are going to do this, at least show it on info migrate on destination? If we are going this way, I think it is going to be better to reuse MigrationState? That way, we could make the info migrate statistics easier to understand? Thanks, Juan.
Re: [Qemu-devel] [PATCH v12 22/36] qapi: Don't let implicit enum MAX member collide
Eric Blake wrote:
> Now that we guarantee the user doesn't have any enum values
> beginning with a single underscore, we can use that for our
> own purposes. Renaming ENUM_MAX to ENUM__MAX makes it obvious
> that the sentinel is generated.
>
> This patch was mostly generated by applying a temporary patch:
>
> |diff --git a/scripts/qapi.py b/scripts/qapi.py
> |index e6d014b..b862ec9 100644
> |--- a/scripts/qapi.py
> |+++ b/scripts/qapi.py
> |@@ -1570,6 +1570,7 @@ const char *const %(c_name)s_lookup[] = {
> | max_index = c_enum_const(name, 'MAX', prefix)
> | ret += mcgen('''
> | [%(max_index)s] = NULL,
> |+// %(max_index)s
> | };
> | ''',
> |max_index=max_index)
>
> then running:
>
> $ cat qapi-{types,event}.c tests/test-qapi-types.c |
> sed -n 's,^// \(.*\)MAX,s|\1MAX|\1_MAX|g,p' > list
> $ git grep -l _MAX | xargs sed -i -f list
>
> The only things not generated are the changes in scripts/qapi.py.
>
> Signed-off-by: Eric Blake
For migration bits, I have zero objections about the changes. I trust
you that you have done all the required changes (i.e. I haven't compiled
it). Rest of the patch is as trivial as the commit log explains, so
Reviewed-by: Juan Quintela
Thanks, Juan.
Re: [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes
"Denis V. Lunev" wrote: for 2.9? You really plane very well in advance? O:-) As you asked for review from migration side > with test > while /bin/true ; do > virsh snapshot-create rhel7 > sleep 10 > virsh snapshot-delete rhel7 --current > done > with enabled iothreads on a running VM leads to a lot of troubles: hangs, > asserts, errors. > > Anyway, I think that the construction like > assert(aio_context_is_locked(aio_context)); > should be widely used to ensure proper locking. > > Changes from v8: > - split patch 5 into 2 separate patches making changes better to understand > and > review > > Changes from v7: > - patch 5: fixes for bdrv_all_find_snapshot. Changed parameter name to > skip_read_only which better reflects the meaning of the value and > fixed checks inside to conform the note from Stefan > > Changes from v6: > - tricky part dropped from patch 7 > - patch 5 reworked to process snapshot list differently in info commands > and on savevm > > Changes from v5: > - dropped already merged patch 11 > - fixed spelling in patch 1 > - changed order of condition in loops in all patches. Thank you Stefan > - dropped patch 9 > - aio_context is not acquired any more in bdrv_all_find_vmstate_bs by request > of Stefan > - patch 10 is implemented in completely different way > > Changes from v4: > - only migration/savevm.c code and monitor is affected now. Generic block > layer stuff will be sent separately to speedup merging. The approach > in general was negotiated with Juan and Stefan. > > Changes from v3: > - more places found > - new aio_poll concept, see patch 10 > > Changes from v2: > - droppped patch 5 as already merged > - changed locking scheme in patch 4 by suggestion of Juan > > Changes from v1: > - aio-context locking added > - comment is rewritten > > Signed-off-by: Denis V. Lunev > CC: Stefan Hajnoczi > CC: Juan Quintela > CC: Kevin Wolf > > Denis V. Lunev (10): > snapshot: create helper to test that block drivers supports snapshots > snapshot: return error code from bdrv_snapshot_delete_by_id_or_name > snapshot: create bdrv_all_delete_snapshot helper > snapshot: create bdrv_all_goto_snapshot helper > snapshot: create bdrv_all_find_snapshot helper > migration: drop find_vmstate_bs check in hmp_delvm > snapshot: create bdrv_all_create_snapshot helper > migration: reorder processing in hmp_savevm > migration: implement bdrv_all_find_vmstate_bs helper > migration: normalize locking in migration/savevm.c > > block/snapshot.c | 135 ++- > include/block/snapshot.h | 25 +- > migration/savevm.c | 207 > +++ > 3 files changed, 217 insertions(+), 150 deletions(-)
Re: [Qemu-devel] [PATCH 01/11] snapshot: create helper to test that block drivers supports snapshots
"Denis V. Lunev" wrote: > The patch enforces proper locking for this operation. > > Signed-off-by: Denis V. Lunev > Reviewed-by: Greg Kurz > Reviewed-by: Stefan Hajnoczi > Reviewed-by: Fam Zheng > CC: Juan Quintela > CC: Kevin Wolf Reviewed-by: Juan Quintela
Re: [Qemu-devel] [PATCH 02/11] snapshot: return error code from bdrv_snapshot_delete_by_id_or_name
"Denis V. Lunev" wrote: > this will make code better in the next patch > > Signed-off-by: Denis V. Lunev > Reviewed-by: Stefan Hajnoczi > Reviewed-by: Fam Zheng > CC: Juan Quintela > CC: Kevin Wolf Reviewed-by: Juan Quintela
Re: [Qemu-devel] [PATCH 03/11] snapshot: create bdrv_all_delete_snapshot helper
"Denis V. Lunev" wrote: > to delete snapshots from all loaded block drivers. > > The patch also ensures proper locking. > > Signed-off-by: Denis V. Lunev > Reviewed-by: Stefan Hajnoczi > Reviewed-by: Fam Zheng > CC: Juan Quintela > CC: Kevin Wolf Reviewed-by: Juan Quintela I will still suggest to rename it to brdv_delete_all_snapshots() or bdrv_delete_snapshot_everywhere() that makes is more clear what it does. A big THANKS for removing this from the migration side of the equation O:-) Later, Juan.
Re: [Qemu-devel] [PATCH 04/11] snapshot: create bdrv_all_goto_snapshot helper
"Denis V. Lunev" wrote:
> to switch to snapshot on all loaded block drivers.
>
> The patch also ensures proper locking.
>
> Signed-off-by: Denis V. Lunev
> Reviewed-by: Greg Kurz
> Reviewed-by: Stefan Hajnoczi
> Reviewed-by: Fam Zheng
> CC: Juan Quintela
> CC: Kevin Wolf
Reviewed-by: Juan Quintela
suggestion:
un-export bdrv_snapshot_goto()?
I haven't looked if it is used anywhere else, but looks like one idea.
> ---
> block/snapshot.c | 20
> include/block/snapshot.h | 1 +
> migration/savevm.c | 15 +--
> 3 files changed, 26 insertions(+), 10 deletions(-)
>
> diff --git a/block/snapshot.c b/block/snapshot.c
> index 61a6ad1..9f07a63 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -403,3 +403,23 @@ int bdrv_all_delete_snapshot(const char *name,
> BlockDriverState **first_bad_bs,
> *first_bad_bs = bs;
> return ret;
> }
> +
> +
> +int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
> +{
> +int err = 0;
> +BlockDriverState *bs = NULL;
> +
> +while (err == 0 && (bs = bdrv_next(bs))) {
> +AioContext *ctx = bdrv_get_aio_context(bs);
> +
> +aio_context_acquire(ctx);
> +if (bdrv_can_snapshot(bs)) {
> +err = bdrv_snapshot_goto(bs, name);
> +}
> +aio_context_release(ctx);
> +}
> +
> +*first_bad_bs = bs;
> +return err;
> +}
> diff --git a/include/block/snapshot.h b/include/block/snapshot.h
> index d02d2b1..0a176c7 100644
> --- a/include/block/snapshot.h
> +++ b/include/block/snapshot.h
> @@ -84,5 +84,6 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState
> *bs,
> bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
> int bdrv_all_delete_snapshot(const char *name, BlockDriverState
> **first_bsd_bs,
> Error **err);
> +int bdrv_all_goto_snapshot(const char *name, BlockDriverState
> **first_bsd_bs);
>
> #endif
> diff --git a/migration/savevm.c b/migration/savevm.c
> index c52cbbe..254e51d 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2093,16 +2093,11 @@ int load_vmstate(const char *name)
> /* Flush all IO requests so they don't interfere with the new state. */
> bdrv_drain_all();
>
> -bs = NULL;
> -while ((bs = bdrv_next(bs))) {
> -if (bdrv_can_snapshot(bs)) {
> -ret = bdrv_snapshot_goto(bs, name);
> -if (ret < 0) {
> -error_report("Error %d while activating snapshot '%s' on
> '%s'",
> - ret, name, bdrv_get_device_name(bs));
> -return ret;
> -}
> -}
> +ret = bdrv_all_goto_snapshot(name, &bs);
> +if (ret < 0) {
> +error_report("Error %d while activating snapshot '%s' on '%s'",
> + ret, name, bdrv_get_device_name(bs));
> +return ret;
> }
>
> /* restore the VM state */
Re: [Qemu-devel] [PATCH 05/11] migration: factor our snapshottability check in load_vmstate
"Denis V. Lunev" wrote: > We should check that all inserted and not read-only images support > snapshotting. This could be made using already invented helper > bdrv_all_can_snapshot(). > > Signed-off-by: Denis V. Lunev Reviewed-by: Juan Quintela
Re: [Qemu-devel] [PATCH 07/11] migration: drop find_vmstate_bs check in hmp_delvm
"Denis V. Lunev" wrote: > There is no much sense to do the check and write warning. > > Signed-off-by: Denis V. Lunev > Reviewed-by: Stefan Hajnoczi > Reviewed-by: Fam Zheng > CC: Juan Quintela Reviewed-by: Juan Quintela
Re: [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes
On 11/18/2015 01:56 PM, Juan Quintela wrote: "Denis V. Lunev" wrote: for 2.9? You really plane very well in advance? O:-) ha-ha :) wrong character replaced. I have planned to switch v8 to v9 but typed 9 instead of 5 2 symbols before... Den
Re: [Qemu-devel] [PATCH 06/11] snapshot: create bdrv_all_find_snapshot helper
"Denis V. Lunev" wrote: > to check that snapshot is available for all loaded block drivers. > The check bs != bs1 in hmp_info_snapshots is an optimization. The check > for availability of this snapshot will return always true as the list > of snapshots was collected from that image. > > The patch also ensures proper locking. > > Signed-off-by: Denis V. Lunev > CC: Juan Quintela > CC: Stefan Hajnoczi > CC: Kevin Wolf Reviewed-by: Juan Quintela
Re: [Qemu-devel] [PATCH 08/11] snapshot: create bdrv_all_create_snapshot helper
"Denis V. Lunev" wrote: > to create snapshot for all loaded block drivers. > > The patch also ensures proper locking. > > Signed-off-by: Denis V. Lunev > Reviewed-by: Stefan Hajnoczi > Reviewed-by: Fam Zheng > CC: Juan Quintela > CC: Kevin Wolf Reviewed-by: Juan Quintela
Re: [Qemu-devel] [PATCH 10/11] migration: implement bdrv_all_find_vmstate_bs helper
"Denis V. Lunev" wrote: > The patch also ensures proper locking for the operation. > > Signed-off-by: Denis V. Lunev > Reviewed-by: Stefan Hajnoczi > Reviewed-by: Fam Zheng > CC: Juan Quintela > CC: Kevin Wolf Reviewed-by: Juan Quintela
Re: [Qemu-devel] [PATCH 09/11] migration: reorder processing in hmp_savevm
"Denis V. Lunev" wrote: > State deletion can be performed on running VM which reduces VM downtime > This approach looks a bit more natural. > > Signed-off-by: Denis V. Lunev > Reviewed-by: Stefan Hajnoczi > Reviewed-by: Fam Zheng > CC: Juan Quintela Reviewed-by: Juan Quintela
Re: [Qemu-devel] [PATCH] MAINTAINERS: Update TCG CPU cores section
On 13/11/2015 18:57, Peter Crosthwaite wrote: > On Fri, Nov 13, 2015 at 9:53 AM, Paolo Bonzini wrote: >> These are the people that I think have been touching it lately >> or reviewing patches. >> >> Signed-off-by: Paolo Bonzini > > >> --- >> MAINTAINERS | 17 + >> 1 file changed, 13 insertions(+), 4 deletions(-) >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 9e1fa72..f67a026 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -62,14 +62,22 @@ Guest CPU cores (TCG): >> -- >> Overall >> L: qemu-devel@nongnu.org >> -S: Odd fixes >> +M: Peter Crosthwaite >> +M: Richard Henderson >> +M: Paolo Bonzini > > Happy to help out where possible. Does the order mean anything though? No meaning at all. I will make it alphabetic. Paolo > Regards, > Peter > >> +S: Maintained >> F: cpu-exec.c >> +F: cpu-exec-common.c >> +F: cpus.c >> F: cputlb.c >> +F: exec.c >> F: softmmu_template.h >> -F: translate-all.c >> -F: include/exec/cpu_ldst.h >> -F: include/exec/cpu_ldst_template.h >> +F: translate-all.* >> +F: translate-common.c >> +F: include/exec/cpu*.h >> +F: include/exec/exec-all.h >> F: include/exec/helper*.h >> +F: include/exec/tb-hash.h >> >> Alpha >> M: Richard Henderson >> @@ -1042,6 +1050,7 @@ S: Supported >> F: include/exec/ioport.h >> F: ioport.c >> F: include/exec/memory.h >> +F: include/exec/ram_addr.h >> F: memory.c >> F: include/exec/memory-internal.h >> F: exec.c >> -- >> 2.5.0 >>
Re: [Qemu-devel] [kvm-unit-test RFC] x86: Memory instructions test case
On 04/11/2015 22:21, Eduardo Habkost wrote:
> Quickly hacked test case for memory instructions (clflush, mfence,
> sfence, lfence, clflushopt, clwb, pcommit), that simply checks for #UD
> exceptions.
>
> This was useful to test TCG handling of those instructions.
>
> The "fake clwb" part will probably break once a new instruction use
> those opcodes.
>
> Signed-off-by: Eduardo Habkost
> ---
> config/config-x86-common.mak | 2 +
> config/config-x86_64.mak | 2 +-
> x86/memory.c | 88
>
> 3 files changed, 91 insertions(+), 1 deletion(-)
> create mode 100644 x86/memory.c
>
> diff --git a/config/config-x86-common.mak b/config/config-x86-common.mak
> index c2f9908..b89684d 100644
> --- a/config/config-x86-common.mak
> +++ b/config/config-x86-common.mak
> @@ -108,6 +108,8 @@ $(TEST_DIR)/vmx.elf: $(cstart.o) $(TEST_DIR)/vmx.o
> $(TEST_DIR)/vmx_tests.o
>
> $(TEST_DIR)/debug.elf: $(cstart.o) $(TEST_DIR)/debug.o
>
> +$(TEST_DIR)/memory.elf: $(cstart.o) $(TEST_DIR)/memory.o
> +
> arch_clean:
> $(RM) $(TEST_DIR)/*.o $(TEST_DIR)/*.flat $(TEST_DIR)/*.elf \
> $(TEST_DIR)/.*.d lib/x86/.*.d
> diff --git a/config/config-x86_64.mak b/config/config-x86_64.mak
> index 7d4eb34..ec4bded 100644
> --- a/config/config-x86_64.mak
> +++ b/config/config-x86_64.mak
> @@ -7,7 +7,7 @@ tests = $(TEST_DIR)/access.flat $(TEST_DIR)/apic.flat \
> $(TEST_DIR)/emulator.flat $(TEST_DIR)/idt_test.flat \
> $(TEST_DIR)/xsave.flat $(TEST_DIR)/rmap_chain.flat \
> $(TEST_DIR)/pcid.flat $(TEST_DIR)/debug.flat \
> - $(TEST_DIR)/ioapic.flat
> + $(TEST_DIR)/ioapic.flat $(TEST_DIR)/memory.flat
> tests += $(TEST_DIR)/svm.flat
> tests += $(TEST_DIR)/vmx.flat
> tests += $(TEST_DIR)/tscdeadline_latency.flat
> diff --git a/x86/memory.c b/x86/memory.c
> new file mode 100644
> index 000..cd1eb46
> --- /dev/null
> +++ b/x86/memory.c
> @@ -0,0 +1,88 @@
> +/*
> + * Test for x86 cache and memory instructions
> + *
> + * Copyright (c) 2015 Red Hat Inc
> + *
> + * Authors:
> + * Eduardo Habkost
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.
> + */
> +
> +#include "libcflat.h"
> +#include "desc.h"
> +#include "processor.h"
> +
> +static long target;
> +static volatile int ud;
> +static volatile int isize;
> +
> +static void handle_ud(struct ex_regs *regs)
> +{
> + ud = 1;
> + regs->rip += isize;
> +}
> +
> +int main(int ac, char **av)
> +{
> + struct cpuid cpuid7, cpuid1;
> + int xfail;
> +
> + setup_idt();
> + handle_exception(UD_VECTOR, handle_ud);
> +
> + cpuid1 = cpuid(1);
> + cpuid7 = cpuid_indexed(7, 0);
> +
> + /* 3-byte instructions: */
> + isize = 3;
> +
> + xfail = !(cpuid1.d & (1U << 19)); /* CLFLUSH */
> + ud = 0;
> + asm volatile("clflush (%0)" : : "b" (&target));
> + report_xfail("clflush", xfail, ud == 0);
> +
> + xfail = !(cpuid1.d & (1U << 25)); /* SSE */
> + ud = 0;
> + asm volatile("sfence");
> + report_xfail("sfence", xfail, ud == 0);
> +
> + xfail = !(cpuid1.d & (1U << 26)); /* SSE2 */
> + ud = 0;
> + asm volatile("lfence");
> + report_xfail("lfence", xfail, ud == 0);
> +
> + ud = 0;
> + asm volatile("mfence");
> + report_xfail("mfence", xfail, ud == 0);
> +
> + /* 4-byte instructions: */
> + isize = 4;
> +
> + xfail = !(cpuid7.b & (1U << 23)); /* CLFLUSHOPT */
> + ud = 0;
> + /* clflushopt (%rbx): */
> + asm volatile(".byte 0x66, 0x0f, 0xae, 0x3b" : : "b" (&target));
> + report_xfail("clflushopt", xfail, ud == 0);
> +
> + xfail = !(cpuid7.b & (1U << 24)); /* CLWB */
> + ud = 0;
> + /* clwb (%rbx): */
> + asm volatile(".byte 0x66, 0x0f, 0xae, 0x33" : : "b" (&target));
> + report_xfail("clwb", xfail, ud == 0);
> +
> + ud = 0;
> + /* clwb requires a memory operand, the following is NOT a valid
> + * CLWB instruction (modrm == 0xF0).
> + */
> + asm volatile(".byte 0x66, 0x0f, 0xae, 0xf0");
> + report("fake clwb", ud);
> +
> + xfail = !(cpuid7.b & (1U << 22)); /* PCOMMIT */
> + ud = 0;
> + /* pcommit: */
> + asm volatile(".byte 0x66, 0x0f, 0xae, 0xf8");
> + report_xfail("pcommit", xfail, ud == 0);
> +
> + return report_summary();
> +}
>
Applied, thanks.
Paolo
Re: [Qemu-devel] [PATCH 11/11] migration: normalize locking in migration/savevm.c
"Denis V. Lunev" wrote:
> basically all bdrv_* operations must be called under aio_context_acquire
> except ones with bdrv_all prefix.
>
> Signed-off-by: Denis V. Lunev
> Reviewed-by: Stefan Hajnoczi
> Reviewed-by: Fam Zheng
> CC: Juan Quintela
> CC: Kevin Wolf
I will preffer that migration code don't know about aiocontexts.
> @@ -2048,9 +2054,12 @@ int load_vmstate(const char *name)
> error_report("No block device supports snapshots");
> return -ENOTSUP;
> }
> +aio_context = bdrv_get_aio_context(bs);
>
> /* Don't even try to load empty VM states */
> +aio_context_acquire(aio_context);
> ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
> +aio_context_release(aio_context);
Why are we dropping it here?
I can understand doing it on the error case.
> if (ret < 0) {
> return ret;
> } else if (sn.vm_state_size == 0) {
> @@ -2078,9 +2087,12 @@ int load_vmstate(const char *name)
>
> qemu_system_reset(VMRESET_SILENT);
> migration_incoming_state_new(f);
> -ret = qemu_loadvm_state(f);
>
> +aio_context_acquire(aio_context);
We have done a qemu_fopen_bdrv() without acquiring the context, not sure
if we really need it (or not). My understanding of locking is that we
should get the context on first use and maintain it until last use.
Does it work in a different way?
> +ret = qemu_loadvm_state(f);
> qemu_fclose(f);
> +aio_context_release(aio_context);
> +
> migration_incoming_state_destroy();
> if (ret < 0) {
> error_report("Error %d while loading VM state", ret);
> @@ -2111,14 +2123,19 @@ void hmp_info_snapshots(Monitor *mon, const QDict
> *qdict)
> int nb_sns, i;
> int total;
> int *available_snapshots;
> +AioContext *aio_context;
>
> bs = bdrv_all_find_vmstate_bs();
> if (!bs) {
> monitor_printf(mon, "No available block device supports
> snapshots\n");
> return;
> }
> +aio_context = bdrv_get_aio_context(bs);
>
> +aio_context_acquire(aio_context);
> nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> +aio_context_release(aio_context);
> +
> if (nb_sns < 0) {
> monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
> return;
I will very much preffer to have a:
bdrv_snapshot_list_full_whatever()
That does the two operations and don't make me know about aio_contexts.
What I need there really is just the block layer to fill the sn_tab and
to told me if there is a problem, no real need of knowing about
contexts, no?
Thanks, Juan.
Re: [Qemu-devel] [PATCH 2/2] add support for KVM_CAP_SPLIT_IRQCHIP
On 14/11/2015 00:25, Matt Gingell wrote: > +#define APIC_DELIVERY_MODE_SHIFT 8; > +#define APIC_POLARITY_SHIFT 14; > +#define APIC_TRIG_MODE_SHIFT 15; > + Stray semicolons, fixed and queued for QEMU 2.6. Paolo
Re: [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes
"Denis V. Lunev" wrote: Hi Kevin, Stefan > with test > while /bin/true ; do > virsh snapshot-create rhel7 > sleep 10 > virsh snapshot-delete rhel7 --current > done > with enabled iothreads on a running VM leads to a lot of troubles: hangs, > asserts, errors. > > Anyway, I think that the construction like > assert(aio_context_is_locked(aio_context)); > should be widely used to ensure proper locking. > > Changes from v8: > - split patch 5 into 2 separate patches making changes better to understand > and > review > > Changes from v7: > - patch 5: fixes for bdrv_all_find_snapshot. Changed parameter name to > skip_read_only which better reflects the meaning of the value and > fixed checks inside to conform the note from Stefan > > Changes from v6: > - tricky part dropped from patch 7 > - patch 5 reworked to process snapshot list differently in info commands > and on savevm > > Changes from v5: > - dropped already merged patch 11 > - fixed spelling in patch 1 > - changed order of condition in loops in all patches. Thank you Stefan > - dropped patch 9 > - aio_context is not acquired any more in bdrv_all_find_vmstate_bs by request > of Stefan > - patch 10 is implemented in completely different way > > Changes from v4: > - only migration/savevm.c code and monitor is affected now. Generic block > layer stuff will be sent separately to speedup merging. The approach > in general was negotiated with Juan and Stefan. > > Changes from v3: > - more places found > - new aio_poll concept, see patch 10 > > Changes from v2: > - droppped patch 5 as already merged > - changed locking scheme in patch 4 by suggestion of Juan > > Changes from v1: > - aio-context locking added > - comment is rewritten > > Signed-off-by: Denis V. Lunev > CC: Stefan Hajnoczi > CC: Juan Quintela > CC: Kevin Wolf > > Denis V. Lunev (10): > snapshot: create helper to test that block drivers supports snapshots > snapshot: return error code from bdrv_snapshot_delete_by_id_or_name > snapshot: create bdrv_all_delete_snapshot helper > snapshot: create bdrv_all_goto_snapshot helper > snapshot: create bdrv_all_find_snapshot helper > migration: drop find_vmstate_bs check in hmp_delvm > snapshot: create bdrv_all_create_snapshot helper > migration: reorder processing in hmp_savevm > migration: implement bdrv_all_find_vmstate_bs helper > migration: normalize locking in migration/savevm.c > > block/snapshot.c | 135 ++- > include/block/snapshot.h | 25 +- > migration/savevm.c | 207 > +++ > 3 files changed, 217 insertions(+), 150 deletions(-) There are more changes on migration/* that on block/*, do you want to take the changes yourselves or should I take them? Should they go for 2.5 or 2.6? Both options are ok with me. Later, Juan.
Re: [Qemu-devel] [PATCH 1/5] migration: split hmp_savevm to do_savevm and hmp_savevm wrapper
"Denis V. Lunev" wrote: > This would be useful in the next step when QMP version of this call will > be introduced. > > Signed-off-by: Denis V. Lunev > CC: Juan Quintela > CC: Amit Shah > CC: Markus Armbruster > CC: Eric Blake Reviewed-by: Juan Quintela
Re: [Qemu-devel] [PATCH 2/5] qmp: create qmp_savevm command
Markus Armbruster wrote:
> "Denis V. Lunev" writes:
>
>> Signed-off-by: Denis V. Lunev
>> CC: Juan Quintela
>> CC: Amit Shah
>> CC: Markus Armbruster
>> CC: Eric Blake
>> ---
>> migration/savevm.c | 5 +
>> qapi-schema.json | 13 +
>> qmp-commands.hx| 25 +
>> 3 files changed, 43 insertions(+)
>>
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index f83ffd0..565b10a 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -2010,6 +2010,11 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>> }
>> }
>>
>> +void qmp_savevm(bool has_name, const char *name, Error **errp)
>> +{
>> +do_savevm(has_name ? name : NULL, errp);
>> +}
>> +
>
> Please name do_savevm() qmp_savevm() and drop this wrapper.
>
> We're working on omitting has_FOO for pointer-valued FOO.
Agreed.
>
>> void qmp_xen_save_devices_state(const char *filename, Error **errp)
>> {
>> QEMUFile *f;
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index b65905f..8cc8b44 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -3962,3 +3962,16 @@
>> ##
>> { 'enum': 'ReplayMode',
>>'data': [ 'none', 'record', 'play' ] }
>> +
>> +##
>> +# @savevm
>> +#
>> +# Save a VM snapshot. Without a name new snapshot is created",
>> +#
>> +# @name: identifier of a snapshot to be created
>
> Missing #optional tag.
>
> What happens when @name is missing?
Why are we allowing this? QMP is going to be called by libvirt or
similar, they can choose a name, thanks very much.
> What happens when @name names an existing snapshot?
We remove them.
/* Delete old snapshots of the same name */
if (name && del_existing_snapshots(mon, name) < 0) {
goto the_end;
}
I think we should give one error. Let the hmp code remove them.
I would preffer to change the interface to "require" a flag if we want
it to be removed. But requiring a flag is the equivalent of requiring
qmp_deletevm
qmp_savevm
or whatever they are called.
> Do we want @name to be optional in QMP? I dimly remember ambiguity
> problems between names and IDs. Perhaps it'll become clear later in the
> series.
Again, I think that requiring a name is the best thing to do. Libvirt,
openstack and friends are good at choosen names, qemu is not.
> The QMP interface needs to make sense on its own, even if that means
> deviating from HMP. Juan, Amit, do you have opinions on the proper QMP
> interface for internal snapshots?
I agree that the "requirement" of a name is a good idea. Not deleting
the existing snapshots looks like a good idea also. It is not difficult
to romeve them previously.
Thanks, Juan.
Re: [Qemu-devel] [PATCH 3/5] qmp: create qmp_delvm command
Markus Armbruster wrote:
> "Denis V. Lunev" writes:
>
>> Signed-off-by: Denis V. Lunev
>> CC: Juan Quintela
>> CC: Amit Shah
>> CC: Markus Armbruster
>> CC: Eric Blake
>> ---
>> migration/savevm.c | 27 ++-
>> qapi-schema.json | 13 +
>> qmp-commands.hx| 23 +++
>> 3 files changed, 54 insertions(+), 9 deletions(-)
>>
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 565b10a..90b6850 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -2115,17 +2115,26 @@ int load_vmstate(const char *name)
>> return 0;
>> }
>>
>> -void hmp_delvm(Monitor *mon, const QDict *qdict)
>> +void qmp_delvm(const char *name, Error **errp)
>> {
>> BlockDriverState *bs;
>> -Error *err;
>> -const char *name = qdict_get_str(qdict, "name");
>> -
>> -if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
>> -monitor_printf(mon,
>> - "Error while deleting snapshot on device '%s': %s\n",
>> - bdrv_get_device_name(bs), error_get_pretty(err));
>> -error_free(err);
>> +Error *local_err = NULL;
>> +
>> +if (bdrv_all_delete_snapshot(name, &bs, errp) < 0) {
>> +error_setg(errp, "Error while deleting snapshot on device '%s': %s",
>> + bdrv_get_device_name(bs), error_get_pretty(local_err));
>> +error_free(local_err);
>> +}
>> +}
>> +
>> +void hmp_delvm(Monitor *mon, const QDict *qdict)
>> +{
>> +Error *local_err = NULL;
>> +qmp_delvm(qdict_get_str(qdict, "name"), &local_err);
>> +
>> +if (local_err != NULL) {
>> +monitor_printf(mon, "%s\n", error_get_pretty(local_err));
>> +error_free(local_err);
>
> error_report_err(), please.
>
>> }
>> }
>
> Juan, Amit, in case you'd prefer to move out the parts that implement
> HMP on top of QMP: they can go into hmp.c as long as they're as simple
> as this one.
Perfect for me.
Thanks, Juan.
Re: [Qemu-devel] [PATCH 3/5] qmp: create qmp_delvm command
"Denis V. Lunev" wrote: > Signed-off-by: Denis V. Lunev > CC: Juan Quintela > CC: Amit Shah > CC: Markus Armbruster > CC: Eric Blake I agree with the changes, move them to hmp.c as Markus suggests.
Re: [Qemu-devel] [PATCH 5/5] qmp: create QMP implementation of loadvm command
"Denis V. Lunev" wrote:
> Signed-off-by: Denis V. Lunev
> CC: Juan Quintela
> CC: Amit Shah
> CC: Markus Armbruster
> CC: Eric Blake
> ---
> migration/savevm.c | 5 +
> qapi-schema.json | 13 +
> qmp-commands.hx| 23 +++
> 3 files changed, 41 insertions(+)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 08c6c65..f67b5d9 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2116,6 +2116,11 @@ int load_vmstate(const char *name, Error **errp)
> return 0;
> }
>
> +void qmp_loadvm(const char *name, Error **errp)
> +{
> +load_vmstate(name, errp);
> +}
> +
As Markus suggested for the other functions, rename load_vmstate qmp_loadvm()
Thanks, Juan.
[Qemu-devel] [PATCH v2] vhost-user: ignore qemu-only features
Some feratures (such as ctrl vq) are supported
by qemu without need to communicate with the
backend.
Drop them from the feature mask so we set them
unconditionally.
Reported-by: Victor Kaplansky
Signed-off-by: Michael S. Tsirkin
---
Changes since v1:
re-add VIRTIO_NET_F_GUEST_ANNOUNCE, add a comment
explaining why it's needed.
hw/net/vhost_net.c | 8 +---
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 14337a4..318c3e6 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -77,14 +77,8 @@ static const int user_feature_bits[] = {
VIRTIO_NET_F_HOST_ECN,
VIRTIO_NET_F_HOST_UFO,
VIRTIO_NET_F_MRG_RXBUF,
-VIRTIO_NET_F_STATUS,
-VIRTIO_NET_F_CTRL_VQ,
-VIRTIO_NET_F_CTRL_RX,
-VIRTIO_NET_F_CTRL_VLAN,
-VIRTIO_NET_F_CTRL_RX_EXTRA,
-VIRTIO_NET_F_CTRL_MAC_ADDR,
-VIRTIO_NET_F_CTRL_GUEST_OFFLOADS,
+/* This bit implies RARP isn't sent by QEMU out of band */
VIRTIO_NET_F_GUEST_ANNOUNCE,
VIRTIO_NET_F_MQ,
--
MST
[Qemu-devel] [PATCH 0/3] [For 2.5] Migration fixes
From: "Dr. David Alan Gilbert" Hi, These are 3 migration fixes that fix things I messed up. Only the 1st one (set last_sent_block) fixes a problem, the other two fix coverity findings. Dave Dr. David Alan Gilbert (3): Set last_sent_block migration: Dead assignment of current_time Unneeded NULL check migration/migration.c | 3 +-- migration/ram.c | 1 + 2 files changed, 2 insertions(+), 2 deletions(-) -- 2.5.0
[Qemu-devel] [PATCH 1/3] Set last_sent_block
From: "Dr. David Alan Gilbert"
In a82d593b61054b3dea43 I accidentally removed the setting of
last_sent_block, put it back.
Symptoms:
Multithreaded compression only uses one thread.
Migration is a bit less efficient since it won't use 'cont' flags.
Signed-off-by: Dr. David Alan Gilbert
Fixes: a82d593b61054b3dea43
---
migration/ram.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/migration/ram.c b/migration/ram.c
index 7f32696..1eb155a 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1249,6 +1249,7 @@ static int ram_save_target_page(MigrationState *ms,
QEMUFile *f,
if (unsentmap) {
clear_bit(dirty_ram_abs >> TARGET_PAGE_BITS, unsentmap);
}
+last_sent_block = block;
}
return res;
--
2.5.0
[Qemu-devel] [PATCH 2/3] migration: Dead assignment of current_time
From: "Dr. David Alan Gilbert"
I set current_time before the postcopy test but never use it;
(I think this was from the original version where it was time based).
Spotted by coverity, CID 1339208
Signed-off-by: Dr. David Alan Gilbert
---
migration/migration.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/migration/migration.c b/migration/migration.c
index 7e4e27b..265d13a 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1643,7 +1643,6 @@ static void *migration_thread(void *opaque)
if (pending_size && pending_size >= max_size) {
/* Still a significant amount to transfer */
-current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
if (migrate_postcopy_ram() &&
s->state != MIGRATION_STATUS_POSTCOPY_ACTIVE &&
pend_nonpost <= max_size &&
--
2.5.0
[Qemu-devel] [PATCH 3/3] Unneeded NULL check
From: "Dr. David Alan Gilbert"
The check is unneccesary, we read the value at the start of the
thread, use it, and never change it. The value is checked to be
non-NULL before thread creation.
Spotted by coverity, CID 1339211
Signed-off-by: Dr. David Alan Gilbert
---
migration/migration.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/migration/migration.c b/migration/migration.c
index 265d13a..1a42aee 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1345,7 +1345,7 @@ static void *source_return_path_thread(void *opaque)
break;
}
}
-if (rp && qemu_file_get_error(rp)) {
+if (qemu_file_get_error(rp)) {
trace_source_return_path_thread_bad_end();
mark_source_rp_bad(ms);
}
--
2.5.0
Re: [Qemu-devel] [PATCH v12 21/36] qapi: Tighten the regex on valid names
Eric Blake writes:
> We already documented that qapi names should match specific
> patterns (such as starting with a letter unless it was an enum
> value or a downstream extension). Tighten that from a suggestion
> into a hard requirement, which frees up names beginning with a
> single underscore for qapi internal usage.
If we care enough about naming conventions to document them, enforcing
them makes only sense.
> Also restrict things
> to avoid 'a__b' or 'a--b' (that is, dash and underscore must not
> occur consecutively).
I feel this is entering "foolish names" territory, where things like
"IcAnFiNdNaMeSeVeNlEsSrEaDaBlEtHaNStudlyCaps" live. Catching fools
automatically is generally futile, they're too creative :)
Let's drop this part.
> Add a new test, reserved-member-underscore, to demonstrate the
> tighter checking.
>
> Signed-off-by: Eric Blake
>
> ---
> v12: new patch
> ---
> docs/qapi-code-gen.txt| 19 ++-
> scripts/qapi.py | 12 +++-
> tests/Makefile| 1 +
> tests/qapi-schema/reserved-member-underscore.err | 1 +
> tests/qapi-schema/reserved-member-underscore.exit | 1 +
> tests/qapi-schema/reserved-member-underscore.json | 4
> tests/qapi-schema/reserved-member-underscore.out | 0
> 7 files changed, 24 insertions(+), 14 deletions(-)
> create mode 100644 tests/qapi-schema/reserved-member-underscore.err
> create mode 100644 tests/qapi-schema/reserved-member-underscore.exit
> create mode 100644 tests/qapi-schema/reserved-member-underscore.json
> create mode 100644 tests/qapi-schema/reserved-member-underscore.out
>
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index ceb9a78..ec225d1 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -118,17 +118,18 @@ tracking optional fields.
>
> Any name (command, event, type, field, or enum value) beginning with
> "x-" is marked experimental, and may be withdrawn or changed
> -incompatibly in a future release. Downstream vendors may add
> -extensions; such extensions should begin with a prefix matching
> +incompatibly in a future release. All names must begin with a letter,
> +and contain only ASCII letters, digits, dash, and underscore, where
> +dash and underscore cannot occur consecutively. There are two
> +exceptions: enum values may start with a digit, and any extensions
> +added by downstream vendors should start with a prefix matching
> "__RFQDN_" (for the reverse-fully-qualified-domain-name of the
> vendor), even if the rest of the name uses dash (example:
> -__com.redhat_drive-mirror). Other than downstream extensions (with
> -leading underscore and the use of dots), all names should begin with a
> -letter, and contain only ASCII letters, digits, dash, and underscore.
> -Names beginning with 'q_' are reserved for the generator: QMP names
> -that resemble C keywords or other problematic strings will be munged
> -in C to use this prefix. For example, a field named "default" in
> -qapi becomes "q_default" in the generated C code.
> +__com.redhat_drive-mirror). Names beginning with 'q_' are reserved
> +for the generator: QMP names that resemble C keywords or other
> +problematic strings will be munged in C to use this prefix. For
> +example, a field named "default" in qapi becomes "q_default" in the
> +generated C code.
>
> In the rest of this document, usage lines are given for each
> expression type, with literal strings written in lower case and
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 4a30bc0..e6d014b 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -353,9 +353,11 @@ def discriminator_find_enum_define(expr):
> return find_enum(discriminator_type)
>
>
> -# FIXME should enforce "other than downstream extensions [...], all
> -# names should begin with a letter".
> -valid_name = re.compile('^[a-zA-Z_][a-zA-Z0-9_.-]*$')
> +# Names must be letters, numbers, -, and _. They must start with letter,
> +# except for downstream extensions which must start with __RFQDN_.
> +# Dots are only valid in the downstream extension prefix.
> +valid_name = re.compile('^(__[a-zA-Z][a-zA-Z0-9.]*_)?'
> +'[a-zA-Z][a-zA-Z0-9]*([_-][a-zA-Z0-9]+)*$')
This regexp consists of two parts: optional __RFQDN_ prefix and the name
proper.
The latter stays simpler if we don't attempt to catch adjacent [-_].
The former isn't quite right. According to RFC 822 Appendix 1 - Domain
Name Syntax Specification:
* We must accept '-' in addition to digits.
* We require RFQDN to start with a letter. This is still a loose match.
The tight match is "labels separated by dots", where labels consist of
letters, digits and '-', starting with a letter. I wouldn't bother
checking first characters are letters at all.
Recommend
valid_name = re.compile('^(__[a-zA-Z0-9.-]+_)?'
Re: [Qemu-devel] [PATCH 1/3] Set last_sent_block
"Dr. David Alan Gilbert (git)" wrote: > From: "Dr. David Alan Gilbert" > > In a82d593b61054b3dea43 I accidentally removed the setting of > last_sent_block, put it back. > > Symptoms: > Multithreaded compression only uses one thread. > Migration is a bit less efficient since it won't use 'cont' flags. > > Signed-off-by: Dr. David Alan Gilbert > Fixes: a82d593b61054b3dea43 Reviewed-by: Juan Quintela
Re: [Qemu-devel] [PATCH 3/3] Unneeded NULL check
"Dr. David Alan Gilbert (git)" wrote: > From: "Dr. David Alan Gilbert" > > The check is unneccesary, we read the value at the start of the > thread, use it, and never change it. The value is checked to be > non-NULL before thread creation. > > Spotted by coverity, CID 1339211 > > Signed-off-by: Dr. David Alan Gilbert Reviewed-by: Juan Quintela
Re: [Qemu-devel] [PATCH 2/3] migration: Dead assignment of current_time
"Dr. David Alan Gilbert (git)" wrote: > From: "Dr. David Alan Gilbert" > > I set current_time before the postcopy test but never use it; > (I think this was from the original version where it was time based). > Spotted by coverity, CID 1339208 > > Signed-off-by: Dr. David Alan Gilbert Reviewed-by: Juan Quintela
Re: [Qemu-devel] [PATCH v12 19/36] blkdebug: Merge hand-rolled and qapi BlkdebugEvent enum
Am 18.11.2015 um 11:30 hat Markus Armbruster geschrieben:
> Eric Blake writes:
>
> > No need to keep two separate enums, where editing one is likely
> > to forget the other. Now that we can specify a qapi enum prefix,
> > we don't even have to change the bulk of the uses.
> >
> > get_event_by_name() could perhaps be replaced by qapi_enum_parse(),
> > but I left that for another day.
> >
> > CC: Kevin Wolf
> > Signed-off-by: Eric Blake
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index a07b13f..603eb60 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -1776,8 +1776,10 @@
> > # @BlkdebugEvent
> > #
> > # Trigger events supported by blkdebug.
> > +#
> > +# Since: 2.0
> > ##
> > -{ 'enum': 'BlkdebugEvent',
> > +{ 'enum': 'BlkdebugEvent', 'prefix': 'BLKDBG',
> >'data': [ 'l1_update', 'l1_grow.alloc_table', 'l1_grow.write_table',
> > 'l1_grow.activate_table', 'l2_load', 'l2_update',
> > 'l2_update_compressed', 'l2_alloc.cow_read', 'l2_alloc.write',
>
> I'm no friend of the 'prefix' feature. You could avoid it here by
> renaming BlkdebugEvent to Blkdbg. No additional churn, because you
> already replace hand-written BlkDebugEvent by generated BlkdebugEvent.
>
> Alternatively, you could reduce churn by renaming it to BlkDebugEvent.
>
> Matter of taste. Perhaps Kevin has a preference.
Wouldn't that mean that we end up with a C type called Blkdbg? In my
opinion that's a bit unspecific, so if you ask me, I would paint my
bikeshed in a different colour.
Kevin
Re: [Qemu-devel] [PULL 0/2] X86 fixes, 2015-11-17
On 17 November 2015 at 19:11, Eduardo Habkost wrote: > The following changes since commit 9be060f5278dc0d732ebfcf2bf0a293f88b833eb: > > Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' > into staging (2015-11-17 11:33:38 +) > > are available in the git repository at: > > git://github.com/ehabkost/qemu.git tags/x86-pull-request > > for you to fetch changes up to 33b5e8c03ae7a62d320d3c5c1104fe297d5c300d: > > target-i386: Disable rdtscp on Opteron_G* CPU models (2015-11-17 17:05:59 > -0200) > > > X86 fixes, 2015-11-17 > > Two X86 fixes, hopefully in time for -rc1. > > > > Eduardo Habkost (1): > target-i386: Disable rdtscp on Opteron_G* CPU models > > Richard Henderson (1): > target-i386: Fix mulx for identical target regs Applied, thanks. -- PMM
[Qemu-devel] KVM call for November 22th
Hi Please, send any topic that you are interested in covering. At the end of Monday I will send an email with the agenda or the cancellation of the call, so hurry up. After discussions on the QEMU Summit, we are going to have always open a KVM call where you can add topics. Call details: By popular demand, a google calendar public entry with it https://www.google.com/calendar/embed?src=dG9iMXRqcXAzN3Y4ZXZwNzRoMHE4a3BqcXNAZ3JvdXAuY2FsZW5kYXIuZ29vZ2xlLmNvbQ (Let me know if you have any problems with the calendar entry. I just gave up about getting right at the same time CEST, CET, EDT and DST). If you need phone number details, contact me privately Thanks, Juan.
Re: [Qemu-devel] [PATCH 44/77] pci-bridge: Set a supported devfn_min for bridge
On 11/11/2015 01:27, Benjamin Herrenschmidt wrote:
> if (bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_SHPC_REQ)) {
> +/* SHCP gets upset if we try to use slot 0 */
> +br->sec_bus.devfn_min = PCI_FUNC_MAX;
> dev->config[PCI_INTERRUPT_PIN] = 0x1;
> memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar",
> shpc_bar_size(dev));
This needs backwards compatibility gunk unfortunately. However we
should fix it in 2.5 because it's a bug. I'll send a patch.
Paolo
Re: [Qemu-devel] [PATCH 45/77] qdev: Add a hook for a bus to device if it can add devices
On 11/11/2015 01:27, Benjamin Herrenschmidt wrote:
> This allows a bus class to tell whether a given bus has room for
> any new device. max_dev isn't sufficient as the rules can depend
> on some arguments or can differ between instances of a bus. This
> will be used by PCI in subsequent patches
>
> Signed-off-by: Benjamin Herrenschmidt
> ---
> include/hw/qdev-core.h | 1 +
> qdev-monitor.c | 13 -
> 2 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 8057aed..6f3dd8d 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -202,6 +202,7 @@ struct BusClass {
> */
> char *(*get_fw_dev_path)(DeviceState *dev);
> void (*reset)(BusState *bus);
> +bool (*can_add_device)(BusState *bus, QemuOpts *opts);
> BusRealize realize;
> BusUnrealize unrealize;
>
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index a35098f..4023357 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -384,7 +384,7 @@ static inline bool qbus_is_full(BusState *bus)
> * Return the bus if found, else %NULL.
> */
> static BusState *qbus_find_recursive(BusState *bus, const char *name,
> - const char *bus_typename)
> + const char *bus_typename, QemuOpts
> *opts)
> {
> BusChild *kid;
> BusState *pick, *child, *ret;
> @@ -398,7 +398,10 @@ static BusState *qbus_find_recursive(BusState *bus,
> const char *name,
> }
>
> if (match && !qbus_is_full(bus)) {
> -return bus; /* root matches and isn't full */
> +BusClass *bc = BUS_GET_CLASS(bus);
> +if (!bc->can_add_device || bc->can_add_device(bus, opts)) {
> +return bus; /* root matches and isn't full */
> + }
> }
>
> pick = match ? bus : NULL;
> @@ -406,7 +409,7 @@ static BusState *qbus_find_recursive(BusState *bus, const
> char *name,
> QTAILQ_FOREACH(kid, &bus->children, sibling) {
> DeviceState *dev = kid->child;
> QLIST_FOREACH(child, &dev->child_bus, sibling) {
> -ret = qbus_find_recursive(child, name, bus_typename);
> + ret = qbus_find_recursive(child, name, bus_typename, opts);
Tabs for indentation. There are other occurrences in the patch.
Apart from this,
Reviewed-by: Paolo Bonzini
Acked-by: Paolo Bonzini
> if (ret && !qbus_is_full(ret)) {
> return ret; /* a descendant matches and isn't full */
> }
> @@ -436,7 +439,7 @@ static BusState *qbus_find(const char *path, Error **errp)
> assert(!path[0]);
> elem[0] = len = 0;
> }
> -bus = qbus_find_recursive(sysbus_get_default(), elem, NULL);
> +bus = qbus_find_recursive(sysbus_get_default(), elem, NULL, NULL);
> if (!bus) {
> error_setg(errp, "Bus '%s' not found", elem);
> return NULL;
> @@ -542,7 +545,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
> return NULL;
> }
> } else if (dc->bus_type != NULL) {
> -bus = qbus_find_recursive(sysbus_get_default(), NULL, dc->bus_type);
> + bus = qbus_find_recursive(sysbus_get_default(), NULL, dc->bus_type,
> opts);
> if (!bus || qbus_is_full(bus)) {
> error_setg(errp, "No '%s' bus found for device '%s'",
> dc->bus_type, driver);
>
Re: [Qemu-devel] [PATCH 46/77] pci: Use the new pci_can_add_device() to enforce devfn_min/max
On 11/11/2015 01:27, Benjamin Herrenschmidt wrote:
> This adds a devfn_max field to PCIBus and adds a pci_can_add_device()
> function which, if no "addr" (aka devfn) is specified, will tell whether
> there is any slot free between devfn_min and devfn_max.
>
> This will be used by some PCI root complex implementations that support
> only one direct child to avoid having qemu put dumb devices at different
> slot numbers.
>
> Signed-off-by: Benjamin Herrenschmidt
CCing maintainer.
Paolo
> ---
> hw/pci/pci.c | 22 ++
> include/hw/pci/pci_bus.h | 1 +
> 2 files changed, 23 insertions(+)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 168b9cc..7003f7c 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -108,6 +108,27 @@ static uint16_t pcibus_numa_node(PCIBus *bus)
> return NUMA_NODE_UNASSIGNED;
> }
>
> +static bool pci_can_add_device(BusState *bus, QemuOpts *opts)
> +{
> +unsigned int devfn, max;
> +PCIBus *pbus = PCI_BUS(bus);
> +
> +/* If address is specified, say yes and let it fail if that doesn't work
> */
> +if (qemu_opt_get(opts, "addr") != NULL) {
> +return true;
> +}
> +max = ARRAY_SIZE(pbus->devices);
> +if (pbus->devfn_max && pbus->devfn_max < max) {
> + max = pbus->devfn_max;
> +}
> +for (devfn = pbus->devfn_min ; devfn < max; devfn += PCI_FUNC_MAX) {
> +if (!pbus->devices[devfn]) {
> +break;
> +}
> +}
> +return devfn < max;
> +}
> +
> static void pci_bus_class_init(ObjectClass *klass, void *data)
> {
> BusClass *k = BUS_CLASS(klass);
> @@ -119,6 +140,7 @@ static void pci_bus_class_init(ObjectClass *klass, void
> *data)
> k->realize = pci_bus_realize;
> k->unrealize = pci_bus_unrealize;
> k->reset = pcibus_reset;
> +k->can_add_device = pci_can_add_device;
>
> pbc->is_root = pcibus_is_root;
> pbc->bus_num = pcibus_num;
> diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> index 403fec6..02055d4 100644
> --- a/include/hw/pci/pci_bus.h
> +++ b/include/hw/pci/pci_bus.h
> @@ -23,6 +23,7 @@ struct PCIBus {
> PCIIOMMUFunc iommu_fn;
> void *iommu_opaque;
> uint8_t devfn_min;
> +uint8_t devfn_max;
> pci_set_irq_fn set_irq;
> pci_map_irq_fn map_irq;
> pci_route_irq_fn route_intx_to_irq;
>
Re: [Qemu-devel] [PATCH for-2.5 44/77] pci-bridge: Set a supported devfn_min for bridge
On 18/11/2015 13:31, Paolo Bonzini wrote:
>
>
> On 11/11/2015 01:27, Benjamin Herrenschmidt wrote:
>> if (bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_SHPC_REQ)) {
>> +/* SHCP gets upset if we try to use slot 0 */
>> +br->sec_bus.devfn_min = PCI_FUNC_MAX;
>> dev->config[PCI_INTERRUPT_PIN] = 0x1;
>> memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar",
>> shpc_bar_size(dev));
>
> This needs backwards compatibility gunk unfortunately. However we
> should fix it in 2.5 because it's a bug. I'll send a patch.
Actually it turns out that the forbidden configuration is blocked elsewhere:
$ x86_64-softmmu/qemu-system-x86_64 \
-device pci-bridge,id=foo,chassis_nr=1 \
-device virtio-scsi-pci,bus=foo
qemu-system-x86_64: -device virtio-scsi-pci,bus=foo: Unsupported PCI
slot 0 for standard hotplug controller. Valid slots are between 1 and 31.
so this patch is just allowing the above command line to work. There's
no effect with or without the patch if addr=0, so the patch is good for
2.5 IMO.
Michael, can you queue it?
Paolo
Re: [Qemu-devel] [PULL 0/6] Ide patches
On 17 November 2015 at 20:12, John Snow wrote: > The following changes since commit c27e9014d56fa4880e7d741275d887c3a5949997: > > Merge remote-tracking branch 'remotes/kraxel/tags/pull-vnc-20151116-1' into > staging (2015-11-17 12:34:07 +) > > are available in the git repository at: > > https://github.com/jnsnow/qemu.git tags/ide-pull-request > > for you to fetch changes up to d66a8fa83b00b3b3d631a0e28cdce8c9b5698822: > > ide: enable buffered requests for PIO read requests (2015-11-17 15:06:39 > -0500) > > > > Applied, thanks. -- PMM
Re: [Qemu-devel] [PATCH v4 4/4] devicetree: update documentation for fw_cfg ARM bindings
On 17/11/2015 23:14, Rob Herring wrote: > On Mon, Nov 16, 2015 at 2:38 AM, Paolo Bonzini wrote: >> >> >> On 15/11/2015 03:07, Rob Herring wrote: >>> We generally don't want DT docs to depend on other kernel documentation. >> >> DT docs do not contain a copy of the data sheets, either. There is no >> reason to say how to use the device (and even then, only doing so >> partially) in the DT docs. > > The difference is datasheets apply to all OS's, kernel documentation > does not. In theory at least this could be used for other OS's, right? Would be nice indeed, as it's part of their intended purpose. For now we have to shoehorn things into linux-only stuff (like initrd) because well, nobody cares enough about NetBSD to compile U-Boot with its internal API, so let alone adding custom Haiku code. And of course, for things linux doesn't care about (like framebuffer description) then we're stuck trying to guess where it's at and writing drivers for our bootloader. So if at least people were considering they aren't the only users of this, that'd make life better for everyone. > Perhaps QEMU is the right place to thoroughly describe this and DT and > sysfs docs can refer to it. The brilliant idea of FDT was that we could have a canonical source and blob for it where people could send patches, but of course Linux and BSD freaks disagreed, so you now find Linux-flavoured DTs for rPi and other things, as well as BSD versions. Please, at least get the binding documentation for this unique and usable for everyone! François.
Re: [Qemu-devel] [PATCH v12 22/36] qapi: Don't let implicit enum MAX member collide
Eric Blake writes:
> Now that we guarantee the user doesn't have any enum values
> beginning with a single underscore, we can use that for our
> own purposes. Renaming ENUM_MAX to ENUM__MAX makes it obvious
> that the sentinel is generated.
The ENUM__MAX are mildly ugly. If we cared, we could get rid of most or
all uses. Right now, I don't.
Hmm, perhaps these ENUM__MAX are your motivation for rejecting adjacent
[-_] in the previous patch, to catch clashes like (foolishly named) type
'FOO--MAX' with enumeration type 'Foo'. I acknowledge the clash.
However, there are many more clashes we don't attempt to catch,
e.g. type 'Foo-lookup' with enumeration type 'Foo'. Perhaps we want to
build a real fence later, but right now I don't want us to ram in more
scattered fence laths.
Might want to say something like "Rejecting enum members named 'MAX' is
now useless, and will be dropped in the next patch".
> This patch was mostly generated by applying a temporary patch:
>
> |diff --git a/scripts/qapi.py b/scripts/qapi.py
> |index e6d014b..b862ec9 100644
> |--- a/scripts/qapi.py
> |+++ b/scripts/qapi.py
> |@@ -1570,6 +1570,7 @@ const char *const %(c_name)s_lookup[] = {
> | max_index = c_enum_const(name, 'MAX', prefix)
> | ret += mcgen('''
> | [%(max_index)s] = NULL,
> |+// %(max_index)s
> | };
> | ''',
> |max_index=max_index)
>
> then running:
>
> $ cat qapi-{types,event}.c tests/test-qapi-types.c |
> sed -n 's,^// \(.*\)MAX,s|\1MAX|\1_MAX|g,p' > list
> $ git grep -l _MAX | xargs sed -i -f list
>
> The only things not generated are the changes in scripts/qapi.py.
>
> Signed-off-by: Eric Blake
Patch looks good.
Re: [Qemu-devel] [PATCH v2] target-mips: Fix exceptions while UX=0
On 17/11/15 17:13, James Hogan wrote:
> Commit 01f728857941 ("target-mips: Status.UX/SX/KX enable 32-bit address
> wrapping") added a new hflag MIPS_HFLAG_AWRAP, which indicates that
> 64-bit addressing is disallowed in the current mode, so hflag users
> don't need to worry about the complexities of working that out, for
> example checking both MIPS_HFLAG_KSU and MIPS_HFLAG_UX.
>
> However when exceptions are taken outside of exception level,
> mips_cpu_do_interrupt() manipulates the env->hflags directly rather than
> using compute_hflags() to update them, and this code wasn't updated
> accordingly. As a result, when UX is cleared, MIPS_HFLAG_AWRAP is set,
> but it doesn't get cleared on entry back into kernel mode due to an
> exception. Kernel mode then cannot access the 64-bit segments resulting
> in a nested exception loop. The same applies to errors and debug
> exceptions.
>
> Fix by updating mips_cpu_do_interrupt() to clear the MIPS_HFLAG_WRAP
> flag when necessary, according to compute_hflags().
>
> Fixes: 01f728857941 ("target-mips: Status.UX/SX/KX enable 32-bit...")
> Signed-off-by: James Hogan
> Cc: Leon Alrae
> Cc: Aurelien Jarno
> ---
> Changes in v2:
> - Add cases for debug exceptions and errors (Leon).
> ---
> target-mips/helper.c | 12
> 1 file changed, 12 insertions(+)
Applied to target-mips queue, thanks.
Leon
Re: [Qemu-devel] [PULL v2 for-2.5 0/3] qemu-ga patch queue for 2.5
On 17 November 2015 at 23:06, Michael Roth wrote: > The following changes since commit c27e9014d56fa4880e7d741275d887c3a5949997: > > Merge remote-tracking branch 'remotes/kraxel/tags/pull-vnc-20151116-1' into > staging (2015-11-17 12:34:07 +) > > are available in the git repository at: > > > git://github.com/mdroth/qemu.git tags/qga-pull-2015-11-13-v2-tag > > for you to fetch changes up to ab59e3ecb2c12fafa89f7bedca7d329a078f3870: > > makefile: fix w32 install target for qemu-ga (2015-11-17 16:32:27 -0600) > > > qemu-ga patch queue for 2.5 > > * fixes for guest-exec gspawn() usage: > - inherit default lookup path by default instead of > explicitly defining it as being empty. > - don't inherit default PATH when PATH/ENV are explicit > > v2: > > * added fix for w32 'make install' target > * added version check for new g_spawn() flag > > > Michael Roth (1): > makefile: fix w32 install target for qemu-ga > > Yuri Pudgorodskiy (2): > qga: fix for default env processing for guest-exec > qga: allow to lookup in PATH from the passed envp for guest-exec > > Makefile | 5 - > qga/commands.c | 5 - > 2 files changed, 8 insertions(+), 2 deletions(-) > Applied, thanks. -- PMM
Re: [Qemu-devel] [PATCH v12 26/36] qapi: Change munging of CamelCase enum values
Eric Blake writes: > When munging enum values, the fact that we were passing the entire > prefix + value through camel_to_upper() meant that enum values > spelled with CamelCase could be turned into CAMEL_CASE. However, > this provides a potential collision (both OneTwo and One-Two would > munge into ONE_TWO) for enum types, when the same two names are > valid side-by-side as qapi member names. By changing the generation > of enum constants to always be prefix + '_' + c_name(value, > False).upper(), and ensuring that there are no case collisions (in > the next patches), we no longer have to worry about names that > would be distinct as qapi members but collide as variant tag names, > without having to think about what munging the heuristics in > camel_to_upper() will actually perform on an enum value. > > Making the change will affect enums that did not follow coding > conventions, using 'CamelCase' rather than desired 'lower-case'. > > Thankfully, there are only two culprits: InputButton and ErrorClass. > We already tweaked ErrorClass to make it an alias of QapiErrorClass, > where only the alias needs changingrather than the whole tree. So s/changingrather/changing rather/ > the bulk of this change is modifying INPUT_BUTTON_WHEEL_UP to the > new INPUT_BUTTON_WHEELUP (and likewise for WHEELDOWN). That part > of this commit may later need reverting if we rename the enum > constants from 'WheelUp' to 'wheel-up' as part of moving > x-input-send-event to a stable interface; but at least we have > documentation bread crumbs in place to remind us (commit 513e7cd), > and it matches the fact that SDL constants are also spelled > SDL_BUTTON_WHEELUP. > > Suggested by: Markus Armbruster > Signed-off-by: Eric Blake > > --- > v12: rebase to simpler ErrorClass changes, document decisions on > InputButton The ugliness is now confined to just the definition of enum ErrorClass. Good.
