Re: [Qemu-block] [Qemu-devel] storing machine data in qcow images?

[Eduardo Habkost]

On Wed, May 23, 2018 at 01:19:46PM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabk...@redhat.com> writes:
> 
> > On Mon, May 21, 2018 at 07:44:40PM +0100, Daniel P. Berrangé wrote:
> >> On Mon, May 21, 2018 at 03:29:28PM -0300, Eduardo Habkost wrote:
> >> > On Sat, May 19, 2018 at 08:05:06AM +0200, Markus Armbruster wrote:
> >> > > Eduardo Habkost <ehabk...@redhat.com> writes:
> >> > > 
> >> > > [...]
> >> > > > About being more expressive than just a single list of key,value
> >> > > > pairs, I don't see any evidence of that being necessary for the
> >> > > > problems we're trying to address.
> >> > > 
> >> > > Short history of a configuration format you might have encountered:
> >> 
> >> [snip]
> >> 
> >> > > How confident are we a single list of (key, value) is really all we're
> >> > > going to need?
> >> > > 
> >> > > Even if we think it is, would it be possible to provide for a future
> >> > > extension to trees at next to no cost?
> >> > 
> >> > I'm confident that a list of key,values is all we need for the
> >> > current problem.
> >> 
> >> I'm not convinced. A disk image may work with Q35 or i440fx,  or
> >> work with any of virtio, ide or sata disk. So that already means
> >> values have to be arrays, not scalars. You could do that with a
> >> simple key,value list, but only by defining a mapping of arrays
> >> into a flattened form. eg do we allow repeated keys, or do we
> >> allow array indexes on keys. 
> >
> > No problem, we can support trees if it's necessary.
> >
> >
> >> > The point here is to allow users to simply copy an existing disk
> >> > image, and it will contain enough hints for a cloud stack to
> >> > choose reasonable defaults for machine-type and disk type
> >> > automatically.  Requiring the user to perform a separate step to
> >> > encapsulate the disk image in another file format defeats the
> >> > whole purpose of the proposal.
> >> 
> >> It doesn't have to mean more work for the user - the application
> >> that is used to create the image can do that on their behalf.
> >> oVirt for example can import/export OVA files, containing OVF
> >> metadata. I could imagine virt-manager, and other tools adding
> >> export ability without much trouble if this was deemed a desirable
> >> thing. Bundling gives ability to have multiple disk images in one
> >> archive, which is something OVF does.
> >
> > I have the impression that "the application that is used to
> > create the image" is a very large set.  It can be virt-manager,
> > virt-install, virt-manager, or even QEMU itself.
> >
> > Today people can simply create a VM on virt-manager, or run QEMU
> > manually, and upload the qcow2 image directly from its original
> > location (they don't need to copy/export it).  Don't we want the
> > same procedure to keep working instead of requiring users to use
> > another tool?
> 
> Today, I can take the disk out of my old computer, put it into my new
> computer, and it just works.  Don't we want the same procedure to keep
> working forever?

I don't think the comparison is fair: downloading hard disk
images for bare metal hardware is not as common as downloading
guest disk images for cloud infrastructure.

> 
> Sadly, wanting something badly enough doesn't make actual solutions any
> easier :)

This part is true.  :)

> 
> My point is: disk images (real or virtual) keep working in different
> hardware contexts by a mixture of flexibility built into system software
> on the image, disciplined evolution of hardware (real or virtual), and
> dumb luck.  It works until it doesn't.  And then you get to tinker.

Personally, I believe that tools for running and managing virtual
hardware can and should be smarter than real hardware.

> 
> With OVF, you solve the problem further up the stack: you do virtual
> appliances instead of disk images.
> 

I guess the main problem is that people are already using disk
images as if they were virtual appliances.

We can tell people to stop doing that and use OVF, but then we
won't make anybody's life any easier: publishers of images might
need to generate both qcow2 and OVF images if they want it to
work with older hosts; consumers will need to find out if they
need qcow2 or OVF.

But I work too deep down the stack to tell if it's really
important to avoid these problems or not.  And as you said, this
doesn't make actual solutions any easier.


> How much space that leaves for useful solutions at the level of QEMU I
> can't say.

I have no doubt about "useful", but I'm not sure about
"important".

I guess the question is if we have people with time and resources
to work on solutions (whether using qcow2 or OVF).

-- 
Eduardo

Re: [Qemu-block] [Qemu-devel] storing machine data in qcow images?

[Eduardo Habkost]

On Tue, May 22, 2018 at 05:02:21PM +0200, Kevin Wolf wrote:
> Am 22.05.2018 um 16:19 hat Michael S. Tsirkin geschrieben:
> > On Tue, May 22, 2018 at 09:35:55AM +0200, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > > You must /sometimes/ supply the correct machine type.
> > > > 
> > > > It is quite dependent on the guest OS you have installed, and even
> > > > just how the guest OS is configured.  In general Linux is very
> > > > flexible and can adapt to a wide range of hardware, automatically
> > > > detecting things as needed. It is possible for a sysadmin to build
> > > > a Linux image in a way that would only work with I440FX, but I
> > > > don't think it would be common to see that.
> > > 
> > > I think it would be pretty hard to actually build such an image.
> > > 
> > > The more critical thing for linux guests is the storage driver which
> > > must be included into the initrd so the image can mount the root
> > > filesystem.  And the firmware, bios vs. uefi is more critical than
> > > pc vs. q35.
> > 
> > I think we can start by finding a location to embed a string in a qcow
> > image, add ability for qemu-img to set and get this string.  We can
> > discuss how it's formatted separately.
> 
> If we want it, we'll find a place to store it.
> 
> But the first thing we need is a spec for what's actually in it. Just
> storing a machine type hint would be a one-off hack that wouldn't last
> very long before we want to add the next thing.
> 
> Essentially, what we need is a description of the virtual machine that
> we suggest to use with this image. We can try to reuse something
> existing there, like libvirt XML or OVF, or invent something new (a JSON
> array describing runtime options?). One difference to existing formats
> is probably that we want only frontends and no backends in the
> description.

The OVF virtual hardware description might be appropriate to
define what's required vs what's recommended, to support multiple
machine-types, and ranges of valid values for variables.

Pro: management software can reuse exactly the same logic for
qcow2 machine descriptions and OVF machine descriptions.

Con: OVF is a pretty complex specification.

-- 
Eduardo

Re: [Qemu-block] [Qemu-devel] storing machine data in qcow images?

[Eduardo Habkost]

On Tue, May 22, 2018 at 09:35:55AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > You must /sometimes/ supply the correct machine type.
> > 
> > It is quite dependent on the guest OS you have installed, and even
> > just how the guest OS is configured.  In general Linux is very
> > flexible and can adapt to a wide range of hardware, automatically
> > detecting things as needed. It is possible for a sysadmin to build
> > a Linux image in a way that would only work with I440FX, but I
> > don't think it would be common to see that.
> 
> I think it would be pretty hard to actually build such an image.
> 
> The more critical thing for linux guests is the storage driver which
> must be included into the initrd so the image can mount the root
> filesystem.  And the firmware, bios vs. uefi is more critical than
> pc vs. q35.
> 
> > That said I'm not really convinced that using the qcow2 headers is
> > a good plan. We have many disk image formats in common use, qcow2
> > is just one. Even if the user provides the image in qcow2 format,
> > that doesn't mean that mgmt apps actually store the qcow2 file.
> 
> > I tend to think we'd be better looking at what we can do in the context
> > of an existing standard like OVF rather than inventing something that
> > only works with qcow2. I think it would need to be more expressive than
> > just a single list of key,value pairs for each item.
> 
> Embed OVF metadata in the qcow2 image?

I'm all for using the same standard for specifying machine hints
on both cases.

Now, is there an existing mechanism for virtual hardware hints
(not requirements) in OVF, or we have to invent one?

-- 
Eduardo

Re: [Qemu-block] [Qemu-devel] storing machine data in qcow images?

[Eduardo Habkost]

On Mon, May 21, 2018 at 09:18:17PM +0100, Daniel P. Berrangé wrote:
> On Fri, May 18, 2018 at 02:41:33PM -0300, Eduardo Habkost wrote:
> > On Fri, May 18, 2018 at 06:09:56PM +0100, Daniel P. Berrangé wrote:
> > > On Fri, May 18, 2018 at 06:30:38PM +0300, Michael S. Tsirkin wrote:
> > > > Hi!
> > > > Right now, QEMU supports multiple machine types within
> > > > a given architecture. This was the case for many architectures
> > > > (like ARM) for a while, somewhat more recently this is the case
> > > > for x86 with I440FX and Q35 options.
> > > > 
> > > > Unfortunately this means that it's no longer possible
> > > > to more or less reliably boot a VM just given a disk image,
> > > > even if you select the correct QEMU binary:
> > > > you must supply the correct machine type.
> > > 
> > > You must /sometimes/ supply the correct machine type.
> > > 
> > > It is quite dependent on the guest OS you have installed, and even
> > > just how the guest OS is configured.  In general Linux is very
> > > flexible and can adapt to a wide range of hardware, automatically
> > > detecting things as needed. It is possible for a sysadmin to build
> > > a Linux image in a way that would only work with I440FX, but I
> > > don't think it would be common to see that. Many distros build
> > > and distribute disk images that can work across VMWare, KVM,
> > > and VirtualBox which all have very quite different hardware.
> > > Non-x86 archs may be more fussy but I don't have personal
> > > experiance with them
> > > 
> > > Windows is probably where things get more tricky, as it is not
> > > happy with disks moving between different controller types
> > > for example, and you might trigger license activation again.
> > 
> > All I'm suggesting here is just adding extra hints that OpenStack
> > can use.
> > 
> > I have very specific goal here: the goal is to make it less
> > painful to users when OpenStack+libvirt+QEMU switch to using a
> > different machine-type by default (q35), and/or when guest OSes
> > stop supporting pc-i440fx.  I assume this is a goal for OpenStack
> > as well.
> > 
> > We can make the solution to be more extensible and solve other
> > problems as well, but my original goal is the one above.
> 
> Configuring the machine type is just one thing that users
> would do with OpenStack though.  A simple example might be
> 
> openstack image set \
>  --property hw_disk_bus=scsi \
>--property hw_vif_model=e1000e
> 
> Or if they're using libosinfo to set preferred devices 
> 
> openstack image set \
>  --property os_distro=fedora26
> 
> which will identify virtio-blk & virtio-net as disk+nic
> respectively. Using libosinfo is more flexible than setting
> the hw_disk_bus & hw_vif_model  explicitly, because libosinfo
> will report multiple devices that can be used, and the virt
> driver can then pick one which best suits the particular
> host or hypervisor.
> 
> Setting a non-default machine type is one extra prop
> 
> openstack image set \
>  --property hw_machine_type=q35
>  --property os_distro=fedora26

Nice.  Are these just hypothetical examples, or something that
already works?


> 
> So while your immediate motivation is only considering the
> machine type, from the Openstack POV thats only one property
> out of many that users might be setting.

Agreed.


> > > That said I'm not really convinced that using the qcow2 headers is
> > > a good plan. We have many disk image formats in common use, qcow2
> > > is just one. Even if the user provides the image in qcow2 format,
> > > that doesn't mean that mgmt apps actually store the qcow2 file.
> > > 
> > 
> > Why this OpenStack implementation detail matters?  Once the hints
> > are included in the input, it's up to OpenStack to choose how to
> > deal with it.
> 
> Well openstack aims to support multiple hypervisors - if there's a
> choice between implementing something that is a cross-vendor standard
> like OVF, or implementing something that only works with qcow2, the
> latter is not very appealing to support.

I still don't understand why you claim this would only work with
qcow2.  If somebody wants to implement the same functionality in
OVF, it's also possible.


> > > The closest to a cross-hypervisor standard is OVF which can store
> > > metadata about required hardware for a VM. I'm pretty sure it does
> > > not have the concept of machine types, but ma

Re: [Qemu-block] [Qemu-devel] storing machine data in qcow images?

[Eduardo Habkost]

On Mon, May 21, 2018 at 07:44:40PM +0100, Daniel P. Berrangé wrote:
> On Mon, May 21, 2018 at 03:29:28PM -0300, Eduardo Habkost wrote:
> > On Sat, May 19, 2018 at 08:05:06AM +0200, Markus Armbruster wrote:
> > > Eduardo Habkost <ehabk...@redhat.com> writes:
> > > 
> > > [...]
> > > > About being more expressive than just a single list of key,value
> > > > pairs, I don't see any evidence of that being necessary for the
> > > > problems we're trying to address.
> > > 
> > > Short history of a configuration format you might have encountered:
> 
> [snip]
> 
> > > How confident are we a single list of (key, value) is really all we're
> > > going to need?
> > > 
> > > Even if we think it is, would it be possible to provide for a future
> > > extension to trees at next to no cost?
> > 
> > I'm confident that a list of key,values is all we need for the
> > current problem.
> 
> I'm not convinced. A disk image may work with Q35 or i440fx,  or
> work with any of virtio, ide or sata disk. So that already means
> values have to be arrays, not scalars. You could do that with a
> simple key,value list, but only by defining a mapping of arrays
> into a flattened form. eg do we allow repeated keys, or do we
> allow array indexes on keys. 

No problem, we can support trees if it's necessary.


> > The point here is to allow users to simply copy an existing disk
> > image, and it will contain enough hints for a cloud stack to
> > choose reasonable defaults for machine-type and disk type
> > automatically.  Requiring the user to perform a separate step to
> > encapsulate the disk image in another file format defeats the
> > whole purpose of the proposal.
> 
> It doesn't have to mean more work for the user - the application
> that is used to create the image can do that on their behalf.
> oVirt for example can import/export OVA files, containing OVF
> metadata. I could imagine virt-manager, and other tools adding
> export ability without much trouble if this was deemed a desirable
> thing. Bundling gives ability to have multiple disk images in one
> archive, which is something OVF does.

I have the impression that "the application that is used to
create the image" is a very large set.  It can be virt-manager,
virt-install, virt-manager, or even QEMU itself.

