Re: [Qemu-block] [Qemu-devel] [PATCH for-2.5?] qcow2: always initialize specific image info

[Kevin Wolf]

Am 07.12.2015 um 20:44 hat Max Reitz geschrieben:
> I'd be completely fine with adding an "else { abort(); }" branch to
> qcow2_get_specific_info().

This is actually what I was going to suggest, too. Of course, it's not
supposed to fix anything now, but defensive coding has never hurt.

Kevin


pgpA0H4E1AkxO.pgp
Description: PGP signature

Re: [Qemu-block] [Qemu-devel] [PATCH] virtio-blk: Drop x-data-plane option

[Cornelia Huck]

On Tue, 8 Dec 2015 09:56:14 +0800
Fam Zheng  wrote:

> On Mon, 12/07 21:02, Fam Zheng wrote:
> > On Mon, 12/07 12:29, Cornelia Huck wrote:
> > > No general objection to removing x-data-plane; but this probably wants
> > > a mention on the changelog as x-data-plane has been described in
> > > various howtos etc. over the years.
> 
> Add a changelog line,
> 
> http://wiki.qemu.org/ChangeLog/2.5#Block_devices_and_tools
> 
> please review.

Looks sane, although I'd probably add a line to "Incompatible changes"
as well.

Re: [Qemu-block] [Qemu-devel] [PATCH 1/2] Add param Error** to msi_init() & modify the callers

[Cao jin]

Hi Markus,
I have to say, you really did a amazing review for this "trivial 
"patch, thanks a lot & really appreciate it:)


On 12/07/2015 05:59 PM, Markus Armbruster wrote:

Cao jin  writes:


msi_init() is a supporting function in PCI device initialization, in order to
convert .init() to .realize(), it should be modified first. Also modify the
callers

Bonus: add more comment for msi_init().


Incomplete.  See notes on impact inline.


Signed-off-by: Cao jin 
---
  hw/audio/intel-hda.c   |  7 ++-
  hw/ide/ich.c   |  2 +-
  hw/net/vmxnet3.c   |  3 ++-
  hw/pci-bridge/ioh3420.c|  6 +-
  hw/pci-bridge/pci_bridge_dev.c |  6 +-
  hw/pci-bridge/xio3130_downstream.c |  7 ++-
  hw/pci-bridge/xio3130_upstream.c   |  7 ++-
  hw/pci/msi.c   | 17 +
  hw/scsi/megasas.c  | 12 +---
  hw/scsi/vmw_pvscsi.c   |  3 ++-
  hw/usb/hcd-xhci.c  |  5 -
  hw/vfio/pci.c  |  3 ++-
  include/hw/pci/msi.h   |  4 ++--
  13 files changed, 63 insertions(+), 19 deletions(-)

diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index 433463e..9d733da 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -1130,6 +1130,7 @@ static void intel_hda_realize(PCIDevice *pci, Error 
**errp)
  {
  IntelHDAState *d = INTEL_HDA(pci);
  uint8_t *conf = d->pci.config;
+int ret;

  d->name = object_get_typename(OBJECT(d));

@@ -1142,7 +1143,11 @@ static void intel_hda_realize(PCIDevice *pci, Error 
**errp)
"intel-hda", 0x4000);
  pci_register_bar(>pci, 0, 0, >mmio);
  if (d->msi) {
-msi_init(>pci, d->old_msi_addr ? 0x50 : 0x60, 1, true, false);
+ret = msi_init(>pci, d->old_msi_addr ? 0x50 : 0x60, 1, true,
+false, errp);
+if(ret < 0) {


Please use scripts/checkpatch.pl to check your patches.  It's
occasionally wrong, so use your judgement.



Thanks for the tips, seems I got dizzy looking because many trivial 
place need to be modified...



+return;


This returns with the device in a half-realized state.  Do we have to
undo prior side effects to put it back into unrealized state?  See also
ioh3420_initfn() below.

Before: msi_init() failure is ignored.  After: it makes device
realization fail.  To assess impact, we need to understand how
msi_init() can fail.



It seems I missed the reality: devices are default to be hot-pluggable & 
most devices are hot-pluggable:-[ Because when cold plugged, process 
will exit on device-init failing, so, the half-realized state doesn`t 
matter in this condition.

Will rework it later.


+}
  }

  hda_codec_bus_init(DEVICE(pci), >codecs, sizeof(d->codecs),
diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index 16925fa..94b1809 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -145,7 +145,7 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error 
**errp)
  /* Although the AHCI 1.3 specification states that the first capability
   * should be PMCAP, the Intel ICH9 data sheet specifies that the ICH9
   * AHCI device puts the MSI capability first, pointing to 0x80. */
-msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false);
+msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, errp);


Do we have to put the device back into unrealized state on failure?


  }

  static void pci_ich9_uninit(PCIDevice *dev)
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 5e3a233..4269141 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2140,9 +2140,10 @@ vmxnet3_init_msi(VMXNET3State *s)
  {
  PCIDevice *d = PCI_DEVICE(s);
  int res;
+Error *local_err = NULL;

  res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
-   VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
+   VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, _err);
  if (0 > res) {
  VMW_WRPRN("Failed to initialize MSI, error %d", res);
  s->msi_used = false;


The error is neither propagated nor handled, and the error object leaks.

Since this function can't handle it, it needs to propagate it.  Requires
adding an Error ** parameter.



[*]Actually, here is my consideration: a device-realize function(take 
the following ioh3420 for example) will call many supporting functions 
like msi_init(), so I am planning, every supporting function goes into a 
patch first, then every "device convert to realize()" goes into a patch, 
otherwise, it may will be a big patch for the reviewer. That`s why I 
didn`t add Error ** param, and propagate it, and plan to do it in 
"convert to realize()" patch. But for now, I think this patch should at 
least be successfully compiled & won`t impact the existed things.


Yes, it seems may have leaks when error happens, but will be fixed when 
the "convert to realize()" patch 

Re: [Qemu-block] [Qemu-devel] [PATCH 1/2] Add param Error** to msi_init() & modify the callers

[Markus Armbruster]

Cao jin  writes:

> Hi Markus,
> I have to say, you really did a amazing review for this "trivial
> "patch, thanks a lot & really appreciate it:)

Thanks!  I'm afraid the problem you picked isn't trivial, but I hope
it's still simple enough to be a useful exercise to get you going with
the code.

> On 12/07/2015 05:59 PM, Markus Armbruster wrote:
>> Cao jin  writes:
>>
>>> msi_init() is a supporting function in PCI device initialization, in order 
>>> to
>>> convert .init() to .realize(), it should be modified first. Also modify the
>>> callers
>>>
>>> Bonus: add more comment for msi_init().
>>
>> Incomplete.  See notes on impact inline.
>>
>>> Signed-off-by: Cao jin 
>>> ---
>>>   hw/audio/intel-hda.c   |  7 ++-
>>>   hw/ide/ich.c   |  2 +-
>>>   hw/net/vmxnet3.c   |  3 ++-
>>>   hw/pci-bridge/ioh3420.c|  6 +-
>>>   hw/pci-bridge/pci_bridge_dev.c |  6 +-
>>>   hw/pci-bridge/xio3130_downstream.c |  7 ++-
>>>   hw/pci-bridge/xio3130_upstream.c   |  7 ++-
>>>   hw/pci/msi.c   | 17 +
>>>   hw/scsi/megasas.c  | 12 +---
>>>   hw/scsi/vmw_pvscsi.c   |  3 ++-
>>>   hw/usb/hcd-xhci.c  |  5 -
>>>   hw/vfio/pci.c  |  3 ++-
>>>   include/hw/pci/msi.h   |  4 ++--
>>>   13 files changed, 63 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
>>> index 433463e..9d733da 100644
>>> --- a/hw/audio/intel-hda.c
>>> +++ b/hw/audio/intel-hda.c
>>> @@ -1130,6 +1130,7 @@ static void intel_hda_realize(PCIDevice *pci, Error 
>>> **errp)
>>>   {
>>>   IntelHDAState *d = INTEL_HDA(pci);
>>>   uint8_t *conf = d->pci.config;
>>> +int ret;
>>>
>>>   d->name = object_get_typename(OBJECT(d));
>>>
>>> @@ -1142,7 +1143,11 @@ static void intel_hda_realize(PCIDevice *pci, Error 
>>> **errp)
>>> "intel-hda", 0x4000);
>>>   pci_register_bar(>pci, 0, 0, >mmio);
>>>   if (d->msi) {
>>> -msi_init(>pci, d->old_msi_addr ? 0x50 : 0x60, 1, true, false);
>>> +ret = msi_init(>pci, d->old_msi_addr ? 0x50 : 0x60, 1, true,
>>> +false, errp);
>>> +if(ret < 0) {
>>
>> Please use scripts/checkpatch.pl to check your patches.  It's
>> occasionally wrong, so use your judgement.
>>
>
> Thanks for the tips, seems I got dizzy looking because many trivial
> place need to be modified...
>
>>> +return;
>>
>> This returns with the device in a half-realized state.  Do we have to
>> undo prior side effects to put it back into unrealized state?  See also
>> ioh3420_initfn() below.
>>
>> Before: msi_init() failure is ignored.  After: it makes device
>> realization fail.  To assess impact, we need to understand how
>> msi_init() can fail.
>>
>
> It seems I missed the reality: devices are default to be hot-pluggable
> & most devices are hot-pluggable:-[ Because when cold plugged, process
> will exit on device-init failing, so, the half-realized state doesn`t
> matter in this condition.
> Will rework it later.

In theory, realize() should always fail cleanly.  In practice, unclean
realize() failure doesn't matter when it's fatal anyway.  Some devices
are only used where it's always fatal.  See also "Error handling in
realize() methods" I just sent to the list; I hope we can come up with
some guidance on when shortcuts in realize() methods are tolerable.

>>> +}
>>>   }
>>>
>>>   hda_codec_bus_init(DEVICE(pci), >codecs, sizeof(d->codecs),
>>> diff --git a/hw/ide/ich.c b/hw/ide/ich.c
>>> index 16925fa..94b1809 100644
>>> --- a/hw/ide/ich.c
>>> +++ b/hw/ide/ich.c
>>> @@ -145,7 +145,7 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error 
>>> **errp)
>>>   /* Although the AHCI 1.3 specification states that the first 
>>> capability
>>>* should be PMCAP, the Intel ICH9 data sheet specifies that the ICH9
>>>* AHCI device puts the MSI capability first, pointing to 0x80. */
>>> -msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false);
>>> +msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, errp);
>>
>> Do we have to put the device back into unrealized state on failure?
>>
>>>   }
>>>
>>>   static void pci_ich9_uninit(PCIDevice *dev)
>>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>>> index 5e3a233..4269141 100644
>>> --- a/hw/net/vmxnet3.c
>>> +++ b/hw/net/vmxnet3.c
>>> @@ -2140,9 +2140,10 @@ vmxnet3_init_msi(VMXNET3State *s)
>>>   {
>>>   PCIDevice *d = PCI_DEVICE(s);
>>>   int res;
>>> +Error *local_err = NULL;
>>>
>>>   res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
>>> -   VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
>>> +   VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, _err);
>>>   if (0 > res) {
>>>   VMW_WRPRN("Failed to 

Re: [Qemu-block] [PATCH v9 00/10] qcow2: Support refcount order amendment

[Kevin Wolf]

Am 27.07.2015 um 17:51 hat Max Reitz geschrieben:
> (v1..v7 were named "qcow2: Support refcount orders != 4")
> 
> This series contains the final 10 patches of my qcow2 refcount order
> series, which add refcount order amendment functionality to qemu-img.

Thanks, applied to block-next (after some trivial rebasing).

Kevin

Re: [Qemu-block] QEMU being able to use audio cdroms

[John Snow]

On 11/25/2015 03:44 PM, Programmingkid wrote:
> Is there any platform where a guest in QEMU can play an audio cd? If not, is 
> this a feature that you would allow into QEMU?
> 

I haven't tested it, nobody has ever asked. I wouldn't reject patches
for such a feature, but I likely wouldn't make reviewing them a priority
either.

I assume this would be primarily for emulating games/etc that use mixed
Data/Audio formats?

--js

Re: [Qemu-block] QEMU being able to use audio cdroms

[Programmingkid]

On Dec 8, 2015, at 1:49 PM, John Snow wrote:

> 
> 
> On 11/25/2015 03:44 PM, Programmingkid wrote:
>> Is there any platform where a guest in QEMU can play an audio cd? If not, is 
>> this a feature that you would allow into QEMU?
>> 
> 
> I haven't tested it, nobody has ever asked. I wouldn't reject patches
> for such a feature, but I likely wouldn't make reviewing them a priority
> either.
> 
> I assume this would be primarily for emulating games/etc that use mixed
> Data/Audio formats?

That could be one reason to do it. I just thought it would be neat to be able 
to play CDs inside a guest.

Re: [Qemu-block] [PATCH] qemu-img / curl: When fetching Content-Size use GET instead of HEAD.

[Michael Tokarev]

08.12.2015 00:23, Boris Schrijver wrote:
[]
> It's is therefore better to use only the GET request method, and discard the
> body at the first call.

Nooo!  Please N!

Fetching a large file once might be too long already.
Fetching it twice is twice as long.  Oh well!..

Thanks,

/mjt

Re: [Qemu-block] [PATCH] virtio-blk: Drop x-data-plane option

[Stefan Hajnoczi]

On Mon, Dec 07, 2015 at 05:10:26PM +, Peter Maydell wrote:
> On 7 December 2015 at 15:19, Paolo Bonzini  wrote:
> >
> >
> > On 07/12/2015 14:02, Fam Zheng wrote:
> >> On Mon, 12/07 12:29, Cornelia Huck wrote:
> >>> On Mon,  7 Dec 2015 18:59:27 +0800
> >>> Fam Zheng  wrote:
> >>>
>  The official way of enabling dataplane is through the "iothread"
>  property that references an iothread object created by "-object
>  iothread".  Since the old "x-data-plane=on" way now even crashes, it's
>  probably easier to just drop it:
> 
>  $ qemu-system-x86_64 -drive file=null-co://,id=d0,if=none \
>  -device virtio-blk-pci,drive=d0,x-data-plane=on
> 
>  ERROR:/home/fam/work/qemu/qom/object.c:1515:
>  object_get_canonical_path_component: assertion failed: (obj->parent != 
>  NULL)
>  Aborted
> >>>
> >>> Do we understand yet why this crashes, btw?
> >>
> >> I think it's because with x-data-plane=on, virtio-blk initialize an object 
> >> that
> >> doesn't have a parent, therefore it doesn't have a valid "canonical path
> >> component" thing, which is different from objects created with "-object" 
> >> CLI.
> >> I'm not very familiar with the QOM semantics here.
> >>
> >>>
> 
>  Signed-off-by: Fam Zheng 
>  ---
>   hw/block/dataplane/virtio-blk.c | 15 ++-
>   hw/block/virtio-blk.c   |  1 -
>   include/hw/virtio/virtio-blk.h  |  1 -
>   3 files changed, 2 insertions(+), 15 deletions(-)
> 
> >>>
> >>> No general objection to removing x-data-plane; but this probably wants
> >>> a mention on the changelog as x-data-plane has been described in
> >>> various howtos etc. over the years.
> >>
> >> Yes, that is a good point.  I don't know if it's too rushing in removing 
> >> it for
> >> 2.5 (this is just posted as one option) and we'll have to count on QOM 
> >> experts
> >> for the fix, if it is.
> >
> > The solution would be to add object_property_add_child to
> > virtio_blk_data_plane_create, between object_initialize and
> > user_creatable_complete.  But I think this patch is ok for 2.5.
> 
> Paolo asked me to apply this to master, so I have done so.

Okay.  I will do my best to communicate that x-data-plane is gone.

Stefan


signature.asc
Description: PGP signature