Re: [Qemu-devel] [PATCH RFC v2 0/2] MSI/MSIX: fix to catch and report errors
Ping. About cleanup: 1. When realize()/init() fail, PCI device will free the config space memory, so the necessary cleanup I can find until now is: MemoryRegion object. Maybe I missed something to cleanup, hope for comments. 2. certain instance: move the msi/msxi init func to the beginning of the realize()/init() func, to avoid cleanup work. @Markus: hope I didn`t miss anything you mentioned in last review;) On 12/15/2015 06:43 PM, Cao jin wrote: msi_init() & msix_init() are supporting functions for PCI devices. catch the errors they produced and report. V2 changelog: 1. Modify as per Markus`s review a. Try to cleanup on function fail, as possible as I can. b. For .init() function, use error_report_err() and return non-zero value. c. For .realize(), propagate the error. d. Special case: TYPE_ICH9_AHCI, it is a on-board device initialized with machine init, so don`t bother to cleanup on failure, as process will exit anyway. Cao jin (2): Add param Error** to msi_init() & modify the callers Add param Error** to msix_init() & modify the callers hw/audio/intel-hda.c | 10 - hw/block/nvme.c| 32 +- hw/ide/ich.c | 2 +- hw/misc/ivshmem.c | 7 ++- hw/net/rocker/rocker.c | 10 +++-- hw/net/vmxnet3.c | 39 +++-- hw/pci-bridge/ioh3420.c| 7 ++- hw/pci-bridge/pci_bridge_dev.c | 8 +++- hw/pci-bridge/xio3130_downstream.c | 8 +++- hw/pci-bridge/xio3130_upstream.c | 8 +++- hw/pci/msi.c | 18 ++-- hw/pci/msix.c | 20 ++--- hw/scsi/megasas.c | 35 +++ hw/scsi/vmw_pvscsi.c | 13 -- hw/usb/hcd-xhci.c | 88 +- hw/vfio/pci.c | 28 ++-- hw/virtio/virtio-bus.c | 3 ++ hw/virtio/virtio-pci.c | 64 +-- include/hw/pci/msi.h | 4 +- include/hw/pci/msix.h | 5 ++- 20 files changed, 264 insertions(+), 145 deletions(-) -- Yours Sincerely, Cao Jin
Re: [Qemu-devel] [PATCH v3 0/3] virtio-net/vhost-net: share cross-endian enablement
On Wed, 18 Nov 2015 22:46:55 +0100 Greg Kurz wrote: > On Wed, 18 Nov 2015 22:48:06 +0200 > "Michael S. Tsirkin" wrote: > > > On Wed, Nov 18, 2015 at 05:23:00PM +0100, Greg Kurz wrote: > > > Since QEMU 2.4.0, vhost-net uses the cross-endian support of TAP devices > > > to > > > fix vnet headers. In fact, virtio-net can do the same instead of hackily > > > patching headers in virtio_net_hdr_swap(). > > > > > > This series moves the enablement of cross-endian support from vhost-net to > > > virtio-net: both vhost and full emulation can now benefit from it. Of > > > course > > > we keep the current hack to fall back on when the backend doesn't support > > > cross-endian. > > > > > > Thanks! > > This is an optimization rather than a bugfix, right? > > As such I'd rather defer this until after 2.5. > > > > Of course. I'll ping or repost later. > Hi Michael, 2.5 is now released and this series still applies cleanly. Should I repost ? -- Greg > > > --- > > > > > > Greg Kurz (3): > > > virtio-net: use the backend cross-endian capabilities > > > Revert "vhost-net: tell tap backend about the vnet endianness" > > > virtio: drop the virtio_needs_swap() helper > > > > > > > > > hw/net/vhost_net.c| 33 +-- > > > hw/net/virtio-net.c | 40 > > > +++-- > > > include/hw/virtio/virtio-access.h |9 > > > include/hw/virtio/virtio-net.h|1 + > > > 4 files changed, 40 insertions(+), 43 deletions(-) > > > >
Re: [Qemu-devel] [PATCH 2/2] iotests: Don't mention bdrv_swap in comments
Am 17.12.2015 um 06:09 hat Fam Zheng geschrieben:
> Signed-off-by: Fam Zheng
> ---
> tests/qemu-iotests/094 | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/tests/qemu-iotests/094 b/tests/qemu-iotests/094
> index 27a2be2..d30c78d 100755
> --- a/tests/qemu-iotests/094
> +++ b/tests/qemu-iotests/094
> @@ -1,6 +1,6 @@
> #!/bin/bash
> #
> -# Test case for drive-mirror to NBD (especially bdrv_swap() on NBD BDS)
> +# Test case for drive-mirror to NBD
> #
> # Copyright (C) 2015 Red Hat, Inc.
> #
> @@ -50,8 +50,6 @@ _send_qemu_cmd $QEMU_HANDLE \
> "{'execute': 'qmp_capabilities'}" \
> 'return'
>
> -# 'format': 'nbd' is not actually "correct", but this is probably the only
> way
> -# to test bdrv_swap() on an NBD BDS
> _send_qemu_cmd $QEMU_HANDLE \
> "{'execute': 'drive-mirror',
>'arguments': {'device': 'src',
Just completely removing the comment doesn't seem right to me if we
leave the "bad" option around.
The test seems to be a regression test for what was fixed in commit
f53a829, i.e. a direct effect of bdrv_swap(). This effect can't exist
any more, so we would keep the test just for some additional coverage
for NBD. Do we still need 'format': 'nbd' (if so, with a comment why we
do that) or should we make it 'raw' now?
Kevin
Re: [Qemu-devel] [PATCH v5 5/6] xlnx-zynqmp: Connect the SPI devices
On 16 December 2015 at 23:24, Paolo Bonzini wrote: > > > On 16/12/2015 22:45, Alistair Francis wrote: >> + >> +/* Rename each SPI bus after the SPI device to allow the board >> + * to access all of the busses from the SoC. >> + */ >> +spi_bus = qdev_get_child_bus(DEVICE(&s->spi[i]), "spi0"); >> +snprintf(bus_name, 6, "spi%d", i); >> +qdev_bus_rename(spi_bus, bus_name); >> + >> +/* Add the SPI buses to the SoC child bus */ >> +/* FIXME: This causes the later buses to be duplicated in >> + * the SPI devices printout when running qtre. >> + */ >> +QLIST_INSERT_HEAD(&dev->child_bus, spi_bus, sibling); > > Isn't the SPI bus accessible with something similar to spi[0-5]/spi0, > even without this hack? > > In any case, I would prefer qdev_bus_rename to stay in xlnx-zynqmp.c. I disagree that it should be in xlnx-zynqmp.c. Either (a) qdev already provides some reasonable mechanism for SoC container like this to allow their users to get at buses provided by their child objects, in which case we should use it (b) qdev doesn't provide such a mechanism, in which case we need to provide one (either qdev_bus_rename or something else if you have a better idea) But we definitely shouldn't have the SoC container code messing around with the internals of the qdev objects. thanks -- PMM
Re: [Qemu-devel] [PATCH v3 03/11] fdc: add disk field
John Snow writes:
> This allows us to distinguish between the current disk type and the
> current drive type. The drive is what's reported to CMOS, the disk is
> whatever the pick_geometry function suspects has been inserted.
>
> The drive field maintains the exact same meaning as it did previously,
> however pick_geometry/fd_revalidate will be refactored to *never* update
> the drive field, considering it frozen in-place during an earlier
> initialization call.
>
> Before this patch, pick_geometry/fd_revalidate could only change the
> drive field when it was FDRIVE_DRV_NONE, which indicated that we had
> not yet reported our drive type to CMOS. After we "pick one," even
> though pick_geometry/fd_revalidate re-set drv->drive, it should always
> be the same as the value going into these calls, so it is effectively
> already static.
>
> As of this patch, disk and drive will always be the same, but that may
> not be true by the end of this series.
>
> Disk does not need to be migrated because it is not user-visible state
> nor is it currently used for any calculations. It is purely informative,
> and will be rebuilt automatically via fd_revalidate on the new host.
>
> Signed-off-by: John Snow
> ---
> hw/block/fdc.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 09bb63d..13fef23 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -133,7 +133,8 @@ typedef struct FDrive {
typedef struct FDrive {
> FDCtrl *fdctrl;
> BlockBackend *blk;
> /* Drive status */
> -FDriveType drive;
> +FDriveType drive; /* CMOS drive type */
> +FDriveType disk; /* Current disk type */
where
typedef enum FDriveType {
FDRIVE_DRV_144 = 0x00, /* 1.44 MB 3"5 drive */
FDRIVE_DRV_288 = 0x01, /* 2.88 MB 3"5 drive */
FDRIVE_DRV_120 = 0x02, /* 1.2 MB 5"25 drive */
FDRIVE_DRV_NONE = 0x03, /* No drive connected */
} FDriveType;
FDriveType makes obvious sense as drive type.
> uint8_t perpendicular;/* 2.88 MB access mode*/
If I understand @perpendicular correctly, it's just an extra hoop a
driver needs to jump through to actually access a 2.88M disk.
> /* Position */
> uint8_t head;
uint8_t track;
uint8_t sect;
/* Media */
Shouldn't @disk go here?
FDiskFlags flags;
uint8_t last_sect;/* Nb sector per track*/
uint8_t max_track;/* Nb of tracks */
uint16_t bps; /* Bytes per sector */
uint8_t ro; /* Is read-only */
uint8_t media_changed;/* Is media changed */
uint8_t media_rate; /* Data rate of medium*/
bool media_inserted; /* Is there a medium in the tray */
} FDrive;
A disk / medium is characterised by it's form factor, density /
FDriveRate, #sides, #tracks, #sectors per track, and a read-only bit
(ignoring a few details that don't interest us). Obviously, the drive
type restricts possible disk types.
What does @disk add over the media description we already have?
Aside: @bps appears to be write-only, and the value written looks odd.
> @@ -157,6 +158,7 @@ static void fd_init(FDrive *drv)
> drv->drive = FDRIVE_DRV_NONE;
> drv->perpendicular = 0;
> /* Disk */
> +drv->disk = FDRIVE_DRV_NONE;
> drv->last_sect = 0;
> drv->max_track = 0;
> }
> @@ -286,6 +288,7 @@ static void pick_geometry(FDrive *drv)
> drv->max_track = parse->max_track;
> drv->last_sect = parse->last_sect;
> drv->drive = parse->drive;
> +drv->disk = drv->media_inserted ? parse->drive : FDRIVE_DRV_NONE;
> drv->media_rate = parse->rate;
>
> if (drv->media_inserted) {
[Qemu-devel] [PULL 09/40] qapi: Factor out QAPISchemaObjectTypeMember.check_clash()
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 Message-Id: <1447836791-369-10-git-send-email-ebl...@redhat.com> --- 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] [PULL 07/40] qapi: Fix up commit 7618b91's clash sanity checking change
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
Message-Id: <1447836791-369-8-git-send-email-ebl...@redhat.com>
---
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] [PULL 06/40] qapi: Clean up after previous commit
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
Message-Id: <1447836791-369-7-git-send-email-ebl...@redhat.com>
---
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] [PULL 00/40] QAPI patches for 2015-12-17
The following changes since commit a8c40fa2d667e585382080db36ac44e216b37a1c: Update version for v2.5.0 release (2015-12-16 16:10:14 +) are available in the git repository at: git://repo.or.cz/qemu/armbru.git tags/pull-qapi-2015-12-17 for you to fetch changes up to bac5429ccb4f41d421ec641b11f1852c8420fdb7: qapi: Detect base class loops (2015-12-17 08:21:29 +0100) QAPI patches for 2015-12-17 Eric Blake (33): qapi: Track simple union tag in object.local_members qapi-types: Consolidate gen_struct() and gen_union() qapi-types: Simplify gen_struct_field[s] qapi: Check for QAPI collisions involving variant members qapi: Factor out QAPISchemaObjectType.check_clash() qapi: Hoist tag collision check to Variants.check() qapi: Remove outdated tests related to QMP/branch collisions qapi: Track owner of each object member qapi: Detect collisions in C member names qapi: Fix c_name() munging qapi: Remove dead visitor code blkdebug: Merge hand-rolled and qapi BlkdebugEvent enum blkdebug: Avoid '.' in enum values qapi: Tighten the regex on valid names qapi: Don't let implicit enum MAX member collide qapi: Remove obsolete tests for MAX collision cpu: Convert CpuInfo into flat union qapi: Add alias for ErrorClass qapi: Change munging of CamelCase enum values qobject: Simplify QObject qobject: Rename qtype_code to QType qapi: Convert QType into QAPI built-in enum type qapi: Simplify visiting of alternate types qapi-types: Drop unnedeed ._fwdefn qapi: Inline _make_implicit_tag() qapi: Fix alternates that accept 'number' but not 'int' qapi: Simplify visits of optional fields qapi: Shorter visits of optional fields qapi: Prepare new QAPISchemaMember base class qapi: Track enum values by QAPISchemaMember, not string qapi: Enforce (or whitelist) case conventions on qapi members qapi: Move duplicate collision checks to schema check() qapi: Detect base class loops Markus Armbruster (7): qapi: Drop obsolete tag value collision assertions qapi: Simplify QAPISchemaObjectTypeMember.check() qapi: Clean up after previous commit qapi: Fix up commit 7618b91's clash sanity checking change qapi: Eliminate QAPISchemaObjectType.check() variable members qapi: Factor out QAPISchemaObjectTypeMember.check_clash() qapi: Simplify QAPISchemaObjectTypeVariants.check() block.c| 2 +- block/blkdebug.c | 79 +- block/parallels.c | 4 +- block/qapi.c | 4 +- block/qcow2.c | 2 +- block/quorum.c | 2 +- block/raw-posix.c | 2 +- blockdev.c | 2 +- cpus.c | 31 ++- docs/blkdebug.txt | 7 +- docs/qapi-code-gen.txt | 30 +-- hmp.c | 44 +-- hw/char/escc.c | 2 +- hw/i386/pc_piix.c | 2 +- hw/i386/pc_q35.c | 2 +- hw/input/hid.c | 6 +- hw/input/ps2.c | 6 +- hw/input/virtio-input-hid.c| 12 +- include/block/block.h | 62 + include/block/block_int.h | 2 +- include/hw/qdev-core.h | 2 +- include/migration/migration.h | 4 +- include/qapi/error.h | 14 + include/qapi/qmp/qbool.h | 1 + include/qapi/qmp/qdict.h | 1 + include/qapi/qmp/qfloat.h | 1 + include/qapi/qmp/qint.h| 1 + include/qapi/qmp/qlist.h | 1 + include/qapi/qmp/qobject.h | 50 ++-- include/qapi/qmp/qstring.h | 1 + include/qapi/visitor-impl.h| 8 +- include/qapi/visitor.h | 22 +- include/qemu/typedefs.h| 1 + migration/migration.c | 4 +- monitor.c | 18 +- net/net.c | 4 +- qapi-schema.json | 120 +++-- qapi/block-core.json | 20 +- qapi/common.json
[Qemu-devel] [PULL 08/40] qapi: Eliminate QAPISchemaObjectType.check() variable members
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
Message-Id: <1447836791-369-9-git-send-email-ebl...@redhat.com>
---
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] [PULL 12/40] qapi: Factor out QAPISchemaObjectType.check_clash()
From: Eric Blake 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 Message-Id: <1447836791-369-13-git-send-email-ebl...@redhat.com> Signed-off-by: Markus Armbruster --- 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] [PULL 04/40] qapi: Drop obsolete tag value collision assertions
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
Message-Id: <1447836791-369-5-git-send-email-ebl...@redhat.com>
---
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] [PULL 02/40] qapi-types: Consolidate gen_struct() and gen_union()
From: Eric Blake
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
Message-Id: <1447836791-369-3-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster
---
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] [PULL 19/40] blkdebug: Merge hand-rolled and qapi BlkdebugEvent enum
From: Eric Blake
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
Message-Id: <1447836791-369-20-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster
---
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 dee3a0e..6bcb7fa 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -36,7 +36,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;
@@ -64,7 +64,7 @@ enum {
};
typedef struct BlkdebugRule {
-BlkDebugEvent event;
+BlkdebugEvent event;
int action;
int state;
union {
@@ -143,69 +143,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
[Qemu-devel] [PULL 13/40] qapi: Hoist tag collision check to Variants.check()
From: Eric Blake 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 Message-Id: <1447836791-369-14-git-send-email-ebl...@redhat.com> Signed-off-by: Markus Armbruster --- 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] [PULL 03/40] qapi-types: Simplify gen_struct_field[s]
From: Eric Blake
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
Message-Id: <1447836791-369-4-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster
---
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] [PULL 01/40] qapi: Track simple union tag in object.local_members
From: Eric Blake
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
Message-Id: <1447836791-369-2-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster
---
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]
self._def_entity(
-QAPI
[Qemu-devel] [PULL 18/40] qapi: Remove dead visitor code
From: Eric Blake Commit cbc95538 removed unused start_handle() and end_handle(), but forgot to remove their declarations. Signed-off-by: Eric Blake Message-Id: <1447836791-369-19-git-send-email-ebl...@redhat.com> Signed-off-by: Markus Armbruster --- 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] [PULL 10/40] qapi: Simplify QAPISchemaObjectTypeVariants.check()
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 Message-Id: <1447836791-369-11-git-send-email-ebl...@redhat.com> --- 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] [PULL 24/40] cpu: Convert CpuInfo into flat union
From: Eric Blake
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
Message-Id: <1447836791-369-25-git-send-email-ebl...@redhat.com>
[Spelling of CPUInfo{SPARC,PPC,MIPS} fixed]
Signed-off-by: Markus Armbruster
---
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 43676fa..ea29584 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1558,22 +1558,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] [PULL 14/40] qapi: Remove outdated tests related to QMP/branch collisions
From: Eric Blake
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
Message-Id: <1447836791-369-15-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster
---
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 a1d03b4..6518ef9 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -281,9 +281,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
@@ -345,7 +343,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.exit
b/tests/qapi-schema/flat-union-clash-branch.exit
deleted fil
[Qemu-devel] [PULL 05/40] qapi: Simplify QAPISchemaObjectTypeMember.check()
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
Message-Id: <1447836791-369-6-git-send-email-ebl...@redhat.com>
---
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] [PULL 25/40] qapi: Add alias for ErrorClass
From: Eric Blake
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
Message-Id: <1447836791-369-26-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster
---
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 4d42cdc..3060b0c 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -91,6 +91,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 5546f8a..289c118 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] [PULL 15/40] qapi: Track owner of each object member
From: Eric Blake
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
Message-Id: <1447836791-369-16-git-send-email-ebl...@redhat.com>
[Incorrect & unused -wrapper case in _pretty_owner() dropped]
Signed-off-by: Markus Armbruster
---
scripts/qapi.py | 40 ++--
1 file changed, 38 insertions(+), 2 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 6fc14be..77d3e0a 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,23 @@ 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')
+# Unreachable and not implemented
+assert False
+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):
@@ -1047,6 +1074,10 @@ class QAPISchemaObjectTypeVariants(object):
self.tag_member = tag_member
self.variants = variants
+def set_owner(self, name):
+for v in self.variants:
+v.set_owner(name)
+
def check(self, schema, seen):
if not self.tag_member:# flat union
self.tag_member = seen[self.tag_name]
@@ -1066,6 +1097,8 @@ class QAPISchemaObjectTypeVariants(object):
class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
+role = 'branch'
+
def __init__(self, name, typ):
QAPISchemaObjectTypeMember.__init__(self, name,
[Qemu-devel] [PULL 21/40] qapi: Tighten the regex on valid names
From: Eric Blake
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.
The tighter regex doesn't forbid everything insane that a user
could provide (for example, a user could name a type 'Foo-lookup'
to collide with the generated 'Foo_lookup[]' for an enum 'Foo'),
but does a good job at protecting the most obvious uses, and
also happens to reserve single leading underscore for later use.
The handling of enum values starting with a digit is tricky:
commit 9fb081e introduced a subtle bug by using c_name() on
a munged value, which would allow an enum to include the
member 'q-int' in spite of our reservation. Furthermore,
munging with a leading '_' would fail our tighter regex. So
fix it by only munging for leading digits (which are never
ticklish in c_name()) and by using a different prefix (I
picked 'D', although any letter should do).
Add new tests, reserved-member-underscore and reserved-enum-q,
to demonstrate the tighter checking.
Signed-off-by: Eric Blake
Message-Id: <1447836791-369-22-git-send-email-ebl...@redhat.com>
Message-Id: <1447883135-18020-1-git-send-email-ebl...@redhat.com>
[Eric's fixup squashed in]
Signed-off-by: Markus Armbruster
---
docs/qapi-code-gen.txt| 22 +++---
scripts/qapi.py | 12 +++-
tests/Makefile| 2 ++
tests/qapi-schema/reserved-enum-q.err | 1 +
tests/qapi-schema/reserved-enum-q.exit| 1 +
tests/qapi-schema/reserved-enum-q.json| 4
tests/qapi-schema/reserved-enum-q.out | 0
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
11 files changed, 32 insertions(+), 16 deletions(-)
create mode 100644 tests/qapi-schema/reserved-enum-q.err
create mode 100644 tests/qapi-schema/reserved-enum-q.exit
create mode 100644 tests/qapi-schema/reserved-enum-q.json
create mode 100644 tests/qapi-schema/reserved-enum-q.out
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..32cc68f 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -118,17 +118,17 @@ 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
-"__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.
+incompatibly in a future release. All names must begin with a letter,
+and contain only ASCII letters, digits, dash, and underscore. 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). 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 d2ce9b3..3e5caa8 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,
+#
[Qemu-devel] [PULL 36/40] qapi: Prepare new QAPISchemaMember base class
From: Eric Blake We want to share some clash detection code between enum values and object type members. To assist with that, split off part of QAPISchemaObjectTypeMember into a new base class QAPISchemaMember that tracks name, owner, and common clash detection code; while the former keeps the additional fields for type and optional flag. Signed-off-by: Eric Blake Message-Id: <1449033659-25497-11-git-send-email-ebl...@redhat.com> Signed-off-by: Markus Armbruster --- scripts/qapi.py | 29 + 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/scripts/qapi.py b/scripts/qapi.py index 58ecdf2..168463a 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -1018,28 +1018,18 @@ class QAPISchemaObjectType(QAPISchemaType): self.members, self.variants) -class QAPISchemaObjectTypeMember(object): +class QAPISchemaMember(object): role = 'member' -def __init__(self, name, typ, optional): +def __init__(self, name): assert isinstance(name, str) -assert isinstance(typ, str) -assert isinstance(optional, bool) self.name = name -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 - def check_clash(self, info, seen): cname = c_name(self.name) if cname in seen: @@ -1066,6 +1056,21 @@ class QAPISchemaObjectTypeMember(object): return "'%s' %s" % (self.name, self._pretty_owner()) +class QAPISchemaObjectTypeMember(QAPISchemaMember): +def __init__(self, name, typ, optional): +QAPISchemaMember.__init__(self, name) +assert isinstance(typ, str) +assert isinstance(optional, bool) +self._type_name = typ +self.type = None +self.optional = optional + +def check(self, schema): +assert self.owner +self.type = schema.lookup_type(self._type_name) +assert self.type + + class QAPISchemaObjectTypeVariants(object): def __init__(self, tag_name, tag_member, variants): # Flat unions pass tag_name but not tag_member. -- 2.4.3
[Qemu-devel] [PULL 28/40] qobject: Rename qtype_code to QType
From: Eric Blake
The name QType matches our CODING_STYLE conventions for type names
in CamelCase. It also matches the fact that we are already naming
all the enum members with a prefix of QTYPE, not QTYPE_CODE. And
doing the rename will also make it easier for the next patch to use
QAPI for providing the enum, which also wants CamelCase type names.
Signed-off-by: Eric Blake
Message-Id: <1449033659-25497-3-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster
---
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 267f147..c0e877e 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -588,7 +588,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]: ";
@@ -606,7 +606,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 c537969..abcdee8 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -239,7 +239,7 @@ struct Property {
PropertyInfo *info;
ptrdiff_toffset;
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 550ba40..c0f1e99 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -45,10 +45,10 @@ typedef enum {
QTYPE_QFLOAT,
QTYPE_QBOOL,
QTYPE_MAX,
-} qtype_code;
+} QType;
typedef struct QObject {
-qtype_code type;
+QType type;
size_t refcnt;
} QObject;
@@ -64,7 +64,7 @@ typedef 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)
{
assert(QTYPE_NONE < type && type < QTYPE_MAX);
obj->refcnt = 1;
@@ -100,7 +100,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(QTYPE_NONE < obj->type && obj->type < QTYPE_MAX);
return obj->type;
diff --git a/qobject/qdict.c b/qobject/qdict.c
index eeade15..19df837 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -177,8 +177,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 10fcfbc..b336fbd 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -33,7 +33,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] [PULL 17/40] qapi: Fix c_name() munging
From: Eric Blake
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 would fail 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
Message-Id: <1447836791-369-18-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster
---
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 4870326..d2ce9b3 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1482,10 +1482,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] [PULL 23/40] qapi: Remove obsolete tests for MAX collision
From: Eric Blake
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
Message-Id: <1447836791-369-24-git-send-email-ebl...@redhat.com>
[Commit message tweaked]
Signed-off-by: Markus Armbruster
---
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 9c541e0..6acef1f 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 2f6b234..0f4914c 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -267,14 +267,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
@@ -347,7 +345,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.o
[Qemu-devel] [PULL 31/40] qapi-types: Drop unnedeed ._fwdefn
From: Eric Blake
Previously, the generated code in qapi-types.c initialized all
enum lookup tables first, prior to any other definitions. But
there are no topological sorting requirements that mandate this
layout, so we can drop the QAPISchemaGenTypeVisitor._fwdefn
field and just generate all definitions in visitation order.
The generated code shows some churn due to reordering, but it
is still fairly straightforward to follow (all the deletions
occur in one hunk, and all the deleted lines are re-inserted
in the same order later in the same files, just spread across
multiple insertion points).
Suggested-by: Markus Armbruster
Signed-off-by: Eric Blake
Message-Id: <1449033659-25497-6-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster
---
scripts/qapi-types.py | 6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 84ec858..0d86269 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -168,21 +168,17 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
self.decl = None
self.defn = None
self._fwdecl = None
-self._fwdefn = None
self._btin = None
def visit_begin(self, schema):
self.decl = ''
self.defn = ''
self._fwdecl = ''
-self._fwdefn = ''
self._btin = guardstart('QAPI_TYPES_BUILTIN')
def visit_end(self):
self.decl = self._fwdecl + self.decl
self._fwdecl = None
-self.defn = self._fwdefn + self.defn
-self._fwdefn = None
# To avoid header dependency hell, we always generate
# declarations for built-in types in our header files and
# simply guard them. See also do_builtins (command line
@@ -209,7 +205,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
self.defn += gen_enum_lookup(name, values, prefix)
else:
self._fwdecl += gen_enum(name, values, prefix)
-self._fwdefn += gen_enum_lookup(name, values, prefix)
+self.defn += gen_enum_lookup(name, values, prefix)
def visit_array_type(self, name, info, element_type):
if isinstance(element_type, QAPISchemaBuiltinType):
--
2.4.3
[Qemu-devel] [PULL 40/40] qapi: Detect base class loops
From: Eric Blake
It should be fairly obvious that qapi base classes need to
form an acyclic graph, since QMP cannot specify the same
key more than once, while base classes are included as flat
members alongside other members added by the child. But the
old check_member_clash() parser function was not prepared to
check for this, and entered an infinite recursion (at least
until Python gives up, complaining about nesting too deep).
Now that check_member_clash() has been recently removed,
attempts at self-inheritance trigger an assertion failure
introduced by commit ac88219a. The obvious fix is to turn
the assertion into a conditional.
This patch includes both the tests (base-cycle-direct and
base-cycle-indirect) and the fix, since the .err file output
for the unfixed case is not useful (particularly when it was
warning about unbounded recursion, as that limit may be
platform-specific).
We don't need to worry about cycles in flat unions (neither
the base type nor the type of a variant can be a union) nor
in alternates (alternate branches cannot themselves be an
alternate). But if we later allow a union type as a variant,
we will still be okay, as QAPISchemaObjectTypeVariants.check()
triggers the same QAPISchemaObjectType.check() that will
detect any loops.
Likewise, we need not worry about the case of diamond
inheritance where the same class is used for a flat union base
class and one of its variants; either both uses will introduce
a collision in trying to insert the same member name twice, or
the shared type is empty and changes nothing.
Signed-off-by: Eric Blake
Message-Id: <1449033659-25497-16-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster
---
scripts/qapi.py| 4 +++-
tests/Makefile | 2 ++
tests/qapi-schema/base-cycle-direct.err| 1 +
tests/qapi-schema/base-cycle-direct.exit | 1 +
tests/qapi-schema/base-cycle-direct.json | 2 ++
tests/qapi-schema/base-cycle-direct.out| 0
tests/qapi-schema/base-cycle-indirect.err | 1 +
tests/qapi-schema/base-cycle-indirect.exit | 1 +
tests/qapi-schema/base-cycle-indirect.json | 3 +++
tests/qapi-schema/base-cycle-indirect.out | 0
10 files changed, 14 insertions(+), 1 deletion(-)
create mode 100644 tests/qapi-schema/base-cycle-direct.err
create mode 100644 tests/qapi-schema/base-cycle-direct.exit
create mode 100644 tests/qapi-schema/base-cycle-direct.json
create mode 100644 tests/qapi-schema/base-cycle-direct.out
create mode 100644 tests/qapi-schema/base-cycle-indirect.err
create mode 100644 tests/qapi-schema/base-cycle-indirect.exit
create mode 100644 tests/qapi-schema/base-cycle-indirect.json
create mode 100644 tests/qapi-schema/base-cycle-indirect.out
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 8edfd79..7dec611 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -940,7 +940,9 @@ class QAPISchemaObjectType(QAPISchemaType):
self.members = None
def check(self, schema):
-assert self.members is not False# not running in cycles
+if self.members is False: # check for cycles
+raise QAPIExprError(self.info,
+"Object %s contains itself" % self.name)
if self.members:
return
self.members = False# mark as being checked
diff --git a/tests/Makefile b/tests/Makefile
index 69cef77..053c1ae 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -257,6 +257,8 @@ qapi-schema += bad-ident.json
qapi-schema += bad-type-bool.json
qapi-schema += bad-type-dict.json
qapi-schema += bad-type-int.json
+qapi-schema += base-cycle-direct.json
+qapi-schema += base-cycle-indirect.json
qapi-schema += command-int.json
qapi-schema += comments.json
qapi-schema += double-data.json
diff --git a/tests/qapi-schema/base-cycle-direct.err
b/tests/qapi-schema/base-cycle-direct.err
new file mode 100644
index 000..9c68f65
--- /dev/null
+++ b/tests/qapi-schema/base-cycle-direct.err
@@ -0,0 +1 @@
+tests/qapi-schema/base-cycle-direct.json:2: Object Loopy contains itself
diff --git a/tests/qapi-schema/base-cycle-direct.exit
b/tests/qapi-schema/base-cycle-direct.exit
new file mode 100644
index 000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/base-cycle-direct.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/base-cycle-direct.json
b/tests/qapi-schema/base-cycle-direct.json
new file mode 100644
index 000..4fc66d0
--- /dev/null
+++ b/tests/qapi-schema/base-cycle-direct.json
@@ -0,0 +1,2 @@
+# we reject a loop in base classes
+{ 'struct': 'Loopy', 'base': 'Loopy', 'data': {} }
diff --git a/tests/qapi-schema/base-cycle-direct.out
b/tests/qapi-schema/base-cycle-direct.out
new file mode 100644
index 000..e69de29
diff --git a/tests/qapi-schema/base-cycle-indirect.err
b/tests/qapi-schema/base-cycle-indirect.err
new file mode 100644
index 000..fc92fe4
--- /dev/null
+++ b/tests/qapi-schema/base-cycle-indirect.err
@@ -0,0 +1 @@
+
[Qemu-devel] [PULL 11/40] qapi: Check for QAPI collisions involving variant members
From: Eric Blake
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 QAPISchemaObjectType.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
QAPISchemaObjectTypeVariants.check_clash().
Note that cloning 'seen' inside .check_clash() resembles
the one we just removed from .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
Message-Id: <1447836791-369-12-git-send-email-ebl...@redhat.com>
[Commit message tweaked]
Signed-off-by: Markus Armbruster
---
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] [PULL 34/40] qapi: Simplify visits of optional fields
From: Eric Blake
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;
|-}
|+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
Message-Id: <1449033659-25497-9-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster
---
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 7e6c396..8bf41db 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1656,15 +1656,11 @@ def gen_visit_fields(members, prefix='',
need_cast=False, skiperr=False):
for memb i
[Qemu-devel] [PULL 27/40] qobject: Simplify QObject
From: Eric Blake
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 the code element of the vtable QType
directly into QObject (renamed to type), and track a separate array
of destroy functions. We can drop qnull_destroy_obj() in the
process.
The remaining QObject subclasses must export their destructor.
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 still the same size (so no change in cache
line pressure), but now have less indirection (although I didn't
bother benchmarking to see if there is a noticeable speedup, as
we don't have hard evidence that this was in a performance hotspot
in the first place).
A future patch could drop the refcnt size to 32 bits for a smaller
struct on 64-bit architectures, if desired (we have limits on the
largest JSON that we are willing to parse, and will probably never
need to take full advantage of a 64-bit refcnt).
Suggested-by: Markus Armbruster
Signed-off-by: Eric Blake
Message-Id: <1449033659-25497-2-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster
---
include/qapi/qmp/qbool.h | 1 +
include/qapi/qmp/qdict.h | 1 +
include/qapi/qmp/qfloat.h | 1 +
include/qapi/qmp/qint.h| 1 +
include/qapi/qmp/qlist.h | 1 +
include/qapi/qmp/qobject.h | 31 +++
include/qapi/qmp/qstring.h | 1 +
qobject/Makefile.objs | 2 +-
qobject/qbool.c| 11 ++-
qobject/qdict.c| 11 ++-
qobject/qfloat.c | 11 ++-
qobject/qint.c | 11 ++-
qobject/qlist.c| 11 ++-
qobject/qnull.c| 12 +---
qobject/qobject.c | 34 ++
qobject/qstring.c | 11 ++-
16 files changed, 69 insertions(+), 82 deletions(-)
create mode 100644 qobject/qobject.c
diff --git a/include/qapi/qmp/qbool.h b/include/qapi/qmp/qbool.h
index d9256e4..836d078 100644
--- a/include/qapi/qmp/qbool.h
+++ b/include/qapi/qmp/qbool.h
@@ -25,5 +25,6 @@ typedef struct QBool {
QBool *qbool_from_bool(bool value);
bool qbool_get_bool(const QBool *qb);
QBool *qobject_to_qbool(const QObject *obj);
+void qbool_destroy_obj(QObject *obj);
#endif /* QBOOL_H */
diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index 787c658..6c2a0e5 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -48,6 +48,7 @@ void qdict_iter(const QDict *qdict,
void *opaque);
const QDictEntry *qdict_first(const QDict *qdict);
const QDictEntry *qdict_next(const QDict *qdict, const QDictEntry *entry);
+void qdict_destroy_obj(QObject *obj);
/* Helper to qdict_put_obj(), accepts any object */
#define qdict_put(qdict, key, obj) \
diff --git a/include/qapi/qmp/qfloat.h b/include/qapi/qmp/qfloat.h
index 46745e5..a8af2a8 100644
--- a/include/qapi/qmp/qfloat.h
+++ b/include/qapi/qmp/qfloat.h
@@ -25,5 +25,6 @@ typedef struct QFloat {
QFloat *qfloat_from_double(double value);
double qfloat_get_double(const QFloat *qi);
QFloat *qobject_to_qfloat(const QObject *obj);
+void qfloat_destroy_obj(QObject *obj);
#endif /* QFLOAT_H */
diff --git a/include/qapi/qmp/qint.h b/include/qapi/qmp/qint.h
index 339a9ab..049e528 100644
--- a/include/qapi/qmp/qint.h
+++ b/include/qapi/qmp/qint.h
@@ -24,5 +24,6 @@ typedef struct QInt {
QInt *qint_from_int(int64_t value);
int64_t qint_get_int(const QInt *qi);
QInt *qobject_to_qint(const QObject *obj);
+void qint_destroy_obj(QObject *obj);
#endif /* QINT_H */
diff --git a/include/qapi/qmp/qlist.h b/include/qapi/qmp/qlist.h
index b1bf785..a84117e 100644
--- a/include/qapi/qmp/qlist.h
+++ b/include/qapi/qmp/qlist.h
@@ -49,6 +49,7 @@ QObject *qlist_peek(QList *qlist);
int qlist_empty(const QList *qlist);
size_t qlist_size(const QList *qlist);
QList *qobject_to_qlist(const QObject *obj);
+void qlist_destroy_obj(QObject *obj);
static inline const QListEntry *qlist_first(const QList *qlist)
{
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index 4b96ed5..550ba40 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -47,15 +47,8 @@ typedef enum {
QTYPE_MAX,
} qtype_code;
-struct QObject;
-
-typedef struct QType {
-qtype_code code;
-void (*destroy)(struct QObject *);
-} QType;
-
typedef struct QObject {
-const QType *type;
+qtype_code type;
size_t refcnt;
} QObject;
@@ -71,9 +64,12 @@ 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
[Qemu-devel] [PULL 26/40] qapi: Change munging of CamelCase enum values
From: Eric Blake
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 changing rather 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
Message-Id: <1447836791-369-27-git-send-email-ebl...@redhat.com>
[Commit message tweaked]
Signed-off-by: Markus Armbruster
---
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 3060b0c..6285cf5 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -96,12 +96,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,
[Qemu-devel] [PULL 32/40] qapi: Inline _make_implicit_tag()
From: Eric Blake
Now that alternates no longer use an implicit tag, we can
inline _make_implicit_tag() into its one caller,
_def_union_type().
No change to generated code.
Suggested-by: Markus Armbruster
Signed-off-by: Eric Blake
Message-Id: <1449033659-25497-7-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster
---
scripts/qapi.py | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 2b46dd0..7e6c396 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1319,11 +1319,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']
@@ -1337,7 +1332,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] [PULL 16/40] qapi: Detect collisions in C member names
From: Eric Blake 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 Message-Id: <1447836791-369-17-git-send-email-ebl...@redhat.com> Signed-off-by: Markus Armbruster --- 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 77d3e0a..4870326 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 @@ -1080,7 +1086,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) @@ -1088,12 +1095,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/qapi-schema/args-name-clash.json b/tests/qapi-schema/args-name-clash.json index 9e8f889..61423cb 100644 --- a/tests/qapi-schema/args-name-clash.json +++ b/tests/qapi-schema/args-name-clash.json @@ -1,5 +1,4 @@ # C member name collision -# FIXME - This parses, but fai
[Qemu-devel] [PULL 38/40] qapi: Enforce (or whitelist) case conventions on qapi members
From: Eric Blake We document that members of enums and objects should be 'lower-case', although we were not enforcing it. We have to whitelist a few pre-existing entities that violate the norms. Add three new tests to expose the new error message, each of which first uses the whitelisted name 'UuidInfo' to prove the whitelist works, then triggers the failure (this is the same pattern used in the existing returns-whitelist.json test). Note that by adding this check, we have effectively forbidden an entity with a case-insensitive clash of member names, for any entity that is not on the whitelist (although there is still the possibility to clash via '-' vs. '_'). Not done here: a future patch should also add naming convention support and whitelist exceptions for command, event, and type names. The additions to QAPISchemaMember.check_clash() check whether info['name'] is in the whitelist (the top-most entity name at the point 'info' tracks), rather than self.owner (the type, possibly implicit, that directly owns the member), because it is easier to maintain the whitelist by the names actually in the user's .json file, rather than worrying about the names of implicit types. Signed-off-by: Eric Blake Message-Id: <1449033659-25497-14-git-send-email-ebl...@redhat.com> [Simplified a bit as per discussion with Eric] Signed-off-by: Markus Armbruster --- scripts/qapi.py | 17 + tests/Makefile | 3 +++ tests/qapi-schema/args-member-case.err | 1 + tests/qapi-schema/args-member-case.exit | 1 + tests/qapi-schema/args-member-case.json | 2 ++ tests/qapi-schema/args-member-case.out | 0 tests/qapi-schema/enum-member-case.err | 1 + tests/qapi-schema/enum-member-case.exit | 1 + tests/qapi-schema/enum-member-case.json | 3 +++ tests/qapi-schema/enum-member-case.out | 0 tests/qapi-schema/union-branch-case.err | 1 + tests/qapi-schema/union-branch-case.exit | 1 + tests/qapi-schema/union-branch-case.json | 2 ++ tests/qapi-schema/union-branch-case.out | 0 14 files changed, 33 insertions(+) create mode 100644 tests/qapi-schema/args-member-case.err create mode 100644 tests/qapi-schema/args-member-case.exit create mode 100644 tests/qapi-schema/args-member-case.json create mode 100644 tests/qapi-schema/args-member-case.out create mode 100644 tests/qapi-schema/enum-member-case.err create mode 100644 tests/qapi-schema/enum-member-case.exit create mode 100644 tests/qapi-schema/enum-member-case.json create mode 100644 tests/qapi-schema/enum-member-case.out create mode 100644 tests/qapi-schema/union-branch-case.err create mode 100644 tests/qapi-schema/union-branch-case.exit create mode 100644 tests/qapi-schema/union-branch-case.json create mode 100644 tests/qapi-schema/union-branch-case.out diff --git a/scripts/qapi.py b/scripts/qapi.py index 07e8303..74a561a 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -59,6 +59,20 @@ returns_whitelist = [ 'guest-sync-delimited', ] +# Whitelist of entities allowed to violate case conventions +case_whitelist = [ +# From QMP: +'ACPISlotType', # DIMM, visible through query-acpi-ospm-status +'CpuInfoBase', # CPU, visible through query-cpu +'CpuInfoMIPS', # PC, visible through query-cpu +'CpuInfoTricore', # PC, visible through query-cpu +'InputAxis',# TODO: drop when x-input-send-event is fixed +'InputButton', # TODO: drop when x-input-send-event is fixed +'QapiErrorClass', # all members, visible through errors +'UuidInfo', # UUID, visible through query-uuid +'X86CPURegister32', # all members, visible indirectly through qom-get +] + enum_types = [] struct_types = [] union_types = [] @@ -1038,6 +1052,9 @@ class QAPISchemaMember(object): def check_clash(self, info, seen): cname = c_name(self.name) +if cname.lower() != cname and self.owner not in case_whitelist: +raise QAPIExprError(info, +"%s should not use uppercase" % self.describe()) if cname in seen: raise QAPIExprError(info, "%s collides with %s" diff --git a/tests/Makefile b/tests/Makefile index 0f4914c..6b8b112 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -246,6 +246,7 @@ qapi-schema += args-array-unknown.json qapi-schema += args-int.json qapi-schema += args-invalid.json qapi-schema += args-member-array-bad.json +qapi-schema += args-member-case.json qapi-schema += args-member-unknown.json qapi-schema += args-name-clash.json qapi-schema += args-union.json @@ -267,6 +268,7 @@ 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-member-case.json qapi-schema += enum-missing-data.json qapi-schema += enum-wrong-data.json qapi-schema += escape-outside-st
[Qemu-devel] [PULL 39/40] qapi: Move duplicate collision checks to schema check()
From: Eric Blake
With the recent commit 'qapi: Detect collisions in C member
names', we have two different locations for detecting clashes -
one at parse time, and another at QAPISchema*.check() time.
Remove all of the ad hoc parser checks, and delete associated
code (for example, the global check_member_clash() method is
no longer needed).
Testing this showed that the test union-bad-branch wasn't adding
much: union-clash-branches also exposes the error message when
branches collide, and we've recently fixed things to avoid an
implicit collision with max. Likewise, the error for
enum-clash-member changes to report our new detection of
upper case in a value name, unless we modify the test to use
all lower case.
The wording of several error messages has changed, but the
change is generally an improvement rather than a regression.
No change to generated code.
Signed-off-by: Eric Blake
Message-Id: <1449033659-25497-15-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster
---
scripts/qapi.py | 51 +--
tests/Makefile| 1 -
tests/qapi-schema/alternate-clash.err | 2 +-
tests/qapi-schema/enum-clash-member.err | 2 +-
tests/qapi-schema/enum-clash-member.json | 2 +-
tests/qapi-schema/flat-union-clash-member.err | 2 +-
tests/qapi-schema/struct-base-clash-deep.err | 2 +-
tests/qapi-schema/struct-base-clash.err | 2 +-
tests/qapi-schema/union-bad-branch.err| 1 -
tests/qapi-schema/union-bad-branch.exit | 1 -
tests/qapi-schema/union-bad-branch.json | 8 -
tests/qapi-schema/union-bad-branch.out| 0
tests/qapi-schema/union-clash-branches.err| 2 +-
tests/qapi-schema/union-clash-branches.json | 2 +-
14 files changed, 9 insertions(+), 69 deletions(-)
delete mode 100644 tests/qapi-schema/union-bad-branch.err
delete mode 100644 tests/qapi-schema/union-bad-branch.exit
delete mode 100644 tests/qapi-schema/union-bad-branch.json
delete mode 100644 tests/qapi-schema/union-bad-branch.out
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 74a561a..8edfd79 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -519,21 +519,6 @@ def check_type(expr_info, source, value, allow_array=False,
'enum'])
-def check_member_clash(expr_info, base_name, data, source=""):
-base = find_struct(base_name)
-assert base
-base_members = base['data']
-for key in data.keys():
-if key.startswith('*'):
-key = key[1:]
-if key in base_members or "*" + key in base_members:
-raise QAPIExprError(expr_info,
-"Member name '%s'%s clashes with base '%s'"
-% (key, source, base_name))
-if base.get('base'):
-check_member_clash(expr_info, base['base'], data, source)
-
-
def check_command(expr, expr_info):
name = expr['command']
@@ -563,7 +548,6 @@ def check_union(expr, expr_info):
base = expr.get('base')
discriminator = expr.get('discriminator')
members = expr['data']
-values = {}
# Two types of unions, determined by discriminator.
@@ -610,15 +594,9 @@ def check_union(expr, expr_info):
for (key, value) in members.items():
check_name(expr_info, "Member of union '%s'" % name, key)
-# Each value must name a known type; furthermore, in flat unions,
-# branches must be a struct with no overlapping member names
+# Each value must name a known type
check_type(expr_info, "Member '%s' of union '%s'" % (key, name),
value, allow_array=not base, allow_metas=allow_metas)
-if base:
-branch_struct = find_struct(value)
-assert branch_struct
-check_member_clash(expr_info, base, branch_struct['data'],
- " of branch '%s'" % key)
# If the discriminator names an enum type, then all members
# of 'data' must also be members of the enum type.
@@ -629,34 +607,16 @@ def check_union(expr, expr_info):
"enum '%s'" %
(key, enum_define["enum_name"]))
-# Otherwise, check for conflicts in the generated enum
-else:
-c_key = camel_to_upper(key)
-if c_key in values:
-raise QAPIExprError(expr_info,
-"Union '%s' member '%s' clashes with '%s'"
-% (name, key, values[c_key]))
-values[c_key] = key
-
def check_alternate(expr, expr_info):
name = expr['alternate']
members = expr['data']
-values = {}
types_seen = {}
# Check every branch
for (key, value) in members.items():
check_name(expr_info, "Member of alternate '%s'" % name, key)
-# Check for conflicts in the branch names
-c_key = c_na
[Qemu-devel] [PULL 29/40] qapi: Convert QType into QAPI built-in enum type
From: Eric Blake
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
Message-Id: <1449033659-25497-4-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster
---
docs/qapi-code-gen.txt | 1 +
include/qapi/qmp/qobject.h | 21 +
include/qemu/typedefs.h | 1 +
qobject/qobject.c| 4 ++--
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 ++
20 files changed, 62 insertions(+), 24 deletions(-)
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 2becba9..79bf072 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -160,6 +160,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 c0f1e99..74459ae 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -34,23 +34,12 @@
#include
#include
+#include "qapi-types.h"
-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 {
+struct QObject {
QType type;
size_t refcnt;
-} QObject;
+};
/* Get the 'base' part of an object */
#define QOBJECT(obj) (&(obj)->base)
@@ -66,7 +55,7 @@ typedef struct QObject {
/* Initialize an object to default values */
static inline void qobject_init(QObject *obj, QType type)
{
-assert(QTYPE_NONE < type && type < QTYPE_MAX);
+assert(QTYPE_NONE < type && type < QTYPE__MAX);
obj->refcnt = 1;
obj->type = type;
}
@@ -102,7 +91,7 @@ static inline void qobject_decref(QObject *obj)
*/
static inline QType qobject_type(const QObject *obj)
{
-assert(QTYPE_NONE < obj->type && obj->type < QTYPE_MAX);
+assert(QTYPE_NONE < obj->type && obj->type < QTYPE__MAX);
return obj->type;
}
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 3eedcf4..78fe6e8 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -80,6 +80,7 @@ typedef struct QEMUSGList QEMUSGList;
typedef struct QEMUSizedBuffer QEMUSizedBuffer;
typedef struct QEMUTimer QEMUTimer;
typedef struct QEMUTimerListGroup QEMUTimerListGroup;
+typedef struct QObject QObject;
typedef struct RAMBlock RAMBlock;
typedef struct Range Range;
typedef struct SerialState SerialState;
diff --git a/qobject/qobject.c b/qobject/qobject.c
index 1df315a..a3ef14e 100644
--- a/qobject/qobject.c
+++ b/qobject/qobject.c
@@ -15,7 +15,7 @@
#include "qapi/qmp/qlist.h"
#include "qapi/qmp/qstring.h"
-static void (*qdestroy[QTYPE_MAX])(QObject *) = {
+static void (*qdestroy[QTYPE__MAX])(QObject *) = {
[QTYPE_NONE] = NULL,
[Qemu-devel] [PULL 35/40] qapi: Shorter visits of optional fields
From: Eric Blake
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
Message-Id: <1449033659-25497-10-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster
---
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 8bf41db..58ecdf2 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1656,8 +1656,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] [PULL 20/40] blkdebug: Avoid '.' in enum values
From: Eric Blake
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
Message-Id: <1447836791-369-21-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster
---
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 18ba6a6..1a5d9ce 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 failed: Input/output error
No errors were found on the image.
For
[Qemu-devel] [PULL 22/40] qapi: Don't let implicit enum MAX member collide
From: Eric Blake
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.
Rejecting enum members named 'MAX' is now useless, and will be dropped
in the next patch.
Signed-off-by: Eric Blake
Message-Id: <1447836791-369-23-git-send-email-ebl...@redhat.com>
Reviewed-by: Juan Quintela
[Rebased to current master, commit message tweaked]
Signed-off-by: Markus Armbruster
---
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 6bcb7fa..59c61eb 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -36,7 +36,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;
@@ -147,7 +147,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;
@@ -507,7 +507,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);
}
@@ -576,7 +576,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;
@@ -633,7 +633,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 f689fde..e4a56a5 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 c
Re: [Qemu-devel] [PATCH] rtc: introduce nmi disable bit handler for cmos
On 17/12/2015 08:17, Gonglei (Arei) wrote:
>> On 16/12/2015 11:28, Gonglei (Arei) wrote:
>>> I'll move the global nmi_disabled into RTCState, then I have to add a global
>> RTCState
>>> Variable so that other C files can use the rtc_state->external_nmi_disabled.
>>
>> Hmm, I think it should be done differently. This is a layering
>> violation, the NMI_EN is essentially a pin (qemu_irq) between the ISA
>> bridges and the RTC. The NMI "button" is also a component of the ISA
>
> So, you mean the NMI_EN can only control NMI injection came from ISA bridge?
> What's this NMI "button" mean?
The NMI command in the monitor is a "virtual NMI button".
>> bridge; you should not need to touch anything except the RTC and the ISA
>> bridges, in particular not the APICs.
>>
> Currently, the qmp command "inject-nmi" doesn't pass ISA bridge. How
> do we address this situation?
That's step two below: make the ISA bridges implement NMIState.
>> First, you need to add a qemu_irq argument to rtc_init. The RTC can
>> raise/lower the IRQ on writes to port 0x70.
>>
>> Second, make the ISA bridges implement NMIState, where the
>> implementation of NMIState is similar to inject_nmi in hw/core/nmi.c:
>>
>> CPU_FOREACH(cs) {
>> X86CPU *cpu = X86_CPU(cs);
>>
>> if (!cpu->apic_state) {
>> cpu_interrupt(cs, CPU_INTERRUPT_NMI);
>> } else {
>> apic_deliver_nmi(cpu->apic_state);
>> }
>> }
>>
>> Third, the ISA bridges (hw/isa/piix4.c and hw/isa/lpc_ich9.c) need to
>
> We don't use hw/isa/piix4.c but hw/pci-host/piix.c in x86 target. Right?
Right, I said I had certainly messed up something. :)
>> export a qemu_irq for nmi_en IRQ (e.g. using qdev_init_gpio_in_named),
>> and you should modify the ISA bridge's implementation of NMIState to
>> latch the NMI if you send one while NMIs are disabled. The nmi_en IRQ
>> can also trigger an NMI when nmi_en is enabled and an NMI was latched.
>
> Sorry, I'm a bit confused. The nmi_en can trigger an NMI? Isn't a flag bit
> which
> can enable/disable the NMI switch?
Suppose an NMI was injected with the monitor while nmi_en was disabled.
The NMI is then latched, and triggered when you enable NMIs again with
nmi_en.
Thanks,
Paolo
[Qemu-devel] [PULL 37/40] qapi: Track enum values by QAPISchemaMember, not string
From: Eric Blake
Rather than using just an array of strings, make enum.values be
an array of the new QAPISchemaMember type, and add a helper
member_names() method to get back at the original list of names.
Likewise, creating an enum requires wrapping strings, via a new
QAPISchema._make_enum_members() method. The benefit of wrapping
enum members in a QAPISchemaMember Python object is that we now
share the existing code for C name clash detection (although the
code is not yet active until a later commit removes the earlier
ad hoc parser checks).
In a related change, the QAPISchemaMember._pretty_owner() method
needs to learn about one more implicit type name: the generated
enum associated with a simple union.
In the interest of keeping the changes of this patch local to one
file, the visitor interface still passes just a list of names
rather than the full list of QAPISchemaMember instances. We may
want to revisit this in the future, if the consistency with
visit_object_type() is worth it.
Signed-off-by: Eric Blake
Message-Id: <1449033659-25497-12-git-send-email-ebl...@redhat.com>
[Eric's simplifying followup squashed in]
Signed-off-by: Markus Armbruster
---
scripts/qapi.py | 37 ++---
1 file changed, 26 insertions(+), 11 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 168463a..07e8303 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -901,13 +901,16 @@ class QAPISchemaEnumType(QAPISchemaType):
def __init__(self, name, info, values, prefix):
QAPISchemaType.__init__(self, name, info)
for v in values:
-assert isinstance(v, str)
+assert isinstance(v, QAPISchemaMember)
+v.set_owner(name)
assert prefix is None or isinstance(prefix, str)
self.values = values
self.prefix = prefix
def check(self, schema):
-assert len(set(self.values)) == len(self.values)
+seen = {}
+for v in self.values:
+v.check_clash(self.info, seen)
def is_implicit(self):
# See QAPISchema._make_implicit_enum_type()
@@ -916,8 +919,11 @@ class QAPISchemaEnumType(QAPISchemaType):
def c_type(self, is_param=False):
return c_name(self.name)
+def member_names(self):
+return [v.name for v in self.values]
+
def c_null(self):
-return c_enum_const(self.name, (self.values + ['_MAX'])[0],
+return c_enum_const(self.name, (self.member_names() + ['_MAX'])[0],
self.prefix)
def json_type(self):
@@ -925,7 +931,7 @@ class QAPISchemaEnumType(QAPISchemaType):
def visit(self, visitor):
visitor.visit_enum_type(self.name, self.info,
-self.values, self.prefix)
+self.member_names(), self.prefix)
class QAPISchemaArrayType(QAPISchemaType):
@@ -1050,6 +1056,9 @@ class QAPISchemaMember(object):
assert owner.endswith('-wrapper')
# Unreachable and not implemented
assert False
+if owner.endswith('Kind'):
+# See QAPISchema._make_implicit_enum_type()
+return '(branch of %s)' % owner[:-4]
return '(%s of %s)' % (self.role, owner)
def describe(self):
@@ -1100,7 +1109,7 @@ class QAPISchemaObjectTypeVariants(object):
# Union names must match enum values; alternate names are
# checked separately. Use 'seen' to tell the two apart.
if seen:
-assert v.name in self.tag_member.type.values
+assert v.name in self.tag_member.type.member_names()
assert isinstance(v.type, QAPISchemaObjectType)
v.type.check(schema)
@@ -1258,15 +1267,20 @@ class QAPISchema(object):
self.the_empty_object_type = QAPISchemaObjectType(':empty', None, None,
[], None)
self._def_entity(self.the_empty_object_type)
-self._def_entity(QAPISchemaEnumType('QType', None,
-['none', 'qnull', 'qint',
- 'qstring', 'qdict', 'qlist',
- 'qfloat', 'qbool'],
+qtype_values = self._make_enum_members(['none', 'qnull', 'qint',
+'qstring', 'qdict', 'qlist',
+'qfloat', 'qbool'])
+self._def_entity(QAPISchemaEnumType('QType', None, qtype_values,
'QTYPE'))
+def _make_enum_members(self, values):
+return [QAPISchemaMember(v) for v in values]
+
def _make_implicit_enum_type(self, name, info, values):
+# See also QAPISchemaObjectTypeMember._pretty_owner()
name = name + 'Kind' # Use namespace reserved by add_name()
-self._def_entity(QAPISchemaEnumType(name, info, value
Re: [Qemu-devel] [PATCH] block: fix bdrv_ioctl called from coroutine
On 17/12/2015 01:59, Fam Zheng wrote:
> On Wed, 12/16 19:33, Paolo Bonzini wrote:
>> When called from a coroutine, bdrv_ioctl must be asynchronous just like
>> e.g. bdrv_flush. The code was incorrectly making it synchronous, fix
>> it.
>>
>> Signed-off-by: Paolo Bonzini
>> ---
>> Fam, any reason why you did it this way? I don't see
>> any coroutine caller, but it doesn't make much sense. :)
>
> That is a surprising question! From a coroutine, it is bdrv_flush ->
> bdrv_flush_co_entry -> bdrv_co_flush, which I think is always synchronous,
> especially, noticing the code around calling bs->bdrv_aio_flush:
>
> acb = bs->drv->bdrv_aio_flush(bs, bdrv_co_io_em_complete, &co);
> if (acb == NULL) {
> ret = -EIO;
> } else {
> qemu_coroutine_yield();
> ret = co.ret;
> }
>
> Am I missing something?
In the coroutine case, the yield is hidden in the drivers, and it may or
may not happen. For example, qcow2_co_flush_to_os starts with
qemu_co_mutex_lock(&s->lock);
which can yield.
Paolo
> Fam
>
>>
>> block/io.c | 7 ---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/io.c b/block/io.c
>> index e00fb5d..841f5b5 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -2614,10 +2614,11 @@ int bdrv_ioctl(BlockDriverState *bs, unsigned long
>> int req, void *buf)
>> bdrv_co_ioctl_entry(&data);
>> } else {
>> Coroutine *co = qemu_coroutine_create(bdrv_co_ioctl_entry);
>> +
>> qemu_coroutine_enter(co, &data);
>> -}
>> -while (data.ret == -EINPROGRESS) {
>> -aio_poll(bdrv_get_aio_context(bs), true);
>> +while (data.ret == -EINPROGRESS) {
>> +aio_poll(bdrv_get_aio_context(bs), true);
>> +}
>> }
>> return data.ret;
>> }
>> --
>> 2.5.0
>>
>
>
[Qemu-devel] [PULL 33/40] qapi: Fix alternates that accept 'number' but not 'int'
From: Eric Blake
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
Message-Id: <1449033659-25497-8-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster
---
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 4797d6e..b93690b 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -184,6 +184,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)
@@ -194,16 +199,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);
+visit_get_next_type(v, &(*obj)->type, %(promote_int)s, name, &err);
if (err) {
goto out_obj;
}
Re: [Qemu-devel] [PATCH] scsi: always call notifier on async cancellation
On 17/12/2015 02:15, Fam Zheng wrote:
>> > if (notifier) {
>> > notifier_list_add(&req->cancel_notifiers, notifier);
>> > }
>> > -if (req->io_canceled) {
>> > -return;
>> > -}
>> > scsi_req_ref(req);
>> > scsi_req_dequeue(req);
>> > req->io_canceled = true;
>if (req->aiocb) {
>blk_aio_cancel_async(req->aiocb);
>} else {
>scsi_req_cancel_complete(req);
>}
>
> A second TMF must be blk_aio_cancel_async case, otherwise the first one would
> have already completed the request synchronously in scsi_req_cancel_complete.
Good point.
> With that in mind, I think returning early is not a problem. But I suppose
> these are also idempotent so this change is not breaking anything, either.
Right, the issue is that all these calls are idempotent, but the
notifier may not; that is why I prefer to be safe and ensure that all
notifier additions are matched by a notify. But you explained well why
this should be safe, I'll add a note to the commit message.
Paolo
Re: [Qemu-devel] [PATCH 0/9] Add full scatter-gather support for SCSI generic devices
Hi Paolo, On 12/16/2015 08:16 PM, Paolo Bonzini wrote: > > > On 16/12/2015 17:55, Alex Pyrgiotis wrote: >> Hi all, >> >> This patch is an attempt to boost the performance of "scsi-generic" and >> "scsi-block" device types, by removing an extra data copy and reducing >> their memory footprint. More specifically, the problem lies in the >> functions in the `scsi-generic_req_ops` struct of scsi-generic.c. These >> functions rely on an intermediate buffer to do the SG_IO ioctl request, >> without checking if the SCSI controller has provided a scatter-gather >> list with the request. >> >> In a nutshell, our proposal is to map the provided scatter-gather list >> (if any) to the address space of the QEMU process and use the resulting >> iovec as the buffer for the ioctl request. You'll find that the logic is >> quite similar to the one used in scsi-disk.c. > > Which commands have large payloads and are on the data path, for > scsi-block? Or is the use case just scsi-generic (e.g. tape devices?)? > > (Just trying to understand before I dive into the patches). Sure, no problem. The commands that have large payloads and are on the data path are the classic SCSI READ/WRITE commands. Usually, these commands are implemented with vectored reads/writes, which utilize the controller's scatter-gather list. However, when opening a "scsi-block" device with the default cache policy (cache=writeback), QEMU fallbacks to the "scsi-generic" functions (i.e, SG_IO ioctl requests) for reading/writing data [1]. In this case, the data are copied in a bounce buffer, which is the issue that this patch tackles. Thanks, Alex [1]: I'll quote the comment on the code for the rationale behind this choice: "If we are not using O_DIRECT, we might read stale data from the host cache if writes were made using other commands than these ones (such as WRITE SAME or EXTENDED COPY, etc.). So, without O_DIRECT everything must go through SG_IO."
[Qemu-devel] [PATCH v2 0/3] virtio: cross-endian helpers fixes
This series tries to rework cross-endian helpers for better clarity. It does not change behaviour, except perhaps patch 3/3 even if I could not measure any performance gain. --- Greg Kurz (3): virtio: move cross-endian helper to vhost vhost: move virtio 1.0 check to cross-endian helper virtio: optimize virtio_access_is_big_endian() for little-endian targets hw/virtio/vhost.c | 22 ++ include/hw/virtio/virtio-access.h | 16 +++- 2 files changed, 21 insertions(+), 17 deletions(-)
[Qemu-devel] [PULL 30/40] qapi: Simplify visiting of alternate types
From: Eric Blake
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.
Related bug, not not fixed in this patch: qapi-visit.py's
gen_visit_enum() generates a cast of its enum * argument to
int *. Marked FIXME.
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
Message-Id: <1449033659-25497-5-git-send-email-ebl...@redhat.com>
Signed-off-by: Markus Armbruster
---
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 | 15 ++-
scripts/qapi.py| 18 +-
tests/qapi-schema/alternate-empty.out | 1 -
tests/qapi-schema/qapi-schema-test.out | 8
tests/test-qmp-input-visitor.c | 31 ---
tests/test-qmp-output-visitor.c| 4 ++--
12 files changed, 54 insertions(+), 79 deletions(-)
diff --git a/docs/qapi-code-gen.tx
Re: [Qemu-devel] [PATCH v2] coverity: Model g_poll()
On 17/12/2015 08:20, Markus Armbruster wrote:
> In my testing, Coverity reported two more CHECKED_RETURN:
>
> * qemu-char.c:1248: fixed in commit c1f2448: "qemu-char: retry g_poll
> on EINTR".
>
> * migration/qemu-file-unix.c:75: harmless, cleaned up in commit
> 4e39f57 "migration: Clean up use of g_poll() in
> socket_writev_buffer()
>
> Signed-off-by: Markus Armbruster
> ---
> v2: Commit message body rewritten, because fixes have been committed
> meanwhile.
Applied, thanks!
Paolo
> scripts/coverity-model.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c
> index e1d5f45..ee5bf9d 100644
> --- a/scripts/coverity-model.c
> +++ b/scripts/coverity-model.c
> @@ -342,6 +342,15 @@ char *g_strconcat(const char *s, ...)
>
> /* Other glib functions */
>
> +typedef struct pollfd GPollFD;
> +
> +int poll();
> +
> +int g_poll (GPollFD *fds, unsigned nfds, int timeout)
> +{
> +return poll(fds, nfds, timeout);
> +}
> +
> typedef struct _GIOChannel GIOChannel;
> GIOChannel *g_io_channel_unix_new(int fd)
> {
>
[Qemu-devel] [PATCH v2 1/3] virtio: move cross-endian helper to vhost
If target is bi-endian (ppc64, arm), the virtio_legacy_is_cross_endian()
indeed returns the runtime state of the virtio device. However, it returns
false unconditionally in the general case. This sounds a bit strange
given the name of the function.
This helper is only useful for vhost actually, where indeed non bi-endian
targets don't have to deal with cross-endian issues.
This patch moves the helper to vhost.c and gives it a more appropriate name.
Signed-off-by: Greg Kurz
Reviewed-by: Cornelia Huck
---
hw/virtio/vhost.c | 17 +++--
include/hw/virtio/virtio-access.h | 13 -
2 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index de29968a7945..2e1e792d599e 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -748,6 +748,19 @@ static void vhost_log_stop(MemoryListener *listener,
/* FIXME: implement */
}
+static inline bool vhost_needs_vring_endian(VirtIODevice *vdev)
+{
+#ifdef TARGET_IS_BIENDIAN
+#ifdef HOST_WORDS_BIGENDIAN
+return !virtio_is_big_endian(vdev);
+#else
+return virtio_is_big_endian(vdev);
+#endif
+#else
+return false;
+#endif
+}
+
static int vhost_virtqueue_set_vring_endian_legacy(struct vhost_dev *dev,
bool is_big_endian,
int vhost_vq_index)
@@ -799,7 +812,7 @@ static int vhost_virtqueue_start(struct vhost_dev *dev,
}
if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1) &&
-virtio_legacy_is_cross_endian(vdev)) {
+vhost_needs_vring_endian(vdev)) {
r = vhost_virtqueue_set_vring_endian_legacy(dev,
virtio_is_big_endian(vdev),
vhost_vq_index);
@@ -896,7 +909,7 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
* native as legacy devices expect so by default.
*/
if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1) &&
-virtio_legacy_is_cross_endian(vdev)) {
+vhost_needs_vring_endian(vdev)) {
r = vhost_virtqueue_set_vring_endian_legacy(dev,
!virtio_is_big_endian(vdev),
vhost_vq_index);
diff --git a/include/hw/virtio/virtio-access.h
b/include/hw/virtio/virtio-access.h
index a01fff2e51d7..f1f12afe9089 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -32,19 +32,6 @@ static inline bool virtio_access_is_big_endian(VirtIODevice
*vdev)
#endif
}
-static inline bool virtio_legacy_is_cross_endian(VirtIODevice *vdev)
-{
-#ifdef TARGET_IS_BIENDIAN
-#ifdef HOST_WORDS_BIGENDIAN
-return !virtio_is_big_endian(vdev);
-#else
-return virtio_is_big_endian(vdev);
-#endif
-#else
-return false;
-#endif
-}
-
static inline uint16_t virtio_lduw_phys(VirtIODevice *vdev, hwaddr pa)
{
if (virtio_access_is_big_endian(vdev)) {
Re: [Qemu-devel] [Qemu-ppc] [PATCH] spapr/pci: populate PCI DT in reverse order
On Thu, 3 Dec 2015 15:53:17 +0100 Greg Kurz wrote: > On Tue, 1 Dec 2015 22:48:38 +0100 > Thomas Huth wrote: > > > On 30/11/15 11:45, Greg Kurz wrote: > > > Since commit 1d2d974244c6 "spapr_pci: enumerate and add PCI device tree", > > > QEMU > > > populates the PCI device tree in the opposite order compared to SLOF. > > > > > > Before 1d2d974244c6: > > > > > > Populating /pci@8002000 > > > 00 (D) : 1af4 1000virtio [ net ] > > > 00 0800 (D) : 1af4 1001virtio [ block ] > > > 00 1000 (D) : 1af4 1009virtio [ network ] > > > Populating /pci@8002000/unknown-legacy-device@2 > > > > > > > > > 7e5294b8 : /pci@8002000 > > > 7e52b998 : |-- ethernet@0 > > > 7e52c0c8 : |-- scsi@1 > > > 7e52c7e8 : +-- unknown-legacy-device@2 ok > > > > > > Since 1d2d974244c6: > > > > > > Populating /pci@8002000 > > > 00 1000 (D) : 1af4 1009virtio [ network ] > > > Populating /pci@8002000/unknown-legacy-device@2 > > > 00 0800 (D) : 1af4 1001virtio [ block ] > > > 00 (D) : 1af4 1000virtio [ net ] > > > > > > > > > 7e5e8118 : /pci@8002000 > > > 7e5ea6a0 : |-- unknown-legacy-device@2 > > > 7e5eadb8 : |-- scsi@1 > > > 7e5eb4d8 : +-- ethernet@0 ok > > > > > > This behaviour change is not actually a bug since no assumptions should be > > > made on DT ordering. But it has no real justification either, other than > > > being the consequence of the way fdt_add_subnode() inserts new elements > > > to the front of the FDT rather than adding them to the tail. > > > > > > This patch reverts to the historical SLOF ordering by walking PCI devices > > > in > > > reverse order. > > > > I've applied your patch here locally, and indeed, the device tree looks > > nicer to me, too, when the nodes are listed in ascending order. > > > > Tested-by: Thomas Huth > > > > > Ping ? > Thanks for testing ! > > Cheers. > > -- > Greg > >
[Qemu-devel] [PATCH v2 2/3] vhost: move virtio 1.0 check to cross-endian helper
Indeed vhost doesn't need to ask for vring endian fixing if the device is
virtio 1.0, since it is already handled by the in-kernel vhost driver. This
patch simply consolidates the logic into the existing helper.
Signed-off-by: Greg Kurz
Reviewed-by: Cornelia Huck
---
hw/virtio/vhost.c |9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 2e1e792d599e..aef750df22ad 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -750,6 +750,9 @@ static void vhost_log_stop(MemoryListener *listener,
static inline bool vhost_needs_vring_endian(VirtIODevice *vdev)
{
+if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
+return false;
+}
#ifdef TARGET_IS_BIENDIAN
#ifdef HOST_WORDS_BIGENDIAN
return !virtio_is_big_endian(vdev);
@@ -811,8 +814,7 @@ static int vhost_virtqueue_start(struct vhost_dev *dev,
return -errno;
}
-if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1) &&
-vhost_needs_vring_endian(vdev)) {
+if (vhost_needs_vring_endian(vdev)) {
r = vhost_virtqueue_set_vring_endian_legacy(dev,
virtio_is_big_endian(vdev),
vhost_vq_index);
@@ -908,8 +910,7 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
/* In the cross-endian case, we need to reset the vring endianness to
* native as legacy devices expect so by default.
*/
-if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1) &&
-vhost_needs_vring_endian(vdev)) {
+if (vhost_needs_vring_endian(vdev)) {
r = vhost_virtqueue_set_vring_endian_legacy(dev,
!virtio_is_big_endian(vdev),
vhost_vq_index);
[Qemu-devel] [PATCH v2 3/3] virtio: optimize virtio_access_is_big_endian() for little-endian targets
When adding cross-endian support, we introduced the TARGET_IS_BIENDIAN macro
and the virtio_access_is_big_endian() helper to have a branchless fast path
in the virtio memory accessors for targets that don't switch endian.
This was considered as a strong requirement at the time.
Now we have added a runtime check for virtio 1.0, which ruins the benefit
of the virtio_access_is_big_endian() helper for always little-endian targets.
With this patch, always little-endian targets stop checking for virtio 1.0,
since the result is little-endian in all cases.
Signed-off-by: Greg Kurz
---
v2: - don't rename helper to virtio_is_big_endian_fast()
---
include/hw/virtio/virtio-access.h |3 +++
1 file changed, 3 insertions(+)
diff --git a/include/hw/virtio/virtio-access.h
b/include/hw/virtio/virtio-access.h
index f1f12afe9089..fba4b4a4e1b2 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -19,10 +19,13 @@
static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
{
+#if defined(TARGET_IS_BIENDIAN) || defined(TARGET_WORDS_BIGENDIAN)
if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
/* Devices conforming to VIRTIO 1.0 or later are always LE. */
return false;
}
+#endif
+
#if defined(TARGET_IS_BIENDIAN)
return virtio_is_big_endian(vdev);
#elif defined(TARGET_WORDS_BIGENDIAN)
Re: [Qemu-devel] [PATCH v2] qom: change object property iterator API contract
"Daniel P. Berrange" writes:
> Currently the object property iterator API works as follows
>
> ObjectPropertyIterator *iter;
>
> iter = object_property_iter_init(obj);
> while ((prop = object_property_iter_next(iter))) {
> ...
> }
> object_property_iter_free(iter);
>
> This has the benefit that the ObjectPropertyIterator struct
> can be opaque, but has the downside that callers need to
> explicitly call a free function. It is also not in keeping
> with iterator style used elsewhere in QEMU/glib2
>
> This patch changes the API to use stack allocation instead
>
> ObjectPropertyIterator iter;
>
> object_property_iter_init(&iter, obj);
> while ((prop = object_property_iter_next(&iter))) {
> ...
> }
>
> Reviewed-by: Eric Blake
> Signed-off-by: Daniel P. Berrange
[...]
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 9630c5b..275a21b 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -971,6 +971,11 @@ ObjectProperty *object_class_property_find(ObjectClass
> *klass, const char *name,
>
> typedef struct ObjectPropertyIterator ObjectPropertyIterator;
>
> +struct ObjectPropertyIterator {
> +ObjectClass *nextclass;
> +GHashTableIter iter;
> +};
> +
Please fuse the two declarations. Perhaps Andreas can do that on
commit.
> /**
> * object_property_iter_init:
> * @obj: the object
Reviewed-by: Markus Armbruster
[Qemu-devel] [PULL 4/6] fw_cfg: avoid calculating invalid current entry pointer
From: "Gabriel L. Somlo"
When calculating a pointer to the currently selected fw_cfg item, the
following is used:
FWCfgEntry *e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
When s->cur_entry is FW_CFG_INVALID, we are calculating the address of
a non-existent element in s->entries[arch][...], which is undefined.
This patch ensures the resulting entry pointer is set to NULL whenever
s->cur_entry is FW_CFG_INVALID.
Reported-by: Laszlo Ersek
Reviewed-by: Laszlo Ersek
Signed-off-by: Gabriel Somlo
Message-id: 1446733972-1602-5-git-send-email-so...@cmu.edu
Cc: Marc Marí
Signed-off-by: Gabriel Somlo
Reviewed-by: Laszlo Ersek
Signed-off-by: Gerd Hoffmann
---
hw/nvram/fw_cfg.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index c2d3a0a..046fa74 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -277,7 +277,8 @@ static int fw_cfg_select(FWCfgState *s, uint16_t key)
static uint8_t fw_cfg_read(FWCfgState *s)
{
int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
-FWCfgEntry *e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
+FWCfgEntry *e = (s->cur_entry == FW_CFG_INVALID) ? NULL :
+&s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
uint8_t ret;
if (s->cur_entry == FW_CFG_INVALID || !e->data || s->cur_offset >= e->len)
@@ -342,7 +343,8 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
}
arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
-e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
+e = (s->cur_entry == FW_CFG_INVALID) ? NULL :
+&s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
if (dma.control & FW_CFG_DMA_CTL_READ) {
read = 1;
--
1.8.3.1
[Qemu-devel] [PULL 3/6] fw_cfg: remove offset argument from callback prototype
From: "Gabriel L. Somlo"
Read callbacks are now only invoked at item selection, before any
data is read. As such, the value of the offset argument passed to
the callback will always be 0. Also, the two callback instances
currently in use both leave their offset argument unused.
This patch removes the offset argument from the fw_cfg read callback
prototype, and from the currently available instances. The unused
(write) callback prototype is also removed (write support was removed
earlier, in commit 023e3148).
Cc: Laszlo Ersek
Cc: Gerd Hoffmann
Cc: Marc Marí
Signed-off-by: Gabriel Somlo
Reviewed-by: Laszlo Ersek
Message-id: 1446733972-1602-4-git-send-email-so...@cmu.edu
Signed-off-by: Gerd Hoffmann
---
hw/arm/virt-acpi-build.c | 2 +-
hw/i386/acpi-build.c | 2 +-
hw/nvram/fw_cfg.c | 2 +-
include/hw/nvram/fw_cfg.h | 3 +--
4 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 3c2c5d6..8fd0b1c 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -631,7 +631,7 @@ static void acpi_ram_update(MemoryRegion *mr, GArray *data)
memory_region_set_dirty(mr, 0, size);
}
-static void virt_acpi_build_update(void *build_opaque, uint32_t offset)
+static void virt_acpi_build_update(void *build_opaque)
{
AcpiBuildState *build_state = build_opaque;
AcpiBuildTables tables;
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 95e0c65..29e30ce 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1818,7 +1818,7 @@ static void acpi_ram_update(MemoryRegion *mr, GArray
*data)
memory_region_set_dirty(mr, 0, size);
}
-static void acpi_build_update(void *build_opaque, uint32_t offset)
+static void acpi_build_update(void *build_opaque)
{
AcpiBuildState *build_state = build_opaque;
AcpiBuildTables tables;
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 6e6414b..c2d3a0a 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -266,7 +266,7 @@ static int fw_cfg_select(FWCfgState *s, uint16_t key)
arch = !!(key & FW_CFG_ARCH_LOCAL);
e = &s->entries[arch][key & FW_CFG_ENTRY_MASK];
if (e->read_callback) {
-e->read_callback(e->callback_opaque, s->cur_offset);
+e->read_callback(e->callback_opaque);
}
}
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index a1cfaa4..664eaf6 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -70,8 +70,7 @@ typedef struct FWCfgDmaAccess {
uint64_t address;
} QEMU_PACKED FWCfgDmaAccess;
-typedef void (*FWCfgCallback)(void *opaque, uint8_t *data);
-typedef void (*FWCfgReadCallback)(void *opaque, uint32_t offset);
+typedef void (*FWCfgReadCallback)(void *opaque);
/**
* fw_cfg_add_bytes:
--
1.8.3.1
[Qemu-devel] [PULL 0/6] fw_cfg: doc updates, various optimizations.
Hi, Here comes a fw_cfg update with a doc update and various optimizations from Gabriel L. Somlo. Read callback is called only once (on select), also sanity checks for multibyte reads are done only once. please pull, Gerd The following changes since commit f05b42d3fd30bb9673cc1ac1ee8c2af8f669964e: Update version for v2.5.0-rc4 release (2015-12-11 16:37:55 +) are available in the git repository at: git://git.kraxel.org/qemu tags/pull-fw-cfg-20151217-1 for you to fetch changes up to 6c8d56a2e95712a6206a2671d2b04b2e59cabc0b: fw_cfg: replace ioport data read with generic method (2015-12-15 11:46:13 +0100) fw_cfg: doc updates, various optimizations. Gabriel L. Somlo (6): fw_cfg: move internal function call docs to header file fw_cfg: amend callback behavior spec to once per select fw_cfg: remove offset argument from callback prototype fw_cfg: avoid calculating invalid current entry pointer fw_cfg: add generic non-DMA read method fw_cfg: replace ioport data read with generic method docs/specs/fw_cfg.txt | 85 +- hw/arm/virt-acpi-build.c | 2 +- hw/i386/acpi-build.c | 2 +- hw/nvram/fw_cfg.c | 75 +-- include/hw/nvram/fw_cfg.h | 128 +- trace-events | 2 +- 6 files changed, 166 insertions(+), 128 deletions(-)
[Qemu-devel] [PULL 1/6] fw_cfg: move internal function call docs to header file
From: "Gabriel L. Somlo" Move documentation for fw_cfg functions internal to qemufrom docs/specs/fw_cfg.txt to the fw_cfg.h header file, next to their prototype declarations, formatted as doc-comments. NOTE: Documentation for fw_cfg_add_callback() is completely dropped by this patch, as that function has been eliminated by commit 023e3148. Suggested-by: Peter Maydell Cc: Laszlo Ersek Cc: Gerd Hoffmann Cc: Marc Marí Cc: Jordan Justen Cc: Paolo Bonzini Cc: Peter Maydell Signed-off-by: Gabriel Somlo Reviewed-by: Laszlo Ersek Message-id: 1446733972-1602-2-git-send-email-so...@cmu.edu Signed-off-by: Gerd Hoffmann --- docs/specs/fw_cfg.txt | 85 +- include/hw/nvram/fw_cfg.h | 129 ++ 2 files changed, 130 insertions(+), 84 deletions(-) diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt index b8c794f..2099ad9 100644 --- a/docs/specs/fw_cfg.txt +++ b/docs/specs/fw_cfg.txt @@ -192,90 +192,7 @@ To check the result, read the "control" field: today due to implementation not being async, but may in the future). -= Host-side API = - -The following functions are available to the QEMU programmer for adding -data to a fw_cfg device during guest initialization (see fw_cfg.h for -each function's complete prototype): - -== fw_cfg_add_bytes() == - -Given a selector key value, starting pointer, and size, create an item -as a raw "blob" of the given size, available by selecting the given key. -The data referenced by the starting pointer is only linked, NOT copied, -into the data structure of the fw_cfg device. - -== fw_cfg_add_string() == - -Instead of a starting pointer and size, this function accepts a pointer -to a NUL-terminated ascii string, and inserts a newly allocated copy of -the string (including the NUL terminator) into the fw_cfg device data -structure. - -== fw_cfg_add_iXX() == - -Insert an XX-bit item, where XX may be 16, 32, or 64. These functions -will convert a 16-, 32-, or 64-bit integer to little-endian, then add -a dynamically allocated copy of the appropriately sized item to fw_cfg -under the given selector key value. - -== fw_cfg_modify_iXX() == - -Modify the value of an XX-bit item (where XX may be 16, 32, or 64). -Similarly to the corresponding fw_cfg_add_iXX() function set, convert -a 16-, 32-, or 64-bit integer to little endian, create a dynamically -allocated copy of the required size, and replace the existing item at -the given selector key value with the newly allocated one. The previous -item, assumed to have been allocated during an earlier call to -fw_cfg_add_iXX() or fw_cfg_modify_iXX() (of the same width XX), is freed -before the function returns. - -== fw_cfg_add_file() == - -Given a filename (i.e., fw_cfg item name), starting pointer, and size, -create an item as a raw "blob" of the given size. Unlike fw_cfg_add_bytes() -above, the next available selector key (above 0x0020, FW_CFG_FILE_FIRST) -will be used, and a new entry will be added to the file directory structure -(at key 0x0019), containing the item name, blob size, and automatically -assigned selector key value. The data referenced by the starting pointer -is only linked, NOT copied, into the fw_cfg data structure. - -== fw_cfg_add_file_callback() == - -Like fw_cfg_add_file(), but additionally sets pointers to a callback -function (and opaque argument), which will be executed host-side by -QEMU each time a byte is read by the guest from this particular item. - -NOTE: The callback function is given the opaque argument set by -fw_cfg_add_file_callback(), but also the current data offset, -allowing it the option of only acting upon specific offset values -(e.g., 0, before the first data byte of the selected item is -returned to the guest). - -== fw_cfg_modify_file() == - -Given a filename (i.e., fw_cfg item name), starting pointer, and size, -completely replace the configuration item referenced by the given item -name with the new given blob. If an existing blob is found, its -callback information is removed, and a pointer to the old data is -returned to allow the caller to free it, helping avoid memory leaks. -If a configuration item does not already exist under the given item -name, a new item will be created as with fw_cfg_add_file(), and NULL -is returned to the caller. In any case, the data referenced by the -starting pointer is only linked, NOT copied, into the fw_cfg data -structure. - -== fw_cfg_add_callback() == - -Like fw_cfg_add_bytes(), but additionally sets pointers to a callback -function (and opaque argument), which will be executed host-side by -QEMU each time a guest-side write operation to this particular item -completes fully overwriting the item's data. - -NOTE: This function is deprecated, and will be completely removed -starting with QEMU v2.4. - -== Externally Provided Items == += Externally Provided Items = As of v2.4, "file" fw_cfg items (i.e., items with
[Qemu-devel] [PULL 2/6] fw_cfg: amend callback behavior spec to once per select
From: "Gabriel L. Somlo"
Currently, the fw_cfg internal API specifies that if an item was set up
with a read callback, the callback must be run each time a byte is read
from the item. This behavior is both wasteful (most items do not have a
read callback set), and impractical for bulk transfers (e.g., DMA read).
At the time of this writing, the only items configured with a callback
are "/etc/table-loader", "/etc/acpi/tables", and "/etc/acpi/rsdp". They
all share the same callback functions: virt_acpi_build_update() on ARM
(in hw/arm/virt-acpi-build.c), and acpi_build_update() on i386 (in
hw/i386/acpi.c). Both of these callbacks are one-shot (i.e. they return
without doing anything at all after the first time they are invoked with
a given build_state; since build_state is also shared across all three
items mentioned above, the callback only ever runs *once*, the first
time either of the listed items is read).
This patch amends the specification for fw_cfg_add_file_callback() to
state that any available read callback will only be invoked once each
time the item is selected. This change has no practical effect on the
current behavior of QEMU, and it enables us to significantly optimize
the behavior of fw_cfg reads during guest firmware setup, eliminating
a large amount of redundant callback checks and invocations.
Cc: Laszlo Ersek
Cc: Gerd Hoffmann
Cc: Marc Marí
Signed-off-by: Gabriel Somlo
Reviewed-by: Laszlo Ersek
Message-id: 1446733972-1602-3-git-send-email-so...@cmu.edu
Signed-off-by: Gerd Hoffmann
---
hw/nvram/fw_cfg.c | 19 ++-
include/hw/nvram/fw_cfg.h | 10 +++---
2 files changed, 13 insertions(+), 16 deletions(-)
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 73b0a81..6e6414b 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -252,7 +252,8 @@ static void fw_cfg_write(FWCfgState *s, uint8_t value)
static int fw_cfg_select(FWCfgState *s, uint16_t key)
{
-int ret;
+int arch, ret;
+FWCfgEntry *e;
s->cur_offset = 0;
if ((key & FW_CFG_ENTRY_MASK) >= FW_CFG_MAX_ENTRY) {
@@ -261,6 +262,12 @@ static int fw_cfg_select(FWCfgState *s, uint16_t key)
} else {
s->cur_entry = key;
ret = 1;
+/* entry successfully selected, now run callback if present */
+arch = !!(key & FW_CFG_ARCH_LOCAL);
+e = &s->entries[arch][key & FW_CFG_ENTRY_MASK];
+if (e->read_callback) {
+e->read_callback(e->callback_opaque, s->cur_offset);
+}
}
trace_fw_cfg_select(s, key, ret);
@@ -276,9 +283,6 @@ static uint8_t fw_cfg_read(FWCfgState *s)
if (s->cur_entry == FW_CFG_INVALID || !e->data || s->cur_offset >= e->len)
ret = 0;
else {
-if (e->read_callback) {
-e->read_callback(e->callback_opaque, s->cur_offset);
-}
ret = e->data[s->cur_offset++];
}
@@ -371,10 +375,6 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
len = (e->len - s->cur_offset);
}
-if (e->read_callback) {
-e->read_callback(e->callback_opaque, s->cur_offset);
-}
-
/* If the access is not a read access, it will be a skip access,
* tested before.
*/
@@ -513,7 +513,8 @@ static void fw_cfg_reset(DeviceState *d)
{
FWCfgState *s = FW_CFG(d);
-fw_cfg_select(s, 0);
+/* we never register a read callback for FW_CFG_SIGNATURE */
+fw_cfg_select(s, FW_CFG_SIGNATURE);
}
/* Save restore 32 bit int as uint16_t
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 4b5e196..a1cfaa4 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -183,13 +183,9 @@ void fw_cfg_add_file(FWCfgState *s, const char *filename,
void *data,
* structure residing at key value FW_CFG_FILE_DIR, containing the item name,
* data size, and assigned selector key value.
* Additionally, set a callback function (and argument) to be called each
- * time a byte is read by the guest from this particular item, or, in the
- * case of DMA, each time a read or skip request overlaps with the valid
- * offset range of the item.
- * NOTE: In addition to the opaque argument set here, the callback function
- * takes the current data offset as an additional argument, allowing it the
- * option of only acting upon specific offset values (e.g., 0, before the
- * first data byte of the selected item is returned to the guest).
+ * time this item is selected (by having its selector key either written to
+ * the fw_cfg control register, or passed to QEMU in FWCfgDmaAccess.control
+ * with FW_CFG_DMA_CTL_SELECT).
*/
void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
FWCfgReadCallback callback, void
*callback_opaque,
--
1.8.3.1
[Qemu-devel] [PULL 5/6] fw_cfg: add generic non-DMA read method
From: "Gabriel L. Somlo"
Introduce fw_cfg_data_read(), a generic read method which works
on all access widths (1 through 8 bytes, inclusive), and can be
used during both IOPort and MMIO read accesses.
To maintain legibility, only fw_cfg_data_mem_read() (the MMIO
data read method) is replaced by this patch. The new method
essentially unwinds the fw_cfg_data_mem_read() + fw_cfg_read()
combo, but without unnecessarily repeating all the validity
checks performed by the latter on each byte being read.
This patch also modifies the trace_fw_cfg_read prototype to
accept a 64-bit value argument, allowing it to work properly
with the new read method, but also remain backward compatible
with existing call sites.
Cc: Laszlo Ersek
Cc: Gerd Hoffmann
Cc: Marc Marí
Signed-off-by: Gabriel Somlo
Reviewed-by: Laszlo Ersek
Message-id: 1446733972-1602-6-git-send-email-so...@cmu.edu
Signed-off-by: Gerd Hoffmann
---
hw/nvram/fw_cfg.c | 45 +++--
trace-events | 2 +-
2 files changed, 32 insertions(+), 15 deletions(-)
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 046fa74..8f566f6 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -274,6 +274,36 @@ static int fw_cfg_select(FWCfgState *s, uint16_t key)
return ret;
}
+static uint64_t fw_cfg_data_read(void *opaque, hwaddr addr, unsigned size)
+{
+FWCfgState *s = opaque;
+int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
+FWCfgEntry *e = (s->cur_entry == FW_CFG_INVALID) ? NULL :
+&s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
+uint64_t value = 0;
+
+assert(size > 0 && size <= sizeof(value));
+if (s->cur_entry != FW_CFG_INVALID && e->data && s->cur_offset < e->len) {
+/* The least significant 'size' bytes of the return value are
+ * expected to contain a string preserving portion of the item
+ * data, padded with zeros on the right in case we run out early.
+ * In technical terms, we're composing the host-endian representation
+ * of the big endian interpretation of the fw_cfg string.
+ */
+do {
+value = (value << 8) | e->data[s->cur_offset++];
+} while (--size && s->cur_offset < e->len);
+/* If size is still not zero, we *did* run out early, so continue
+ * left-shifting, to add the appropriate number of padding zeros
+ * on the right.
+ */
+value <<= 8 * size;
+}
+
+trace_fw_cfg_read(s, value);
+return value;
+}
+
static uint8_t fw_cfg_read(FWCfgState *s)
{
int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
@@ -291,19 +321,6 @@ static uint8_t fw_cfg_read(FWCfgState *s)
return ret;
}
-static uint64_t fw_cfg_data_mem_read(void *opaque, hwaddr addr,
- unsigned size)
-{
-FWCfgState *s = opaque;
-uint64_t value = 0;
-unsigned i;
-
-for (i = 0; i < size; ++i) {
-value = (value << 8) | fw_cfg_read(s);
-}
-return value;
-}
-
static void fw_cfg_data_mem_write(void *opaque, hwaddr addr,
uint64_t value, unsigned size)
{
@@ -485,7 +502,7 @@ static const MemoryRegionOps fw_cfg_ctl_mem_ops = {
};
static const MemoryRegionOps fw_cfg_data_mem_ops = {
-.read = fw_cfg_data_mem_read,
+.read = fw_cfg_data_read,
.write = fw_cfg_data_mem_write,
.endianness = DEVICE_BIG_ENDIAN,
.valid = {
diff --git a/trace-events b/trace-events
index 2fce98e..7fc4d11 100644
--- a/trace-events
+++ b/trace-events
@@ -196,7 +196,7 @@ ecc_diag_mem_readb(uint64_t addr, uint32_t ret) "Read
diagnostic %"PRId64"= %02x
# hw/nvram/fw_cfg.c
fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d"
-fw_cfg_read(void *s, uint8_t ret) "%p = %d"
+fw_cfg_read(void *s, uint64_t ret) "%p = %"PRIx64
fw_cfg_add_file(void *s, int index, char *name, size_t len) "%p #%d: %s (%zd
bytes)"
# hw/block/hd-geometry.c
--
1.8.3.1
Re: [Qemu-devel] [PATCH untested] mirror: start drained section earlier
On 17/12/2015 03:14, Fam Zheng wrote:
> @@ -388,7 +390,9 @@ static void mirror_exit(BlockJob *job, void *opaque)
> bdrv_unref(s->target);
> block_job_completed(&s->common, data->ret);
> g_free(data);
> -bdrv_drained_end(src);
> +if (drained_begin) {
> +bdrv_drained_end(src);
> +}
> bdrv_unref(src);
> }
>
> @@ -571,6 +575,7 @@ static void coroutine_fn mirror_run(void *opaque)
> */
> assert(QLIST_EMPTY(&bs->tracked_requests));
> s->common.cancelled = false;
> +s->drained_begin = true;
> break;
> }
> bdrv_drained_end(s->common.bs);
>
Good point, thanks!
Paolo
Re: [Qemu-devel] [PATCH v2] numa: Clean up query-memdev error handling
Markus Armbruster writes: > "Michael S. Tsirkin" writes: > >> On Mon, Nov 23, 2015 at 09:35:31AM +0100, Markus Armbruster wrote: >>> qmp_query_memdev() has two error paths: >>> >>> * When object_get_objects_root() returns null. It never does, so >>> simply drop the useless error handling. >>> >>> * When query_memdev() fails. It leaks err then. But any failure >>> there is actually a programming error. Switch it to &error_abort, >>> and drop the useless error handling. >>> >>> Messed up in commit 76b5d85 "qmp: add query-memdev". >>> >>> Signed-off-by: Markus Armbruster >> >> >> Post 2.5 right? > > I don't mind. I meant v1 for 2.5, but we've since convinced ourselves > that errors can't happen, and the memory leak is only latent. Friendly ping for 2.6.
[Qemu-devel] [PULL 6/6] fw_cfg: replace ioport data read with generic method
From: "Gabriel L. Somlo"
IOPort read access is limited to one byte at a time by
fw_cfg_comb_valid(). As such, fw_cfg_comb_read() may safely
ignore its size argument (which will always be 1), and simply
call its fw_cfg_read() helper function once, returning 8 bits
via the least significant byte of a 64-bit return value.
This patch replaces fw_cfg_comb_read() with the generic method
fw_cfg_data_read(), and removes the unused fw_cfg_read() helper.
When called with size = 1, fw_cfg_data_read() acts exactly like
fw_cfg_read(), performing the same set of sanity checks, and
executing the while loop at most once (subject to the current
read offset being within range).
Cc: Laszlo Ersek
Cc: Gerd Hoffmann
Cc: Marc Marí
Signed-off-by: Gabriel Somlo
Message-id: 1446733972-1602-7-git-send-email-so...@cmu.edu
Reviewed-by: Laszlo Ersek
Signed-off-by: Gerd Hoffmann
---
hw/nvram/fw_cfg.c | 25 +
1 file changed, 1 insertion(+), 24 deletions(-)
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 8f566f6..a1d650d 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -304,23 +304,6 @@ static uint64_t fw_cfg_data_read(void *opaque, hwaddr
addr, unsigned size)
return value;
}
-static uint8_t fw_cfg_read(FWCfgState *s)
-{
-int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
-FWCfgEntry *e = (s->cur_entry == FW_CFG_INVALID) ? NULL :
-&s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
-uint8_t ret;
-
-if (s->cur_entry == FW_CFG_INVALID || !e->data || s->cur_offset >= e->len)
-ret = 0;
-else {
-ret = e->data[s->cur_offset++];
-}
-
-trace_fw_cfg_read(s, ret);
-return ret;
-}
-
static void fw_cfg_data_mem_write(void *opaque, hwaddr addr,
uint64_t value, unsigned size)
{
@@ -470,12 +453,6 @@ static bool fw_cfg_ctl_mem_valid(void *opaque, hwaddr addr,
return is_write && size == 2;
}
-static uint64_t fw_cfg_comb_read(void *opaque, hwaddr addr,
- unsigned size)
-{
-return fw_cfg_read(opaque);
-}
-
static void fw_cfg_comb_write(void *opaque, hwaddr addr,
uint64_t value, unsigned size)
{
@@ -513,7 +490,7 @@ static const MemoryRegionOps fw_cfg_data_mem_ops = {
};
static const MemoryRegionOps fw_cfg_comb_mem_ops = {
-.read = fw_cfg_comb_read,
+.read = fw_cfg_data_read,
.write = fw_cfg_comb_write,
.endianness = DEVICE_LITTLE_ENDIAN,
.valid.accepts = fw_cfg_comb_valid,
--
1.8.3.1
Re: [Qemu-devel] [PATCH] rtc: introduce nmi disable bit handler for cmos
>
> From: Paolo Bonzini [mailto:paolo.bonz...@gmail.com] On Behalf Of Paolo
> Bonzini
> Sent: Thursday, December 17, 2015 4:37 PM
>
>
> On 17/12/2015 08:17, Gonglei (Arei) wrote:
> >> On 16/12/2015 11:28, Gonglei (Arei) wrote:
> >>> I'll move the global nmi_disabled into RTCState, then I have to add
> >>> a global
> >> RTCState
> >>> Variable so that other C files can use the
> rtc_state->external_nmi_disabled.
> >>
> >> Hmm, I think it should be done differently. This is a layering
> >> violation, the NMI_EN is essentially a pin (qemu_irq) between the ISA
> >> bridges and the RTC. The NMI "button" is also a component of the ISA
> >
> > So, you mean the NMI_EN can only control NMI injection came from ISA
> bridge?
> > What's this NMI "button" mean?
>
> The NMI command in the monitor is a "virtual NMI button".
>
Okay, I see.
> >> bridge; you should not need to touch anything except the RTC and the
> >> ISA bridges, in particular not the APICs.
> >>
> > Currently, the qmp command "inject-nmi" doesn't pass ISA bridge. How
> > do we address this situation?
>
> That's step two below: make the ISA bridges implement NMIState.
>
Yes, It's more reasonable.
> >> First, you need to add a qemu_irq argument to rtc_init. The RTC can
> >> raise/lower the IRQ on writes to port 0x70.
> >>
> >> Second, make the ISA bridges implement NMIState, where the
> >> implementation of NMIState is similar to inject_nmi in hw/core/nmi.c:
> >>
> >> CPU_FOREACH(cs) {
> >>X86CPU *cpu = X86_CPU(cs);
> >>
> >>if (!cpu->apic_state) {
> >>cpu_interrupt(cs, CPU_INTERRUPT_NMI);
> >>} else {
> >>apic_deliver_nmi(cpu->apic_state);
> >>}
> >> }
> >>
> >> Third, the ISA bridges (hw/isa/piix4.c and hw/isa/lpc_ich9.c) need to
> >
> > We don't use hw/isa/piix4.c but hw/pci-host/piix.c in x86 target. Right?
>
> Right, I said I had certainly messed up something. :)
>
> >> export a qemu_irq for nmi_en IRQ (e.g. using
> >> qdev_init_gpio_in_named), and you should modify the ISA bridge's
> >> implementation of NMIState to latch the NMI if you send one while
> >> NMIs are disabled. The nmi_en IRQ can also trigger an NMI when nmi_en
> is enabled and an NMI was latched.
> >
> > Sorry, I'm a bit confused. The nmi_en can trigger an NMI? Isn't a flag
> > bit which can enable/disable the NMI switch?
>
> Suppose an NMI was injected with the monitor while nmi_en was disabled.
> The NMI is then latched, and triggered when you enable NMIs again with
> nmi_en.
>
It's clear. I'll try to do this as your suggestion. Thank you so much!
Regards,
-Gonglei
Re: [Qemu-devel] [PATCH] target-mips: silence NaNs for cvt.s.d and cvt.d.s
On 06/12/15 16:11, Aurelien Jarno wrote: > cvt.s.d and cvt.d.s are FP operations and thus need to convert input > sNaN into corresponding qNaN. Explicitely use the floatXX_maybe_silence_nan > functions for that as the floatXX_to_floatXX functions do not do that. > > Cc: Leon Alrae > Signed-off-by: Aurelien Jarno > --- > target-mips/op_helper.c | 2 ++ > 1 file changed, 2 insertions(+) Thanks, applied to target-mips queue. Regards, Leon
Re: [Qemu-devel] [RFC PATCH v0 0/9] Generic cpu-core device
On Wed, Dec 16, 2015 at 16:11:08 +0100, Igor Mammedov wrote: > On Fri, 11 Dec 2015 09:27:57 +0530 > Bharata B Rao wrote: > > > On Thu, Dec 10, 2015 at 01:35:05PM +0100, Igor Mammedov wrote: > > > On Thu, 10 Dec 2015 11:45:35 +0530 > > > Bharata B Rao wrote: [...] > > > For initial conversion of x86-cpus to device-add we could do pretty > > > much the same like we do now, where cpu devices will appear under: > > > /machine (pc-i440fx-2.5-machine) > > > /unattached (container) > > > /device[x] (qemu64-x86_64-cpu) > > > > > > since we don't have to maintain/model dummy socket/core objects. > > > > > > PowerPC could do the similar only at core level since it has > > > need for modeling core objects. > > > > > > It doesn't change anything wrt current introspection state, since > > > cpus could be still found by mgmt tools that parse QOM tree. > > > > > > We probably should split 2 conflicting goals we are trying to meet > > > here, > > > > > > 1. make device-add/dell work with cpus / > > > drop support for cpu-add in favor of device_add > > > > > > 2. how to model QOM tree view for CPUs in arch independent manner > > > to make mgmt layer life easier. > > > > > > and work on them independently instead of arguing for years, > > > that would allow us to make progress in #1 while still thinking > > > about how to do #2 the right way if we really need it. > > > > Makes sense, s390 developer also recommends the same. Given that we > > have CPU hotplug patchsets from x86, PowerPC and s390 all > > implementing device_add semantics pending on the list, can we hope to > > get them merged for QEMU-2.6 ? > > > > So as seen below, the device is either "cpu_model-cpu_type" or just > > "cpu_type". > generic device_add isn't able to deal with 'cpu_model' stuff, so > it should be concrete 'cpu_type'. > Question is if libvirt can get a list of supported CPU types. > > > > > -device POWER8-powerpc64-cpu (pseries) > > -device qemu64-x86_64-cpu (pc) > > -device s390-cpu (s390) > > > > Is this going to be the final acceptable semantics ? Would libvirt be > > able to work with this different CPU device names for different > > guests ? > CCing Peter to check if libvirt could do it nad if his is ok with > proposed device_add semantics as in the it's he who will deal with it > on libvirt side. Well, this depends entirely on the implementation and the variety of the cpu device models. Libvirt requires that the cpu model for a given arch/machine type/whatever can be inferred either completely offline or via monitor commands that are available when qemu is started with the 'none' machine type. This is required as we query capabilities of a qemu binary beforehand and then use the cached data to create the command line. Running a qemu process just to query a cpu model is not acceptable as it adds significant overhead. Peter signature.asc Description: Digital signature
Re: [Qemu-devel] [PATCH v2] coverity: Model g_poll()
Paolo Bonzini writes: > On 17/12/2015 08:20, Markus Armbruster wrote: >> In my testing, Coverity reported two more CHECKED_RETURN: >> >> * qemu-char.c:1248: fixed in commit c1f2448: "qemu-char: retry g_poll >> on EINTR". >> >> * migration/qemu-file-unix.c:75: harmless, cleaned up in commit >> 4e39f57 "migration: Clean up use of g_poll() in >> socket_writev_buffer() >> >> Signed-off-by: Markus Armbruster >> --- >> v2: Commit message body rewritten, because fixes have been committed >> meanwhile. > > Applied, thanks! There's also Subject: [PATCH] coverity: Model g_memdup() Date: Mon, 30 Nov 2015 17:32:32 +0100 Message-Id: <1448901152-11716-1-git-send-email-arm...@redhat.com> I can post a pull request for both, but I'm of course happy to let them flow through your tree.
Re: [Qemu-devel] [PATCH 1/2] ohci: delay first SOF interrupt
On 16/12/15 14:39, Laurent Vivier wrote:
> On overcommitted CPU, kernel can be so slow that an interrupt can
> be triggered by the device whereas the driver is not ready to receive
> it. This drives us into an infinite loop.
>
> This does not happen on real hardware because real hardware never send
> interrupt immediately after the controller has been moved to OPERATION state.
>
> This patch tries to delay the first SOF interrupt to let driver exits from
> the critical section (which is not protected against interrupts...)
>
> Some details:
>
> - ohci_irq(): the OHCI interrupt handler, acknowledges the SOF IRQ
> only if the state of the driver (rh_state) is OHCI_STATE_RUNNING.
> So if this interrupt happens and the driver is not in this state,
> the function is called again and again, moving the system to a
> CPU starvation.
>
> - ohci_rh_resume(): the driver re-enables operation with OHCI_USB_OPER.
> In QEMU this start the SOF timer and QEMU starts to send IRQs. As
> the driver is not in OHCI_STATE_RUNNING and not protected against IRQ,
> the ohci_irq() can be called and the driver never moved to
> OHCI_STATE_RUNNING.
>
> Suggested-by: Gerd Hoffmann
> Signed-off-by: Laurent Vivier
> ---
> hw/usb/hcd-ohci.c | 10 ++
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
> index 7d65818..5f15ebb 100644
> --- a/hw/usb/hcd-ohci.c
> +++ b/hw/usb/hcd-ohci.c
> @@ -1232,11 +1232,13 @@ static int ohci_service_ed_list(OHCIState *ohci,
> uint32_t head, int completion)
> }
>
> /* Generate a SOF event, and set a timer for EOF */
May I suggest to reflect the new behavior (with the explanation) in the
comment above? ... otherwise this might be hard to understand in a
couple of years if you only read the source code and not the changelog.
> -static void ohci_sof(OHCIState *ohci)
> +static void ohci_sof(OHCIState *ohci, bool first)
> {
> ohci->sof_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> timer_mod(ohci->eof_timer, ohci->sof_time + usb_frame_time);
> -ohci_set_interrupt(ohci, OHCI_INTR_SF);
> +if (!first) {
> +ohci_set_interrupt(ohci, OHCI_INTR_SF);
> +}
> }
>
> /* Process Control and Bulk lists. */
> @@ -1318,7 +1320,7 @@ static void ohci_frame_boundary(void *opaque)
> ohci->done_count--;
>
> /* Do SOF stuff here */
> -ohci_sof(ohci);
> +ohci_sof(ohci, false);
>
> /* Writeback HCCA */
> if (ohci_put_hcca(ohci, ohci->hcca, &hcca)) {
> @@ -1343,7 +1345,7 @@ static int ohci_bus_start(OHCIState *ohci)
>
> trace_usb_ohci_start(ohci->name);
>
> -ohci_sof(ohci);
> +ohci_sof(ohci, true);
>
> return 1;
> }
Alternate idea: Split ohci_sof into two functions, e.g. like this:
static void ohci_sof_timer(OHCIState *ohci)
{
ohci->sof_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
timer_mod(ohci->eof_timer, ohci->sof_time + usb_frame_time);
}
static void ohci_sof(OHCIState *ohci)
{
ohci_sof_timer(ohci);
ohci_set_interrupt(ohci, OHCI_INTR_SF);
}
... and then only call ohci_sof_timer() in ohci_bus_start(). I think
that would be a little bit easier to read than the stuff with the
"first" parameter.
Anyway, the patch looks basically like a good idea to me, so I'm also
fine with the original form if you don't want to change it.
Thomas
Re: [Qemu-devel] [RESEND RFC 5/6] hw/arm/sysbus-fdt: helpers for clock node generation
Hi Alex (B),
On 11/26/2015 05:06 PM, Alex Bennée wrote:
>
> Eric Auger writes:
>
>> Some passthrough'ed devices depend on clock nodes. Those need to be
>> generated in the guest device tree. This patch introduces some helpers
>> to build a clock node from information retrieved from host device tree.
>>
>> - inherit_properties copies properties from a host device tree node to
>> a guest device tree node
>> - fdt_build_clock_node builds a guest clock node and checks the host
>> fellow clock is a fixed one.
>>
>> fdt_build_clock_node will become static as soon as a they get used
>>
>> Signed-off-by: Eric Auger
>> ---
>> hw/arm/sysbus-fdt.c | 110
>>
>> 1 file changed, 110 insertions(+)
>>
>> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
>> index 9d28797..22e5801 100644
>> --- a/hw/arm/sysbus-fdt.c
>> +++ b/hw/arm/sysbus-fdt.c
>> @@ -21,6 +21,7 @@
>> *
>> */
>>
>> +#include
>> #include "hw/arm/sysbus-fdt.h"
>> #include "qemu/error-report.h"
>> #include "sysemu/device_tree.h"
>> @@ -56,6 +57,115 @@ typedef struct NodeCreationPair {
>> int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque);
>> } NodeCreationPair;
>>
>> +/* helpers */
>> +
>> +struct HostProperty {
>> +const char *name;
>> +bool optional;
>> +};
>> +
>> +typedef struct HostProperty HostProperty;
>> +
>> +/**
>> + * inherit_properties
>> + *
>> + * copies properties listed in an array from host device tree to
>> + * guest device tree. If a non optional property is not found, the
>> + * function exits. Not found optional ones are ignored.
>> + * props: array of HostProperty to copy
>> + * nb_props: number of properties in the array
>> + * host_dt: host device tree blob
>> + * guest_dt: guest device tree blob
>> + * node_path: host dt node path where the property is supposed to be
>> + found
>> + * nodename: guest node name the properties should be added to
>> + *
>> + */
>> +static void inherit_properties(HostProperty *props, int nb_props,
>> + void *host_fdt, void *guest_fdt,
>> + char *node_path, char *nodename)
>> +{
>> +int i, prop_len;
>> +const void *r;
>> +
>> +for (i = 0; i < nb_props; i++) {
>> +r = qemu_fdt_getprop_optional(host_fdt, node_path,
>> + props[i].name,
>> + props[i].optional,
>> + &prop_len);
>
> indentation?
>
>> +if (r) {
>> +qemu_fdt_setprop(guest_fdt, nodename,
>> + props[i].name, r, prop_len);
>> +}
>> +}
>> +}
>> +
>> +/* clock properties whose values are copied/pasted from host */
>> +static HostProperty clock_inherited_properties[] = {
>> +{"compatible", 0},
>> +{"#clock-cells", 0},
>> +{"clock-frequency", 1},
>> +{"clock-output-names", 1},
>> +};
>> +
>> +/**
>> + * fdt_build_clock_node
>> + *
>> + * Build a guest clock node, used as a dependency from a passthrough'ed
>> + * device. Most information are retrieved from the host fellow node.
>> + * Also check the host clock is a fixed one.
>> + *
>> + * host_fdt: host device tree blob from which info are retrieved
>> + * guest_fdt: guest device tree blob where the clock node is added
>> + * host_phandle: phandle of the clock in host device tree
>> + * guest_phandle: phandle to assign to the guest node
>> + */
>> +int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
>> +uint32_t host_phandle,
>> +uint32_t guest_phandle);
>> +int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
>> +uint32_t host_phandle,
>> +uint32_t guest_phandle)
>
> I'm seeing double here ;-)
I am coming back to you wrt this comment. This function will become
static but in that patch it is not used yet so compilation fails (the
function starts being used in next path file). That's why I did not use
the static here. Then the compiler complains the function needs to be
pre-declared. Hence the declaration here. In next patch that disappears
and becomes static. Is that acceptable? If I squash this patch in next
one, this becomes a huge one.
Best Regards
Eric
>
>> +{
>> +char node_path[256];
>> +char *nodename;
>> +const void *r;
>> +int ret, prop_len;
>> +
>> +ret = fdt_node_offset_by_phandle(host_fdt, host_phandle);
>> +if (ret <= 0) {
>> +error_report("not able to locate clock handle %d in host device
>> tree\n",
>> + host_phandle);
>> +goto out;
>> +}
>> +ret = fdt_get_path(host_fdt, ret, node_path, 256);
>> +if (ret < 0) {
>> +error_report("not able to retrieve node path for clock handle %d\n",
>> + host_phandle);
>> +goto out;
>> +}
>> +
>> +r = qemu_fdt_getprop(host_fdt, node_path, "compatible", &prop
Re: [Qemu-devel] [PATCH 2/2] ohci: clear pending SOF on suspend
On 16/12/15 14:39, Laurent Vivier wrote: > On overcommitted CPU, kernel can be so slow that an interrupt can > be triggered by the device whereas the driver is not ready to receive > it. This drives us into an infinite loop. > > On suspend, if a SOF interrupt is raised between the stop of the > device processing and the change of the device internal state to > OHCI_USB_SUSPEND (QEMU stops SOF timer on this state change), this > interrupt is never acknowledged. > > This patch clears pending SOF interrupt on OHCI_USB_SUSPEND setting. > > Some details: > > - ohci_irq(): the OHCI interrupt handler, acknowledges the SOF IRQ > only if the state of the driver (rh_state) is OHCI_STATE_RUNNING. > So if this interrupt happens and the driver is not in this state, > the function is called again and again, moving the system to a > CPU starvation. > > - ohci_rh_suspend(): the function stop the operation and acknowledge > pending interrupts (but doesn't disable it). Later in the function, > the device is moved to OHCI_SUSPEND_STATE, and the driver to > OHCI_RH_SUSPENDED. If between the moment when the interrupt is > acknowledged and the moment when the device is suspended a new > interrupt is raised, it will be never acknowledged because the > driver is now not in OHCI_RH_RUNNING state. > > Signed-off-by: Laurent Vivier > --- > hw/usb/hcd-ohci.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c > index 5f15ebb..b5a4e39 100644 > --- a/hw/usb/hcd-ohci.c > +++ b/hw/usb/hcd-ohci.c > @@ -1438,6 +1438,9 @@ static void ohci_set_ctl(OHCIState *ohci, uint32_t val) > break; > case OHCI_USB_SUSPEND: > ohci_bus_stop(ohci); > +/* clear pending SF otherwise driver loops in ohci_irq() */ May I suggest to talk about "Linux driver" instead of only "driver" here? ... QEMU also supports other guests, so the context might not be clear otherwise. > +ohci->intr_status &= ~OHCI_INTR_SF; > +ohci_intr_update(ohci); > break; > case OHCI_USB_RESUME: > trace_usb_ohci_resume(ohci->name); Apart from that nit in the comment, patch looks sane to me. Reviewed-by: Thomas Huth
Re: [Qemu-devel] [PATCH v2 11/14] pc: Remove PcGuestInfo.isapc_ram_fw field
On 12/16/2015 09:48 PM, Eduardo Habkost wrote: On Tue, Dec 15, 2015 at 04:27:51PM +0200, Marcel Apfelbaum wrote: On 12/11/2015 08:42 PM, Eduardo Habkost wrote: [...] @@ -131,8 +130,7 @@ static void pc_q35_init(MachineState *machine) rom_memory = get_system_memory(); } -guest_info = pc_guest_info_init(pcms); -guest_info->isapc_ram_fw = false; This may not be an issue, I just want be sure. For Q35 isapc_ram_fw is always false, but now we are always querying !pcmc->pci_enabled. Now we have a Q35 case when !pcmc->pci_enabled *can* be true. Do we? pcmc->pci_enabled is always true in Q35. OK, thanks, so all the pcmc->pci_enabled "if" clauses in pc_q35_init are not necessary, right? Anyway, this is no connected tot his patch. Reviewed-by: Marcel Apfelbaum Thanks, Marcel
[Qemu-devel] [PATCH 3/4] Revert "tracetool: use Python 2.4-compatible exception handling syntax"
This reverts commit 662da3854e3f490223373b40afdcfcc339d14aa5. Signed-off-by: Markus Armbruster --- scripts/tracetool.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/tracetool.py b/scripts/tracetool.py index 83bde7b..7b82959 100755 --- a/scripts/tracetool.py +++ b/scripts/tracetool.py @@ -71,7 +71,7 @@ def main(args): try: opts, args = getopt.getopt(args[1:], "", long_opts) -except getopt.GetoptError, err: +except getopt.GetoptError as err: error_opt(str(err)) check_backends = False @@ -132,7 +132,7 @@ def main(args): try: tracetool.generate(sys.stdin, arg_format, arg_backends, binary=binary, probe_prefix=probe_prefix) -except tracetool.TracetoolError, e: +except tracetool.TracetoolError as e: error_opt(str(e)) if __name__ == "__main__": -- 2.4.3
[Qemu-devel] [PATCH 1/4] qapi: Use Python 2.6 "except E as ..." syntax
PEP 8 calls for it, because it's forward compatibile with Python 3. Signed-off-by: Markus Armbruster --- scripts/qapi.py | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/scripts/qapi.py b/scripts/qapi.py index 7c50cc4..ba7151b 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -152,7 +152,7 @@ class QAPISchemaParser(object): continue try: fobj = open(incl_abs_fname, 'r') -except IOError, e: +except IOError as e: raise QAPIExprError(expr_info, '%s: %s' % (e.strerror, include)) exprs_include = QAPISchemaParser(fobj, previously_included, @@ -1148,7 +1148,7 @@ class QAPISchema(object): self._predefining = False self._def_exprs() self.check() -except (QAPISchemaError, QAPIExprError), err: +except (QAPISchemaError, QAPIExprError) as err: print >>sys.stderr, err exit(1) @@ -1639,7 +1639,7 @@ def parse_command_line(extra_options="", extra_long_options=[]): "chp:o:" + extra_options, ["source", "header", "prefix=", "output-dir="] + extra_long_options) -except getopt.GetoptError, err: +except getopt.GetoptError as err: print >>sys.stderr, "%s: %s" % (sys.argv[0], str(err)) sys.exit(1) @@ -1693,7 +1693,7 @@ def open_output(output_dir, do_c, do_h, prefix, c_file, h_file, if output_dir: try: os.makedirs(output_dir) -except os.error, e: +except os.error as e: if e.errno != errno.EEXIST: raise -- 2.4.3
[Qemu-devel] [PATCH 4/4] tests: Use Python 2.6 "except E as ..." syntax
PEP 8 calls for it, because it's forward compatibile with Python 3.
Signed-off-by: Markus Armbruster
---
tests/image-fuzzer/runner.py | 12 ++--
tests/qemu-iotests/qed.py| 2 +-
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/tests/image-fuzzer/runner.py b/tests/image-fuzzer/runner.py
index be7e283..96a1c11 100755
--- a/tests/image-fuzzer/runner.py
+++ b/tests/image-fuzzer/runner.py
@@ -157,7 +157,7 @@ class TestEnv(object):
try:
os.makedirs(self.current_dir)
-except OSError, e:
+except OSError as e:
print >>sys.stderr, \
"Error: The working directory '%s' cannot be used. Reason: %s"\
% (self.work_dir, e[1])
@@ -244,7 +244,7 @@ class TestEnv(object):
temp_log = StringIO.StringIO()
try:
retcode = run_app(temp_log, current_cmd)
-except OSError, e:
+except OSError as e:
multilog("%sError: Start of '%s' failed. Reason: %s\n\n"
% (test_summary, os.path.basename(current_cmd[0]),
e[1]),
@@ -356,7 +356,7 @@ if __name__ == '__main__':
opts, args = getopt.gnu_getopt(sys.argv[1:], 'c:hs:kvd:',
['command=', 'help', 'seed=', 'config=',
'keep_passed', 'verbose', 'duration='])
-except getopt.error, e:
+except getopt.error as e:
print >>sys.stderr, \
"Error: %s\n\nTry 'runner.py --help' for more information" % e
sys.exit(1)
@@ -374,7 +374,7 @@ if __name__ == '__main__':
elif opt in ('-c', '--command'):
try:
command = json.loads(arg)
-except (TypeError, ValueError, NameError), e:
+except (TypeError, ValueError, NameError) as e:
print >>sys.stderr, \
"Error: JSON array of test commands cannot be loaded.\n" \
"Reason: %s" % e
@@ -390,7 +390,7 @@ if __name__ == '__main__':
elif opt == '--config':
try:
config = json.loads(arg)
-except (TypeError, ValueError, NameError), e:
+except (TypeError, ValueError, NameError) as e:
print >>sys.stderr, \
"Error: JSON array with the fuzzer configuration cannot" \
" be loaded\nReason: %s" % e
@@ -414,7 +414,7 @@ if __name__ == '__main__':
try:
image_generator = __import__(generator_name)
-except ImportError, e:
+except ImportError as e:
print >>sys.stderr, \
"Error: The image generator '%s' cannot be imported.\n" \
"Reason: %s" % (generator_name, e)
diff --git a/tests/qemu-iotests/qed.py b/tests/qemu-iotests/qed.py
index 52ff845..748068d 100755
--- a/tests/qemu-iotests/qed.py
+++ b/tests/qemu-iotests/qed.py
@@ -227,7 +227,7 @@ def main():
qed = QED(open(filename, 'r+b'))
try:
globals()[cmd](qed, *sys.argv[3:])
-except TypeError, e:
+except TypeError as e:
sys.stderr.write(globals()[cmd].__doc__ + '\n')
sys.exit(1)
--
2.4.3
[Qemu-devel] [PATCH 2/4] scripts/qmp: Use Python 2.6 "except E as ..." syntax
PEP 8 calls for it, because it's forward compatibile with Python 3. Signed-off-by: Markus Armbruster --- scripts/qmp/qemu-ga-client | 2 +- scripts/qmp/qmp| 4 ++-- scripts/qmp/qmp-shell | 2 +- scripts/qmp/qmp.py | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/scripts/qmp/qemu-ga-client b/scripts/qmp/qemu-ga-client index 9908f21..fd05605 100755 --- a/scripts/qmp/qemu-ga-client +++ b/scripts/qmp/qemu-ga-client @@ -259,7 +259,7 @@ def main(address, cmd, args): try: client = QemuGuestAgentClient(address) -except QemuGuestAgent.error, e: +except QemuGuestAgent.error as e: import errno print(e) diff --git a/scripts/qmp/qmp b/scripts/qmp/qmp index 1db3c7f..514b539 100755 --- a/scripts/qmp/qmp +++ b/scripts/qmp/qmp @@ -91,8 +91,8 @@ def main(args): try: os.environ['QMP_PATH'] = path os.execvp(fullcmd, [fullcmd] + args) -except OSError, (errno, msg): -if errno == 2: +except OSError as exc: +if exc.errno == 2: print 'Command "%s" not found.' % (fullcmd) return 1 raise diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell index fa39bf0..7a402ed 100755 --- a/scripts/qmp/qmp-shell +++ b/scripts/qmp/qmp-shell @@ -240,7 +240,7 @@ class QMPShell(qmp.QEMUMonitorProtocol): def _execute_cmd(self, cmdline): try: qmpcmd = self.__build_cmd(cmdline) -except Exception, e: +except Exception as e: print 'Error while parsing command line: %s' % e print 'command format: ', print '[arg-name1=arg1] ... [arg-nameN=argN]' diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py index 1d38e3e..779332f 100644 --- a/scripts/qmp/qmp.py +++ b/scripts/qmp/qmp.py @@ -92,7 +92,7 @@ class QEMUMonitorProtocol: self.__sock.setblocking(0) try: self.__json_read() -except socket.error, err: +except socket.error as err: if err[0] == errno.EAGAIN: # No data available pass @@ -150,7 +150,7 @@ class QEMUMonitorProtocol: """ try: self.__sock.sendall(json.dumps(qmp_cmd)) -except socket.error, err: +except socket.error as err: if err[0] == errno.EPIPE: return raise socket.error(err) -- 2.4.3
[Qemu-devel] [PATCH 0/4] Use Python 2.6 "except E as ..." syntax
I can take this through my tree, but of course don't mind if a maintainer prefers to pick up "his" parts. Markus Armbruster (4): qapi: Use Python 2.6 "except E as ..." syntax scripts/qmp: Use Python 2.6 "except E as ..." syntax Revert "tracetool: use Python 2.4-compatible exception handling syntax" tests: Use Python 2.6 "except E as ..." syntax scripts/qapi.py | 8 scripts/qmp/qemu-ga-client | 2 +- scripts/qmp/qmp | 4 ++-- scripts/qmp/qmp-shell| 2 +- scripts/qmp/qmp.py | 4 ++-- scripts/tracetool.py | 4 ++-- tests/image-fuzzer/runner.py | 12 ++-- tests/qemu-iotests/qed.py| 2 +- 8 files changed, 19 insertions(+), 19 deletions(-) -- 2.4.3
Re: [Qemu-devel] [PATCH v5 5/6] xlnx-zynqmp: Connect the SPI devices
On 17/12/2015 09:26, Peter Maydell wrote: >> > In any case, I would prefer qdev_bus_rename to stay in xlnx-zynqmp.c. > I disagree that it should be in xlnx-zynqmp.c. Either > (a) qdev already provides some reasonable mechanism for > SoC container like this to allow their users to get at > buses provided by their child objects, in which case we > should use it > (b) qdev doesn't provide such a mechanism, in which case > we need to provide one (either qdev_bus_rename or something > else if you have a better idea) > > But we definitely shouldn't have the SoC container code > messing around with the internals of the qdev objects. It's a hack and I don't want it to become a sanctioned way to do it. It's already messing around pretty heavily with qdev internals, see the line right after QLIST_INSERT_HEAD: QLIST_INSERT_HEAD(&dev->child_bus, spi_bus, sibling); Paolo
[Qemu-devel] [PATCH] virtio: use smp_load_acquire/smp_store_release
virtio ring entries have exactly the acquire/release
semantics:
- reading used index acquires a ring entry from host
- updating the available index releases it to host
Thus when using weak barriers and building for SMP (as most people
do), smp_load_acquire and smp_store_release will do exactly
the right thing to synchronize with the host.
In fact, QEMU already uses __atomic_thread_fence(__ATOMIC_ACQUIRE) and
__atomic_thread_fence(__ATOMIC_RELEASE);
Documentation/circular-buffers.txt suggests smp_load_acquire and
smp_store_release for head and tail updates.
Since we have to worry about UP and strong barrier users,
let's add APIs to wrap these, and use in virtio_ring.c
It is tempting to use this in vhost as well,
this needs a bit more work to make acquire/release macros
work on __user pointers.
Signed-off-by: Michael S. Tsirkin
---
include/linux/virtio_ring.h | 30 ++
drivers/virtio/virtio_ring.c | 20 ++--
2 files changed, 40 insertions(+), 10 deletions(-)
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index 8e50888..0135c16 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -47,6 +47,36 @@ static inline void virtio_wmb(bool weak_barriers)
wmb();
}
+static inline __virtio16 virtio_load_acquire(bool weak_barriers, __virtio16 *p)
+{
+ if (!weak_barriers) {
+ rmb();
+ return READ_ONCE(*p);
+ }
+#ifdef CONFIG_SMP
+ return smp_load_acquire(p);
+#else
+ dma_rmb();
+ return READ_ONCE(*p);
+#endif
+}
+
+static inline void virtio_store_release(bool weak_barriers,
+ __virtio16 *p, __virtio16 v)
+{
+ if (!weak_barriers) {
+ wmb();
+ WRITE_ONCE(*p, v);
+ return;
+ }
+#ifdef CONFIG_SMP
+ smp_store_release(p, v);
+#else
+ dma_wmb();
+ WRITE_ONCE(*p, v);
+#endif
+}
+
struct virtio_device;
struct virtqueue;
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index ee663c4..f822cab 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -244,11 +244,11 @@ static inline int virtqueue_add(struct virtqueue *_vq,
avail = vq->avail_idx_shadow & (vq->vring.num - 1);
vq->vring.avail->ring[avail] = cpu_to_virtio16(_vq->vdev, head);
+ vq->avail_idx_shadow++;
/* Descriptors and available array need to be set before we expose the
* new available array entries. */
- virtio_wmb(vq->weak_barriers);
- vq->avail_idx_shadow++;
- vq->vring.avail->idx = cpu_to_virtio16(_vq->vdev, vq->avail_idx_shadow);
+ virtio_store_release(vq->weak_barriers, &vq->vring.avail->idx,
+cpu_to_virtio16(_vq->vdev, vq->avail_idx_shadow));
vq->num_added++;
pr_debug("Added buffer head %i to %p\n", head, vq);
@@ -453,9 +453,10 @@ static void detach_buf(struct vring_virtqueue *vq,
unsigned int head)
vq->vq.num_free++;
}
-static inline bool more_used(const struct vring_virtqueue *vq)
+static inline
+bool more_used(const struct vring_virtqueue *vq, __virtio16 used_idx)
{
- return vq->last_used_idx != virtio16_to_cpu(vq->vq.vdev,
vq->vring.used->idx);
+ return vq->last_used_idx != virtio16_to_cpu(vq->vq.vdev, used_idx);
}
/**
@@ -488,15 +489,14 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned
int *len)
return NULL;
}
- if (!more_used(vq)) {
+ /* Only get used array entries after they have been exposed by host. */
+ if (!more_used(vq, virtio_load_acquire(vq->weak_barriers,
+ &vq->vring.used->idx))) {
pr_debug("No more buffers in queue\n");
END_USE(vq);
return NULL;
}
- /* Only get used array entries after they have been exposed by host. */
- virtio_rmb(vq->weak_barriers);
-
last_used = (vq->last_used_idx & (vq->vring.num - 1));
i = virtio32_to_cpu(_vq->vdev, vq->vring.used->ring[last_used].id);
*len = virtio32_to_cpu(_vq->vdev, vq->vring.used->ring[last_used].len);
@@ -704,7 +704,7 @@ irqreturn_t vring_interrupt(int irq, void *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
- if (!more_used(vq)) {
+ if (!more_used(vq, vq->vring.used->idx)) {
pr_debug("virtqueue interrupt with no work for %p\n", vq);
return IRQ_NONE;
}
--
MST
Re: [Qemu-devel] [PATCH 0/9] Add full scatter-gather support for SCSI generic devices
On 17/12/2015 09:47, Alex Pyrgiotis wrote: >> > Which commands have large payloads and are on the data path, for >> > scsi-block? Or is the use case just scsi-generic (e.g. tape devices?)? >> > >> > (Just trying to understand before I dive into the patches). > Sure, no problem. The commands that have large payloads and are on the > data path are the classic SCSI READ/WRITE commands. Usually, these > commands are implemented with vectored reads/writes, which utilize the > controller's scatter-gather list. > > However, when opening a "scsi-block" device with the default cache > policy (cache=writeback), QEMU fallbacks to the "scsi-generic" functions > (i.e, SG_IO ioctl requests) for reading/writing data [1]. In this case, > the data are copied in a bounce buffer, which is the issue that this > patch tackles. Right, I forgot about that. However, falling back to scsi-generic effectively means that scsi-block is always O_DIRECT/cache=none. So why not just specify cache=none? We can improve the code to print a warning if you don't. (It needs some care: iscsi never caches, independent of the cache= argument, so we don't want to warn for it. But it can be done). Paolo
[Qemu-devel] [PATCH] virtio_ring: use smp_store_mb
We need a full barrier after writing out event index, using smp_store_mb
there seems better than open-coding.
As usual, we need a wrapper to account for strong barriers/non smp.
It's tempting to use this in vhost as well, for that, we'll
need a variant of smp_store_mb that works on __user pointers.
Signed-off-by: Michael S. Tsirkin
---
Seems to give a speedup on my box but I'm less sure about this one. E.g. as
xchng faster than mfence on all/most intel CPUs? Anyone has an opinion?
include/linux/virtio_ring.h | 14 ++
drivers/virtio/virtio_ring.c | 15 +--
2 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index 0135c16..8912189 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -47,6 +47,20 @@ static inline void virtio_wmb(bool weak_barriers)
wmb();
}
+static inline void virtio_store_mb(bool weak_barriers,
+ __virtio16 *p, __virtio16 v)
+{
+#ifdef CONFIG_SMP
+ if (weak_barriers)
+ smp_store_mb(*p, v);
+ else
+#endif
+ {
+ WRITE_ONCE(*p, v);
+ mb();
+ }
+}
+
static inline __virtio16 virtio_load_acquire(bool weak_barriers, __virtio16 *p)
{
if (!weak_barriers) {
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index f822cab..b0aea67 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -517,10 +517,10 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned
int *len)
/* If we expect an interrupt for the next entry, tell host
* by writing event index and flush out the write before
* the read in the next get_buf call. */
- if (!(vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) {
- vring_used_event(&vq->vring) = cpu_to_virtio16(_vq->vdev,
vq->last_used_idx);
- virtio_mb(vq->weak_barriers);
- }
+ if (!(vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT))
+ virtio_store_mb(vq->weak_barriers,
+ &vring_used_event(&vq->vring),
+ cpu_to_virtio16(_vq->vdev, vq->last_used_idx));
#ifdef DEBUG
vq->last_add_time_valid = false;
@@ -653,8 +653,11 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
}
/* TODO: tune this threshold */
bufs = (u16)(vq->avail_idx_shadow - vq->last_used_idx) * 3 / 4;
- vring_used_event(&vq->vring) = cpu_to_virtio16(_vq->vdev,
vq->last_used_idx + bufs);
- virtio_mb(vq->weak_barriers);
+
+ virtio_store_mb(vq->weak_barriers,
+ &vring_used_event(&vq->vring),
+ cpu_to_virtio16(_vq->vdev, vq->last_used_idx + bufs));
+
if (unlikely((u16)(virtio16_to_cpu(_vq->vdev, vq->vring.used->idx) -
vq->last_used_idx) > bufs)) {
END_USE(vq);
return false;
--
MST
[Qemu-devel] [PATCH 0/6] seabios: update to release 1.9.0
Hi, seabios 1.9.0 was released in November, missing hard freeze by a few days. Now 2.5 is out of the door and the master branch is open, time to care about the update now, so here we go. The patch series carries the seabios update itself, a few build and config adjustments and acpi cleanups. The update is also available here: git://git.kraxel.org/qemu work/seabios Please test & review, Gerd PS: Pull request will most likely have to wait until january, when I'm back online after xmas and newyear vacation. Gerd Hoffmann (6): seabios: update submodule to release 1.9.0 seabios: update 128k bios config seabios: use new EXTRAVERSION to tag qemu builds seabios: stop updating aml files seabios: update binaries to release 1.9.0 q35: skip q35-acpi-dsdt.aml load if not needed hw/i386/pc_q35.c | 5 - pc-bios/bios-256k.bin | Bin 262144 -> 262144 bytes pc-bios/bios.bin | Bin 131072 -> 131072 bytes pc-bios/vgabios-cirrus.bin | Bin 38400 -> 38400 bytes pc-bios/vgabios-qxl.bin| Bin 38400 -> 38912 bytes pc-bios/vgabios-stdvga.bin | Bin 38400 -> 38912 bytes pc-bios/vgabios-virtio.bin | Bin 38400 -> 38912 bytes pc-bios/vgabios-vmware.bin | Bin 38400 -> 38912 bytes pc-bios/vgabios.bin| Bin 38400 -> 38400 bytes roms/Makefile | 7 +++ roms/config.seabios-128k | 2 ++ roms/seabios | 2 +- 12 files changed, 10 insertions(+), 6 deletions(-) -- 1.8.3.1
[Qemu-devel] [PATCH 3/6] seabios: use new EXTRAVERSION to tag qemu builds
Signed-off-by: Gerd Hoffmann --- roms/Makefile | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/roms/Makefile b/roms/Makefile index 09e33b5..01f2701 100644 --- a/roms/Makefile +++ b/roms/Makefile @@ -35,7 +35,7 @@ powerpc_cross_prefix := $(call find-cross-prefix,powerpc) x86_64_cross_prefix := $(call find-cross-prefix,x86_64) # tag our seabios builds -SEABIOS_VERSION="$(shell cd seabios; git describe --tags --long) by qemu-project.org" +SEABIOS_EXTRAVERSION="-prebuilt.qemu-project.org" # # EfiRom utility is shipped with edk2 / tianocore, in BaseTools/ @@ -78,12 +78,12 @@ build-seabios-config-%: config.% mkdir -p seabios/builds/$* cp $< seabios/builds/$*/.config $(MAKE) -C seabios \ - VERSION=$(SEABIOS_VERSION) \ + EXTRAVERSION=$(SEABIOS_EXTRAVERSION) \ CROSS_COMPILE=$(x86_64_cross_prefix) \ KCONFIG_CONFIG=$(CURDIR)/seabios/builds/$*/.config \ OUT=$(CURDIR)/seabios/builds/$*/ oldnoconfig $(MAKE) -C seabios \ - VERSION=$(SEABIOS_VERSION) \ + EXTRAVERSION=$(SEABIOS_EXTRAVERSION) \ CROSS_COMPILE=$(x86_64_cross_prefix) \ KCONFIG_CONFIG=$(CURDIR)/seabios/builds/$*/.config \ OUT=$(CURDIR)/seabios/builds/$*/ all -- 1.8.3.1
[Qemu-devel] [PATCH 6/6] q35: skip q35-acpi-dsdt.aml load if not needed
Only old machine types which don't use the acpi builder (qemu 1.7 + older)
have to load that file for proper acpi support.
Signed-off-by: Gerd Hoffmann
---
hw/i386/pc_q35.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 133bc68..727269e 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -129,7 +129,10 @@ static void pc_q35_init(MachineState *machine)
}
pc_cpus_init(pcms);
-pc_acpi_init("q35-acpi-dsdt.aml");
+if (!has_acpi_build) {
+/* only machine types 1.7 & older need this */
+pc_acpi_init("q35-acpi-dsdt.aml");
+}
kvmclock_create();
--
1.8.3.1
[Qemu-devel] [PATCH 2/6] seabios: update 128k bios config
Turn off OHCI + TPM support to keep the size below 128k. Signed-off-by: Gerd Hoffmann --- roms/config.seabios-128k | 2 ++ 1 file changed, 2 insertions(+) diff --git a/roms/config.seabios-128k b/roms/config.seabios-128k index c719ba6..0a9da77 100644 --- a/roms/config.seabios-128k +++ b/roms/config.seabios-128k @@ -3,6 +3,8 @@ CONFIG_QEMU=y CONFIG_ROM_SIZE=128 CONFIG_XEN=n +CONFIG_USB_OHCI=n CONFIG_USB_XHCI=n CONFIG_USB_UAS=n CONFIG_SDCARD=n +CONFIG_TCGBIOS=n -- 1.8.3.1
[Qemu-devel] [PATCH 4/6] seabios: stop updating aml files
ACPI aml files traditionally have been managed in the seabios repo. In qemu version 2.0 we've switched over to have qemu generate the acpi tables and provide them to the firmware via fw_cfg. The old aml files are still there and used for old machine types. Well, actually the q35 file only, the piix4 version is compiled into seabios (unless built with CONFIG_ACPI_DSDT=n) and is there for reference only. The aml files havn't been touched for a long time, and given that new features requiring acpi changes are typically only added to new machine types this is unlikely to change in the future. So stop updating them. That allows to cleanup things a bit on the seabios side in the future. Signed-off-by: Gerd Hoffmann --- roms/Makefile | 1 - 1 file changed, 1 deletion(-) diff --git a/roms/Makefile b/roms/Makefile index 01f2701..7bd1252 100644 --- a/roms/Makefile +++ b/roms/Makefile @@ -64,7 +64,6 @@ default: bios: build-seabios-config-seabios-128k build-seabios-config-seabios-256k cp seabios/builds/seabios-128k/bios.bin ../pc-bios/bios.bin cp seabios/builds/seabios-256k/bios.bin ../pc-bios/bios-256k.bin - cp seabios/builds/seabios-256k/src/fw/*dsdt.aml ../pc-bios/ seavgabios: $(patsubst %,seavgabios-%,$(vgabios_variants)) -- 1.8.3.1
[Qemu-devel] [PATCH 1/6] seabios: update submodule to release 1.9.0
Highlights / user visible changes in seabios:
* boot menu key is ESC now.
* virtio 1.0 support.
* sdcard support.
* fw_cfg dma suport.
* usual share of bugfixes ;)
In vgabios:
* Emulates leal instruction. Works around a bug in old x86emu versions,
which makes old xorg vesa drivers work (RHEL-5 for example).
full shortlog rel-1.8.2..rel-1.9.0
--
Ameya Palande (1):
x86: add barrier to read{b,w,l} and write{b,w,l} functions
Andreas Färber (1):
checkrom: Fix typo in error message
Chen Fan (1):
pci: enable SERR# for error forwarding in bridge control register
Gerd Hoffmann (28):
vga: simplify vga builds
vga: rework virtio-vga support
vga: add virtio-vga to kconfig
pci: allow to loop over capabilities
virtio: run drivers in 32bit mode
virtio: add struct vp_device
virtio: pass struct pci_device to vp_init_simple
virtio: add version 1.0 structs and #defines
virtio: add version 0.9.5 struct
virtio: find version 1.0 virtio capabilities
virtio: create vp_cap struct for legacy bar
virtio: add read/write functions and macros
virtio: make features 64bit, support version 1.0 features
virtio: add version 1.0 support to vp_{get,set}_status
virtio: add version 1.0 support to vp_get_isr
virtio: add version 1.0 support to vp_reset
virtio: add version 1.0 support to vp_notify
virtio: remove unused vp_del_vq
virtio: add version 1.0 support to vp_find_vq
virtio-scsi: fix initialization for version 1.0
virtio-blk: fix initialization for version 1.0
virtio: use version 1.0 if available (flip the big switch)
virtio: also probe version 1.0 pci ids
virtio: legacy cleanup
virtio-blk: 32bit cleanup
virtio-scsi: 32bit cleanup
virtio-ring: 32bit cleanup
virtio-pci: use high memory for rings
Julius Werner (1):
xhci: Count new Max Scratchpad Bufs bits from XHCI 1.1
Kevin O'Connor (126):
docs: add page for SeaVGABIOS
docs: Add page describing the patch contribution process
docs: Add page on available CBFS/fw_cfg runtime config files
docs: Prefer triple backticks to multiple lines with single backticks
smp: Fix smp race introduced in 0673b787
docs: Note release date of 1.8.1
vgabios: On bda_save_restore() the saved vbe_mode also has flags in it
vgabios: Don't use extra stack if it appears a modern OS is in use
docs: Clarify that pci-optionrom-exec doesn't apply to roms in cbfs
checkstack: Replace function information tuple with class
checkstack: Simplify yield calculations
checkstack: Prefer passing "function" class instead of function address
smbios: Use integer signature instead of string signature
vgabios: Don't use "smsww" instruction - it confuses x86emu
vgabios: Add config option for assembler fixups
vgabios: Emulate "leal" instruction
checkstack: Minor - continue if not a regular asm line
Don't forward declare functions with "inline" in headers
build: Support "make VERSION=xyz" to override the default build version
tcg: Use seabios setup()/prepboot() calling convention for tcg
build: CONFIG_VGA_FIXUP_ASM should depend on CONFIG_BUILD_VGABIOS
bootorder: Update "extra pci root" buses bootorder format to match qemu
Make sure all code checks for malloc failures
docs: Note release date of 1.8.2
block: Split process_op() command dispatch up into multiple functions
block: Introduce default_process_op() with common command handling codes
block: Route scsi style commands through 'struct disk_op_s'
blockcmd: Introduce scsi_fill_cmd()
ata: Handle ATA ATAPI drives directly via 'struct disk_op_s' requests
ahci: Handle AHCI ATAPI drives directly via 'struct disk_op_s' requests
usb-msc: Handle USB drives directly via 'struct disk_op_s' requests
usb-uas: Handle USB drives directly via 'struct disk_op_s' requests
lsi-scsi: Handle LSI drives directly via 'struct disk_op_s' requests
esp-scsi: Handle ESP drives directly via 'struct disk_op_s' requests
megasas: Handle Megasas drives directly via 'struct disk_op_s' requests
virtio-scsi: Handle virtio drives directly via 'struct disk_op_s' requests
pvscsi: Move pvscsi_fill_req() code into pvscsi_cmd()
pvscsi: Handle pvscsi drives directly via 'struct disk_op_s' requests
blockcmd: Remove unused scsi_process_op() and cdb_cmd_data()
blockcmd: Convert cdb_is_read() to scsi_is_read()
block: Rename process_XXX_op() functions to XXX_process_op()
coreboot: Try to auto-detect if the CBFS anchor pointer is a relative
pointer
ps2: Support mode for polling the PS2 port instead of using irqs
ata: Make sure "chanid" is relative to PCI device for bootorder file
Don't enable interrupts prior to IVT and PIC setup
p
[Qemu-devel] [PATCH 1/1] HMP: Add equivalent to x-blockdev-change
From: "Dr. David Alan Gilbert"
x-blockdev-change has no HMP equivalent, so add x_block_change.
Example useages are:
x_block_change foo -a bah
to add the node bah to the parent foo
x_block_change foo -d bah
to delete the node bah from the parent foo
Signed-off-by: Dr. David Alan Gilbert
---
hmp-commands.hx | 18 ++
hmp.c | 20
hmp.h | 1 +
3 files changed, 39 insertions(+)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index a381b0b..cf2459b 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -57,6 +57,24 @@ Quit the emulator.
ETEXI
{
+.name = "x_block_change",
+.args_type = "parent:B,add:-a,del:-d,child:B",
+.params = "parent [-a] [-d] child",
+.help = "add or remove a child from a block driver",
+.mhandler.cmd = hmp_block_change,
+},
+
+STEXI
+@item x_block_change
+@findex x_block_change
+Dynamically reconfigure the block driver state graph. It can be used
+to add, remove, insert or replace a block driver state. Currently only
+the Quorum driver implements this feature to add or remove its child.
+This is useful to fix a broken quorum child.
+ETEXI
+
+
+{
.name = "block_resize",
.args_type = "device:B,size:o",
.params = "device size",
diff --git a/hmp.c b/hmp.c
index dc6dc30..631dacb 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1042,6 +1042,26 @@ void hmp_balloon(Monitor *mon, const QDict *qdict)
}
}
+void hmp_block_change(Monitor *mon, const QDict *qdict)
+{
+const char *parent = qdict_get_str(qdict, "parent");
+const char *child = qdict_get_str(qdict, "child");
+bool add = qdict_get_try_bool(qdict, "add", false);
+bool del = qdict_get_try_bool(qdict, "del", false);
+Error *err = NULL;
+
+if (add == del) {
+error_setg(&err, "One of -a or -d must be set");
+hmp_handle_error(mon, &err);
+return;
+}
+
+qmp_x_blockdev_change(parent,
+ del, child,
+ add, child, &err);
+hmp_handle_error(mon, &err);
+}
+
void hmp_block_resize(Monitor *mon, const QDict *qdict)
{
const char *device = qdict_get_str(qdict, "device");
diff --git a/hmp.h b/hmp.h
index 864a300..1588850 100644
--- a/hmp.h
+++ b/hmp.h
@@ -53,6 +53,7 @@ void hmp_cont(Monitor *mon, const QDict *qdict);
void hmp_system_wakeup(Monitor *mon, const QDict *qdict);
void hmp_nmi(Monitor *mon, const QDict *qdict);
void hmp_set_link(Monitor *mon, const QDict *qdict);
+void hmp_block_change(Monitor *mon, const QDict *qdict);
void hmp_block_passwd(Monitor *mon, const QDict *qdict);
void hmp_balloon(Monitor *mon, const QDict *qdict);
void hmp_block_resize(Monitor *mon, const QDict *qdict);
--
2.5.0
Re: [Qemu-devel] [PATCH] virtio_ring: use smp_store_mb
On Thu, Dec 17, 2015 at 12:32:53PM +0200, Michael S. Tsirkin wrote:
> +static inline void virtio_store_mb(bool weak_barriers,
> +__virtio16 *p, __virtio16 v)
> +{
> +#ifdef CONFIG_SMP
> + if (weak_barriers)
> + smp_store_mb(*p, v);
> + else
> +#endif
> + {
> + WRITE_ONCE(*p, v);
> + mb();
> + }
> +}
This is a different barrier depending on SMP, that seems wrong.
smp_mb(), as (should be) used by smp_store_mb() does not provide a
barrier against IO. mb() otoh does.
Since this is virtIO I would expect you always want mb().
Re: [Qemu-devel] [PATCH COLO-Frame v12 00/38] COarse-grain LOck-stepping(COLO) Virtual Machines for Non-stop Service (FT)
* Hailiang Zhang (zhang.zhanghaili...@huawei.com) wrote:
> On 2015/12/15 20:14, Dr. David Alan Gilbert wrote:
> >* zhanghailiang (zhang.zhanghaili...@huawei.com) wrote:
> >>This is the 12th version of COLO.
> >>
> >>As usual, this version of COLO is only support periodic checkpoint,
> >>just like MicroCheckpointing and Remus does.
> >>
> >>Here is only COLO frame part, you can get the whole codes from github:
> >>https://github.com/coloft/qemu/commits/colo-v2.3-periodic-mode
> >
> >Hi,
> > Have you tried wiring in Zhang Chen's new userland colo proxy yet?
> >I'd like to start trying it out.
> >
>
> Not yet, actually, for frame part, we can re-use most of the previous codes
> that based on
> kernel proxy. And, yes, please, you are welcome to join us. ;)
Yes, that's certainly something I'll look at immediately at the start of the
new year
(I'm out for 2 weeks from Friday).
I've just tested this series on my machines, and it works well.
Two things:
1) I just posted a patch to add an HMP equivalent to x-blockdev-change
2) If you run with an older machine type (e.g. pc-i440fx-2.3) then if I
failover to the
secondary then I hit a 'invalid runstate transition: 'inmigrate' ->
'prelaunch'';
I guess this is something to do with global_state.
Dave
> >Dave
> >
> >>Test procedure:
> >>1. Startup qemu
> >>Primary side:
> >>#x86_64-softmmu/qemu-system-x86_64 -enable-kvm -boot c -m 2048 -smp 2 -qmp
> >>stdio -vnc :7 -name primary -cpu qemu64,+kvmclock -device piix3-usb-uhci
> >>-device usb-tablet -netdev tap,id=hn0,vhost=off -device
> >>virtio-net-pci,id=net-pci0,netdev=hn0 -drive
> >>if=virtio,id=colo-disk0,driver=quorum,read-pattern=fifo,vote-threshold=1,children.0.file.filename=/mnt/sdd/pure_IMG/linux/redhat/rhel_6.5_64_2U_ide,children.0.driver=raw
> >>Secondary side:
> >>#x86_64-softmmu/qemu-system-x86_64 -boot c -m 2048 -smp 2 -qmp stdio -vnc
> >>:7 -name secondary -enable-kvm -cpu qemu64,+kvmclock -device piix3-usb-uhci
> >>-device usb-tablet -netdev tap,id=hn0,vhost=off -device
> >>virtio-net-pci,id=net-pci0,netdev=hn0 -drive
> >>if=none,id=colo-disk0,file.filename=/mnt/sdd/pure_IMG/linux/redhat/rhel_6.5_64_2U_ide,driver=raw,node-name=node0
> >> -drive
> >>if=virtio,id=active-disk0,throttling.bps-total=7000,driver=replication,mode=secondary,file.driver=qcow2,file.file.filename=/mnt/ramfs/active_disk.img,file.backing.driver=qcow2,file.backing.file.filename=/mnt/ramfs/hidden_disk.img,file.backing.backing=colo-disk0
> >> -incoming tcp:0:
> >>2. On Secondary VM's QEMU monitor, issue command
> >>{'execute':'qmp_capabilities'}
> >>{'execute': 'nbd-server-start', 'arguments': {'addr': {'type': 'inet',
> >>'data': {'host': '192.168.2.88', 'port': '8889'} } } }
> >>{'execute': 'nbd-server-add', 'arguments': {'device': 'colo-disk0',
> >>'writable': true } }
> >>{'execute': 'trace-event-set-state', 'arguments': {'name': 'colo*',
> >>'enable': true} }
> >>
> >>3. On Primary VM's QEMU monitor, issue command:
> >>{'execute':'qmp_capabilities'}
> >>{'execute': 'human-monitor-command', 'arguments': {'command-line':
> >>'drive_add buddy
> >>driver=replication,mode=primary,file.driver=nbd,file.host=9.61.1.7,file.port=8889,file.export=colo-disk0,node-name=node0,if=none'}}
> >>{'execute':'x-blockdev-change', 'arguments':{'parent': 'colo-disk0',
> >>'node': 'node0' } }
> >>{'execute': 'migrate-set-capabilities', 'arguments': {'capabilities': [
> >>{'capability': 'x-colo', 'state': true } ] } }
> >>{'execute': 'migrate', 'arguments': {'uri': 'tcp:192.168.2.88:' } }
> >>
> >>4. After the above steps, you will see, whenever you make changes to PVM,
> >>SVM will be synced.
> >>You can by issue command '{ "execute": "migrate-set-parameters" ,
> >>"arguments":{ "x-checkpoint-delay": 2000 } }'
> >>to change the checkpoint period time.
> >>
> >>5. Failover test
> >>You can kill Primary VM and run 'x_colo_lost_heartbeat' in Secondary VM's
> >>monitor at the same time, then SVM will failover and client will not feel
> >>this
> >>change.
> >>
> >>Before issuing '{ "execute": "x-colo-lost-heartbeat" }' command, we have to
> >>issue block related command to stop block replication.
> >>Primary:
> >> Remove the nbd child from the quorum:
> >> { 'execute': 'x-blockdev-change', 'arguments': {'parent': 'colo-disk0',
> >> 'child': 'children.1'}}
> >> Note: there is no qmp command to remove the blockdev now
> >>
> >>Secondary:
> >> The primary host is down, so we should do the following thing:
> >> { 'execute': 'nbd-server-stop' }
> >>
> >>Please review, thanks.
> >>
> >>TODO:
> >>1. Implement packets compare module (proxy) in qemu (Doing)
> >>2. Checkpoint based on proxy in qemu
> >>3. The capability of continuous FT
> >>
> >>v12:
> >> - Fix the bug that default buffer filter broken vhost-net.
> >> - Add an flag in struct NetFilterState to help skipping default
> >> filter for packets travelling through filter layer.
> >> - Remove the default failover treatment which may cause split-brain.
> >
Re: [Qemu-devel] [PATCH] virtio: use smp_load_acquire/smp_store_release
On Thu, Dec 17, 2015 at 12:29:03PM +0200, Michael S. Tsirkin wrote:
> +static inline __virtio16 virtio_load_acquire(bool weak_barriers, __virtio16
> *p)
> +{
> + if (!weak_barriers) {
> + rmb();
> + return READ_ONCE(*p);
> + }
> +#ifdef CONFIG_SMP
> + return smp_load_acquire(p);
> +#else
> + dma_rmb();
> + return READ_ONCE(*p);
> +#endif
> +}
This too is wrong. Look for example at arm.
dma_rmb() is dmb(osh), while the smp_mb() used by smp_load_acquire() is
dmb(ish). They order completely different types of memory accesses.
Also, load_acquire() is first load, then barrier, and an ACQUIRE barrier
at that, not a READ barrier.
So your #else branch should look something like:
var = READ_ONCE(*p);
dma_mb();
return var;
Re: [Qemu-devel] [PULL 0/5] usb: ehci idt fix, event support for mtp
On 15 December 2015 at 10:02, Gerd Hoffmann wrote: > Hi, > > Qemu 2.5 is almost out of the door, /me starts picking up patches which > had to wait due to the freeze, to clear my queues before I disappear > into my xmas family vacation next week. > > First is the usb patch queue, featuring mtp event support: your guest is > notified about changes in the file tree. Also a cve fix for ehci (low > impact, DoS). > > cheers, > Gerd > > The following changes since commit f05b42d3fd30bb9673cc1ac1ee8c2af8f669964e: > > Update version for v2.5.0-rc4 release (2015-12-11 16:37:55 +) > > are available in the git repository at: > > git://git.kraxel.org/qemu tags/pull-usb-20151215-1 > > for you to fetch changes up to 156a2e4dbffa85997636a7a39ef12da6f1b40254: > > ehci: make idt processing more robust (2015-12-15 09:49:03 +0100) > > > usb: ehci idt fix, event support for mtp > Applied, thanks. -- PMM
Re: [Qemu-devel] [PATCH v5 5/6] xlnx-zynqmp: Connect the SPI devices
On 17 December 2015 at 10:28, Paolo Bonzini wrote: > > > On 17/12/2015 09:26, Peter Maydell wrote: >>> > In any case, I would prefer qdev_bus_rename to stay in xlnx-zynqmp.c. >> I disagree that it should be in xlnx-zynqmp.c. Either >> (a) qdev already provides some reasonable mechanism for >> SoC container like this to allow their users to get at >> buses provided by their child objects, in which case we >> should use it >> (b) qdev doesn't provide such a mechanism, in which case >> we need to provide one (either qdev_bus_rename or something >> else if you have a better idea) >> >> But we definitely shouldn't have the SoC container code >> messing around with the internals of the qdev objects. > > It's a hack and I don't want it to become a sanctioned way to do it. > It's already messing around pretty heavily with qdev internals, see the > line right after QLIST_INSERT_HEAD: > > QLIST_INSERT_HEAD(&dev->child_bus, spi_bus, sibling); Well, that doesn't look good either. I think my point still stands -- we should be providing proper infrastructure at the qdev level to allow SoC container devices to do the things they need to do, not just letting the containers mess with the qdev internals. thanks -- PMM
Re: [Qemu-devel] [PATCH v5 5/6] xlnx-zynqmp: Connect the SPI devices
On 17/12/2015 12:11, Peter Maydell wrote: > > It's a hack and I don't want it to become a sanctioned way to do it. > > It's already messing around pretty heavily with qdev internals, see the > > line right after QLIST_INSERT_HEAD: > > > > QLIST_INSERT_HEAD(&dev->child_bus, spi_bus, sibling); > > Well, that doesn't look good either. I think my point still > stands -- we should be providing proper infrastructure at > the qdev level to allow SoC container devices to do the > things they need to do, not just letting the containers > mess with the qdev internals. I agree completely. Paolo
Re: [Qemu-devel] [PATCH] virtio_ring: use smp_store_mb
On Thu, Dec 17, 2015 at 12:32:53PM +0200, Michael S. Tsirkin wrote:
> Seems to give a speedup on my box but I'm less sure about this one. E.g. as
> xchng faster than mfence on all/most intel CPUs? Anyone has an opinion?
Would help if you Cc people who would actually know this :-)
Yes, we've recently established that xchg is indeed faster than mfence
on at least recent machines, see:
lkml.kernel.org/r/ca+55afynbkeuugs9s-q+fly6merba6mjeywwbbe7a5aaqsa...@mail.gmail.com
> +static inline void virtio_store_mb(bool weak_barriers,
> +__virtio16 *p, __virtio16 v)
> +{
> +#ifdef CONFIG_SMP
> + if (weak_barriers)
> + smp_store_mb(*p, v);
> + else
> +#endif
> + {
> + WRITE_ONCE(*p, v);
> + mb();
> + }
> +}
Note that virtio_mb() is weirdly inconsistent with virtio_[rw]mb() in
that they use dma_* ops for weak_barriers, while virtio_mb() uses
smp_mb().
As previously stated, smp_mb() does not cover the same memory domains as
dma_mb() would.
Re: [Qemu-devel] [PATCH v6 0/5] Add an i.MX25 specific CCM driver
On 7 December 2015 at 22:53, Jean-Christophe Dubois wrote: > i.MX25 SOC has a different CCM device than i.MX31. > > Qemu i.MX25 emulation was built with i.MX31 CCM driver. This allows > Linux to work on top of the i.MX25 emultion but this is not correct. > > Furthermore, other SOC we could emulate like i.MX6 have yet a different > implementation of the CCM device. > > So we split the i.MX31 into a generic base class and a specific i.MX31 > class. > > We then add an i.MX25 specific CCM Device and have the i.MX25 SOC use it. > > Jean-Christophe Dubois (5): > i.MX: Fix i.MX31 default/reset configuration. > i.MX: rename i.MX CCM get_clock() function and CLK ID enum names > i.MX: Split the CCM class into an abstact base class and a concrete > class. > i.MX: Add an i.MX25 specific CCM class/instance. > i.MX: move i.MX31 CCM object to register array. At Peter C's suggestion I've applied 1-4 to target-arm.next; 5 is still awaiting Peter's review. thanks -- PMM
Re: [Qemu-devel] [PULL 0/5] coreaudio: use new-in-OSX-10.6 APIs, cleanups.
On 15 December 2015 at 10:18, Gerd Hoffmann wrote: > Hi, > > Next in line: coreaudio modernization. Use newer APIs id possible, > some cleanups. Builds fine on my linux box, so it must be good ;) > > please pull, > Gerd > > The following changes since commit f05b42d3fd30bb9673cc1ac1ee8c2af8f669964e: > > Update version for v2.5.0-rc4 release (2015-12-11 16:37:55 +) > > are available in the git repository at: > > git://git.kraxel.org/qemu tags/pull-audio-20151215-1 > > for you to fetch changes up to 2f79a18fdd6e8e75ab9bd4edc90a641ab730824d: > > audio/coreaudio.c: Avoid deprecated AudioDeviceAdd/RemoveIOProc APIs > (2015-12-15 11:08:12 +0100) > > > coreaudio: use new-in-OSX-10.6 APIs, cleanups. > > Applied, thanks. -- PMM
Re: [Qemu-devel] proposal: drop support for OSX 10.5 hosts from QEMU 2.6
On 28 November 2015 at 22:22, Alexander Graf wrote: >> Am 28.11.2015 um 23:10 schrieb Peter Maydell : >> My suggested plan would be: >> * in the 2.5 release notes, announce that support for OSX 10.5 and PPC >> hosts is deprecated and will be removed from QEMU 2.6 unless somebody >> steps forward to help with testing >> * at some point probably a little before 2.6 softfreeze, remove the >> now unnecessary ifdeffery >> >> Any disagreements? Am I wrong about nobody testing QEMU on 10.5? > > I think that sounds reasonable :) I have added a note to 2.5's ChangeLog to say we're going to drop 10.5 support unless anybody comes forward to help with the testing. thanks -- PMM