Today people can simply create a VM on virt-manager, or run QEMU
manually, and upload the qcow2 image directly from its original
location (they don't need to copy/export it).  Don't we want the
same procedure to keep working instead of requiring users to use
another tool?

-- 
Eduardo

Re: [Qemu-block] [Qemu-devel] storing machine data in qcow images?

[Eduardo Habkost]

On Sat, May 19, 2018 at 08:05:06AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabk...@redhat.com> writes:
> 
> [...]
> > About being more expressive than just a single list of key,value
> > pairs, I don't see any evidence of that being necessary for the
> > problems we're trying to address.
> 
> Short history of a configuration format you might have encountered:
> 
> 1. A couple of (key, value) is all we ne need for the problems we're
> trying to address.  (v0.4, 2003)
> 
> 2.1. I got this one special snowflake problem where I actually need a few
> related values.  Fortunately, this little ad hoc parser can take apart
> the key's single value easily.  (ca. v0.8, 2005)
> 
> ...
> 
> 2.n. Snowflakes are surprisingly common, but fortunately one more little
> ad hoc parser can't hurt.
> 
> 3. Umm, this is getting messy.  Let's have proper infrastructure for
> two-level keys.  Surely two levels are all we ne need for the problems
> we're trying to address.  Fortunately, we can bolt them on without too
> much trouble.  (v0.12, 2009)
> 
> 4. Err, trees, I'm afraid we actually need trees.  Fortunately, we can
> hack them into the existing two-level infrastructure without too much
> trouble.  (v1.3, 2013)
> 
> 5. You are in a maze of twisting little passages, all different.
> (today)
> 
> 
> How confident are we a single list of (key, value) is really all we're
> going to need?
> 
> Even if we think it is, would it be possible to provide for a future
> extension to trees at next to no cost?

I'm confident that a list of key,values is all we need for the
current problem.

I also agree that being possible to represent trees is a good
idea, and it would probably have next to no cost.

But I disagree if the point here is "we will eventually need much
more complex data in the future, so let's require users to move
to OVF instead".

The point here is to allow users to simply copy an existing disk
image, and it will contain enough hints for a cloud stack to
choose reasonable defaults for machine-type and disk type
automatically.  Requiring the user to perform a separate step to
encapsulate the disk image in another file format defeats the
whole purpose of the proposal.

-- 
Eduardo

Re: [Qemu-block] [Qemu-devel] storing machine data in qcow images?

[Eduardo Habkost]

On Fri, May 18, 2018 at 06:09:56PM +0100, Daniel P. Berrangé wrote:
> On Fri, May 18, 2018 at 06:30:38PM +0300, Michael S. Tsirkin wrote:
> > Hi!
> > Right now, QEMU supports multiple machine types within
> > a given architecture. This was the case for many architectures
> > (like ARM) for a while, somewhat more recently this is the case
> > for x86 with I440FX and Q35 options.
> > 
> > Unfortunately this means that it's no longer possible
> > to more or less reliably boot a VM just given a disk image,
> > even if you select the correct QEMU binary:
> > you must supply the correct machine type.
> 
> You must /sometimes/ supply the correct machine type.
> 
> It is quite dependent on the guest OS you have installed, and even
> just how the guest OS is configured.  In general Linux is very
> flexible and can adapt to a wide range of hardware, automatically
> detecting things as needed. It is possible for a sysadmin to build
> a Linux image in a way that would only work with I440FX, but I
> don't think it would be common to see that. Many distros build
> and distribute disk images that can work across VMWare, KVM,
> and VirtualBox which all have very quite different hardware.
> Non-x86 archs may be more fussy but I don't have personal
> experiance with them
> 
> Windows is probably where things get more tricky, as it is not
> happy with disks moving between different controller types
> for example, and you might trigger license activation again.

All I'm suggesting here is just adding extra hints that OpenStack
can use.

I have very specific goal here: the goal is to make it less
painful to users when OpenStack+libvirt+QEMU switch to using a
different machine-type by default (q35), and/or when guest OSes
stop supporting pc-i440fx.  I assume this is a goal for OpenStack
as well.

We can make the solution to be more extensible and solve other
problems as well, but my original goal is the one above.

> 
> 
> > Some guests go even further and require specific devices to be present.
> > 
> > Would it be reasonable to support storing this information in the qcow
> > image itself?  For example, I can see it following immediately the
> > backing file path within the image.
> 
> The backing file string needs to go in space between the end of headers
> and start of first cluster, and the spec explicitly says nothing else
> must be stored there. Also we can already hit the length limit on the
> backing file.
> 
> There would need to be an explicit header extension defined with its
> own clusters allocated instead.

This sounds correct.


> 
> That said I'm not really convinced that using the qcow2 headers is
> a good plan. We have many disk image formats in common use, qcow2
> is just one. Even if the user provides the image in qcow2 format,
> that doesn't mean that mgmt apps actually store the qcow2 file.
> 

Why this OpenStack implementation detail matters?  Once the hints
are included in the input, it's up to OpenStack to choose how to
deal with it.


> For example in some deployments OpenStack will immediately
> convert the image to raw for storage in an RBD volume as it is
> uploaded to Glance. So the glance image store would need to
> have a way to extract & save the info at time of upload. OpenStack
> targets multiple hypervisors though, so I'm not sure they would
> welcome something that is specific to just qcow2 in this area.
> 

I don't get the "something that is specific to just qcow2" part.
Adding extra info to qcow2 doesn't prevent other file formats
from carrying the same information as well.


> The closest to a cross-hypervisor standard is OVF which can store
> metadata about required hardware for a VM. I'm pretty sure it does
> not have the concept of machine types, but maybe it has a way for
> people to define metadata extensions. Since it is just XML at the
> end of the day, even if there was nothing official in OVF, it would
> be possible to just define a custom XML namespace and declare a
> schema for that to follow.

There's nothing preventing OVF from supporting the same kind of
hints.

I just don't think we should require people to migrate to OVF if
all they need is to tell OpenStack what's the recommended
machine-type for a guest image.

Requiring a different image format seems very likely to not
fulfill the goal I stated above: it will require using different
tools to create the guest images, and we can't force everybody
publishing guest images to stop using qcow2.

> 
> 
> > As Eduardo pointed out off-list, the format could be a set of key-value
> > pairs. Initially qemu-img could gain ability to retrieve and manipulate
> > these. Down the road we could teach qemu to use them automatically.
> > We could also thinkably warn the user, or drop the image from the boot
> > order.
> > 
> > Reasonable (IMO) things we could store in such a section:
> > - qemu architecture to use with the image
> > - machine type
> 
> A concern is about what you actually put here. We could easily create a
> situation 

Re: [Qemu-block] storing machine data in qcow images?

[Eduardo Habkost]

On Fri, May 18, 2018 at 06:30:38PM +0300, Michael S. Tsirkin wrote:
> Hi!
> Right now, QEMU supports multiple machine types within
> a given architecture. This was the case for many architectures
> (like ARM) for a while, somewhat more recently this is the case
> for x86 with I440FX and Q35 options.
> 
> Unfortunately this means that it's no longer possible
> to more or less reliably boot a VM just given a disk image,
> even if you select the correct QEMU binary:
> you must supply the correct machine type.
> 
> Some guests go even further and require specific devices to be present.
> 
> Would it be reasonable to support storing this information in the qcow
> image itself?  For example, I can see it following immediately the
> backing file path within the image.
> 
> As Eduardo pointed out off-list, the format could be a set of key-value
> pairs. Initially qemu-img could gain ability to retrieve and manipulate
> these. Down the road we could teach qemu to use them automatically.
> We could also thinkably warn the user, or drop the image from the boot
> order.

Some additional context:

Currently OpenStack and other management stacks support importing
"guest images", that are often just qcow2 disk images.  Today all
management stacks suppose x86 guest images all work using
pc-i440fx, but this is likely to change with newer guest OS
versions.

Right now it's very convenient for users to simply create disk
images using whatever VM management tools they have (e.g.
virt-image, virt-install, virsh) to install and configure a
guest, and all they need to do is to upload the resulting disk
image.

If information about the machine-type and disk type used to
create the VM is saved in the disk image, OpenStack and other
management stacks can use this information as hints to choose the
right machine-type for a given guest image.  This would also help
the system detect mistakes like using an image for the wrong
architecture.

I don't think QEMU needs to use this information automatically,
necessarily.  I think the first step is to simply make QEMU save
this information in the disk image, and making qemu-img able to
read and write this information.

> 
> Reasonable (IMO) things we could store in such a section:
> - qemu architecture to use with the image
> - machine type

Maybe just the machine-type family would be enough?

> 
> more possibilities:
> - required cpu flags
> - expected frontend devices
> - kernel flags for device tree based guests

All these might be useful in some cases.  I think it's important
to highlight that these would be just hints for systems importing
the disk image, and not mandatory.

> 
> Security considerations
> - If there is a machine type specific security issue,
>   this makes it easier to trick user to hitting it.
>   Not sure how common this is.

Yeah, we need to keep this in mind for every hint we add to this
system.

I would prefer a system with a very limited set of
possible input values, to avoid transforming this into a new
attack vector.

For example, I think the hint needs to specify: only the
machine-type family instead of the full machine-type version;
only expected NIC model instead of NIC model + mac address + PCI
address; only the CPU architecture instead of CPU model name +
flags.

(But "guest kernel flags" seems acceptable, because it's parsed
only by guest code.)

If any management stack requires a more detailed VM description,
they won't be covered by this system, and they can't expect qcow2
disk images to carry all the information they need.


> - We most likely shouldn't get backend parameters from the image

Agreed.

-- 
Eduardo

[Qemu-block] [RFC 10/10] python: futurize -f lib2to3.fixes.fix_numliterals

[Eduardo Habkost]

Convert octal literals into the new syntax.

This is necessary for Python 3 compatibility.

Done using:

  $ py=$( (g grep -l -E '^#!.*python';find -name '*.py' -printf '%P\n';) | \
sort -u | grep -v README.sh4)
  $ futurize -w -f lib2to3.fixes.fix_numliterals $py

Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 scripts/qmp/qom-fuse   |  6 +++---
 tests/qemu-iotests/118 | 24 
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/scripts/qmp/qom-fuse b/scripts/qmp/qom-fuse
index b00cb0a0af..e524e798fc 100755
--- a/scripts/qmp/qom-fuse
+++ b/scripts/qmp/qom-fuse
@@ -90,7 +90,7 @@ class QOMFS(Fuse):
 
 def getattr(self, path):
 if self.is_link(path):
-value = posix.stat_result((0755 | stat.S_IFLNK,
+value = posix.stat_result((0o755 | stat.S_IFLNK,
self.get_ino(path),
0,
2,
@@ -101,7 +101,7 @@ class QOMFS(Fuse):
0,
0))
 elif self.is_object(path):
-value = posix.stat_result((0755 | stat.S_IFDIR,
+value = posix.stat_result((0o755 | stat.S_IFDIR,
self.get_ino(path),
0,
2,
@@ -112,7 +112,7 @@ class QOMFS(Fuse):
0,
0))
 elif self.is_property(path):
-value = posix.stat_result((0644 | stat.S_IFREG,
+value = posix.stat_result((0o644 | stat.S_IFREG,
self.get_ino(path),
0,
1,
diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118
index a0469b570e..ff3b2ae3e7 100755
--- a/tests/qemu-iotests/118
+++ b/tests/qemu-iotests/118
@@ -390,14 +390,14 @@ class TestChangeReadOnly(ChangeBaseClass):
 
 def tearDown(self):
 self.vm.shutdown()
-os.chmod(old_img, 0666)
-os.chmod(new_img, 0666)
+os.chmod(old_img, 0o666)
+os.chmod(new_img, 0o666)
 os.remove(old_img)
 os.remove(new_img)
 
 def test_ro_ro_retain(self):
-os.chmod(old_img, 0444)
-os.chmod(new_img, 0444)
+os.chmod(old_img, 0o444)
+os.chmod(new_img, 0o444)
 self.vm.add_drive(old_img, 'media=disk,read-only=on', 'none')
 self.vm.add_device('floppy,drive=drive0,id=%s' % self.device_name)
 self.vm.launch()
@@ -417,7 +417,7 @@ class TestChangeReadOnly(ChangeBaseClass):
 self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
 
 def test_ro_rw_retain(self):
-os.chmod(old_img, 0444)
+os.chmod(old_img, 0o444)
 self.vm.add_drive(old_img, 'media=disk,read-only=on', 'none')
 self.vm.add_device('floppy,drive=drive0,id=%s' % self.device_name)
 self.vm.launch()
@@ -437,7 +437,7 @@ class TestChangeReadOnly(ChangeBaseClass):
 self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
 
 def test_rw_ro_retain(self):
-os.chmod(new_img, 0444)
+os.chmod(new_img, 0o444)
 self.vm.add_drive(old_img, 'media=disk', 'none')
 self.vm.add_device('floppy,drive=drive0,id=%s' % self.device_name)
 self.vm.launch()
@@ -459,7 +459,7 @@ class TestChangeReadOnly(ChangeBaseClass):
 self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
 
 def test_ro_rw(self):
-os.chmod(old_img, 0444)
+os.chmod(old_img, 0o444)
 self.vm.add_drive(old_img, 'media=disk,read-only=on', 'none')
 self.vm.add_device('floppy,drive=drive0,id=%s' % self.device_name)
 self.vm.launch()
@@ -480,7 +480,7 @@ class TestChangeReadOnly(ChangeBaseClass):
 self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
 
 def test_rw_ro(self):
-os.chmod(new_img, 0444)
+os.chmod(new_img, 0o444)
 self.vm.add_drive(old_img, 'media=disk', 'none')
 self.vm.add_device('floppy,drive=drive0,id=%s' % self.device_name)
 self.vm.launch()
@@ -521,7 +521,7 @@ class TestChangeReadOnly(ChangeBaseClass):
 self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
 
 def test_make_ro_rw(self):
-os.chmod(new_img, 0444)
+os.chmod(new_img, 0o444)
 self.vm.add_drive(old_img, 'media=disk', 'none')
 self.vm.add_device('floppy,drive=drive0,id=%s' % self.device_name)
 self.vm.launch()
@@ -542,7 +542,7 @@ class TestChangeReadOnly(ChangeBaseClass):
 self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
 
 def test_make_rw_ro_by_retain(self):
-os.chmod(old_img, 0444)
+os.chmod(old_img, 0o444)
 self.vm.add_drive(old_img, 'medi

[Qemu-block] [RFC 06/10] python: futurize -f lib2to3.fixes.fix_reduce

[Eduardo Habkost]

Handle the move of reduce() to functools.reduce().

This is necessary for Python 3 compatibility.

Done using:

  $ py=$( (g grep -l -E '^#!.*python';find -name '*.py' -printf '%P\n';) | \
sort -u | grep -v README.sh4)
  $ futurize -w -f lib2to3.fixes.fix_reduce $py

Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 tests/image-fuzzer/qcow2/fuzz.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/image-fuzzer/qcow2/fuzz.py b/tests/image-fuzzer/qcow2/fuzz.py
index 20eba6bc1b..abc4f0635d 100644
--- a/tests/image-fuzzer/qcow2/fuzz.py
+++ b/tests/image-fuzzer/qcow2/fuzz.py
@@ -17,6 +17,7 @@
 #
 
 import random
+from functools import reduce
 
 UINT8 = 0xff
 UINT16 = 0x
-- 
2.14.3

[Qemu-block] [RFC 05/10] python: futurize -f lib2to3.fixes.fix_standarderror

[Eduardo Habkost]

Rename StandardError to Exception.

This is necessary for Python 3 compatibility.

Done using:

  $ py=$( (g grep -l -E '^#!.*python';find -name '*.py' -printf '%P\n';) | \
sort -u | grep -v README.sh4)
  $ futurize -w -f lib2to3.fixes.fix_standarderror $py

Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 scripts/qmp/qemu-ga-client | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/qmp/qemu-ga-client b/scripts/qmp/qemu-ga-client
index 6045fcd3f2..976e69e05f 100755
--- a/scripts/qmp/qemu-ga-client
+++ b/scripts/qmp/qemu-ga-client
@@ -137,7 +137,7 @@ class QemuGuestAgentClient:
 
 def fsfreeze(self, cmd):
 if cmd not in ['status', 'freeze', 'thaw']:
-raise StandardError('Invalid command: ' + cmd)
+raise Exception('Invalid command: ' + cmd)
 
 return getattr(self.qga, 'fsfreeze' + '_' + cmd)()
 
@@ -146,7 +146,7 @@ class QemuGuestAgentClient:
 
 def suspend(self, mode):
 if mode not in ['disk', 'ram', 'hybrid']:
-raise StandardError('Invalid mode: ' + mode)
+raise Exception('Invalid mode: ' + mode)
 
 try:
 getattr(self.qga, 'suspend' + '_' + mode)()
@@ -157,7 +157,7 @@ class QemuGuestAgentClient:
 
 def shutdown(self, mode='powerdown'):
 if mode not in ['powerdown', 'halt', 'reboot']:
-raise StandardError('Invalid mode: ' + mode)
+raise Exception('Invalid mode: ' + mode)
 
 try:
 self.qga.shutdown(mode=mode)
-- 
2.14.3

[Qemu-block] [RFC 09/10] python: futurize -f lib2to3.fixes.fix_except

[Eduardo Habkost]

Convert "except X, T" to "except X as T".

This is necessary for Python 3 compatibility.

Done using:

  $ py=$( (g grep -l -E '^#!.*python';find -name '*.py' -printf '%P\n';) | \
sort -u | grep -v README.sh4)
  $ futurize -w -f lib2to3.fixes.fix_except $py

Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 scripts/simpletrace.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/simpletrace.py b/scripts/simpletrace.py
index 2658b5cc7c..d4a50a1e2b 100755
--- a/scripts/simpletrace.py
+++ b/scripts/simpletrace.py
@@ -45,7 +45,7 @@ def get_record(edict, idtoname, rechdr, fobj):
 rec = (name, rechdr[1], rechdr[3])
 try:
 event = edict[name]
-except KeyError, e:
+except KeyError as e:
 import sys
 sys.stderr.write('%s event is logged but is not declared ' \
  'in the trace events file, try using ' \
-- 
2.14.3

[Qemu-block] [RFC 07/10] python: futurize -f lib2to3.fixes.fix_tuple_params

[Eduardo Habkost]

Remove implicit tuple parameter unpacking.

This is necessary for Python 3 compatibility.

Done using:

  $ py=$( (g grep -l -E '^#!.*python';find -name '*.py' -printf '%P\n';) | \
sort -u | grep -v README.sh4)
  $ futurize -w -f lib2to3.fixes.fix_tuple_params $py

Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 scripts/analyse-locks-simpletrace.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/analyse-locks-simpletrace.py 
b/scripts/analyse-locks-simpletrace.py
index 352bc9c22d..30090bdfff 100755
--- a/scripts/analyse-locks-simpletrace.py
+++ b/scripts/analyse-locks-simpletrace.py
@@ -78,7 +78,7 @@ if __name__ == '__main__':
 
 # Now dump the individual lock stats
 for key, val in sorted(analyser.mutex_records.iteritems(),
-   key=lambda (k,v): v["locks"]):
+   key=lambda k_v: k_v[1]["locks"]):
 print ("Lock: %#x locks: %d, locked: %d, unlocked: %d" %
(key, val["locks"], val["locked"], val["unlocked"]))
 
-- 
2.14.3

[Qemu-block] [RFC 08/10] python: futurize -f lib2to3.fixes.fix_renames

[Eduardo Habkost]

Change sys.maxint to sys.maxsize.

This is necessary for Python 3 compatibility.

Done using:

  $ py=$( (g grep -l -E '^#!.*python';find -name '*.py' -printf '%P\n';) | \
sort -u | grep -v README.sh4)
  $ futurize -w -f lib2to3.fixes.fix_renames $py

Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 tests/image-fuzzer/runner.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/image-fuzzer/runner.py b/tests/image-fuzzer/runner.py
index 45e8fca63f..95d84f38f3 100755
--- a/tests/image-fuzzer/runner.py
+++ b/tests/image-fuzzer/runner.py
@@ -128,7 +128,7 @@ class TestEnv(object):
 if seed is not None:
 self.seed = seed
 else:
-self.seed = str(random.randint(0, sys.maxint))
+self.seed = str(random.randint(0, sys.maxsize))
 random.seed(self.seed)
 
 self.init_path = os.getcwd()
-- 
2.14.3

[Qemu-block] [RFC 01/10] python: futurize -f libfuturize.fixes.fix_print_with_import

[Eduardo Habkost]

Change all Python code to use print as a function.

This is necessary for Python 3 compatibility.

Done using:

  $ py=$( (g grep -l -E '^#!.*python';find -name '*.py' -printf '%P\n';) | \
sort -u | grep -v README.sh4)
  $ futurize -w -f libfuturize.fixes.fix_print_with_import $py

Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 scripts/analyse-9p-simpletrace.py| 89 
 scripts/analyse-locks-simpletrace.py |  1 +
 scripts/analyze-migration.py | 11 ++--
 scripts/dump-guest-memory.py |  1 +
 scripts/replay-dump.py   | 21 
 scripts/signrom.py   |  1 +
 scripts/simpletrace.py   |  3 +-
 scripts/vmstate-static-checker.py| 85 +++---
 scripts/device-crash-test|  3 +-
 scripts/kvm/kvm_flightrecorder   | 21 
 scripts/kvm/vmxcap   |  1 +
 scripts/qmp/qemu-ga-client   |  1 +
 scripts/qmp/qmp  | 17 +++---
 scripts/qmp/qmp-shell| 35 +++--
 scripts/qmp/qom-get  |  7 +--
 scripts/qmp/qom-list | 11 ++--
 scripts/qmp/qom-set  |  5 +-
 scripts/qmp/qom-tree | 11 ++--
 tests/docker/docker.py   | 11 ++--
 tests/docker/travis.py   | 15 +++---
 tests/guest-debug/test-gdbstub.py|  1 +
 tests/image-fuzzer/runner.py | 38 ++
 tests/migration/guestperf/engine.py  | 29 ++-
 tests/migration/guestperf/plot.py| 17 +++---
 tests/migration/guestperf/shell.py   | 19 +++
 tests/qemu-iotests/149   |  3 +-
 tests/qemu-iotests/165   |  3 +-
 tests/qemu-iotests/iotests.py|  5 +-
 tests/qemu-iotests/nbd-fault-injector.py |  7 +--
 tests/qemu-iotests/qcow2.py  | 39 +++---
 tests/qemu-iotests/qed.py| 17 +++---
 tests/vm/basevm.py   |  3 +-
 32 files changed, 278 insertions(+), 253 deletions(-)

diff --git a/scripts/analyse-9p-simpletrace.py 
b/scripts/analyse-9p-simpletrace.py
index 3c3dee4337..710e01adba 100755
--- a/scripts/analyse-9p-simpletrace.py
+++ b/scripts/analyse-9p-simpletrace.py
@@ -3,6 +3,7 @@
 # Usage: ./analyse-9p-simpletrace  
 #
 # Author: Harsh Prateek Bora
+from __future__ import print_function
 import os
 import simpletrace
 
@@ -79,135 +80,135 @@ symbol_9p = {
 
 class VirtFSRequestTracker(simpletrace.Analyzer):
 def begin(self):
-print "Pretty printing 9p simpletrace log ..."
+print("Pretty printing 9p simpletrace log ...")
 
 def v9fs_rerror(self, tag, id, err):
-print "RERROR (tag =", tag, ", id =", symbol_9p[id], ", err = 
\"", os.strerror(err), "\")"
+print("RERROR (tag =", tag, ", id =", symbol_9p[id], ", err = 
\"", os.strerror(err), "\")")
 
 def v9fs_version(self, tag, id, msize, version):
-print "TVERSION (tag =", tag, ", msize =", msize, ", version 
=", version, ")"
+print("TVERSION (tag =", tag, ", msize =", msize, ", version 
=", version, ")")
 
 def v9fs_version_return(self, tag, id, msize, version):
-print "RVERSION (tag =", tag, ", msize =", msize, ", version 
=", version, ")"
+print("RVERSION (tag =", tag, ", msize =", msize, ", version 
=", version, ")")
 
 def v9fs_attach(self, tag, id, fid, afid, uname, aname):
-print "TATTACH (tag =", tag, ", fid =", fid, ", afid =", afid, 
", uname =", uname, ", aname =", aname, ")"
+print("TATTACH (tag =", tag, ", fid =", fid, ", afid =", afid, 
", uname =", uname, ", aname =", aname, ")")
 
 def v9fs_attach_return(self, tag, id, type, version, path):
-print "RATTACH (tag =", tag, ", qid={type =", type, ", version 
=", version, ", path =", path, "})"
+print("RATTACH (tag =", tag, ", qid={type =", type, ", version 
=", version, ", path =", path, "})")
 
 def v9fs_stat(self, tag, id, fid):
-print "TSTAT (tag =", tag, ", fid =", fid, ")"
+print("TSTAT (tag =", tag, ", fid =", fid, ")")
 
 def v9fs_stat_return(self, tag, id, mode, atime, mtime, length):
-print "RST

[Qemu-block] [RFC 04/10] python: futurize -f lib2to3.fixes.fix_has_key

[Eduardo Habkost]

Change "dict.has_key(key)" to "key in dict"

This is necessary for Python 3 compatibility.

Done using:

  $ py=$( (g grep -l -E '^#!.*python';find -name '*.py' -printf '%P\n';) | \
sort -u | grep -v README.sh4)
  $ futurize -w -f lib2to3.fixes.fix_has_key $py

Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 scripts/qmp/qmp| 4 ++--
 scripts/qmp/qmp-shell  | 2 +-
 scripts/qmp/qom-fuse   | 2 +-
 scripts/qmp/qom-get| 2 +-
 scripts/qmp/qom-list   | 2 +-
 scripts/qmp/qom-set| 2 +-
 scripts/qmp/qom-tree   | 2 +-
 tests/qemu-iotests/093 | 2 +-
 tests/qemu-iotests/096 | 4 ++--
 tests/qemu-iotests/136 | 2 +-
 10 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/scripts/qmp/qmp b/scripts/qmp/qmp
index 4d2be4e98a..33a0d6b73a 100755
--- a/scripts/qmp/qmp
+++ b/scripts/qmp/qmp
@@ -36,7 +36,7 @@ def main(args):
 path = None
 
 # Use QMP_PATH if it's set
-if os.environ.has_key('QMP_PATH'):
+if 'QMP_PATH' in os.environ:
 path = os.environ['QMP_PATH']
 
 while len(args):
@@ -80,7 +80,7 @@ def main(args):
 
 def do_command(srv, cmd, **kwds):
 rsp = srv.cmd(cmd, kwds)
-if rsp.has_key('error'):
+if 'error' in rsp:
 raise Exception(rsp['error']['desc'])
 return rsp['return']
 
diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
index 38c99d8f72..26418dab95 100755
--- a/scripts/qmp/qmp-shell
+++ b/scripts/qmp/qmp-shell
@@ -134,7 +134,7 @@ class QMPShell(qmp.QEMUMonitorProtocol):
 
 def _fill_completion(self):
 cmds = self.cmd('query-commands')
-if cmds.has_key('error'):
+if 'error' in cmds:
 return
 for cmd in cmds['return']:
 self._completer.append(cmd['name'])
diff --git a/scripts/qmp/qom-fuse b/scripts/qmp/qom-fuse
index b75aa72767..b00cb0a0af 100755
--- a/scripts/qmp/qom-fuse
+++ b/scripts/qmp/qom-fuse
@@ -29,7 +29,7 @@ class QOMFS(Fuse):
 self.ino_count = 1
 
 def get_ino(self, path):
-if self.ino_map.has_key(path):
+if path in self.ino_map:
 return self.ino_map[path]
 self.ino_map[path] = self.ino_count
 self.ino_count += 1
diff --git a/scripts/qmp/qom-get b/scripts/qmp/qom-get
index 6313f27e8e..a3f5d7660e 100755
--- a/scripts/qmp/qom-get
+++ b/scripts/qmp/qom-get
@@ -45,7 +45,7 @@ if len(args) > 0:
 args = args[2:]
 
 if not socket_path:
-if os.environ.has_key('QMP_SOCKET'):
+if 'QMP_SOCKET' in os.environ:
 socket_path = os.environ['QMP_SOCKET']
 else:
 usage_error("no QMP socket path or address given");
diff --git a/scripts/qmp/qom-list b/scripts/qmp/qom-list
index 80b0a3d1be..2ba25e1792 100755
--- a/scripts/qmp/qom-list
+++ b/scripts/qmp/qom-list
@@ -45,7 +45,7 @@ if len(args) > 0:
 args = args[2:]
 
 if not socket_path:
-if os.environ.has_key('QMP_SOCKET'):
+if 'QMP_SOCKET' in os.environ:
 socket_path = os.environ['QMP_SOCKET']
 else:
 usage_error("no QMP socket path or address given");
diff --git a/scripts/qmp/qom-set b/scripts/qmp/qom-set
index cbffb65880..0352668812 100755
--- a/scripts/qmp/qom-set
+++ b/scripts/qmp/qom-set
@@ -46,7 +46,7 @@ if len(args) > 0:
 args = args[2:]
 
 if not socket_path:
-if os.environ.has_key('QMP_SOCKET'):
+if 'QMP_SOCKET' in os.environ:
 socket_path = os.environ['QMP_SOCKET']
 else:
 usage_error("no QMP socket path or address given");
diff --git a/scripts/qmp/qom-tree b/scripts/qmp/qom-tree
index ad4be233e6..32e708a13e 100755
--- a/scripts/qmp/qom-tree
+++ b/scripts/qmp/qom-tree
@@ -47,7 +47,7 @@ if len(args) > 0:
 args = args[2:]
 
 if not socket_path:
-if os.environ.has_key('QMP_SOCKET'):
+if 'QMP_SOCKET' in os.environ:
 socket_path = os.environ['QMP_SOCKET']
 else:
 usage_error("no QMP socket path or address given");
diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
index c3404a3171..68e344f8c1 100755
--- a/tests/qemu-iotests/093
+++ b/tests/qemu-iotests/093
@@ -237,7 +237,7 @@ class ThrottleTestGroupNames(iotests.QMPTestCase):
 if name:
 self.assertEqual(info["group"], name)
 else:
-self.assertFalse(info.has_key('group'))
+self.assertFalse('group' in info)
 return
 
 raise Exception("No group information found for '%s'" % device)
diff --git a/tests/qemu-iotests/096 b/tests/qemu-iotests/096
index aeeb3753cf..a69439602d 100755
--- a/tests/qemu-iotests/096
+++ b/tests/qemu-iotests/096
@@ -53,9 +53,9 @@ class TestLiveSnapshot(iotests.QMPTestCase):
 self.assertEqual(r['iops'], self.iops)
 self.assertEqual(r['iops_size'], self.iops_size)
 else:
-self.assertFalse(r.has_key('group'))
+self.assertFalse('group'

[Qemu-block] [RFC 02/10] python: futurize -f libfuturize.fixes.fix_absolute_import

[Eduardo Habkost]

Make implicit relative imports explicit and add "from __future__ import
absolute_import" at the top of each relevant module.

This is necessary for Python 3 compatibility.

Done using:

  $ py=$( (g grep -l -E '^#!.*python';find -name '*.py' -printf '%P\n';) | \
sort -u | grep -v README.sh4)
  $ futurize -w -f libfuturize.fixes.fix_absolute_import $py

Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 scripts/qmp/qemu-ga-client   | 3 ++-
 scripts/qmp/qmp  | 3 ++-
 scripts/qmp/qmp-shell| 3 ++-
 scripts/qmp/qom-fuse | 3 ++-
 scripts/qmp/qom-get  | 3 ++-
 scripts/qmp/qom-list | 3 ++-
 scripts/qmp/qom-set  | 3 ++-
 scripts/qmp/qom-tree | 3 ++-
 tests/image-fuzzer/qcow2/__init__.py | 3 ++-
 tests/image-fuzzer/qcow2/layout.py   | 3 ++-
 10 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/scripts/qmp/qemu-ga-client b/scripts/qmp/qemu-ga-client
index 8510814683..6045fcd3f2 100755
--- a/scripts/qmp/qemu-ga-client
+++ b/scripts/qmp/qemu-ga-client
@@ -37,10 +37,11 @@
 #
 
 from __future__ import print_function
+from __future__ import absolute_import
 import base64
 import random
 
-import qmp
+from . import qmp
 
 
 class QemuGuestAgent(qmp.QEMUMonitorProtocol):
diff --git a/scripts/qmp/qmp b/scripts/qmp/qmp
index 16d3bdb6fe..4d2be4e98a 100755
--- a/scripts/qmp/qmp
+++ b/scripts/qmp/qmp
@@ -11,8 +11,9 @@
 # See the COPYING file in the top-level directory.
 
 from __future__ import print_function
+from __future__ import absolute_import
 import sys, os
-from qmp import QEMUMonitorProtocol
+from .qmp import QEMUMonitorProtocol
 
 def print_response(rsp, prefix=[]):
 if type(rsp) == list:
diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
index b1cc7e2271..38c99d8f72 100755
--- a/scripts/qmp/qmp-shell
+++ b/scripts/qmp/qmp-shell
@@ -66,7 +66,8 @@
 # sent to QEMU, which is useful for debugging and documentation generation.
 
 from __future__ import print_function
-import qmp
+from __future__ import absolute_import
+from . import qmp
 import json
 import ast
 import readline
diff --git a/scripts/qmp/qom-fuse b/scripts/qmp/qom-fuse
index 5c6754aa63..b75aa72767 100755
--- a/scripts/qmp/qom-fuse
+++ b/scripts/qmp/qom-fuse
@@ -11,11 +11,12 @@
 # the COPYING file in the top-level directory.
 ##
 
+from __future__ import absolute_import
 import fuse, stat
 from fuse import Fuse
 import os, posix
 from errno import *
-from qmp import QEMUMonitorProtocol
+from .qmp import QEMUMonitorProtocol
 
 fuse.fuse_python_api = (0, 2)
 
diff --git a/scripts/qmp/qom-get b/scripts/qmp/qom-get
index 291c8bfbc2..6313f27e8e 100755
--- a/scripts/qmp/qom-get
+++ b/scripts/qmp/qom-get
@@ -12,9 +12,10 @@
 ##
 
 from __future__ import print_function
+from __future__ import absolute_import
 import sys
 import os
-from qmp import QEMUMonitorProtocol
+from .qmp import QEMUMonitorProtocol
 
 cmd, args = sys.argv[0], sys.argv[1:]
 socket_path = None
diff --git a/scripts/qmp/qom-list b/scripts/qmp/qom-list
index cd907bb81f..80b0a3d1be 100755
--- a/scripts/qmp/qom-list
+++ b/scripts/qmp/qom-list
@@ -12,9 +12,10 @@
 ##
 
 from __future__ import print_function
+from __future__ import absolute_import
 import sys
 import os
-from qmp import QEMUMonitorProtocol
+from .qmp import QEMUMonitorProtocol
 
 cmd, args = sys.argv[0], sys.argv[1:]
 socket_path = None
diff --git a/scripts/qmp/qom-set b/scripts/qmp/qom-set
index fbe4b3e471..cbffb65880 100755
--- a/scripts/qmp/qom-set
+++ b/scripts/qmp/qom-set
@@ -12,9 +12,10 @@
 ##
 
 from __future__ import print_function
+from __future__ import absolute_import
 import sys
 import os
-from qmp import QEMUMonitorProtocol
+from .qmp import QEMUMonitorProtocol
 
 cmd, args = sys.argv[0], sys.argv[1:]
 socket_path = None
diff --git a/scripts/qmp/qom-tree b/scripts/qmp/qom-tree
index 0ffd1ff1de..ad4be233e6 100755
--- a/scripts/qmp/qom-tree
+++ b/scripts/qmp/qom-tree
@@ -14,9 +14,10 @@
 ##
 
 from __future__ import print_function
+from __future__ import absolute_import
 import sys
 import os
-from qmp import QEMUMonitorProtocol
+from .qmp import QEMUMonitorProtocol
 
 cmd, args = sys.argv[0], sys.argv[1:]
 socket_path = None
diff --git a/tests/image-fuzzer/qcow2/__init__.py 
b/tests/image-fuzzer/qcow2/__init__.py
index e2ebe19311..09ef59821b 100644
--- a/tests/image-fuzzer/qcow2/__init__.py
+++ b/tests/image-fuzzer/qcow2/__init__.py
@@ -1 +1,2 @@
-from layout import create_image
+from __future__ import absolute_import
+from .layout import create_image
diff --git a/tests/image-fuzzer/qcow2/layout.py 
b/tests/image-fuzzer/qcow2/layout.py
index 63e801f4e8..df2131a855 100644
--- a/tests/image-fuzzer/qcow2/layout.py
+++ b/tests/image-fuzzer/qcow2/layout.py
@@ -1,3 +1,4 @@
+from __future__ import absolute_import
 # Generator of fuzzed qcow2 images
 #
 # Copyright (C) 2014 Maria Kustova <mari...@catit.be>
@@ -18,7 +19,7 @@
 
 import random
 import str

[Qemu-block] [RFC 03/10] python: futurize -f libfuturize.fixes.fix_next_call

[Eduardo Habkost]

Change obj.next() calls to next(obj).

This is necessary for Python 3 compatibility.

Done using:

  $ py=$( (g grep -l -E '^#!.*python';find -name '*.py' -printf '%P\n';) | \
sort -u | grep -v README.sh4)
  $ futurize -w -f libfuturize.fixes.fix_next_call $py

Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 scripts/ordereddict.py| 4 ++--
 scripts/vmstate-static-checker.py | 4 ++--
 tests/image-fuzzer/runner.py  | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/scripts/ordereddict.py b/scripts/ordereddict.py
index 2d1d81370b..68ed340b33 100644
--- a/scripts/ordereddict.py
+++ b/scripts/ordereddict.py
@@ -71,9 +71,9 @@ class OrderedDict(dict, DictMixin):
 if not self:
 raise KeyError('dictionary is empty')
 if last:
-key = reversed(self).next()
+key = next(reversed(self))
 else:
-key = iter(self).next()
+key = next(iter(self))
 value = self.pop(key)
 return key, value
 
diff --git a/scripts/vmstate-static-checker.py 
b/scripts/vmstate-static-checker.py
index 60d1bf4cda..d3467288dc 100755
--- a/scripts/vmstate-static-checker.py
+++ b/scripts/vmstate-static-checker.py
@@ -158,7 +158,7 @@ def check_fields(src_fields, dest_fields, desc, sec):
 while True:
 if advance_src:
 try:
-s_item = s_iter.next()
+s_item = next(s_iter)
 except StopIteration:
 if s_iter_list == []:
 break
@@ -173,7 +173,7 @@ def check_fields(src_fields, dest_fields, desc, sec):
 
 if advance_dest:
 try:
-d_item = d_iter.next()
+d_item = next(d_iter)
 except StopIteration:
 if d_iter_list == []:
 # We were not in a substruct
diff --git a/tests/image-fuzzer/runner.py b/tests/image-fuzzer/runner.py
index 8de656933e..45e8fca63f 100755
--- a/tests/image-fuzzer/runner.py
+++ b/tests/image-fuzzer/runner.py
@@ -422,7 +422,7 @@ if __name__ == '__main__':
 test_id = count(1)
 while should_continue(duration, start_time):
 try:
-run_test(str(test_id.next()), seed, work_dir, run_log, cleanup,
+run_test(str(next(test_id)), seed, work_dir, run_log, cleanup,
  log_all, command, config)
 except (KeyboardInterrupt, SystemExit):
 sys.exit(1)
-- 
2.14.3

[Qemu-block] [RFC 00/10] [TESTING NEEDED] python: futurize --stage1 (Python 3 compatibility)

[Eduardo Habkost]

TESTING NEEDED: Due to the amount of changes, I didn't test all
scripts touched by this series.  If you are responsible for any
of the touched files, I would appreciate help on testing the
series.

>From the futurize[1] documentation:

> This applies fixes that modernize Python 2 code without
> changing the effect of the code.  With luck, this will not
> introduce any bugs into the code, or will at least be trivial
> to fix. The changes are those that bring the Python code
> up-to-date without breaking Py2 compatibility.  The resulting
> code will be modern Python 2.6-compatible code plus __future__
> imports from the following set:
>
> from __future__ import absolute_import
> from __future__ import division
> from __future__ import print_function
>
[...]
> The goal for this stage is to create most of the diff for the
> entire porting process, but without introducing any bugs.  It
> should be uncontroversial and safe to apply to every Python 2
> package.  The subsequent patches introducing Python 3
> compatibility should then be shorter and easier to review.

This series run all the fixers from futurize --stage1 on all
Python code in the tree.  To make review and testing easier, I
have run the fixers separately instead of doing all changes in a
single patch.

[1] http://python-future.org/automatic_conversion.html

Eduardo Habkost (10):
  python: futurize -f libfuturize.fixes.fix_print_with_import
  python: futurize -f libfuturize.fixes.fix_absolute_import
  python: futurize -f libfuturize.fixes.fix_next_call
  python: futurize -f lib2to3.fixes.fix_has_key
  python: futurize -f lib2to3.fixes.fix_standarderror
  python: futurize -f lib2to3.fixes.fix_reduce
  python: futurize -f lib2to3.fixes.fix_tuple_params
  python: futurize -f lib2to3.fixes.fix_renames
  python: futurize -f lib2to3.fixes.fix_except
  python: futurize -f lib2to3.fixes.fix_numliterals

 scripts/analyse-9p-simpletrace.py| 89 
 scripts/analyse-locks-simpletrace.py |  3 +-
 scripts/analyze-migration.py | 11 ++--
 scripts/dump-guest-memory.py |  1 +
 scripts/ordereddict.py   |  4 +-
 scripts/replay-dump.py   | 21 
 scripts/signrom.py   |  1 +
 scripts/simpletrace.py   |  5 +-
 scripts/vmstate-static-checker.py| 89 
 scripts/device-crash-test|  3 +-
 scripts/kvm/kvm_flightrecorder   | 21 
 scripts/kvm/vmxcap   |  1 +
 scripts/qmp/qemu-ga-client   | 10 ++--
 scripts/qmp/qmp  | 24 +
 scripts/qmp/qmp-shell| 40 +++---
 scripts/qmp/qom-fuse | 11 ++--
 scripts/qmp/qom-get  | 12 +++--
 scripts/qmp/qom-list | 16 +++---
 scripts/qmp/qom-set  | 10 ++--
 scripts/qmp/qom-tree | 16 +++---
 tests/docker/docker.py   | 11 ++--
 tests/docker/travis.py   | 15 +++---
 tests/guest-debug/test-gdbstub.py|  1 +
 tests/image-fuzzer/qcow2/__init__.py |  3 +-
 tests/image-fuzzer/qcow2/fuzz.py |  1 +
 tests/image-fuzzer/qcow2/layout.py   |  3 +-
 tests/image-fuzzer/runner.py | 42 +++
 tests/migration/guestperf/engine.py  | 29 ++-
 tests/migration/guestperf/plot.py| 17 +++---
 tests/migration/guestperf/shell.py   | 19 +++
 tests/qemu-iotests/093   |  2 +-
 tests/qemu-iotests/096   |  4 +-
 tests/qemu-iotests/118   | 24 -
 tests/qemu-iotests/136   |  2 +-
 tests/qemu-iotests/149   |  3 +-
 tests/qemu-iotests/165   |  3 +-
 tests/qemu-iotests/iotests.py|  5 +-
 tests/qemu-iotests/nbd-fault-injector.py |  7 +--
 tests/qemu-iotests/qcow2.py  | 39 +++---
 tests/qemu-iotests/qed.py| 17 +++---
 tests/vm/basevm.py   |  3 +-
 41 files changed, 337 insertions(+), 301 deletions(-)

-- 
2.14.3

Re: [Qemu-block] [Qemu-devel] [PATCH V5] pci: removed the is_express field since a uniform interface was inserted

[Eduardo Habkost]

On Mon, Dec 18, 2017 at 05:21:40PM +0200, Yoni Bettan wrote:
> according to Eduardo Habkost's commit fd3b02c889 all PCIEs now implement
> INTERFACE_PCIE_DEVICE so we don't need is_express field anymore.
> 
> Devices that implements only INTERFACE_PCIE_DEVICE (is_express == 1)
> or
> devices that implements only INTERFACE_CONVENTIONAL_PCI_DEVICE (is_express == 
> 0)
> where not affected by the change.
> 
> The only devices that were affected are those that are hybrid and also
> had (is_express == 1) - therefor only:
>   - hw/vfio/pci.c
>   - hw/usb/hcd-xhci.c
>   - hw/xen/xen_pt.c
> 
> For those 3 I made sure that QEMU_PCI_CAP_EXPRESS is on in instance_init()
> 
> Signed-off-by: Yoni Bettan <ybet...@redhat.com>

Reviewed-by: Eduardo Habkost <ehabk...@redhat.com>

-- 
Eduardo

Re: [Qemu-block] [Qemu-devel] [PATCH V4] pci: removed the is_express field since a uniform interface was inserted

[Eduardo Habkost]

On Tue, Dec 12, 2017 at 07:36:49AM +0200, Yoni Bettan wrote:
> * according to Eduardo Habkost's commit
>   fd3b02c8896d597dd8b9e053dec579cf0386aee1
> 
> * since all PCIEs now implement INTERFACE_PCIE_DEVICE we
>   don't need this field anymore
> 
> * Devices that where only INTERFACE_PCIE_DEVICE (is_express == 1)
>   or
>   devices that where only INTERFACE_CONVENTIONAL_PCI_DEVICE 
> (is_express == 0)
>   where not affected by the change
> 
>   The only devices that were affected are those that are hybrid and 
> also
>   had (is_express == 1) - therefor only:
> - hw/vfio/pci.c
> - hw/usb/hcd-xhci.c

Oops, we now have xen-pt too.  See:

  From: Simon Gaiser 
  Date: Sat, 28 Oct 2017 04:53:15 +0200
  Message-Id: <20171028025315.13500-1-h...@ipsumj.de>
  Subject: [PATCH] xen/pt: Set is_express to avoid out-of-bounds write

Which was included in a pull request sent on last Thursday.

> 
>   For both I made sure that QEMU_PCI_CAP_EXPRESS is on
> 
> Signed-off-by: Yoni Bettan 
[...]

-- 
Eduardo

Re: [Qemu-block] [Qemu-devel] [PATCH V4] pci: removed the is_express field since a uniform interface was inserted

[Eduardo Habkost]

On Tue, Dec 12, 2017 at 07:36:49AM +0200, Yoni Bettan wrote:
> * according to Eduardo Habkost's commit
>   fd3b02c8896d597dd8b9e053dec579cf0386aee1
> 
> * since all PCIEs now implement INTERFACE_PCIE_DEVICE we
>   don't need this field anymore
> 
> * Devices that where only INTERFACE_PCIE_DEVICE (is_express == 1)
>   or
>   devices that where only INTERFACE_CONVENTIONAL_PCI_DEVICE 
> (is_express == 0)
>   where not affected by the change
> 
>   The only devices that were affected are those that are hybrid and 
> also
>   had (is_express == 1) - therefor only:
> - hw/vfio/pci.c
> - hw/usb/hcd-xhci.c
> 
>   For both I made sure that QEMU_PCI_CAP_EXPRESS is on
> 
> Signed-off-by: Yoni Bettan <ybet...@redhat.com>

The code looks good.

Reviewed-by: Eduardo Habkost <ehabk...@redhat.com>

But I suggest reformatting the commit message before committing
this.


> ---
>  docs/pcie_pci_bridge.txt   | 2 +-
>  hw/block/nvme.c| 1 -
>  hw/net/e1000e.c| 1 -
>  hw/pci-bridge/pcie_pci_bridge.c| 1 -
>  hw/pci-bridge/pcie_root_port.c | 1 -
>  hw/pci-bridge/xio3130_downstream.c | 1 -
>  hw/pci-bridge/xio3130_upstream.c   | 1 -
>  hw/pci-host/xilinx-pcie.c  | 1 -
>  hw/pci/pci.c   | 8 ++--
>  hw/scsi/megasas.c  | 4 
>  hw/usb/hcd-xhci.c  | 9 -
>  hw/vfio/pci.c  | 5 -
>  include/hw/pci/pci.h   | 3 ---
>  13 files changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/docs/pcie_pci_bridge.txt b/docs/pcie_pci_bridge.txt
> index 5a4203f97c..ab35ebf3ca 100644
> --- a/docs/pcie_pci_bridge.txt
> +++ b/docs/pcie_pci_bridge.txt
> @@ -110,5 +110,5 @@ To enable device hot-plug into the bridge on Linux 
> there're 3 ways:
>  Implementation
>  ==
>  The PCIE-PCI bridge is based on PCI-PCI bridge, but also accumulates PCI 
> Express
> -features as a PCI Express device (is_express=1).
> +features as a PCI Express device.
>  
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 441e21ed1f..9325bc0911 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1087,7 +1087,6 @@ static void nvme_class_init(ObjectClass *oc, void *data)
>  pc->vendor_id = PCI_VENDOR_ID_INTEL;
>  pc->device_id = 0x5845;
>  pc->revision = 2;
> -pc->is_express = 1;
>  
>  set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>  dc->desc = "Non-Volatile Memory Express";
> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> index f1af279e8d..c360f0d8c9 100644
> --- a/hw/net/e1000e.c
> +++ b/hw/net/e1000e.c
> @@ -675,7 +675,6 @@ static void e1000e_class_init(ObjectClass *class, void 
> *data)
>  c->revision = 0;
>  c->romfile = "efi-e1000e.rom";
>  c->class_id = PCI_CLASS_NETWORK_ETHERNET;
> -c->is_express = 1;
>  
>  dc->desc = "Intel 82574L GbE Controller";
>  dc->reset = e1000e_qdev_reset;
> diff --git a/hw/pci-bridge/pcie_pci_bridge.c b/hw/pci-bridge/pcie_pci_bridge.c
> index a4d827c99d..b7d9ebbec2 100644
> --- a/hw/pci-bridge/pcie_pci_bridge.c
> +++ b/hw/pci-bridge/pcie_pci_bridge.c
> @@ -169,7 +169,6 @@ static void pcie_pci_bridge_class_init(ObjectClass 
> *klass, void *data)
>  DeviceClass *dc = DEVICE_CLASS(klass);
>  HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>  
> -k->is_express = 1;
>  k->is_bridge = 1;
>  k->vendor_id = PCI_VENDOR_ID_REDHAT;
>  k->device_id = PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE;
> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
> index 9b6e4ce512..45f9e8cd4a 100644
> --- a/hw/pci-bridge/pcie_root_port.c
> +++ b/hw/pci-bridge/pcie_root_port.c
> @@ -145,7 +145,6 @@ static void rp_class_init(ObjectClass *klass, void *data)
>  DeviceClass *dc = DEVICE_CLASS(klass);
>  PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  
> -k->is_express = 1;
>  k->is_bridge = 1;
>  k->config_write = rp_write_config;
>  k->realize = rp_realize;
> diff --git a/hw/pci-bridge/xio3130_downstream.c 
> b/hw/pci-bridge/xio3130_downstream.c
> index 1e09d2afb7..613a0d6bb7 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -177,7 +177,6 @@ static void xio3130_downstream_class_init(ObjectClass 
> *klass, void *data)
>  DeviceClass *dc = DEVICE_CLASS(klass);
>  PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  
> -k->is_express = 1;
>  k->is_bridge = 1;
&g

Re: [Qemu-block] [Qemu-devel] [PATCH V3] pci: removed the is_express field since a uniform interface was inserted

[Eduardo Habkost]

On Mon, Dec 11, 2017 at 03:11:39PM +0200, Yoni Bettan wrote:
> 
> 
> On 12/07/2017 10:58 PM, Eduardo Habkost wrote:
> > On Tue, Dec 05, 2017 at 07:17:06PM +0200, Yoni Bettan wrote:
> > >  * according to Eduardo Habkost's commit
> > >fd3b02c8896d597dd8b9e053dec579cf0386aee1
> > > 
> > >  * since all PCIEs now implement INTERFACE_PCIE_DEVICE we
> > >don't need this field anymore
> > > 
> > >  * Devices that where only INTERFACE_PCIE_DEVICE (is_express == 1)
> > >or
> > >devices that where only INTERFACE_CONVENTIONAL_PCI_DEVICE 
> > > (is_express == 0)
> > >where not affected by the change
> > > 
> > >The only devices that were affected are those that are hybrid 
> > > and also
> > >had (is_express == 1) - therefor only:
> > >  - hw/vfio/pci.c
> > >  - hw/usb/hcd-xhci.c
> > > 
> > >For both I made sure that QEMU_PCI_CAP_EXPRESS is on
> > > 
> > > Signed-off-by: Yoni Bettan <ybet...@redhat.com>
> > > ---
> > >   docs/pcie_pci_bridge.txt   | 2 +-
> > >   hw/block/nvme.c| 1 -
> > >   hw/net/e1000e.c| 1 -
> > >   hw/pci-bridge/pcie_pci_bridge.c| 1 -
> > >   hw/pci-bridge/pcie_root_port.c | 1 -
> > >   hw/pci-bridge/xio3130_downstream.c | 1 -
> > >   hw/pci-bridge/xio3130_upstream.c   | 1 -
> > >   hw/pci-host/xilinx-pcie.c  | 1 -
> > >   hw/pci/pci.c   | 4 +++-
> > >   hw/scsi/megasas.c  | 4 
> > >   hw/usb/hcd-xhci.c  | 7 ++-
> > >   hw/vfio/pci.c  | 3 ++-
> > >   include/hw/pci/pci.h   | 3 ---
> > >   13 files changed, 12 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/docs/pcie_pci_bridge.txt b/docs/pcie_pci_bridge.txt
> > > index 5a4203f97c..ab35ebf3ca 100644
> > > --- a/docs/pcie_pci_bridge.txt
> > > +++ b/docs/pcie_pci_bridge.txt
> > > @@ -110,5 +110,5 @@ To enable device hot-plug into the bridge on Linux 
> > > there're 3 ways:
> > >   Implementation
> > >   ==
> > >   The PCIE-PCI bridge is based on PCI-PCI bridge, but also accumulates 
> > > PCI Express
> > > -features as a PCI Express device (is_express=1).
> > > +features as a PCI Express device.
> > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > > index 441e21ed1f..9325bc0911 100644
> > > --- a/hw/block/nvme.c
> > > +++ b/hw/block/nvme.c
> > > @@ -1087,7 +1087,6 @@ static void nvme_class_init(ObjectClass *oc, void 
> > > *data)
> > >   pc->vendor_id = PCI_VENDOR_ID_INTEL;
> > >   pc->device_id = 0x5845;
> > >   pc->revision = 2;
> > > -pc->is_express = 1;
> > >   set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> > >   dc->desc = "Non-Volatile Memory Express";
> > > diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> > > index f1af279e8d..c360f0d8c9 100644
> > > --- a/hw/net/e1000e.c
> > > +++ b/hw/net/e1000e.c
> > > @@ -675,7 +675,6 @@ static void e1000e_class_init(ObjectClass *class, 
> > > void *data)
> > >   c->revision = 0;
> > >   c->romfile = "efi-e1000e.rom";
> > >   c->class_id = PCI_CLASS_NETWORK_ETHERNET;
> > > -c->is_express = 1;
> > >   dc->desc = "Intel 82574L GbE Controller";
> > >   dc->reset = e1000e_qdev_reset;
> > > diff --git a/hw/pci-bridge/pcie_pci_bridge.c 
> > > b/hw/pci-bridge/pcie_pci_bridge.c
> > > index a4d827c99d..b7d9ebbec2 100644
> > > --- a/hw/pci-bridge/pcie_pci_bridge.c
> > > +++ b/hw/pci-bridge/pcie_pci_bridge.c
> > > @@ -169,7 +169,6 @@ static void pcie_pci_bridge_class_init(ObjectClass 
> > > *klass, void *data)
> > >   DeviceClass *dc = DEVICE_CLASS(klass);
> > >   HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
> > > -k->is_express = 1;
> > >   k->is_bridge = 1;
> > >   k->vendor_id = PCI_VENDOR_ID_REDHAT;
> > >   k->device_id = PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE;
> > > diff --git a/hw/pci-bridge/pcie_root_port.c 
> > > b/hw/pci-bridge/pcie_root_port.c
> > > index 9b6e4ce512..45f9e8cd4a 100644
> > > --- a/hw/pci-bridge/p

Re: [Qemu-block] [Qemu-devel] [PATCH V3] pci: removed the is_express field since a uniform interface was inserted

[Eduardo Habkost]

On Tue, Dec 05, 2017 at 07:17:06PM +0200, Yoni Bettan wrote:
> * according to Eduardo Habkost's commit
>   fd3b02c8896d597dd8b9e053dec579cf0386aee1
> 
> * since all PCIEs now implement INTERFACE_PCIE_DEVICE we
>   don't need this field anymore
> 
> * Devices that where only INTERFACE_PCIE_DEVICE (is_express == 1)
>   or
>   devices that where only INTERFACE_CONVENTIONAL_PCI_DEVICE 
> (is_express == 0)
>   where not affected by the change
> 
>   The only devices that were affected are those that are hybrid and 
> also
>   had (is_express == 1) - therefor only:
> - hw/vfio/pci.c
> - hw/usb/hcd-xhci.c
> 
>   For both I made sure that QEMU_PCI_CAP_EXPRESS is on
> 
> Signed-off-by: Yoni Bettan 
> ---
>  docs/pcie_pci_bridge.txt   | 2 +-
>  hw/block/nvme.c| 1 -
>  hw/net/e1000e.c| 1 -
>  hw/pci-bridge/pcie_pci_bridge.c| 1 -
>  hw/pci-bridge/pcie_root_port.c | 1 -
>  hw/pci-bridge/xio3130_downstream.c | 1 -
>  hw/pci-bridge/xio3130_upstream.c   | 1 -
>  hw/pci-host/xilinx-pcie.c  | 1 -
>  hw/pci/pci.c   | 4 +++-
>  hw/scsi/megasas.c  | 4 
>  hw/usb/hcd-xhci.c  | 7 ++-
>  hw/vfio/pci.c  | 3 ++-
>  include/hw/pci/pci.h   | 3 ---
>  13 files changed, 12 insertions(+), 18 deletions(-)
> 
> diff --git a/docs/pcie_pci_bridge.txt b/docs/pcie_pci_bridge.txt
> index 5a4203f97c..ab35ebf3ca 100644
> --- a/docs/pcie_pci_bridge.txt
> +++ b/docs/pcie_pci_bridge.txt
> @@ -110,5 +110,5 @@ To enable device hot-plug into the bridge on Linux 
> there're 3 ways:
>  Implementation
>  ==
>  The PCIE-PCI bridge is based on PCI-PCI bridge, but also accumulates PCI 
> Express
> -features as a PCI Express device (is_express=1).
> +features as a PCI Express device.
>  
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 441e21ed1f..9325bc0911 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1087,7 +1087,6 @@ static void nvme_class_init(ObjectClass *oc, void *data)
>  pc->vendor_id = PCI_VENDOR_ID_INTEL;
>  pc->device_id = 0x5845;
>  pc->revision = 2;
> -pc->is_express = 1;
>  
>  set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>  dc->desc = "Non-Volatile Memory Express";
> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> index f1af279e8d..c360f0d8c9 100644
> --- a/hw/net/e1000e.c
> +++ b/hw/net/e1000e.c
> @@ -675,7 +675,6 @@ static void e1000e_class_init(ObjectClass *class, void 
> *data)
>  c->revision = 0;
>  c->romfile = "efi-e1000e.rom";
>  c->class_id = PCI_CLASS_NETWORK_ETHERNET;
> -c->is_express = 1;
>  
>  dc->desc = "Intel 82574L GbE Controller";
>  dc->reset = e1000e_qdev_reset;
> diff --git a/hw/pci-bridge/pcie_pci_bridge.c b/hw/pci-bridge/pcie_pci_bridge.c
> index a4d827c99d..b7d9ebbec2 100644
> --- a/hw/pci-bridge/pcie_pci_bridge.c
> +++ b/hw/pci-bridge/pcie_pci_bridge.c
> @@ -169,7 +169,6 @@ static void pcie_pci_bridge_class_init(ObjectClass 
> *klass, void *data)
>  DeviceClass *dc = DEVICE_CLASS(klass);
>  HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>  
> -k->is_express = 1;
>  k->is_bridge = 1;
>  k->vendor_id = PCI_VENDOR_ID_REDHAT;
>  k->device_id = PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE;
> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
> index 9b6e4ce512..45f9e8cd4a 100644
> --- a/hw/pci-bridge/pcie_root_port.c
> +++ b/hw/pci-bridge/pcie_root_port.c
> @@ -145,7 +145,6 @@ static void rp_class_init(ObjectClass *klass, void *data)
>  DeviceClass *dc = DEVICE_CLASS(klass);
>  PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  
> -k->is_express = 1;
>  k->is_bridge = 1;
>  k->config_write = rp_write_config;
>  k->realize = rp_realize;
> diff --git a/hw/pci-bridge/xio3130_downstream.c 
> b/hw/pci-bridge/xio3130_downstream.c
> index 1e09d2afb7..613a0d6bb7 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -177,7 +177,6 @@ static void xio3130_downstream_class_init(ObjectClass 
> *klass, void *data)
>  DeviceClass *dc = DEVICE_CLASS(klass);
>  PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  
> -k->is_express = 1;
>  k->is_bridge = 1;
>  k->config_write = xio3130_downstream_write_config;
>  k->realize = xio3130_downstream_realize;
> diff --git a/hw/pci-bridge/xio3130_upstream.c 
> b/hw/pci-bridge/xio3130_upstream.c
> index 227997ce46..d4645bddee 100644
> --- a/hw/pci-bridge/xio3130_upstream.c
> +++ b/hw/pci-bridge/xio3130_upstream.c
> @@ -148,7 +148,6 @@ static void xio3130_upstream_class_init(ObjectClass 
> *klass, void *data)
>  DeviceClass *dc = DEVICE_CLASS(klass);
>  PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  
> -k->is_express = 1;
>  k->is_bridge = 1;
>  k->config_write = 

[Qemu-block] [PULL 1/5] iotests: Set up Python logging

[Eduardo Habkost]

Set up Python logging module instead of relying on
QEMUMachine._debug to enable debugging messages.

Cc: Kevin Wolf <kw...@redhat.com>
Cc: Max Reitz <mre...@redhat.com>
Cc: qemu-block@nongnu.org
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
Message-Id: <20170927130339.21444-3-ehabk...@redhat.com>
Reviewed-by: Daniel P. Berrange <berra...@redhat.com>
Reviewed-by: Lukáš Doktor <ldok...@redhat.com>
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 tests/qemu-iotests/iotests.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 1af117e37d..36a7757aaf 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -28,6 +28,7 @@ import qtest
 import struct
 import json
 import signal
+import logging
 
 
 # This will not work if arguments contain spaces but is necessary if we
@@ -467,6 +468,8 @@ def main(supported_fmts=[], supported_oses=['linux']):
 else:
 output = StringIO.StringIO()
 
+logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN))
+
 class MyTestRunner(unittest.TextTestRunner):
 def __init__(self, stream=output, descriptions=True, 
verbosity=verbosity):
 unittest.TextTestRunner.__init__(self, stream, descriptions, 
verbosity)
-- 
2.13.6

[Qemu-block] [PATCH] xen/pt: Mark TYPE_XEN_PT_DEVICE as hybrid

[Eduardo Habkost]

xen-pt doesn't set the is_express field, but is supposed to be
able to handle PCI Express devices too.  Mark it as hybrid.

Suggested-by: Jan Beulich <jbeul...@suse.com>
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 hw/xen/xen_pt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 01df3414d3..9bba717708 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -966,6 +966,7 @@ static const TypeInfo xen_pci_passthrough_info = {
 .class_init = xen_pci_passthrough_class_init,
 .interfaces = (InterfaceInfo[]) {
 { INTERFACE_CONVENTIONAL_PCI_DEVICE },
+{ INTERFACE_PCIE_DEVICE },
 { },
 },
 };
-- 
2.13.6

Re: [Qemu-block] xen-pci-passthrough PCI Express support? (Re: [Qemu-devel] [Xen-devel] [PATCH v2 4/5] pci: Add INTERFACE_CONVENTIONAL_PCI_DEVICE to Conventional PCI devices)

[Eduardo Habkost]

On Wed, Oct 04, 2017 at 03:08:15AM -0600, Jan Beulich wrote:
> >>> On 03.10.17 at 02:12, <ehabk...@redhat.com> wrote:
> > On Thu, Sep 28, 2017 at 10:12:34AM -0300, Eduardo Habkost wrote:
> >> On Thu, Sep 28, 2017 at 02:33:57AM -0600, Jan Beulich wrote:
> >> > >>> On 27.09.17 at 21:56, <ehabk...@redhat.com> wrote:
> >> > > --- a/hw/xen/xen_pt.c
> >> > > +++ b/hw/xen/xen_pt.c
> >> > > @@ -964,6 +964,10 @@ static const TypeInfo xen_pci_passthrough_info = {
> >> > >  .instance_size = sizeof(XenPCIPassthroughState),
> >> > >  .instance_finalize = xen_pci_passthrough_finalize,
> >> > >  .class_init = xen_pci_passthrough_class_init,
> >> > > +.interfaces = (InterfaceInfo[]) {
> >> > > +{ INTERFACE_CONVENTIONAL_PCI_DEVICE },
> >> > > +{ },
> >> > > +},
> >> > >  };
> >> > 
> >> > Passed through devices can be both PCI and PCIe, so following
> >> > the description of the patch I don't think these can be statically
> >> > given either property. Granted quite a bit of PCIe specific
> >> > functionality may be missing in the Xen code ...
> >> 
> >> This is just static data about what the device type supports, not
> >> about what a given device instance really is.  Deciding if the
> >> device is PCIe or Conventional at runtime is out of the scope of
> >> this series.
> >> 
> >> That said, if passed through PCI Express devices are really
> >> supported, it looks like this should be marked as hybrid.
> > 
> > Can anybody confirm if PCI Express devices are really supported
> > by xen-pci-passthrough?
> 
> I think I've clearly said they're supported, with some limitations.

Sorry, thanks.  I thought the possible missing PCIe functionality
could mean the device couldn't appear as PCI Express to the
guest.

I will submit a follow-up patch adding INTERFACE_PCIE_DEVICE to
xen-pci-passthrough.

-- 
Eduardo

[Qemu-block] xen-pci-passthrough PCI Express support? (Re: [Qemu-devel] [Xen-devel] [PATCH v2 4/5] pci: Add INTERFACE_CONVENTIONAL_PCI_DEVICE to Conventional PCI devices)

[Eduardo Habkost]

On Thu, Sep 28, 2017 at 10:12:34AM -0300, Eduardo Habkost wrote:
> On Thu, Sep 28, 2017 at 02:33:57AM -0600, Jan Beulich wrote:
> > >>> On 27.09.17 at 21:56, <ehabk...@redhat.com> wrote:
> > > --- a/hw/xen/xen_pt.c
> > > +++ b/hw/xen/xen_pt.c
> > > @@ -964,6 +964,10 @@ static const TypeInfo xen_pci_passthrough_info = {
> > >  .instance_size = sizeof(XenPCIPassthroughState),
> > >  .instance_finalize = xen_pci_passthrough_finalize,
> > >  .class_init = xen_pci_passthrough_class_init,
> > > +.interfaces = (InterfaceInfo[]) {
> > > +{ INTERFACE_CONVENTIONAL_PCI_DEVICE },
> > > +{ },
> > > +},
> > >  };
> > 
> > Passed through devices can be both PCI and PCIe, so following
> > the description of the patch I don't think these can be statically
> > given either property. Granted quite a bit of PCIe specific
> > functionality may be missing in the Xen code ...
> 
> This is just static data about what the device type supports, not
> about what a given device instance really is.  Deciding if the
> device is PCIe or Conventional at runtime is out of the scope of
> this series.
> 
> That said, if passed through PCI Express devices are really
> supported, it looks like this should be marked as hybrid.

Can anybody confirm if PCI Express devices are really supported
by xen-pci-passthrough?

I suggest we add only INTERFACE_CONVENTIONAL_PCI_DEVICE to the
class info until we confirm that.

(In other words, apply this patch as-is, and add
INTERFACE_PCIE_DEVICE later as a follow-up patch if appropriate.)

-- 
Eduardo

Re: [Qemu-block] [Xen-devel] [PATCH v2 4/5] pci: Add INTERFACE_CONVENTIONAL_PCI_DEVICE to Conventional PCI devices

[Eduardo Habkost]

On Thu, Sep 28, 2017 at 02:33:57AM -0600, Jan Beulich wrote:
> >>> On 27.09.17 at 21:56,  wrote:
> > --- a/hw/xen/xen_pt.c
> > +++ b/hw/xen/xen_pt.c
> > @@ -964,6 +964,10 @@ static const TypeInfo xen_pci_passthrough_info = {
> >  .instance_size = sizeof(XenPCIPassthroughState),
> >  .instance_finalize = xen_pci_passthrough_finalize,
> >  .class_init = xen_pci_passthrough_class_init,
> > +.interfaces = (InterfaceInfo[]) {
> > +{ INTERFACE_CONVENTIONAL_PCI_DEVICE },
> > +{ },
> > +},
> >  };
> 
> Passed through devices can be both PCI and PCIe, so following
> the description of the patch I don't think these can be statically
> given either property. Granted quite a bit of PCIe specific
> functionality may be missing in the Xen code ...

This is just static data about what the device type supports, not
about what a given device instance really is.  Deciding if the
device is PCIe or Conventional at runtime is out of the scope of
this series.

That said, if passed through PCI Express devices are really
supported, it looks like this should be marked as hybrid.

-- 
Eduardo

[Qemu-block] [PATCH v2 4/5] pci: Add INTERFACE_CONVENTIONAL_PCI_DEVICE to Conventional PCI devices

[Eduardo Habkost]

Add INTERFACE_CONVENTIONAL_PCI_DEVICE to all direct subtypes of
TYPE_PCI_DEVICE, except:

1) The ones that already have INTERFACE_PCIE_DEVICE set:

* base-xhci
* e1000e
* nvme
* pvscsi
* vfio-pci
* virtio-pci
* vmxnet3

2) base-pci-bridge

Not all PCI bridges are Conventional PCI devices, so
INTERFACE_CONVENTIONAL_PCI_DEVICE is added only to the subtypes
that are actually Conventional PCI:

* dec-21154-p2p-bridge
* i82801b11-bridge
* pbm-bridge
* pci-bridge

The direct subtypes of base-pci-bridge not touched by this patch
are:

* xilinx-pcie-root: Already marked as PCIe-only.
* pcie-pci-bridge: Already marked as PCIe-only.
* pcie-port: all non-abstract subtypes of pcie-port are already
  marked as PCIe-only devices.

3) megasas-base

Not all megasas devices are Conventional PCI devices, so the
interface names are added to the subclasses registered by
megasas_register_types(), according to information in the
megasas_devices[] array.

"megasas-gen2" already implements INTERFACE_PCIE_DEVICE, so add
INTERFACE_CONVENTIONAL_PCI_DEVICE only to "megasas".

Acked-by: Alberto Garcia <be...@igalia.com>
Acked-by: John Snow <js...@redhat.com>
Acked-by: Anthony PERARD <anthony.per...@citrix.com>
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
Changes v1 -> v2:
* s/legacy/conventional/
  * Suggested-by: Alex Williamson <alex.william...@redhat.com>
* Note about pcie-pci-bridge on commit message.
* New devices: sungem, sunhme

Cc: "Michael S. Tsirkin" <m...@redhat.com>
Cc: Igor Mammedov <imamm...@redhat.com>
Cc: Gerd Hoffmann <kra...@redhat.com>
Cc: Paolo Bonzini <pbonz...@redhat.com>
Cc: Richard Henderson <r...@twiddle.net>
Cc: Eduardo Habkost <ehabk...@redhat.com>
Cc: Stefano Stabellini <sstabell...@kernel.org>
Cc: Anthony Perard <anthony.per...@citrix.com>
Cc: John Snow <js...@redhat.com>
Cc: Alberto Garcia <be...@igalia.com>
Cc: Aurelien Jarno <aurel...@aurel32.net>
Cc: Yongbok Kim <yongbok@imgtec.com>
Cc: Jiri Slaby <jsl...@suse.cz>
Cc: Alexander Graf <ag...@suse.de>
Cc: Marcel Apfelbaum <mar...@redhat.com>
Cc: Jason Wang <jasow...@redhat.com>
Cc: Jiri Pirko <j...@resnulli.us>
Cc: "Hervé Poussineau" <hpous...@reactos.org>
Cc: Peter Maydell <peter.mayd...@linaro.org>
Cc: David Gibson <da...@gibson.dropbear.id.au>
Cc: Hannes Reinecke <h...@suse.com>
Cc: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>
Cc: Artyom Tarasenko <atar4q...@gmail.com>
Cc: Alex Williamson <alex.william...@redhat.com>
Cc: qemu-de...@nongnu.org
Cc: xen-de...@lists.xenproject.org
Cc: qemu-block@nongnu.org
Cc: qemu-...@nongnu.org
Cc: qemu-...@nongnu.org
---
 hw/acpi/piix4.c |  1 +
 hw/audio/ac97.c |  4 
 hw/audio/es1370.c   |  4 
 hw/audio/intel-hda.c|  4 
 hw/char/serial-pci.c| 12 
 hw/display/cirrus_vga.c |  4 
 hw/display/qxl.c|  4 
 hw/display/sm501.c  |  4 
 hw/display/vga-pci.c|  4 
 hw/display/vmware_vga.c |  4 
 hw/i2c/smbus_ich9.c |  4 
 hw/i386/amd_iommu.c |  4 
 hw/i386/kvm/pci-assign.c|  4 
 hw/i386/pc_piix.c   |  4 
 hw/i386/xen/xen_platform.c  |  4 
 hw/i386/xen/xen_pvdevice.c  |  4 
 hw/ide/ich.c|  4 
 hw/ide/pci.c|  4 
 hw/ipack/tpci200.c  |  4 
 hw/isa/i82378.c |  4 
 hw/isa/lpc_ich9.c   |  1 +
 hw/isa/piix4.c  |  4 
 hw/isa/vt82c686.c   | 16 
 hw/mips/gt64xxx_pci.c   |  4 
 hw/misc/edu.c   |  5 +
 hw/misc/ivshmem.c   |  4 
 hw/misc/macio/macio.c   |  4 
 hw/misc/pci-testdev.c   |  4 
 hw/net/e1000.c  |  4 
 hw/net/eepro100.c   |  4 
 hw/net/ne2000.c |  4 
 hw/net/pcnet-pci.c  |  4 
 hw/net/rocker/rocker.c  |  4 
 hw/net/rtl8139.c|  4 
 hw/net/sungem.c |  4 
 hw/net/sunhme.c |  4 
 hw/pci-bridge/dec.c |  8 
 hw/pci-bridge/i82801b11.c   |  4 
 hw/pci-bridge/pci_bridge_dev.c  |  1 +
 hw/pci-bridge/pci_expander_bridge.c |  8 
 hw/pci-host/apb.c   |  8 
 hw/pci-host/bonito.c|  4 
 hw/pci-host/gpex.c  |  4 
 hw/pci-host/grackle.c   |  4 
 hw/pci-host/piix.c  |  8 
 hw/pci-host/ppce500.c   |  4 
 hw

[Qemu-block] [PATCH v2 3/5] pci: Add INTERFACE_PCIE_DEVICE to all PCIe devices

[Eduardo Habkost]

Change all devices that set is_express=1 to implement
INTERFACE_PCIE_DEVICE.

Cc: Keith Busch <keith.bu...@intel.com>
Cc: Kevin Wolf <kw...@redhat.com>
Cc: Max Reitz <mre...@redhat.com>
Cc: Dmitry Fleytman <dmi...@daynix.com>
Cc: Jason Wang <jasow...@redhat.com>
Cc: "Michael S. Tsirkin" <m...@redhat.com>
Cc: Marcel Apfelbaum <mar...@redhat.com>
Cc: Paul Burton <paul.bur...@imgtec.com>
Cc: Paolo Bonzini <pbonz...@redhat.com>
Cc: Hannes Reinecke <h...@suse.com>
Cc: qemu-block@nongnu.org
Reviewed-by: Alistair Francis <alistair.fran...@xilinx.com>
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
Changes v1 -> v2:
* base-xhci is marked as hybrid, now (in another patch)
* Included pcie-pci-bridge
---
 hw/block/nvme.c| 4 
 hw/net/e1000e.c| 4 
 hw/pci-bridge/pcie_pci_bridge.c| 1 +
 hw/pci-bridge/pcie_root_port.c | 4 
 hw/pci-bridge/xio3130_downstream.c | 4 
 hw/pci-bridge/xio3130_upstream.c   | 4 
 hw/pci-host/xilinx-pcie.c  | 4 
 hw/scsi/megasas.c  | 6 ++
 8 files changed, 31 insertions(+)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 9aa32692a3..441e21ed1f 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1110,6 +1110,10 @@ static const TypeInfo nvme_info = {
 .instance_size = sizeof(NvmeCtrl),
 .class_init= nvme_class_init,
 .instance_init = nvme_instance_init,
+.interfaces = (InterfaceInfo[]) {
+{ INTERFACE_PCIE_DEVICE },
+{ }
+},
 };
 
 static void nvme_register_types(void)
diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index 6c42b4478c..81f7934a59 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -708,6 +708,10 @@ static const TypeInfo e1000e_info = {
 .instance_size = sizeof(E1000EState),
 .class_init = e1000e_class_init,
 .instance_init = e1000e_instance_init,
+.interfaces = (InterfaceInfo[]) {
+{ INTERFACE_PCIE_DEVICE },
+{ }
+},
 };
 
 static void e1000e_register_types(void)
diff --git a/hw/pci-bridge/pcie_pci_bridge.c b/hw/pci-bridge/pcie_pci_bridge.c
index 9aa5cc3e45..88db143633 100644
--- a/hw/pci-bridge/pcie_pci_bridge.c
+++ b/hw/pci-bridge/pcie_pci_bridge.c
@@ -180,6 +180,7 @@ static const TypeInfo pcie_pci_bridge_info = {
 .class_init = pcie_pci_bridge_class_init,
 .interfaces = (InterfaceInfo[]) {
 { TYPE_HOTPLUG_HANDLER },
+{ INTERFACE_PCIE_DEVICE },
 { },
 }
 };
diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
index 4d588cb22e..9b6e4ce512 100644
--- a/hw/pci-bridge/pcie_root_port.c
+++ b/hw/pci-bridge/pcie_root_port.c
@@ -161,6 +161,10 @@ static const TypeInfo rp_info = {
 .class_init= rp_class_init,
 .abstract  = true,
 .class_size = sizeof(PCIERootPortClass),
+.interfaces = (InterfaceInfo[]) {
+{ INTERFACE_PCIE_DEVICE },
+{ }
+},
 };
 
 static void rp_register_types(void)
diff --git a/hw/pci-bridge/xio3130_downstream.c 
b/hw/pci-bridge/xio3130_downstream.c
index e706f36cb7..7d2f7629c1 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -195,6 +195,10 @@ static const TypeInfo xio3130_downstream_info = {
 .name  = "xio3130-downstream",
 .parent= TYPE_PCIE_SLOT,
 .class_init= xio3130_downstream_class_init,
+.interfaces = (InterfaceInfo[]) {
+{ INTERFACE_PCIE_DEVICE },
+{ }
+},
 };
 
 static void xio3130_downstream_register_types(void)
diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
index a052224bbf..227997ce46 100644
--- a/hw/pci-bridge/xio3130_upstream.c
+++ b/hw/pci-bridge/xio3130_upstream.c
@@ -166,6 +166,10 @@ static const TypeInfo xio3130_upstream_info = {
 .name  = "x3130-upstream",
 .parent= TYPE_PCIE_PORT,
 .class_init= xio3130_upstream_class_init,
+.interfaces = (InterfaceInfo[]) {
+{ INTERFACE_PCIE_DEVICE },
+{ }
+},
 };
 
 static void xio3130_upstream_register_types(void)
diff --git a/hw/pci-host/xilinx-pcie.c b/hw/pci-host/xilinx-pcie.c
index 4613dda1d2..7659253090 100644
--- a/hw/pci-host/xilinx-pcie.c
+++ b/hw/pci-host/xilinx-pcie.c
@@ -317,6 +317,10 @@ static const TypeInfo xilinx_pcie_root_info = {
 .parent = TYPE_PCI_BRIDGE,
 .instance_size = sizeof(XilinxPCIERoot),
 .class_init = xilinx_pcie_root_class_init,
+.interfaces = (InterfaceInfo[]) {
+{ INTERFACE_PCIE_DEVICE },
+{ }
+},
 };
 
 static void xilinx_pcie_register(void)
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 0db68aacee..535ee267c3 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2451,6 +2451,7 @@ typedef struct MegasasInfo {
 int osts;
 const VMStateDescription *vmsd;
 Property *props;
+InterfaceInfo *interfaces;
 } Megasas

[Qemu-block] [PATCH 2/5] iotests: Set up Python logging

[Eduardo Habkost]

Set up Python logging module instead of relying on
QEMUMachine._debug to enable debugging messages.

Cc: Kevin Wolf <kw...@redhat.com>
Cc: Max Reitz <mre...@redhat.com>
Cc: qemu-block@nongnu.org
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 tests/qemu-iotests/iotests.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 1af117e37d..36a7757aaf 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -28,6 +28,7 @@ import qtest
 import struct
 import json
 import signal
+import logging
 
 
 # This will not work if arguments contain spaces but is necessary if we
@@ -467,6 +468,8 @@ def main(supported_fmts=[], supported_oses=['linux']):
 else:
 output = StringIO.StringIO()
 
+logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN))
+
 class MyTestRunner(unittest.TextTestRunner):
 def __init__(self, stream=output, descriptions=True, 
verbosity=verbosity):
 unittest.TextTestRunner.__init__(self, stream, descriptions, 
verbosity)
-- 
2.13.5

Re: [Qemu-block] [PATCH v2 2/3] block-jobs: Optionally unregister live block operations

[Eduardo Habkost]

On Wed, Sep 06, 2017 at 03:00:41PM +0200, Kevin Wolf wrote:
> Am 30.08.2017 um 19:01 hat Jeff Cody geschrieben:
> > From: Jeffrey Cody 
> > 
> > If configured without live block operations enabled, unregister the
> > live block operation commands.
> > 
> > Signed-off-by: Jeff Cody 
> 
> How sure are we that libvirt will like a qemu that advertises these
> commands in the schema, but doesn't actually provide them?

If libvirt wants to know if a command is available, it uses
'query-commands', not 'query-qmp-schema'.

The only query-qmp-schema elements used by latest libvirt are
gluster-related blockdev-add options (see
libvirt/src/qemu/qemu_capabilities.c:virQEMUCapsQMPSchemaQueries]]).

