Re: [PULL 00/20] Ide patches

[Mark Cave-Ayland]

On 19/03/2020 18:02, John Snow wrote:

> On 3/19/20 8:33 AM, Peter Maydell wrote:
>> On Tue, 17 Mar 2020 at 23:23, John Snow  wrote:
>>>
>>> The following changes since commit 373c7068dd610e97f0b551b5a6d0a27cd6da4506:
>>>
>>>   qemu.nsi: Install Sphinx documentation (2020-03-09 16:45:00 +)
>>>
>>> are available in the Git repository at:
>>>
>>>   https://github.com/jnsnow/qemu.git tags/ide-pull-request
>>>
>>> for you to fetch changes up to 7d0776ca7f853d466b6174d96daa5c8afc43d1a4:
>>>
>>>   hw/ide: Remove unneeded inclusion of hw/ide.h (2020-03-17 12:22:36 -0400)
>>>
>>> 
>>> Pull request
>>>
>>> 
>>>
>>
>>
>> Applied, thanks.
>>
>> Please update the changelog at https://wiki.qemu.org/ChangeLog/5.0
>> for any user-visible changes.
>>
>> -- PMM
>>
> 
> Mark, I'm sorry to foist this on you, but would you mind updating the
> changelog?
> 
> --js

Done. I've added them under https://wiki.qemu.org/ChangeLog/5.0#Block_devices 
since
there doesn't seem to be a dedicated IDE section.


ATB,

Mark.

Re: [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()

[BALATON Zoltan]

On Mon, 23 Mar 2020, John Snow wrote:

On 3/23/20 1:04 PM, BALATON Zoltan wrote:

On Mon, 23 Mar 2020, Peter Maydell wrote:

Coverity points out (CID 1421984) that we are leaking the
memory returned by qemu_allocate_irqs(). We can avoid this
leak by switching to using qdev_init_gpio_in(); the base
class finalize will free the irqs that this allocates under
the hood.

Signed-off-by: Peter Maydell 
---
This is how the 'use qdev gpio' approach to fixing the leak looks.
Disclaimer: I have only tested this with "make check", nothing more.

hw/ide/sii3112.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 06605d7af2b..2ae6f5d9df6 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -251,8 +251,8 @@ static void sii3112_pci_realize(PCIDevice *dev,
Error **errp)
{
    SiI3112PCIState *d = SII3112_PCI(dev);
    PCIIDEState *s = PCI_IDE(dev);
+    DeviceState *ds = DEVICE(dev);
    MemoryRegion *mr;
-    qemu_irq *irq;
    int i;

    pci_config_set_interrupt_pin(dev->config, 1);
@@ -280,10 +280,10 @@ static void sii3112_pci_realize(PCIDevice *dev,
Error **errp)
    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio,
0, 16);
    pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr);