-- 
Eduardo

Re: [Qemu-block] [Qemu-devel] [PATCH 4/5] pci: Add INTERFACE_LEGACY_PCI_DEVICE to legacy PCI devices

[Eduardo Habkost]

On Mon, Aug 28, 2017 at 06:58:37PM -0400, John Snow wrote:
> 
> 
> On 08/25/2017 03:39 PM, Eduardo Habkost wrote:
> > CCing maintainers of affected devices (sorry for not CCing you
> > before).
> > 
> > On Wed, Aug 23, 2017 at 07:14:44PM -0300, Eduardo Habkost wrote:
> >> Add INTERFACE_LEGACY_PCI_DEVICE to all direct subtypes of
> >> TYPE_PCI_DEVICE, except:
> >>
> >> 1) The ones that already have INTERFACE_PCIE_DEVICE set:
> >>
> >> * base-xhci
> >> * e1000e
> >> * nvme
> >> * pvscsi
> >> * vfio-pci
> >> * virtio-pci
> >> * vmxnet3
> >>
> >> 2) base-pci-bridge
> >>
> >> Not all PCI bridges are legacy PCI devices, so
> >> INTERFACE_LEGACY_PCI_DEVICE is added only to the subtypes that
> >> are actually legacy PCI devices:
> >>
> >> * dec-21154-p2p-bridge
> >> * i82801b11-bridge
> >> * pbm-bridge
> >> * pci-bridge
> >>
> >> The direct subtypes of base-pci-bridge not touched by this patch
> >> are:
> >>
> >> * xilinx-pcie-root: Already marked as PCIe-only device.
> >> * pcie-port: all non-abstract subtypes of pcie-port are already
> >>   marked as PCIe-only devices.
> >>
> >> 3) megasas-base
> >>
> >> Not all megasas devices are legacy PCI devices, so the interface
> >> names are added to the subclasses registered by
> >> megasas_register_types(), according to information in the
> >> megasas_devices[] array.
> >>
> >> "megasas-gen2" already implements INTERFACE_PCIE_DEVICE, so add
> >> INTERFACE_LEGACY_PCI_DEVICE only to "megasas".
> >>
> >> Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
> >> ---
> 
> [...]
> 
> >>  hw/ide/ich.c|  4 
> >>  hw/ide/pci.c|  4 
> 
> Acked-by: John Snow <js...@redhat.com>
> 
> 
> (Random fly-by comment without looking at the other patches: I assume
> there are reasons it's not appropriate or good to add a legacy PCI
> device parent that we inherit from, and it's instead better to manually
> add the property to all children?)

Yes, the reason I'm using interfaces instead of regular
inheritance is the existence of hybrid devices (see patch 2/5).

-- 
Eduardo

Re: [Qemu-block] [Qemu-devel] [PATCH 4/5] pci: Add INTERFACE_LEGACY_PCI_DEVICE to legacy PCI devices

[Eduardo Habkost]

CCing maintainers of affected devices (sorry for not CCing you
before).

On Wed, Aug 23, 2017 at 07:14:44PM -0300, Eduardo Habkost wrote:
> Add INTERFACE_LEGACY_PCI_DEVICE to all direct subtypes of
> TYPE_PCI_DEVICE, except:
> 
> 1) The ones that already have INTERFACE_PCIE_DEVICE set:
> 
> * base-xhci
> * e1000e
> * nvme
> * pvscsi
> * vfio-pci
> * virtio-pci
> * vmxnet3
> 
> 2) base-pci-bridge
> 
> Not all PCI bridges are legacy PCI devices, so
> INTERFACE_LEGACY_PCI_DEVICE is added only to the subtypes that
> are actually legacy PCI devices:
> 
> * dec-21154-p2p-bridge
> * i82801b11-bridge
> * pbm-bridge
> * pci-bridge
> 
> The direct subtypes of base-pci-bridge not touched by this patch
> are:
> 
> * xilinx-pcie-root: Already marked as PCIe-only device.
> * pcie-port: all non-abstract subtypes of pcie-port are already
>   marked as PCIe-only devices.
> 
> 3) megasas-base
> 
> Not all megasas devices are legacy PCI devices, so the interface
> names are added to the subclasses registered by
> megasas_register_types(), according to information in the
> megasas_devices[] array.
> 
> "megasas-gen2" already implements INTERFACE_PCIE_DEVICE, so add
> INTERFACE_LEGACY_PCI_DEVICE only to "megasas".
> 
> Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
> ---
>  hw/acpi/piix4.c |  1 +
>  hw/audio/ac97.c |  4 
>  hw/audio/es1370.c   |  4 
>  hw/audio/intel-hda.c|  4 
>  hw/char/serial-pci.c| 12 
>  hw/display/cirrus_vga.c |  4 
>  hw/display/qxl.c|  4 
>  hw/display/sm501.c  |  4 
>  hw/display/vga-pci.c|  4 
>  hw/display/vmware_vga.c |  4 
>  hw/i2c/smbus_ich9.c |  4 
>  hw/i386/amd_iommu.c |  4 
>  hw/i386/kvm/pci-assign.c|  4 
>  hw/i386/pc_piix.c   |  4 
>  hw/i386/xen/xen_platform.c  |  4 
>  hw/i386/xen/xen_pvdevice.c  |  4 
>  hw/ide/ich.c|  4 
>  hw/ide/pci.c|  4 
>  hw/ipack/tpci200.c  |  4 
>  hw/isa/i82378.c |  4 
>  hw/isa/lpc_ich9.c   |  1 +
>  hw/isa/piix4.c  |  4 
>  hw/isa/vt82c686.c   | 16 
>  hw/mips/gt64xxx_pci.c   |  4 
>  hw/misc/edu.c   |  5 +
>  hw/misc/ivshmem.c   |  4 
>  hw/misc/macio/macio.c   |  4 
>  hw/misc/pci-testdev.c   |  4 
>  hw/net/e1000.c  |  4 
>  hw/net/eepro100.c   |  4 
>  hw/net/ne2000.c |  4 
>  hw/net/pcnet-pci.c  |  4 
>  hw/net/rocker/rocker.c  |  4 
>  hw/net/rtl8139.c|  4 
>  hw/pci-bridge/dec.c |  8 
>  hw/pci-bridge/i82801b11.c   |  4 
>  hw/pci-bridge/pci_bridge_dev.c  |  1 +
>  hw/pci-bridge/pci_expander_bridge.c |  8 
>  hw/pci-host/apb.c   |  8 
>  hw/pci-host/bonito.c|  4 
>  hw/pci-host/gpex.c  |  4 
>  hw/pci-host/grackle.c   |  4 
>  hw/pci-host/piix.c  |  8 
>  hw/pci-host/ppce500.c   |  4 
>  hw/pci-host/prep.c  |  4 
>  hw/pci-host/q35.c   |  4 
>  hw/pci-host/uninorth.c  | 16 
>  hw/pci-host/versatile.c |  4 
>  hw/ppc/ppc4xx_pci.c |  4 
>  hw/scsi/esp-pci.c   |  4 
>  hw/scsi/lsi53c895a.c|  4 
>  hw/scsi/megasas.c   |  4 
>  hw/scsi/mptsas.c|  4 
>  hw/sd/sdhci.c   |  4 
>  hw/sh4/sh_pci.c |  4 
>  hw/sparc64/sun4u.c  |  4 
>  hw/usb/hcd-ehci-pci.c   |  4 
>  hw/usb/hcd-ohci.c   |  4 
>  hw/usb/hcd-uhci.c   |  4 
>  hw/vfio/pci-quirks.c|  4 
>  hw/watchdog/wdt_i6300esb.c  |  4 
>  hw/xen/xen_pt.c |  4 
>  62 files changed, 288 insertions(+)
> 
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index f276967..defe98a 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -723,6 +723,7 @@ static const TypeInfo piix4_pm_info = {
>  .interfaces = (Inter

Re: [Qemu-block] [Qemu-devel] [PATCH 3/5] pci: Add INTERFACE_PCIE_DEVICE to all PCIe devices

[Eduardo Habkost]

CCing maintainers of affected devices (sorry for not CCing you
before).

On Wed, Aug 23, 2017 at 07:14:43PM -0300, Eduardo Habkost wrote:
> Change all devices that set is_express=1 to implement
> INTERFACE_PCIE_DEVICE.
> 
> Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
> ---
>  hw/block/nvme.c| 4 
>  hw/net/e1000e.c| 4 
>  hw/pci-bridge/pcie_root_port.c | 4 
>  hw/pci-bridge/xio3130_downstream.c | 4 
>  hw/pci-bridge/xio3130_upstream.c   | 4 
>  hw/pci-host/xilinx-pcie.c  | 4 
>  hw/scsi/megasas.c  | 6 ++
>  hw/usb/hcd-xhci.c  | 4 
>  8 files changed, 34 insertions(+)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 6071dc1..26d58b6 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1110,6 +1110,10 @@ static const TypeInfo nvme_info = {
>  .instance_size = sizeof(NvmeCtrl),
>  .class_init= nvme_class_init,
>  .instance_init = nvme_instance_init,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ }
> +},
>  };
>  
>  static void nvme_register_types(void)
> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> index 6c42b44..81f7934 100644
> --- a/hw/net/e1000e.c
> +++ b/hw/net/e1000e.c
> @@ -708,6 +708,10 @@ static const TypeInfo e1000e_info = {
>  .instance_size = sizeof(E1000EState),
>  .class_init = e1000e_class_init,
>  .instance_init = e1000e_instance_init,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ }
> +},
>  };
>  
>  static void e1000e_register_types(void)
> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
> index 4d588cb..9b6e4ce 100644
> --- a/hw/pci-bridge/pcie_root_port.c
> +++ b/hw/pci-bridge/pcie_root_port.c
> @@ -161,6 +161,10 @@ static const TypeInfo rp_info = {
>  .class_init= rp_class_init,
>  .abstract  = true,
>  .class_size = sizeof(PCIERootPortClass),
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ }
> +},
>  };
>  
>  static void rp_register_types(void)
> diff --git a/hw/pci-bridge/xio3130_downstream.c 
> b/hw/pci-bridge/xio3130_downstream.c
> index e706f36..7d2f762 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -195,6 +195,10 @@ static const TypeInfo xio3130_downstream_info = {
>  .name  = "xio3130-downstream",
>  .parent= TYPE_PCIE_SLOT,
>  .class_init= xio3130_downstream_class_init,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ }
> +},
>  };
>  
>  static void xio3130_downstream_register_types(void)
> diff --git a/hw/pci-bridge/xio3130_upstream.c 
> b/hw/pci-bridge/xio3130_upstream.c
> index a052224..227997c 100644
> --- a/hw/pci-bridge/xio3130_upstream.c
> +++ b/hw/pci-bridge/xio3130_upstream.c
> @@ -166,6 +166,10 @@ static const TypeInfo xio3130_upstream_info = {
>  .name  = "x3130-upstream",
>  .parent= TYPE_PCIE_PORT,
>  .class_init= xio3130_upstream_class_init,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ }
> +},
>  };
>  
>  static void xio3130_upstream_register_types(void)
> diff --git a/hw/pci-host/xilinx-pcie.c b/hw/pci-host/xilinx-pcie.c
> index 4613dda..7659253 100644
> --- a/hw/pci-host/xilinx-pcie.c
> +++ b/hw/pci-host/xilinx-pcie.c
> @@ -317,6 +317,10 @@ static const TypeInfo xilinx_pcie_root_info = {
>  .parent = TYPE_PCI_BRIDGE,
>  .instance_size = sizeof(XilinxPCIERoot),
>  .class_init = xilinx_pcie_root_class_init,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ }
> +},
>  };
>  
>  static void xilinx_pcie_register(void)
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 734fdae..3641c30 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -2451,6 +2451,7 @@ typedef struct MegasasInfo {
>  int osts;
>  const VMStateDescription *vmsd;
>  Property *props;
> +InterfaceInfo *interfaces;
>  } MegasasInfo;
>  
>  static struct MegasasInfo megasas_devices[] = {
> @@ -2480,6 +2481,10 @@ static struct MegasasInfo megasas_devices[] = {
>  .is_express = true,
>  .vmsd = _megasas_gen2,
>  .props = megasas_properties_gen2,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ }
> +},
>  }
>  };
>  
> @@ -2531,6 +2536,7

Re: [Qemu-block] [RFC v4 08/13] ide: enumerate_slots implementation

[Eduardo Habkost]