-    irq = qemu_allocate_irqs(sii3112_set_irq, d, 2);
+    qdev_init_gpio_in(ds, sii3112_set_irq, 2);
    for (i = 0; i < 2; i++) {
    ide_bus_new(&s->bus[i], sizeof(s->bus[i]), DEVICE(dev), i, 1);
-    ide_init2(&s->bus[i], irq[i]);
+    ide_init2(&s->bus[i], qdev_get_gpio_in(ds, i));


Maybe we could just use DEVICE(dev) here and above as well just like in
ide_bus_new above just to keep it consistent and avoid the confusion
caused by having dev, d, s and now also ds. DEVICE(dev) is short and
clear enough I think.

Regards,
BALATON Zoltan



Reviewed-by: John Snow 


The named temporary is fine. We probably should be using a named
temporary in the other locations, too.


Yes that's fine too if using cast more than once is less efficient. Could 
you change the existing DEVICE(dev) also to ds when applying please? 
Probably no need for a v2 for that.



I will run my usual tests, but admit I don't really test the non-x86
boards directly. Do you want to give a tested-by on this, if it matters
to you? Otherwise, I'm fairly content to trust Peter's judgment here.


I've tried that AmigaOS still boots on sam460ex so:

Tested-by: BALATON Zoltan 

The sii3112 should also work on x86 or other platforms with Linux's 
sata_sil driver but only as a 2nd non-bootable controller because we don't 
have option ROM and BIOS probably has no driver. Apart from that it's a 
common PCI SATA controller so likely a lot of guests have driver for it.


Regards,
BALATON Zoltan

[PATCH] qcow2: Forbid discard in qcow2 v2 images with backing files

[Alberto Garcia]

A discard request deallocates the selected clusters so they read back
as zeroes. This is done by clearing the cluster offset field and
setting QCOW_OFLAG_ZERO in the L2 entry.

This flag is however only supported when qcow_version >= 3. In older
images the cluster is simply deallocated, exposing any possible stale
data from the backing file.

Since discard is an advisory operation it's safer to simply forbid it
in this scenario.

Note that we are adding this check to qcow2_co_pdiscard() and not to
qcow2_cluster_discard() or discard_in_l2_slice() because the last
two are also used by qcow2_snapshot_create() to discard the clusters
used by the VM state. In this case there's no risk of exposing stale
data to the guest and we really want that the clusters are always
discarded.

Signed-off-by: Alberto Garcia 
---
 block/qcow2.c  |  6 +++
 tests/qemu-iotests/060 |  5 ++-
 tests/qemu-iotests/060.out |  2 -
 tests/qemu-iotests/289 | 90 ++
 tests/qemu-iotests/289.out | 52 ++
 tests/qemu-iotests/group   |  1 +
 6 files changed, 152 insertions(+), 4 deletions(-)
 create mode 100755 tests/qemu-iotests/289
 create mode 100644 tests/qemu-iotests/289.out

diff --git a/block/qcow2.c b/block/qcow2.c
index d44b45633d..7bb7e392e1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3763,6 +3763,12 @@ static coroutine_fn int 
qcow2_co_pdiscard(BlockDriverState *bs,
 int ret;
 BDRVQcow2State *s = bs->opaque;
 
+/* If the image does not support QCOW_OFLAG_ZERO then discarding
+ * clusters could expose stale data from the backing file. */
+if (s->qcow_version < 3 && bs->backing) {
+return -ENOTSUP;
+}
+
 if (!QEMU_IS_ALIGNED(offset | bytes, s->cluster_size)) {
 assert(bytes < s->cluster_size);
 /* Ignore partial clusters, except for the special case of the
diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index 043f12904a..4a4fdfb1e1 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -167,9 +167,10 @@ _make_test_img -o 'compat=0.10' -b "$BACKING_IMG" 1G
 # Write two clusters, the second one enforces creation of an L2 table after
 # the first data cluster.
 $QEMU_IO -c 'write 0k 64k' -c 'write 512M 64k' "$TEST_IMG" | _filter_qemu_io
-# Discard the first cluster. This cluster will soon enough be reallocated and
+# Free the first cluster. This cluster will soon enough be reallocated and
 # used for COW.
-$QEMU_IO -c 'discard 0k 64k' "$TEST_IMG" | _filter_qemu_io
+poke_file "$TEST_IMG" '262144' "\x00\x00\x00\x00\x00\x00\x00\x00" # 0x4 - 
L2 entry
+poke_file "$TEST_IMG" '131082' "\x00\x00" # 0x2000a - Refcount entry
 # Now, corrupt the image by marking the second L2 table cluster as free.
 poke_file "$TEST_IMG" '131084' "\x00\x00" # 0x2000c
 # Start a write operation requiring COW on the image stopping it right before
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index d27692a33c..09caaea865 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -105,8 +105,6 @@ wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote 65536/65536 bytes at offset 536870912
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-discard 65536/65536 bytes at offset 0
-64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 qcow2: Marking image as corrupt: Preventing invalid write on metadata 
(overlaps with active L2 table); further corruption events will be suppressed
 blkdebug: Suspended request '0'
 write failed: Input/output error
diff --git a/tests/qemu-iotests/289 b/tests/qemu-iotests/289
new file mode 100755
index 00..13b4984721
--- /dev/null
+++ b/tests/qemu-iotests/289
@@ -0,0 +1,90 @@
+#!/usr/bin/env bash
+#
+# Test how 'qemu-io -c discard' behaves on v2 and v3 qcow2 images
+#
+# Copyright (C) 2020 Igalia, S.L.
+# Author: Alberto Garcia 
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=be...@igalia.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1# failure is the default!
+
+_cleanup()
+{
+_cleanup_test_img
+rm -f "$TEST_IMG.backing"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+echo
+echo "### Te

[PULL for-5.0 1/1] aio-posix: fix io_uring with external events

[Stefan Hajnoczi]

When external event sources are disabled fdmon-io_uring falls back to
fdmon-poll.  The ->need_wait() callback needs to watch for this so it
can return true when external event sources are disabled.

It is also necessary to call ->wait() when AioHandlers have changed
because io_uring is asynchronous and we must submit new sqes.

Both of these changes to ->need_wait() together fix tests/test-aio -p
/aio/external-client, which failed with:

  test-aio: tests/test-aio.c:404: test_aio_external_client: Assertion 
`aio_poll(ctx, false)' failed.

Reported-by: Julia Suvorova 
Signed-off-by: Stefan Hajnoczi 
Reviewed-by: Stefano Garzarella 
Message-id: 20200319163559.117903-1-stefa...@redhat.com
Signed-off-by: Stefan Hajnoczi 
---
 util/fdmon-io_uring.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/util/fdmon-io_uring.c b/util/fdmon-io_uring.c
index 893b79b622..7e143ef515 100644
--- a/util/fdmon-io_uring.c
+++ b/util/fdmon-io_uring.c
@@ -290,7 +290,18 @@ static int fdmon_io_uring_wait(AioContext *ctx, 
AioHandlerList *ready_list,
 
 static bool fdmon_io_uring_need_wait(AioContext *ctx)
 {
-return io_uring_cq_ready(&ctx->fdmon_io_uring);
+/* Have io_uring events completed? */
+if (io_uring_cq_ready(&ctx->fdmon_io_uring)) {
+return true;
+}
+
+/* Do we need to submit new io_uring sqes? */
+if (!QSLIST_EMPTY_RCU(&ctx->submit_list)) {
+return true;
+}
+
+/* Are we falling back to fdmon-poll? */
+return atomic_read(&ctx->external_disable_cnt);
 }
 
 static const FDMonOps fdmon_io_uring_ops = {
-- 
2.24.1

[PULL for-5.0 0/1] Block patches

[Stefan Hajnoczi]

The following changes since commit 29e0855c5af62bbb0b0b6fed792e004dad92ba95:

  Merge remote-tracking branch 'remotes/elmarco/tags/slirp-pull-request' into 
staging (2020-03-22 21:00:38 +)

are available in the Git repository at:

  https://github.com/stefanha/qemu.git tags/block-pull-request

for you to fetch changes up to ff807d559205a434fd37d3343f01a0ab128bd190:

  aio-posix: fix io_uring with external events (2020-03-23 11:05:44 +)


Pull request



Stefan Hajnoczi (1):
  aio-posix: fix io_uring with external events

 util/fdmon-io_uring.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

-- 
2.24.1

Re: [PATCH] ide/sii3112: Avoid leaking irqs array

[John Snow]

On 3/23/20 10:32 AM, BALATON Zoltan wrote:
> Coverity CID 1421984 reports a leak in allocated irqs, this patch
> attempts to plug that.
> 
> Signed-off-by: BALATON Zoltan 

Dequeueing in favor of Peter Maydell's patch.

(Let me know if you feel that is a mistake.)

Re: [PATCH-for-5.0 v2 05/11] hw/ide/sii3112: Remove dead assignment

[John Snow]

On 3/21/20 10:41 AM, Philippe Mathieu-Daudé wrote:
> Fix warning reported by Clang static code analyzer:
> 
> CC  hw/ide/sii3112.o
>   hw/ide/sii3112.c:204:9: warning: Value stored to 'val' is never read
>   val = 0;
>   ^ ~
> 
> Fixes: a9dd6604
> Reported-by: Clang Static Analyzer
> Reviewed-by: BALATON Zoltan 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> v2: Fix the correct function (Aleksandar review)
> ---
>  hw/ide/sii3112.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
> index 06605d7af2..b2ff6dd6d9 100644
> --- a/hw/ide/sii3112.c
> +++ b/hw/ide/sii3112.c
> @@ -42,7 +42,7 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr,
>  unsigned int size)
>  {
>  SiI3112PCIState *d = opaque;
> -uint64_t val = 0;
> +uint64_t val;
>  
>  switch (addr) {
>  case 0x00:
> @@ -126,6 +126,7 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr 
> addr,
>  break;
>  default:
>  val = 0;
> +break;
>  }
>  trace_sii3112_read(size, addr, val);
>  return val;
> @@ -201,7 +202,7 @@ static void sii3112_reg_write(void *opaque, hwaddr addr,
>  d->regs[1].sien = (val >> 16) & 0x3eed;
>  break;
>  default:
> -val = 0;
> +break;
>  }
>  }
>  
> 

Acked-by: John Snow 

Re: [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()

[John Snow]

On 3/23/20 1:04 PM, BALATON Zoltan wrote:
> On Mon, 23 Mar 2020, Peter Maydell wrote:
>> Coverity points out (CID 1421984) that we are leaking the
>> memory returned by qemu_allocate_irqs(). We can avoid this
>> leak by switching to using qdev_init_gpio_in(); the base
>> class finalize will free the irqs that this allocates under
>> the hood.
>>
>> Signed-off-by: Peter Maydell 
>> ---
>> This is how the 'use qdev gpio' approach to fixing the leak looks.
>> Disclaimer: I have only tested this with "make check", nothing more.
>>
>> hw/ide/sii3112.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
>> index 06605d7af2b..2ae6f5d9df6 100644
>> --- a/hw/ide/sii3112.c
>> +++ b/hw/ide/sii3112.c
>> @@ -251,8 +251,8 @@ static void sii3112_pci_realize(PCIDevice *dev,
>> Error **errp)
>> {
>>     SiI3112PCIState *d = SII3112_PCI(dev);
>>     PCIIDEState *s = PCI_IDE(dev);
>> +    DeviceState *ds = DEVICE(dev);
>>     MemoryRegion *mr;
>> -    qemu_irq *irq;
>>     int i;
>>
>>     pci_config_set_interrupt_pin(dev->config, 1);
>> @@ -280,10 +280,10 @@ static void sii3112_pci_realize(PCIDevice *dev,
>> Error **errp)
>>     memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio,
>> 0, 16);
>>     pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr);
>>
>> -    irq = qemu_allocate_irqs(sii3112_set_irq, d, 2);
>> +    qdev_init_gpio_in(ds, sii3112_set_irq, 2);
>>     for (i = 0; i < 2; i++) {
>>     ide_bus_new(&s->bus[i], sizeof(s->bus[i]), DEVICE(dev), i, 1);
>> -    ide_init2(&s->bus[i], irq[i]);
>> +    ide_init2(&s->bus[i], qdev_get_gpio_in(ds, i));
> 
> Maybe we could just use DEVICE(dev) here and above as well just like in
> ide_bus_new above just to keep it consistent and avoid the confusion
> caused by having dev, d, s and now also ds. DEVICE(dev) is short and
> clear enough I think.
> 
> Regards,
> BALATON Zoltan
> 

Reviewed-by: John Snow 


The named temporary is fine. We probably should be using a named
temporary in the other locations, too.

I will run my usual tests, but admit I don't really test the non-x86
boards directly. Do you want to give a tested-by on this, if it matters
to you? Otherwise, I'm fairly content to trust Peter's judgment here.

--js

How do I use patchew to be a good block-devel citizen? (was: Re: [PATCH] iotests/026: Move v3-exclusive test to new file)

[John Snow]

On 3/23/20 8:23 AM, Max Reitz wrote:
> On 12.03.20 23:19, John Snow wrote:
>>
>>
>> On 3/11/20 10:07 AM, Max Reitz wrote:
>>> data_file does not work with v2, and we probably want 026 to keep
>>> working for v2 images.  Thus, open a new file for v3-exclusive error
>>> path test cases.
>>>
>>> Fixes: 81311255f217859413c94f2cd9cebf2684bbda94
>>>(“iotests/026: Test EIO on allocation in a data-file”)
>>> Signed-off-by: Max Reitz 
>>
>> Let me start this reply with something good, or at least something
>> that's not bad. It's value neutral at worst.
>>
>> Reviewed-by: John Snow 
>> Tested-by: John Snow 
> 
> Thanks. :)
> 
>> Now, let's get cracking on some prime nonsense.
>>
>> I assume this patch is still 'pending'. Here's a complete tangent
>> unrelated to your patch in every single way:
> 
> Reasonable, but it’s a bit of a shame you bury it here.  I feel like
> this makes it unlikely to reach the people you want to reach.
> 

I'm not sure if I was trying to reach anyone, exactly, but maybe I
should have been. Let's recirculate this to a wider audience.

So, directly below, find the $subject question:

>> What's the best way to use patchew to see series that are "pending" in
>> some way? I'd like to:
>>
>> - Search only the block list (to:qemu-block@nongnu.org. I assume this
>> catches CCs too.)
>> - Exclude series that are merged (-is:merged)
>> - Exclude obsoleted series (-is:obsolete)
>>
>> This gets a bit closer to things that are interesting in some way --
>> give or take some fuzziness with patchew's detection of "merged" or
>> "obsoleted" sometimes.
>>
>> - Exclude pull requests. (-is:pull seems broken, actually.)
>> - Exclude reviewed series (-is:reviewed -- what does patchew consider
>> 'reviewed'? does this mean fully reviewed, or any reviews?)
>>
>> This gives me something a bit more useful.
>>
>> - Exclude 'expired' series. I use 30 days as a mental model for this. It
>> might be nice to formalize this and mark patches that received no
>> replies and didn't detect any other state change as "expired" and send
>> an autoreply from the bot.
>>
>> (I.e., patches that are complete, applied, passed CI, were not
>> obsoleted, did not appear to be merged, and received no replies from
>> anyone except the patch author)
>>
>>
>> ("Hi, this patch received no replies from anyone except the author (you)
>> for 30 days. The series is being dropped from the pending queue and is
>> being marked expired. If the patches are still important, please rebase
>> them and re-send to the list.
>>
>> Please use scripts/get_maintainers.pl to identify candidate maintainers
>> and reviewers and make sure they are CC'd.
>>
>> This series appears to touch files owned by the following maintainers:
>> - Blah
>> - Etc
>> - And so on
>>
>> For more information on the contribution process, please visit:
>> ")
>>
>> We don't have anything like that, so age:<30d suffices. Alright, this
>> list is starting to look *pretty* decent.
>>
>> project:QEMU to:qemu-block@nongnu.org not:obsolete not:merged
>> -is:reviewed age:<30d
>>
>> Lastly, maybe we can exclude series that don't have replies yet.
> 
> Maybe Patchew should ping these after 14 days or so.
> 
> That’s about the only thing I can contribute, because I don’t really use
> Patchew myself...  I keep patches in a subfolder of my inbox on unread;
> and I keep my own pending series in a separate folder.  (Before I did
> that, I was much more prone to forgetting my own patches rather than
> someone else’s.)
> 
> Max
> 
>> It's
>> not clear to patchew which replies are:
>>
>> - Unrelated comments, like this one here
>> - Requests for a change
>> - A question for the submitter
>> - A softly-worded N-A-C-K
>>
>> and without a concept of designated reviewer, perhaps lack of replies is
>> good evidence that the series is untouched and needs someone to 'pick it
>> up'; (-has:replies)
>>
>> https://patchew.org/search?q=project%3AQEMU+to%3Aqemu-block%40nongnu.org+not%3Aobsolete+not%3Amerged+-is%3Areviewed+age%3A%3C30d+-has%3Areplies
>>
>> Alright, that's pretty good, actually.
>>
>> OK, yes, this patch still needs love as far as patchew understands.
>>

[Snip: no longer relevant to the topic.]

Re: [PATCH 3/3] iotests: Increase pause_wait() timeout

[John Snow]

On 3/20/20 6:21 AM, Philippe Mathieu-Daudé wrote:
> On 3/13/20 9:36 AM, Kevin Wolf wrote:
>> Waiting for only 1 second proved to be too short on a loaded system,
>> resulting in false positives when testing pull requests. Increase the
>> timeout a bit to make this less likely.
>>
>> Signed-off-by: Kevin Wolf 
>> ---
>>   tests/qemu-iotests/iotests.py | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/qemu-iotests/iotests.py
>> b/tests/qemu-iotests/iotests.py
>> index b859c303a2..7bc4934cd2 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -925,7 +925,7 @@ class QMPTestCase(unittest.TestCase):
>>   self.assert_qmp(event, 'data/type', 'mirror')
>>     def pause_wait(self, job_id='job0'):
>> -    with Timeout(1, "Timeout waiting for job to pause"):
>> +    with Timeout(3, "Timeout waiting for job to pause"):
>>   while True:
>>   result = self.vm.qmp('query-block-jobs')
>>   found = False
>>
> 
> I wonder if this might be more accurate:
> 
>   load_timeout = math.ceil(os.getloadavg()[0])
>   with Timeout(1 + load_timeout, "Timeout waiting for job to pause"):
>     ...
> 
> Anyhow:
> Reviewed-by: Philippe Mathieu-Daudé 
> 

They're just tests ... and this is just a worst-case timeout. I don't
think we need to get too cute with it. This only affects cases where the
test (or the binary) is broken and we have to wait without getting a
response for 3 seconds before being able to declare that the test is
indeed broken.

Optimizing this waiting time is probably not helpful; as when the test
is passing we'll never see this delay.

--js

Re: [PATCH v5 3/4] qmp: Move dispatcher to a coroutine

[Marc-André Lureau]

Hi

On Mon, Mar 23, 2020 at 6:41 PM Kevin Wolf  wrote:
>
> Am 18.03.2020 um 16:36 hat Markus Armbruster geschrieben:
> > Kevin Wolf  writes:
> >
> > > This moves the QMP dispatcher to a coroutine and runs all QMP command
> > > handlers that declare 'coroutine': true in coroutine context so they
> > > can avoid blocking the main loop while doing I/O or waiting for other
> > > events.
> > >
> > > For commands that are not declared safe to run in a coroutine, the
> > > dispatcher drops out of coroutine context by calling the QMP command
> > > handler from a bottom half.
> > >
> > > Signed-off-by: Kevin Wolf 
> >
> > Uh, what about @cur_mon?
> >
> > @cur_mon points to the current monitor while a command executes.
> > Initial value is null.  It is set in three places (not counting unit
> > tests), and all three save, set, do something that may use @cur_mon,
> > restore:
> >
> > * monitor_qmp_dispatch(), for use within qmp_dispatch()
> > * monitor_read(), for use within handle_hmp_command()
> > * qmp_human_monitor_command(), also for use within handle_hmp_command()
> >
> > Therefore, @cur_mon is null unless we're running within qmp_dispatch()
> > or handle_hmp_command().
>
> Can we make it NULL for coroutine-enabled handlers?

fwiw, I think /dev/fdset doesn't care about cur_mon. However, qmp
handlers that use monitor_get_fd() usually depend on cur_mon.

Note: I wonder if add-fd (fdsets) and getfd (monitor fds) deserve to co-exist.

>
> > Example of use: error_report() & friends print "to current monitor if we
> > have one, else to stderr."  Makes sharing code between HMP and CLI
> > easier.  Uses @cur_mon under the hood.
>
> error_report() eventually prints to stderr both for cur_mon == NULL and
> for QMP monitors. Is there an important difference between both cases?
>
> There is error_vprintf_unless_qmp(), which behaves differently for both
> cases. However, it is only used in VNC code, so that code would have to
> keep coroutines disabled.
>
> Is cur_mon used much in other functions called by QMP handlers?
>
> > @cur_mon is thread-local.
> >
> > I'm afraid we have to save, clear and restore @cur_mon around a yield.
>
> That sounds painful enough that I'd rather avoid it.
>
> Kevin
>


-- 
Marc-André Lureau

block stream and bitmaps

[John Snow]

Hi Kevin,

I'm hoping to get some preliminary ideas from you (capped at five
minutes' effort?) on this subject. My ideas here are a bit shaky, I only
have really rough notions here.

We want to use bitmaps with 'drive' semantics; i.e. tracking changes to
the visible guest data. What we have are bitmaps with node semantics,
tracking changes to the data at a particular level in the graph.

For commit, this isn't a big deal: we can disable bitmaps in the backing
file while we commit and then re-enable it on completion. We usually
have a separate bitmap enabled on the root node that is recording writes
during this time, and can be moved later.

For streaming, this is more challenging: new writes will dirty the
bitmap, but so will writes from the stream job itself.

Semantically, we want to ignore writes from the stream while recording
them from the guest -- differentiating based on source.

Bitmaps aren't really geared to do that right now. With the changes to
Bdrv Roles that Max was engineering, do you think it's possible to add
some kind of write source discrimination to bitmaps, or is that too messy?

For both commit and stream, it might be nice to say: "This bitmap is
enabled, but ignores writes from [all? specific types? specific
instances?] jobs.

Or, I wonder if what we truly want is some kind of bitmap "forwarder"
object on block-backend objects that represent the semantic drive view,
and only writes through that *backend* get forwarded to the bitmaps
attached to whatever node the bitmap is actually associated with.

(That might wind up causing weird problems too, though... since those
objects are no longer intended to be user-addressable, managing that
configuration might get intensely strange.)

Let me know if you have any quick thoughts.

--js

Re: [PATCH v5 3/4] qmp: Move dispatcher to a coroutine

[Kevin Wolf]

Am 18.03.2020 um 16:36 hat Markus Armbruster geschrieben:
> Kevin Wolf  writes:
> 
> > This moves the QMP dispatcher to a coroutine and runs all QMP command
> > handlers that declare 'coroutine': true in coroutine context so they
> > can avoid blocking the main loop while doing I/O or waiting for other
> > events.
> >
> > For commands that are not declared safe to run in a coroutine, the
> > dispatcher drops out of coroutine context by calling the QMP command
> > handler from a bottom half.
> >
> > Signed-off-by: Kevin Wolf 
> 
> Uh, what about @cur_mon?
> 
> @cur_mon points to the current monitor while a command executes.
> Initial value is null.  It is set in three places (not counting unit
> tests), and all three save, set, do something that may use @cur_mon,
> restore:
> 
> * monitor_qmp_dispatch(), for use within qmp_dispatch()
> * monitor_read(), for use within handle_hmp_command()
> * qmp_human_monitor_command(), also for use within handle_hmp_command()
> 
> Therefore, @cur_mon is null unless we're running within qmp_dispatch()
> or handle_hmp_command().

Can we make it NULL for coroutine-enabled handlers?

> Example of use: error_report() & friends print "to current monitor if we
> have one, else to stderr."  Makes sharing code between HMP and CLI
> easier.  Uses @cur_mon under the hood.

error_report() eventually prints to stderr both for cur_mon == NULL and
for QMP monitors. Is there an important difference between both cases?

There is error_vprintf_unless_qmp(), which behaves differently for both
cases. However, it is only used in VNC code, so that code would have to
keep coroutines disabled.

Is cur_mon used much in other functions called by QMP handlers?

> @cur_mon is thread-local.
> 
> I'm afraid we have to save, clear and restore @cur_mon around a yield.

That sounds painful enough that I'd rather avoid it.

Kevin

Re: [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()

[Peter Maydell]

On Mon, 23 Mar 2020 at 17:04, BALATON Zoltan  wrote:
>
> On Mon, 23 Mar 2020, Peter Maydell wrote:
> > Coverity points out (CID 1421984) that we are leaking the
> > memory returned by qemu_allocate_irqs(). We can avoid this
> > leak by switching to using qdev_init_gpio_in(); the base
> > class finalize will free the irqs that this allocates under
> > the hood.
> >
> > Signed-off-by: Peter Maydell 
> > ---
> > This is how the 'use qdev gpio' approach to fixing the leak looks.
> > Disclaimer: I have only tested this with "make check", nothing more.
> >
> > hw/ide/sii3112.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
> > index 06605d7af2b..2ae6f5d9df6 100644
> > --- a/hw/ide/sii3112.c
> > +++ b/hw/ide/sii3112.c
> > @@ -251,8 +251,8 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
> > **errp)
> > {
> > SiI3112PCIState *d = SII3112_PCI(dev);
> > PCIIDEState *s = PCI_IDE(dev);
> > +DeviceState *ds = DEVICE(dev);
> > MemoryRegion *mr;
> > -qemu_irq *irq;
> > int i;
> >
> > pci_config_set_interrupt_pin(dev->config, 1);
> > @@ -280,10 +280,10 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
> > **errp)
> > memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio, 0, 
> > 16);
> > pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr);
> >
> > -irq = qemu_allocate_irqs(sii3112_set_irq, d, 2);
> > +qdev_init_gpio_in(ds, sii3112_set_irq, 2);
> > for (i = 0; i < 2; i++) {
> > ide_bus_new(&s->bus[i], sizeof(s->bus[i]), DEVICE(dev), i, 1);
> > -ide_init2(&s->bus[i], irq[i]);
> > +ide_init2(&s->bus[i], qdev_get_gpio_in(ds, i));
>
> Maybe we could just use DEVICE(dev) here and above as well just like in
> ide_bus_new above just to keep it consistent and avoid the confusion
> caused by having dev, d, s and now also ds. DEVICE(dev) is short and clear
> enough I think.

Usual style in these methods is to have local variables if
it's going to be used more than once or twice -- QOM casts
aren't free the way C casts are. We already do that here for
the SII3112_PCI() and the PCI_IDE().

In this case things are a bit confused by the function having
used "dev" for the PCIDevice* and 's' for the PCIIDEState.
QEMU is far from consistent in this matter, but usually 's'
is the pointer to the device's own state (ie SiI3112PCIState*
in this case) and the DeviceState* is 'dev'. The PCIDevice *
is often 'pci_dev'. I would call the PCIIDEState* 'pci_ide'.

I hadn't noticed the use of DEVICE() in ide_bus_new(); you're
right that we should be consistent about whether we use the cast
macro or the local variable.

thanks
-- PMM

Re: [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()

[BALATON Zoltan]

On Mon, 23 Mar 2020, Peter Maydell wrote:

Coverity points out (CID 1421984) that we are leaking the
memory returned by qemu_allocate_irqs(). We can avoid this
leak by switching to using qdev_init_gpio_in(); the base
class finalize will free the irqs that this allocates under
the hood.

Signed-off-by: Peter Maydell 
---
This is how the 'use qdev gpio' approach to fixing the leak looks.
Disclaimer: I have only tested this with "make check", nothing more.

hw/ide/sii3112.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 06605d7af2b..2ae6f5d9df6 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -251,8 +251,8 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
{
SiI3112PCIState *d = SII3112_PCI(dev);
PCIIDEState *s = PCI_IDE(dev);
+DeviceState *ds = DEVICE(dev);
MemoryRegion *mr;
-qemu_irq *irq;
int i;

pci_config_set_interrupt_pin(dev->config, 1);
@@ -280,10 +280,10 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio, 0, 16);
pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr);

-irq = qemu_allocate_irqs(sii3112_set_irq, d, 2);
+qdev_init_gpio_in(ds, sii3112_set_irq, 2);
for (i = 0; i < 2; i++) {
ide_bus_new(&s->bus[i], sizeof(s->bus[i]), DEVICE(dev), i, 1);
-ide_init2(&s->bus[i], irq[i]);
+ide_init2(&s->bus[i], qdev_get_gpio_in(ds, i));


Maybe we could just use DEVICE(dev) here and above as well just like in 
ide_bus_new above just to keep it consistent and avoid the confusion 
caused by having dev, d, s and now also ds. DEVICE(dev) is short and clear 
enough I think.


Regards,
BALATON Zoltan



bmdma_init(&s->bus[i], &s->bmdma[i], s);
s->bmdma[i].bus = &s->bus[i];

Re: [PATCH-for-5.0 v2 10/11] hw/timer/pxa2xx_timer: Add assertion to silent static analyzer warning

[Alistair Francis]

On Sat, Mar 21, 2020 at 7:50 AM Philippe Mathieu-Daudé
 wrote:
>
> pxa2xx_timer_tick4() takes an opaque pointer, then calls
> pxa2xx_timer_update4(), so the static analyzer can not
> verify that the 'n < 8':
>
>   425 static void pxa2xx_timer_tick4(void *opaque)
>   426 {
>   427 PXA2xxTimer4 *t = (PXA2xxTimer4 *) opaque;
>   428 PXA2xxTimerInfo *i = (PXA2xxTimerInfo *) t->tm.info;
>   429
>   430 pxa2xx_timer_tick(&t->tm);
>   433 if (t->control & (1 << 6))
>   434 pxa2xx_timer_update4(i, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), 
> t->tm.num - 4);
>
>   135 static void pxa2xx_timer_update4(void *opaque, uint64_t now_qemu, int n)
>   136 {
>   137 PXA2xxTimerInfo *s = (PXA2xxTimerInfo *) opaque;
>   140 static const int counters[8] = { 0, 0, 0, 0, 4, 4, 6, 6 };
>   142
>   143 if (s->tm4[n].control & (1 << 7))
>   144 counter = n;
>   145 else
>   146 counter = counters[n];
>
> Add an assert() to give the static analyzer a hint, this fixes a
> warning reported by Clang static code analyzer:
>
> CC  hw/timer/pxa2xx_timer.o
>   hw/timer/pxa2xx_timer.c:146:17: warning: Assigned value is garbage or 
> undefined
>   counter = counters[n];
>   ^ ~~~
>
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  hw/timer/pxa2xx_timer.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/timer/pxa2xx_timer.c b/hw/timer/pxa2xx_timer.c
> index cd172cc1e9..944c165889 100644
> --- a/hw/timer/pxa2xx_timer.c
> +++ b/hw/timer/pxa2xx_timer.c
> @@ -140,6 +140,7 @@ static void pxa2xx_timer_update4(void *opaque, uint64_t 
> now_qemu, int n)
>  static const int counters[8] = { 0, 0, 0, 0, 4, 4, 6, 6 };
>  int counter;
>
> +assert(n < ARRAY_SIZE(counters));
>  if (s->tm4[n].control & (1 << 7))
>  counter = n;
>  else
> --
> 2.21.1
>
>

Re: [PATCH-for-5.0 v2 09/11] hw/timer/stm32f2xx_timer: Remove dead assignment

[Alistair Francis]

On Sat, Mar 21, 2020 at 7:52 AM Philippe Mathieu-Daudé
 wrote:
>
> Fix warning reported by Clang static code analyzer:
>
> CC  hw/timer/stm32f2xx_timer.o
>   hw/timer/stm32f2xx_timer.c:225:9: warning: Value stored to 'value' is never 
> read
>   value = timer_val;
>   ^   ~
>
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  hw/timer/stm32f2xx_timer.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/hw/timer/stm32f2xx_timer.c b/hw/timer/stm32f2xx_timer.c
> index 06ec8a02c2..ba8694dcd3 100644
> --- a/hw/timer/stm32f2xx_timer.c
> +++ b/hw/timer/stm32f2xx_timer.c
> @@ -222,7 +222,6 @@ static void stm32f2xx_timer_write(void *opaque, hwaddr 
> offset,
>  case TIM_PSC:
>  timer_val = stm32f2xx_ns_to_ticks(s, now) - s->tick_offset;
>  s->tim_psc = value & 0x;
> -value = timer_val;
>  break;
>  case TIM_CNT:
>  timer_val = value;
> --
> 2.21.1
>
>

Re: [PATCH-for-5.0 v2 08/11] hw/timer/exynos4210_mct: Remove dead assignments

[Alistair Francis]

On Sat, Mar 21, 2020 at 7:50 AM Philippe Mathieu-Daudé
 wrote:
>
> Fix warnings reported by Clang static code analyzer:
>
>   hw/timer/exynos4210_mct.c:1370:9: warning: Value stored to 'index' is never 
> read
> index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
> ^   ~
>   hw/timer/exynos4210_mct.c:1399:9: warning: Value stored to 'index' is never 
> read
> index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
> ^   ~
>   hw/timer/exynos4210_mct.c:1441:9: warning: Value stored to 'index' is never 
> read
> index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
> ^   ~
>
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  hw/timer/exynos4210_mct.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
> index 944120aea5..c0a25e71ec 100644
> --- a/hw/timer/exynos4210_mct.c
> +++ b/hw/timer/exynos4210_mct.c
> @@ -1367,7 +1367,6 @@ static void exynos4210_mct_write(void *opaque, hwaddr 
> offset,
>
>  case L0_TCNTB: case L1_TCNTB:
>  lt_i = GET_L_TIMER_IDX(offset);
> -index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
>
>  /*
>   * TCNTB is updated to internal register only after CNT expired.
> @@ -1396,7 +1395,6 @@ static void exynos4210_mct_write(void *opaque, hwaddr 
> offset,
>
>  case L0_ICNTB: case L1_ICNTB:
>  lt_i = GET_L_TIMER_IDX(offset);
> -index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
>
>  s->l_timer[lt_i].reg.wstat |= L_WSTAT_ICNTB_WRITE;
>  s->l_timer[lt_i].reg.cnt[L_REG_CNT_ICNTB] = value &
> @@ -1438,7 +1436,6 @@ static void exynos4210_mct_write(void *opaque, hwaddr 
> offset,
>
>  case L0_FRCNTB: case L1_FRCNTB:
>  lt_i = GET_L_TIMER_IDX(offset);
> -index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
>
>  DPRINTF("local timer[%d] FRCNTB write %llx\n", lt_i, value);
>
> --
> 2.21.1
>
>

Re: [PATCH v9 1/4] qcow2: introduce compression type feature

[Vladimir Sementsov-Ogievskiy]

23.03.2020 17:25, Denis Plotnikov wrote:

The patch adds some preparation parts for incompatible compression type
feature to qcow2 allowing the use different compression methods for
image clusters (de)compressing.

It is implied that the compression type is set on the image creation and
can be changed only later by image conversion, thus compression type
defines the only compression algorithm used for the image, and thus,
for all image clusters.

The goal of the feature is to add support of other compression methods
to qcow2. For example, ZSTD which is more effective on compression than ZLIB.

The default compression is ZLIB. Images created with ZLIB compression type
are backward compatible with older qemu versions.

Adding of the compression type breaks a number of tests because now the
compression type is reported on image creation and there are some changes
in the qcow2 header in size and offsets.

The tests are fixed in the following ways:
 * filter out compression_type for many tests
 * fix header size, feature table size and backing file offset
   affected tests: 031, 036, 061, 080
   header_size +=8: 1 byte compression type
7 bytes padding
   feature_table += 48: incompatible feature compression type
   backing_file_offset += 56 (8 + 48 -> header_change + 
feature_table_change)
 * add "compression type" for test output matching when it isn't filtered
   affected tests: 049, 060, 061, 065, 144, 182, 242, 255

Signed-off-by: Denis Plotnikov 
---
  qapi/block-core.json |  22 +-
  block/qcow2.h|  20 +-
  include/block/block_int.h|   1 +
  block/qcow2.c| 113 +++
  tests/qemu-iotests/031.out   |  14 ++--
  tests/qemu-iotests/036.out   |   4 +-
  tests/qemu-iotests/049.out   | 102 ++--
  tests/qemu-iotests/060.out   |   1 +
  tests/qemu-iotests/061.out   |  34 ++
  tests/qemu-iotests/065   |  28 +---
  tests/qemu-iotests/080   |   2 +-
  tests/qemu-iotests/144.out   |   4 +-
  tests/qemu-iotests/182.out   |   2 +-
  tests/qemu-iotests/242.out   |   5 ++
  tests/qemu-iotests/255.out   |   8 +--
  tests/qemu-iotests/common.filter |   3 +-
  16 files changed, 267 insertions(+), 96 deletions(-)




[..]


--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -146,8 +146,16 @@ typedef struct QCowHeader {
  
  uint32_t refcount_order;

  uint32_t header_length;
+
+/* Additional fields */
+uint8_t compression_type;
+
+/* header must be a multiple of 8 */
+uint8_t padding[7];
  } QEMU_PACKED QCowHeader;
  
+QEMU_BUILD_BUG_ON(sizeof(QCowHeader) % 8 != 0);


Better write

QEMU_BUILD_BUG_ON(QEMU_IS_ALIGNED(sizeof(QCowHeader), 8);

with or without this:

Reviewed-by: Vladimir Sementsov-Ogievskiy 


+
  typedef struct QEMU_PACKED QCowSnapshotHeader {
  /* header is 8 byte aligned */
  uint64_t l1_table_offset;
@@ -216,13 +224,16 @@ enum {
  QCOW2_INCOMPAT_DIRTY_BITNR  = 0,
  QCOW2_INCOMPAT_CORRUPT_BITNR= 1,
  QCOW2_INCOMPAT_DATA_FILE_BITNR  = 2,
+QCOW2_INCOMPAT_COMPRESSION_BITNR = 3,
  QCOW2_INCOMPAT_DIRTY= 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
  QCOW2_INCOMPAT_CORRUPT  = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
  QCOW2_INCOMPAT_DATA_FILE= 1 << QCOW2_INCOMPAT_DATA_FILE_BITNR,
+QCOW2_INCOMPAT_COMPRESSION  = 1 << QCOW2_INCOMPAT_COMPRESSION_BITNR,
  
  QCOW2_INCOMPAT_MASK = QCOW2_INCOMPAT_DIRTY

  | QCOW2_INCOMPAT_CORRUPT
-| QCOW2_INCOMPAT_DATA_FILE,
+| QCOW2_INCOMPAT_DATA_FILE
+| QCOW2_INCOMPAT_COMPRESSION,
  };
  
  /* Compatible feature bits */

@@ -369,6 +380,13 @@ typedef struct BDRVQcow2State {
  
  bool metadata_preallocation_checked;

  bool metadata_preallocation;
+/*
+ * Compression type used for the image. Default: 0 - ZLIB
+ * The image compression type is set on image creation.
+ * For now, the only way to change the compression type
+ * is to convert the image with the desired compression type set.
+ */
+Qcow2CompressionType compression_type;
  } BDRVQcow2State;
  
  typedef struct Qcow2COWRegion {



[..]


--
Best regards,
Vladimir

Re: [PATCH-for-5.0 v2 06/11] hw/isa/i82378: Remove dead assignment

[Alistair Francis]

On Sat, Mar 21, 2020 at 7:46 AM Philippe Mathieu-Daudé
 wrote:
>
> Rename the unique variable assigned as 'pit' which better
> represents what it holds, to fix a warning reported by the
> Clang static code analyzer:
>
> CC  hw/isa/i82378.o
>   hw/isa/i82378.c:108:5: warning: Value stored to 'isa' is never read
>   isa = isa_create_simple(isabus, "i82374");
>   ^ ~~~
>
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  hw/isa/i82378.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c
> index dcb6b479ea..d9e6c7fa00 100644
> --- a/hw/isa/i82378.c
> +++ b/hw/isa/i82378.c
> @@ -67,7 +67,7 @@ static void i82378_realize(PCIDevice *pci, Error **errp)
>  I82378State *s = I82378(dev);
>  uint8_t *pci_conf;
>  ISABus *isabus;
> -ISADevice *isa;
> +ISADevice *pit;
>
>  pci_conf = pci->config;
>  pci_set_word(pci_conf + PCI_COMMAND,
> @@ -99,13 +99,13 @@ static void i82378_realize(PCIDevice *pci, Error **errp)
>  isa_bus_irqs(isabus, s->i8259);
>
>  /* 1 82C54 (pit) */
> -isa = i8254_pit_init(isabus, 0x40, 0, NULL);
> +pit = i8254_pit_init(isabus, 0x40, 0, NULL);
>
>  /* speaker */
> -pcspk_init(isabus, isa);
> +pcspk_init(isabus, pit);
>
>  /* 2 82C37 (dma) */
> -isa = isa_create_simple(isabus, "i82374");
> +isa_create_simple(isabus, "i82374");
>  }
>
>  static void i82378_init(Object *obj)
> --
> 2.21.1
>
>

Re: [PATCH-for-5.0 v2 03/11] hw/i2c/pm_smbus: Remove dead assignment

[Alistair Francis]

On Sat, Mar 21, 2020 at 7:45 AM Philippe Mathieu-Daudé
 wrote:
>
> Fix warning reported by Clang static code analyzer:
>
> CC  hw/i2c/pm_smbus.o
>   hw/i2c/pm_smbus.c:187:17: warning: Value stored to 'ret' is never read
>   ret = 0;
>   ^ ~
>
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  hw/i2c/pm_smbus.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
> index 36994ff585..4728540c37 100644
> --- a/hw/i2c/pm_smbus.c
> +++ b/hw/i2c/pm_smbus.c
> @@ -184,7 +184,6 @@ static void smb_transaction(PMSMBus *s)
>  s->smb_stat |= STS_HOST_BUSY | STS_BYTE_DONE;
>  s->smb_data[0] = s->smb_blkdata;
>  s->smb_index = 0;
> -ret = 0;
>  }
>  goto out;
>  }
> --
> 2.21.1
>
>

Re: [PATCH-for-5.0 v2 01/11] block: Avoid dead assignment

[Alistair Francis]

On Sat, Mar 21, 2020 at 7:42 AM Philippe Mathieu-Daudé
 wrote:
>
> Fix warning reported by Clang static code analyzer:
>
>   block.c:3167:5: warning: Value stored to 'ret' is never read
>   ret = bdrv_fill_options(&options, filename, &flags, &local_err);
>   ^ ~
>
> Fixes: 462f5bcf6
> Reported-by: Clang Static Analyzer
> Suggested-by: Markus Armbruster 
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Alistair Francis 

Alistair

> ---
> v2: Keep 'ret' assigned and check it (Markus)
> ---
>  block.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block.c b/block.c
> index a2542c977b..a6f069d8bd 100644
> --- a/block.c
> +++ b/block.c
> @@ -3165,7 +3165,7 @@ static BlockDriverState *bdrv_open_inherit(const char 
> *filename,
>  }
>
>  ret = bdrv_fill_options(&options, filename, &flags, &local_err);
> -if (local_err) {
> +if (ret < 0) {
>  goto fail;
>  }
>
> --
> 2.21.1
>
>

Re: [PATCH] ide/sii3112: Avoid leaking irqs array

[BALATON Zoltan]

On Mon, 23 Mar 2020, Philippe Mathieu-Daudé wrote:

On 3/23/20 3:32 PM, BALATON Zoltan wrote:

Coverity CID 1421984 reports a leak in allocated irqs, this patch
attempts to plug that.

Signed-off-by: BALATON Zoltan 
---
  hw/ide/sii3112.c | 14 +++---
  1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 06605d7af2..c886916873 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -31,6 +31,7 @@ typedef struct SiI3112Regs {
  typedef struct SiI3112PCIState {
  PCIIDEState i;
  MemoryRegion mmio;
+qemu_irq *irqs;
  SiI3112Regs regs[2];
  } SiI3112PCIState;
  @@ -252,7 +253,6 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)

  SiI3112PCIState *d = SII3112_PCI(dev);
  PCIIDEState *s = PCI_IDE(dev);
  MemoryRegion *mr;
-qemu_irq *irq;
  int i;
pci_config_set_interrupt_pin(dev->config, 1);
@@ -280,10 +280,10 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
  memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio, 0, 
16);

  pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr);
  -irq = qemu_allocate_irqs(sii3112_set_irq, d, 2);
+d->irqs = qemu_allocate_irqs(sii3112_set_irq, d, 2);
  for (i = 0; i < 2; i++) {
  ide_bus_new(&s->bus[i], sizeof(s->bus[i]), DEVICE(dev), i, 1);
-ide_init2(&s->bus[i], irq[i]);
+ide_init2(&s->bus[i], d->irqs[i]);
bmdma_init(&s->bus[i], &s->bmdma[i], s);
  s->bmdma[i].bus = &s->bus[i];
@@ -291,6 +291,13 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)

  }
  }
  +static void sii3112_unrealize(DeviceState *dev, Error **errp)
+{
+SiI3112PCIState *d = SII3112_PCI(dev);
+
+qemu_free_irqs(d->irqs, 2);


You can't do that without calling unrealize() on all the devices in each 
IDEBus.


What? Those devices are not created in this object so whoever adds them 
later is supposed to free them before this object is unrelaized. Or is 
ownership of those devices silently passed to the the controller when 
adding devices? Anyway, Peter's patch is simpler and should also fix the 
issue so this does not matter any more (other than maybe showing we might 
also leak the devices if their ownership is not clear).


Apparently there is no code available to do that. Maybe easier to not 
add any sii3112_unrealize(). Keeping a reference in the state should be 
enough to silent Coverity.


The idea was to fix the problem not to hide it from Coverity so if it 
can't be fixed it's probably better to be reminded about it than hiding 
it.


Regards,
BALATON Zoltan

Re: Coverity CID 1421984

[Peter Maydell]

On Mon, 23 Mar 2020 at 15:28, BALATON Zoltan  wrote:
> Is this documented anywhere?

Unfortunately not. You're quite right that we should document this
(I hadn't realized/had forgotten that the qdev gpio APIs are
entirely undocumented -- they date from a time when we were
much less strict about asking for at least a doc-comment documentation
of new globally visible functions.)

thanks
-- PMM

Re: [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()

[Philippe Mathieu-Daudé]

On 3/23/20 4:17 PM, Peter Maydell wrote:

Coverity points out (CID 1421984) that we are leaking the
memory returned by qemu_allocate_irqs(). We can avoid this
leak by switching to using qdev_init_gpio_in(); the base
class finalize will free the irqs that this allocates under
the hood.

Signed-off-by: Peter Maydell 
---
This is how the 'use qdev gpio' approach to fixing the leak looks.
Disclaimer: I have only tested this with "make check", nothing more.

  hw/ide/sii3112.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 06605d7af2b..2ae6f5d9df6 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -251,8 +251,8 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
  {
  SiI3112PCIState *d = SII3112_PCI(dev);
  PCIIDEState *s = PCI_IDE(dev);
+DeviceState *ds = DEVICE(dev);
  MemoryRegion *mr;
-qemu_irq *irq;
  int i;
  
  pci_config_set_interrupt_pin(dev->config, 1);

@@ -280,10 +280,10 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
  memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio, 0, 16);
  pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr);
  
-irq = qemu_allocate_irqs(sii3112_set_irq, d, 2);

+qdev_init_gpio_in(ds, sii3112_set_irq, 2);
  for (i = 0; i < 2; i++) {
  ide_bus_new(&s->bus[i], sizeof(s->bus[i]), DEVICE(dev), i, 1);
-ide_init2(&s->bus[i], irq[i]);
+ide_init2(&s->bus[i], qdev_get_gpio_in(ds, i));
  
  bmdma_init(&s->bus[i], &s->bmdma[i], s);

  s->bmdma[i].bus = &s->bus[i];



Reviewed-by: Philippe Mathieu-Daudé 

Re: Coverity CID 1421984

[BALATON Zoltan]

On Mon, 23 Mar 2020, Peter Maydell wrote:

On Mon, 23 Mar 2020 at 14:43, BALATON Zoltan  wrote:

On Mon, 23 Mar 2020, Peter Maydell wrote:

On Mon, 23 Mar 2020 at 14:06, BALATON Zoltan  wrote:

On Mon, 23 Mar 2020, Peter Maydell wrote:

Coverity has flagged up a lot of leaks involving qemu_allocate_irqs();
most of them I've for the moment just set as "insignificant, fix
required" because they're in called-once functions like board init.
If this device can't be hot-unplugged and so we will only ever call
realize once, it would fall in that category too. Otherwise I'd
suggest conversion to qdev_init_gpio_in(). (This allocates arrays
of IRQs under the hood too, but the device_finalize() function will
automatically free them for you, so it's easier to use non-leakily.)


I think I can't do that in sii3112 becuase I need to pass irq to this func:

void ide_init2(IDEBus *bus, qemu_irq irq);


ide_init2(bus, qdev_get_gpio_in(DEVICE(dev), i);

should do what you want, I think.


I don't understand what you mean.


I mean that if you allocate the IRQs with qdev_init_gpio_in()
then the way to get a qemu_irq from within them to pass
to another function is to call qdev_get_gpio_in(). So you
just want to make your call to ide_init2() be the line I
suggest above.


Is this documented anywhere? Even after this reply I did not understand 
until I saw your patch. (And even after that I'm not quite sure when and 
how to use gpios so probably this should be made a bit more clear in 
docs or somewhere.)


Problem is that there are no complete docs so people are directed to look 
at the code. But the code is full of bad examples (like ahci and cmd646 in 
this case) so can't find out what's the latest preferred way. At least a 
list of preferred vs. legacy functions/APIs should exist somewhere and the 
preferred ones should be documented. Irq handling and gpios is one of the 
mysteries that are hard to get so some info on that would help.


Thanks for fixing this.

Regards,
BALATON Zoltan

Re: Coverity CID 1421984

[Philippe Mathieu-Daudé]

On 3/23/20 3:46 PM, Peter Maydell wrote:

On Mon, 23 Mar 2020 at 14:43, BALATON Zoltan  wrote:


On Mon, 23 Mar 2020, Peter Maydell wrote:

On Mon, 23 Mar 2020 at 14:06, BALATON Zoltan  wrote:

On Mon, 23 Mar 2020, Peter Maydell wrote:

Coverity has flagged up a lot of leaks involving qemu_allocate_irqs();
most of them I've for the moment just set as "insignificant, fix
required" because they're in called-once functions like board init.
If this device can't be hot-unplugged and so we will only ever call
realize once, it would fall in that category too. Otherwise I'd
suggest conversion to qdev_init_gpio_in(). (This allocates arrays
of IRQs under the hood too, but the device_finalize() function will
automatically free them for you, so it's easier to use non-leakily.)


I think I can't do that in sii3112 becuase I need to pass irq to this func:

void ide_init2(IDEBus *bus, qemu_irq irq);


ide_init2(bus, qdev_get_gpio_in(DEVICE(dev), i);

should do what you want, I think.


I don't understand what you mean.


I mean that if you allocate the IRQs with qdev_init_gpio_in()
then the way to get a qemu_irq from within them to pass
to another function is to call qdev_get_gpio_in(). So you
just want to make your call to ide_init2() be the line I
suggest above.


I understand Zoltan can have hard time understanding qdev_get_gpio_in() 
because it has no documentation. What would be the best example to follow?





Sent a patch that I think might fix this
warning for now. I'd leave qdevifying ide code to someone else.


There's no need to qdevify IDE for this.

thanks
-- PMM

[PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()

[Peter Maydell]

Coverity points out (CID 1421984) that we are leaking the
memory returned by qemu_allocate_irqs(). We can avoid this
leak by switching to using qdev_init_gpio_in(); the base
class finalize will free the irqs that this allocates under
the hood.

Signed-off-by: Peter Maydell 
---
This is how the 'use qdev gpio' approach to fixing the leak looks.
Disclaimer: I have only tested this with "make check", nothing more.

 hw/ide/sii3112.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 06605d7af2b..2ae6f5d9df6 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -251,8 +251,8 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
 {
 SiI3112PCIState *d = SII3112_PCI(dev);
 PCIIDEState *s = PCI_IDE(dev);
+DeviceState *ds = DEVICE(dev);
 MemoryRegion *mr;
-qemu_irq *irq;
 int i;
 
 pci_config_set_interrupt_pin(dev->config, 1);
@@ -280,10 +280,10 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
 memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio, 0, 16);
 pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr);
 
-irq = qemu_allocate_irqs(sii3112_set_irq, d, 2);
+qdev_init_gpio_in(ds, sii3112_set_irq, 2);
 for (i = 0; i < 2; i++) {
 ide_bus_new(&s->bus[i], sizeof(s->bus[i]), DEVICE(dev), i, 1);
-ide_init2(&s->bus[i], irq[i]);
+ide_init2(&s->bus[i], qdev_get_gpio_in(ds, i));
 
 bmdma_init(&s->bus[i], &s->bmdma[i], s);
 s->bmdma[i].bus = &s->bus[i];
-- 
2.20.1

Re: [PATCH-for-5.0 v2 00/11] misc: Trivial static code analyzer fixes

[Philippe Mathieu-Daudé]

On 3/23/20 3:55 PM, Laurent Vivier wrote:

Le 23/03/2020 à 15:45, Philippe Mathieu-Daudé a écrit :

On 3/23/20 3:32 PM, Laurent Vivier wrote:

Le 21/03/2020 à 15:40, Philippe Mathieu-Daudé a écrit :

Fix trivial warnings reported by the Clang static code analyzer.

Since v1:
- Addressed Markus/Zoltan/Aleksandar review comments

Philippe Mathieu-Daudé (11):
    block: Avoid dead assignment
    blockdev: Remove dead assignment
    hw/i2c/pm_smbus: Remove dead assignment
    hw/input/adb-kbd: Remove dead assignment
    hw/ide/sii3112: Remove dead assignment
    hw/isa/i82378: Remove dead assignment
    hw/gpio/aspeed_gpio: Remove dead assignment
    hw/timer/exynos4210_mct: Remove dead assignments
    hw/timer/stm32f2xx_timer: Remove dead assignment
    hw/timer/pxa2xx_timer: Add assertion to silent static analyzer
warning
    hw/scsi/esp-pci: Remove dead assignment

   block.c    | 2 +-
   blockdev.c | 2 +-
   hw/gpio/aspeed_gpio.c  | 2 +-
   hw/i2c/pm_smbus.c  | 1 -
   hw/ide/sii3112.c   | 5 +++--
   hw/input/adb-kbd.c | 6 +-
   hw/isa/i82378.c    | 8 
   hw/scsi/esp-pci.c  | 1 -
   hw/timer/exynos4210_mct.c  | 3 ---
   hw/timer/pxa2xx_timer.c    | 1 +
   hw/timer/stm32f2xx_timer.c | 1 -
   11 files changed, 12 insertions(+), 20 deletions(-)



I think your series covers cases already covered by:

[PATCH v3 00/12] redundant code: Fix warnings reported by Clang static
code analyzer
https://patchew.org/QEMU/20200302130715.29440-1-kuhn.ch


Unfortunately [for me...] I don't have v3 in my INBOX... *sigh*
This was 3 weeks ago. *sigh*.

I can see the series in the archives:
https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg00219.html
But I can't find the outcome, was it queued in the trivial tree?
Any idea when this will be merged in the master tree?


Some patches are already merged via trivial (1, 2 (should go by SCSI
queue) 3, 5, 6, 7, 9, 11 (by USB queue), 12).

But others needed R-b tags or new version. I didn't check which of your
patches are already covered by this series.

I'm sorry to not have checked your series earlier...


Don't be sorry, the problem is my INBOX that is unreliable.

I was using NNTP last month until I heard it was working properly again, 
which is not the case apparently. I'll try to find them on NNTP and 
review them.




Thanks,
Laurent

Re: [PATCH-for-5.0 v2 00/11] misc: Trivial static code analyzer fixes

[Laurent Vivier]

Le 23/03/2020 à 15:45, Philippe Mathieu-Daudé a écrit :
> On 3/23/20 3:32 PM, Laurent Vivier wrote:
>> Le 21/03/2020 à 15:40, Philippe Mathieu-Daudé a écrit :
>>> Fix trivial warnings reported by the Clang static code analyzer.
>>>
>>> Since v1:
>>> - Addressed Markus/Zoltan/Aleksandar review comments
>>>
>>> Philippe Mathieu-Daudé (11):
>>>    block: Avoid dead assignment
>>>    blockdev: Remove dead assignment
>>>    hw/i2c/pm_smbus: Remove dead assignment
>>>    hw/input/adb-kbd: Remove dead assignment
>>>    hw/ide/sii3112: Remove dead assignment
>>>    hw/isa/i82378: Remove dead assignment
>>>    hw/gpio/aspeed_gpio: Remove dead assignment
>>>    hw/timer/exynos4210_mct: Remove dead assignments
>>>    hw/timer/stm32f2xx_timer: Remove dead assignment
>>>    hw/timer/pxa2xx_timer: Add assertion to silent static analyzer
>>> warning
>>>    hw/scsi/esp-pci: Remove dead assignment
>>>
>>>   block.c    | 2 +-
>>>   blockdev.c | 2 +-
>>>   hw/gpio/aspeed_gpio.c  | 2 +-
>>>   hw/i2c/pm_smbus.c  | 1 -
>>>   hw/ide/sii3112.c   | 5 +++--
>>>   hw/input/adb-kbd.c | 6 +-
>>>   hw/isa/i82378.c    | 8 
>>>   hw/scsi/esp-pci.c  | 1 -
>>>   hw/timer/exynos4210_mct.c  | 3 ---
>>>   hw/timer/pxa2xx_timer.c    | 1 +
>>>   hw/timer/stm32f2xx_timer.c | 1 -
>>>   11 files changed, 12 insertions(+), 20 deletions(-)
>>>
>>
>> I think your series covers cases already covered by:
>>
>> [PATCH v3 00/12] redundant code: Fix warnings reported by Clang static
>> code analyzer
>> https://patchew.org/QEMU/20200302130715.29440-1-kuhn.ch
> 
> Unfortunately [for me...] I don't have v3 in my INBOX... *sigh*
> This was 3 weeks ago. *sigh*.
> 
> I can see the series in the archives:
> https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg00219.html
> But I can't find the outcome, was it queued in the trivial tree?
> Any idea when this will be merged in the master tree?

Some patches are already merged via trivial (1, 2 (should go by SCSI
queue) 3, 5, 6, 7, 9, 11 (by USB queue), 12).

But others needed R-b tags or new version. I didn't check which of your
patches are already covered by this series.

I'm sorry to not have checked your series earlier...

Thanks,
Laurent

Re: [PATCH] ide/sii3112: Avoid leaking irqs array

[Philippe Mathieu-Daudé]

On 3/23/20 3:32 PM, BALATON Zoltan wrote:

Coverity CID 1421984 reports a leak in allocated irqs, this patch
attempts to plug that.

Signed-off-by: BALATON Zoltan 
---
  hw/ide/sii3112.c | 14 +++---
  1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 06605d7af2..c886916873 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -31,6 +31,7 @@ typedef struct SiI3112Regs {
  typedef struct SiI3112PCIState {
  PCIIDEState i;
  MemoryRegion mmio;
+qemu_irq *irqs;
  SiI3112Regs regs[2];
  } SiI3112PCIState;
  
@@ -252,7 +253,6 @@ static void sii3112_pci_realize(PCIDevice *dev, Error **errp)

  SiI3112PCIState *d = SII3112_PCI(dev);
  PCIIDEState *s = PCI_IDE(dev);
  MemoryRegion *mr;
-qemu_irq *irq;
  int i;
  
  pci_config_set_interrupt_pin(dev->config, 1);

@@ -280,10 +280,10 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
  memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio, 0, 16);
  pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr);
  
-irq = qemu_allocate_irqs(sii3112_set_irq, d, 2);

+d->irqs = qemu_allocate_irqs(sii3112_set_irq, d, 2);
  for (i = 0; i < 2; i++) {
  ide_bus_new(&s->bus[i], sizeof(s->bus[i]), DEVICE(dev), i, 1);
-ide_init2(&s->bus[i], irq[i]);
+ide_init2(&s->bus[i], d->irqs[i]);
  
  bmdma_init(&s->bus[i], &s->bmdma[i], s);

  s->bmdma[i].bus = &s->bus[i];
@@ -291,6 +291,13 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
  }
  }
  
+static void sii3112_unrealize(DeviceState *dev, Error **errp)

+{
+SiI3112PCIState *d = SII3112_PCI(dev);
+
+qemu_free_irqs(d->irqs, 2);


You can't do that without calling unrealize() on all the devices in each 
IDEBus. Apparently there is no code available to do that. Maybe easier 
to not add any sii3112_unrealize(). Keeping a reference in the state 
should be enough to silent Coverity.



+}
+
  static void sii3112_pci_class_init(ObjectClass *klass, void *data)
  {
  DeviceClass *dc = DEVICE_CLASS(klass);
@@ -301,6 +308,7 @@ static void sii3112_pci_class_init(ObjectClass *klass, void 
*data)
  pd->class_id = PCI_CLASS_STORAGE_RAID;
  pd->revision = 1;
  pd->realize = sii3112_pci_realize;
+dc->unrealize = sii3112_unrealize;
  dc->reset = sii3112_reset;
  dc->desc = "SiI3112A SATA controller";
  set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);

Re: Coverity CID 1421984

[Peter Maydell]

On Mon, 23 Mar 2020 at 14:43, BALATON Zoltan  wrote:
>
> On Mon, 23 Mar 2020, Peter Maydell wrote:
> > On Mon, 23 Mar 2020 at 14:06, BALATON Zoltan  wrote:
> >> On Mon, 23 Mar 2020, Peter Maydell wrote:
> >>> Coverity has flagged up a lot of leaks involving qemu_allocate_irqs();
> >>> most of them I've for the moment just set as "insignificant, fix
> >>> required" because they're in called-once functions like board init.
> >>> If this device can't be hot-unplugged and so we will only ever call
> >>> realize once, it would fall in that category too. Otherwise I'd
> >>> suggest conversion to qdev_init_gpio_in(). (This allocates arrays
> >>> of IRQs under the hood too, but the device_finalize() function will
> >>> automatically free them for you, so it's easier to use non-leakily.)
> >>
> >> I think I can't do that in sii3112 becuase I need to pass irq to this func:
> >>
> >> void ide_init2(IDEBus *bus, qemu_irq irq);
> >
> > ide_init2(bus, qdev_get_gpio_in(DEVICE(dev), i);
> >
> > should do what you want, I think.
>
> I don't understand what you mean.

I mean that if you allocate the IRQs with qdev_init_gpio_in()
then the way to get a qemu_irq from within them to pass
to another function is to call qdev_get_gpio_in(). So you
just want to make your call to ide_init2() be the line I
suggest above.

> Sent a patch that I think might fix this
> warning for now. I'd leave qdevifying ide code to someone else.

There's no need to qdevify IDE for this.

thanks
-- PMM

Re: [PATCH-for-5.0 v2 00/11] misc: Trivial static code analyzer fixes

[Philippe Mathieu-Daudé]

On 3/23/20 3:32 PM, Laurent Vivier wrote:

Le 21/03/2020 à 15:40, Philippe Mathieu-Daudé a écrit :

Fix trivial warnings reported by the Clang static code analyzer.

Since v1:
- Addressed Markus/Zoltan/Aleksandar review comments

Philippe Mathieu-Daudé (11):
   block: Avoid dead assignment
   blockdev: Remove dead assignment
   hw/i2c/pm_smbus: Remove dead assignment
   hw/input/adb-kbd: Remove dead assignment
   hw/ide/sii3112: Remove dead assignment
   hw/isa/i82378: Remove dead assignment
   hw/gpio/aspeed_gpio: Remove dead assignment
   hw/timer/exynos4210_mct: Remove dead assignments
   hw/timer/stm32f2xx_timer: Remove dead assignment
   hw/timer/pxa2xx_timer: Add assertion to silent static analyzer warning
   hw/scsi/esp-pci: Remove dead assignment

  block.c| 2 +-
  blockdev.c | 2 +-
  hw/gpio/aspeed_gpio.c  | 2 +-
  hw/i2c/pm_smbus.c  | 1 -
  hw/ide/sii3112.c   | 5 +++--
  hw/input/adb-kbd.c | 6 +-
  hw/isa/i82378.c| 8 
  hw/scsi/esp-pci.c  | 1 -
  hw/timer/exynos4210_mct.c  | 3 ---
  hw/timer/pxa2xx_timer.c| 1 +
  hw/timer/stm32f2xx_timer.c | 1 -
  11 files changed, 12 insertions(+), 20 deletions(-)



I think your series covers cases already covered by:

[PATCH v3 00/12] redundant code: Fix warnings reported by Clang static
code analyzer
https://patchew.org/QEMU/20200302130715.29440-1-kuhn.ch


Unfortunately [for me...] I don't have v3 in my INBOX... *sigh*
This was 3 weeks ago. *sigh*.

I can see the series in the archives:
https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg00219.html
But I can't find the outcome, was it queued in the trivial tree?
Any idea when this will be merged in the master tree?

What a waste of time...



Thanks,
Laurent

Re: Coverity CID 1421984

[BALATON Zoltan]

On Mon, 23 Mar 2020, Peter Maydell wrote:

On Mon, 23 Mar 2020 at 14:06, BALATON Zoltan  wrote:

On Mon, 23 Mar 2020, Peter Maydell wrote:

Coverity has flagged up a lot of leaks involving qemu_allocate_irqs();
most of them I've for the moment just set as "insignificant, fix
required" because they're in called-once functions like board init.
If this device can't be hot-unplugged and so we will only ever call
realize once, it would fall in that category too. Otherwise I'd
suggest conversion to qdev_init_gpio_in(). (This allocates arrays
of IRQs under the hood too, but the device_finalize() function will
automatically free them for you, so it's easier to use non-leakily.)


I think I can't do that in sii3112 becuase I need to pass irq to this func:

void ide_init2(IDEBus *bus, qemu_irq irq);


ide_init2(bus, qdev_get_gpio_in(DEVICE(dev), i);

should do what you want, I think.


I don't understand what you mean. Sent a patch that I think might fix this 
warning for now. I'd leave qdevifying ide code to someone else.


Regards,
BALATON Zoltan

[PATCH] ide/sii3112: Avoid leaking irqs array

[BALATON Zoltan]

Coverity CID 1421984 reports a leak in allocated irqs, this patch
attempts to plug that.

Signed-off-by: BALATON Zoltan 
---
 hw/ide/sii3112.c | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 06605d7af2..c886916873 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -31,6 +31,7 @@ typedef struct SiI3112Regs {
 typedef struct SiI3112PCIState {
 PCIIDEState i;
 MemoryRegion mmio;
+qemu_irq *irqs;
 SiI3112Regs regs[2];
 } SiI3112PCIState;
 
@@ -252,7 +253,6 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
 SiI3112PCIState *d = SII3112_PCI(dev);
 PCIIDEState *s = PCI_IDE(dev);
 MemoryRegion *mr;
-qemu_irq *irq;
 int i;
 
 pci_config_set_interrupt_pin(dev->config, 1);
@@ -280,10 +280,10 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
 memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio, 0, 16);
 pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr);
 
-irq = qemu_allocate_irqs(sii3112_set_irq, d, 2);
+d->irqs = qemu_allocate_irqs(sii3112_set_irq, d, 2);
 for (i = 0; i < 2; i++) {
 ide_bus_new(&s->bus[i], sizeof(s->bus[i]), DEVICE(dev), i, 1);
-ide_init2(&s->bus[i], irq[i]);
+ide_init2(&s->bus[i], d->irqs[i]);
 
 bmdma_init(&s->bus[i], &s->bmdma[i], s);
 s->bmdma[i].bus = &s->bus[i];
@@ -291,6 +291,13 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
 }
 }
 
+static void sii3112_unrealize(DeviceState *dev, Error **errp)
+{
+SiI3112PCIState *d = SII3112_PCI(dev);
+
+qemu_free_irqs(d->irqs, 2);
+}
+
 static void sii3112_pci_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
@@ -301,6 +308,7 @@ static void sii3112_pci_class_init(ObjectClass *klass, void 
*data)
 pd->class_id = PCI_CLASS_STORAGE_RAID;
 pd->revision = 1;
 pd->realize = sii3112_pci_realize;
+dc->unrealize = sii3112_unrealize;
 dc->reset = sii3112_reset;
 dc->desc = "SiI3112A SATA controller";
 set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-- 
2.21.1

Re: Coverity CID 1421984

[Peter Maydell]

On Mon, 23 Mar 2020 at 14:06, BALATON Zoltan  wrote:
> On Mon, 23 Mar 2020, Peter Maydell wrote:
> > Coverity has flagged up a lot of leaks involving qemu_allocate_irqs();
> > most of them I've for the moment just set as "insignificant, fix
> > required" because they're in called-once functions like board init.
> > If this device can't be hot-unplugged and so we will only ever call
> > realize once, it would fall in that category too. Otherwise I'd
> > suggest conversion to qdev_init_gpio_in(). (This allocates arrays
> > of IRQs under the hood too, but the device_finalize() function will
> > automatically free them for you, so it's easier to use non-leakily.)
>
> I think I can't do that in sii3112 becuase I need to pass irq to this func:
>
> void ide_init2(IDEBus *bus, qemu_irq irq);

 ide_init2(bus, qdev_get_gpio_in(DEVICE(dev), i);

should do what you want, I think.

thanks
-- PMM

Re: [PATCH-for-5.0 v2 11/11] hw/scsi/esp-pci: Remove dead assignment

[Laurent Vivier]

Le 21/03/2020 à 15:41, Philippe Mathieu-Daudé a écrit :
> Fix warning reported by Clang static code analyzer:
> 
> CC  hw/scsi/esp-pci.o
>   hw/scsi/esp-pci.c:198:9: warning: Value stored to 'size' is never read
>   size = 4;
>   ^  ~
> 
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/scsi/esp-pci.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
> index d5a1f9e017..2e6cc07d4e 100644
> --- a/hw/scsi/esp-pci.c
> +++ b/hw/scsi/esp-pci.c
> @@ -195,7 +195,6 @@ static void esp_pci_io_write(void *opaque, hwaddr addr,
>  val <<= shift;
>  val |= current & ~(mask << shift);
>  addr &= ~3;
> -size = 4;
>  }
>  
>  if (addr < 0x40) {
> 

This one has already been reported.

I would prefer an assert(), see my comment:
https://patchew.org/QEMU/20200302130715.29440-1-kuhn.chen...@huawei.com/20200302130715.29440-6-kuhn.chen...@huawei.com/

Thanks,
Laurent

Re: [PATCH-for-5.0 v2 00/11] misc: Trivial static code analyzer fixes

[Laurent Vivier]

Le 21/03/2020 à 15:40, Philippe Mathieu-Daudé a écrit :
> Fix trivial warnings reported by the Clang static code analyzer.
> 
> Since v1:
> - Addressed Markus/Zoltan/Aleksandar review comments
> 
> Philippe Mathieu-Daudé (11):
>   block: Avoid dead assignment
>   blockdev: Remove dead assignment
>   hw/i2c/pm_smbus: Remove dead assignment
>   hw/input/adb-kbd: Remove dead assignment
>   hw/ide/sii3112: Remove dead assignment
>   hw/isa/i82378: Remove dead assignment
>   hw/gpio/aspeed_gpio: Remove dead assignment
>   hw/timer/exynos4210_mct: Remove dead assignments
>   hw/timer/stm32f2xx_timer: Remove dead assignment
>   hw/timer/pxa2xx_timer: Add assertion to silent static analyzer warning
>   hw/scsi/esp-pci: Remove dead assignment
> 
>  block.c| 2 +-
>  blockdev.c | 2 +-
>  hw/gpio/aspeed_gpio.c  | 2 +-
>  hw/i2c/pm_smbus.c  | 1 -
>  hw/ide/sii3112.c   | 5 +++--
>  hw/input/adb-kbd.c | 6 +-
>  hw/isa/i82378.c| 8 
>  hw/scsi/esp-pci.c  | 1 -
>  hw/timer/exynos4210_mct.c  | 3 ---
>  hw/timer/pxa2xx_timer.c| 1 +
>  hw/timer/stm32f2xx_timer.c | 1 -
>  11 files changed, 12 insertions(+), 20 deletions(-)
> 

I think your series covers cases already covered by:

[PATCH v3 00/12] redundant code: Fix warnings reported by Clang static
code analyzer
https://patchew.org/QEMU/20200302130715.29440-1-kuhn.ch

Thanks,
Laurent

[PATCH v9 3/4] qcow2: add zstd cluster compression

[Denis Plotnikov]

zstd significantly reduces cluster compression time.
It provides better compression performance maintaining
the same level of the compression ratio in comparison with
zlib, which, at the moment, is the only compression
method available.

The performance test results:
Test compresses and decompresses qemu qcow2 image with just
installed rhel-7.6 guest.
Image cluster size: 64K. Image on disk size: 2.2G

The test was conducted with brd disk to reduce the influence
of disk subsystem to the test results.
The results is given in seconds.

compress cmd:
  time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
  src.img [zlib|zstd]_compressed.img
decompress cmd
  time ./qemu-img convert -O qcow2
  [zlib|zstd]_compressed.img uncompressed.img

   compression   decompression
 zlib   zstd   zlib zstd

real 65.5   16.3 (-75 %)1.9  1.6 (-16 %)
user 65.0   15.85.3  2.5
sys   3.30.22.0  2.0

Both ZLIB and ZSTD gave the same compression ratio: 1.57
compressed image size in both cases: 1.4G

Signed-off-by: Denis Plotnikov 
QAPI part:
Acked-by: Markus Armbruster 
---
 docs/interop/qcow2.txt |   1 +
 configure  |   2 +-
 qapi/block-core.json   |   3 +-
 block/qcow2-threads.c  | 134 +
 block/qcow2.c  |   7 +++
 5 files changed, 145 insertions(+), 2 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 5597e24474..795dbb21dd 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -208,6 +208,7 @@ version 2.
 
 Available compression type values:
 0: zlib 
+1: zstd 
 
 
 === Header padding ===
diff --git a/configure b/configure
index caa65f5883..b2a0aa241a 100755
--- a/configure
+++ b/configure
@@ -1835,7 +1835,7 @@ disabled with --disable-FEATURE, default is enabled if 
available:
   lzfse   support of lzfse compression library
   (for reading lzfse-compressed dmg images)
   zstdsupport for zstd compression library
-  (for migration compression)
+  (for migration compression and qcow2 cluster compression)
   seccomp seccomp support
   coroutine-pool  coroutine freelist (better performance)
   glusterfs   GlusterFS backend
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a306484973..8953451818 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4401,11 +4401,12 @@
 # Compression type used in qcow2 image file
 #
 # @zlib: zlib compression, see 
+# @zstd: zstd compression, see 
 #
 # Since: 5.0
 ##
 { 'enum': 'Qcow2CompressionType',
-  'data': [ 'zlib' ] }
+  'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
 
 ##
 # @BlockdevCreateOptionsQcow2:
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 7dbaf53489..f321d4e1a7 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -28,6 +28,11 @@
 #define ZLIB_CONST
 #include 
 
+#ifdef CONFIG_ZSTD
+#include 
+#include 
+#endif
+
 #include "qcow2.h"
 #include "block/thread-pool.h"
 #include "crypto.h"
@@ -166,6 +171,125 @@ static ssize_t qcow2_zlib_decompress(void *dest, size_t 
dest_size,
 return ret;
 }
 
+#ifdef CONFIG_ZSTD
+
+/*
+ * qcow2_zstd_compress()
+ *
+ * Compress @src_size bytes of data using zstd compression method
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: compressed size on success
+ *  -ENOMEM destination buffer is not enough to store compressed data
+ *  -EIOon any other error
+ */
+static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
+   const void *src, size_t src_size)
+{
+size_t ret;
+ZSTD_outBuffer output = { dest, dest_size, 0 };
+ZSTD_inBuffer input = { src, src_size, 0 };
+ZSTD_CCtx *cctx = ZSTD_createCCtx();
+
+if (!cctx) {
+return -EIO;
+}
+/*
+ * ZSTD spec: "You must continue calling ZSTD_compressStream2()
+ * with ZSTD_e_end until it returns 0, at which point you are
+ * free to start a new frame"
+ */
+{
+/*
+ * zstd simple interface requires the exact compressed size.
+ * zstd stream interface reads the comressed size from
+ * the compressed stream frame.
+ * Instruct zstd to compress the whole buffer and write
+ * the frame which includes the compressed size.
+ * This allows as to use zstd streaming semantics and
+ * don't store the compressed size for the zstd decompression.
+ */
+ret = ZSTD_compressStream2(cctx, &output, &input, ZSTD_e_end);

[PATCH v9 0/4] qcow2: Implement zstd cluster compression method

[Denis Plotnikov]

v9:
   * 01: fix error checking and reporting in qcow2_amend compression type part 
[Vladimir]
   * 03: replace asserts with -EIO in qcow2_zstd_decompression [Vladimir, 
Alberto]
   * 03: reword/amend/add comments, fix typos [Vladimir]

v8:
   * 03: switch zstd API from simple to stream [Eric]
 No need to state a special cluster layout for zstd
 compressed clusters.
v7:
   * use qapi_enum_parse instead of the open-coding [Eric]
   * fix wording, typos and spelling [Eric]

v6:
   * "block/qcow2-threads: fix qcow2_decompress" is removed from the series
  since it has been accepted by Max already
   * add compile time checking for Qcow2Header to be a multiple of 8 [Max, 
Alberto]
   * report error on qcow2 amending when the compression type is actually 
chnged [Max]
   * remove the extra space and the extra new line [Max]
   * re-arrange acks and signed-off-s [Vladimir]

v5:
   * replace -ENOTSUP with abort in qcow2_co_decompress [Vladimir]
   * set cluster size for all test cases in the beginning of the 287 test

v4:
   * the series is rebased on top of 01 "block/qcow2-threads: fix 
qcow2_decompress"
   * 01 is just a no-change resend to avoid extra dependencies. Still, it may 
be merged in separate

v3:
   * remove redundant max compression type value check [Vladimir, Eric]
 (the switch below checks everything)
   * prevent compression type changing on "qemu-img amend" [Vladimir]
   * remove zstd config setting, since it has been added already by
 "migration" patches [Vladimir]
   * change the compression type error message [Vladimir] 
   * fix alignment and 80-chars exceeding [Vladimir]

v2:
   * rework compression type setting [Vladimir]
   * squash iotest changes to the compression type introduction patch 
[Vladimir, Eric]
   * fix zstd availability checking in zstd iotest [Vladimir]
   * remove unnecessry casting [Eric]
   * remove rudundant checks [Eric]
   * fix compressed cluster layout in qcow2 spec [Vladimir]
   * fix wording [Eric, Vladimir]
   * fix compression type filtering in iotests [Eric]

v1:
   the initial series


Denis Plotnikov (4):
  qcow2: introduce compression type feature
  qcow2: rework the cluster compression routine
  qcow2: add zstd cluster compression
  iotests: 287: add qcow2 compression type test

 docs/interop/qcow2.txt   |   1 +
 configure|   2 +-
 qapi/block-core.json |  23 +++-
 block/qcow2.h|  20 ++-
 include/block/block_int.h|   1 +
 block/qcow2-threads.c| 205 +--
 block/qcow2.c| 120 ++
 tests/qemu-iotests/031.out   |  14 +--
 tests/qemu-iotests/036.out   |   4 +-
 tests/qemu-iotests/049.out   | 102 +++
 tests/qemu-iotests/060.out   |   1 +
 tests/qemu-iotests/061.out   |  34 ++---
 tests/qemu-iotests/065   |  28 +++--
 tests/qemu-iotests/080   |   2 +-
 tests/qemu-iotests/144.out   |   4 +-
 tests/qemu-iotests/182.out   |   2 +-
 tests/qemu-iotests/242.out   |   5 +
 tests/qemu-iotests/255.out   |   8 +-
 tests/qemu-iotests/287   | 128 +++
 tests/qemu-iotests/287.out   |  43 +++
 tests/qemu-iotests/common.filter |   3 +-
 tests/qemu-iotests/group |   1 +
 22 files changed, 643 insertions(+), 108 deletions(-)
 create mode 100755 tests/qemu-iotests/287
 create mode 100644 tests/qemu-iotests/287.out

-- 
2.17.0

[PATCH v9 2/4] qcow2: rework the cluster compression routine

[Denis Plotnikov]

The patch enables processing the image compression type defined
for the image and chooses an appropriate method for image clusters
(de)compression.

Signed-off-by: Denis Plotnikov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Alberto Garcia 
---
 block/qcow2-threads.c | 71 ---
 1 file changed, 60 insertions(+), 11 deletions(-)

diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index a68126f291..7dbaf53489 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -74,7 +74,9 @@ typedef struct Qcow2CompressData {
 } Qcow2CompressData;
 
 /*
- * qcow2_compress()
+ * qcow2_zlib_compress()
+ *
+ * Compress @src_size bytes of data using zlib compression method
  *
  * @dest - destination buffer, @dest_size bytes
  * @src - source buffer, @src_size bytes
@@ -83,8 +85,8 @@ typedef struct Qcow2CompressData {
  *  -ENOMEM destination buffer is not enough to store compressed data
  *  -EIOon any other error
  */
-static ssize_t qcow2_compress(void *dest, size_t dest_size,
-  const void *src, size_t src_size)
+static ssize_t qcow2_zlib_compress(void *dest, size_t dest_size,
+   const void *src, size_t src_size)
 {
 ssize_t ret;
 z_stream strm;
@@ -119,10 +121,10 @@ static ssize_t qcow2_compress(void *dest, size_t 
dest_size,
 }
 
 /*
- * qcow2_decompress()
+ * qcow2_zlib_decompress()
  *
  * Decompress some data (not more than @src_size bytes) to produce exactly
- * @dest_size bytes.
+ * @dest_size bytes using zlib compression method
  *
  * @dest - destination buffer, @dest_size bytes
  * @src - source buffer, @src_size bytes
@@ -130,8 +132,8 @@ static ssize_t qcow2_compress(void *dest, size_t dest_size,
  * Returns: 0 on success
  *  -EIO on fail
  */
-static ssize_t qcow2_decompress(void *dest, size_t dest_size,
-const void *src, size_t src_size)
+static ssize_t qcow2_zlib_decompress(void *dest, size_t dest_size,
+ const void *src, size_t src_size)
 {
 int ret;
 z_stream strm;
@@ -191,20 +193,67 @@ qcow2_co_do_compress(BlockDriverState *bs, void *dest, 
size_t dest_size,
 return arg.ret;
 }
 
+/*
+ * qcow2_co_compress()
+ *
+ * Compress @src_size bytes of data using the compression
+ * method defined by the image compression type
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: compressed size on success
+ *  a negative error code on failure
+ */
 ssize_t coroutine_fn
 qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
   const void *src, size_t src_size)
 {
-return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
-qcow2_compress);
+BDRVQcow2State *s = bs->opaque;
+Qcow2CompressFunc fn;
+
+switch (s->compression_type) {
+case QCOW2_COMPRESSION_TYPE_ZLIB:
+fn = qcow2_zlib_compress;
+break;
+
+default:
+abort();
+}
+
+return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
 }
 
+/*
+ * qcow2_co_decompress()
+ *
+ * Decompress some data (not more than @src_size bytes) to produce exactly
+ * @dest_size bytes using the compression method defined by the image
+ * compression type
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: 0 on success
+ *  a negative error code on failure
+ */
 ssize_t coroutine_fn
 qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
 const void *src, size_t src_size)
 {
-return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
-qcow2_decompress);
+BDRVQcow2State *s = bs->opaque;
+Qcow2CompressFunc fn;
+
+switch (s->compression_type) {
+case QCOW2_COMPRESSION_TYPE_ZLIB:
+fn = qcow2_zlib_decompress;
+break;
+
+default:
+abort();
+}
+
+return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
 }
 
 
-- 
2.17.0

[PATCH v9 1/4] qcow2: introduce compression type feature

[Denis Plotnikov]

The patch adds some preparation parts for incompatible compression type
feature to qcow2 allowing the use different compression methods for
image clusters (de)compressing.

It is implied that the compression type is set on the image creation and
can be changed only later by image conversion, thus compression type
defines the only compression algorithm used for the image, and thus,
for all image clusters.

The goal of the feature is to add support of other compression methods
to qcow2. For example, ZSTD which is more effective on compression than ZLIB.

The default compression is ZLIB. Images created with ZLIB compression type
are backward compatible with older qemu versions.

Adding of the compression type breaks a number of tests because now the
compression type is reported on image creation and there are some changes
in the qcow2 header in size and offsets.

The tests are fixed in the following ways:
* filter out compression_type for many tests
* fix header size, feature table size and backing file offset
  affected tests: 031, 036, 061, 080
  header_size +=8: 1 byte compression type
   7 bytes padding
  feature_table += 48: incompatible feature compression type
  backing_file_offset += 56 (8 + 48 -> header_change + feature_table_change)
* add "compression type" for test output matching when it isn't filtered
  affected tests: 049, 060, 061, 065, 144, 182, 242, 255

Signed-off-by: Denis Plotnikov 
---
 qapi/block-core.json |  22 +-
 block/qcow2.h|  20 +-
 include/block/block_int.h|   1 +
 block/qcow2.c| 113 +++
 tests/qemu-iotests/031.out   |  14 ++--
 tests/qemu-iotests/036.out   |   4 +-
 tests/qemu-iotests/049.out   | 102 ++--
 tests/qemu-iotests/060.out   |   1 +
 tests/qemu-iotests/061.out   |  34 ++
 tests/qemu-iotests/065   |  28 +---
 tests/qemu-iotests/080   |   2 +-
 tests/qemu-iotests/144.out   |   4 +-
 tests/qemu-iotests/182.out   |   2 +-
 tests/qemu-iotests/242.out   |   5 ++
 tests/qemu-iotests/255.out   |   8 +--
 tests/qemu-iotests/common.filter |   3 +-
 16 files changed, 267 insertions(+), 96 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 85e27bb61f..a306484973 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -78,6 +78,8 @@
 #
 # @bitmaps: A list of qcow2 bitmap details (since 4.0)
 #
+# @compression-type: the image cluster compression method (since 5.0)
+#
 # Since: 1.7
 ##
 { 'struct': 'ImageInfoSpecificQCow2',
@@ -89,7 +91,8 @@
   '*corrupt': 'bool',
   'refcount-bits': 'int',
   '*encrypt': 'ImageInfoSpecificQCow2Encryption',
-  '*bitmaps': ['Qcow2BitmapInfo']
+  '*bitmaps': ['Qcow2BitmapInfo'],
+  'compression-type': 'Qcow2CompressionType'
   } }
 
 ##
@@ -4392,6 +4395,18 @@
   'data': [ 'v2', 'v3' ] }
 
 
+##
+# @Qcow2CompressionType:
+#
+# Compression type used in qcow2 image file
+#
+# @zlib: zlib compression, see 
+#
+# Since: 5.0
+##
+{ 'enum': 'Qcow2CompressionType',
+  'data': [ 'zlib' ] }
+
 ##
 # @BlockdevCreateOptionsQcow2:
 #
@@ -4415,6 +4430,8 @@
 # allowed values: off, falloc, full, metadata)
 # @lazy-refcounts: True if refcounts may be updated lazily (default: off)
 # @refcount-bits: Width of reference counts in bits (default: 16)
+# @compression-type: The image cluster compression method
+#(default: zlib, since 5.0)
 #
 # Since: 2.12
 ##
@@ -4430,7 +4447,8 @@
 '*cluster-size':'size',
 '*preallocation':   'PreallocMode',
 '*lazy-refcounts':  'bool',
-'*refcount-bits':   'int' } }
+'*refcount-bits':   'int',
+'*compression-type':'Qcow2CompressionType' } }
 
 ##
 # @BlockdevCreateOptionsQed:
diff --git a/block/qcow2.h b/block/qcow2.h
index 0942126232..cb6bf2ab83 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -146,8 +146,16 @@ typedef struct QCowHeader {
 
 uint32_t refcount_order;
 uint32_t header_length;
+
+/* Additional fields */
+uint8_t compression_type;
+
+/* header must be a multiple of 8 */
+uint8_t padding[7];
 } QEMU_PACKED QCowHeader;
 
+QEMU_BUILD_BUG_ON(sizeof(QCowHeader) % 8 != 0);
+
 typedef struct QEMU_PACKED QCowSnapshotHeader {
 /* header is 8 byte aligned */
 uint64_t l1_table_offset;
@@ -216,13 +224,16 @@ enum {
 QCOW2_INCOMPAT_DIRTY_BITNR  = 0,
 QCOW2_INCOMPAT_CORRUPT_BITNR= 1,
 QCOW2_INCOMPAT_DATA_FILE_BITNR  = 2,
+QCOW2_INCOMPAT_COMPRESSION_BITNR = 3,
 QCOW2_INCOMPAT_DIRTY= 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
 QCOW2_INCOMPAT_CORRUPT  = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
 QCOW2_INCOMPAT_DATA_FILE= 1 << QCOW2_INCOMPAT_DATA_FILE_BITNR,
+QCOW2_INCOMPAT_COMPRESSION  = 1 << QCOW2_INCOMPAT_COMPRESSION_BITNR,
 
 QCOW2_INC

[PATCH v9 4/4] iotests: 287: add qcow2 compression type test

[Denis Plotnikov]

The test checks fulfilling qcow2 requiriements for the compression
type feature and zstd compression type operability.

Signed-off-by: Denis Plotnikov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/287 | 128 +
 tests/qemu-iotests/287.out |  43 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 172 insertions(+)
 create mode 100755 tests/qemu-iotests/287
 create mode 100644 tests/qemu-iotests/287.out

diff --git a/tests/qemu-iotests/287 b/tests/qemu-iotests/287
new file mode 100755
index 00..49d15b3d43
--- /dev/null
+++ b/tests/qemu-iotests/287
@@ -0,0 +1,128 @@
+#!/usr/bin/env bash
+#
+# Test case for an image using zstd compression
+#
+# Copyright (c) 2020 Virtuozzo International GmbH
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=dplotni...@virtuozzo.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+status=1   # failure is the default!
+
+_cleanup()
+{
+   _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# standard environment
+. ./common.rc
+. ./common.filter
+
+# This tests qocw2-specific low-level functionality
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+# for all the cases
+CLUSTER_SIZE=65536
+
+# Check if we can run this test.
+
+IMGOPTS='compression_type=zstd' _make_test_img 64M | grep "Invalid parameter 
'zstd'" 2>&1 1>/dev/null
+
+ZSTD_SUPPORTED=$?
+
+if (($ZSTD_SUPPORTED==0)); then
+_notrun "ZSTD is disabled"
+fi
+
+# Test: when compression is zlib the incompatible bit is unset
+echo
+echo "=== Testing compression type incompatible bit setting for zlib ==="
+echo
+
+IMGOPTS='compression_type=zlib' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+
+# Test: when compression differs from zlib the incompatible bit is set
+echo
+echo "=== Testing compression type incompatible bit setting for zstd ==="
+echo
+
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+
+# Test: an image can't be openned if compression type is zlib and
+#   incompatible feature compression type is set
+echo
+echo "=== Testing zlib with incompatible bit set  ==="
+echo
+
+IMGOPTS='compression_type=zlib' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" set-feature-bit incompatible 3
+# to make sure the bit was actually set
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+$QEMU_IMG info "$TEST_IMG" 2>1 1>/dev/null
+if (($?==0)); then
+echo "Error: The image openned successfully. The image must not be openned"
+fi
+
+# Test: an image can't be openned if compression type is NOT zlib and
+#   incompatible feature compression type is UNSET
+echo
+echo "=== Testing zstd with incompatible bit unset  ==="
+echo
+
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" set-header incompatible_features 0
+# to make sure the bit was actually unset
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+$QEMU_IMG info "$TEST_IMG" 2>1 1>/dev/null
+if (($?==0)); then
+echo "Error: The image openned successfully. The image must not be openned"
+fi
+# Test: check compression type values
+echo
+echo "=== Testing compression type values  ==="
+echo
+# zlib=0
+IMGOPTS='compression_type=zlib' _make_test_img 64M
+od -j104 -N1 -An -vtu1 "$TEST_IMG"
+
+# zstd=1
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+od -j104 -N1 -An -vtu1 "$TEST_IMG"
+
+# Test: using zstd compression, write to and read from an image
+echo
+echo "=== Testing reading and writing with zstd ==="
+echo
+
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+$QEMU_IO -c "write -c -P 0xAC 65536 64k " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -P 0xAC 65536 65536 " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -v 131070 8 " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -v 65534 8" "$TEST_IMG" | _filter_qemu_io
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/287.out b/tests/qemu-iotests/287.out
new file mode 100644
index 00..8e51c3078d
--- /dev/null
+++ b/tests/qemu-iotests/287.out
@@ -0,0 +1,43 @@
+QA output created by 287
+
+=== Testing compression type incompatible bit setting for zlib ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+

Re: [PATCH 0/2] zero pointer after bdrv_unref_child

[Max Reitz]

On 16.03.20 07:06, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> I faced use-after-free of bs->backing pointer after bdrv_unref_child in
> bdrv_set_backing_hd.
> 
> Fix it, and do similar thing for s->data_file in qcow2.c.
> 
> I'm not sure that this is the full fix. Is it safe to keep bs->backing
> during bdrv_unref_child itself? Is it safe to keep bs->backing during
> all-child-unref loop in bdrv_close?
> 
> 
> Vladimir Sementsov-Ogievskiy (2):
>   block: bdrv_set_backing_bs: fix use-after-free
>   block/qcow2: zero data_file child after free

Thanks, applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max



signature.asc
Description: OpenPGP digital signature

Re: Coverity CID 1421984

[BALATON Zoltan]

On Mon, 23 Mar 2020, Peter Maydell wrote:

On Mon, 23 Mar 2020 at 13:12, BALATON Zoltan  wrote:

On Mon, 23 Mar 2020, Philippe Mathieu-Daudé wrote:

Cc'ing qemu-ppc since this is restricted to the aCube Sam460ex board.
On 3/23/20 12:46 PM, Max Reitz wrote:

Hi,

I was triaging new Coverity block layer reports today, and one that
seemed like a real bug was CID 1421984:

It complains about a memleak in sii3112_pci_realize() in
hw/ide/sii3112.c, specifically about @irq being leaked (it’s allocated
by qemu_allocate_irqs(), but never put anywhere or freed).

I’m not really well-versed in anything under hw/ide, so I was wondering
whether you agree it’s a bug and whether you know the correct way to fix
it.  (I assume it’s just a g_free(irqs), but maybe there’s a more
specific way that’s applicable here.)


What does other devices is hold a reference in the DeviceState
(SiI3112PCIState) to make static analyzers happy.


Other IDE devices such as ahci and cmd646 seem to free it at the end of
the init function after calling ide_init2 with it. However it's not clear
to me how all this is supposed to work.


Anything that uses qemu_allocate_irqs() is old pre-qom/qdev code.
The qdev way of doing this kind of thing (ie "I am a device with
some inbound lines and this is the handler function to call when
the line is set") is to use qdev_init_gpio_in().

Coverity has flagged up a lot of leaks involving qemu_allocate_irqs();
most of them I've for the moment just set as "insignificant, fix
required" because they're in called-once functions like board init.
If this device can't be hot-unplugged and so we will only ever call
realize once, it would fall in that category too. Otherwise I'd
suggest conversion to qdev_init_gpio_in(). (This allocates arrays
of IRQs under the hood too, but the device_finalize() function will
automatically free them for you, so it's easier to use non-leakily.)


I think I can't do that in sii3112 becuase I need to pass irq to this func:

void ide_init2(IDEBus *bus, qemu_irq irq);

That's what all ide devices do so you probably mean ide emulation should 
be qdevified in QEMU but that's way beyond fixing this Coverity warning.



I think in the long term we should be thinking about getting rid
of all uses of qemu_allocate_irqs(): they seem to generally be
leaky.

The right way to free something allocated with qemu_allocate_irqs()
is to call qemu_free_irqs() on it, but that will free both the
array and all the IRQs within it, so you can't do that until the
device is destroyed. If the device can never be destroyed, we
usually don't write the unrealize function for it, so it would just
be a matter of storing the returned pointer from qemu_allocate_irqs()
in the device struct for a theoretical unrealize to be able to use.
If you just g_free() the pointer you got back then this leaves all
the IRQs themselves allocated, so you still have a nominal leak,
you've just swept it under the rug enough to stop Coverity seeing it :-)


That means other ide devices are likely also leaking just not noticed due 
to g_free-ing the array. For sii3112 I can implement an unrealize function 
that frees the allocated irqs which should fix it according the above.


Regards,
BALATON Zoltan

Re: [PATCH-for-5.0] block: Assert BlockDriver::format_name is not NULL

[Max Reitz]

On 18.03.20 23:22, Philippe Mathieu-Daudé wrote:
> bdrv_do_find_format() calls strcmp() using BlockDriver::format_name
> as argument, which must not be NULL. Assert this field is not null
> when we register a block driver in bdrv_register().
> 
> Reported-by: Mansour Ahmadi 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  block.c | 1 +
>  1 file changed, 1 insertion(+)

Thanks, applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max



signature.asc
Description: OpenPGP digital signature

Re: Coverity CID 1421984

[Peter Maydell]

On Mon, 23 Mar 2020 at 13:12, BALATON Zoltan  wrote:
>
> On Mon, 23 Mar 2020, Philippe Mathieu-Daudé wrote:
> > Cc'ing qemu-ppc since this is restricted to the aCube Sam460ex board.
> > On 3/23/20 12:46 PM, Max Reitz wrote:
> >> Hi,
> >>
> >> I was triaging new Coverity block layer reports today, and one that
> >> seemed like a real bug was CID 1421984:
> >>
> >> It complains about a memleak in sii3112_pci_realize() in
> >> hw/ide/sii3112.c, specifically about @irq being leaked (it’s allocated
> >> by qemu_allocate_irqs(), but never put anywhere or freed).
> >>
> >> I’m not really well-versed in anything under hw/ide, so I was wondering
> >> whether you agree it’s a bug and whether you know the correct way to fix
> >> it.  (I assume it’s just a g_free(irqs), but maybe there’s a more
> >> specific way that’s applicable here.)
> >
> > What does other devices is hold a reference in the DeviceState
> > (SiI3112PCIState) to make static analyzers happy.
>
> Other IDE devices such as ahci and cmd646 seem to free it at the end of
> the init function after calling ide_init2 with it. However it's not clear
> to me how all this is supposed to work.

Anything that uses qemu_allocate_irqs() is old pre-qom/qdev code.
The qdev way of doing this kind of thing (ie "I am a device with
some inbound lines and this is the handler function to call when
the line is set") is to use qdev_init_gpio_in().

Coverity has flagged up a lot of leaks involving qemu_allocate_irqs();
most of them I've for the moment just set as "insignificant, fix
required" because they're in called-once functions like board init.
If this device can't be hot-unplugged and so we will only ever call
realize once, it would fall in that category too. Otherwise I'd
suggest conversion to qdev_init_gpio_in(). (This allocates arrays
of IRQs under the hood too, but the device_finalize() function will
automatically free them for you, so it's easier to use non-leakily.)

I think in the long term we should be thinking about getting rid
of all uses of qemu_allocate_irqs(): they seem to generally be
leaky.

The right way to free something allocated with qemu_allocate_irqs()
is to call qemu_free_irqs() on it, but that will free both the
array and all the IRQs within it, so you can't do that until the
device is destroyed. If the device can never be destroyed, we
usually don't write the unrealize function for it, so it would just
be a matter of storing the returned pointer from qemu_allocate_irqs()
in the device struct for a theoretical unrealize to be able to use.
If you just g_free() the pointer you got back then this leaves all
the IRQs themselves allocated, so you still have a nominal leak,
you've just swept it under the rug enough to stop Coverity seeing it :-)

thanks
-- PMM

Re: [PATCH] block: Avoid memleak on qcow2 image info failure

[Max Reitz]

On 20.03.20 19:36, Eric Blake wrote:
> If we fail to get bitmap info, we must not leak the encryption info.
> 
> Fixes: b8968c875f403
> Fixes: Coverity CID 1421894
> Signed-off-by: Eric Blake 
> ---
>  block/qcow2.c | 1 +
>  1 file changed, 1 insertion(+)

Thanks, applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max



signature.asc
Description: OpenPGP digital signature

[PATCH] fix vhost_user_blk_watch crash

[Li Feng]

the G_IO_HUP is watched in tcp_chr_connect, and the callback
vhost_user_blk_watch is not needed, because tcp_chr_hup is registered as
callback. And it will close the tcp link.

Signed-off-by: Li Feng 
---
 hw/block/vhost-user-blk.c  | 19 ---
 include/hw/virtio/vhost-user-blk.h |  1 -
 2 files changed, 20 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 12925a47ec..17df5338e7 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -349,18 +349,6 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
 vhost_dev_cleanup(&s->dev);
 }
 
-static gboolean vhost_user_blk_watch(GIOChannel *chan, GIOCondition cond,
- void *opaque)
-{
-DeviceState *dev = opaque;
-VirtIODevice *vdev = VIRTIO_DEVICE(dev);
-VHostUserBlk *s = VHOST_USER_BLK(vdev);
-
-qemu_chr_fe_disconnect(&s->chardev);
-
-return true;
-}
-
 static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
 {
 DeviceState *dev = opaque;
@@ -373,15 +361,9 @@ static void vhost_user_blk_event(void *opaque, 
QEMUChrEvent event)
 qemu_chr_fe_disconnect(&s->chardev);
 return;
 }
-s->watch = qemu_chr_fe_add_watch(&s->chardev, G_IO_HUP,
- vhost_user_blk_watch, dev);
 break;
 case CHR_EVENT_CLOSED:
 vhost_user_blk_disconnect(dev);
-if (s->watch) {
-g_source_remove(s->watch);
-s->watch = 0;
-}
 break;
 case CHR_EVENT_BREAK:
 case CHR_EVENT_MUX_IN:
@@ -428,7 +410,6 @@ static void vhost_user_blk_device_realize(DeviceState *dev, 
Error **errp)
 
 s->inflight = g_new0(struct vhost_inflight, 1);
 s->vhost_vqs = g_new0(struct vhost_virtqueue, s->num_queues);
-s->watch = 0;
 s->connected = false;
 
 qemu_chr_fe_set_handlers(&s->chardev,  NULL, NULL, vhost_user_blk_event,
diff --git a/include/hw/virtio/vhost-user-blk.h 
b/include/hw/virtio/vhost-user-blk.h
index 05ea0ad183..34ad6f0c0e 100644
--- a/include/hw/virtio/vhost-user-blk.h
+++ b/include/hw/virtio/vhost-user-blk.h
@@ -38,7 +38,6 @@ typedef struct VHostUserBlk {
 VhostUserState vhost_user;
 struct vhost_virtqueue *vhost_vqs;
 VirtQueue **virtqs;
-guint watch;
 bool connected;
 } VHostUserBlk;
 
-- 
2.11.0


-- 
The SmartX email address is only for business purpose. Any sent message 
that is not related to the business is not authorized or permitted by 
SmartX.
本邮箱为北京志凌海纳科技有限公司（SmartX）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.

Re: [PATCH v4] block/nvme: introduce PMR support from NVMe 1.4 spec

[Stefan Hajnoczi]

On Fri, Mar 20, 2020 at 02:50:29PM -0700, Andrzej Jakowski wrote:
> This patch introduces support for PMR that has been defined as part of NVMe 
> 1.4
> spec. User can now specify a pmrdev option that should point to 
> HostMemoryBackend.
> pmrdev memory region will subsequently be exposed as PCI BAR 2 in emulated 
> NVMe
> device. Guest OS can perform mmio read and writes to the PMR region that will 
> stay
> persistent across system reboot.
> 
> Signed-off-by: Andrzej Jakowski 
> Reviewed-by: Klaus Jensen 
> ---
> v3:
>  - replaced qemu_msync() use with qemu_ram_writeback() to allow pmem_persist()
>or qemu_msync() be called depending on configuration [4] (Stefan)
>  - rephrased comments to improve clarity and fixed code style issues [4]
>(Stefan, Klaus)
> 
> v2:
>  - reworked PMR to use HostMemoryBackend instead of directly mapping PMR
>backend file into qemu [1] (Stefan)
> 
> v1:
>  - provided support for Bit 1 from PMRWBM register instead of Bit 0 to ensure
>improved performance in virtualized environment [2] (Stefan)
> 
>  - added check if pmr size is power of two in size [3] (David)
> 
>  - addressed cross compilation build problems reported by CI environment
> 
> [1]: 
> https://lore.kernel.org/qemu-devel/20200306223853.37958-1-andrzej.jakow...@linux.intel.com/
> [2]: 
> https://nvmexpress.org/wp-content/uploads/NVM-Express-1_4-2019.06.10-Ratified.pdf
> [3]: 
> https://lore.kernel.org/qemu-devel/20200218224811.30050-1-andrzej.jakow...@linux.intel.com/
> [4]: 
> https://lore.kernel.org/qemu-devel/20200318200303.11322-1-andrzej.jakow...@linux.intel.com/
> ---
> Persistent Memory Region (PMR) is a new optional feature provided in NVMe 1.4
> specification. This patch implements initial support for it in NVMe driver.
> ---
>  hw/block/Makefile.objs |   2 +-
>  hw/block/nvme.c| 109 ++
>  hw/block/nvme.h|   2 +
>  hw/block/trace-events  |   4 +
>  include/block/nvme.h   | 172 +
>  5 files changed, 288 insertions(+), 1 deletion(-)

Excellent, thank you!

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [PATCH v4 2/2] lockable: replaced locks with lock guard macros where appropriate

[Stefan Hajnoczi]

On Fri, Mar 20, 2020 at 05:31:37AM -0700, dnbrd...@gmail.com wrote:
> From: Daniel Brodsky 
> 
> - ran regexp "qemu_mutex_lock\(.*\).*\n.*if" to find targets
> - replaced result with QEMU_LOCK_GUARD if all unlocks at function end
> - replaced result with WITH_QEMU_LOCK_GUARD if unlock not at end
> 
> Signed-off-by: Daniel Brodsky 
> ---
>  block/iscsi.c |  7 ++
>  block/nfs.c   | 51 ---
>  cpus-common.c | 14 +---
>  hw/display/qxl.c  | 43 +---
>  hw/vfio/platform.c|  5 ++---
>  migration/migration.c |  3 +--
>  migration/multifd.c   |  8 +++
>  migration/ram.c   |  3 +--
>  monitor/misc.c|  4 +---
>  ui/spice-display.c| 14 ++--
>  util/log.c|  4 ++--
>  util/qemu-timer.c | 17 +++
>  util/rcu.c|  8 +++
>  util/thread-pool.c|  3 +--
>  util/vfio-helpers.c   |  5 ++---
>  15 files changed, 83 insertions(+), 106 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: Coverity CID 1421984

[Max Reitz]

On 23.03.20 14:11, BALATON Zoltan wrote:
> On Mon, 23 Mar 2020, Philippe Mathieu-Daudé wrote:
>> Cc'ing qemu-ppc since this is restricted to the aCube Sam460ex board.
>> On 3/23/20 12:46 PM, Max Reitz wrote:
>>> Hi,
>>>
>>> I was triaging new Coverity block layer reports today, and one that
>>> seemed like a real bug was CID 1421984:
>>>
>>> It complains about a memleak in sii3112_pci_realize() in
>>> hw/ide/sii3112.c, specifically about @irq being leaked (it’s allocated
>>> by qemu_allocate_irqs(), but never put anywhere or freed).
>>>
>>> I’m not really well-versed in anything under hw/ide, so I was wondering
>>> whether you agree it’s a bug and whether you know the correct way to fix
>>> it.  (I assume it’s just a g_free(irqs), but maybe there’s a more
>>> specific way that’s applicable here.)
>>
>> What does other devices is hold a reference in the DeviceState
>> (SiI3112PCIState) to make static analyzers happy.
> 
> Other IDE devices such as ahci and cmd646 seem to free it at the end of
> the init function after calling ide_init2 with it. However it's not
> clear to me how all this is supposed to work. Ahci does for example:
> 
> qemu_irq *irqs = qemu_allocate_irqs(ahci_irq_set, s, s->ports);
> for (i = 0; i < s->ports; i++) {
>     ide_init2(&ad->port, irqs[i]);
> }
> g_free(irqs);
> 
> So it allocates an array of s->ports irqs then only frees a single
> element?

It doesn’t free a single element, it frees the array.

> Also aren't these needed after ide_init2 to actually raise the
> irq or are these end up copied to the irq field of the BMDMAState
> sonehow? Where will that be freed?

I don’t know where the array elements end up, but they aren’t freed by
the g_free().

(irqs is an array of pointers, so freeing the array does not touch its
elements, specifically it doesn’t free what those pointers point to.)

>> Ideally we should free such memory in the DeviceUnrealize handler, but
>> we in the reality we only care for hotunpluggable devices.
>> PCI devices usually are. There is a trick however, you can mark
>> overwrite the DeviceClass::hotpluggable field in sii3112_pci_class_init:
>>
>>  dc->hotpluggable = false;
> 
> If the above is correct then simply adding g_free(irq) after the loop at
> end of sii3112_pci_realize should be enough but I can't tell if that's
> correct. Setting hotpluggable to false does not seem to be a good fix.

Well, if other code just uses g_free(irqs), it sounds good to me.  But
again, I don’t know anything about this code so far.

Max



signature.asc
Description: OpenPGP digital signature

Re: Coverity CID 1421984

[BALATON Zoltan]

On Mon, 23 Mar 2020, Philippe Mathieu-Daudé wrote:

Cc'ing qemu-ppc since this is restricted to the aCube Sam460ex board.
On 3/23/20 12:46 PM, Max Reitz wrote:

Hi,

I was triaging new Coverity block layer reports today, and one that
seemed like a real bug was CID 1421984:

It complains about a memleak in sii3112_pci_realize() in
hw/ide/sii3112.c, specifically about @irq being leaked (it’s allocated
by qemu_allocate_irqs(), but never put anywhere or freed).

I’m not really well-versed in anything under hw/ide, so I was wondering
whether you agree it’s a bug and whether you know the correct way to fix
it.  (I assume it’s just a g_free(irqs), but maybe there’s a more
specific way that’s applicable here.)


What does other devices is hold a reference in the DeviceState 
(SiI3112PCIState) to make static analyzers happy.


Other IDE devices such as ahci and cmd646 seem to free it at the end of 
the init function after calling ide_init2 with it. However it's not clear 
to me how all this is supposed to work. Ahci does for example:


qemu_irq *irqs = qemu_allocate_irqs(ahci_irq_set, s, s->ports);
for (i = 0; i < s->ports; i++) {
ide_init2(&ad->port, irqs[i]);
}
g_free(irqs);

So it allocates an array of s->ports irqs then only frees a single 
element? Also aren't these needed after ide_init2 to actually raise the 
irq or are these end up copied to the irq field of the BMDMAState sonehow? 
Where will that be freed?


Ideally we should free such memory in the DeviceUnrealize handler, but we in 
the reality we only care for hotunpluggable devices.
PCI devices usually are. There is a trick however, you can mark overwrite the 
DeviceClass::hotpluggable field in sii3112_pci_class_init:


 dc->hotpluggable = false;


If the above is correct then simply adding g_free(irq) after the loop at 
end of sii3112_pci_realize should be enough but I can't tell if that's 
correct. Setting hotpluggable to false does not seem to be a good fix.


Regards,
BALATON Zoltan

Re: [PATCH 3/3] block: fail on open when file size is unaligned to request_alignment

[Max Reitz]

On 12.03.20 13:06, Vladimir Sementsov-Ogievskiy wrote:
> 12.03.2020 14:59, Vladimir Sementsov-Ogievskiy wrote:
>> 11.03.2020 14:06, Max Reitz wrote:
>>> On 30.01.20 16:22, Vladimir Sementsov-Ogievskiy wrote:
 Prior to the commit the following command lead to crash:

    ./qemu-io --image-opts -c 'write 0 512' \
    driver=blkdebug,align=4096,image.driver=null-co,image.size=512

 It failes on assertion in bdrv_aligned_pwritev:
    "end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE"

 The problem is obvious: 512 is aligned to 4096 and becomes larger than
 file size. And the core bad thing is that file size is unaligned to
 request_alignment.

 Let's catch such case on bdrv_open_driver and fail.
>>>
>>> I think we had a discussion on this before, but I can’t find it right
>>> now.  (Although I think that had more to do with something in the
>>> file-posix driver, because it wasn’t limited to alignments above 512.)
>>>
>>> In any case, the file itself is totally valid.  Most importantly, qcow2
>>> will regularly create files with unaligned file lengths.
>>>
>>> So let me create a qcow2 image on a 4k-aligned device:
>>>
>>> $ truncate 512M fs.img

Oops, “truncate --size=512M fs.img”, of course.

>>> $ sudo losetup -f --show -b 4096 fs.img
>>> /dev/loop0
>>> $ sudo mkfs.ext4 /dev/loop0
>>> [...]
>>> $ sudo mount /dev/loop0 /mnt/tmp
>>>
>>> $ sudo ./qemu-img create -f qcow2 /mnt/tmp/foo.qcow2 64M
>>> Formatting '/mnt/tmp/foo.qcow2', fmt=qcow2 size=67108864
>>> cluster_size=65536 lazy_refcounts=off refcount_bits=16
>>> $ sudo ./qemu-io -t none -c quit /mnt/tmp/foo.qcow2
>>> qemu-io: can't open device /mnt/tmp/foo.qcow2: File size is unaligned to
>>> request alignment
>>>
>>> Which is too bad.
>>
>> What exactly is bad?

That with this patch you can no longer create a qcow2 image on a 4k
block size medium and then open it with O_DIRECT.

>> Is it correct that create succeeded? Without new error, how would
>> qcow2 driver
>> read from unaligned tail of file-posix? It will crash, isn't it?

Well, the above test works, which is better already.

Basically part of the problem seems to be changing from unsafe caching
(during create) to O_DIRECT.  If we used O_DIRECT all the time, the
image would always be aligned to the block alignment.

Now that makes me wonder how such cases are handled right now.  Do we crash?

So I tried to create an image with 512 byte clusters, write to it with
normal WB caching, thus creating an image that isn’t aligned to 4k:

$ sudo ./qemu-img create -f qcow2 -o cluster_size=512 \
  /mnt/tmp/foo.qcow2 64M
[...]
$ sudo ./qemu-io -c 'write -P 42 0 512' /mnt/tmp/foo.qcow2
[...]
$ printf '%x\n' $(stat -c '%s' /mnt/tmp/foo.qcow2)
4a00

(So not aligned to 4k)

$ sudo ./qemu-io -t none -c 'read -P 42 0 512' /mnt/tmp/foo.qcow2
read 512/512 bytes at offset 0
512 bytes, 1 ops; 00.00 sec (4.567 MiB/sec and 9352.6997 ops/sec)

So actually, that works just fine right now.

When I let file-posix print all pread() calls, this is what I can see:

[...]
pread(9, buf_ofs=+0, file_ofs=0x4000, size=0x1000)
-> 2560
pread(9, buf_ofs=+0xa00, file_ofs=0x4a00, size=0x600)
-> 0
[...]

So all reads are increased to the alignment (4k), and to read the data
cluster, we read all 4k from 0x4000 (even though it actually is just 512
bytes at 0x4800).  This succeeds, even though just as a partial read
(2560 == 0xa00), so we try to read the rest of the 4k block, but that
doesn’t work (it’s at the EOF), and so we stop reading.

handle_aiocb_rw() then zeroes out the rest of the buffer (below the
“out” label) and everyone’s happy.

So to me it looks like it works perfectly fine right now.  No crashes.

> Hmm, it crashes only on write-part if don't have RESIZE permission.. So
> for qcow2
> everything is OK.
> 
> And generic read don't care about reading past-EOF because of alignment,
> read is passed
> to driver.

(And only after the test, I realize that you apparently answered the
question yourself...  Oops.)

>>> So the real solution would probably...  Be to align the file size up to
>>> the alignment?
>>
>> On creation, you mean?

Yes.  But I suspect we’d need to do it all the time, because see my
above example: The user might access an image with WB caching at one
point, and then with O_DIRECT the next time.

But I can see two problems:

First, we don’t even know what the block alignment is.  It’s hard enough
to figure it out for O_DIRECT images, I’m not sure we can reliably do so
for WB-cached images.

Second, this wouldn’t help with pre-existing images.  And note that
“pre-existing” might mean an image a user creates on an FS with 512 byte
blocks and then moves it to another with 4k blocks.

So I don’t think aligning the file size up would work, actually.  Except
if we decided to always align it up to 4k, but then we’d still have a
problem with older pre-existing images...

Max

>>>
>>> Max
>>>
 Note, that file size and request_alignment may become out o

Re: [PATCH v8 3/4] qcow2: add zstd cluster compression

[Denis Plotnikov]

On 23.03.2020 15:47, Alberto Garcia wrote:

On Mon 23 Mar 2020 11:20:42 AM CET, Denis Plotnikov wrote:

But consider corrupted image: it may contain any data. And we should
not crash because of it. So, we should return error here.

If the image is corrupted we can't continue anyway. If we return -EIO
on this condition, we need to do some work investigating what has
happened.

Cannot you just mark the image as corrupted and return -EIO ? I also
don't think that we should crash QEMU because of malformed input data.

Berto

ok, will return -EIO

Re: [PATCH v8 3/4] qcow2: add zstd cluster compression

[Alberto Garcia]

On Mon 23 Mar 2020 11:20:42 AM CET, Denis Plotnikov wrote:
>> But consider corrupted image: it may contain any data. And we should
>> not crash because of it. So, we should return error here.
> If the image is corrupted we can't continue anyway. If we return -EIO
> on this condition, we need to do some work investigating what has
> happened.

Cannot you just mark the image as corrupted and return -EIO ? I also
don't think that we should crash QEMU because of malformed input data.

Berto

Re: [PATCH v8 1/4] qcow2: introduce compression type feature

[Denis Plotnikov]

On 23.03.2020 15:26, Vladimir Sementsov-Ogievskiy wrote:

23.03.2020 15:22, Denis Plotnikov wrote:



On 23.03.2020 11:00, Vladimir Sementsov-Ogievskiy wrote:

21.03.2020 17:34, Denis Plotnikov wrote:
The patch adds some preparation parts for incompatible compression 
type

feature to qcow2 allowing the use different compression methods for
image clusters (de)compressing.

It is implied that the compression type is set on the image 
creation and

can be changed only later by image conversion, thus compression type
defines the only compression algorithm used for the image, and thus,
for all image clusters.

The goal of the feature is to add support of other compression methods
to qcow2. For example, ZSTD which is more effective on compression 
than ZLIB.


The default compression is ZLIB. Images created with ZLIB 
compression type

are backward compatible with older qemu versions.

Adding of the compression type breaks a number of tests because now 
the
compression type is reported on image creation and there are some 
changes

in the qcow2 header in size and offsets.

The tests are fixed in the following ways:
 * filter out compression_type for many tests
 * fix header size, feature table size and backing file offset
   affected tests: 031, 036, 061, 080
   header_size +=8: 1 byte compression type
    7 bytes padding
   feature_table += 48: incompatible feature compression type
   backing_file_offset += 56 (8 + 48 -> header_change + 
feature_table_change)
 * add "compression type" for test output matching when it 
isn't filtered

   affected tests: 049, 060, 061, 065, 144, 182, 242, 255

Signed-off-by: Denis Plotnikov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---


[...]

@@ -4859,6 +4949,7 @@ static ImageInfoSpecific 
*qcow2_get_specific_info(BlockDriverState *bs,

  .data_file  = g_strdup(s->image_data_file),
  .has_data_file_raw  = has_data_file(bs),
  .data_file_raw  = data_file_is_raw(bs),
+    .compression_type   = s->compression_type,
  };
  } else {
  /* if this assertion fails, this probably means a new 
version was
@@ -5248,6 +5339,22 @@ static int 
qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,

   "images");
  return -EINVAL;
  }
+    } else if (!strcmp(desc->name, BLOCK_OPT_COMPRESSION_TYPE)) {
+    int compression_type =
+ qapi_enum_parse(&Qcow2CompressionType_lookup,
+    qemu_opt_get(opts, 
BLOCK_OPT_COMPRESSION_TYPE),

+    -1, errp);
+
+    if (compression_type == -EINVAL) {


You should compare to -1, as qapi_enum_parse returns given default 
on error.

ok



+    error_setg(errp, "Unknown compression type");


and errp is already set (ofcourse, if qemu_opt_get returned non 
NULL, but I hope it is guaranteed by if (!strcmp(desc->name, 
BLOCK_OPT_COMPRESSION_TYPE)) condition
I wouldn't propagate the error from qapi_enum_parse because it looks 
like "invalid parameter value: foo". I think it's better to print 
"Unknown compression type: foo"


No objections. Then you should pass NULL to qapi_enum_parse instead of 
errp. (As you can't set errp twice, second try will crash).

ok





+    return -ENOTSUP;
+    }
+
+    if (compression_type != s->compression_type) {
+    error_setg(errp, "Changing the compression type "
+ "is not supported");
+    return -ENOTSUP;
+    }
  } else {
  /* if this point is reached, this probably means a 
new option was

   * added without having it covered here */
@@ -5516,6 +5623,12 @@ static QemuOptsList qcow2_create_opts = {
  .help = "Width of a reference count entry in bits",
  .def_value_str = "16"
  },
+    {
+    .name = BLOCK_OPT_COMPRESSION_TYPE,
+    .type = QEMU_OPT_STRING,
+    .help = "Compression method used for image cluster 
compression",

+    .def_value_str = "zlib"
+    },
  { /* end of list */ }
  }
  };



[...]

Re: [PATCH v8 1/4] qcow2: introduce compression type feature

[Vladimir Sementsov-Ogievskiy]

23.03.2020 15:22, Denis Plotnikov wrote:



On 23.03.2020 11:00, Vladimir Sementsov-Ogievskiy wrote:

21.03.2020 17:34, Denis Plotnikov wrote:

The patch adds some preparation parts for incompatible compression type
feature to qcow2 allowing the use different compression methods for
image clusters (de)compressing.

It is implied that the compression type is set on the image creation and
can be changed only later by image conversion, thus compression type
defines the only compression algorithm used for the image, and thus,
for all image clusters.

The goal of the feature is to add support of other compression methods
to qcow2. For example, ZSTD which is more effective on compression than ZLIB.

The default compression is ZLIB. Images created with ZLIB compression type
are backward compatible with older qemu versions.

Adding of the compression type breaks a number of tests because now the
compression type is reported on image creation and there are some changes
in the qcow2 header in size and offsets.

The tests are fixed in the following ways:
 * filter out compression_type for many tests
 * fix header size, feature table size and backing file offset
   affected tests: 031, 036, 061, 080
   header_size +=8: 1 byte compression type
    7 bytes padding
   feature_table += 48: incompatible feature compression type
   backing_file_offset += 56 (8 + 48 -> header_change + 
feature_table_change)
 * add "compression type" for test output matching when it isn't filtered
   affected tests: 049, 060, 061, 065, 144, 182, 242, 255

Signed-off-by: Denis Plotnikov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---


[...]


@@ -4859,6 +4949,7 @@ static ImageInfoSpecific 
*qcow2_get_specific_info(BlockDriverState *bs,
  .data_file  = g_strdup(s->image_data_file),
  .has_data_file_raw  = has_data_file(bs),
  .data_file_raw  = data_file_is_raw(bs),
+    .compression_type   = s->compression_type,
  };
  } else {
  /* if this assertion fails, this probably means a new version was
@@ -5248,6 +5339,22 @@ static int qcow2_amend_options(BlockDriverState *bs, 
QemuOpts *opts,
   "images");
  return -EINVAL;
  }
+    } else if (!strcmp(desc->name, BLOCK_OPT_COMPRESSION_TYPE)) {
+    int compression_type =
+ qapi_enum_parse(&Qcow2CompressionType_lookup,
+    qemu_opt_get(opts, BLOCK_OPT_COMPRESSION_TYPE),
+    -1, errp);
+
+    if (compression_type == -EINVAL) {


You should compare to -1, as qapi_enum_parse returns given default on error.

ok



+    error_setg(errp, "Unknown compression type");


and errp is already set (ofcourse, if qemu_opt_get returned non NULL, but I hope 
it is guaranteed by if (!strcmp(desc->name, BLOCK_OPT_COMPRESSION_TYPE)) 
condition

I wouldn't propagate the error from qapi_enum_parse because it looks like "invalid parameter 
value: foo". I think it's better to print "Unknown compression type: foo"


No objections. Then you should pass NULL to qapi_enum_parse instead of errp. 
(As you can't set errp twice, second try will crash).




+    return -ENOTSUP;
+    }
+
+    if (compression_type != s->compression_type) {
+    error_setg(errp, "Changing the compression type "
+ "is not supported");
+    return -ENOTSUP;
+    }
  } else {
  /* if this point is reached, this probably means a new option was
   * added without having it covered here */
@@ -5516,6 +5623,12 @@ static QemuOptsList qcow2_create_opts = {
  .help = "Width of a reference count entry in bits",
  .def_value_str = "16"
  },
+    {
+    .name = BLOCK_OPT_COMPRESSION_TYPE,
+    .type = QEMU_OPT_STRING,
+    .help = "Compression method used for image cluster compression",
+    .def_value_str = "zlib"
+    },
  { /* end of list */ }
  }
  };



[...]







--
Best regards,
Vladimir

Re: [PATCH] iotests/026: Move v3-exclusive test to new file

[Max Reitz]

On 12.03.20 23:19, John Snow wrote:
> 
> 
> On 3/11/20 10:07 AM, Max Reitz wrote:
>> data_file does not work with v2, and we probably want 026 to keep
>> working for v2 images.  Thus, open a new file for v3-exclusive error
>> path test cases.
>>
>> Fixes: 81311255f217859413c94f2cd9cebf2684bbda94
>>(“iotests/026: Test EIO on allocation in a data-file”)
>> Signed-off-by: Max Reitz 
> 
> Let me start this reply with something good, or at least something
> that's not bad. It's value neutral at worst.
> 
> Reviewed-by: John Snow 
> Tested-by: John Snow 

Thanks. :)

> Now, let's get cracking on some prime nonsense.
> 
> I assume this patch is still 'pending'. Here's a complete tangent
> unrelated to your patch in every single way:

Reasonable, but it’s a bit of a shame you bury it here.  I feel like
this makes it unlikely to reach the people you want to reach.

> What's the best way to use patchew to see series that are "pending" in
> some way? I'd like to:
> 
> - Search only the block list (to:qemu-block@nongnu.org. I assume this
> catches CCs too.)
> - Exclude series that are merged (-is:merged)
> - Exclude obsoleted series (-is:obsolete)
> 
> This gets a bit closer to things that are interesting in some way --
> give or take some fuzziness with patchew's detection of "merged" or
> "obsoleted" sometimes.
> 
> - Exclude pull requests. (-is:pull seems broken, actually.)
> - Exclude reviewed series (-is:reviewed -- what does patchew consider
> 'reviewed'? does this mean fully reviewed, or any reviews?)
> 
> This gives me something a bit more useful.
> 
> - Exclude 'expired' series. I use 30 days as a mental model for this. It
> might be nice to formalize this and mark patches that received no
> replies and didn't detect any other state change as "expired" and send
> an autoreply from the bot.
> 
> (I.e., patches that are complete, applied, passed CI, were not
> obsoleted, did not appear to be merged, and received no replies from
> anyone except the patch author)
> 
> 
> ("Hi, this patch received no replies from anyone except the author (you)
> for 30 days. The series is being dropped from the pending queue and is
> being marked expired. If the patches are still important, please rebase
> them and re-send to the list.
> 
> Please use scripts/get_maintainers.pl to identify candidate maintainers
> and reviewers and make sure they are CC'd.
> 
> This series appears to touch files owned by the following maintainers:
> - Blah
> - Etc
> - And so on
> 
> For more information on the contribution process, please visit:
> ")
> 
> We don't have anything like that, so age:<30d suffices. Alright, this
> list is starting to look *pretty* decent.
> 
> project:QEMU to:qemu-block@nongnu.org not:obsolete not:merged
> -is:reviewed age:<30d
> 
> Lastly, maybe we can exclude series that don't have replies yet.

Maybe Patchew should ping these after 14 days or so.

That’s about the only thing I can contribute, because I don’t really use
Patchew myself...  I keep patches in a subfolder of my inbox on unread;
and I keep my own pending series in a separate folder.  (Before I did
that, I was much more prone to forgetting my own patches rather than
someone else’s.)

Max

> It's
> not clear to patchew which replies are:
> 
> - Unrelated comments, like this one here
> - Requests for a change
> - A question for the submitter
> - A softly-worded N-A-C-K
> 
> and without a concept of designated reviewer, perhaps lack of replies is
> good evidence that the series is untouched and needs someone to 'pick it
> up'; (-has:replies)
> 
> https://patchew.org/search?q=project%3AQEMU+to%3Aqemu-block%40nongnu.org+not%3Aobsolete+not%3Amerged+-is%3Areviewed+age%3A%3C30d+-has%3Areplies
> 
> Alright, that's pretty good, actually.
> 
> OK, yes, this patch still needs love as far as patchew understands.
> 
>> ---
>>  tests/qemu-iotests/026 | 31 ---
>>  tests/qemu-iotests/026.out |  6 --
>>  tests/qemu-iotests/026.out.nocache |  6 --
>>  tests/qemu-iotests/289 | 89 ++
>>  tests/qemu-iotests/289.out |  8 +++
>>  tests/qemu-iotests/group   |  1 +
>>  6 files changed, 98 insertions(+), 43 deletions(-)
>>  create mode 100755 tests/qemu-iotests/289
>>  create mode 100644 tests/qemu-iotests/289.out
>>
>> diff --git a/tests/qemu-iotests/026 b/tests/qemu-iotests/026
>> index b05a4692cf..b9713eb591 100755
>> --- a/tests/qemu-iotests/026
>> +++ b/tests/qemu-iotests/026
>> @@ -240,37 +240,6 @@ $QEMU_IO -c "write 0 $CLUSTER_SIZE" "$BLKDBG_TEST_IMG" 
>> | _filter_qemu_io
>>  
>>  _check_test_img
>>  
>> -echo
>> -echo === Avoid freeing external data clusters on failure ===
>> -echo
>> -
>> -# Similar test as the last one, except we test what happens when there
>> -# is an error when writing to an external data file instead of when
>> -# writing to a preallocated zero cluster
>> -_make_test_img -o "data_file=$TEST_IMG.data_file" $CLUSTER_SIZE
>> -
>> -# Put blkdebug above the data-file

Re: [PATCH v8 1/4] qcow2: introduce compression type feature

[Denis Plotnikov]

On 23.03.2020 11:00, Vladimir Sementsov-Ogievskiy wrote:

21.03.2020 17:34, Denis Plotnikov wrote:

The patch adds some preparation parts for incompatible compression type
feature to qcow2 allowing the use different compression methods for
image clusters (de)compressing.

It is implied that the compression type is set on the image creation and
can be changed only later by image conversion, thus compression type
defines the only compression algorithm used for the image, and thus,
for all image clusters.

The goal of the feature is to add support of other compression methods
to qcow2. For example, ZSTD which is more effective on compression 
than ZLIB.


The default compression is ZLIB. Images created with ZLIB compression 
type

are backward compatible with older qemu versions.

Adding of the compression type breaks a number of tests because now the
compression type is reported on image creation and there are some 
changes

in the qcow2 header in size and offsets.

The tests are fixed in the following ways:
 * filter out compression_type for many tests
 * fix header size, feature table size and backing file offset
   affected tests: 031, 036, 061, 080
   header_size +=8: 1 byte compression type
    7 bytes padding
   feature_table += 48: incompatible feature compression type
   backing_file_offset += 56 (8 + 48 -> header_change + 
feature_table_change)
 * add "compression type" for test output matching when it isn't 
filtered

   affected tests: 049, 060, 061, 065, 144, 182, 242, 255

Signed-off-by: Denis Plotnikov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---


[...]

@@ -4859,6 +4949,7 @@ static ImageInfoSpecific 
*qcow2_get_specific_info(BlockDriverState *bs,

  .data_file  = g_strdup(s->image_data_file),
  .has_data_file_raw  = has_data_file(bs),
  .data_file_raw  = data_file_is_raw(bs),
+    .compression_type   = s->compression_type,
  };
  } else {
  /* if this assertion fails, this probably means a new 
version was
@@ -5248,6 +5339,22 @@ static int 
qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,

   "images");
  return -EINVAL;
  }
+    } else if (!strcmp(desc->name, BLOCK_OPT_COMPRESSION_TYPE)) {
+    int compression_type =
+ qapi_enum_parse(&Qcow2CompressionType_lookup,
+    qemu_opt_get(opts, 
BLOCK_OPT_COMPRESSION_TYPE),

+    -1, errp);
+
+    if (compression_type == -EINVAL) {


You should compare to -1, as qapi_enum_parse returns given default on 
error.

ok



+    error_setg(errp, "Unknown compression type");


and errp is already set (ofcourse, if qemu_opt_get returned non NULL, 
but I hope it is guaranteed by if (!strcmp(desc->name, 
BLOCK_OPT_COMPRESSION_TYPE)) condition
I wouldn't propagate the error from qapi_enum_parse because it looks 
like "invalid parameter value: foo". I think it's better to print 
"Unknown compression type: foo"



+    return -ENOTSUP;
+    }
+
+    if (compression_type != s->compression_type) {
+    error_setg(errp, "Changing the compression type "
+ "is not supported");
+    return -ENOTSUP;
+    }
  } else {
  /* if this point is reached, this probably means a new 
option was

   * added without having it covered here */
@@ -5516,6 +5623,12 @@ static QemuOptsList qcow2_create_opts = {
  .help = "Width of a reference count entry in bits",
  .def_value_str = "16"
  },
+    {
+    .name = BLOCK_OPT_COMPRESSION_TYPE,
+    .type = QEMU_OPT_STRING,
+    .help = "Compression method used for image cluster 
compression",

+    .def_value_str = "zlib"
+    },
  { /* end of list */ }
  }
  };



[...]

Re: [PATCH v8 3/4] qcow2: add zstd cluster compression

[Vladimir Sementsov-Ogievskiy]

23.03.2020 13:20, Denis Plotnikov wrote:



On 23.03.2020 11:44, Vladimir Sementsov-Ogievskiy wrote:

21.03.2020 17:34, Denis Plotnikov wrote:

zstd significantly reduces cluster compression time.
It provides better compression performance maintaining
the same level of the compression ratio in comparison with
zlib, which, at the moment, is the only compression
method available.

The performance test results:
Test compresses and decompresses qemu qcow2 image with just
installed rhel-7.6 guest.
Image cluster size: 64K. Image on disk size: 2.2G

The test was conducted with brd disk to reduce the influence
of disk subsystem to the test results.
The results is given in seconds.

compress cmd:
   time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
   src.img [zlib|zstd]_compressed.img
decompress cmd
   time ./qemu-img convert -O qcow2
   [zlib|zstd]_compressed.img uncompressed.img

    compression   decompression
  zlib   zstd   zlib zstd

real 65.5   16.3 (-75 %)    1.9  1.6 (-16 %)
user 65.0   15.8    5.3  2.5
sys   3.3    0.2    2.0  2.0

Both ZLIB and ZSTD gave the same compression ratio: 1.57
compressed image size in both cases: 1.4G

Signed-off-by: Denis Plotnikov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Alberto Garcia 
QAPI part:
Acked-by: Markus Armbruster 


You forget to drop signs, patch is changed significantly, including algorithm.

ok, my fault



---
  docs/interop/qcow2.txt |   1 +
  configure  |   2 +-
  qapi/block-core.json   |   3 +-
  block/qcow2-threads.c  | 129 +
  block/qcow2.c  |   7 +++
  5 files changed, 140 insertions(+), 2 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 5597e24474..795dbb21dd 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -208,6 +208,7 @@ version 2.
    Available compression type values:
  0: zlib 
+    1: zstd 
      === Header padding ===
diff --git a/configure b/configure
index caa65f5883..b2a0aa241a 100755
--- a/configure
+++ b/configure
@@ -1835,7 +1835,7 @@ disabled with --disable-FEATURE, default is enabled if 
available:
    lzfse   support of lzfse compression library
    (for reading lzfse-compressed dmg images)
    zstd    support for zstd compression library
-  (for migration compression)
+  (for migration compression and qcow2 cluster compression)
    seccomp seccomp support
    coroutine-pool  coroutine freelist (better performance)
    glusterfs   GlusterFS backend
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a306484973..8953451818 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4401,11 +4401,12 @@
  # Compression type used in qcow2 image file
  #
  # @zlib: zlib compression, see 
+# @zstd: zstd compression, see 
  #
  # Since: 5.0
  ##
  { 'enum': 'Qcow2CompressionType',
-  'data': [ 'zlib' ] }
+  'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
    ##
  # @BlockdevCreateOptionsQcow2:
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 7dbaf53489..ee4bad8d5b 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -28,6 +28,11 @@
  #define ZLIB_CONST
  #include 
  +#ifdef CONFIG_ZSTD
+#include 
+#include 
+#endif
+
  #include "qcow2.h"
  #include "block/thread-pool.h"
  #include "crypto.h"
@@ -166,6 +171,120 @@ static ssize_t qcow2_zlib_decompress(void *dest, size_t 
dest_size,
  return ret;
  }
  +#ifdef CONFIG_ZSTD
+
+/*
+ * qcow2_zstd_compress()
+ *
+ * Compress @src_size bytes of data using zstd compression method
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: compressed size on success
+ *  -ENOMEM destination buffer is not enough to store compressed data
+ *  -EIO    on any other error
+ */
+static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
+   const void *src, size_t src_size)
+{
+    size_t ret;
+    ZSTD_outBuffer output = { dest, dest_size, 0 };
+    ZSTD_inBuffer input = { src, src_size, 0 };
+    ZSTD_CCtx *cctx = ZSTD_createCCtx();
+
+    if (!cctx) {
+    return -EIO;
+    }
+
+    ret = ZSTD_CCtx_setParameter(cctx, ZSTD_c_compressionLevel,
+ ZSTD_CLEVEL_DEFAULT);


Hmm, looks a bit strange.. Isn't it already default by default?)

Should I remove it? Doesn't worth to express that explicitly? This line removes 
the question what compression level is used.


No it doesn't, as we don't know the constant, neither 

Re: Coverity CID 1421984

[Philippe Mathieu-Daudé]

Cc'ing qemu-ppc since this is restricted to the aCube Sam460ex board.

On 3/23/20 12:46 PM, Max Reitz wrote:

Hi,

I was triaging new Coverity block layer reports today, and one that
seemed like a real bug was CID 1421984:

It complains about a memleak in sii3112_pci_realize() in
hw/ide/sii3112.c, specifically about @irq being leaked (it’s allocated
by qemu_allocate_irqs(), but never put anywhere or freed).

I’m not really well-versed in anything under hw/ide, so I was wondering
whether you agree it’s a bug and whether you know the correct way to fix
it.  (I assume it’s just a g_free(irqs), but maybe there’s a more
specific way that’s applicable here.)


What does other devices is hold a reference in the DeviceState 
(SiI3112PCIState) to make static analyzers happy.


Ideally we should free such memory in the DeviceUnrealize handler, but 
we in the reality we only care for hotunpluggable devices.
PCI devices usually are. There is a trick however, you can mark 
overwrite the DeviceClass::hotpluggable field in sii3112_pci_class_init:


  dc->hotpluggable = false;

Coverity CID 1421984

[Max Reitz]

Hi,

I was triaging new Coverity block layer reports today, and one that
seemed like a real bug was CID 1421984:

It complains about a memleak in sii3112_pci_realize() in
hw/ide/sii3112.c, specifically about @irq being leaked (it’s allocated
by qemu_allocate_irqs(), but never put anywhere or freed).

I’m not really well-versed in anything under hw/ide, so I was wondering
whether you agree it’s a bug and whether you know the correct way to fix
it.  (I assume it’s just a g_free(irqs), but maybe there’s a more
specific way that’s applicable here.)

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] block: Avoid memleak on qcow2 image info failure

[Andrey Shinkevich]

From: Eric Blake 
Sent: Friday, March 20, 2020 9:36 PM
To: qemu-de...@nongnu.org 
Cc: peter.mayd...@linaro.org ; Andrey Shinkevich 
; Kevin Wolf ; Max Reitz 
; open list:qcow2 
Subject: [PATCH] block: Avoid memleak on qcow2 image info failure

If we fail to get bitmap info, we must not leak the encryption info.

Fixes: b8968c875f403
Fixes: Coverity CID 1421894
Signed-off-by: Eric Blake 
---

Thank you Eric.
I have made a smoke test only without the crypto object instantiation.

Andrey


Reviewed-by: Andrey Shinkevich 

Tested-by: Andrey Shinkevich 

Re: [PATCH] aio-posix: fix io_uring with external events

[Stefan Hajnoczi]

On Thu, Mar 19, 2020 at 04:35:59PM +, Stefan Hajnoczi wrote:
> When external event sources are disabled fdmon-io_uring falls back to
> fdmon-poll.  The ->need_wait() callback needs to watch for this so it
> can return true when external event sources are disabled.
> 
> It is also necessary to call ->wait() when AioHandlers have changed
> because io_uring is asynchronous and we must submit new sqes.
> 
> Both of these changes to ->need_wait() together fix tests/test-aio -p
> /aio/external-client, which failed with:
> 
>   test-aio: tests/test-aio.c:404: test_aio_external_client: Assertion 
> `aio_poll(ctx, false)' failed.
> 
> Reported-by: Julia Suvorova 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  util/fdmon-io_uring.c | 13 -
>  1 file changed, 12 insertions(+), 1 deletion(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan


signature.asc
Description: PGP signature

Re: [PATCH v8 3/4] qcow2: add zstd cluster compression

[Denis Plotnikov]

On 23.03.2020 11:44, Vladimir Sementsov-Ogievskiy wrote:

21.03.2020 17:34, Denis Plotnikov wrote:

zstd significantly reduces cluster compression time.
It provides better compression performance maintaining
the same level of the compression ratio in comparison with
zlib, which, at the moment, is the only compression
method available.

The performance test results:
Test compresses and decompresses qemu qcow2 image with just
installed rhel-7.6 guest.
Image cluster size: 64K. Image on disk size: 2.2G

The test was conducted with brd disk to reduce the influence
of disk subsystem to the test results.
The results is given in seconds.

compress cmd:
   time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
   src.img [zlib|zstd]_compressed.img
decompress cmd
   time ./qemu-img convert -O qcow2
   [zlib|zstd]_compressed.img uncompressed.img

    compression   decompression
  zlib   zstd   zlib zstd

real 65.5   16.3 (-75 %)    1.9  1.6 (-16 %)
user 65.0   15.8    5.3  2.5
sys   3.3    0.2    2.0  2.0

Both ZLIB and ZSTD gave the same compression ratio: 1.57
compressed image size in both cases: 1.4G

Signed-off-by: Denis Plotnikov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Alberto Garcia 
QAPI part:
Acked-by: Markus Armbruster 


You forget to drop signs, patch is changed significantly, including 
algorithm.

ok, my fault



---
  docs/interop/qcow2.txt |   1 +
  configure  |   2 +-
  qapi/block-core.json   |   3 +-
  block/qcow2-threads.c  | 129 +
  block/qcow2.c  |   7 +++
  5 files changed, 140 insertions(+), 2 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 5597e24474..795dbb21dd 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -208,6 +208,7 @@ version 2.
    Available compression type values:
  0: zlib 
+    1: zstd 
      === Header padding ===
diff --git a/configure b/configure
index caa65f5883..b2a0aa241a 100755
--- a/configure
+++ b/configure
@@ -1835,7 +1835,7 @@ disabled with --disable-FEATURE, default is 
enabled if available:

    lzfse   support of lzfse compression library
    (for reading lzfse-compressed dmg images)
    zstd    support for zstd compression library
-  (for migration compression)
+  (for migration compression and qcow2 cluster 
compression)

    seccomp seccomp support
    coroutine-pool  coroutine freelist (better performance)
    glusterfs   GlusterFS backend
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a306484973..8953451818 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4401,11 +4401,12 @@
  # Compression type used in qcow2 image file
  #
  # @zlib: zlib compression, see 
+# @zstd: zstd compression, see 
  #
  # Since: 5.0
  ##
  { 'enum': 'Qcow2CompressionType',
-  'data': [ 'zlib' ] }
+  'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } 
] }

    ##
  # @BlockdevCreateOptionsQcow2:
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 7dbaf53489..ee4bad8d5b 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -28,6 +28,11 @@
  #define ZLIB_CONST
  #include 
  +#ifdef CONFIG_ZSTD
+#include 
+#include 
+#endif
+
  #include "qcow2.h"
  #include "block/thread-pool.h"
  #include "crypto.h"
@@ -166,6 +171,120 @@ static ssize_t qcow2_zlib_decompress(void 
*dest, size_t dest_size,

  return ret;
  }
  +#ifdef CONFIG_ZSTD
+
+/*
+ * qcow2_zstd_compress()
+ *
+ * Compress @src_size bytes of data using zstd compression method
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: compressed size on success
+ *  -ENOMEM destination buffer is not enough to store 
compressed data

+ *  -EIO    on any other error
+ */
+static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
+   const void *src, size_t src_size)
+{
+    size_t ret;
+    ZSTD_outBuffer output = { dest, dest_size, 0 };
+    ZSTD_inBuffer input = { src, src_size, 0 };
+    ZSTD_CCtx *cctx = ZSTD_createCCtx();
+
+    if (!cctx) {
+    return -EIO;
+    }
+
+    ret = ZSTD_CCtx_setParameter(cctx, ZSTD_c_compressionLevel,
+ ZSTD_CLEVEL_DEFAULT);


Hmm, looks a bit strange.. Isn't it already default by default?)
Should I remove it? Doesn't worth to express that explicitly? This line 
removes the question what compression level is used.
And gives a hint where to change it ,if we decide to implement 
compression ratio changin

Re: discard and v2 qcow2 images

[Daniel P . Berrangé]

On Fri, Mar 20, 2020 at 02:35:44PM -0500, Eric Blake wrote:
> On 3/20/20 1:58 PM, Alberto Garcia wrote:
> > Hi,
> > 
> > when full_discard is false in discard_in_l2_slice() then the selected
> > cluster should be deallocated and it should read back as zeroes. This
> > is done by clearing the cluster offset field and setting OFLAG_ZERO in
> > the L2 entry.
> > 
> > This flag is however only supported when qcow_version >= 3. In older
> > images the cluster is simply deallocated, exposing any possible
> > previous data from the backing file.
> 
> Discard is advisory, and has no requirements that discarded data read back
> as zero.  However, if write zeroes uses discard under the hood, then THAT
> usage must guarantee reading back as zero.
> 
> > 
> > This can be trivially reproduced like this:
> > 
> > qemu-img create -f qcow2 backing.img 64k
> > qemu-io -c 'write -P 0xff 0 64k' backing.img
> > qemu-img create -f qcow2 -o compat=0.10 -b backing.img top.img
> > qemu-io -c 'write -P 0x01 0 64k' top.img
> > 
> > After this, top.img is filled with 0x01. Now we issue a discard
> > command:
> > 
> > qemu-io -c 'discard 0 64k' top.img
> > 
> > top.img should now read as zeroes, but instead you get the data from
> > the backing file (0xff). If top.img was created with compat=1.1
> > instead (the default) then it would read as zeroes after the discard.
> 
> I'd argue that this is undesirable behavior, but not a bug.

I think the ability to read old data from the backing file could
potentially be considered a security flaw, depending on what the
original data was in the backing file.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH v8 3/4] qcow2: add zstd cluster compression

[Vladimir Sementsov-Ogievskiy]

21.03.2020 17:34, Denis Plotnikov wrote:

zstd significantly reduces cluster compression time.
It provides better compression performance maintaining
the same level of the compression ratio in comparison with
zlib, which, at the moment, is the only compression
method available.

The performance test results:
Test compresses and decompresses qemu qcow2 image with just
installed rhel-7.6 guest.
Image cluster size: 64K. Image on disk size: 2.2G

The test was conducted with brd disk to reduce the influence
of disk subsystem to the test results.
The results is given in seconds.

compress cmd:
   time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
   src.img [zlib|zstd]_compressed.img
decompress cmd
   time ./qemu-img convert -O qcow2
   [zlib|zstd]_compressed.img uncompressed.img

compression   decompression
  zlib   zstd   zlib zstd

real 65.5   16.3 (-75 %)1.9  1.6 (-16 %)
user 65.0   15.85.3  2.5
sys   3.30.22.0  2.0

Both ZLIB and ZSTD gave the same compression ratio: 1.57
compressed image size in both cases: 1.4G

Signed-off-by: Denis Plotnikov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Alberto Garcia 
QAPI part:
Acked-by: Markus Armbruster 


You forget to drop signs, patch is changed significantly, including algorithm.


---
  docs/interop/qcow2.txt |   1 +
  configure  |   2 +-
  qapi/block-core.json   |   3 +-
  block/qcow2-threads.c  | 129 +
  block/qcow2.c  |   7 +++
  5 files changed, 140 insertions(+), 2 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 5597e24474..795dbb21dd 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -208,6 +208,7 @@ version 2.
  
  Available compression type values:

  0: zlib 
+1: zstd 
  
  
  === Header padding ===

diff --git a/configure b/configure
index caa65f5883..b2a0aa241a 100755
--- a/configure
+++ b/configure
@@ -1835,7 +1835,7 @@ disabled with --disable-FEATURE, default is enabled if 
available:
lzfse   support of lzfse compression library
(for reading lzfse-compressed dmg images)
zstdsupport for zstd compression library
-  (for migration compression)
+  (for migration compression and qcow2 cluster compression)
seccomp seccomp support
coroutine-pool  coroutine freelist (better performance)
glusterfs   GlusterFS backend
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a306484973..8953451818 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4401,11 +4401,12 @@
  # Compression type used in qcow2 image file
  #
  # @zlib: zlib compression, see 
+# @zstd: zstd compression, see 
  #
  # Since: 5.0
  ##
  { 'enum': 'Qcow2CompressionType',
-  'data': [ 'zlib' ] }
+  'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
  
  ##

  # @BlockdevCreateOptionsQcow2:
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 7dbaf53489..ee4bad8d5b 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -28,6 +28,11 @@
  #define ZLIB_CONST
  #include 
  
+#ifdef CONFIG_ZSTD

+#include 
+#include 
+#endif
+
  #include "qcow2.h"
  #include "block/thread-pool.h"
  #include "crypto.h"
@@ -166,6 +171,120 @@ static ssize_t qcow2_zlib_decompress(void *dest, size_t 
dest_size,
  return ret;
  }
  
+#ifdef CONFIG_ZSTD

+
+/*
+ * qcow2_zstd_compress()
+ *
+ * Compress @src_size bytes of data using zstd compression method
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: compressed size on success
+ *  -ENOMEM destination buffer is not enough to store compressed data
+ *  -EIOon any other error
+ */
+static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
+   const void *src, size_t src_size)
+{
+size_t ret;
+ZSTD_outBuffer output = { dest, dest_size, 0 };
+ZSTD_inBuffer input = { src, src_size, 0 };
+ZSTD_CCtx *cctx = ZSTD_createCCtx();
+
+if (!cctx) {
+return -EIO;
+}
+
+ret = ZSTD_CCtx_setParameter(cctx, ZSTD_c_compressionLevel,
+ ZSTD_CLEVEL_DEFAULT);


Hmm, looks a bit strange.. Isn't it already default by default?)


+if (ZSTD_isError(ret)) {
+ret = -EIO;
+goto out;
+}
+


Hmm, strange that we need a loop, but zstd spec directly requires it, possibly 
we need to make a comment from it:

  "ZSTD spec: You must continue calling ZSTD_compressStream2() with ZSTD_e_flush 
until it

Re: [PATCH v8 1/4] qcow2: introduce compression type feature

[Vladimir Sementsov-Ogievskiy]

21.03.2020 17:34, Denis Plotnikov wrote:

The patch adds some preparation parts for incompatible compression type
feature to qcow2 allowing the use different compression methods for
image clusters (de)compressing.

It is implied that the compression type is set on the image creation and
can be changed only later by image conversion, thus compression type
defines the only compression algorithm used for the image, and thus,
for all image clusters.

The goal of the feature is to add support of other compression methods
to qcow2. For example, ZSTD which is more effective on compression than ZLIB.

The default compression is ZLIB. Images created with ZLIB compression type
are backward compatible with older qemu versions.

Adding of the compression type breaks a number of tests because now the
compression type is reported on image creation and there are some changes
in the qcow2 header in size and offsets.

The tests are fixed in the following ways:
 * filter out compression_type for many tests
 * fix header size, feature table size and backing file offset
   affected tests: 031, 036, 061, 080
   header_size +=8: 1 byte compression type
7 bytes padding
   feature_table += 48: incompatible feature compression type
   backing_file_offset += 56 (8 + 48 -> header_change + 
feature_table_change)
 * add "compression type" for test output matching when it isn't filtered
   affected tests: 049, 060, 061, 065, 144, 182, 242, 255

Signed-off-by: Denis Plotnikov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---


[...]


@@ -4859,6 +4949,7 @@ static ImageInfoSpecific 
*qcow2_get_specific_info(BlockDriverState *bs,
  .data_file  = g_strdup(s->image_data_file),
  .has_data_file_raw  = has_data_file(bs),
  .data_file_raw  = data_file_is_raw(bs),
+.compression_type   = s->compression_type,
  };
  } else {
  /* if this assertion fails, this probably means a new version was
@@ -5248,6 +5339,22 @@ static int qcow2_amend_options(BlockDriverState *bs, 
QemuOpts *opts,
   "images");
  return -EINVAL;
  }
+} else if (!strcmp(desc->name, BLOCK_OPT_COMPRESSION_TYPE)) {
+int compression_type =
+qapi_enum_parse(&Qcow2CompressionType_lookup,
+qemu_opt_get(opts, BLOCK_OPT_COMPRESSION_TYPE),
+-1, errp);
+
+if (compression_type == -EINVAL) {


You should compare to -1, as qapi_enum_parse returns given default on error.


+error_setg(errp, "Unknown compression type");


and errp is already set (ofcourse, if qemu_opt_get returned non NULL, but I hope 
it is guaranteed by if (!strcmp(desc->name, BLOCK_OPT_COMPRESSION_TYPE)) 
condition


+return -ENOTSUP;
+}
+
+if (compression_type != s->compression_type) {
+error_setg(errp, "Changing the compression type "
+ "is not supported");
+return -ENOTSUP;
+}
  } else {
  /* if this point is reached, this probably means a new option was
   * added without having it covered here */
@@ -5516,6 +5623,12 @@ static QemuOptsList qcow2_create_opts = {
  .help = "Width of a reference count entry in bits",
  .def_value_str = "16"
  },
+{
+.name = BLOCK_OPT_COMPRESSION_TYPE,
+.type = QEMU_OPT_STRING,
+.help = "Compression method used for image cluster compression",
+.def_value_str = "zlib"
+},
  { /* end of list */ }
  }
  };



[...]


--
Best regards,
Vladimir