On Wed, Aug 16, 2017 at 05:46:18PM -0400, John Snow wrote:
> 
> 
> On 08/14/2017 05:57 PM, Eduardo Habkost wrote:
> > Example output when using "-machine q35":
> > 
> >   {
> > "available": true,
> > "count": 1,
> > "device-types": [
> >   "ide-device"
> > ],
> > "hotpluggable": false,
> > "opts": [
> >   { "option": "unit", "values": 0 },
> >   { "option": "bus", "values": "ide.2" }
> > ],
> > "opts-complete": true
> >   }
> >   {
> > "available": false,
> > "count": 1,
> > "device": "/machine/unattached/device[19]",
> > "device-types": [
> >   "ide-device"
> > ],
> > "hotpluggable": false,
> > "opts": [
> >   { "option": "unit", "values": 1 },
> >   { "option": "bus", "values": "ide.2" } ],
> > "opts-complete": true
> >   }
> >   {
> > "available": true,
> > "count": 10,
> > "device-types": [
> >   "ide-device"
> > ],
> > "hotpluggable": false,
> > "opts": [
> >   { "option": "unit", "values": [ [ 0, 1 ] ] },
> 
> Hm, these unit values aren't really correct -- we do not support
> primary/secondary semantics for IDE buses on the AHCI device. I guess
> they technically exist, but you cannot use them for anything.
> 
> Should I do something to "disable" or otherwise hide the unusable
> secondary unit slots for AHCI devices?

If the device is already rejecting -device ...,unit=1, then the
bug is in my implementation of enumerate_devices.  Maybe it
should just look at IDEBus::max_units to find that out?

-- 
Eduardo

[Qemu-block] [RFC v4 08/13] ide: enumerate_slots implementation

[Eduardo Habkost]

Example output when using "-machine q35":

  {
"available": true,
"count": 1,
"device-types": [
  "ide-device"
],
"hotpluggable": false,
"opts": [
  { "option": "unit", "values": 0 },
  { "option": "bus", "values": "ide.2" }
],
"opts-complete": true
  }
  {
"available": false,
"count": 1,
"device": "/machine/unattached/device[19]",
"device-types": [
  "ide-device"
],
"hotpluggable": false,
"opts": [
  { "option": "unit", "values": 1 },
  { "option": "bus", "values": "ide.2" } ],
"opts-complete": true
  }
  {
"available": true,
"count": 10,
"device-types": [
  "ide-device"
],
"hotpluggable": false,
"opts": [
  { "option": "unit", "values": [ [ 0, 1 ] ] },
  { "option": "bus", "values": [ "ide.4", "ide.3", "ide.5", "ide.0", 
"ide.1" ] }
],
"opts-complete": true
  }

Cc: John Snow <js...@redhat.com>
Cc: qemu-block@nongnu.org
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 hw/ide/qdev.c | 26 ++
 1 file changed, 26 insertions(+)

diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index f17da1f..cc96f6f 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -25,6 +25,7 @@
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
 #include "hw/block/block.h"
+#include "hw/qdev-slotinfo.h"
 #include "sysemu/sysemu.h"
 #include "qapi/visitor.h"
 
@@ -38,6 +39,30 @@ static Property ide_props[] = {
 DEFINE_PROP_END_OF_LIST(),
 };
 
+static DeviceSlotInfoList *ide_bus_enumerate_slots(BusState *bus)
+{
+int unit;
+DeviceSlotInfoList *r = NULL;
+IDEBus *ib = IDE_BUS(bus);
+
+for (unit = 0; unit < 2; unit++) {
+DeviceSlotInfo *s = make_slot(bus);
+IDEDevice *dev = (unit ? ib->master : ib->slave);
+slot_add_opt_int(s, "unit", unit);
+s->opts_complete = true;
+s->has_count = true;
+s->count = 1;
+if (dev) {
+s->available = false;
+s->has_device = true;
+s->device = object_get_canonical_path(OBJECT(dev));
+}
+slot_list_add_slot(, s);
+}
+
+return r;
+}
+
 static void ide_bus_class_init(ObjectClass *klass, void *data)
 {
 BusClass *k = BUS_CLASS(klass);
@@ -45,6 +70,7 @@ static void ide_bus_class_init(ObjectClass *klass, void *data)
 k->get_fw_dev_path = idebus_get_fw_dev_path;
 k->unrealize = idebus_unrealize;
 k->device_type = TYPE_IDE_DEVICE;
+k->enumerate_slots = ide_bus_enumerate_slots;
 }
 
 static void idebus_unrealize(BusState *bus, Error **errp)
-- 
2.9.4

[Qemu-block] [PATCH 2/5] block: Don't try to set *errp directly

[Eduardo Habkost]

Assigning directly to *errp is not valid, as errp may be NULL,
_fatal, or _abort.  Use error_propagate() instead.

With this, there's no need to check if errp is NULL anymore, as
error_propagate() and error_prepend() are able to handle that.

Cc: Kevin Wolf <kw...@redhat.com>
Cc: Max Reitz <mre...@redhat.com>
Cc: qemu-block@nongnu.org
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 block.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index fa1d06d846..1750a1838e 100644
--- a/block.c
+++ b/block.c
@@ -4263,11 +4263,9 @@ bool bdrv_op_is_blocked(BlockDriverState *bs, 
BlockOpType op, Error **errp)
 assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
 if (!QLIST_EMPTY(>op_blockers[op])) {
 blocker = QLIST_FIRST(>op_blockers[op]);
-if (errp) {
-*errp = error_copy(blocker->reason);
-error_prepend(errp, "Node '%s' is busy: ",
-  bdrv_get_device_or_node_name(bs));
-}
+error_propagate(errp, error_copy(blocker->reason));
+error_prepend(errp, "Node '%s' is busy: ",
+  bdrv_get_device_or_node_name(bs));
 return true;
 }
 return false;
-- 
2.11.0.259.g40922b1

[Qemu-block] [PULL 10/22] sysbus-ahci: Remove user_creatable flag

[Eduardo Habkost]

The sysbus-ahci devices are supposed to be created and wired by
code from other devices, like calxeda_init() and
xlnx_zynqmp_realize(), and won't work with -device. Remove the
user_creatable flag from the device class.

Cc: John Snow <js...@redhat.com>
Cc: qemu-block@nongnu.org
Cc: Rob Herring <r...@kernel.org>
Cc: Peter Maydell <peter.mayd...@linaro.org>
Cc: Alistair Francis <alistair.fran...@xilinx.com>
Cc: "Edgar E. Iglesias" <edgar.igles...@gmail.com>
Cc: Marcel Apfelbaum <mar...@redhat.com>
Reviewed-by: Alistair Francis <alistair.fran...@xilinx.com>
Acked-by: John Snow <js...@redhat.com>
Acked-by: Marcel Apfelbaum <mar...@redhat.com>
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
Message-Id: <20170503203604.31462-11-ehabk...@redhat.com>
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 hw/ide/ahci.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 7f10cda354..2ea1a282ef 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1721,11 +1721,6 @@ static void sysbus_ahci_class_init(ObjectClass *klass, 
void *data)
 dc->props = sysbus_ahci_properties;
 dc->reset = sysbus_ahci_reset;
 set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-/*
- * FIXME: Set only because we are not sure yet if this device
- * will be outside the q35 sysbus whitelist.
- */
-dc->user_creatable = true;
 }
 
 static const TypeInfo sysbus_ahci_info = {
-- 
2.11.0.259.g40922b1

[Qemu-block] [PULL 02/22] sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE

[Eduardo Habkost]

commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
cannot_instantiate_with_device_add_yet in TYPE_SYSBUS, making all
sysbus devices appear on "-device help" and lack the "no-user"
flag in "info qdm".

To fix this, we can set user_creatable=false by default on
TYPE_SYS_BUS_DEVICE, but this requires setting
user_creatable=true explicitly on the sysbus devices that
actually work with -device.

Fortunately today we have just a few has_dynamic_sysbus=1
machines: virt, pc-q35-*, ppce500, and spapr.

virt, ppce500, and spapr have extra checks to ensure just a few
device types can be instantiated:

* virt supports only TYPE_VFIO_CALXEDA_XGMAC, TYPE_VFIO_AMD_XGBE.
* ppce500 supports only TYPE_ETSEC_COMMON.
* spapr supports only TYPE_SPAPR_PCI_HOST_BRIDGE.

This patch sets user_creatable=true explicitly on those 4 device
classes.

Now, the more complex cases:

pc-q35-*: q35 has no sysbus device whitelist yet (which is a
separate bug). We are in the process of fixing it and building a
sysbus whitelist on q35, but in the meantime we can fix the
"-device help" and "info qdm" bugs mentioned above. Also, despite
not being strictly necessary for fixing the q35 bug, reducing the
list of user_creatable=true devices will help us be more
confident when building the q35 whitelist.

xen: We also have a hack at xen_set_dynamic_sysbus(), that sets
has_dynamic_sysbus=true at runtime when using the Xen
accelerator. This hack is only used to allow xen-backend devices
to be dynamically plugged/unplugged.

This means today we can use -device with the following 22 device
types, that are the ones compiled into the qemu-system-x86_64 and
qemu-system-i386 binaries:

* allwinner-ahci
* amd-iommu
* cfi.pflash01
* esp
* fw_cfg_io
* fw_cfg_mem
* generic-sdhci
* hpet
* intel-iommu
* ioapic
* isabus-bridge
* kvmclock
* kvm-ioapic
* kvmvapic
* SUNW,fdtwo
* sysbus-ahci
* sysbus-fdc
* sysbus-ohci
* unimplemented-device
* virtio-mmio
* xen-backend
* xen-sysdev

This patch adds user_creatable=true explicitly to those devices,
temporarily, just to keep 100% compatibility with existing
behavior of q35. Subsequent patches will remove
user_creatable=true from the devices that are really not meant to
user-creatable on any machine, and remove the FIXME comment from
the ones that are really supposed to be user-creatable. This is
being done in separate patches because we still don't have an
obvious list of devices that will be whitelisted by q35, and I
would like to get each device reviewed individually.

Cc: Alexander Graf <ag...@suse.de>
Cc: Alex Williamson <alex.william...@redhat.com>
Cc: Alistair Francis <alistair.fran...@xilinx.com>
Cc: Beniamino Galvani <b.galv...@gmail.com>
Cc: Christian Borntraeger <borntrae...@de.ibm.com>
Cc: Cornelia Huck <cornelia.h...@de.ibm.com>
Cc: David Gibson <da...@gibson.dropbear.id.au>
Cc: "Edgar E. Iglesias" <edgar.igles...@gmail.com>
Cc: Eduardo Habkost <ehabk...@redhat.com>
Cc: Frank Blaschka <frank.blasc...@de.ibm.com>
Cc: Gabriel L. Somlo <so...@cmu.edu>
Cc: Gerd Hoffmann <kra...@redhat.com>
Cc: Igor Mammedov <imamm...@redhat.com>
Cc: Jason Wang <jasow...@redhat.com>
Cc: John Snow <js...@redhat.com>
Cc: Juergen Gross <jgr...@suse.com>
Cc: Kevin Wolf <kw...@redhat.com>
Cc: Laszlo Ersek <ler...@redhat.com>
Cc: Marcel Apfelbaum <mar...@redhat.com>
Cc: Markus Armbruster <arm...@redhat.com>
Cc: Max Reitz <mre...@redhat.com>
Cc: "Michael S. Tsirkin" <m...@redhat.com>
Cc: Paolo Bonzini <pbonz...@redhat.com>
Cc: Peter Maydell <peter.mayd...@linaro.org>
Cc: Pierre Morel <pmo...@linux.vnet.ibm.com>
Cc: Prasad J Pandit <p...@fedoraproject.org>
Cc: qemu-...@nongnu.org
Cc: qemu-block@nongnu.org
Cc: qemu-...@nongnu.org
Cc: Richard Henderson <r...@twiddle.net>
Cc: Rob Herring <r...@kernel.org>
Cc: Shannon Zhao <zhaoshengl...@huawei.com>
Cc: sstabell...@kernel.org
Cc: Thomas Huth <th...@redhat.com>
Cc: Yi Min Zhao <zyi...@linux.vnet.ibm.com>
Acked-by: John Snow <js...@redhat.com>
Acked-by: Juergen Gross <jgr...@suse.com>
Acked-by: Marcel Apfelbaum <mar...@redhat.com>
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
Message-Id: <20170503203604.31462-3-ehabk...@redhat.com>
Reviewed-by: Markus Armbruster <arm...@redhat.com>
[ehabkost: Small changes at sysbus_device_class_init() comments]
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 hw/block/fdc.c   | 10 ++
 hw/block/pflash_cfi01.c  |  5 +
 hw/core/sysbus.c | 11 +++
 hw/i386/amd_iommu.c  |  5 +
 hw/i386/intel_iommu.c|  5 +
 hw/i386/kvm/clock.c  |  5 +
 hw/i386/kvm/ioapic.c |  5 +
 hw/i386/kvmvapic.c   |  5 +
 hw/ide/ahci.c| 10 ++
 hw/intc/ioapic.c |  5 +
 hw/isa/

[Qemu-block] [PULL 06/22] pflash_cfi01: Remove user_creatable flag

[Eduardo Habkost]

TYPE_CFI_PFLASH01 devices need to be mapped by
pflash_cfi01_register() (or equivalent) and can't be used with
-device. Remove user_creatable from the device class.

Cc: Kevin Wolf <kw...@redhat.com>
Cc: Max Reitz <mre...@redhat.com>
Cc: qemu-block@nongnu.org
Cc: Laszlo Ersek <ler...@redhat.com>
Cc: Philippe Mathieu-Daudé <f4...@amsat.org>
Cc: Marcel Apfelbaum <mar...@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
Acked-by: Marcel Apfelbaum <mar...@redhat.com>
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
Message-Id: <20170503203604.31462-7-ehabk...@redhat.com>
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 hw/block/pflash_cfi01.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index ef71956433..594d4cf6fe 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -927,11 +927,6 @@ static void pflash_cfi01_class_init(ObjectClass *klass, 
void *data)
 dc->props = pflash_cfi01_properties;
 dc->vmsd = _pflash;
 set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-/*
- * FIXME: Set only because we are not sure yet if this device
- * will be outside the q35 sysbus whitelist.
- */
-dc->user_creatable = true;
 }
 
 
-- 
2.11.0.259.g40922b1

[Qemu-block] [PULL 05/22] fdc: Remove user_creatable flag from sysbus-fdc & SUNW, fdtwo

[Eduardo Habkost]

sysbus-fdc and SUNW,fdtwo devices need IRQs to be wired and mmio
to be mapped, and can't be used with -device. Unset
user_creatable on their device classes.

Cc: John Snow <js...@redhat.com>
Cc: Kevin Wolf <kw...@redhat.com>
Cc: Marcel Apfelbaum <mar...@redhat.com>
Cc: Max Reitz <mre...@redhat.com>
Cc: qemu-block@nongnu.org
Cc: Thomas Huth <th...@redhat.com>
Acked-by: John Snow <js...@redhat.com>
Reviewed-by: Thomas Huth <th...@redhat.com>
Acked-by: Marcel Apfelbaum <mar...@redhat.com>
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
Message-Id: <20170503203604.31462-6-ehabk...@redhat.com>
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 hw/block/fdc.c | 10 --
 1 file changed, 10 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 5c28a0b0ad..2e629b398b 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2880,11 +2880,6 @@ static void sysbus_fdc_class_init(ObjectClass *klass, 
void *data)
 
 dc->props = sysbus_fdc_properties;
 set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-/*
- * FIXME: Set only because we are not sure yet if this device
- * will be outside the q35 sysbus whitelist.
- */
-dc->user_creatable = true;
 }
 
 static const TypeInfo sysbus_fdc_info = {
@@ -2911,11 +2906,6 @@ static void sun4m_fdc_class_init(ObjectClass *klass, 
void *data)
 
 dc->props = sun4m_fdc_properties;
 set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-/*
- * FIXME: Set only because we are not sure yet if this device
- * will be outside the q35 sysbus whitelist.
- */
-dc->user_creatable = true;
 }
 
 static const TypeInfo sun4m_fdc_info = {
-- 
2.11.0.259.g40922b1

[Qemu-block] [PATCH] fixup! sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE

[Eduardo Habkost]

On Fri, May 05, 2017 at 01:54:19PM -0300, Eduardo Habkost wrote:
> On Fri, May 05, 2017 at 09:36:22AM +0200, Cornelia Huck wrote:
> [...]
> > > diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> > > index c0f560b289..6a2eec8dd0 100644
> > > --- a/hw/core/sysbus.c
> > > +++ b/hw/core/sysbus.c
> > > @@ -326,6 +326,17 @@ static void sysbus_device_class_init(ObjectClass 
> > > *klass, void *data)
> > >  DeviceClass *k = DEVICE_CLASS(klass);
> > >  k->init = sysbus_device_init;
> > >  k->bus_type = TYPE_SYSTEM_BUS;
> > > +/*
> > > + * device_add plugs devices into suitable bus.  For "real" buses,
> > 
> > s/suitable/a suitable/
> 
> Thanks, I will fix it.
> 
> > 
> > > + * that actually connects the device.  For sysbus, the connections
> > > + * need to be made separately, and device_add can't do that.  The
> > > + * device would be left unconnected, and will probably not work
> > > + *
> > > + * However, a few machines and a few devices can handle a few sysbus
> > > + * devices. In this case, the device subclass needs to override
> > 
> > Should that rather be "a few machines can handle a few specific sysbus
> > devices"?
> 
> I will extend it to "a few machines can handle device_add for a
> few specific sysbus devices". Thanks for spotting it.

Fixup will be included in v3, or can be applied by maintainer:

Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 hw/core/sysbus.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 6a2eec8dd0..5d0887f499 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -327,14 +327,14 @@ static void sysbus_device_class_init(ObjectClass *klass, 
void *data)
 k->init = sysbus_device_init;
 k->bus_type = TYPE_SYSTEM_BUS;
 /*
- * device_add plugs devices into suitable bus.  For "real" buses,
+ * device_add plugs devices into a suitable bus.  For "real" buses,
  * that actually connects the device.  For sysbus, the connections
  * need to be made separately, and device_add can't do that.  The
  * device would be left unconnected, and will probably not work
  *
- * However, a few machines and a few devices can handle a few sysbus
- * devices. In this case, the device subclass needs to override
- * it and set user_creatable=true.
+ * However, a few machines can handle device_add/-device with
+ * a few specific sysbus devices. In those cases, the device
+ * subclass needs to override it and set user_creatable=true.
  */
 k->user_creatable = false;
 }
-- 
2.11.0.259.g40922b1

Re: [Qemu-block] [PATCH RESEND v2 02/21] sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE

[Eduardo Habkost]

On Fri, May 05, 2017 at 09:36:22AM +0200, Cornelia Huck wrote:
[...]
> > diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> > index c0f560b289..6a2eec8dd0 100644
> > --- a/hw/core/sysbus.c
> > +++ b/hw/core/sysbus.c
> > @@ -326,6 +326,17 @@ static void sysbus_device_class_init(ObjectClass 
> > *klass, void *data)
> >  DeviceClass *k = DEVICE_CLASS(klass);
> >  k->init = sysbus_device_init;
> >  k->bus_type = TYPE_SYSTEM_BUS;
> > +/*
> > + * device_add plugs devices into suitable bus.  For "real" buses,
> 
> s/suitable/a suitable/

Thanks, I will fix it.

> 
> > + * that actually connects the device.  For sysbus, the connections
> > + * need to be made separately, and device_add can't do that.  The
> > + * device would be left unconnected, and will probably not work
> > + *
> > + * However, a few machines and a few devices can handle a few sysbus
> > + * devices. In this case, the device subclass needs to override
> 
> Should that rather be "a few machines can handle a few specific sysbus
> devices"?

I will extend it to "a few machines can handle device_add for a
few specific sysbus devices". Thanks for spotting it.

> 
> > + * it and set user_creatable=true.
> > + */
> > +k->user_creatable = false;
> >  }
> > 
> >  static const TypeInfo sysbus_device_type_info = {
> 

-- 
Eduardo

[Qemu-block] [PATCH RESEND v2 11/21] allwinner-ahci: Remove user_creatable flag

[Eduardo Habkost]

allwinner-ahci needs its IRQ to be connected and mmio to be
mapped (this is done by the alwinner-a10 device realize method),
and won't work with -device. Remove the user_creatable flag from
the device class.

Cc: John Snow <js...@redhat.com>
Cc: qemu-block@nongnu.org
Cc: Beniamino Galvani <b.galv...@gmail.com>
Cc: Peter Maydell <peter.mayd...@linaro.org>
Cc: qemu-...@nongnu.org
Cc: Marcel Apfelbaum <mar...@redhat.com>
Acked-by: John Snow <js...@redhat.com>
Acked-by: Marcel Apfelbaum <mar...@redhat.com>
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
Changes v1 -> v2:
* Commit message rewrite only
---
 hw/ide/ahci.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 2ea1a282ef..f60826d6e0 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1815,11 +1815,6 @@ static void allwinner_ahci_class_init(ObjectClass 
*klass, void *data)
 DeviceClass *dc = DEVICE_CLASS(klass);
 
 dc->vmsd = _allwinner_ahci;
-/*
- * FIXME: Set only because we are not sure yet if this device
- * will be outside the q35 sysbus whitelist.
- */
-dc->user_creatable = true;
 }
 
 static const TypeInfo allwinner_ahci_info = {
-- 
2.11.0.259.g40922b1

[Qemu-block] [PATCH RESEND v2 06/21] pflash_cfi01: Remove user_creatable flag

[Eduardo Habkost]

TYPE_CFI_PFLASH01 devices need to be mapped by
pflash_cfi01_register() (or equivalent) and can't be used with
-device. Remove user_creatable from the device class.

Cc: Kevin Wolf <kw...@redhat.com>
Cc: Max Reitz <mre...@redhat.com>
Cc: qemu-block@nongnu.org
Cc: Laszlo Ersek <ler...@redhat.com>
Cc: Philippe Mathieu-Daudé <f4...@amsat.org>
Cc: Marcel Apfelbaum <mar...@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
Acked-by: Marcel Apfelbaum <mar...@redhat.com>
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
Changes v1 -> v2:
* Commit message rewrite only
---
 hw/block/pflash_cfi01.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index ef71956433..594d4cf6fe 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -927,11 +927,6 @@ static void pflash_cfi01_class_init(ObjectClass *klass, 
void *data)
 dc->props = pflash_cfi01_properties;
 dc->vmsd = _pflash;
 set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-/*
- * FIXME: Set only because we are not sure yet if this device
- * will be outside the q35 sysbus whitelist.
- */
-dc->user_creatable = true;
 }
 
 
-- 
2.11.0.259.g40922b1

[Qemu-block] [PATCH RESEND v2 10/21] sysbus-ahci: Remove user_creatable flag

[Eduardo Habkost]

The sysbus-ahci devices are supposed to be created and wired by
code from other devices, like calxeda_init() and
xlnx_zynqmp_realize(), and won't work with -device. Remove the
user_creatable flag from the device class.

Cc: John Snow <js...@redhat.com>
Cc: qemu-block@nongnu.org
Cc: Rob Herring <r...@kernel.org>
Cc: Peter Maydell <peter.mayd...@linaro.org>
Cc: Alistair Francis <alistair.fran...@xilinx.com>
Cc: "Edgar E. Iglesias" <edgar.igles...@gmail.com>
Cc: Marcel Apfelbaum <mar...@redhat.com>
Reviewed-by: Alistair Francis <alistair.fran...@xilinx.com>
Acked-by: John Snow <js...@redhat.com>
Acked-by: Marcel Apfelbaum <mar...@redhat.com>
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
Changes v1 -> v2:
* (none)
---
 hw/ide/ahci.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 7f10cda354..2ea1a282ef 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1721,11 +1721,6 @@ static void sysbus_ahci_class_init(ObjectClass *klass, 
void *data)
 dc->props = sysbus_ahci_properties;
 dc->reset = sysbus_ahci_reset;
 set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-/*
- * FIXME: Set only because we are not sure yet if this device
- * will be outside the q35 sysbus whitelist.
- */
-dc->user_creatable = true;
 }
 
 static const TypeInfo sysbus_ahci_info = {
-- 
2.11.0.259.g40922b1

[Qemu-block] [PATCH RESEND v2 02/21] sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE

[Eduardo Habkost]

commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
cannot_instantiate_with_device_add_yet in TYPE_SYSBUS, making all
sysbus devices appear on "-device help" and lack the "no-user"
flag in "info qdm".

To fix this, we can set user_creatable=false by default on
TYPE_SYS_BUS_DEVICE, but this requires setting
user_creatable=true explicitly on the sysbus devices that
actually work with -device.

Fortunately today we have just a few has_dynamic_sysbus=1
machines: virt, pc-q35-*, ppce500, and spapr.

virt, ppce500, and spapr have extra checks to ensure just a few
device types can be instantiated:

* virt supports only TYPE_VFIO_CALXEDA_XGMAC, TYPE_VFIO_AMD_XGBE.
* ppce500 supports only TYPE_ETSEC_COMMON.
* spapr supports only TYPE_SPAPR_PCI_HOST_BRIDGE.

This patch sets user_creatable=true explicitly on those 4 device
classes.

Now, the more complex cases:

pc-q35-*: q35 has no sysbus device whitelist yet (which is a
separate bug). We are in the process of fixing it and building a
sysbus whitelist on q35, but in the meantime we can fix the
"-device help" and "info qdm" bugs mentioned above. Also, despite
not being strictly necessary for fixing the q35 bug, reducing the
list of user_creatable=true devices will help us be more
confident when building the q35 whitelist.

xen: We also have a hack at xen_set_dynamic_sysbus(), that sets
has_dynamic_sysbus=true at runtime when using the Xen
accelerator. This hack is only used to allow xen-backend devices
to be dynamically plugged/unplugged.

This means today we can use -device with the following 22 device
types, that are the ones compiled into the qemu-system-x86_64 and
qemu-system-i386 binaries:

* allwinner-ahci
* amd-iommu
* cfi.pflash01
* esp
* fw_cfg_io
* fw_cfg_mem
* generic-sdhci
* hpet
* intel-iommu
* ioapic
* isabus-bridge
* kvmclock
* kvm-ioapic
* kvmvapic
* SUNW,fdtwo
* sysbus-ahci
* sysbus-fdc
* sysbus-ohci
* unimplemented-device
* virtio-mmio
* xen-backend
* xen-sysdev

This patch adds user_creatable=true explicitly to those devices,
temporarily, just to keep 100% compatibility with existing
behavior of q35. Subsequent patches will remove
user_creatable=true from the devices that are really not meant to
user-creatable on any machine, and remove the FIXME comment from
the ones that are really supposed to be user-creatable. This is
being done in separate patches because we still don't have an
obvious list of devices that will be whitelisted by q35, and I
would like to get each device reviewed individually.

Cc: Alexander Graf <ag...@suse.de>
Cc: Alex Williamson <alex.william...@redhat.com>
Cc: Alistair Francis <alistair.fran...@xilinx.com>
Cc: Beniamino Galvani <b.galv...@gmail.com>
Cc: Christian Borntraeger <borntrae...@de.ibm.com>
Cc: Cornelia Huck <cornelia.h...@de.ibm.com>
Cc: David Gibson <da...@gibson.dropbear.id.au>
Cc: "Edgar E. Iglesias" <edgar.igles...@gmail.com>
Cc: Eduardo Habkost <ehabk...@redhat.com>
Cc: Frank Blaschka <frank.blasc...@de.ibm.com>
Cc: Gabriel L. Somlo <so...@cmu.edu>
Cc: Gerd Hoffmann <kra...@redhat.com>
Cc: Igor Mammedov <imamm...@redhat.com>
Cc: Jason Wang <jasow...@redhat.com>
Cc: John Snow <js...@redhat.com>
Cc: Juergen Gross <jgr...@suse.com>
Cc: Kevin Wolf <kw...@redhat.com>
Cc: Laszlo Ersek <ler...@redhat.com>
Cc: Marcel Apfelbaum <mar...@redhat.com>
Cc: Markus Armbruster <arm...@redhat.com>
Cc: Max Reitz <mre...@redhat.com>
Cc: "Michael S. Tsirkin" <m...@redhat.com>
Cc: Paolo Bonzini <pbonz...@redhat.com>
Cc: Peter Maydell <peter.mayd...@linaro.org>
Cc: Pierre Morel <pmo...@linux.vnet.ibm.com>
Cc: Prasad J Pandit <p...@fedoraproject.org>
Cc: qemu-...@nongnu.org
Cc: qemu-block@nongnu.org
Cc: qemu-...@nongnu.org
Cc: Richard Henderson <r...@twiddle.net>
Cc: Rob Herring <r...@kernel.org>
Cc: Shannon Zhao <zhaoshengl...@huawei.com>
Cc: sstabell...@kernel.org
Cc: Thomas Huth <th...@redhat.com>
Cc: Yi Min Zhao <zyi...@linux.vnet.ibm.com>
Acked-by: John Snow <js...@redhat.com>
Acked-by: Juergen Gross <jgr...@suse.com>
Acked-by: Marcel Apfelbaum <mar...@redhat.com>
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
Changes v1 -> v2:
* Rewrite commit message: don't pretend we are actually fixing
  the q35 issue. We're just fixing "info qdm" and "-device help".
  Making it easier to fix q35 is just a nice side-effect.
* Rewrite FIXME comments to make it clear that we just have
  user_creatable=true because we don't know yet if the device
  should be in the q35 whitelist
---
 hw/block/fdc.c   | 10 ++
 hw/block/pflash_cfi01.c  |  5 +
 hw/core/sysbus.c | 11 +++
 hw/i386/amd_iommu.c  |  5 +
 hw/i386/intel_iommu.c|  5 +
 hw/i386/kvm/clock.c  |  5 +
 hw/i386/kvm/ioapic

[Qemu-block] [PATCH RESEND v2 05/21] fdc: Remove user_creatable flag from sysbus-fdc & SUNW, fdtwo

[Eduardo Habkost]

sysbus-fdc and SUNW,fdtwo devices need IRQs to be wired and mmio
to be mapped, and can't be used with -device. Unset
user_creatable on their device classes.

Cc: John Snow <js...@redhat.com>
Cc: Kevin Wolf <kw...@redhat.com>
Cc: Marcel Apfelbaum <mar...@redhat.com>
Cc: Max Reitz <mre...@redhat.com>
Cc: qemu-block@nongnu.org
Cc: Thomas Huth <th...@redhat.com>
Acked-by: John Snow <js...@redhat.com>
Reviewed-by: Thomas Huth <th...@redhat.com>
Acked-by: Marcel Apfelbaum <mar...@redhat.com>
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
Changes v1 -> v2:
* Commit message rewrite only
---
 hw/block/fdc.c | 10 --
 1 file changed, 10 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 5c28a0b0ad..2e629b398b 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2880,11 +2880,6 @@ static void sysbus_fdc_class_init(ObjectClass *klass, 
void *data)
 
 dc->props = sysbus_fdc_properties;
 set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-/*
- * FIXME: Set only because we are not sure yet if this device
- * will be outside the q35 sysbus whitelist.
- */
-dc->user_creatable = true;
 }
 
 static const TypeInfo sysbus_fdc_info = {
@@ -2911,11 +2906,6 @@ static void sun4m_fdc_class_init(ObjectClass *klass, 
void *data)
 
 dc->props = sun4m_fdc_properties;
 set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-/*
- * FIXME: Set only because we are not sure yet if this device
- * will be outside the q35 sysbus whitelist.
- */
-dc->user_creatable = true;
 }
 
 static const TypeInfo sun4m_fdc_info = {
-- 
2.11.0.259.g40922b1

[Qemu-block] [PATCH v2 11/21] allwinner-ahci: Remove user_creatable flag

[Eduardo Habkost]

allwinner-ahci needs its IRQ to be connected and mmio to be
mapped (this is done by the alwinner-a10 device realize method),
and won't work with -device. Remove the user_creatable flag from
the device class.

Cc: John Snow <js...@redhat.com>
Cc: qemu-block@nongnu.org
Cc: Beniamino Galvani <b.galv...@gmail.com>
Cc: Peter Maydell <peter.mayd...@linaro.org>
Cc: qemu-...@nongnu.org
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
Changes v1 -> v2:
* Commit message rewrite only
---
 hw/ide/ahci.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 2ea1a282ef..f60826d6e0 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1815,11 +1815,6 @@ static void allwinner_ahci_class_init(ObjectClass 
*klass, void *data)
 DeviceClass *dc = DEVICE_CLASS(klass);
 
 dc->vmsd = _allwinner_ahci;
-/*
- * FIXME: Set only because we are not sure yet if this device
- * will be outside the q35 sysbus whitelist.
- */
-dc->user_creatable = true;
 }
 
 static const TypeInfo allwinner_ahci_info = {
-- 
2.11.0.259.g40922b1

[Qemu-block] [PATCH v2 06/21] pflash_cfi01: Remove user_creatable flag

[Eduardo Habkost]

TYPE_CFI_PFLASH01 devices need to be mapped by
pflash_cfi01_register() (or equivalent) and can't be used with
-device. Remove user_creatable from the device class.

Cc: Kevin Wolf <kw...@redhat.com>
Cc: Max Reitz <mre...@redhat.com>
Cc: qemu-block@nongnu.org
Cc: Laszlo Ersek <ler...@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
Changes v1 -> v2:
* Commit message rewrite only
---
 hw/block/pflash_cfi01.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index ef71956433..594d4cf6fe 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -927,11 +927,6 @@ static void pflash_cfi01_class_init(ObjectClass *klass, 
void *data)
 dc->props = pflash_cfi01_properties;
 dc->vmsd = _pflash;
 set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-/*
- * FIXME: Set only because we are not sure yet if this device
- * will be outside the q35 sysbus whitelist.
- */
-dc->user_creatable = true;
 }
 
 
-- 
2.11.0.259.g40922b1

[Qemu-block] [PATCH v2 05/21] fdc: Remove user_creatable flag from sysbus-fdc & SUNW, fdtwo

[Eduardo Habkost]

sysbus-fdc and SUNW,fdtwo devices need IRQs to be wired and mmio
to be mapped, and can't be used with -device. Unset
user_creatable on their device classes.

Cc: John Snow <js...@redhat.com>
Cc: Kevin Wolf <kw...@redhat.com>
Cc: Max Reitz <mre...@redhat.com>
Cc: qemu-block@nongnu.org
Cc: Thomas Huth <th...@redhat.com>
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
Changes v1 -> v2:
* Commit message rewrite only
---
 hw/block/fdc.c | 10 --
 1 file changed, 10 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 3d05565628..a328693d15 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2880,11 +2880,6 @@ static void sysbus_fdc_class_init(ObjectClass *klass, 
void *data)
 
 dc->props = sysbus_fdc_properties;
 set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-/*
- * FIXME: Set only because we are not sure yet if this device
- * will be outside the q35 sysbus whitelist.
- */
-dc->user_creatable = true;
 }
 
 static const TypeInfo sysbus_fdc_info = {
@@ -2911,11 +2906,6 @@ static void sun4m_fdc_class_init(ObjectClass *klass, 
void *data)
 
 dc->props = sun4m_fdc_properties;
 set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-/*
- * FIXME: Set only because we are not sure yet if this device
- * will be outside the q35 sysbus whitelist.
- */
-dc->user_creatable = true;
 }
 
 static const TypeInfo sun4m_fdc_info = {
-- 
2.11.0.259.g40922b1

Re: [Qemu-block] [RFC 03/19] sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE

[Eduardo Habkost]

On Tue, Apr 04, 2017 at 03:06:30PM +0200, Alexander Graf wrote:
> On 04/04/2017 02:59 PM, Eduardo Habkost wrote:
> > On Tue, Apr 04, 2017 at 09:02:28AM +0200, Alexander Graf wrote:
> > > 
> > > On 04.04.17 08:58, Thomas Huth wrote:
> > > > On 04.04.2017 08:53, Alexander Graf wrote:
> > > > > 
> > > > > On 03.04.17 23:00, Eduardo Habkost wrote:
> > > > > > On Mon, Apr 03, 2017 at 10:15:44PM +0200, Alexander Graf wrote:
> > > > > > > 
> > > > > > > On 03.04.17 22:10, Eduardo Habkost wrote:
> > > > > > > > On Mon, Apr 03, 2017 at 08:49:16PM +0100, Peter Maydell wrote:
> > > > > > > > > On 1 April 2017 at 01:46, Eduardo Habkost 
> > > > > > > > > <ehabk...@redhat.com> wrote:
> > > > > > > > > > commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
> > > > > > > > > > cannot_instantiate_with_device_add_yet in TYPE_SYSBUS, 
> > > > > > > > > > making
> > > > > > > > > > all kinds of untested devices available to -device and
> > > > > > > > > > device_add.
> > > > > > > > > > 
> > > > > > > > > > The problem with that is: setting has_dynamic_sysbus on a
> > > > > > > > > > machine-type lets it accept all the 288 sysbus device types 
> > > > > > > > > > we
> > > > > > > > > > have in QEMU, and most of them were never meant to be used 
> > > > > > > > > > with
> > > > > > > > > > -device. That's a lot of untested code.
> > > > > > > > > > 
> > > > > > > > > > Fortunately today we have just a few has_dynamic_sysbus=1
> > > > > > > > > > machines: virt, pc-q35-*, ppce500, and spapr.
> > > > > > > > > > 
> > > > > > > > > > virt, ppce500, and spapr have extra checks to ensure just a 
> > > > > > > > > > few
> > > > > > > > > > device types can be instantiated:
> > > > > > > > > > 
> > > > > > > > > > * virt supports only TYPE_VFIO_CALXEDA_XGMAC, 
> > > > > > > > > > TYPE_VFIO_AMD_XGBE.
> > > > > > > > > > * ppce500 supports only TYPE_ETSEC_COMMON.
> > > > > > > > > > * spapr supports only TYPE_SPAPR_PCI_HOST_BRIDGE.
> > > > > > > > > > 
> > > > > > > > > > q35 has no code to block unsupported sysbus devices, 
> > > > > > > > > > however, and
> > > > > > > > > > accepts all device types. Fortunately, only the following 20
> > > > > > > > > > device types are compiled into the qemu-system-x86_64 and
> > > > > > > > > > qemu-system-i386 binaries:
> > > > > > > > > > 
> > > > > > > > > > * allwinner-ahci
> > > > > > > > > > * amd-iommu
> > > > > > > > > > * cfi.pflash01
> > > > > > > > > > * esp
> > > > > > > > > > * fw_cfg_io
> > > > > > > > > > * fw_cfg_mem
> > > > > > > > > > * generic-sdhci
> > > > > > > > > > * hpet
> > > > > > > > > > * intel-iommu
> > > > > > > > > > * ioapic
> > > > > > > > > > * isabus-bridge
> > > > > > > > > > * kvmclock
> > > > > > > > > > * kvm-ioapic
> > > > > > > > > > * kvmvapic
> > > > > > > > > > * SUNW,fdtwo
> > > > > > > > > > * sysbus-ahci
> > > > > > > > > > * sysbus-fdc
> > > > > > > > > > * sysbus-ohci
> > > > > > > > > > * unimplemented-device
> > > > > > > > > > * virtio-mmio
> > > > > > > > > > 
> > > > > > > > > > Instead of requiring each machine-type with 
> > > > > > > > > > has_dynamic_sysbus=1
> > > > > > > > > > to implement its own mechanism 

Re: [Qemu-block] [RFC 03/19] sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE

[Eduardo Habkost]

On Mon, Apr 03, 2017 at 10:15:44PM +0200, Alexander Graf wrote:
> 
> 
> On 03.04.17 22:10, Eduardo Habkost wrote:
> > On Mon, Apr 03, 2017 at 08:49:16PM +0100, Peter Maydell wrote:
> > > On 1 April 2017 at 01:46, Eduardo Habkost <ehabk...@redhat.com> wrote:
> > > > commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
> > > > cannot_instantiate_with_device_add_yet in TYPE_SYSBUS, making
> > > > all kinds of untested devices available to -device and
> > > > device_add.
> > > > 
> > > > The problem with that is: setting has_dynamic_sysbus on a
> > > > machine-type lets it accept all the 288 sysbus device types we
> > > > have in QEMU, and most of them were never meant to be used with
> > > > -device. That's a lot of untested code.
> > > > 
> > > > Fortunately today we have just a few has_dynamic_sysbus=1
> > > > machines: virt, pc-q35-*, ppce500, and spapr.
> > > > 
> > > > virt, ppce500, and spapr have extra checks to ensure just a few
> > > > device types can be instantiated:
> > > > 
> > > > * virt supports only TYPE_VFIO_CALXEDA_XGMAC, TYPE_VFIO_AMD_XGBE.
> > > > * ppce500 supports only TYPE_ETSEC_COMMON.
> > > > * spapr supports only TYPE_SPAPR_PCI_HOST_BRIDGE.
> > > > 
> > > > q35 has no code to block unsupported sysbus devices, however, and
> > > > accepts all device types. Fortunately, only the following 20
> > > > device types are compiled into the qemu-system-x86_64 and
> > > > qemu-system-i386 binaries:
> > > > 
> > > > * allwinner-ahci
> > > > * amd-iommu
> > > > * cfi.pflash01
> > > > * esp
> > > > * fw_cfg_io
> > > > * fw_cfg_mem
> > > > * generic-sdhci
> > > > * hpet
> > > > * intel-iommu
> > > > * ioapic
> > > > * isabus-bridge
> > > > * kvmclock
> > > > * kvm-ioapic
> > > > * kvmvapic
> > > > * SUNW,fdtwo
> > > > * sysbus-ahci
> > > > * sysbus-fdc
> > > > * sysbus-ohci
> > > > * unimplemented-device
> > > > * virtio-mmio
> > > > 
> > > > Instead of requiring each machine-type with has_dynamic_sysbus=1
> > > > to implement its own mechanism to block unsupported devices, we
> > > > can use the user_creatable flag to ensure we won't let the user
> > > > plug anything that will never work.
> > > 
> > > How does this work? Which devices can be dynamically
> > > plugged is machine dependent. You can't dynamically-plug
> > > an intel-iommu on the ARM virt board, and you can't
> > > dynamically-plug the vfio-calxeda-xgmac on the spapr
> > > board, and so on. So I don't see how we can just have
> > > a flag on the device itself that controls whether
> > > it can be dynamically plugged.
> > > 
> > > So I'm definitely coming around to the opinion that
> > > it's just a bug in the q35 board that it doesn't have
> > > any device whitelisting, and we should fix that.
> > 
> > OK, let's assume q35 must implement a whitelist:
> > 
> > To build that whitelist, we need to be able to know what should
> > be in the whitelist, or not. And nobody knew for sure what was
> > user-creatable in q35 by accident, and what was really supposed
> > to be user-creatable in q35. See the "q35 and sysbus devices"
> > thread I started ~2 weeks ago.
> > 
> > Building a q35 whitelist will be much easier if make
> > sys-bus-devices non-user-creatable by default.
> 
> So why are they user creatable in the first place?
> 
> We used to have boards that were dynamic sysbus aware (ppce500, virt) that
> allowed dynamic creation and every other board did not. I don't remember the
> exact mechanism behind it though.
> 
> When did that behavior change? It sounds like a regression somewhere.

See patch description:

> > > > commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
> > > > cannot_instantiate_with_device_add_yet in TYPE_SYSBUS,

Note that the commit above is not a regression[1] (because q35
didn't have has_dynamic_sysbus=1 yet), but having sysbus devices
have cannot_instantiate_with_device_add_yet=false/user_creatable=true
by default makes it harder to build the whitelist for q35 (or
other machines that will have has_dynamic_sysbus=1 in the
future).

[1] Well, it was a regression on the "info qdm" HMP command
output, which is a minor bug. But it's still a bug we still
want to fix anyway.

-- 
Eduardo

Re: [Qemu-block] [RFC 03/19] sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE

[Eduardo Habkost]

On Mon, Apr 03, 2017 at 08:49:16PM +0100, Peter Maydell wrote:
> On 1 April 2017 at 01:46, Eduardo Habkost <ehabk...@redhat.com> wrote:
> > commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
> > cannot_instantiate_with_device_add_yet in TYPE_SYSBUS, making
> > all kinds of untested devices available to -device and
> > device_add.
> >
> > The problem with that is: setting has_dynamic_sysbus on a
> > machine-type lets it accept all the 288 sysbus device types we
> > have in QEMU, and most of them were never meant to be used with
> > -device. That's a lot of untested code.
> >
> > Fortunately today we have just a few has_dynamic_sysbus=1
> > machines: virt, pc-q35-*, ppce500, and spapr.
> >
> > virt, ppce500, and spapr have extra checks to ensure just a few
> > device types can be instantiated:
> >
> > * virt supports only TYPE_VFIO_CALXEDA_XGMAC, TYPE_VFIO_AMD_XGBE.
> > * ppce500 supports only TYPE_ETSEC_COMMON.
> > * spapr supports only TYPE_SPAPR_PCI_HOST_BRIDGE.
> >
> > q35 has no code to block unsupported sysbus devices, however, and
> > accepts all device types. Fortunately, only the following 20
> > device types are compiled into the qemu-system-x86_64 and
> > qemu-system-i386 binaries:
> >
> > * allwinner-ahci
> > * amd-iommu
> > * cfi.pflash01
> > * esp
> > * fw_cfg_io
> > * fw_cfg_mem
> > * generic-sdhci
> > * hpet
> > * intel-iommu
> > * ioapic
> > * isabus-bridge
> > * kvmclock
> > * kvm-ioapic
> > * kvmvapic
> > * SUNW,fdtwo
> > * sysbus-ahci
> > * sysbus-fdc
> > * sysbus-ohci
> > * unimplemented-device
> > * virtio-mmio
> >
> > Instead of requiring each machine-type with has_dynamic_sysbus=1
> > to implement its own mechanism to block unsupported devices, we
> > can use the user_creatable flag to ensure we won't let the user
> > plug anything that will never work.
> 
> How does this work? Which devices can be dynamically
> plugged is machine dependent. You can't dynamically-plug
> an intel-iommu on the ARM virt board, and you can't
> dynamically-plug the vfio-calxeda-xgmac on the spapr
> board, and so on. So I don't see how we can just have
> a flag on the device itself that controls whether
> it can be dynamically plugged.
> 
> So I'm definitely coming around to the opinion that
> it's just a bug in the q35 board that it doesn't have
> any device whitelisting, and we should fix that.

OK, let's assume q35 must implement a whitelist:

To build that whitelist, we need to be able to know what should
be in the whitelist, or not. And nobody knew for sure what was
user-creatable in q35 by accident, and what was really supposed
to be user-creatable in q35. See the "q35 and sysbus devices"
thread I started ~2 weeks ago.

Building a q35 whitelist will be much easier if make
sys-bus-devices non-user-creatable by default.

-- 
Eduardo

[Qemu-block] [RFC 10/19] sysbus-ahci: Remove user_creatable flag

[Eduardo Habkost]

The sysbus-ahci devices are supposed to be created and wired by
code from other devices, like calxeda_init() and
xlnx_zynqmp_realize(), and won't work with -device. Remove the
user_creatable flag from the device class.

Cc: John Snow <js...@redhat.com>
Cc: qemu-block@nongnu.org
Cc: Rob Herring <r...@kernel.org>
Cc: Peter Maydell <peter.mayd...@linaro.org>
Cc: Alistair Francis <alistair.fran...@xilinx.com>
Cc: "Edgar E. Iglesias" <edgar.igles...@gmail.com>
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 hw/ide/ahci.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index b1b7780d56..68f2ce09ee 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1721,11 +1721,6 @@ static void sysbus_ahci_class_init(ObjectClass *klass, 
void *data)
 dc->props = sysbus_ahci_properties;
 dc->reset = sysbus_ahci_reset;
 set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-/*
- * FIXME: Set only for compatibility on q35 machine-type.
- * Probably never meant to be user-creatable
- */
-dc->user_creatable = true;
 }
 
 static const TypeInfo sysbus_ahci_info = {
-- 
2.11.0.259.g40922b1

[Qemu-block] [RFC 11/19] allwinner-ahci: Remove user_creatable flag

[Eduardo Habkost]

allwinner-ahci needs to be created and wired by the alwinner-a10
device, and won't work with -device. Remove the user_creatable
flag from the device class.

Cc: John Snow <js...@redhat.com>
Cc: qemu-block@nongnu.org
Cc: Beniamino Galvani <b.galv...@gmail.com>
Cc: Peter Maydell <peter.mayd...@linaro.org>
Cc: qemu-...@nongnu.org
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 hw/ide/ahci.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 68f2ce09ee..f60826d6e0 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1815,11 +1815,6 @@ static void allwinner_ahci_class_init(ObjectClass 
*klass, void *data)
 DeviceClass *dc = DEVICE_CLASS(klass);
 
 dc->vmsd = _allwinner_ahci;
-/*
- * FIXME: Set only for compatibility on q35 machine-type.
- * Probably never meant to be user-creatable
- */
-dc->user_creatable = true;
 }
 
 static const TypeInfo allwinner_ahci_info = {
-- 
2.11.0.259.g40922b1

[Qemu-block] [RFC 03/19] sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE

[Eduardo Habkost]

commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
cannot_instantiate_with_device_add_yet in TYPE_SYSBUS, making
all kinds of untested devices available to -device and
device_add.

The problem with that is: setting has_dynamic_sysbus on a
machine-type lets it accept all the 288 sysbus device types we
have in QEMU, and most of them were never meant to be used with
-device. That's a lot of untested code.

Fortunately today we have just a few has_dynamic_sysbus=1
machines: virt, pc-q35-*, ppce500, and spapr.

virt, ppce500, and spapr have extra checks to ensure just a few
device types can be instantiated:

* virt supports only TYPE_VFIO_CALXEDA_XGMAC, TYPE_VFIO_AMD_XGBE.
* ppce500 supports only TYPE_ETSEC_COMMON.
* spapr supports only TYPE_SPAPR_PCI_HOST_BRIDGE.

q35 has no code to block unsupported sysbus devices, however, and
accepts all device types. Fortunately, only the following 20
device types are compiled into the qemu-system-x86_64 and
qemu-system-i386 binaries:

* allwinner-ahci
* amd-iommu
* cfi.pflash01
* esp
* fw_cfg_io
* fw_cfg_mem
* generic-sdhci
* hpet
* intel-iommu
* ioapic
* isabus-bridge
* kvmclock
* kvm-ioapic
* kvmvapic
* SUNW,fdtwo
* sysbus-ahci
* sysbus-fdc
* sysbus-ohci
* unimplemented-device
* virtio-mmio

Instead of requiring each machine-type with has_dynamic_sysbus=1
to implement its own mechanism to block unsupported devices, we
can use the user_creatable flag to ensure we won't let the user
plug anything that will never work.

This patch adds user_creatable=true explicitly to the
24 device types mentioned above, to keep compatibility, and set
user_creatable=false on TYPE_SYS_BUS_DEVICE.

Subsequent patches will remove user_creatable=true from the
devices that were not meant to be creatable using -device
despite being currently accepted by q35.

Cc: Peter Maydell <peter.mayd...@linaro.org>
Cc: Alexander Graf <ag...@suse.de>
Cc: John Snow <js...@redhat.com>
Cc: Kevin Wolf <kw...@redhat.com>
Cc: Max Reitz <mre...@redhat.com>
Cc: Paolo Bonzini <pbonz...@redhat.com>
Cc: Richard Henderson <r...@twiddle.net>
Cc: Eduardo Habkost <ehabk...@redhat.com>
Cc: "Michael S. Tsirkin" <m...@redhat.com>
Cc: Scott Wood <scottw...@freescale.com>
Cc: Jason Wang <jasow...@redhat.com>
Cc: David Gibson <da...@gibson.dropbear.id.au>
Cc: Gerd Hoffmann <kra...@redhat.com>
Cc: Alex Williamson <alex.william...@redhat.com>
Cc: qemu-block@nongnu.org
Cc: qemu-...@nongnu.org
Cc: Alistair Francis <alistair.fran...@xilinx.com>
Cc: Beniamino Galvani <b.galv...@gmail.com>
Cc: "Edgar E. Iglesias" <edgar.igles...@gmail.com>
Cc: Gabriel L. Somlo <so...@cmu.edu>
Cc: Igor Mammedov <imamm...@redhat.com>
Cc: Laszlo Ersek <ler...@redhat.com>
Cc: Marcel Apfelbaum <mar...@redhat.com>
Cc: Prasad J Pandit <p...@fedoraproject.org>
Cc: qemu-...@nongnu.org
Cc: Shannon Zhao <zhaoshengl...@huawei.com>
Cc: Thomas Huth <th...@redhat.com>
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 hw/block/fdc.c   | 10 ++
 hw/block/pflash_cfi01.c  |  5 +
 hw/core/sysbus.c | 11 +++
 hw/i386/amd_iommu.c  |  5 +
 hw/i386/intel_iommu.c|  5 +
 hw/i386/kvm/clock.c  |  5 +
 hw/i386/kvm/ioapic.c |  5 +
 hw/i386/kvmvapic.c   |  5 +
 hw/ide/ahci.c| 10 ++
 hw/intc/ioapic.c |  5 +
 hw/isa/isa-bus.c |  5 +
 hw/misc/unimp.c  |  5 +
 hw/net/fsl_etsec/etsec.c |  1 +
 hw/nvram/fw_cfg.c| 10 ++
 hw/ppc/spapr_pci.c   |  1 +
 hw/scsi/esp.c|  5 +
 hw/sd/sdhci.c|  5 +
 hw/timer/hpet.c  |  5 +
 hw/usb/hcd-ohci.c|  5 +
 hw/vfio/amd-xgbe.c   |  1 +
 hw/vfio/calxeda-xgmac.c  |  1 +
 hw/virtio/virtio-mmio.c  |  5 +
 22 files changed, 115 insertions(+)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index a328693d15..a06c8e358c 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2880,6 +2880,11 @@ static void sysbus_fdc_class_init(ObjectClass *klass, 
void *data)
 
 dc->props = sysbus_fdc_properties;
 set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+/*
+ * FIXME: Set only for compatibility on q35 machine-type.
+ * Probably never meant to be user-creatable
+ */
+dc->user_creatable = true;
 }
 
 static const TypeInfo sysbus_fdc_info = {
@@ -2906,6 +2911,11 @@ static void sun4m_fdc_class_init(ObjectClass *klass, 
void *data)
 
 dc->props = sun4m_fdc_properties;
 set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+/*
+ * FIXME: Set only for compatibility on q35 machine-type.
+ * Probably never meant to be user-creatable
+ */
+dc->user_creatable = true;
 }
 
 static const TypeInfo sun4m_fdc_info = {
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 594d4cf6fe..f48dc20035 100644
--- a/hw/block/pf

[Qemu-block] [RFC 05/19] pflash_cfi01: Remove user_creatable flag

[Eduardo Habkost]

TYPE_CFI_PFLASH01 devices need to be mapped by
pflash_cfi01_register() and can't be used with -device. Remove
user_creatable from the device class.

Cc: Kevin Wolf <kw...@redhat.com>
Cc: Max Reitz <mre...@redhat.com>
Cc: qemu-block@nongnu.org
Cc: Laszlo Ersek <ler...@redhat.com>
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 hw/block/pflash_cfi01.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index f48dc20035..594d4cf6fe 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -927,11 +927,6 @@ static void pflash_cfi01_class_init(ObjectClass *klass, 
void *data)
 dc->props = pflash_cfi01_properties;
 dc->vmsd = _pflash;
 set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-/*
- * FIXME: Set only for compatibility on q35 machine-type.
- * Probably never meant to be user-creatable
- */
-dc->user_creatable = true;
 }
 
 
-- 
2.11.0.259.g40922b1

[Qemu-block] [RFC 04/19] fdc: Remove user_creatable flag from sysbus-fdc & SUNW, fdtwo

[Eduardo Habkost]

sysbus-fdc and SUNW,fdtwo devices need to be wired by the fdc
initialization code, and can't be used with -device. Unset
user_creatable on their device classes.

Cc: John Snow <js...@redhat.com>
Cc: Kevin Wolf <kw...@redhat.com>
Cc: Max Reitz <mre...@redhat.com>
Cc: qemu-block@nongnu.org
Cc: Thomas Huth <th...@redhat.com>
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 hw/block/fdc.c | 10 --
 1 file changed, 10 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index a06c8e358c..a328693d15 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2880,11 +2880,6 @@ static void sysbus_fdc_class_init(ObjectClass *klass, 
void *data)
 
 dc->props = sysbus_fdc_properties;
 set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-/*
- * FIXME: Set only for compatibility on q35 machine-type.
- * Probably never meant to be user-creatable
- */
-dc->user_creatable = true;
 }
 
 static const TypeInfo sysbus_fdc_info = {
@@ -2911,11 +2906,6 @@ static void sun4m_fdc_class_init(ObjectClass *klass, 
void *data)
 
 dc->props = sun4m_fdc_properties;
 set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-/*
- * FIXME: Set only for compatibility on q35 machine-type.
- * Probably never meant to be user-creatable
- */
-dc->user_creatable = true;
 }
 
 static const TypeInfo sun4m_fdc_info = {
-- 
2.11.0.259.g40922b1

Re: [Qemu-block] [Qemu-devel] [PATCH] block/parallels.c: avoid integer overflow in allocate_clusters()

[Eduardo Habkost]

On Fri, Mar 31, 2017 at 05:18:39PM +0100, Stefan Hajnoczi wrote:
> On Fri, Mar 31, 2017 at 10:40:33AM -0300, Eduardo Habkost wrote:
> > On Fri, Mar 31, 2017 at 10:27:44AM -0300, Philippe Mathieu-Daudé wrote:
> > > Hi,
> > > 
> > > Eduardo you seem skilled regarding Coccinelle scripts, is it possible to
> > > write one to find those overflows?
> > 
> > Probably not. AFAIK, Coccinelle rules are based on local code
> > syntax only. This means it doesn't know the data type of
> > expressions like (s->tracks).
> 
> I'm surprised by that statement.  Coccinelle isn't a text matcher, it's
> a proper C compiler frontend that parses the all code in the compilation
> unit.  Therefore it must have the type information even for s->tracks.

You are probably not wrong about it not being just a text
matcher. But I'm not sure about it being able to have type
information for s->tracks. The documentation isn't clear about
that.

The 'idexpression' declarations seems to accept some kind of C
type annotations (I didn't know that!), but the documentation
also says: "A more complex description of a location, such as
a->b is considered to be an expression, not an idexpression".
And 'expression' metavariables don't seem to support type
annotations.

My impression is that Coccinelle has limited support to
understand simple variable declarations, but not the full set of
C type declarations and type system rules that would allow it to
figure out the type of an expression like s->tracks.

But I really hope to be wrong, because that would be very useful. :)

> Disclaimer: This should in no way be considered a volunteer offer to
> write cocci scripts now or at any time in the future :).  I'm not fluent
> in the semantic patch syntax.

I don't believe there's anybody in the world fluent in the SmPL
syntax. Maybe its authors are, but I wouldn't be so sure. :)

-- 
Eduardo

Re: [Qemu-block] [Qemu-devel] [PATCH] block/parallels.c: avoid integer overflow in allocate_clusters()

[Eduardo Habkost]

On Fri, Mar 31, 2017 at 10:27:44AM -0300, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> Eduardo you seem skilled regarding Coccinelle scripts, is it possible to
> write one to find those overflows?

Probably not. AFAIK, Coccinelle rules are based on local code
syntax only. This means it doesn't know the data type of
expressions like (s->tracks).

> 
> Peter having one more macro might help or confuses more?
> 
> #define MULTIPLY64(a32, b32) ((int64_t)a32 * b32)
> 
> On 03/31/2017 10:13 AM, Peter Maydell wrote:
> > Coverity (CID 1307776) points out that in the multiply:
> >   space = to_allocate * s->tracks;
> > we are trying to calculate a 64 bit result but the types
> > of to_allocate and s->tracks mean that we actually calculate
> > a 32 bit result. Add an explicit cast to force a 64 bit
> > multiply.
> > 
> > Signed-off-by: Peter Maydell 
> 
> Reviewed-by: Philippe Mathieu-Daudé 
> 
> > ---
> > NB: compile-and-make-check tested only...
> > ---
> >  block/parallels.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/block/parallels.c b/block/parallels.c
> > index 4173b3f..3886c30 100644
> > --- a/block/parallels.c
> > +++ b/block/parallels.c
> > @@ -206,7 +206,7 @@ static int64_t allocate_clusters(BlockDriverState *bs, 
> > int64_t sector_num,
> >  }
> > 
> >  to_allocate = DIV_ROUND_UP(sector_num + *pnum, s->tracks) - idx;
> > -space = to_allocate * s->tracks;
> > +space = (int64_t)to_allocate * s->tracks;
> >  if (s->data_end + space > bdrv_getlength(bs->file->bs) >> 
> > BDRV_SECTOR_BITS) {
> >  int ret;
> >  space += s->prealloc_size;
> > 

-- 
Eduardo

Re: [Qemu-block] [Qemu-devel] [PATCH v3 0/3] coccinelle: Clean up error checks and return value variables

[Eduardo Habkost]

On Tue, Jun 14, 2016 at 11:03:20AM +0200, Markus Armbruster wrote:
[...]
> > * Manual fixups
> 
> With the commit message of 3/3 amended, series
> Reviewed-by: Markus Armbruster 
> 
> My other suggested touch ups are optional.  If you don't object, I'll do
> them, and take the result through my error-next branch.

I'm OK with the changes you suggested. Thanks!

-- 
Eduardo

[Qemu-block] [PATCH v3 0/3] coccinelle: Clean up error checks and return value variables

[Eduardo Habkost]

  pixman_image_get_height(image), NULL,
  +return pixman_image_create_bits(format,
  +pixman_image_get_width(image),
  +pixman_image_get_height(image),
  +NULL,
   pixman_image_get_stride(image));
   }

Eduardo Habkost (3):
  error: Remove NULL checks on error_propagate() calls
  error: Remove unnecessary local_err variables
  coccinelle: Remove unnecessary variables for function return value

 scripts/coccinelle/error_propagate_null.cocci | 10 +++
 scripts/coccinelle/remove_local_err.cocci | 29 ++
 scripts/coccinelle/return_directly.cocci  | 21 ++
 audio/audio.c | 10 ++-
 block.c   | 20 -
 block/archipelago.c   |  4 +--
 block/qcow2-cluster.c |  7 ++---
 block/qcow2-refcount.c|  7 ++---
 block/qcow2.c |  4 +--
 block/quorum.c|  4 +--
 block/raw-posix.c | 24 +++
 block/raw_bsd.c   |  9 +-
 block/rbd.c   |  4 +--
 block/snapshot.c  |  4 +--
 block/vmdk.c  |  6 ++--
 block/vvfat.c |  5 +---
 blockdev.c| 12 ++--
 bootdevice.c  |  4 +--
 dump.c|  4 +--
 hw/acpi/aml-build.c   | 13 ++---
 hw/audio/intel-hda.c  |  5 +---
 hw/display/vga.c  |  4 +--
 hw/ide/qdev.c |  4 +--
 hw/intc/s390_flic_kvm.c   |  5 ++--
 hw/net/ne2000-isa.c   |  4 +--
 hw/pci-host/uninorth.c|  5 +---
 hw/ppc/spapr_vio.c|  5 +---
 hw/s390x/s390-virtio-ccw.c|  5 +---
 hw/s390x/virtio-ccw.c | 42 +--
 hw/scsi/megasas.c |  5 +---
 hw/scsi/scsi-generic.c|  5 +---
 hw/timer/mc146818rtc.c|  4 +--
 hw/usb/dev-storage.c  |  4 +--
 hw/virtio/virtio-pci.c|  4 +--
 linux-user/signal.c   | 15 +++---
 page_cache.c  |  5 +---
 qga/commands-posix.c  |  4 +--
 qga/commands-win32.c  | 13 ++---
 qobject/qlist.c   |  5 +---
 qom/object.c  |  4 +--
 target-i386/cpu.c |  4 +--
 target-i386/fpu_helper.c  | 10 ++-
 target-i386/kvm.c |  5 ++--
 target-mips/op_helper.c   |  4 +--
 target-s390x/helper.c |  6 +---
 target-sparc/cc_helper.c  | 25 
 target-tricore/op_helper.c| 13 +++--
 tests/display-vga-test.c  |  6 +---
 tests/endianness-test.c   |  5 +---
 tests/i440fx-test.c   |  4 +--
 tests/intel-hda-test.c|  6 +---
 tests/test-filter-redirector.c|  6 +---
 tests/virtio-blk-test.c   |  5 +---
 tests/virtio-console-test.c   |  6 +---
 tests/virtio-net-test.c   |  6 +---
 tests/virtio-scsi-test.c  |  6 +---
 tests/wdt_ib700-test.c|  6 +---
 ui/cursor.c   | 10 ++-
 ui/qemu-pixman.c  | 13 -
 util/module.c |  6 +---
 vl.c  |  5 +---
 61 files changed, 159 insertions(+), 346 deletions(-)
 create mode 100644 scripts/coccinelle/error_propagate_null.cocci
 create mode 100644 scripts/coccinelle/remove_local_err.cocci
 create mode 100644 scripts/coccinelle/return_directly.cocci

-- 
2.5.5

Re: [Qemu-block] [Qemu-devel] [RFC v2 3/3] Remove unnecessary variables for function return value

[Eduardo Habkost]

On Mon, Jun 13, 2016 at 01:29:47PM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabk...@redhat.com> writes:
> 
> > Use Coccinelle script to replace 'ret = E; return ret' with
> > 'return E'. The script will do the substitution only when the
> > function return type and variable type are the same.
> >
> > Sending as RFC because the patch looks more intrusive than the
> > others. Probably better to split it per subsystem and let each
> > maintainer review and apply it?
> 
> As far as I'm concerned, obvious mechanical cleanups like this one can
> go in as a single tree-wide patch.  I'd consider making you split it up,
> then chase maintainers a waste of your time[*].

Not wasting my time sounds like a good idea. :)

Once the issues below are fixed and Eric's comments are
addressed, should it go through your error reporting tree?

> 
> checkpatch.pl is unhappy:
> 
> 529: WARNING: line over 80 characters
> 563: WARNING: line over 80 characters
> 695: WARNING: line over 80 characters
> 811: ERROR: return is not a function, parentheses are not required
> 830: ERROR: return is not a function, parentheses are not required
> 849: ERROR: return is not a function, parentheses are not required
> 
> 
> [*] Been there, done that, but if it is what it takes...

-- 
Eduardo

Re: [Qemu-block] [Qemu-devel] [PATCH v2 2/3] error: Remove unnecessary local_err variables

[Eduardo Habkost]

On Mon, Jun 13, 2016 at 08:49:37PM +0200, Markus Armbruster wrote:
> Eric Blake  writes:
[...]
> >> 
> >> See, e.g.:
> >> 
> >> void qmp_guest_suspend_disk(Error **errp)
> >> {
> >> Error *local_err = NULL;
> >> GuestSuspendMode *mode = g_new(GuestSuspendMode, 1);
> >> 
> >> *mode = GUEST_SUSPEND_MODE_DISK;
> >> check_suspend_mode(*mode, _err);
> >> acquire_privilege(SE_SHUTDOWN_NAME, _err);
> >> execute_async(do_suspend, mode, _err);
> >
> > That usage is a bug.  A Coccinelle script to root out such buggy
> > instances would be nice.
> 
> We've discussed this before.  See for instance commit 297a364:
> 
> qapi: Replace uncommon use of the error API by the common one

That explains why I was confused: I remember seeing that QAPI
visitors could be called with *errp set.

I will try to change the script as suggested, to only apply the
changes if errp is never touched before the error_setg() call.

-- 
Eduardo

Re: [Qemu-block] [Qemu-devel] [PATCH v2 2/3] error: Remove unnecessary local_err variables

[Eduardo Habkost]

On Mon, Jun 13, 2016 at 10:01:16AM -0600, Eric Blake wrote:
> On 06/13/2016 09:52 AM, Eduardo Habkost wrote:
[...]
> > 
> > See, e.g.:
> > 
> > void qmp_guest_suspend_disk(Error **errp)
> > {
> > Error *local_err = NULL;
> > GuestSuspendMode *mode = g_new(GuestSuspendMode, 1);
> > 
> > *mode = GUEST_SUSPEND_MODE_DISK;
> > check_suspend_mode(*mode, _err);
> > acquire_privilege(SE_SHUTDOWN_NAME, _err);
> > execute_async(do_suspend, mode, _err);
> 
> That usage is a bug.  A Coccinelle script to root out such buggy
> instances would be nice.

I've tried to write one, but got lots of false positives due to
error-checking using the function return value, not local_err.
For reference, this is the script I have used:

@@
typedef Error;
identifier F1 !~ "hmp_handle_error|error_free_or_abort";
identifier F2 !~ "hmp_handle_error|error_free_or_abort";
idexpression Error *ERR;
@@
-F1(..., )
+FIXME_incorrect_error_usage1()
... when != ERR
-F2(..., )
+FIXME_incorrect_error_usage2()

@@
identifier F1 !~ "hmp_handle_error|error_free_or_abort";
identifier F2 !~ "hmp_handle_error|error_free_or_abort";
idexpression Error **ERRP;
@@
-F1(..., ERRP)
+FIXME_incorrect_error_usage1()
 ... when != ERRP
-F2(..., ERRP)
+FIXME_incorrect_error_usage2()

-- 
Eduardo

Re: [Qemu-block] [Qemu-devel] [PATCH v2 2/3] error: Remove unnecessary local_err variables

[Eduardo Habkost]

On Mon, Jun 13, 2016 at 01:42:15PM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabk...@redhat.com> writes:
> 
> > This patch simplifies code that uses a local_err variable just to
> > immediately use it for an error_propagate() call.
> >
> > Coccinelle patch used to perform the changes added to
> > scripts/coccinelle/remove_local_err.cocci.
> >
> > Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
> [...]
> > diff --git a/scripts/coccinelle/remove_local_err.cocci 
> > b/scripts/coccinelle/remove_local_err.cocci
> > new file mode 100644
> > index 000..5f0b6c0
> > --- /dev/null
> > +++ b/scripts/coccinelle/remove_local_err.cocci
> > @@ -0,0 +1,27 @@
> > +// Replace unnecessary usage of local_err variable with
> > +// direct usage of errp argument
> > +
> > +@@
> > +expression list ARGS;
> > +expression F2;
> > +identifier LOCAL_ERR;
> > +expression ERRP;
> > +idexpression V;
> > +typedef Error;
> > +expression I;
> 
> I isn't used.
> 
> > +@@
> > + {
> > + ...
> > +-Error *LOCAL_ERR;
> > + ... when != LOCAL_ERR
> > +(
> > +-F2(ARGS, _ERR);
> > +-error_propagate(ERRP, LOCAL_ERR);
> > ++F2(ARGS, ERRP);
> > +|
> > +-V = F2(ARGS, _ERR);
> > +-error_propagate(ERRP, LOCAL_ERR);
> > ++V = F2(ARGS, ERRP);
> > +)
> > + ... when != LOCAL_ERR
> > + }
> 
> There is an (ugly) difference between
> 
> error_setg(_err, ...);
> error_propagate(errp, _err);
> 
> and
> 
> error_setg(errp, ...);
> 
> The latter aborts when @errp already contains an error, the former does
> nothing.

Why the difference? Couldn't we change that so that both are equivalent?

> 
> Your transformation has the error_setg() or similar hidden in F2().  It
> can add aborts.
> 
> I think it can be salvaged: we know that @errp must not contain an error
> on function entry.  If @errp doesn't occur elsewhere in this function,
> it cannot pick up an error on the way to the transformed spot.  Can you
> add that to your when constraints?

Do we really know that *errp is NULL on entry? Aren't we allowed to call
functions with a non-NULL *errp?

See, e.g.:

void qmp_guest_suspend_disk(Error **errp)
{
Error *local_err = NULL;
GuestSuspendMode *mode = g_new(GuestSuspendMode, 1);

*mode = GUEST_SUSPEND_MODE_DISK;
check_suspend_mode(*mode, _err);
acquire_privilege(SE_SHUTDOWN_NAME, _err);
execute_async(do_suspend, mode, _err);

if (local_err) {
error_propagate(errp, local_err);
g_free(mode);
}
}


-- 
Eduardo

Re: [Qemu-block] [PATCH v2 2/3] error: Remove unnecessary local_err variables

[Eduardo Habkost]

On Fri, Jun 10, 2016 at 02:59:55PM -0600, Eric Blake wrote:
> On 06/10/2016 02:12 PM, Eduardo Habkost wrote:
> > This patch simplifies code that uses a local_err variable just to
> > immediately use it for an error_propagate() call.
> > 
> > Coccinelle patch used to perform the changes added to
> > scripts/coccinelle/remove_local_err.cocci.
> > 
> > Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
> > ---
> >  block.c   |  8 ++--
> >  block/raw-posix.c |  8 ++--
> >  block/raw_bsd.c   |  4 +---
> >  blockdev.c| 16 +---
> >  hw/s390x/s390-virtio-ccw.c|  5 +
> >  hw/s390x/virtio-ccw.c | 28 +++-
> >  scripts/coccinelle/remove_local_err.cocci | 27 +++
> >  target-i386/cpu.c |  4 +---
> >  8 files changed, 46 insertions(+), 54 deletions(-)
> >  create mode 100644 scripts/coccinelle/remove_local_err.cocci
> > 
> 
> > +++ b/block.c
> > @@ -294,14 +294,12 @@ typedef struct CreateCo {
> >  
> >  static void coroutine_fn bdrv_create_co_entry(void *opaque)
> >  {
> > -Error *local_err = NULL;
> >  int ret;
> >  
> >  CreateCo *cco = opaque;
> >  assert(cco->drv);
> >  
> > -ret = cco->drv->bdrv_create(cco->filename, cco->opts, _err);
> > -error_propagate(>err, local_err);
> > +ret = cco->drv->bdrv_create(cco->filename, cco->opts, >err);
> >  cco->ret = ret;
> 
> This hunk doesn't get simplified by 3/3; you may want to consider a
> manual followup to drop 'int ret' and just assign
> cco->drv->bdrv_create() directly to cco->ret.  But doesn't change this
> patch.

This could become yet another Coccinelle script, but we need to
be careful about type conversions, and tell it to do it only if
the types of 'ret', 'cc->drv->bdrv_create()' and 'cco->ret' are
the same.

-- 
Eduardo

[Qemu-block] [PATCH v2 2/3] error: Remove unnecessary local_err variables

[Eduardo Habkost]

This patch simplifies code that uses a local_err variable just to
immediately use it for an error_propagate() call.

Coccinelle patch used to perform the changes added to
scripts/coccinelle/remove_local_err.cocci.

Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 block.c   |  8 ++--
 block/raw-posix.c |  8 ++--
 block/raw_bsd.c   |  4 +---
 blockdev.c| 16 +---
 hw/s390x/s390-virtio-ccw.c|  5 +
 hw/s390x/virtio-ccw.c | 28 +++-
 scripts/coccinelle/remove_local_err.cocci | 27 +++
 target-i386/cpu.c |  4 +---
 8 files changed, 46 insertions(+), 54 deletions(-)
 create mode 100644 scripts/coccinelle/remove_local_err.cocci

diff --git a/block.c b/block.c
index ecca55a..d516ab6 100644
--- a/block.c
+++ b/block.c
@@ -294,14 +294,12 @@ typedef struct CreateCo {
 
 static void coroutine_fn bdrv_create_co_entry(void *opaque)
 {
-Error *local_err = NULL;
 int ret;
 
 CreateCo *cco = opaque;
 assert(cco->drv);
 
-ret = cco->drv->bdrv_create(cco->filename, cco->opts, _err);
-error_propagate(>err, local_err);
+ret = cco->drv->bdrv_create(cco->filename, cco->opts, >err);
 cco->ret = ret;
 }
 
@@ -353,7 +351,6 @@ out:
 int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
 {
 BlockDriver *drv;
-Error *local_err = NULL;
 int ret;
 
 drv = bdrv_find_protocol(filename, true, errp);
@@ -361,8 +358,7 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, 
Error **errp)
 return -ENOENT;
 }
 
-ret = bdrv_create(drv, filename, opts, _err);
-error_propagate(errp, local_err);
+ret = bdrv_create(drv, filename, opts, errp);
 return ret;
 }
 
diff --git a/block/raw-posix.c b/block/raw-posix.c
index cb663d8..d7397bf 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -582,12 +582,10 @@ static int raw_open(BlockDriverState *bs, QDict *options, 
int flags,
 Error **errp)
 {
 BDRVRawState *s = bs->opaque;
-Error *local_err = NULL;
 int ret;
 
 s->type = FTYPE_FILE;
-ret = raw_open_common(bs, options, flags, 0, _err);
-error_propagate(errp, local_err);
+ret = raw_open_common(bs, options, flags, 0, errp);
 return ret;
 }
 
@@ -2442,14 +2440,12 @@ static int cdrom_open(BlockDriverState *bs, QDict 
*options, int flags,
   Error **errp)
 {
 BDRVRawState *s = bs->opaque;
-Error *local_err = NULL;
 int ret;
 
 s->type = FTYPE_CD;
 
 /* open will not fail even if no CD is inserted, so add O_NONBLOCK */
-ret = raw_open_common(bs, options, flags, O_NONBLOCK, _err);
-error_propagate(errp, local_err);
+ret = raw_open_common(bs, options, flags, O_NONBLOCK, errp);
 return ret;
 }
 
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 5af11b6..b51ac98 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -190,11 +190,9 @@ static int raw_has_zero_init(BlockDriverState *bs)
 
 static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
 {
-Error *local_err = NULL;
 int ret;
 
-ret = bdrv_create_file(filename, opts, _err);
-error_propagate(errp, local_err);
+ret = bdrv_create_file(filename, opts, errp);
 return ret;
 }
 
diff --git a/blockdev.c b/blockdev.c
index 028dba3..3b6d242 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3654,7 +3654,6 @@ void qmp_blockdev_mirror(const char *device, const char 
*target,
 BlockBackend *blk;
 BlockDriverState *target_bs;
 AioContext *aio_context;
-Error *local_err = NULL;
 
 blk = blk_by_name(device);
 if (!blk) {
@@ -3678,16 +3677,11 @@ void qmp_blockdev_mirror(const char *device, const char 
*target,
 
 bdrv_set_aio_context(target_bs, aio_context);
 
-blockdev_mirror_common(bs, target_bs,
-   has_replaces, replaces, sync,
-   has_speed, speed,
-   has_granularity, granularity,
-   has_buf_size, buf_size,
-   has_on_source_error, on_source_error,
-   has_on_target_error, on_target_error,
-   true, true,
-   _err);
-error_propagate(errp, local_err);
+blockdev_mirror_common(bs, target_bs, has_replaces, replaces, sync,
+   has_speed, speed, has_granularity, granularity,
+   has_buf_size, buf_size, has_on_source_error,
+   on_source_error, has_on_target_error,
+   on_target_error, true, true, errp);
 
 aio_context_release(aio_context);
 }
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 95ff5e3..b7112d0 100644
--- a/hw/s3

[Qemu-block] [PATCH v2 1/3] error: Remove NULL checks on error_propagate() calls

[Eduardo Habkost]

error_propagate() already ignores local_err==NULL, so there's no
need to check it before calling.

Coccinelle patch used to perform the changes added to
scripts/coccinelle/error_propagate_null.cocci.

Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 block.c   | 20 +--
 block/qcow2.c |  4 +---
 block/quorum.c|  4 +---
 block/raw-posix.c | 16 ---
 block/raw_bsd.c   |  4 +---
 block/snapshot.c  |  4 +---
 blockdev.c| 12 +++-
 bootdevice.c  |  4 +---
 dump.c|  4 +---
 hw/ide/qdev.c |  4 +---
 hw/net/ne2000-isa.c   |  4 +---
 hw/s390x/virtio-ccw.c | 28 +++
 hw/usb/dev-storage.c  |  4 +---
 qga/commands-win32.c  |  8 ++--
 qom/object.c  |  4 +---
 scripts/coccinelle/error_propagate_null.cocci | 10 ++
 16 files changed, 41 insertions(+), 93 deletions(-)
 create mode 100644 scripts/coccinelle/error_propagate_null.cocci

diff --git a/block.c b/block.c
index f54bc25..ecca55a 100644
--- a/block.c
+++ b/block.c
@@ -301,9 +301,7 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque)
 assert(cco->drv);
 
 ret = cco->drv->bdrv_create(cco->filename, cco->opts, _err);
-if (local_err) {
-error_propagate(>err, local_err);
-}
+error_propagate(>err, local_err);
 cco->ret = ret;
 }
 
@@ -364,9 +362,7 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, 
Error **errp)
 }
 
 ret = bdrv_create(drv, filename, opts, _err);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 return ret;
 }
 
@@ -1760,18 +1756,14 @@ fail:
 QDECREF(options);
 bs->options = NULL;
 bdrv_unref(bs);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 return NULL;
 
 close_and_fail:
 bdrv_unref(bs);
 QDECREF(snapshot_options);
 QDECREF(options);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 return NULL;
 }
 
@@ -3591,9 +3583,7 @@ void bdrv_img_create(const char *filename, const char 
*fmt,
 out:
 qemu_opts_del(opts);
 qemu_opts_free(create_opts);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 }
 
 AioContext *bdrv_get_aio_context(BlockDriverState *bs)
diff --git a/block/qcow2.c b/block/qcow2.c
index 6f5fb81..4504846 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2394,9 +2394,7 @@ static int qcow2_create(const char *filename, QemuOpts 
*opts, Error **errp)
 ret = qcow2_create2(filename, size, backing_file, backing_fmt, flags,
 cluster_size, prealloc, opts, version, refcount_order,
 _err);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 
 finish:
 g_free(backing_file);
diff --git a/block/quorum.c b/block/quorum.c
index ec6f3b9..331b726 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -971,9 +971,7 @@ close_exit:
 exit:
 qemu_opts_del(opts);
 /* propagate error */
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 return ret;
 }
 
diff --git a/block/raw-posix.c b/block/raw-posix.c
index ce2e20f..cb663d8 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -587,9 +587,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, 
int flags,
 
 s->type = FTYPE_FILE;
 ret = raw_open_common(bs, options, flags, 0, _err);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 return ret;
 }
 
@@ -2239,9 +2237,7 @@ hdev_open_Mac_error:
 
 ret = raw_open_common(bs, options, flags, 0, _err);
 if (ret < 0) {
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 #if defined(__APPLE__) && defined(__MACH__)
 if (*bsd_path) {
 filename = bsd_path;
@@ -2453,9 +2449,7 @@ static int cdrom_open(BlockDriverState *bs, QDict 
*options, int flags,
 
 /* open will not fail even if no CD is inserted, so add O_NONBLOCK */
 ret = raw_open_common(bs, options, flags, O_NONBLOCK, _err);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 return ret;
 }
 
@@ -2573,9 +2567,7 @@ static int cdrom_open(BlockDriverState *bs, QDict 
*opti

[Qemu-block] [RFC v2 3/3] Remove unnecessary variables for function return value

[Eduardo Habkost]

Use Coccinelle script to replace 'ret = E; return ret' with
'return E'. The script will do the substitution only when the
function return type and variable type are the same.

Sending as RFC because the patch looks more intrusive than the
others. Probably better to split it per subsystem and let each
maintainer review and apply it?

Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 audio/audio.c| 10 ++
 block.c  |  4 +---
 block/archipelago.c  |  4 +---
 block/qcow2-cluster.c|  7 ++-
 block/qcow2-refcount.c   |  7 ++-
 block/raw-posix.c|  8 ++--
 block/raw_bsd.c  |  5 +
 block/rbd.c  |  5 +
 block/vmdk.c |  6 ++
 block/vvfat.c|  5 +
 hw/acpi/aml-build.c  | 13 +++--
 hw/audio/intel-hda.c |  5 +
 hw/display/vga.c |  4 +---
 hw/intc/s390_flic_kvm.c  |  5 ++---
 hw/pci-host/uninorth.c   |  5 +
 hw/ppc/spapr_vio.c   |  7 +--
 hw/scsi/megasas.c| 10 +-
 hw/scsi/scsi-generic.c   |  5 +
 hw/timer/mc146818rtc.c   |  5 +
 hw/virtio/virtio-pci.c   |  4 +---
 linux-user/signal.c  | 15 ---
 page_cache.c |  5 +
 qga/commands-posix.c |  4 +---
 qga/commands-win32.c |  6 +-
 qobject/qlist.c  |  5 +
 scripts/coccinelle/return_directly.cocci | 19 +++
 target-i386/fpu_helper.c | 10 ++
 target-i386/kvm.c|  5 ++---
 target-mips/dsp_helper.c | 15 +++
 target-mips/op_helper.c  |  4 +---
 target-s390x/helper.c|  6 +-
 target-sparc/cc_helper.c | 25 +
 target-tricore/op_helper.c   | 13 -
 tests/display-vga-test.c |  6 +-
 tests/endianness-test.c  |  5 +
 tests/i440fx-test.c  |  4 +---
 tests/intel-hda-test.c   |  6 +-
 tests/test-filter-redirector.c   |  6 +-
 tests/virtio-blk-test.c  |  5 +
 tests/virtio-console-test.c  |  6 +-
 tests/virtio-net-test.c  |  6 +-
 tests/virtio-scsi-test.c |  6 +-
 tests/wdt_ib700-test.c   |  6 +-
 ui/cursor.c  | 10 ++
 ui/qemu-pixman.c | 11 +++
 util/module.c|  6 +-
 vl.c |  5 +
 47 files changed, 90 insertions(+), 254 deletions(-)
 create mode 100644 scripts/coccinelle/return_directly.cocci

diff --git a/audio/audio.c b/audio/audio.c
index e60c124..9d4dcc7 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1131,8 +1131,6 @@ static void audio_timer (void *opaque)
  */
 int AUD_write (SWVoiceOut *sw, void *buf, int size)
 {
-int bytes;
-
 if (!sw) {
 /* XXX: Consider options */
 return size;
@@ -1143,14 +1141,11 @@ int AUD_write (SWVoiceOut *sw, void *buf, int size)
 return 0;
 }
 
-bytes = sw->hw->pcm_ops->write (sw, buf, size);
-return bytes;
+return sw->hw->pcm_ops->write(sw, buf, size);
 }
 
 int AUD_read (SWVoiceIn *sw, void *buf, int size)
 {
-int bytes;
-
 if (!sw) {
 /* XXX: Consider options */
 return size;
@@ -1161,8 +1156,7 @@ int AUD_read (SWVoiceIn *sw, void *buf, int size)
 return 0;
 }
 
-bytes = sw->hw->pcm_ops->read (sw, buf, size);
-return bytes;
+return sw->hw->pcm_ops->read(sw, buf, size);
 }
 
 int AUD_get_buffer_size_out (SWVoiceOut *sw)
diff --git a/block.c b/block.c
index d516ab6..c537307 100644
--- a/block.c
+++ b/block.c
@@ -351,15 +351,13 @@ out:
 int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
 {
 BlockDriver *drv;
-int ret;
 
 drv = bdrv_find_protocol(filename, true, errp);
 if (drv == NULL) {
 return -ENOENT;
 }
 
-ret = bdrv_create(drv, filename, opts, errp);
-return ret;
+return bdrv_create(drv, filename, opts, errp);
 }
 
 /**
diff --git a/block/archipelago.c b/block/archipelago.c
index b9f5e69..37b8aca 100644
--- a/block/archipelago.c
+++ b/block/archipelago.c
@@ -974,11 +974,9 @@ err_exit2:
 
 static int64_t qemu_archipelago_getlength(BlockDriverState *bs)
 {
-int64_t ret;
 BDRVArchipelagoState *s = bs->opaque;
 
-ret = archipelago_volume_info(s);
-return ret;
+return

[Qemu-block] [PATCH v2 0/3] coccinelle: Clean up error checks and return value variables

[Eduardo Habkost]

v2 of the previous "error: Remove NULL checks on
error_propagate() calls" patch, now it became a series.

Changes v1 -> v2:
* The Coccinelle scripts were simplified by using "when"
  constraints to detect when a variable is not used elsewhere
  inside the function.
* Added script to remove unnecessary variables for function
  return value.
* Coccinelle scripts added to scripts/coccinelle.

Eduardo Habkost (3):
  error: Remove NULL checks on error_propagate() calls
  error: Remove unnecessary local_err variables
  [RFC] Remove unnecessary variables for function return value

 audio/audio.c | 10 ++-
 block.c   | 26 -
 block/archipelago.c   |  4 +--
 block/qcow2-cluster.c |  7 ++---
 block/qcow2-refcount.c|  7 ++---
 block/qcow2.c |  4 +--
 block/quorum.c|  4 +--
 block/raw-posix.c | 24 +++
 block/raw_bsd.c   |  9 +-
 block/rbd.c   |  5 +---
 block/snapshot.c  |  4 +--
 block/vmdk.c  |  6 ++--
 block/vvfat.c |  5 +---
 blockdev.c| 26 +
 bootdevice.c  |  4 +--
 dump.c|  4 +--
 hw/acpi/aml-build.c   | 13 ++---
 hw/audio/intel-hda.c  |  5 +---
 hw/display/vga.c  |  4 +--
 hw/ide/qdev.c |  4 +--
 hw/intc/s390_flic_kvm.c   |  5 ++--
 hw/net/ne2000-isa.c   |  4 +--
 hw/pci-host/uninorth.c|  5 +---
 hw/ppc/spapr_vio.c|  7 +
 hw/s390x/s390-virtio-ccw.c|  5 +---
 hw/s390x/virtio-ccw.c | 42 +--
 hw/scsi/megasas.c | 10 +--
 hw/scsi/scsi-generic.c|  5 +---
 hw/timer/mc146818rtc.c|  5 +---
 hw/usb/dev-storage.c  |  4 +--
 hw/virtio/virtio-pci.c|  4 +--
 linux-user/signal.c   | 15 +++---
 page_cache.c  |  5 +---
 qga/commands-posix.c  |  4 +--
 qga/commands-win32.c  | 14 ++---
 qobject/qlist.c   |  5 +---
 qom/object.c  |  4 +--
 scripts/coccinelle/error_propagate_null.cocci | 10 +++
 scripts/coccinelle/remove_local_err.cocci | 27 +
 scripts/coccinelle/return_directly.cocci  | 19 
 target-i386/cpu.c |  4 +--
 target-i386/fpu_helper.c  | 10 ++-
 target-i386/kvm.c |  5 ++--
 target-mips/dsp_helper.c  | 15 ++
 target-mips/op_helper.c   |  4 +--
 target-s390x/helper.c |  6 +---
 target-sparc/cc_helper.c  | 25 
 target-tricore/op_helper.c| 13 +++--
 tests/display-vga-test.c  |  6 +---
 tests/endianness-test.c   |  5 +---
 tests/i440fx-test.c   |  4 +--
 tests/intel-hda-test.c|  6 +---
 tests/test-filter-redirector.c|  6 +---
 tests/virtio-blk-test.c   |  5 +---
 tests/virtio-console-test.c   |  6 +---
 tests/virtio-net-test.c   |  6 +---
 tests/virtio-scsi-test.c  |  6 +---
 tests/wdt_ib700-test.c|  6 +---
 ui/cursor.c   | 10 ++-
 ui/qemu-pixman.c  | 11 ++-
 util/module.c |  6 +---
 vl.c  |  5 +---
 62 files changed, 160 insertions(+), 384 deletions(-)
 create mode 100644 scripts/coccinelle/error_propagate_null.cocci
 create mode 100644 scripts/coccinelle/remove_local_err.cocci
 create mode 100644 scripts/coccinelle/return_directly.cocci

-- 
2.5.5

Re: [Qemu-block] [PATCH] error: Avoid redudant error_propagate() usage

[Eduardo Habkost]

On Thu, Jun 09, 2016 at 02:54:51PM -0600, Eric Blake wrote:
> On 06/09/2016 02:47 PM, Eduardo Habkost wrote:
> > On Thu, Jun 09, 2016 at 05:21:34PM -0300, Eduardo Habkost wrote:
> >> This patch simplifies code that uses a local_err variable just to 
> >> immediately
> >> use it for an error_propagate() call.
> > 
> > Please ignore this one. I found a better way to tell Coccinelle
> > to do that. Updated Coccinelle patch is below, but I will send
> > the actual patch tomorrow.
> 
> Cool.  Worth sticking these in scripts/coccinelle/ as part of your
> commit so that we can rerun them later (and refer to them for ideas when
> writing new scripts)?

I didn't know about scripts/coccinelle. I will do!

-- 
Eduardo

[Qemu-block] [PATCH] error: Avoid redudant error_propagate() usage

[Eduardo Habkost]

This patch simplifies code that uses a local_err variable just to immediately
use it for an error_propagate() call.

Done using the following Coccinelle patch:

  @@
  expression R;
  expression list ARGS;
  type T;
  identifier F1, F2;
  identifier LOCAL_ERR;
  identifier ERRP;
  idexpression V;
  typedef Error;
  @@
   T F1(..., Error **ERRP)
   {
  (
   // this slice guarantees that the code won't be changed
   // if LOCAL_ERR is used elsewhere in the function
   // (i.e. if it appears 3+ times in the function body)
   ...
   Error *LOCAL_ERR;
   ...
   LOCAL_ERR
   ...
   LOCAL_ERR
   ...
   LOCAL_ERR
   ...
  |
   ...
  -Error *LOCAL_ERR;
   ...
  (
  -F2(ARGS, _ERR);
  -error_propagate(ERRP, LOCAL_ERR);
  +F2(ARGS, ERRP);
  |
  -V = F2(ARGS, _ERR);
  -error_propagate(ERRP, LOCAL_ERR);
  +V = F2(ARGS, ERRP);
  )
   ...
  )
   }

Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
In the end, I found a way to avoid matching cases where local_err
is used elsewhere in the function.
---
 block.c|  4 +---
 block/raw-posix.c  |  8 ++--
 block/raw_bsd.c|  4 +---
 blockdev.c | 16 +---
 hw/s390x/s390-virtio-ccw.c |  5 +
 hw/s390x/virtio-ccw.c  | 28 +++-
 target-i386/cpu.c  |  4 +---
 7 files changed, 18 insertions(+), 51 deletions(-)

diff --git a/block.c b/block.c
index ecca55a..5cc83e5 100644
--- a/block.c
+++ b/block.c
@@ -353,7 +353,6 @@ out:
 int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
 {
 BlockDriver *drv;
-Error *local_err = NULL;
 int ret;
 
 drv = bdrv_find_protocol(filename, true, errp);
@@ -361,8 +360,7 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, 
Error **errp)
 return -ENOENT;
 }
 
-ret = bdrv_create(drv, filename, opts, _err);
-error_propagate(errp, local_err);
+ret = bdrv_create(drv, filename, opts, errp);
 return ret;
 }
 
diff --git a/block/raw-posix.c b/block/raw-posix.c
index cb663d8..d7397bf 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -582,12 +582,10 @@ static int raw_open(BlockDriverState *bs, QDict *options, 
int flags,
 Error **errp)
 {
 BDRVRawState *s = bs->opaque;
-Error *local_err = NULL;
 int ret;
 
 s->type = FTYPE_FILE;
-ret = raw_open_common(bs, options, flags, 0, _err);
-error_propagate(errp, local_err);
+ret = raw_open_common(bs, options, flags, 0, errp);
 return ret;
 }
 
@@ -2442,14 +2440,12 @@ static int cdrom_open(BlockDriverState *bs, QDict 
*options, int flags,
   Error **errp)
 {
 BDRVRawState *s = bs->opaque;
-Error *local_err = NULL;
 int ret;
 
 s->type = FTYPE_CD;
 
 /* open will not fail even if no CD is inserted, so add O_NONBLOCK */
-ret = raw_open_common(bs, options, flags, O_NONBLOCK, _err);
-error_propagate(errp, local_err);
+ret = raw_open_common(bs, options, flags, O_NONBLOCK, errp);
 return ret;
 }
 
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 5af11b6..b51ac98 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -190,11 +190,9 @@ static int raw_has_zero_init(BlockDriverState *bs)
 
 static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
 {
-Error *local_err = NULL;
 int ret;
 
-ret = bdrv_create_file(filename, opts, _err);
-error_propagate(errp, local_err);
+ret = bdrv_create_file(filename, opts, errp);
 return ret;
 }
 
diff --git a/blockdev.c b/blockdev.c
index 028dba3..3b6d242 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3654,7 +3654,6 @@ void qmp_blockdev_mirror(const char *device, const char 
*target,
 BlockBackend *blk;
 BlockDriverState *target_bs;
 AioContext *aio_context;
-Error *local_err = NULL;
 
 blk = blk_by_name(device);
 if (!blk) {
@@ -3678,16 +3677,11 @@ void qmp_blockdev_mirror(const char *device, const char 
*target,
 
 bdrv_set_aio_context(target_bs, aio_context);
 
-blockdev_mirror_common(bs, target_bs,
-   has_replaces, replaces, sync,
-   has_speed, speed,
-   has_granularity, granularity,
-   has_buf_size, buf_size,
-   has_on_source_error, on_source_error,
-   has_on_target_error, on_target_error,
-   true, true,
-   _err);
-error_propagate(errp, local_err);
+blockdev_mirror_common(bs, target_bs, has_replaces, replaces, sync,
+   has_speed, speed, has_granularity, granularity,
+   has_buf_size, buf_size, has_on_source_error,
+   on_source_error, has_on_target_error,
+   on_target_error, true, true, errp);
 
 aio_context_r

Re: [Qemu-block] [Qemu-devel] [PATCH] error: Remove NULL checks on error_propagate() calls

[Eduardo Habkost]

On Thu, Jun 09, 2016 at 01:37:23PM -0600, Eric Blake wrote:
[...]
> 
> Hmm - it seems like in most of the cases where the ONLY thing done in
> the if (local_err) block is to propagate the error, we should instead be
> directly assigning to errp instead of wasting a local variable.  At this
> point, my review is repetitive enough that I'll stop looking, and leave
> it up to you and Markus whether to attempt a more ambitious Coccinelle
> script.

If it happens immediately before the function end or a return
statement it should be easy, but it would still require some
manual work to remove the unused variable declaration. Probably
easier to do that in a follow-up patch.

It's harder (impossible?) to make Coccinelle avoid matching if
local_err is used somewhere else in the function. But it's
probably doable with some manual work, in a follow-up patch.

-- 
Eduardo

[Qemu-block] [PATCH] error: Remove NULL checks on error_propagate() calls

[Eduardo Habkost]

error_propagate() already ignores local_err==NULL, so there's no
need to check it before calling.

Done using the following Coccinelle patch:

  @@
  identifier L;
  expression E;
  @@
  -if (L) {
   error_propagate(E, L);
  -}

Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 block.c   | 20 +---
 block/qcow2.c |  4 +---
 block/quorum.c|  4 +---
 block/raw-posix.c | 16 
 block/raw_bsd.c   |  4 +---
 block/snapshot.c  |  4 +---
 blockdev.c| 12 +++-
 bootdevice.c  |  4 +---
 dump.c|  4 +---
 hw/ide/qdev.c |  4 +---
 hw/net/ne2000-isa.c   |  4 +---
 hw/s390x/virtio-ccw.c | 28 +++-
 hw/usb/dev-storage.c  |  4 +---
 qga/commands-win32.c  |  8 ++--
 qom/object.c  |  4 +---
 15 files changed, 31 insertions(+), 93 deletions(-)

diff --git a/block.c b/block.c
index f54bc25..ecca55a 100644
--- a/block.c
+++ b/block.c
@@ -301,9 +301,7 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque)
 assert(cco->drv);
 
 ret = cco->drv->bdrv_create(cco->filename, cco->opts, _err);
-if (local_err) {
-error_propagate(>err, local_err);
-}
+error_propagate(>err, local_err);
 cco->ret = ret;
 }
 
@@ -364,9 +362,7 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, 
Error **errp)
 }
 
 ret = bdrv_create(drv, filename, opts, _err);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 return ret;
 }
 
@@ -1760,18 +1756,14 @@ fail:
 QDECREF(options);
 bs->options = NULL;
 bdrv_unref(bs);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 return NULL;
 
 close_and_fail:
 bdrv_unref(bs);
 QDECREF(snapshot_options);
 QDECREF(options);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 return NULL;
 }
 
@@ -3591,9 +3583,7 @@ void bdrv_img_create(const char *filename, const char 
*fmt,
 out:
 qemu_opts_del(opts);
 qemu_opts_free(create_opts);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 }
 
 AioContext *bdrv_get_aio_context(BlockDriverState *bs)
diff --git a/block/qcow2.c b/block/qcow2.c
index 6f5fb81..4504846 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2394,9 +2394,7 @@ static int qcow2_create(const char *filename, QemuOpts 
*opts, Error **errp)
 ret = qcow2_create2(filename, size, backing_file, backing_fmt, flags,
 cluster_size, prealloc, opts, version, refcount_order,
 _err);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 
 finish:
 g_free(backing_file);
diff --git a/block/quorum.c b/block/quorum.c
index ec6f3b9..331b726 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -971,9 +971,7 @@ close_exit:
 exit:
 qemu_opts_del(opts);
 /* propagate error */
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 return ret;
 }
 
diff --git a/block/raw-posix.c b/block/raw-posix.c
index ce2e20f..cb663d8 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -587,9 +587,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, 
int flags,
 
 s->type = FTYPE_FILE;
 ret = raw_open_common(bs, options, flags, 0, _err);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 return ret;
 }
 
@@ -2239,9 +2237,7 @@ hdev_open_Mac_error:
 
 ret = raw_open_common(bs, options, flags, 0, _err);
 if (ret < 0) {
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 #if defined(__APPLE__) && defined(__MACH__)
 if (*bsd_path) {
 filename = bsd_path;
@@ -2453,9 +2449,7 @@ static int cdrom_open(BlockDriverState *bs, QDict 
*options, int flags,
 
 /* open will not fail even if no CD is inserted, so add O_NONBLOCK */
 ret = raw_open_common(bs, options, flags, O_NONBLOCK, _err);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 return ret;
 }
 
@@ -2573,9 +2567,7 @@ static int cdrom_open(BlockDriverState *bs, QDict 
*options, int flags,
 
 ret = raw_open_common(bs, options, flags, 0, _err);
 if (ret) {
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 return ret;
 }
 
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index b1d5237..5af11b6 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -194,9 +194,7 @@ static int raw_create(const char *filename, QemuOpts

Re: [Qemu-block] [Qemu-devel] [PATCH] error: Remove NULL checks on error_propagate() calls

[Eduardo Habkost]

On Thu, Jun 09, 2016 at 02:11:23PM -0600, Eric Blake wrote:
> On 06/09/2016 01:50 PM, Eduardo Habkost wrote:
> > On Thu, Jun 09, 2016 at 01:37:23PM -0600, Eric Blake wrote:
> > [...]
> >>
> >> Hmm - it seems like in most of the cases where the ONLY thing done in
> >> the if (local_err) block is to propagate the error, we should instead be
> >> directly assigning to errp instead of wasting a local variable.  At this
> >> point, my review is repetitive enough that I'll stop looking, and leave
> >> it up to you and Markus whether to attempt a more ambitious Coccinelle
> >> script.
> > 
> > If it happens immediately before the function end or a return
> > statement it should be easy, but it would still require some
> > manual work to remove the unused variable declaration. Probably
> > easier to do that in a follow-up patch.
> 
> I think Coccinelle can be used to eliminate unused local variables, but
> don't know the recipe off-hand; maybe a web search will turn up something?

I found something called "when constraints", but the
documentation isn't very clear.

There's an example here:
http://coccinelle.lip6.fr/rules/unused.cocci

> 
> > 
> > It's harder (impossible?) to make Coccinelle avoid matching if
> > local_err is used somewhere else in the function. But it's
> > probably doable with some manual work, in a follow-up patch.
> 
> I don't know - Coccinelle is rather powerful, and there may indeed be a
> way to flag conditions for a variable that is not used anywhere except
> in the lines mentioned in the recipe, vs. a variable that can also be
> used in the ... portion of the recipe.

I found a hackish way to do that. Sending a follow-up patch in 1
minute.

-- 
Eduardo

Re: [Qemu-block] [Qemu-devel] [RFC] ide: Don't use qemu_hw_version() for firmware revision

[Eduardo Habkost]

On Thu, Nov 12, 2015 at 09:27:56AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabk...@redhat.com> writes:
> 
> > The IDEState.version field is used for firmware version
> > information returned to the guest. Updating firmware information
> > on QEMU upgrade is supposed to be acceptable, so IDE doesn't need
> > the version compatibility magic of qemu_hw_version() and can use
> > QEMU_VERSION directly.
> >
> > Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
> > ---
> > I'm sending this just to start a discussion about what exactly we
> > are supposed to return to the guest on those IDE fields. Should
> > we return:
> >
> > 1) Something that never changes and don't reveal QEMU version
> >information (e.g. "QEMU")?
> > 2) Something that is always the same depending on the
> >machine-type (machine-type name? MachineClass.hw_version?)
> > 3) Something that change every time QEMU is upgraded (QEMU_VERSION)?
> > 4) Something else?
> >
> > This patch implements option (3).
> >
> > ---
> >  hw/ide/core.c | 2 +-
> >  hw/ide/internal.h | 2 ++
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > index 364ba21..1602707 100644
> > --- a/hw/ide/core.c
> > +++ b/hw/ide/core.c
> > @@ -2312,7 +2312,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, 
> > IDEDriveKind kind,
> >  if (version) {
> >  pstrcpy(s->version, sizeof(s->version), version);
> >  } else {
> > -pstrcpy(s->version, sizeof(s->version), qemu_hw_version());
> > +pstrcpy(s->version, sizeof(s->version), QEMU_VERSION);
> 
> Is s->version migrated?

AFAICS, it's not.

> 
> If no, live migration to a newer QEMU changes the version, doesn't it?
> The "firmware upgrade is acceptable" argument doesn't apply there.  I
> guess a spontaneous version change is relatively unlikely to cause
> trouble, but why risk it?

Good point.

Michael proposed we just change the default value of
qemu_hw_version() to something constant (like "3.0" or "2.5+")
starting on pc-*-2.5, and never update hw_version on newer
machine-types anymore. I think his proposal makes sense.

-- 
Eduardo

[Qemu-block] [RFC] ide: Don't use qemu_hw_version() for firmware revision

[Eduardo Habkost]

The IDEState.version field is used for firmware version
information returned to the guest. Updating firmware information
on QEMU upgrade is supposed to be acceptable, so IDE doesn't need
the version compatibility magic of qemu_hw_version() and can use
QEMU_VERSION directly.

Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
I'm sending this just to start a discussion about what exactly we
are supposed to return to the guest on those IDE fields. Should
we return:

1) Something that never changes and don't reveal QEMU version
   information (e.g. "QEMU")?
2) Something that is always the same depending on the
   machine-type (machine-type name? MachineClass.hw_version?)
3) Something that change every time QEMU is upgraded (QEMU_VERSION)?
4) Something else?

This patch implements option (3).

---
 hw/ide/core.c | 2 +-
 hw/ide/internal.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 364ba21..1602707 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2312,7 +2312,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, 
IDEDriveKind kind,
 if (version) {
 pstrcpy(s->version, sizeof(s->version), version);
 } else {
-pstrcpy(s->version, sizeof(s->version), qemu_hw_version());
+pstrcpy(s->version, sizeof(s->version), QEMU_VERSION);
 }
 
 ide_reset(s);
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index e4629b0..a4277ce 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -378,6 +378,7 @@ struct IDEState {
 /* set for lba48 access */
 uint8_t lba48;
 BlockBackend *blk;
+/* Firmware revision/version */
 char version[9];
 /* ATAPI specific */
 struct unreported_events events;
@@ -488,6 +489,7 @@ struct IDEDevice {
 uint32_t unit;
 BlockConf conf;
 int chs_trans;
+/* Firmware revision/version */
 char *version;
 char *serial;
 char *model;
-- 
2.1.0

Re: [Qemu-block] [PATCH v7 12/35] util: let qemu_fd_getlength support block device

[Eduardo Habkost]

On Mon, Nov 09, 2015 at 01:58:27PM +0800, Xiao Guangrong wrote:
> 
> 
> On 11/06/2015 11:54 PM, Eduardo Habkost wrote:
> >On Mon, Nov 02, 2015 at 05:13:14PM +0800, Xiao Guangrong wrote:
> >>lseek can not work for all block devices as the man page says:
> >>| Some devices are incapable of seeking and POSIX does not specify
> >>| which devices must support lseek().
> >>
> >>This patch tries to add the support on Linux by using BLKGETSIZE64
> >>ioctl
> >>
> >>Signed-off-by: Xiao Guangrong <guangrong.x...@linux.intel.com>
> >
> >On which cases is this patch necessary? Do you know any examples of
> >Linux block devices that won't work with lseek(SEEK_END)?
> 
> To be honest, i have not checked all block device, this patch was made
> based on the man page. However, i do not mind drop this patch (and maybe
> other patches) to make this pachset smaller. BLKGETSIZE64 can be added
> in the future if we meet such device.

By looking at the Linux source code implementing BLKGETSIZE64, it looks
like it should work for all block devices where SEEK_END works:

* BLKGETSIZE64 returns i_size_read(bdev->bd_inode)
  (block/ioctl.c:blkdev_ioctl())
* llseek(SEEK_END) uses i_size_read(bd_inode) as the offset
  (fs/block_dev.c:block_llseek())

That's probably why raw_getlength() never needed a Linux-specific
BLKGETSIZE call.

-- 
Eduardo

Re: [Qemu-block] [PATCH v7 11/35] util: introduce qemu_file_getlength()

[Eduardo Habkost]

On Mon, Nov 09, 2015 at 12:44:55PM +0800, Xiao Guangrong wrote:
> On 11/06/2015 11:50 PM, Eduardo Habkost wrote:
> >As this patch affects raw_getlength(), CCing the raw block driver
> >maintainer and the qemu-block mailing list.
> 
> Eduardo, thanks for your reminder. I will keep CCing Kevin and qemu-block mail
> list for future version.
> 
> >
> >On Mon, Nov 02, 2015 at 05:13:13PM +0800, Xiao Guangrong wrote:
> >>It is used to get the size of the specified file, also qemu_fd_getlength()
> >>is introduced to unify the code with raw_getlength() in block/raw-posix.c
> >>
> >>Signed-off-by: Xiao Guangrong <guangrong.x...@linux.intel.com>
> >>---
> >>  block/raw-posix.c|  7 +--
> >>  include/qemu/osdep.h |  2 ++
> >>  util/osdep.c | 31 +++
> >
> >I know I was the one who suggested osdep.c, but maybe oslib-posix.c is a
> >more appropriate place for the new function?
> >
> 
> Since the function we introduced here can work on both windows and posix, so
> i thing osdep.c is the right place. Otherwise we should implement it for 
> multiple
> platforms.

I didn't notice it was going to be used by a platform-independent
qemu_file_getlength() function in addition to the posix-specific
raw_getlength(). Have you tested it on Windows, though?

If you didn't test it on Windows, what about keeping
qemu_file_getlength() available only on posix, by now? The only
users are raw-posix.c and hostmem-file.c, currently. If in the
future somebody need it on Windows, they can decide between
moving the SEEK_END code to osdep.c (after testing it), or moving
the existing raw-win32.c:raw_getlength() code to a
oslib-win32.c:qemu_file_getlength() function.

-- 
Eduardo

Re: [Qemu-block] [PATCH v7 12/35] util: let qemu_fd_getlength support block device

[Eduardo Habkost]

As this patch affects raw_getlength(), CCing the raw block driver
maintainer and the qemu-block mailing list.

On Mon, Nov 02, 2015 at 05:13:14PM +0800, Xiao Guangrong wrote:
> lseek can not work for all block devices as the man page says:
> | Some devices are incapable of seeking and POSIX does not specify
> | which devices must support lseek().
> 
> This patch tries to add the support on Linux by using BLKGETSIZE64
> ioctl
> 
> Signed-off-by: Xiao Guangrong 
> ---
>  util/osdep.c | 20 
>  1 file changed, 20 insertions(+)
> 
> diff --git a/util/osdep.c b/util/osdep.c
> index 5a61e19..b20c793 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -45,6 +45,11 @@
>  extern int madvise(caddr_t, size_t, int);
>  #endif
>  
> +#ifdef CONFIG_LINUX
> +#include 
> +#include 
> +#endif
> +
>  #include "qemu-common.h"
>  #include "qemu/sockets.h"
>  #include "qemu/error-report.h"
> @@ -433,6 +438,21 @@ int64_t qemu_fd_getlength(int fd)
>  {
>  int64_t size;
>  
> +#ifdef CONFIG_LINUX
> +struct stat stat_buf;
> +if (fstat(fd, _buf) < 0) {
> +return -errno;
> +}
> +
> +if ((S_ISBLK(stat_buf.st_mode)) && !ioctl(fd, BLKGETSIZE64, )) {
> +/* The size of block device is larger than max int64_t? */
> +if (size < 0) {
> +return -EOVERFLOW;
> +}
> +return size;
> +}
> +#endif
> +
>  size = lseek(fd, 0, SEEK_END);
>  if (size < 0) {
>  return -errno;
> -- 
> 1.8.3.1
> 

-- 
Eduardo

Re: [Qemu-block] [PATCH v7 11/35] util: introduce qemu_file_getlength()

[Eduardo Habkost]

As this patch affects raw_getlength(), CCing the raw block driver
maintainer and the qemu-block mailing list.

On Mon, Nov 02, 2015 at 05:13:13PM +0800, Xiao Guangrong wrote:
> It is used to get the size of the specified file, also qemu_fd_getlength()
> is introduced to unify the code with raw_getlength() in block/raw-posix.c
> 
> Signed-off-by: Xiao Guangrong 
> ---
>  block/raw-posix.c|  7 +--
>  include/qemu/osdep.h |  2 ++
>  util/osdep.c | 31 +++

I know I was the one who suggested osdep.c, but maybe oslib-posix.c is a
more appropriate place for the new function?


>  3 files changed, 34 insertions(+), 6 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 918c756..734e6dd 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -1592,18 +1592,13 @@ static int64_t raw_getlength(BlockDriverState *bs)
>  {
>  BDRVRawState *s = bs->opaque;
>  int ret;
> -int64_t size;
>  
>  ret = fd_open(bs);
>  if (ret < 0) {
>  return ret;
>  }
>  
> -size = lseek(s->fd, 0, SEEK_END);
> -if (size < 0) {
> -return -errno;
> -}
> -return size;
> +return qemu_fd_getlength(s->fd);
>  }
>  #endif
>  
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index dbc17dc..ca4c3fa 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -303,4 +303,6 @@ int qemu_read_password(char *buf, int buf_size);
>  pid_t qemu_fork(Error **errp);
>  
>  size_t qemu_file_get_page_size(const char *mem_path, Error **errp);
> +int64_t qemu_fd_getlength(int fd);
> +size_t qemu_file_getlength(const char *file, Error **errp);
>  #endif
> diff --git a/util/osdep.c b/util/osdep.c
> index 0092bb6..5a61e19 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -428,3 +428,34 @@ writev(int fd, const struct iovec *iov, int iov_cnt)
>  return readv_writev(fd, iov, iov_cnt, true);
>  }
>  #endif
> +
> +int64_t qemu_fd_getlength(int fd)
> +{
> +int64_t size;
> +
> +size = lseek(fd, 0, SEEK_END);
> +if (size < 0) {
> +return -errno;
> +}
> +return size;
> +}
> +
> +size_t qemu_file_getlength(const char *file, Error **errp)
> +{
> +int64_t size;
> +int fd = qemu_open(file, O_RDONLY);
> +
> +if (fd < 0) {
> +error_setg_file_open(errp, errno, file);
> +return 0;
> +}
> +
> +size = qemu_fd_getlength(fd);
> +if (size < 0) {
> +error_setg_errno(errp, -size, "can't get size of file %s", file);
> +size = 0;
> +}
> +
> +qemu_close(fd);
> +return size;
> +}
> -- 
> 1.8.3.1
> 

-- 
Eduardo

Re: [Qemu-block] [PATCH v7 12/35] util: let qemu_fd_getlength support block device

[Eduardo Habkost]

On Mon, Nov 02, 2015 at 05:13:14PM +0800, Xiao Guangrong wrote:
> lseek can not work for all block devices as the man page says:
> | Some devices are incapable of seeking and POSIX does not specify
> | which devices must support lseek().
> 
> This patch tries to add the support on Linux by using BLKGETSIZE64
> ioctl
> 
> Signed-off-by: Xiao Guangrong 

On which cases is this patch necessary? Do you know any examples of
Linux block devices that won't work with lseek(SEEK_END)?

> ---
>  util/osdep.c | 20 
>  1 file changed, 20 insertions(+)
> 
> diff --git a/util/osdep.c b/util/osdep.c
> index 5a61e19..b20c793 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -45,6 +45,11 @@
>  extern int madvise(caddr_t, size_t, int);
>  #endif
>  
> +#ifdef CONFIG_LINUX
> +#include 
> +#include 
> +#endif
> +
>  #include "qemu-common.h"
>  #include "qemu/sockets.h"
>  #include "qemu/error-report.h"
> @@ -433,6 +438,21 @@ int64_t qemu_fd_getlength(int fd)
>  {
>  int64_t size;
>  
> +#ifdef CONFIG_LINUX
> +struct stat stat_buf;
> +if (fstat(fd, _buf) < 0) {
> +return -errno;
> +}
> +
> +if ((S_ISBLK(stat_buf.st_mode)) && !ioctl(fd, BLKGETSIZE64, )) {
> +/* The size of block device is larger than max int64_t? */
> +if (size < 0) {
> +return -EOVERFLOW;
> +}
> +return size;
> +}
> +#endif
> +
>  size = lseek(fd, 0, SEEK_END);
>  if (size < 0) {
>  return -errno;
> -- 
> 1.8.3.1
> 

-- 
Eduardo

[Qemu-block] [PATCH RESEND v2 3/3] megasas: Use qemu_hw_version() instead of QEMU_VERSION

[Eduardo Habkost]

Guest visible data shouldn't change with a simple QEMU upgrade, so use
qemu_hw_version() to ensure it won't change (as long as the machine
class being used has hw_version set).

Cc: Hannes Reinecke <h...@suse.de>
Cc: Paolo Bonzini <pbonz...@redhat.com>
Cc: qemu-block@nongnu.org
Reviewed-by: Hannes Reinecke <h...@suse.com>
Acked-by: Laszlo Ersek <ler...@redhat.com>
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 hw/scsi/megasas.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index dcd724e..d7dc667 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -757,7 +757,7 @@ static int megasas_ctrl_get_info(MegasasState *s, 
MegasasCmd *cmd)
 
 memcpy(info.product_name, base_class->product_name, 24);
 snprintf(info.serial_number, 32, "%s", s->hba_serial);
-snprintf(info.package_version, 0x60, "%s-QEMU", QEMU_VERSION);
+snprintf(info.package_version, 0x60, "%s-QEMU", qemu_hw_version());
 memcpy(info.image_component[0].name, "APP", 3);
 snprintf(info.image_component[0].version, 10, "%s-QEMU",
  base_class->product_version);
-- 
2.1.0

[Qemu-block] [PATCH 3/3] megasas: Use qemu_hw_version() instead of QEMU_VERSION

[Eduardo Habkost]

Guest visible data shouldn't change with a simple QEMU upgrade, so use
qemu_hw_version() to ensure it won't change (as long as the machine
class being used has hw_version set).

Cc: Hannes Reinecke <h...@suse.de>
Cc: Paolo Bonzini <pbonz...@redhat.com>
Cc: qemu-block@nongnu.org
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
 hw/scsi/megasas.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index a04369c..e0529b1 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -757,7 +757,7 @@ static int megasas_ctrl_get_info(MegasasState *s, 
MegasasCmd *cmd)
 
 memcpy(info.product_name, base_class->product_name, 24);
 snprintf(info.serial_number, 32, "%s", s->hba_serial);
-snprintf(info.package_version, 0x60, "%s-QEMU", QEMU_VERSION);
+snprintf(info.package_version, 0x60, "%s-QEMU", qemu_hw_version());
 memcpy(info.image_component[0].name, "APP", 3);
 snprintf(info.image_component[0].version, 10, "%s-QEMU",
  base_class->product_version);
-- 
2.1.0

Re: [Qemu-block] [PATCH 3/3] megasas: Use qemu_hw_version() instead of QEMU_VERSION

[Eduardo Habkost]

On Tue, Sep 22, 2015 at 10:33:32PM +0200, Laszlo Ersek wrote:
> On 09/22/15 22:16, Eduardo Habkost wrote:
> > Guest visible data shouldn't change with a simple QEMU upgrade, so use
> > qemu_hw_version() to ensure it won't change (as long as the machine
> > class being used has hw_version set).
> > 
> > Cc: Hannes Reinecke <h...@suse.de>
> > Cc: Paolo Bonzini <pbonz...@redhat.com>
> > Cc: qemu-block@nongnu.org
> > Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
> > ---
> >  hw/scsi/megasas.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> > index a04369c..e0529b1 100644
> > --- a/hw/scsi/megasas.c
> > +++ b/hw/scsi/megasas.c
> > @@ -757,7 +757,7 @@ static int megasas_ctrl_get_info(MegasasState *s, 
> > MegasasCmd *cmd)
> >  
> >  memcpy(info.product_name, base_class->product_name, 24);
> >  snprintf(info.serial_number, 32, "%s", s->hba_serial);
> > -snprintf(info.package_version, 0x60, "%s-QEMU", QEMU_VERSION);
> > +snprintf(info.package_version, 0x60, "%s-QEMU", qemu_hw_version());
> >  memcpy(info.image_component[0].name, "APP", 3);
> >  snprintf(info.image_component[0].version, 10, "%s-QEMU",
> >   base_class->product_version);
> > 
> 
> I assume you audited all uses of QEMU_VERSION, and this was the only one
> exposed to the guest directly.
> 
> However, in "hw/usb/redirect.c", QEMU_VERSION is embedded in VERSION,
> and the latter is then passed to usbredirparser_init() in
> usbredir_create_parser().
> 
> I tried to look up the documentation for usbredirparser_init() in
> "/usr/include/usbredirparser.h", but I still have no clue what that
> "version" parameter controls.
> 
> Hm... from the source code of usbredir, and
> <http://www.spice-space.org/page/UsbRedir> stating "usbredir is the name
> of a network protocol for sending usb device traffic over a network
> connection", it looks like the version number is embedded in the hello
> message of that network protocol; so it shouldn't be exposed to the
> guest indeed.

Investigating how VERSION was used in usbredirparser was next on my
todo-list. Thanks for doing that. :)

-- 
Eduardo