Re: [Qemu-block] [Qemu-devel] [PATCH 4/5] atapi: call ide_set_irq before ide_transfer_start

[Paolo Bonzini]

On 21/03/2018 01:35, John Snow wrote:
>> The ATAPI_INT_REASON_IO interrupt is raised when I/O starts, but in the
>> AHCI case ide_set_irq was actually called at the end of a mutual recursion.
>> Move it early, with the side effect that ide_transfer_start becomes a tail
>> call in ide_atapi_cmd_reply_end.
>>
> Do you know in which spec it specifies that we should interrupt?
> 
> Seems okay, but I wanted to double check the wording in the spec (was
> looking in scsi MMC and ATA8) and couldn't find it quickly.

I have an "Interrupt Reason" field in my copy of the ATA/ATAPI spec:

---
6.4.3 Input/Output (I/O) bit
The Input/Output bit shall be cleared to zero if the transfer is to the
device. The Input/Output bit shall be set to one if the transfer is to
the host. In ATA/ATAPI-7 this bit was documented as the I/O bit.
---

I suppose the commit message needs some improvement---the point is that
since this is a READ, the I/O bit must be set before we start the
transfer.  It can be done after the disk I/O starts, but it must be done
before the PIO transfer starts; in other words, the bug is only there
for AHCI.

Paolo

Re: [Qemu-block] [Qemu-devel] [PATCH 2/5] ide: push end_transfer callback to ide_transfer_halt

[Paolo Bonzini]

On 20/03/2018 23:11, John Snow wrote:
> Seems sane, with some lingering questions about what the PIO Setup FIS
> is supposed to do to begin with still remaining.

I agree here too.  Smashing the ATA and controller in the same device
makes things tricky, so I tried to make these patches pure code motion,
as much as I could.

Paolo

Re: [Qemu-block] [Qemu-devel] [PATCH 1/5] ide: push call to end_transfer_func out of start_transfer callback

[Paolo Bonzini]

On 20/03/2018 22:46, John Snow wrote:
>>  }
>> -if (s->bus->dma->ops->start_transfer) {
>> -s->bus->dma->ops->start_transfer(s->bus->dma);
>> +if (!s->bus->dma->ops->start_transfer) {
>> +s->end_transfer_func = end_transfer_func;
>> +return;
>>  }
>> +s->bus->dma->ops->start_transfer(s->bus->dma);
>> +end_transfer_func(s);
> wow, this makes me really uneasy to look at. The assumption now is: "If
> you have a start_transfer method, that method if successful implies that
> the transfer is *COMPLETED*."
> 
> It's implicit here, but I think anyone but the two of us would probably
> not understand that implication. (Or me in three months.) What can we do
> about that? Since AHCI is the only interface that uses this callback, I
> wonder if we can't just rename it to something more obvious.

You are completely right, it should be renamed to pio_transfer.

> Under normal circumstances this function really does just "start" a
> transfer and it's not obvious from context that some adapters have
> synchronous methods to finish the transfer without further PIO pingpong
> with the guest.

Yeah, though it is already doing it---the patch is only unhiding the
mutual recursion by pulling it one level up.

Paolo

Re: [Qemu-block] [Qemu-devel] [PATCH v1 1/1] iotests: fix test case 185

[QingFeng Hao]

在 2018/3/21 11:12, QingFeng Hao 写道:



在 2018/3/20 22:31, Stefan Hajnoczi 写道:

On Tue, Mar 20, 2018 at 11:11:01AM +0100, Kevin Wolf wrote:

Am 19.03.2018 um 18:53 hat Christian Borntraeger geschrieben:



On 03/19/2018 05:10 PM, Stefan Hajnoczi wrote:
On Mon, Mar 19, 2018 at 9:35 AM, QingFeng Hao 
 wrote:
Test case 185 failed since commit 4486e89c219 --- "vl: introduce 
vm_shutdown()".
It's because of the newly introduced function vm_shutdown calls 
bdrv_drain_all,
which is called later by bdrv_close_all. bdrv_drain_all resumes 
the jobs

that doubles the speed and offset is doubled.
Some jobs' status are changed as well.

Thus, let's not call bdrv_drain_all in vm_shutdown.

Signed-off-by: QingFeng Hao 
---
  cpus.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/cpus.c b/cpus.c
index 2e6701795b..ae2962508c 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1006,8 +1006,9 @@ static int do_vm_stop(RunState state, bool 
send_stop)

  qapi_event_send_stop(_abort);
  }
  }
-
-    bdrv_drain_all();
+    if (send_stop) {
+    bdrv_drain_all();
+    }


Thanks for looking into this bug!

This if statement breaks the contract of the function when send_stop
is false.  Drain ensures that pending I/O completes and that device
emulation finishes before this function returns.  This is important
for live migration, where there must be no more guest-related activity
after vm_stop().

This patch relies on the caller invoking bdrv_close_all() immediately
after do_vm_stop(), which is not documented and could lead to more
bugs in the future.

I suggest a different solution:

Test 185 relies on internal QEMU behavior which can change from time
to time.  Buffer sizes and block job iteration counts are
implementation details that are not part of qapi-schema.json or other
documented behavior.

In fact, the existing test case was already broken in IOThread mode
since iothread_stop_all() -> bdrv_set_aio_context() also includes a
bdrv_drain() with the side-effect of an extra blockjob iteration.

After 4486e89c219 ("vl: introduce vm_shutdown()") both IOThread and
non-IOThread mode perform the drain.  This has caused the test
failure.

Instead of modifying QEMU to keep the same arbitrary internal behavior
as before, please send a patch that updates tests/qemu-iotests/185.out
with the latest output.


Wouldnt it be better if the test actually masks out the values the 
are not
really part of the "agreed upon" behaviour? Wouldnt block_job_offset 
from

common.filter be what we want?


The test case has the following note:

# Note that the reference output intentionally includes the 'offset' 
field in
# BLOCK_JOB_CANCELLED events for all of the following block jobs. 
They are
# predictable and any change in the offsets would hint at a bug in 
the job

# throttling code.

Now the question is if this particular change is okay rather than
hinting at a bug and we should update the reference output or whether we
need to change qemu again.

I think the change isn't really bad, but we are doing more useless work
now than we used to do before, and we're exiting slower because we're
spawning additional I/O that we have to wait for. So the better state
was certainly the old one.


Here is the reason for the additional I/O and how it could be
eliminated:

child_job_drained_begin() pauses and child_job_drained_end() resumes the
job.  Resuming the job cancels the timer (if pending) and enters the
blockjob's coroutine.  This is why draining BlockDriverState forces an
extra iteration of the blockjob.

The current behavior is intended for the case where the job has I/O
pending at child_job_drained_begin() time.  When the I/O completes, the
job will see it must pause and it will yield at a pause point.  It makes
sense for child_job_drained_end() to resume the job so it can start I/O
again.

In our case this behavior doesn't make sense though.  The job already
yielded before child_job_drained_begin() and it doesn't need to resume
at child_job_drained_end() time.  We'd prefer to wait for the timer
expiry.
Thanks for all of your good comments! I think the better way is to 
filter out this case but I am sure if this is a proper way to do it that
adds a yielded field in struct BlockJob to record if it's yielded by 
block_job_do_yield. However, this way can only solve the offset problem 
but not the status. Maybe we need also to change 185.out? I am bit 
confused. thanks
What I did is setting job->yielded in block_job_do_yield and adding the 
check for it in block_job_pause and block_job_resume that if yielded is

true, just return.


We need to distinguish these two cases to avoid resuming the job in the
latter case.  I think this would be the proper way to avoid unnecessary
blockjob activity across drain.

I favor updating the test output though, because the code change adds
complexity and the practical benefit is not obvious to me.

QingFeng, if you decide to 

Re: [Qemu-block] [Qemu-devel] [PATCH v1 1/1] iotests: fix test case 185

[QingFeng Hao]

在 2018/3/20 22:31, Stefan Hajnoczi 写道:

On Tue, Mar 20, 2018 at 11:11:01AM +0100, Kevin Wolf wrote:

Am 19.03.2018 um 18:53 hat Christian Borntraeger geschrieben:



On 03/19/2018 05:10 PM, Stefan Hajnoczi wrote:

On Mon, Mar 19, 2018 at 9:35 AM, QingFeng Hao  wrote:

Test case 185 failed since commit 4486e89c219 --- "vl: introduce vm_shutdown()".
It's because of the newly introduced function vm_shutdown calls bdrv_drain_all,
which is called later by bdrv_close_all. bdrv_drain_all resumes the jobs
that doubles the speed and offset is doubled.
Some jobs' status are changed as well.

Thus, let's not call bdrv_drain_all in vm_shutdown.

Signed-off-by: QingFeng Hao 
---
  cpus.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/cpus.c b/cpus.c
index 2e6701795b..ae2962508c 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1006,8 +1006,9 @@ static int do_vm_stop(RunState state, bool send_stop)
  qapi_event_send_stop(_abort);
  }
  }
-
-bdrv_drain_all();
+if (send_stop) {
+bdrv_drain_all();
+}


Thanks for looking into this bug!

This if statement breaks the contract of the function when send_stop
is false.  Drain ensures that pending I/O completes and that device
emulation finishes before this function returns.  This is important
for live migration, where there must be no more guest-related activity
after vm_stop().

This patch relies on the caller invoking bdrv_close_all() immediately
after do_vm_stop(), which is not documented and could lead to more
bugs in the future.

I suggest a different solution:

Test 185 relies on internal QEMU behavior which can change from time
to time.  Buffer sizes and block job iteration counts are
implementation details that are not part of qapi-schema.json or other
documented behavior.

In fact, the existing test case was already broken in IOThread mode
since iothread_stop_all() -> bdrv_set_aio_context() also includes a
bdrv_drain() with the side-effect of an extra blockjob iteration.

After 4486e89c219 ("vl: introduce vm_shutdown()") both IOThread and
non-IOThread mode perform the drain.  This has caused the test
failure.

Instead of modifying QEMU to keep the same arbitrary internal behavior
as before, please send a patch that updates tests/qemu-iotests/185.out
with the latest output.


Wouldnt it be better if the test actually masks out the values the are not
really part of the "agreed upon" behaviour? Wouldnt block_job_offset from
common.filter be what we want?


The test case has the following note:

# Note that the reference output intentionally includes the 'offset' field in
# BLOCK_JOB_CANCELLED events for all of the following block jobs. They are
# predictable and any change in the offsets would hint at a bug in the job
# throttling code.

Now the question is if this particular change is okay rather than
hinting at a bug and we should update the reference output or whether we
need to change qemu again.

I think the change isn't really bad, but we are doing more useless work
now than we used to do before, and we're exiting slower because we're
spawning additional I/O that we have to wait for. So the better state
was certainly the old one.


Here is the reason for the additional I/O and how it could be
eliminated:

child_job_drained_begin() pauses and child_job_drained_end() resumes the
job.  Resuming the job cancels the timer (if pending) and enters the
blockjob's coroutine.  This is why draining BlockDriverState forces an
extra iteration of the blockjob.

The current behavior is intended for the case where the job has I/O
pending at child_job_drained_begin() time.  When the I/O completes, the
job will see it must pause and it will yield at a pause point.  It makes
sense for child_job_drained_end() to resume the job so it can start I/O
again.

In our case this behavior doesn't make sense though.  The job already
yielded before child_job_drained_begin() and it doesn't need to resume
at child_job_drained_end() time.  We'd prefer to wait for the timer
expiry.
Thanks for all of your good comments! I think the better way is to 
filter out this case but I am sure if this is a proper way to do it that
adds a yielded field in struct BlockJob to record if it's yielded by 
block_job_do_yield. However, this way can only solve the offset problem 
but not the status. Maybe we need also to change 185.out? I am bit 
confused. thanks


We need to distinguish these two cases to avoid resuming the job in the
latter case.  I think this would be the proper way to avoid unnecessary
blockjob activity across drain.

I favor updating the test output though, because the code change adds
complexity and the practical benefit is not obvious to me.

QingFeng, if you decide to modify blockjobs, please CC Jeffrey Cody
 and I'd also be happy to review the patch

Stefan



--
Regards
QingFeng Hao

Re: [Qemu-block] [Qemu-devel] [PATCH 4/5] atapi: call ide_set_irq before ide_transfer_start

[John Snow]

On 02/23/2018 10:26 AM, Paolo Bonzini wrote:
> The ATAPI_INT_REASON_IO interrupt is raised when I/O starts, but in the
> AHCI case ide_set_irq was actually called at the end of a mutual recursion.
> Move it early, with the side effect that ide_transfer_start becomes a tail
> call in ide_atapi_cmd_reply_end.
> 

Do you know in which spec it specifies that we should interrupt?

Seems okay, but I wanted to double check the wording in the spec (was
looking in scsi MMC and ATA8) and couldn't find it quickly.

> Signed-off-by: Paolo Bonzini 
> ---
>  hw/ide/atapi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index c0509c8bf5..be99a929cf 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -287,6 +287,7 @@ void ide_atapi_cmd_reply_end(IDEState *s)
>  } else {
>  /* a new transfer is needed */
>  s->nsector = (s->nsector & ~7) | ATAPI_INT_REASON_IO;
> +ide_set_irq(s->bus);
>  byte_count_limit = atapi_byte_count_limit(s);
>  trace_ide_atapi_cmd_reply_end_bcl(s, byte_count_limit);
>  size = s->packet_transfer_size;
> @@ -307,10 +308,9 @@ void ide_atapi_cmd_reply_end(IDEState *s)
>  s->packet_transfer_size -= size;
>  s->elementary_transfer_size -= size;
>  s->io_buffer_index += size;
> +trace_ide_atapi_cmd_reply_end_new(s, s->status);
>  ide_transfer_start(s, s->io_buffer + s->io_buffer_index - size,
> size, ide_atapi_cmd_reply_end);
> -ide_set_irq(s->bus);
> -trace_ide_atapi_cmd_reply_end_new(s, s->status);
>  }
>  }
>  }
> 

Re: [Qemu-block] [Qemu-devel] [PATCH 3/5] ide: do not set s->end_transfer_func to ide_transfer_cancel

[John Snow]

On 02/23/2018 10:26 AM, Paolo Bonzini wrote:
> There is code checking s->end_transfer_func and it was not taught
> about ide_transfer_cancel.  We can just use ide_transfer_stop because
> s->end_transfer_func is only ever called in the DRQ phase: after
> ide_transfer_cancel, the value of s->end_transfer_func is only used
> as a marker and never used to actually invoke the function.
> 

I think you're right, but I think it's also pretty non-obvious to a
reader that a callback might be used in this way.

"John, it's your component dude, why don't you fix that?"

Err, I'm afraid of disturbing the delicate balance of spaghetti code.

*cough*

Anyway, a little comment explaining that we don't actually *expect* that
callback to actually get called would probably answer a question or two
before they arise.

> Signed-off-by: Paolo Bonzini 
> ---
>  hw/ide/core.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index c4710a6f55..447d9624df 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -553,10 +553,9 @@ static void ide_cmd_done(IDEState *s)
>  }
>  }
>  
> -static void ide_transfer_halt(IDEState *s,
> -  void(*end_transfer_func)(IDEState *))
> +static void ide_transfer_halt(IDEState *s)
>  {
> -s->end_transfer_func = end_transfer_func;
> +s->end_transfer_func = ide_transfer_stop;
>  s->data_ptr = s->io_buffer;
>  s->data_end = s->io_buffer;
>  s->status &= ~DRQ_STAT;
> @@ -564,7 +563,7 @@ static void ide_transfer_halt(IDEState *s,
>  
>  void ide_transfer_stop(IDEState *s)
>  {
> -ide_transfer_halt(s, ide_transfer_stop);
> +ide_transfer_halt(s);
>  if (s->bus->dma->ops->end_transfer) {
>  s->bus->dma->ops->end_transfer(s->bus->dma);
>  }
> @@ -573,7 +572,7 @@ void ide_transfer_stop(IDEState *s)
>  
>  static void ide_transfer_cancel(IDEState *s)
>  {
> -ide_transfer_halt(s, ide_transfer_cancel);
> +ide_transfer_halt(s);
>  }
>  
>  int64_t ide_get_sector(IDEState *s)
> 

"LGTM"

Re: [Qemu-block] [Qemu-devel] [PATCH 2/5] ide: push end_transfer callback to ide_transfer_halt

[John Snow]

On 02/23/2018 10:26 AM, Paolo Bonzini wrote:
> The callback must be invoked once we get out of the DRQ phase; because
> all end_transfer_funcs end up invoking ide_transfer_stop, call it there.
> While at it, remove the "notify" argument from ide_transfer_halt; the
> code can simply be moved to ide_transfer_stop.
> 
> Old PATA controllers have no end_transfer callback, so there is no change
> there.  For AHCI the end_transfer_func already called ide_transfer_stop
> so the effect is that the PIO FIS is written before the D2H FIS rather
> than after, which is arguably an improvement.
> 

MMK, so this is the "PIO Setup FIS" which I... actually, I was never
sure what role this was supposed to play *exactly*.

"Used by the device to provide the host adapter with sufficient
information regarding a Programmed Input/Output (PIO) data phase to
allow the host adapter to efficiently handle PIO data transfers."

So in a perfect world, the SATA layer generates this *for* the AHCI
adapter, but really both of those models are smooshed into one layer for
us, so we just generate this so the host driver doesn't freak out.

"For PIO data transfers, the device shall send to the host a PIO Setup –
Device to Host FIS just before each and every data transfer FIS that is
required to complete the data transfer. Data transfers from Host to
Device as well as data transfers from Device to Host shall follow this
algorithm."

Well, we don't emulate Data FIS packets and I don't *think* those get
cached in the received FIS data structure area for the AHCI HBA, so we
just skip those.

"Because of the stringent timing constraints in the ATA Standard, the
PIO Setup FIS includes both the starting and ending status values. These
are used by the host adapter to first signal to host software readiness
for PIO write data (BSY bit is cleared to zero and DRQ bit is set to
one), and following the PIO write burst to properly signal host software
by clearing the DRQ bit to zero and possibly setting the BSY bit to one."

This part confuses me. The starting and ending status values? How would
we know the ending status value if this gets sent by the device before a
transfer?

> However, ahci_end_transfer is now called _after_ the DRQ_STAT has been
> cleared, so remove the status check in ahci_end_transfer; it was only
> there to ensure the FIS was not written more than once, and this cannot
> happen anymore.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  hw/ide/ahci.c |  7 ++-
>  hw/ide/core.c | 15 +++
>  2 files changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 937bad55fb..c3c6843b76 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1326,12 +1326,9 @@ out:
>  static void ahci_end_transfer(IDEDMA *dma)
>  {
>  AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
> -IDEState *s = >port.ifs[0];
>  
> -if (!(s->status & DRQ_STAT)) {
> -/* done with PIO send/receive */
> -ahci_write_fis_pio(ad, le32_to_cpu(ad->cur_cmd->status));
> -}
> +/* done with PIO send/receive */
> +ahci_write_fis_pio(ad, le32_to_cpu(ad->cur_cmd->status));
>  }
>  
>  static void ahci_start_dma(IDEDMA *dma, IDEState *s,
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 92f4424dc3..c4710a6f55 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -544,7 +544,6 @@ void ide_transfer_start(IDEState *s, uint8_t *buf, int 
> size,
>  }
>  s->bus->dma->ops->start_transfer(s->bus->dma);
>  end_transfer_func(s);
> -s->bus->dma->ops->end_transfer(s->bus->dma);
>  }
>  
>  static void ide_cmd_done(IDEState *s)
> @@ -555,26 +554,26 @@ static void ide_cmd_done(IDEState *s)
>  }
>  
>  static void ide_transfer_halt(IDEState *s,
> -  void(*end_transfer_func)(IDEState *),
> -  bool notify)
> +  void(*end_transfer_func)(IDEState *))
>  {
>  s->end_transfer_func = end_transfer_func;
>  s->data_ptr = s->io_buffer;
>  s->data_end = s->io_buffer;
>  s->status &= ~DRQ_STAT;
> -if (notify) {
> -ide_cmd_done(s);
> -}
>  }
>  
>  void ide_transfer_stop(IDEState *s)
>  {
> -ide_transfer_halt(s, ide_transfer_stop, true);
> +ide_transfer_halt(s, ide_transfer_stop);
> +if (s->bus->dma->ops->end_transfer) {
> +s->bus->dma->ops->end_transfer(s->bus->dma);
> +}
> +ide_cmd_done(s);
>  }
>  
>  static void ide_transfer_cancel(IDEState *s)
>  {
> -ide_transfer_halt(s, ide_transfer_cancel, false);
> +ide_transfer_halt(s, ide_transfer_cancel);
>  }
>  
>  int64_t ide_get_sector(IDEState *s)
> 

Seems sane, with some lingering questions about what the PIO Setup FIS
is supposed to do to begin with still remaining.

Re: [Qemu-block] [Qemu-devel] [PATCH 1/5] ide: push call to end_transfer_func out of start_transfer callback

[John Snow]

On 02/23/2018 10:26 AM, Paolo Bonzini wrote:
> Split the PIO transfer across two callbacks, thus pushing the (possibly
> recursive) call to end_transfer_func up one level and out of the
> AHCI-specific code.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  hw/ide/ahci.c | 7 ++-
>  hw/ide/core.c | 9 ++---
>  include/hw/ide/internal.h | 1 +
>  3 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index e22d7be05f..937bad55fb 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1321,8 +1321,12 @@ out:
>  
>  /* Update number of transferred bytes, destroy sglist */
>  dma_buf_commit(s, size);
> +}
>  
> -s->end_transfer_func(s);
> +static void ahci_end_transfer(IDEDMA *dma)
> +{
> +AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
> +IDEState *s = >port.ifs[0];
>  
>  if (!(s->status & DRQ_STAT)) {
>  /* done with PIO send/receive */
> @@ -1444,6 +1448,7 @@ static const IDEDMAOps ahci_dma_ops = {
>  .restart = ahci_restart,
>  .restart_dma = ahci_restart_dma,
>  .start_transfer = ahci_start_transfer,
> +.end_transfer = ahci_end_transfer,
>  .prepare_buf = ahci_dma_prepare_buf,
>  .commit_buf = ahci_commit_buf,
>  .rw_buf = ahci_dma_rw_buf,
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 257b429381..92f4424dc3 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -532,16 +532,19 @@ static void ide_clear_retry(IDEState *s)
>  void ide_transfer_start(IDEState *s, uint8_t *buf, int size,
>  EndTransferFunc *end_transfer_func)
>  {
> -s->end_transfer_func = end_transfer_func;
>  s->data_ptr = buf;
>  s->data_end = buf + size;
>  ide_set_retry(s);
>  if (!(s->status & ERR_STAT)) {
>  s->status |= DRQ_STAT;
>  }
> -if (s->bus->dma->ops->start_transfer) {
> -s->bus->dma->ops->start_transfer(s->bus->dma);
> +if (!s->bus->dma->ops->start_transfer) {
> +s->end_transfer_func = end_transfer_func;
> +return;
>  }
> +s->bus->dma->ops->start_transfer(s->bus->dma);
> +end_transfer_func(s);

wow, this makes me really uneasy to look at. The assumption now is: "If
you have a start_transfer method, that method if successful implies that
the transfer is *COMPLETED*."

It's implicit here, but I think anyone but the two of us would probably
not understand that implication. (Or me in three months.) What can we do
about that? Since AHCI is the only interface that uses this callback, I
wonder if we can't just rename it to something more obvious.

perform_transfer ? do_transfer ?

Under normal circumstances this function really does just "start" a
transfer and it's not obvious from context that some adapters have
synchronous methods to finish the transfer without further PIO pingpong
with the guest.

> +s->bus->dma->ops->end_transfer(s->bus->dma);
>  }
>  
>  static void ide_cmd_done(IDEState *s)
> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
> index 88212f59df..efaabbd815 100644
> --- a/include/hw/ide/internal.h
> +++ b/include/hw/ide/internal.h
> @@ -445,6 +445,7 @@ struct IDEState {
>  struct IDEDMAOps {
>  DMAStartFunc *start_dma;
>  DMAVoidFunc *start_transfer;
> +DMAVoidFunc *end_transfer;
>  DMAInt32Func *prepare_buf;
>  DMAu32Func *commit_buf;
>  DMAIntFunc *rw_buf;
> 

Technically-OK-but-my-sadness-increased: John Snow 

Re: [Qemu-block] [PATCH for-2.12 12/12] qemu-iotests: Test vhdx image creation with QMP

[Eric Blake]

On 03/20/2018 12:36 PM, Kevin Wolf wrote:

Signed-off-by: Kevin Wolf 
---
  tests/qemu-iotests/213 | 349 +
  tests/qemu-iotests/213.out | 121 
  tests/qemu-iotests/group   |   1 +
  3 files changed, 471 insertions(+)
  create mode 100755 tests/qemu-iotests/213
  create mode 100644 tests/qemu-iotests/213.out




+
+echo
+echo "=== Invalid sizes ==="
+echo
+
+# TODO Negative image sizes aren't handled correctly, but this is a problem
+# with QAPI's implementation of the 'size' type and affects other commands as
+# well. Once this is fixed, we may want to add a test case here.
+
+# 1. 2^64 - 512
+# 2. 2^63 = 8 EB (qemu-img enforces image sizes less than this)
+# 3. 2^63 - 512 (generally valid, but with the crypto header the file will
+#exceed 63 bits)


Same comments as before on whether this comment is slightly stale after 
copy-and-paste.



+# 4. 2^46 + 1 (512 bytes more than maximum image size)


Does this image format require 512-byte alignment?  If so, are you 
missing a test of unaligned sizes?  If not, why not just 1 byte more 
than the maximum?


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [PATCH for-2.12 11/12] vhdx: Check for 4 GB maximum log size on creation

[Eric Blake]

On 03/20/2018 12:36 PM, Kevin Wolf wrote:

It's unclear what the real maximum is, but we use an uint32_t to store
the log size in vhdx_co_create(), so we should check that the given
value fits in 32 bits.

Signed-off-by: Kevin Wolf 
---
  block/vhdx.c | 4 
  1 file changed, 4 insertions(+)


Reviewed-by: Eric Blake 



diff --git a/block/vhdx.c b/block/vhdx.c
index 0e48179b81..a1a0302799 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1829,6 +1829,10 @@ static int coroutine_fn 
vhdx_co_create(BlockdevCreateOptions *opts,
  if (!vhdx_opts->has_log_size) {
  log_size = DEFAULT_LOG_SIZE;
  } else {
+if (vhdx_opts->log_size > UINT32_MAX) {
+error_setg(errp, "Log size must be smaller than 4 GB");
+return -EINVAL;
+}
  log_size = vhdx_opts->log_size;
  }
  if (log_size < MiB || (log_size % MiB) != 0) {



--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [PATCH for-2.12 10/12] vhdx: Don't use error_setg_errno() with constant errno

[Eric Blake]

On 03/20/2018 12:36 PM, Kevin Wolf wrote:

error_setg_errno() is meant for cases where we got an errno from the OS
that can add useful extra information to an error message. It's
pointless if we pass a constant errno, these cases should use plain
error_setg().


Well, it DOES let you get the libc-specific spelling of the strerror for 
that errno (not all libc report the same string) - but you're right that 
it didn't add much here.




Signed-off-by: Kevin Wolf 
---
  block/vhdx.c | 9 -
  1 file changed, 4 insertions(+), 5 deletions(-)


Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [PATCH for-2.12 09/12] vhdx: Require power-of-two block size on create

[Eric Blake]

On 03/20/2018 12:36 PM, Kevin Wolf wrote:

Images with a non-power-of-two block size are invalid and cannot be
opened. Reject such block sizes when creating an image.

Signed-off-by: Kevin Wolf 
---
  block/vhdx.c | 4 
  1 file changed, 4 insertions(+)



Reviewed-by: Eric Blake 


diff --git a/block/vhdx.c b/block/vhdx.c
index f1b97f4b49..ca211a3196 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1877,6 +1877,10 @@ static int coroutine_fn 
vhdx_co_create(BlockdevCreateOptions *opts,
  error_setg_errno(errp, EINVAL, "Block size must be a multiple of 1 
MB");
  return -EINVAL;
  }
+if (!is_power_of_2(block_size)) {
+error_setg(errp, "Block size must be a power of two");
+return -EINVAL;
+}
  if (block_size > VHDX_BLOCK_SIZE_MAX) {
  error_setg_errno(errp, EINVAL, "Block size must not exceed %d",
   VHDX_BLOCK_SIZE_MAX);



--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [PATCH for-2.12 08/12] qemu-iotests: Test parallels image creation with QMP

[Eric Blake]

On 03/20/2018 12:36 PM, Kevin Wolf wrote:

Signed-off-by: Kevin Wolf 
---
  tests/qemu-iotests/212 | 326 +
  tests/qemu-iotests/212.out | 111 +++
  tests/qemu-iotests/group   |   1 +
  3 files changed, 438 insertions(+)
  create mode 100755 tests/qemu-iotests/212
  create mode 100644 tests/qemu-iotests/212.out




+echo
+echo "=== Invalid sizes ==="
+echo
+
+# TODO Negative image sizes aren't handled correctly, but this is a problem
+# with QAPI's implementation of the 'size' type and affects other commands as
+# well. Once this is fixed, we may want to add a test case here.
+
+# 1. Misaligned image size
+# 2. 2^64 - 512
+# 3. 2^63 = 8 EB (qemu-img enforces image sizes less than this)
+# 4. 2^63 - 512 (generally valid, but with the crypto header the file will
+#exceed 63 bits)


Is this part of the comment stale copy-and-paste? There's no crypto 
header, and the real max is much smaller...



+# 5. 2^52 (512 bytes more than maximum image size)
+



+echo
+echo "=== Invalid cluster size ==="
+echo
+

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [PATCH for-2.12 07/12] parallels: Check maximum cluster size on create

[Eric Blake]

On 03/20/2018 12:36 PM, Kevin Wolf wrote:

It's unclear what the real maximum cluster size is for the Parallels
format, but let's at least make sure that we don't get integer
overflows in our .bdrv_co_create implementation.

Signed-off-by: Kevin Wolf 
---
  block/parallels.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/block/parallels.c b/block/parallels.c
index 2da5e56a9d..e4ca018c2e 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -526,6 +526,11 @@ static int coroutine_fn 
parallels_co_create(BlockdevCreateOptions* opts,
  cl_size = DEFAULT_CLUSTER_SIZE;
  }
  
+/* XXX What is the real limit here? This is an insanely large maximum. */

+if (cl_size >= UINT64_MAX / MAX_PARALLELS_IMAGE_FACTOR) {


INT64_MAX is probably a saner starting point for the division...


+error_setg(errp, "Cluster size is too large");
+return -EINVAL;
+}
  if (total_size >= MAX_PARALLELS_IMAGE_FACTOR * cl_size) {


since total_size still has to fit within off_t (63 bits, not 64)


  error_setg(errp, "Image size is too large for this cluster size");
  return -E2BIG;



With that change,
Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [PATCH for-2.12 06/12] qemu-iotests: Test invalid resize on luks

[Eric Blake]

On 03/20/2018 12:36 PM, Kevin Wolf wrote:

This tests that the .bdrv_truncate implementation for luks doesn't crash
for invalid image sizes.

Signed-off-by: Kevin Wolf 
---
  tests/qemu-iotests/210 | 37 +
  1 file changed, 37 insertions(+)


Missing a change to 210.out?

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [PATCH for-2.12 05/12] luks: Turn another invalid assertion into check

[Eric Blake]

On 03/20/2018 12:36 PM, Kevin Wolf wrote:

Commit e39e959e fixed an invalid assertion in the .bdrv_length
implementation, but left a similar assertion in place for
.bdrv_truncate. Instead of crashing when the user requests a too large
image size, fail gracefully.

A file size of exactly INT64_MAX caused failure before, but is actually
legal.

Signed-off-by: Kevin Wolf 
---
  block/crypto.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)


Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [PATCH for-2.12 04/12] qemu-iotests: Enable 025 for luks

[Eric Blake]

On 03/20/2018 12:36 PM, Kevin Wolf wrote:

We want to test resizing even for luks. The only change that is needed
is to explicitly zero out new space for luks because it's undefined.

Signed-off-by: Kevin Wolf 
---
  tests/qemu-iotests/025 | 9 -
  1 file changed, 8 insertions(+), 1 deletion(-)



Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [PATCH for-2.12 03/12] qemu-iotests: Test vdi image creation with QMP

[Eric Blake]

On 03/20/2018 12:36 PM, Kevin Wolf wrote:

Signed-off-by: Kevin Wolf 
---
  tests/qemu-iotests/211 | 256 +
  tests/qemu-iotests/211.out |  98 +
  tests/qemu-iotests/group   |   1 +
  3 files changed, 355 insertions(+)
  create mode 100755 tests/qemu-iotests/211
  create mode 100644 tests/qemu-iotests/211.out




+
+# creator
+owner=kw...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1   # failure is the default!


More of the useless boilerplate we've been meaning to clean up.  Oh well.



+# TODO Negative image sizes aren't handled correctly, but this is a problem
+# with QAPI's implementation of the 'size' type and affects other commands as
+# well. Once this is fixed, we may want to add a test case here.
+
+# 1. 2^64 - 512
+# 2. 2^63 = 8 EB (qemu-img enforces image sizes less than this)
+# 3. 2^63 - 512 (generally valid, but with the crypto header the file will
+#exceed 63 bits)
+# 4. 0x1f800 (maximum image size for VDI)
+# 5. (one byte more than maximum image size for VDI)


You list 5 cases...


+
+run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <

...but only four x-blockdev-create.  Why?

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [PATCH for-2.12 02/12] vdi: Fix build with CONFIG_VDI_DEBUG

[Eric Blake]

On 03/20/2018 12:36 PM, Kevin Wolf wrote:

Use qemu_uuid_unparse() instead of uuid_unparse() to make vdi.c compile
again when CONFIG_VDI_DEBUG is set. In order to prevent future bitrot,
replace '#ifdef CONFIG_VDI_DEBUG' by 'if (VDI_DEBUG)' so that the
compiler always sees the code.

Signed-off-by: Kevin Wolf 
---
  block/vdi.c | 22 ++
  1 file changed, 10 insertions(+), 12 deletions(-)



Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [PATCH for-2.12 01/12] vdi: Change 'static' create option to 'preallocation' in QMP

[Eric Blake]

On 03/20/2018 12:36 PM, Kevin Wolf wrote:

What static=on really does is what we call metadata preallocation for
other block drivers. While we can still change the QMP interface, make
it more consistent by using 'preallocation' for VDI, too.


The x- naming of the command means we don't HAVE to get this patch in 
for 2.12, but I agree that sooner is better than later and that doing 
this in 2.12 is worth the effort.




This doesn't implement any new functionality, so the only supported
preallocation modes are 'off' and 'metadata' for now.

Signed-off-by: Kevin Wolf 
---
  qapi/block-core.json |  6 ++
  block/vdi.c  | 24 ++--
  2 files changed, 24 insertions(+), 6 deletions(-)




+++ b/qapi/block-core.json
@@ -3834,16 +3834,14 @@
  #
  # @file Node to create the image format on
  # @size Size of the virtual disk in bytes
-# @static   Whether to create a statically (true) or
-#   dynamically (false) allocated image
-#   (default: false, i.e. dynamic)
+# @preallocationPreallocation mode for the new image (default: off)


Do we want to document that 'full' is not supported yet?  Or is just 
relying on the error message good enough?


Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [PATCH 4/5] migration/block: limit the number of parallel I/O requests

[Juan Quintela]

Peter Lieven  wrote:
> the current implementation submits up to 512 I/O requests in parallel
> which is much to high especially for a background task.
> This patch adds a maximum limit of 16 I/O requests that can
> be submitted in parallel to avoid monopolizing the I/O device.
>
> Signed-off-by: Peter Lieven 

Reviewed-by: Juan Quintela 

PD.  I can't see a trivial way to change things without refactoring the
whole code.

> ---
>  migration/block.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/migration/block.c b/migration/block.c
> index 41b95d1..ce939e2 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -37,6 +37,7 @@
>  #define MAX_IS_ALLOCATED_SEARCH (65536 * BDRV_SECTOR_SIZE)
>  
>  #define MAX_IO_BUFFERS 512
> +#define MAX_PARALLEL_IO 16
>  
>  //#define DEBUG_BLK_MIGRATION
>  
> @@ -775,6 +776,7 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
>  while ((block_mig_state.submitted +
>  block_mig_state.read_done) * BLOCK_SIZE <
> qemu_file_get_rate_limit(f) &&
> +   block_mig_state.submitted < MAX_PARALLEL_IO &&
> (block_mig_state.submitted + block_mig_state.read_done) <
> MAX_IO_BUFFERS) {
>  blk_mig_unlock();

Re: [Qemu-block] [PATCH 5/5] migration/block: compare only read blocks against the rate limiter

[Juan Quintela]

Peter Lieven  wrote:
> only read_done blocks are in the queued to be flushed to the migration
> stream. submitted blocks are still in flight.
>
> Signed-off-by: Peter Lieven 

Reviewed-by: Juan Quintela 


> ---
>  migration/block.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/migration/block.c b/migration/block.c
> index ce939e2..4e950c2 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -773,8 +773,7 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
>  
>  /* control the rate of transfer */
>  blk_mig_lock();
> -while ((block_mig_state.submitted +
> -block_mig_state.read_done) * BLOCK_SIZE <
> +while (block_mig_state.read_done * BLOCK_SIZE <
> qemu_file_get_rate_limit(f) &&
> block_mig_state.submitted < MAX_PARALLEL_IO &&
> (block_mig_state.submitted + block_mig_state.read_done) <

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.12] qcow2: Reset free_cluster_index when allocating a new refcount block

[Eric Blake]

On 03/20/2018 08:55 AM, Alberto Garcia wrote:

When we try to allocate new clusters we first look for available ones
starting from s->free_cluster_index and once we find them we increase
their reference counts. Before we get to call update_refcount() to do
this last step s->free_cluster_index is already pointing to the next
cluster after the ones we are trying to allocate.


> During update_refcount() it may happen however that we also need to
> allocate a new refcount block in order to store the refcounts of these
> new clusters

Your changes to test 121 covers this...

> (and to complicate things further that may also require
> us to grow the refcount table).

...but not this.  Is it worth also trying to cover this case in the 
testsuite as well?  Probably made easier if you use a refcount_order of 
6 instead of the default of 4, so that it is easier to make the refcount 
table spill into a second cluster because refcount blocks have fewer 
entries.



This can be reproduced easily:

  qemu-img create -f qcow2 -o cluster_size=512 hd.qcow2 1M
  qemu-io -c 'write 0 124k' hd.qcow2

After this the image has 132608 bytes (256 clusters), and the refcount
block is full. If we write 512 more bytes it should allocate two new
clusters: the data cluster itself and a new refcount block.

  qemu-io -c 'write 124k 512' hd.qcow2


Nice test case!  And yes, 512-byte clusters are easier for proving when 
we have inefficient allocation strategies.



What this patch does is reset s->free_cluster_index to its previous
value when alloc_refcount_block() returns -EAGAIN. This way the caller
will try to allocate again the original clusters if they are still
free.

The output of iotest 026 also needs to be updated because now that
images have no holes some tests fail at a different point and the
number of leaked clusters is different.

Signed-off-by: Alberto Garcia 
---
  block/qcow2-refcount.c |  7 +++
  tests/qemu-iotests/026.out |  6 +++---
  tests/qemu-iotests/121 | 20 
  tests/qemu-iotests/121.out | 10 ++
  4 files changed, 40 insertions(+), 3 deletions(-)


Long-standing bug, but it IS a bug fix, so I think it is safe for 2.12.



+/* If the caller needs to restart the search for free clusters,
+ * try the same ones first to see if they're still free. */
+if (ret == -EAGAIN) {
+if (s->free_cluster_index > (start >> s->cluster_bits)) {
+s->free_cluster_index = (start >> s->cluster_bits);
+}


Is there any harm in making this assignment unconditional, instead of 
only doing it when free_cluster_index has grown larger than start?  [And 
unrelated, but it might be nice to do a followup cleanup to track 
free_cluster_offset by bytes instead of having to shift 
free_cluster_index everywhere]


At any rate, my comments can be deferred to later if desired, so

Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [Qemu-devel] [PATCH] qemu: include generated files with <> and not ""

[Daniel P . Berrangé]

On Tue, Mar 20, 2018 at 05:33:42PM +0100, Stefan Weil wrote:
> 
> Very large projects often split in sub projects, maybe one of them
> describing the API. Then that API headers are similar to system headers
> and can be included using <>, although they still belong to the same
> larger project. Do we have a stable QEMU API described in a (small)
> number of include files which typically do not change? If yes, then
> those include files could be included using <> because we don't need
> them in dependency lists or in static code analysis reports.

QEMU doesn't have anything we'd call a stable API at the source level,
anything is subject to change at any time, and often does.

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: [Qemu-block] [Qemu-devel] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 05:34:01PM +, Daniel P. Berrangé wrote:
> On Tue, Mar 20, 2018 at 07:10:42PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Mar 20, 2018 at 05:33:42PM +0100, Stefan Weil wrote:
> > > Using <> for system include files and "" for local include files is a
> > > convention, and as far as I know most projects adhere to that
> > > convention. So does QEMU currently. Such conventions are not only
> > > important for humans, but also for tools. There are more tools than the
> > > C preprocessor which handle <> and "" differently. For example the GNU
> > > compiler uses -MD or -MMD to automatically generate dependency rules for
> > > make. While -MD generates dependencies to all include files, -MMD does
> > > so only for user include files, but not for system include files. "user"
> > > and "system" means the different forms how include statements are
> > > written. QEMU still seems to use -MMD:
> > > 
> > > rules.mak:QEMU_DGFLAGS += -MMD -MP -MT $@ -MF $(@D)/$(*F).d
> > 
> > To my knowledge, and according to my limited testing,
> > system headers in this context means
> > the default ones not supplied with -I.
> 
> GCC's definition of system header is here:
> 
>   https://gcc.gnu.org/onlinedocs/cpp/System-Headers.html
> 
> 
> Regards,
> Daniel

Proves my point, does it not?  You will note that it does not refer to
include <> anywhere.

2.8 System Headers
The header files declaring interfaces to the operating system and runtime 
libraries often cannot be written in strictly conforming C. Therefore, GCC 
gives code found in system headers special treatment. All warnings, other than 
those generated by ‘#warning’ (see Diagnostics), are suppressed while GCC is 
processing a system header. Macros defined in a system header are immune to a 
few warnings wherever they are expanded. This immunity is granted on an ad-hoc 
basis, when we find that a warning generates lots of false positives because of 
code in macros defined in system headers.

Normally, only the headers found in specific directories are considered 
system headers. These directories are determined when GCC is compiled. There 
are, however, two ways to make normal headers into system headers:

Header files found in directories added to the search path with the 
-isystem and -idirafter command-line options are treated as system headers for 
the purposes of diagnostics.
There is also a directive, #pragma GCC system_header, which tells GCC to 
consider the rest of the current include file a system header, no matter where 
it was found. Code that comes before the ‘#pragma’ in the file is not affected. 
#pragma GCC system_header has no effect in the primary source file.


Conclusion: #include <> is ignored for purposes of determining whether a header 
is
a system one or not.


> -- 
> |: 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 :|

[Qemu-block] [PATCH for-2.12 07/12] parallels: Check maximum cluster size on create

[Kevin Wolf]

It's unclear what the real maximum cluster size is for the Parallels
format, but let's at least make sure that we don't get integer
overflows in our .bdrv_co_create implementation.

Signed-off-by: Kevin Wolf 
---
 block/parallels.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/block/parallels.c b/block/parallels.c
index 2da5e56a9d..e4ca018c2e 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -526,6 +526,11 @@ static int coroutine_fn 
parallels_co_create(BlockdevCreateOptions* opts,
 cl_size = DEFAULT_CLUSTER_SIZE;
 }
 
+/* XXX What is the real limit here? This is an insanely large maximum. */
+if (cl_size >= UINT64_MAX / MAX_PARALLELS_IMAGE_FACTOR) {
+error_setg(errp, "Cluster size is too large");
+return -EINVAL;
+}
 if (total_size >= MAX_PARALLELS_IMAGE_FACTOR * cl_size) {
 error_setg(errp, "Image size is too large for this cluster size");
 return -E2BIG;
-- 
2.13.6

[Qemu-block] [PATCH for-2.12 12/12] qemu-iotests: Test vhdx image creation with QMP

[Kevin Wolf]

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/213 | 349 +
 tests/qemu-iotests/213.out | 121 
 tests/qemu-iotests/group   |   1 +
 3 files changed, 471 insertions(+)
 create mode 100755 tests/qemu-iotests/213
 create mode 100644 tests/qemu-iotests/213.out

diff --git a/tests/qemu-iotests/213 b/tests/qemu-iotests/213
new file mode 100755
index 00..7e44accb06
--- /dev/null
+++ b/tests/qemu-iotests/213
@@ -0,0 +1,349 @@
+#!/bin/bash
+#
+# Test vhdx and file image creation
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# 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=kw...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1   # failure is the default!
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt vhdx
+_supported_proto file
+_supported_os Linux
+
+function do_run_qemu()
+{
+echo Testing: "$@"
+$QEMU -nographic -qmp stdio -serial none "$@"
+echo
+}
+
+function run_qemu()
+{
+do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp \
+  | _filter_qemu | _filter_imgfmt \
+  | _filter_actual_image_size
+}
+
+echo
+echo "=== Successful image creation (defaults) ==="
+echo
+
+size=$((128 * 1024 * 1024))
+
+run_qemu <

[Qemu-block] [PATCH for-2.12 02/12] vdi: Fix build with CONFIG_VDI_DEBUG

[Kevin Wolf]

Use qemu_uuid_unparse() instead of uuid_unparse() to make vdi.c compile
again when CONFIG_VDI_DEBUG is set. In order to prevent future bitrot,
replace '#ifdef CONFIG_VDI_DEBUG' by 'if (VDI_DEBUG)' so that the
compiler always sees the code.

Signed-off-by: Kevin Wolf 
---
 block/vdi.c | 22 ++
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 73c059e69d..4a2d1ff88d 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -235,7 +235,6 @@ static void vdi_header_to_le(VdiHeader *header)
 qemu_uuid_bswap(>uuid_parent);
 }
 
-#if defined(CONFIG_VDI_DEBUG)
 static void vdi_header_print(VdiHeader *header)
 {
 char uuid[37];
@@ -257,16 +256,15 @@ static void vdi_header_print(VdiHeader *header)
 logout("block extra 0x%04x\n", header->block_extra);
 logout("blocks tot. 0x%04x\n", header->blocks_in_image);
 logout("blocks all. 0x%04x\n", header->blocks_allocated);
-uuid_unparse(header->uuid_image, uuid);
+qemu_uuid_unparse(>uuid_image, uuid);
 logout("uuid image  %s\n", uuid);
-uuid_unparse(header->uuid_last_snap, uuid);
+qemu_uuid_unparse(>uuid_last_snap, uuid);
 logout("uuid snap   %s\n", uuid);
-uuid_unparse(header->uuid_link, uuid);
+qemu_uuid_unparse(>uuid_link, uuid);
 logout("uuid link   %s\n", uuid);
-uuid_unparse(header->uuid_parent, uuid);
+qemu_uuid_unparse(>uuid_parent, uuid);
 logout("uuid parent %s\n", uuid);
 }
-#endif
 
 static int coroutine_fn vdi_co_check(BlockDriverState *bs, BdrvCheckResult 
*res,
  BdrvCheckMode fix)
@@ -387,9 +385,9 @@ static int vdi_open(BlockDriverState *bs, QDict *options, 
int flags,
 }
 
 vdi_header_to_cpu();
-#if defined(CONFIG_VDI_DEBUG)
-vdi_header_print();
-#endif
+if (VDI_DEBUG) {
+vdi_header_print();
+}
 
 if (header.disk_size > VDI_DISK_SIZE_MAX) {
 error_setg(errp, "Unsupported VDI image size (size is 0x%" PRIx64
@@ -825,9 +823,9 @@ static int coroutine_fn 
vdi_co_do_create(BlockdevCreateOptions *create_options,
 qemu_uuid_generate(_image);
 qemu_uuid_generate(_last_snap);
 /* There is no need to set header.uuid_link or header.uuid_parent here. */
-#if defined(CONFIG_VDI_DEBUG)
-vdi_header_print();
-#endif
+if (VDI_DEBUG) {
+vdi_header_print();
+}
 vdi_header_to_le();
 ret = blk_pwrite(blk, offset, , sizeof(header), 0);
 if (ret < 0) {
-- 
2.13.6

[Qemu-block] [PATCH for-2.12 06/12] qemu-iotests: Test invalid resize on luks

[Kevin Wolf]

This tests that the .bdrv_truncate implementation for luks doesn't crash
for invalid image sizes.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/210 | 37 +
 1 file changed, 37 insertions(+)

diff --git a/tests/qemu-iotests/210 b/tests/qemu-iotests/210
index 96a5213e77..e607c0d296 100755
--- a/tests/qemu-iotests/210
+++ b/tests/qemu-iotests/210
@@ -204,6 +204,43 @@ run_qemu -blockdev 
driver=file,filename="$TEST_IMG_FILE",node-name=node0 \
 { "execute": "quit" }
 EOF
 
+echo
+echo "=== Resize image with invalid sizes ==="
+echo
+
+run_qemu -blockdev driver=file,filename="$TEST_IMG_FILE",node-name=node0 \
+ -blockdev driver=luks,file=node0,key-secret=keysec0,node-name=node1 \
+ -object secret,id=keysec0,data="foo" <

[Qemu-block] [PATCH for-2.12 10/12] vhdx: Don't use error_setg_errno() with constant errno

[Kevin Wolf]

error_setg_errno() is meant for cases where we got an errno from the OS
that can add useful extra information to an error message. It's
pointless if we pass a constant errno, these cases should use plain
error_setg().

Signed-off-by: Kevin Wolf 
---
 block/vhdx.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/block/vhdx.c b/block/vhdx.c
index ca211a3196..0e48179b81 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1822,7 +1822,7 @@ static int coroutine_fn 
vhdx_co_create(BlockdevCreateOptions *opts,
 /* Validate options and set default values */
 image_size = vhdx_opts->size;
 if (image_size > VHDX_MAX_IMAGE_SIZE) {
-error_setg_errno(errp, EINVAL, "Image size too large; max of 64TB");
+error_setg(errp, "Image size too large; max of 64TB");
 return -EINVAL;
 }
 
@@ -1832,7 +1832,7 @@ static int coroutine_fn 
vhdx_co_create(BlockdevCreateOptions *opts,
 log_size = vhdx_opts->log_size;
 }
 if (log_size < MiB || (log_size % MiB) != 0) {
-error_setg_errno(errp, EINVAL, "Log size must be a multiple of 1 MB");
+error_setg(errp, "Log size must be a multiple of 1 MB");
 return -EINVAL;
 }
 
@@ -1874,7 +1874,7 @@ static int coroutine_fn 
vhdx_co_create(BlockdevCreateOptions *opts,
 }
 
 if (block_size < MiB || (block_size % MiB) != 0) {
-error_setg_errno(errp, EINVAL, "Block size must be a multiple of 1 
MB");
+error_setg(errp, "Block size must be a multiple of 1 MB");
 return -EINVAL;
 }
 if (!is_power_of_2(block_size)) {
@@ -1882,8 +1882,7 @@ static int coroutine_fn 
vhdx_co_create(BlockdevCreateOptions *opts,
 return -EINVAL;
 }
 if (block_size > VHDX_BLOCK_SIZE_MAX) {
-error_setg_errno(errp, EINVAL, "Block size must not exceed %d",
- VHDX_BLOCK_SIZE_MAX);
+error_setg(errp, "Block size must not exceed %d", VHDX_BLOCK_SIZE_MAX);
 return -EINVAL;
 }
 
-- 
2.13.6

[Qemu-block] [PATCH for-2.12 03/12] qemu-iotests: Test vdi image creation with QMP

[Kevin Wolf]

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/211 | 256 +
 tests/qemu-iotests/211.out |  98 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 355 insertions(+)
 create mode 100755 tests/qemu-iotests/211
 create mode 100644 tests/qemu-iotests/211.out

diff --git a/tests/qemu-iotests/211 b/tests/qemu-iotests/211
new file mode 100755
index 00..6bf1ca784c
--- /dev/null
+++ b/tests/qemu-iotests/211
@@ -0,0 +1,256 @@
+#!/bin/bash
+#
+# Test VDI and file image creation
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# 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=kw...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1   # failure is the default!
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt vdi
+_supported_proto file
+_supported_os Linux
+
+function do_run_qemu()
+{
+echo Testing: "$@"
+$QEMU -nographic -qmp stdio -serial none "$@"
+echo
+}
+
+function run_qemu()
+{
+do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp \
+  | _filter_qemu | _filter_imgfmt \
+  | _filter_actual_image_size
+}
+
+echo
+echo "=== Successful image creation (defaults) ==="
+echo
+
+size=$((128 * 1024 * 1024))
+
+run_qemu <

[Qemu-block] [PATCH for-2.12 08/12] qemu-iotests: Test parallels image creation with QMP

[Kevin Wolf]

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/212 | 326 +
 tests/qemu-iotests/212.out | 111 +++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 438 insertions(+)
 create mode 100755 tests/qemu-iotests/212
 create mode 100644 tests/qemu-iotests/212.out

diff --git a/tests/qemu-iotests/212 b/tests/qemu-iotests/212
new file mode 100755
index 00..112a8420ce
--- /dev/null
+++ b/tests/qemu-iotests/212
@@ -0,0 +1,326 @@
+#!/bin/bash
+#
+# Test parallels and file image creation
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# 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=kw...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1   # failure is the default!
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt parallels
+_supported_proto file
+_supported_os Linux
+
+function do_run_qemu()
+{
+echo Testing: "$@"
+$QEMU -nographic -qmp stdio -serial none "$@"
+echo
+}
+
+function run_qemu()
+{
+do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp \
+  | _filter_qemu | _filter_imgfmt \
+  | _filter_actual_image_size
+}
+
+echo
+echo "=== Successful image creation (defaults) ==="
+echo
+
+size=$((128 * 1024 * 1024))
+
+run_qemu <

[Qemu-block] [PATCH for-2.12 09/12] vhdx: Require power-of-two block size on create

[Kevin Wolf]

Images with a non-power-of-two block size are invalid and cannot be
opened. Reject such block sizes when creating an image.

Signed-off-by: Kevin Wolf 
---
 block/vhdx.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/block/vhdx.c b/block/vhdx.c
index f1b97f4b49..ca211a3196 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1877,6 +1877,10 @@ static int coroutine_fn 
vhdx_co_create(BlockdevCreateOptions *opts,
 error_setg_errno(errp, EINVAL, "Block size must be a multiple of 1 
MB");
 return -EINVAL;
 }
+if (!is_power_of_2(block_size)) {
+error_setg(errp, "Block size must be a power of two");
+return -EINVAL;
+}
 if (block_size > VHDX_BLOCK_SIZE_MAX) {
 error_setg_errno(errp, EINVAL, "Block size must not exceed %d",
  VHDX_BLOCK_SIZE_MAX);
-- 
2.13.6

[Qemu-block] [PATCH for-2.12 11/12] vhdx: Check for 4 GB maximum log size on creation

[Kevin Wolf]

It's unclear what the real maximum is, but we use an uint32_t to store
the log size in vhdx_co_create(), so we should check that the given
value fits in 32 bits.

Signed-off-by: Kevin Wolf 
---
 block/vhdx.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/block/vhdx.c b/block/vhdx.c
index 0e48179b81..a1a0302799 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1829,6 +1829,10 @@ static int coroutine_fn 
vhdx_co_create(BlockdevCreateOptions *opts,
 if (!vhdx_opts->has_log_size) {
 log_size = DEFAULT_LOG_SIZE;
 } else {
+if (vhdx_opts->log_size > UINT32_MAX) {
+error_setg(errp, "Log size must be smaller than 4 GB");
+return -EINVAL;
+}
 log_size = vhdx_opts->log_size;
 }
 if (log_size < MiB || (log_size % MiB) != 0) {
-- 
2.13.6

[Qemu-block] [PATCH for-2.12 05/12] luks: Turn another invalid assertion into check

[Kevin Wolf]

Commit e39e959e fixed an invalid assertion in the .bdrv_length
implementation, but left a similar assertion in place for
.bdrv_truncate. Instead of crashing when the user requests a too large
image size, fail gracefully.

A file size of exactly INT64_MAX caused failure before, but is actually
legal.

Signed-off-by: Kevin Wolf 
---
 block/crypto.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/block/crypto.c b/block/crypto.c
index e0b8856f74..bc6c7e3795 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -357,7 +357,11 @@ static int block_crypto_truncate(BlockDriverState *bs, 
int64_t offset,
 BlockCrypto *crypto = bs->opaque;
 uint64_t payload_offset =
 qcrypto_block_get_payload_offset(crypto->block);
-assert(payload_offset < (INT64_MAX - offset));
+
+if (payload_offset > INT64_MAX - offset) {
+error_setg(errp, "The requested file size is too large");
+return -EFBIG;
+}
 
 offset += payload_offset;
 
-- 
2.13.6

[Qemu-block] [PATCH for-2.12 04/12] qemu-iotests: Enable 025 for luks

[Kevin Wolf]

We want to test resizing even for luks. The only change that is needed
is to explicitly zero out new space for luks because it's undefined.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/025 | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/025 b/tests/qemu-iotests/025
index f5e672e6b3..70dd5f10aa 100755
--- a/tests/qemu-iotests/025
+++ b/tests/qemu-iotests/025
@@ -38,7 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.filter
 . ./common.pattern
 
-_supported_fmt raw qcow2 qed
+_supported_fmt raw qcow2 qed luks
 _supported_proto file sheepdog rbd nfs
 _supported_os Linux
 
@@ -62,6 +62,13 @@ length
 EOF
 _check_test_img
 
+# bdrv_truncate() doesn't zero the new space, so we need to do that explicitly.
+# We still want to test automatic zeroing for other formats even though
+# bdrv_truncate() doesn't guarantee it.
+if [ "$IMGFMT" == "luks" ]; then
+$QEMU_IO -c "write -z $small_size $((big_size - small_size))" "$TEST_IMG" 
> /dev/null
+fi
+
 echo
 echo "=== Verifying image size after reopen"
 $QEMU_IO -c "length" "$TEST_IMG"
-- 
2.13.6

[Qemu-block] [PATCH for-2.12 01/12] vdi: Change 'static' create option to 'preallocation' in QMP

[Kevin Wolf]

What static=on really does is what we call metadata preallocation for
other block drivers. While we can still change the QMP interface, make
it more consistent by using 'preallocation' for VDI, too.

This doesn't implement any new functionality, so the only supported
preallocation modes are 'off' and 'metadata' for now.

Signed-off-by: Kevin Wolf 
---
 qapi/block-core.json |  6 ++
 block/vdi.c  | 24 ++--
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5b0ad1a8b7..f6673f2b13 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3834,16 +3834,14 @@
 #
 # @file Node to create the image format on
 # @size Size of the virtual disk in bytes
-# @static   Whether to create a statically (true) or
-#   dynamically (false) allocated image
-#   (default: false, i.e. dynamic)
+# @preallocationPreallocation mode for the new image (default: off)
 #
 # Since: 2.12
 ##
 { 'struct': 'BlockdevCreateOptionsVdi',
   'data': { 'file': 'BlockdevRef',
 'size': 'size',
-'*static':  'bool' } }
+'*preallocation':   'PreallocMode' } }
 
 ##
 # @BlockdevVhdxSubformat:
diff --git a/block/vdi.c b/block/vdi.c
index d939b034c4..73c059e69d 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -728,7 +728,7 @@ static int coroutine_fn 
vdi_co_do_create(BlockdevCreateOptions *create_options,
 int ret = 0;
 uint64_t bytes = 0;
 uint32_t blocks;
-uint32_t image_type = VDI_TYPE_DYNAMIC;
+uint32_t image_type;
 VdiHeader header;
 size_t i;
 size_t bmap_size;
@@ -744,9 +744,22 @@ static int coroutine_fn 
vdi_co_do_create(BlockdevCreateOptions *create_options,
 
 /* Validate options and set default values */
 bytes = vdi_opts->size;
-if (vdi_opts->q_static) {
+
+if (!vdi_opts->has_preallocation) {
+vdi_opts->preallocation = PREALLOC_MODE_OFF;
+}
+switch (vdi_opts->preallocation) {
+case PREALLOC_MODE_OFF:
+image_type = VDI_TYPE_DYNAMIC;
+break;
+case PREALLOC_MODE_METADATA:
 image_type = VDI_TYPE_STATIC;
+break;
+default:
+error_setg(errp, "Preallocation mode not supported for vdi");
+return -EINVAL;
 }
+
 #ifndef CONFIG_VDI_STATIC_IMAGE
 if (image_type == VDI_TYPE_STATIC) {
 ret = -ENOTSUP;
@@ -874,6 +887,7 @@ static int coroutine_fn vdi_co_create_opts(const char 
*filename, QemuOpts *opts,
 BlockdevCreateOptions *create_options = NULL;
 BlockDriverState *bs_file = NULL;
 uint64_t block_size = DEFAULT_CLUSTER_SIZE;
+bool is_static = false;
 Visitor *v;
 Error *local_err = NULL;
 int ret;
@@ -895,6 +909,9 @@ static int coroutine_fn vdi_co_create_opts(const char 
*filename, QemuOpts *opts,
 goto done;
 }
 #endif
+if (qemu_opt_get_bool_del(opts, BLOCK_OPT_STATIC, false)) {
+is_static = true;
+}
 
 qdict = qemu_opts_to_qdict_filtered(opts, NULL, _create_opts, true);
 
@@ -913,6 +930,9 @@ static int coroutine_fn vdi_co_create_opts(const char 
*filename, QemuOpts *opts,
 
 qdict_put_str(qdict, "driver", "vdi");
 qdict_put_str(qdict, "file", bs_file->node_name);
+if (is_static) {
+qdict_put_str(qdict, "preallocation", "metadata");
+}
 
 /* Get the QAPI object */
 v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
-- 
2.13.6

[Qemu-block] [PATCH for-2.12 00/12] block: Follow-up for .bdrv_co_create (part 1)

[Kevin Wolf]

This series adds qemu-iotests for a few more block drivers (yet more to
come in another series) and fixes a few things that previous review and
these tests brought up.

The only major design change is that I converted the vdi block driver
from a boolean 'static' create option to the standard 'preallocation'
one that other drivers are using. This seems like a good move to make
while the interface isn't stable yet.

Kevin Wolf (12):
  vdi: Change 'static' create option to 'preallocation' in QMP
  vdi: Fix build with CONFIG_VDI_DEBUG
  qemu-iotests: Test vdi image creation with QMP
  qemu-iotests: Enable 025 for luks
  luks: Turn another invalid assertion into check
  qemu-iotests: Test invalid resize on luks
  parallels: Check maximum cluster size on create
  qemu-iotests: Test parallels image creation with QMP
  vhdx: Require power-of-two block size on create
  vhdx: Don't use error_setg_errno() with constant errno
  vhdx: Check for 4 GB maximum log size on creation
  qemu-iotests: Test vhdx image creation with QMP

 qapi/block-core.json   |   6 +-
 block/crypto.c |   6 +-
 block/parallels.c  |   5 +
 block/vdi.c|  46 --
 block/vhdx.c   |  17 ++-
 tests/qemu-iotests/025 |   9 +-
 tests/qemu-iotests/210 |  37 +
 tests/qemu-iotests/211 | 256 +
 tests/qemu-iotests/211.out |  98 +
 tests/qemu-iotests/212 | 326 ++
 tests/qemu-iotests/212.out | 111 ++
 tests/qemu-iotests/213 | 349 +
 tests/qemu-iotests/213.out | 121 
 tests/qemu-iotests/group   |   3 +
 14 files changed, 1365 insertions(+), 25 deletions(-)
 create mode 100755 tests/qemu-iotests/211
 create mode 100644 tests/qemu-iotests/211.out
 create mode 100755 tests/qemu-iotests/212
 create mode 100644 tests/qemu-iotests/212.out
 create mode 100755 tests/qemu-iotests/213
 create mode 100644 tests/qemu-iotests/213.out

-- 
2.13.6

Re: [Qemu-block] [Qemu-devel] [PATCH v3 15/16] block/mirror: Add copy mode QAPI interface

[Eric Blake]

On 02/28/2018 12:05 PM, Max Reitz wrote:

This patch allows the user to specify whether to use active or only
background mode for mirror block jobs.  Currently, this setting will
remain constant for the duration of the entire block job.

Signed-off-by: Max Reitz 
---
  qapi/block-core.json  | 11 +--
  include/block/block_int.h |  4 +++-
  block/mirror.c| 12 +++-
  blockdev.c|  9 -
  4 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index c73b769c27..1186c007ae 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1573,6 +1573,9 @@
  # written. Both will result in identical contents.
  # Default is true. (Since 2.4)
  #
+# @copy-mode: when to copy data to the destination; defaults to 'background'
+# (Since: 2.12)
+#


Are we still aiming for 2.12, or is this a feature which missed 
softfreeze and should therefore be 2.13?


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [Qemu-devel] [PATCH] qemu: include generated files with <> and not ""

[Daniel P . Berrangé]

On Tue, Mar 20, 2018 at 07:10:42PM +0200, Michael S. Tsirkin wrote:
> On Tue, Mar 20, 2018 at 05:33:42PM +0100, Stefan Weil wrote:
> > Using <> for system include files and "" for local include files is a
> > convention, and as far as I know most projects adhere to that
> > convention. So does QEMU currently. Such conventions are not only
> > important for humans, but also for tools. There are more tools than the
> > C preprocessor which handle <> and "" differently. For example the GNU
> > compiler uses -MD or -MMD to automatically generate dependency rules for
> > make. While -MD generates dependencies to all include files, -MMD does
> > so only for user include files, but not for system include files. "user"
> > and "system" means the different forms how include statements are
> > written. QEMU still seems to use -MMD:
> > 
> > rules.mak:QEMU_DGFLAGS += -MMD -MP -MT $@ -MF $(@D)/$(*F).d
> 
> To my knowledge, and according to my limited testing,
> system headers in this context means
> the default ones not supplied with -I.

GCC's definition of system header is here:

  https://gcc.gnu.org/onlinedocs/cpp/System-Headers.html


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: [Qemu-block] [Qemu-devel] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 05:33:42PM +0100, Stefan Weil wrote:
> Using <> for system include files and "" for local include files is a
> convention, and as far as I know most projects adhere to that
> convention. So does QEMU currently. Such conventions are not only
> important for humans, but also for tools. There are more tools than the
> C preprocessor which handle <> and "" differently. For example the GNU
> compiler uses -MD or -MMD to automatically generate dependency rules for
> make. While -MD generates dependencies to all include files, -MMD does
> so only for user include files, but not for system include files. "user"
> and "system" means the different forms how include statements are
> written. QEMU still seems to use -MMD:
> 
> rules.mak:QEMU_DGFLAGS += -MMD -MP -MT $@ -MF $(@D)/$(*F).d

To my knowledge, and according to my limited testing,
system headers in this context means
the default ones not supplied with -I.

If I had to guess, that is the definition most other tools
are likely to use.


>
> Other tools like static code analysers could restrict their warning and
> error messages to user include files and ignore problems in system
> include files.

Could you give some exacmples of tools like that?

IMHO they would not work correctly if they did not treat
include directives the way compiler does.

C standard is pretty explicit that the only difference
is extra directories searched:

A preprocessing directive of the form
# include "q-char-sequence" new-line
causes the replacement of that directive by the entire contents of the source 
file identified
by the specified sequence between the " delimiters. The named source file is 
searched
for in an implementation-defined manner. If this search is not supported, or if 
the search
fails, the directive is reprocessed as if it read
# include  new-line
with the identical contained sequence (including > characters, if any) from the 
original
directive.



> Very large projects often split in sub projects, maybe one of them
> describing the API. Then that API headers are similar to system headers
> and can be included using <>, although they still belong to the same
> larger project. Do we have a stable QEMU API described in a (small)
> number of include files which typically do not change? If yes, then
> those include files could be included using <> because we don't need
> them in dependency lists or in static code analysis reports.
> 
> For all other QEMU include files, I'd stick to using "".
> 
> Regards
> Stefan

Most people know that system headers are the ones in /usr/include
The distinction that they are pulled in with include ""
is a QEMU construct.

If we want to be able to distinguish between internal and
external headers, the standard way to do it
in C is by prefixing the names with qemu/ qemu- or qemu_.

In fact we kind of already do this - if you see a name with
a slash in there you can be pretty sure it's internal
to qemu.

Exceptions are elf.h glib-compat.h and the generated trace.h.

Let's
mv include/* include/qemu/ ?

-- 
MST

Re: [Qemu-block] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 02:54:37PM +0100, Max Reitz wrote:
> But I guess the main advantage with using this rule I see is that it's
> better for people reading the code.  It's just nice to know whether a
> file belongs to qemu or not by just looking at the #include statement.
> (Note that this implies that it is indeed more difficult to determine
> whether a header belongs to qemu than whether it sits in the same
> directory as the C file, though!)  So I think the old (current) rule is
> better for reading code, Michael's rule would be better for writing
> code.  I think reading code is what should be easier.

How about prefixing all internal headers with qemu/ ?

That's a way to avoid namespace collisions that is actually standard.

-- 
MST 

[Qemu-block] [PATCH v4 2/3] qcow2: fix bitmaps loading when bitmaps already exist

[Vladimir Sementsov-Ogievskiy]

On reopen with existing bitmaps, instead of loading bitmaps, lets
reopen them if needed. This also fixes bitmaps migration through
shared storage.
Consider the case. Persistent bitmaps are stored on bdrv_inactivate.
Then, on destination process_incoming_migration_bh() calls
bdrv_invalidate_cache_all() which leads to
qcow2_load_autoloading_dirty_bitmaps() which fails if bitmaps are
already loaded on destination start.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.c | 17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 7472af6931..d2d129a2e6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1480,7 +1480,22 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
*bs, QDict *options,
 s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
 }
 
-if (qcow2_load_dirty_bitmaps(bs, _err)) {
+if (bdrv_dirty_bitmap_next(bs, NULL)) {
+/* It's some kind of reopen with already existing dirty bitmaps. There
+ * are no known cases where we need loading bitmaps in such situation,
+ * so it's safer don't load them.
+ *
+ * Moreover, if we have some readonly bitmaps and we are reopening for
+ * rw we should reopen bitmaps correspondingly.
+ */
+if (bdrv_has_readonly_bitmaps(bs) &&
+!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE))
+{
+bool header_updated = false;
+qcow2_reopen_bitmaps_rw_hint(bs, _updated, _err);
+update_header = update_header && !header_updated;
+}
+} else if (qcow2_load_dirty_bitmaps(bs, _err)) {
 update_header = false;
 }
 if (local_err != NULL) {
-- 
2.11.1

[Qemu-block] [PATCH v4 2/3] qcow2: handle reopening bitmaps on bdrv_invalidate_cache

[Vladimir Sementsov-Ogievskiy]

On reopen with existing bitmaps, instead of loading bitmaps, lets
reopen them if needed. This also fixes bitmaps migration through
shared storage.
Consider the case. Persistent bitmaps are stored on bdrv_inactivate.
Then, on destination process_incoming_migration_bh() calls
bdrv_invalidate_cache_all() which leads to
qcow2_load_autoloading_dirty_bitmaps() which fails if bitmaps are
already loaded on destination start.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.c | 17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 7472af6931..d2d129a2e6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1480,7 +1480,22 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
*bs, QDict *options,
 s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
 }
 
-if (qcow2_load_dirty_bitmaps(bs, _err)) {
+if (bdrv_dirty_bitmap_next(bs, NULL)) {
+/* It's some kind of reopen with already existing dirty bitmaps. There
+ * are no known cases where we need loading bitmaps in such situation,
+ * so it's safer don't load them.
+ *
+ * Moreover, if we have some readonly bitmaps and we are reopening for
+ * rw we should reopen bitmaps correspondingly.
+ */
+if (bdrv_has_readonly_bitmaps(bs) &&
+!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE))
+{
+bool header_updated = false;
+qcow2_reopen_bitmaps_rw_hint(bs, _updated, _err);
+update_header = update_header && !header_updated;
+}
+} else if (qcow2_load_dirty_bitmaps(bs, _err)) {
 update_header = false;
 }
 if (local_err != NULL) {
-- 
2.11.1

[Qemu-block] [PATCH v4 1/3] qcow2-bitmap: add qcow2_reopen_bitmaps_rw_hint()

[Vladimir Sementsov-Ogievskiy]

Add version of qcow2_reopen_bitmaps_rw, which do the same work but
also return a hint about was header updated or not. This will be
used in the following fix for bitmaps reloading after migration.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Reviewed-by: Max Reitz 
---
 block/qcow2.h|  2 ++
 block/qcow2-bitmap.c | 15 ++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index ccb92a9696..d301f77cea 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -671,6 +671,8 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, 
BdrvCheckResult *res,
   void **refcount_table,
   int64_t *refcount_table_size);
 bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
+ Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 3010adb909..6e93ec43e1 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1004,7 +1004,8 @@ fail:
 return false;
 }
 
-int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
+int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
+ Error **errp)
 {
 BDRVQcow2State *s = bs->opaque;
 Qcow2BitmapList *bm_list;
@@ -1012,6 +1013,10 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error 
**errp)
 GSList *ro_dirty_bitmaps = NULL;
 int ret = 0;
 
+if (header_updated != NULL) {
+*header_updated = false;
+}
+
 if (s->nb_bitmaps == 0) {
 /* No bitmaps - nothing to do */
 return 0;
@@ -1055,6 +1060,9 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error 
**errp)
 error_setg_errno(errp, -ret, "Can't update bitmap directory");
 goto out;
 }
+if (header_updated != NULL) {
+*header_updated = true;
+}
 g_slist_foreach(ro_dirty_bitmaps, set_readonly_helper, false);
 }
 
@@ -1065,6 +1073,11 @@ out:
 return ret;
 }
 
+int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
+{
+return qcow2_reopen_bitmaps_rw_hint(bs, NULL, errp);
+}
+
 /* store_bitmap_data()
  * Store bitmap to image, filling bitmap table accordingly.
  */
-- 
2.11.1

Re: [Qemu-block] [PATCH v4 2/3] qcow2: handle reopening bitmaps on bdrv_invalidate_cache DROP IT

[Vladimir Sementsov-Ogievskiy]

Oh, sorry, just drop it.
(the only difference is commit message subject)

20.03.2018 20:05, Vladimir Sementsov-Ogievskiy wrote:

On reopen with existing bitmaps, instead of loading bitmaps, lets
reopen them if needed. This also fixes bitmaps migration through
shared storage.
Consider the case. Persistent bitmaps are stored on bdrv_inactivate.
Then, on destination process_incoming_migration_bh() calls
bdrv_invalidate_cache_all() which leads to
qcow2_load_autoloading_dirty_bitmaps() which fails if bitmaps are
already loaded on destination start.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/qcow2.c | 17 -
  1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 7472af6931..d2d129a2e6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1480,7 +1480,22 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
*bs, QDict *options,
  s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
  }
  
-if (qcow2_load_dirty_bitmaps(bs, _err)) {

+if (bdrv_dirty_bitmap_next(bs, NULL)) {
+/* It's some kind of reopen with already existing dirty bitmaps. There
+ * are no known cases where we need loading bitmaps in such situation,
+ * so it's safer don't load them.
+ *
+ * Moreover, if we have some readonly bitmaps and we are reopening for
+ * rw we should reopen bitmaps correspondingly.
+ */
+if (bdrv_has_readonly_bitmaps(bs) &&
+!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE))
+{
+bool header_updated = false;
+qcow2_reopen_bitmaps_rw_hint(bs, _updated, _err);
+update_header = update_header && !header_updated;
+}
+} else if (qcow2_load_dirty_bitmaps(bs, _err)) {
  update_header = false;
  }
  if (local_err != NULL) {



--
Best regards,
Vladimir

[Qemu-block] [PATCH v4 for 2.12 0/3] fix bitmaps migration through shared storage

[Vladimir Sementsov-Ogievskiy]

Hi all.

This fixes bitmaps migration through shared storage. Look at 02 for
details.

The bug introduced in 2.10 with the whole qcow2 bitmaps feature, so
qemu-stable in CC. However I doubt that someone really suffered from this.

Do we need dirty bitmaps at all in inactive case? - that was a question in v2.
And, keeping in mind that we are going to use inactive mode not only for
incoming migration, I'm not sure that answer is NO (but, it may be "NO" for 
2.10, 2.11), so let's fix it in proposed here manner at least for 2.12.

v4:

01: r-b: Max
02: rewrite to do not load bitmaps if any bitmaps alredy exit, drop r-b
03: add missed change of 169.out

v3: tiny context changes in 01,02
03: instead of a separate test, enable corresponding case in existent one

v2:
   John, thank you for reviewing v1.
   changes:
add John's r-bs, change s/timeout=10/timeout=10.0/ in last patch
and drop old 03 patch, related to this timeout fix.

Vladimir Sementsov-Ogievskiy (3):
  qcow2-bitmap: add qcow2_reopen_bitmaps_rw_hint()
  qcow2: fix bitmaps loading when bitmaps already exist
  iotests: enable shared migration cases in 169

 block/qcow2.h  |  2 ++
 block/qcow2-bitmap.c   | 15 ++-
 block/qcow2.c  | 17 -
 tests/qemu-iotests/169 |  8 +++-
 tests/qemu-iotests/169.out |  4 ++--
 5 files changed, 37 insertions(+), 9 deletions(-)

-- 
2.11.1

[Qemu-block] [PATCH v4 3/3] iotests: enable shared migration cases in 169

[Vladimir Sementsov-Ogievskiy]

Shared migration for dirty bitmaps is fixed by previous patches,
so we can enable the test.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/169 | 8 +++-
 tests/qemu-iotests/169.out | 4 ++--
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169
index 3a8db91f6f..153b10b6e7 100755
--- a/tests/qemu-iotests/169
+++ b/tests/qemu-iotests/169
@@ -140,16 +140,14 @@ def inject_test_case(klass, name, method, *args, 
**kwargs):
 mc = operator.methodcaller(method, *args, **kwargs)
 setattr(klass, 'test_' + name, new.instancemethod(mc, None, klass))
 
-for cmb in list(itertools.product((True, False), repeat=3)):
+for cmb in list(itertools.product((True, False), repeat=4)):
 name = ('_' if cmb[0] else '_not_') + 'persistent_'
 name += ('_' if cmb[1] else '_not_') + 'migbitmap_'
 name += '_online' if cmb[2] else '_offline'
-
-# TODO fix shared-storage bitmap migration and enable cases for it
-args = list(cmb) + [False]
+name += '_shared' if cmb[3] else '_nonshared'
 
 inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration',
- *args)
+ *list(cmb))
 
 
 if __name__ == '__main__':
diff --git a/tests/qemu-iotests/169.out b/tests/qemu-iotests/169.out
index 594c16f49f..b6f257674e 100644
--- a/tests/qemu-iotests/169.out
+++ b/tests/qemu-iotests/169.out
@@ -1,5 +1,5 @@
-
+
 --
-Ran 8 tests
+Ran 16 tests
 
 OK
-- 
2.11.1

Re: [Qemu-block] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 11:12:00AM -0500, Eric Blake wrote:
> 
> Why can't we fix Makefile to include BOTH the build and the source
> directories (to pick up generated files first, and then version-controlled
> files), and possibly include logic to simplify to a single -I instead of two
> when doing in-tree builds?

That's the issue.  Whatever you do, #include "" pulls in the current
directory first.

-- 
MST

Re: [Qemu-block] [PATCH] qemu: include generated files with <> and not ""

[Daniel P . Berrangé]

On Tue, Mar 20, 2018 at 11:12:00AM -0500, Eric Blake wrote:
> On 03/19/2018 08:54 PM, Michael S. Tsirkin wrote:
> > QEMU coding style at the moment asks for all non-system
> > include files to be used with #include "foo.h".
> 
> [I'm replying without having read the rest of the thread, so bear with me if
> I repeat some of the other comments that have already been made]
> 
> And Markus even just did a cleanup along those lines.
> 
> > However this rule actually does not make sense and
> > creates issues for when the included file is generated.
> > 
> > In C, include "file" means look in current directory,
> > then on include search path. Current directory here
> > means the source file directory.
> > By comparison include  means look on include search path.
> 
> It's also nice when "file" means file belonging to our project, and 
> means 3rd-party file.  So we have to choose which semantics are easier;
> perhaps better Makefile rules that prevent us from seeing stale files is a
> better solution than figuring out which files are generated.

That's what I've attempted here:

  https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg05421.html


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: [Qemu-block] [Qemu-devel] [PATCH] qemu: include generated files with <> and not ""

[Stefan Weil]

Am 20.03.2018 um 02:54 schrieb Michael S. Tsirkin:
> QEMU coding style at the moment asks for all non-system
> include files to be used with #include "foo.h".
> However this rule actually does not make sense and
> creates issues for when the included file is generated.
> 
> In C, include "file" means look in current directory,
> then on include search path. Current directory here
> means the source file directory.
> By comparison include  means look on include search path.


Hello Michael,

the compiler (or to be more precise the preprocessor) works as you
describe it, so technically all include files which are only found by
searching the include paths would be found faster with <> than with "":
the unsuccessful search relative to the source file directory would be
omitted. Practically this will not save much build time because of
caching effects. So the suggested change is not required to get faster
build speed.

Using <> for system include files and "" for local include files is a
convention, and as far as I know most projects adhere to that
convention. So does QEMU currently. Such conventions are not only
important for humans, but also for tools. There are more tools than the
C preprocessor which handle <> and "" differently. For example the GNU
compiler uses -MD or -MMD to automatically generate dependency rules for
make. While -MD generates dependencies to all include files, -MMD does
so only for user include files, but not for system include files. "user"
and "system" means the different forms how include statements are
written. QEMU still seems to use -MMD:

rules.mak:QEMU_DGFLAGS += -MMD -MP -MT $@ -MF $(@D)/$(*F).d

Other tools like static code analysers could restrict their warning and
error messages to user include files and ignore problems in system
include files.

Very large projects often split in sub projects, maybe one of them
describing the API. Then that API headers are similar to system headers
and can be included using <>, although they still belong to the same
larger project. Do we have a stable QEMU API described in a (small)
number of include files which typically do not change? If yes, then
those include files could be included using <> because we don't need
them in dependency lists or in static code analysis reports.

For all other QEMU include files, I'd stick to using "".

Regards
Stefan

Re: [Qemu-block] [PATCH v3 2/3] qcow2: handle reopening bitmaps on bdrv_invalidate_cache

[Vladimir Sementsov-Ogievskiy]

20.03.2018 16:44, Max Reitz wrote:

On 2018-03-19 10:02, Vladimir Sementsov-Ogievskiy wrote:

Consider migration with shared storage. Persistent bitmaps are stored
on bdrv_inactivate. Then, on destination
process_incoming_migration_bh() calls bdrv_invalidate_cache_all() which
leads to qcow2_load_autoloading_dirty_bitmaps() which fails if bitmaps
are already loaded on destination start. In this case we should call
qcow2_reopen_bitmaps_rw instead.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
---
  block/qcow2.c | 8 +++-
  1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 7472af6931..6219666d4a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1480,7 +1480,13 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
*bs, QDict *options,
  s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
  }
  
-if (qcow2_load_dirty_bitmaps(bs, _err)) {

+if (bdrv_has_readonly_bitmaps(bs)) {

So if there are any read-only bitmaps, we'll skip
qcow2_load_dirty_bitmaps() altogether.  That doesn't seem so bad because
qcow2_load_dirty_bitmaps() seems to assume no bitmaps have been loaded yet.

But if that's the case, shouldn't we skip that function if any bitmap
has been loaded already, RO or R/W?  (And we can call
qcow2_reopen_bitmaps_rw_hint() even if there aren't any RO bitmaps, it
just won't do anything then.)

This doesn't make this patch really wrong, I'm just wondering whether it
can do better.  (To add to that, I don't even know whether there is a
case where qcow2_do_open() would be called with any R/W bitmaps already
present.)

Max


hm reasonable. So, we don't know about such cases, it's better to don't 
allow them.





+if (!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) 
{
+bool header_updated = false;
+qcow2_reopen_bitmaps_rw_hint(bs, _updated, _err);
+update_header = update_header && !header_updated;
+}
+} else if (qcow2_load_dirty_bitmaps(bs, _err)) {
  update_header = false;
  }
  if (local_err != NULL) {




--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v3 3/3] iotests: enable shared migration cases in 169

[Vladimir Sementsov-Ogievskiy]

20.03.2018 17:01, Max Reitz wrote:

On 2018-03-19 10:02, Vladimir Sementsov-Ogievskiy wrote:

Shared migration for dirty bitmaps is fixed by previous patches,
so we can enable the test.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  tests/qemu-iotests/169 | 8 +++-
  1 file changed, 3 insertions(+), 5 deletions(-)

Isn't this missing some changes to 169.out (because the number of tests
is doubled)?

Max


Oh, yes, forget to add it.

--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH] qemu: include generated files with <> and not ""

[Eric Blake]

On 03/19/2018 08:54 PM, Michael S. Tsirkin wrote:

QEMU coding style at the moment asks for all non-system
include files to be used with #include "foo.h".


[I'm replying without having read the rest of the thread, so bear with 
me if I repeat some of the other comments that have already been made]


And Markus even just did a cleanup along those lines.


However this rule actually does not make sense and
creates issues for when the included file is generated.

In C, include "file" means look in current directory,
then on include search path. Current directory here
means the source file directory.
By comparison include  means look on include search path.


It's also nice when "file" means file belonging to our project, and 
 means 3rd-party file.  So we have to choose which semantics are 
easier; perhaps better Makefile rules that prevent us from seeing stale 
files is a better solution than figuring out which files are generated.




As generated files are not in the search directory (unless the build
directory happens to match the source directory),


Why can't we fix Makefile to include BOTH the build and the source 
directories (to pick up generated files first, and then 
version-controlled files), and possibly include logic to simplify to a 
single -I instead of two when doing in-tree builds?



it does not make sense
to include them with "" - doing so is merely more work for preprocessor
and a source or errors if a stale file happens to exist in the source
directory.

This changes include directives for all generated files, across the
tree. The idea is to avoid sending a huge amount of email.  But when
merging, the changes will be split with one commit per file, e.g. for
ease of bisect in case of build failures, and to ease merging.

Note that should some generated files be missed by this tree-wide
refactoring, it isn't a big deal - this merely maintains the status quo,
and this can be addressed by a separate patch on top.


I'm not sure I'm a fan of this patch yet, if there may be other 
alternatives that solve the real issue of stale generated files breaking 
incremental builds.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] luks: Initial content of unwritten blocks

[Daniel P . Berrangé]

On Tue, Mar 20, 2018 at 04:04:21PM +0100, Kevin Wolf wrote:
> Hi,
> 
> I just tried to add luks support to qemu-iotests 025, a basic resize
> test, which made me realise that luks doesn't read zeros in unwritten
> blocks (which makes the test fail).
> 
> The reason for this is that we get zeros on the protocol layer (i.e. in
> the encrpyted data as it is on the disk), but not in the decrpyted
> format layer.

That is basically the same that you would see with dm-crypt luks impl
over a physical device. ie if /dev/sda1 is filled with zeros (or sparse
such that reading from unallocated sectors returns zeros), and you then
format it with luks, and then read data you'll not get zeros - you'll
get the decrypted zeros which is essentially random garbage. The user
would have to make a concious decision to fill the disk with all zeros
in the cipher text if they want their newly formatted luks voilume
to return all zeros.

> Now I'm wondering if that's a problem. Not reading zeros in the guest is
> unusual, but not unheard of (e.g. host_device), but visible zeros in the
> image file could be considered an information leak. On the other hand,
> qcow2 metadata for encrypted images is visible, too, so that's really
> just the same thing.

Leaking information to the guest on whether a sector in a qcow2 file is
allocated or unallocated, might be considered undesirable, but I think
that's one of the tradeoffs of using qcow2 in general not just when
luks is enabled in qcow.

> What do you think, is the current behaviour good enough or should we
> essentially enforce full preallocation for luks images and initialise
> the image so that zeros are visible on the format layer, but random
> encrypted stuff on the protocol layer?

I don't think that's neccessary, as long as users have the option to
opt-in to do that, which they do.

The more important missing piece of the puzzle currently is secure
delete. We need to provide a 'qemu-img rm' command for deleting
volumes that would explicitly overwrite the LUKS headers and key
material, rather than having apps just run plain 'rm'.

> Either way, we should probably document the decision somewhere as
> intentional.

Agreed.


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 :|

[Qemu-block] luks: Initial content of unwritten blocks

[Kevin Wolf]

Hi,

I just tried to add luks support to qemu-iotests 025, a basic resize
test, which made me realise that luks doesn't read zeros in unwritten
blocks (which makes the test fail).

The reason for this is that we get zeros on the protocol layer (i.e. in
the encrpyted data as it is on the disk), but not in the decrpyted
format layer.

Now I'm wondering if that's a problem. Not reading zeros in the guest is
unusual, but not unheard of (e.g. host_device), but visible zeros in the
image file could be considered an information leak. On the other hand,
qcow2 metadata for encrypted images is visible, too, so that's really
just the same thing.

What do you think, is the current behaviour good enough or should we
essentially enforce full preallocation for luks images and initialise
the image so that zeros are visible on the format layer, but random
encrypted stuff on the protocol layer?

Either way, we should probably document the decision somewhere as
intentional.

Kevin

Re: [Qemu-block] [PATCH v1 1/1] iotests: fix test case 185

[Stefan Hajnoczi]

On Tue, Mar 20, 2018 at 11:11:01AM +0100, Kevin Wolf wrote:
> Am 19.03.2018 um 18:53 hat Christian Borntraeger geschrieben:
> > 
> > 
> > On 03/19/2018 05:10 PM, Stefan Hajnoczi wrote:
> > > On Mon, Mar 19, 2018 at 9:35 AM, QingFeng Hao  
> > > wrote:
> > >> Test case 185 failed since commit 4486e89c219 --- "vl: introduce 
> > >> vm_shutdown()".
> > >> It's because of the newly introduced function vm_shutdown calls 
> > >> bdrv_drain_all,
> > >> which is called later by bdrv_close_all. bdrv_drain_all resumes the jobs
> > >> that doubles the speed and offset is doubled.
> > >> Some jobs' status are changed as well.
> > >>
> > >> Thus, let's not call bdrv_drain_all in vm_shutdown.
> > >>
> > >> Signed-off-by: QingFeng Hao 
> > >> ---
> > >>  cpus.c | 5 +++--
> > >>  1 file changed, 3 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/cpus.c b/cpus.c
> > >> index 2e6701795b..ae2962508c 100644
> > >> --- a/cpus.c
> > >> +++ b/cpus.c
> > >> @@ -1006,8 +1006,9 @@ static int do_vm_stop(RunState state, bool 
> > >> send_stop)
> > >>  qapi_event_send_stop(_abort);
> > >>  }
> > >>  }
> > >> -
> > >> -bdrv_drain_all();
> > >> +if (send_stop) {
> > >> +bdrv_drain_all();
> > >> +}
> > > 
> > > Thanks for looking into this bug!
> > > 
> > > This if statement breaks the contract of the function when send_stop
> > > is false.  Drain ensures that pending I/O completes and that device
> > > emulation finishes before this function returns.  This is important
> > > for live migration, where there must be no more guest-related activity
> > > after vm_stop().
> > > 
> > > This patch relies on the caller invoking bdrv_close_all() immediately
> > > after do_vm_stop(), which is not documented and could lead to more
> > > bugs in the future.
> > > 
> > > I suggest a different solution:
> > > 
> > > Test 185 relies on internal QEMU behavior which can change from time
> > > to time.  Buffer sizes and block job iteration counts are
> > > implementation details that are not part of qapi-schema.json or other
> > > documented behavior.
> > > 
> > > In fact, the existing test case was already broken in IOThread mode
> > > since iothread_stop_all() -> bdrv_set_aio_context() also includes a
> > > bdrv_drain() with the side-effect of an extra blockjob iteration.
> > > 
> > > After 4486e89c219 ("vl: introduce vm_shutdown()") both IOThread and
> > > non-IOThread mode perform the drain.  This has caused the test
> > > failure.
> > > 
> > > Instead of modifying QEMU to keep the same arbitrary internal behavior
> > > as before, please send a patch that updates tests/qemu-iotests/185.out
> > > with the latest output.
> > 
> > Wouldnt it be better if the test actually masks out the values the are not
> > really part of the "agreed upon" behaviour? Wouldnt block_job_offset from
> > common.filter be what we want?
> 
> The test case has the following note:
> 
> # Note that the reference output intentionally includes the 'offset' field in
> # BLOCK_JOB_CANCELLED events for all of the following block jobs. They are
> # predictable and any change in the offsets would hint at a bug in the job
> # throttling code.
> 
> Now the question is if this particular change is okay rather than
> hinting at a bug and we should update the reference output or whether we
> need to change qemu again.
> 
> I think the change isn't really bad, but we are doing more useless work
> now than we used to do before, and we're exiting slower because we're
> spawning additional I/O that we have to wait for. So the better state
> was certainly the old one.

Here is the reason for the additional I/O and how it could be
eliminated:

child_job_drained_begin() pauses and child_job_drained_end() resumes the
job.  Resuming the job cancels the timer (if pending) and enters the
blockjob's coroutine.  This is why draining BlockDriverState forces an
extra iteration of the blockjob.

The current behavior is intended for the case where the job has I/O
pending at child_job_drained_begin() time.  When the I/O completes, the
job will see it must pause and it will yield at a pause point.  It makes
sense for child_job_drained_end() to resume the job so it can start I/O
again.

In our case this behavior doesn't make sense though.  The job already
yielded before child_job_drained_begin() and it doesn't need to resume
at child_job_drained_end() time.  We'd prefer to wait for the timer
expiry.

We need to distinguish these two cases to avoid resuming the job in the
latter case.  I think this would be the proper way to avoid unnecessary
blockjob activity across drain.

I favor updating the test output though, because the code change adds
complexity and the practical benefit is not obvious to me.

QingFeng, if you decide to modify blockjobs, please CC Jeffrey Cody
 and I'd also be happy to review the patch.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH] qemu: include generated files with <> and not ""

[Daniel P . Berrangé]

On Tue, Mar 20, 2018 at 03:50:02PM +0200, Michael S. Tsirkin wrote:
> On Tue, Mar 20, 2018 at 01:41:17PM +, Daniel P. Berrangé wrote:
> > On Tue, Mar 20, 2018 at 02:32:16PM +0100, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > > > So for these, we should use "".  None of these are generated files 
> > > > > though.
> > > > 
> > > > That leads to crazy inconsistent message for developers where 50% of 
> > > > QEMU
> > > > header files must use <> and the other 50% of header files must use "".
> > > 
> > > The rules are pretty simple though:
> > > 
> > >(1) Headers which are generated use <>.
> > >(2) Headers which are in include/ use <>.
> > >(3) Headers sitting in the same directory as the source files use "".
> > 
> > We have 1200 header files in QEMU - a developer might just reasonably 
> > remember
> > which header file is a QEMU header, vs which is a 3rd party system header.
> > Expecting devs to remember which of those 3 buckets each QEMU header falls
> > under is unreasonable IMHO. 
> 
> That's the C language for you though.  For better or worse, these are
> the rules that K came up with.

No one seriously tries to keep compliance with original K C these days,
things have moved on massively since then.

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: [Qemu-block] [PATCH] qemu: include generated files with <> and not ""

[Daniel P . Berrangé]

On Tue, Mar 20, 2018 at 02:32:16PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > > So for these, we should use "".  None of these are generated files though.
> > 
> > That leads to crazy inconsistent message for developers where 50% of QEMU
> > header files must use <> and the other 50% of header files must use "".
> 
> The rules are pretty simple though:
> 
>(1) Headers which are generated use <>.
>(2) Headers which are in include/ use <>.
>(3) Headers sitting in the same directory as the source files use "".

We have 1200 header files in QEMU - a developer might just reasonably remember
which header file is a QEMU header, vs which is a 3rd party system header.
Expecting devs to remember which of those 3 buckets each QEMU header falls
under is unreasonable IMHO. 

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: [Qemu-block] [PATCH] qemu: include generated files with <> and not ""

[Gerd Hoffmann]

  Hi,

> > So for these, we should use "".  None of these are generated files though.
> 
> That leads to crazy inconsistent message for developers where 50% of QEMU
> header files must use <> and the other 50% of header files must use "".

The rules are pretty simple though:

   (1) Headers which are generated use <>.
   (2) Headers which are in include/ use <>.
   (3) Headers sitting in the same directory as the source files use "".

Other cases should not exist (when headers from another source directory
are included they should most likely be moved to include/).

cheers,
  Gerd

Re: [Qemu-block] [PATCH v3 1/3] qcow2-bitmap: add qcow2_reopen_bitmaps_rw_hint()

[Max Reitz]

On 2018-03-19 10:02, Vladimir Sementsov-Ogievskiy wrote:
> Add version of qcow2_reopen_bitmaps_rw, which do the same work but
> also return a hint about was header updated or not. This will be
> used in the following fix for bitmaps reloading after migration.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> Reviewed-by: John Snow 
> ---
>  block/qcow2.h|  2 ++
>  block/qcow2-bitmap.c | 15 ++-
>  2 files changed, 16 insertions(+), 1 deletion(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 01:41:17PM +, Daniel P. Berrangé wrote:
> On Tue, Mar 20, 2018 at 02:32:16PM +0100, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > > So for these, we should use "".  None of these are generated files 
> > > > though.
> > > 
> > > That leads to crazy inconsistent message for developers where 50% of QEMU
> > > header files must use <> and the other 50% of header files must use "".
> > 
> > The rules are pretty simple though:
> > 
> >(1) Headers which are generated use <>.
> >(2) Headers which are in include/ use <>.
> >(3) Headers sitting in the same directory as the source files use "".
> 
> We have 1200 header files in QEMU - a developer might just reasonably remember
> which header file is a QEMU header, vs which is a 3rd party system header.
> Expecting devs to remember which of those 3 buckets each QEMU header falls
> under is unreasonable IMHO. 
> 
> Regards,
> Daniel

That's the C language for you though.  For better or worse, these are
the rules that K came up with.


> -- 
> |: 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: [Qemu-block] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 01:58:32PM +, Daniel P. Berrangé wrote:
> > That's the C language for you though.  For better or worse, these are
> > the rules that K came up with.
> 
> No one seriously tries to keep compliance with original K C these days,
> things have moved on massively since then.

Not where the preprocessor is involved.

> 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: [Qemu-block] [PATCH] qemu: include generated files with <> and not ""

[Thomas Huth]

On 20.03.2018 14:32, Gerd Hoffmann wrote:
>   Hi,
> 
>>> So for these, we should use "".  None of these are generated files though.
>>
>> That leads to crazy inconsistent message for developers where 50% of QEMU
>> header files must use <> and the other 50% of header files must use "".
> 
> The rules are pretty simple though:
> 
>(1) Headers which are generated use <>.
>(2) Headers which are in include/ use <>.
>(3) Headers sitting in the same directory as the source files use "".

Ugh, no. Please don't. The normal way of including header files in QEMU
is to use "" - also for headers that are not in the same directory. For
example just do a

 grep -r '^#include.*hw/' hw/

from the top directory and you'll see what I mean. Changing that rule is
crazy. So please, let's just fix the configure script to detect some
more stale files in the source tree, and we're done.

 Thomas

Re: [Qemu-block] [PATCH v3 3/3] iotests: enable shared migration cases in 169

[Max Reitz]

On 2018-03-19 10:02, Vladimir Sementsov-Ogievskiy wrote:
> Shared migration for dirty bitmaps is fixed by previous patches,
> so we can enable the test.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  tests/qemu-iotests/169 | 8 +++-
>  1 file changed, 3 insertions(+), 5 deletions(-)

Isn't this missing some changes to 169.out (because the number of tests
is doubled)?

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH] qemu: include generated files with <> and not ""

[Max Reitz]

On 2018-03-20 14:41, Daniel P. Berrangé wrote:
> On Tue, Mar 20, 2018 at 02:32:16PM +0100, Gerd Hoffmann wrote:
>>   Hi,
>>
 So for these, we should use "".  None of these are generated files though.
>>>
>>> That leads to crazy inconsistent message for developers where 50% of QEMU
>>> header files must use <> and the other 50% of header files must use "".
>>
>> The rules are pretty simple though:
>>
>>(1) Headers which are generated use <>.
>>(2) Headers which are in include/ use <>.
>>(3) Headers sitting in the same directory as the source files use "".
> 
> We have 1200 header files in QEMU - a developer might just reasonably remember
> which header file is a QEMU header, vs which is a 3rd party system header.
> Expecting devs to remember which of those 3 buckets each QEMU header falls
> under is unreasonable IMHO. 

I agree with this in principle, but I don't find it unreasonable for
someone to look up whether a header file exists in the same directory.
I find that much simpler than to look up whether it is a system header,
actually.

And I have to agree with Michael that his rule when to use "" (if the
file is in the same directory) and when to use <> (otherwise) is
actually what every C developer might do by instinct.  (I guess it's
also easier to check in a script than the original guideline?)

However, I also think that if any problem arises because "" was used for
a generated file and that then uses a stale file, the bug is somewhere
else.  And I think that while Michael's proposal is the more intuitive
(C) way, it is a tiny bit more complicated to explain than 'Use "" for
qemu headers, <> for everything else'.

But I guess the main advantage with using this rule I see is that it's
better for people reading the code.  It's just nice to know whether a
file belongs to qemu or not by just looking at the #include statement.
(Note that this implies that it is indeed more difficult to determine
whether a header belongs to qemu than whether it sits in the same
directory as the C file, though!)  So I think the old (current) rule is
better for reading code, Michael's rule would be better for writing
code.  I think reading code is what should be easier.

But since that may be eaten up by build breakages due to stale files, I
don't have a strong opinion either way.  I just wanted to chime in
because in my opinion 'Use "" for headers in the same directory, <> for
everything else' is by no means an unreasonably complicated rule for
people writing code.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 01:10:41PM +, Stefan Hajnoczi wrote:
> On Tue, Mar 20, 2018 at 03:54:36AM +0200, Michael S. Tsirkin wrote:
> > QEMU coding style at the moment asks for all non-system
> > include files to be used with #include "foo.h".
> > However this rule actually does not make sense and
> > creates issues for when the included file is generated.
> > 
> > In C, include "file" means look in current directory,
> > then on include search path. Current directory here
> > means the source file directory.
> > By comparison include  means look on include search path.
> > 
> > As generated files are not in the search directory (unless the build
> > directory happens to match the source directory), it does not make sense
> > to include them with "" - doing so is merely more work for preprocessor
> > and a source or errors if a stale file happens to exist in the source
> > directory.
> > 
> > This changes include directives for all generated files, across the
> > tree. The idea is to avoid sending a huge amount of email.  But when
> > merging, the changes will be split with one commit per file, e.g. for
> > ease of bisect in case of build failures, and to ease merging.
> > 
> > Note that should some generated files be missed by this tree-wide
> > refactoring, it isn't a big deal - this merely maintains the status quo,
> > and this can be addressed by a separate patch on top.
> 
> Stale header files are a pain.  I often do make distclean before
> checking out a radically different QEMU version to avoid the problem.
> 
> This patch trades off the stale header file issue for a new approach to
> using <> vs "", which will be hard to use consistently

The proposed rule is to use <> everywhere except if the file is in the
source directory.  It will be very easy to use consistently for the
simple reason that compiler will enforce it.  If you use <> for a header
in the current directory build fails. So you are forced to use "".

> in the future
> since it is unconventional.

All compilers I know without exception implement
include <> meaning look in -I search path, and
include "" meaning look in current directory then
in -I search path.

Looks conventional to me.

> Is the build time improvement worth it (please post numbers)?
> 
> Stefan

Haven't tested it frankly. Will it convince anyone if it's marginally
faster?

-- 
MST

Re: [Qemu-block] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 02:46:46PM +0100, Thomas Huth wrote:
> On 20.03.2018 14:32, Gerd Hoffmann wrote:
> >   Hi,
> > 
> >>> So for these, we should use "".  None of these are generated files though.
> >>
> >> That leads to crazy inconsistent message for developers where 50% of QEMU
> >> header files must use <> and the other 50% of header files must use "".
> > 
> > The rules are pretty simple though:
> > 
> >(1) Headers which are generated use <>.
> >(2) Headers which are in include/ use <>.
> >(3) Headers sitting in the same directory as the source files use "".
> 
> Ugh, no. Please don't. The normal way of including header files in QEMU

Stress on "in QEMU".

> is to use "" - also for headers that are not in the same directory. For
> example just do a
> 
>  grep -r '^#include.*hw/' hw/
> 
> from the top directory and you'll see what I mean. Changing that rule is
> crazy.
> So please, let's just fix the configure script to detect some
> more stale files in the source tree, and we're done.
> 
>  Thomas

The rule probably served a useful purpose when all
headers where in the same directory as the source.
we moved them out so that is no longer the case.

-- 
MST

[Qemu-block] [PATCH] qcow2: Reset free_cluster_index when allocating a new refcount block

[Alberto Garcia]

When we try to allocate new clusters we first look for available ones
starting from s->free_cluster_index and once we find them we increase
their reference counts. Before we get to call update_refcount() to do
this last step s->free_cluster_index is already pointing to the next
cluster after the ones we are trying to allocate.

During update_refcount() it may happen however that we also need to
allocate a new refcount block in order to store the refcounts of these
new clusters (and to complicate things further that may also require
us to grow the refcount table). After all this we don't know if the
clusters that we originally tried to allocate are still available, so
we return -EAGAIN to ask the caller to restart the search for free
clusters.

This is what can happen in a common scenario:

  1) We want to allocate a new cluster and we see that cluster N is
 free.

  2) We try to increase N's refcount but all refcount blocks are full,
 so we allocate a new one at N+1 (where s->free_cluster_index was
 pointing at).

  3) Once we're done we return -EAGAIN to look again for a free
 cluster, but now s->free_cluster_index points at N+2, so that's
 the one we allocate. Cluster N remains unallocated and we have a
 hole in the qcow2 file.

This can be reproduced easily:

 qemu-img create -f qcow2 -o cluster_size=512 hd.qcow2 1M
 qemu-io -c 'write 0 124k' hd.qcow2

After this the image has 132608 bytes (256 clusters), and the refcount
block is full. If we write 512 more bytes it should allocate two new
clusters: the data cluster itself and a new refcount block.

 qemu-io -c 'write 124k 512' hd.qcow2

However the image has now three new clusters (259 in total), and the
first one of them is empty (and unallocated):

 dd if=hd.qcow2 bs=512c skip=256 count=1 | hexdump -C

If we write larger amounts of data in the last step instead of the 512
bytes used in this example we can create larger holes in the qcow2
file.

What this patch does is reset s->free_cluster_index to its previous
value when alloc_refcount_block() returns -EAGAIN. This way the caller
will try to allocate again the original clusters if they are still
free.

The output of iotest 026 also needs to be updated because now that
images have no holes some tests fail at a different point and the
number of leaked clusters is different.

Signed-off-by: Alberto Garcia 
---
 block/qcow2-refcount.c |  7 +++
 tests/qemu-iotests/026.out |  6 +++---
 tests/qemu-iotests/121 | 20 
 tests/qemu-iotests/121.out | 10 ++
 4 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 362deaf303..6b8b63514a 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -839,6 +839,13 @@ static int QEMU_WARN_UNUSED_RESULT 
update_refcount(BlockDriverState *bs,
 qcow2_cache_put(s->refcount_block_cache, _block);
 }
 ret = alloc_refcount_block(bs, cluster_index, _block);
+/* If the caller needs to restart the search for free clusters,
+ * try the same ones first to see if they're still free. */
+if (ret == -EAGAIN) {
+if (s->free_cluster_index > (start >> s->cluster_bits)) {
+s->free_cluster_index = (start >> s->cluster_bits);
+}
+}
 if (ret < 0) {
 goto fail;
 }
diff --git a/tests/qemu-iotests/026.out b/tests/qemu-iotests/026.out
index 86a50a2e13..8e89416a86 100644
--- a/tests/qemu-iotests/026.out
+++ b/tests/qemu-iotests/026.out
@@ -533,7 +533,7 @@ Failed to flush the L2 table cache: No space left on device
 Failed to flush the refcount block cache: No space left on device
 write failed: No space left on device
 
-11 leaked clusters were found on the image.
+10 leaked clusters were found on the image.
 This means waste of disk space, but no harm to data.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
@@ -561,7 +561,7 @@ Failed to flush the L2 table cache: No space left on device
 Failed to flush the refcount block cache: No space left on device
 write failed: No space left on device
 
-11 leaked clusters were found on the image.
+10 leaked clusters were found on the image.
 This means waste of disk space, but no harm to data.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
@@ -589,7 +589,7 @@ Failed to flush the L2 table cache: No space left on device
 Failed to flush the refcount block cache: No space left on device
 write failed: No space left on device
 
-11 leaked clusters were found on the image.
+10 leaked clusters were found on the image.
 This means waste of disk space, but no harm to data.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
diff --git a/tests/qemu-iotests/121 b/tests/qemu-iotests/121
index 1307b4e327..13b2479e86 100755
--- a/tests/qemu-iotests/121
+++ b/tests/qemu-iotests/121
@@ 

Re: [Qemu-block] [PATCH v3 2/3] qcow2: handle reopening bitmaps on bdrv_invalidate_cache

[Max Reitz]

On 2018-03-19 10:02, Vladimir Sementsov-Ogievskiy wrote:
> Consider migration with shared storage. Persistent bitmaps are stored
> on bdrv_inactivate. Then, on destination
> process_incoming_migration_bh() calls bdrv_invalidate_cache_all() which
> leads to qcow2_load_autoloading_dirty_bitmaps() which fails if bitmaps
> are already loaded on destination start. In this case we should call
> qcow2_reopen_bitmaps_rw instead.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> Reviewed-by: John Snow 
> ---
>  block/qcow2.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 7472af6931..6219666d4a 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1480,7 +1480,13 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
> *bs, QDict *options,
>  s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
>  }
>  
> -if (qcow2_load_dirty_bitmaps(bs, _err)) {
> +if (bdrv_has_readonly_bitmaps(bs)) {

So if there are any read-only bitmaps, we'll skip
qcow2_load_dirty_bitmaps() altogether.  That doesn't seem so bad because
qcow2_load_dirty_bitmaps() seems to assume no bitmaps have been loaded yet.

But if that's the case, shouldn't we skip that function if any bitmap
has been loaded already, RO or R/W?  (And we can call
qcow2_reopen_bitmaps_rw_hint() even if there aren't any RO bitmaps, it
just won't do anything then.)

This doesn't make this patch really wrong, I'm just wondering whether it
can do better.  (To add to that, I don't even know whether there is a
case where qcow2_do_open() would be called with any R/W bitmaps already
present.)

Max

> +if (!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & 
> BDRV_O_INACTIVE)) {
> +bool header_updated = false;
> +qcow2_reopen_bitmaps_rw_hint(bs, _updated, _err);
> +update_header = update_header && !header_updated;
> +}
> +} else if (qcow2_load_dirty_bitmaps(bs, _err)) {
>  update_header = false;
>  }
>  if (local_err != NULL) {
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 03:54:36AM +0200, Michael S. Tsirkin wrote:
> QEMU coding style at the moment asks for all non-system
> include files to be used with #include "foo.h".
> However this rule actually does not make sense and
> creates issues for when the included file is generated.
> 
> In C, include "file" means look in current directory,
> then on include search path. Current directory here
> means the source file directory.
> By comparison include  means look on include search path.
> 
> As generated files are not in the search directory (unless the build
> directory happens to match the source directory), it does not make sense
> to include them with "" - doing so is merely more work for preprocessor
> and a source or errors if a stale file happens to exist in the source
> directory.
> 
> This changes include directives for all generated files, across the
> tree. The idea is to avoid sending a huge amount of email.  But when
> merging, the changes will be split with one commit per file, e.g. for
> ease of bisect in case of build failures, and to ease merging.
> 
> Note that should some generated files be missed by this tree-wide
> refactoring, it isn't a big deal - this merely maintains the status quo,
> and this can be addressed by a separate patch on top.
> 
> Signed-off-by: Michael S. Tsirkin 


For the record, the stated advantage is that one can
have a header file that happens to match the system
header.

To put it bluntly that does not work as designed.

For example, if a system header foo.h somewhere has #include 
then the compiler will happily pull in our own version (since that is in
the -I path) and completely ignore the system one, breaking things in
the process.

When does it make sense to use include ""? When the header is
a directory-specific one, located with the source.
This approach would both be enforced by the compiler
and help people know where to find the header.

-- 
MST

Re: [Qemu-block] [PATCH] qemu: include generated files with <> and not ""

[Daniel P . Berrangé]

On Tue, Mar 20, 2018 at 02:28:42PM +0200, Michael S. Tsirkin wrote:
> On Tue, Mar 20, 2018 at 12:18:41PM +, Daniel P. Berrangé wrote:
> > On Tue, Mar 20, 2018 at 02:12:24PM +0200, Michael S. Tsirkin wrote:
> > > On Tue, Mar 20, 2018 at 09:44:06AM +, Daniel P. Berrangé wrote:
> > > > On Tue, Mar 20, 2018 at 09:58:23AM +0100, Laurent Vivier wrote:
> > > > > Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit :
> > > > > > QEMU coding style at the moment asks for all non-system
> > > > > > include files to be used with #include "foo.h".
> > > > > > However this rule actually does not make sense and
> > > > > > creates issues for when the included file is generated.
> > > > > 
> > > > > If you change that, we can have issue when a system include has the 
> > > > > same
> > > > > name as our local include. With "", system header are taken 
> > > > > first.
> > > > 
> > > > > > In C, include "file" means look in current directory,
> > > > > > then on include search path. Current directory here
> > > > > > means the source file directory.
> > > > > > By comparison include  means look on include search path.
> > > > > 
> > > > > Not exactly, there is the notion of "system header" too.
> > > > > 
> > > > > https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html
> > > > > 
> > > > > #include 
> > > > > This variant is used for system header files. It searches for a file
> > > > > named file in a standard list of system directories. You can prepend
> > > > > directories to this list with the -I option (see Invocation).
> > > > > 
> > > > > #include "file"
> > > > > This variant is used for header files of your own program. It searches
> > > > > for a file named file first in the directory containing the current
> > > > > file, then in the quote directories and then the same directories used
> > > > > for . You can prepend directories to the list of quote 
> > > > > directories
> > > > > with the -iquote option.
> > > > > 
> > > > > > As generated files are not in the search directory (unless the build
> > > > > > directory happens to match the source directory), it does not make 
> > > > > > sense
> > > > > > to include them with "" - doing so is merely more work for 
> > > > > > preprocessor
> > > > > > and a source or errors if a stale file happens to exist in the 
> > > > > > source
> > > > > > directory.
> > > > > 
> > > > > I agree there is a problem with stale files. But linux, for instance,
> > > > > asks for a "make mrproper" to avoid this.
> > > > 
> > > > We can follow what autoconf does, and add a check to configure to see if
> > > > there are generated files left in the source dir, when configuring with
> > > > builddir != srcdir, and exit with error, telling user to clean their
> > > > src dir first.
> > > > 
> > > > > > This changes include directives for all generated files, across the
> > > > > > tree. The idea is to avoid sending a huge amount of email.  But when
> > > > > > merging, the changes will be split with one commit per file, e.g. 
> > > > > > for
> > > > > > ease of bisect in case of build failures, and to ease merging.
> > > > > > 
> > > > > > Note that should some generated files be missed by this tree-wide
> > > > > > refactoring, it isn't a big deal - this merely maintains the status 
> > > > > > quo,
> > > > > > and this can be addressed by a separate patch on top.
> > > > > > 
> > > > > > Signed-off-by: Michael S. Tsirkin 
> > > > > 
> > > > > I think your idea conflicts with what Markus has started to do:
> > > > 
> > > > Yes, I don't think we should revert what Markus started.   Both ways of
> > > > referencing QEMU headers have downsides, but I think "..." has fewer
> > > > downsides that "<">.
> > > 
> > > Could you please explain what the advantage of "" is?
> > > It seems to be gone since we moved headers away from
> > > source.
> > 
> > We moved *some* headers into the include/ directory tree.
> > 
> > I still count 650+ headers which are alongside the .c files.
> 
> So for these, we should use "".  None of these are generated files though.

That leads to crazy inconsistent message for developers where 50% of QEMU
header files must use <> and the other 50% of header files must use "".
Having a consistent message for developers is one of the key reasons why
Markus submitted the patches to standardize on the use of "" for QEMU
header files, leaving <> for system headers & external dependancies.

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: [Qemu-block] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 12:18:41PM +, Daniel P. Berrangé wrote:
> On Tue, Mar 20, 2018 at 02:12:24PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Mar 20, 2018 at 09:44:06AM +, Daniel P. Berrangé wrote:
> > > On Tue, Mar 20, 2018 at 09:58:23AM +0100, Laurent Vivier wrote:
> > > > Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit :
> > > > > QEMU coding style at the moment asks for all non-system
> > > > > include files to be used with #include "foo.h".
> > > > > However this rule actually does not make sense and
> > > > > creates issues for when the included file is generated.
> > > > 
> > > > If you change that, we can have issue when a system include has the same
> > > > name as our local include. With "", system header are taken first.
> > > 
> > > > > In C, include "file" means look in current directory,
> > > > > then on include search path. Current directory here
> > > > > means the source file directory.
> > > > > By comparison include  means look on include search path.
> > > > 
> > > > Not exactly, there is the notion of "system header" too.
> > > > 
> > > > https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html
> > > > 
> > > > #include 
> > > > This variant is used for system header files. It searches for a file
> > > > named file in a standard list of system directories. You can prepend
> > > > directories to this list with the -I option (see Invocation).
> > > > 
> > > > #include "file"
> > > > This variant is used for header files of your own program. It searches
> > > > for a file named file first in the directory containing the current
> > > > file, then in the quote directories and then the same directories used
> > > > for . You can prepend directories to the list of quote directories
> > > > with the -iquote option.
> > > > 
> > > > > As generated files are not in the search directory (unless the build
> > > > > directory happens to match the source directory), it does not make 
> > > > > sense
> > > > > to include them with "" - doing so is merely more work for 
> > > > > preprocessor
> > > > > and a source or errors if a stale file happens to exist in the source
> > > > > directory.
> > > > 
> > > > I agree there is a problem with stale files. But linux, for instance,
> > > > asks for a "make mrproper" to avoid this.
> > > 
> > > We can follow what autoconf does, and add a check to configure to see if
> > > there are generated files left in the source dir, when configuring with
> > > builddir != srcdir, and exit with error, telling user to clean their
> > > src dir first.
> > > 
> > > > > This changes include directives for all generated files, across the
> > > > > tree. The idea is to avoid sending a huge amount of email.  But when
> > > > > merging, the changes will be split with one commit per file, e.g. for
> > > > > ease of bisect in case of build failures, and to ease merging.
> > > > > 
> > > > > Note that should some generated files be missed by this tree-wide
> > > > > refactoring, it isn't a big deal - this merely maintains the status 
> > > > > quo,
> > > > > and this can be addressed by a separate patch on top.
> > > > > 
> > > > > Signed-off-by: Michael S. Tsirkin 
> > > > 
> > > > I think your idea conflicts with what Markus has started to do:
> > > 
> > > Yes, I don't think we should revert what Markus started.   Both ways of
> > > referencing QEMU headers have downsides, but I think "..." has fewer
> > > downsides that "<">.
> > 
> > Could you please explain what the advantage of "" is?
> > It seems to be gone since we moved headers away from
> > source.
> 
> We moved *some* headers into the include/ directory tree.
> 
> I still count 650+ headers which are alongside the .c files.
> 
> Regards,
> Daniel

So for these, we should use "".  None of these are generated files though.

> -- 
> |: 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: [Qemu-block] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 09:44:06AM +, Daniel P. Berrangé wrote:
> On Tue, Mar 20, 2018 at 09:58:23AM +0100, Laurent Vivier wrote:
> > Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit :
> > > QEMU coding style at the moment asks for all non-system
> > > include files to be used with #include "foo.h".
> > > However this rule actually does not make sense and
> > > creates issues for when the included file is generated.
> > 
> > If you change that, we can have issue when a system include has the same
> > name as our local include. With "", system header are taken first.
> 
> > > In C, include "file" means look in current directory,
> > > then on include search path. Current directory here
> > > means the source file directory.
> > > By comparison include  means look on include search path.
> > 
> > Not exactly, there is the notion of "system header" too.
> > 
> > https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html
> > 
> > #include 
> > This variant is used for system header files. It searches for a file
> > named file in a standard list of system directories. You can prepend
> > directories to this list with the -I option (see Invocation).
> > 
> > #include "file"
> > This variant is used for header files of your own program. It searches
> > for a file named file first in the directory containing the current
> > file, then in the quote directories and then the same directories used
> > for . You can prepend directories to the list of quote directories
> > with the -iquote option.
> > 
> > > As generated files are not in the search directory (unless the build
> > > directory happens to match the source directory), it does not make sense
> > > to include them with "" - doing so is merely more work for preprocessor
> > > and a source or errors if a stale file happens to exist in the source
> > > directory.
> > 
> > I agree there is a problem with stale files. But linux, for instance,
> > asks for a "make mrproper" to avoid this.
> 
> We can follow what autoconf does, and add a check to configure to see if
> there are generated files left in the source dir, when configuring with
> builddir != srcdir, and exit with error, telling user to clean their
> src dir first.
> 
> > > This changes include directives for all generated files, across the
> > > tree. The idea is to avoid sending a huge amount of email.  But when
> > > merging, the changes will be split with one commit per file, e.g. for
> > > ease of bisect in case of build failures, and to ease merging.
> > > 
> > > Note that should some generated files be missed by this tree-wide
> > > refactoring, it isn't a big deal - this merely maintains the status quo,
> > > and this can be addressed by a separate patch on top.
> > > 
> > > Signed-off-by: Michael S. Tsirkin 
> > 
> > I think your idea conflicts with what Markus has started to do:
> 
> Yes, I don't think we should revert what Markus started.   Both ways of
> referencing QEMU headers have downsides, but I think "..." has fewer
> downsides that "<">.

Could you please explain what the advantage of "" is?
It seems to be gone since we moved headers away from
source.

> The problem Michael is tackling should be pretty rare, because moist
> developers aren't frequently switching between srcdir==builddir and
> srcdir!=biulddir setups - they have their preference for which to use
> and stick with it. As long as we get ./configure to warn about the
> dirty srcdir it should be good enough
> 
> 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: [Qemu-block] [PATCH] qemu: include generated files with <> and not ""

[Stefan Hajnoczi]

On Tue, Mar 20, 2018 at 03:54:36AM +0200, Michael S. Tsirkin wrote:
> QEMU coding style at the moment asks for all non-system
> include files to be used with #include "foo.h".
> However this rule actually does not make sense and
> creates issues for when the included file is generated.
> 
> In C, include "file" means look in current directory,
> then on include search path. Current directory here
> means the source file directory.
> By comparison include  means look on include search path.
> 
> As generated files are not in the search directory (unless the build
> directory happens to match the source directory), it does not make sense
> to include them with "" - doing so is merely more work for preprocessor
> and a source or errors if a stale file happens to exist in the source
> directory.
> 
> This changes include directives for all generated files, across the
> tree. The idea is to avoid sending a huge amount of email.  But when
> merging, the changes will be split with one commit per file, e.g. for
> ease of bisect in case of build failures, and to ease merging.
> 
> Note that should some generated files be missed by this tree-wide
> refactoring, it isn't a big deal - this merely maintains the status quo,
> and this can be addressed by a separate patch on top.

Stale header files are a pain.  I often do make distclean before
checking out a radically different QEMU version to avoid the problem.

This patch trades off the stale header file issue for a new approach to
using <> vs "", which will be hard to use consistently in the future
since it is unconventional.

Is the build time improvement worth it (please post numbers)?

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 09:58:23AM +0100, Laurent Vivier wrote:
> Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit :
> > QEMU coding style at the moment asks for all non-system
> > include files to be used with #include "foo.h".
> > However this rule actually does not make sense and
> > creates issues for when the included file is generated.
> 
> If you change that, we can have issue when a system include has the same
> name as our local include. With "", system header are taken first.

Are you sure? I just tested and that is not the case with
either gcc or clang.

> > In C, include "file" means look in current directory,
> > then on include search path. Current directory here
> > means the source file directory.
> > By comparison include  means look on include search path.
> 
> Not exactly, there is the notion of "system header" too.
> 
> https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html
> 
> #include 
> This variant is used for system header files. It searches for a file
> named file in a standard list of system directories. You can prepend
> directories to this list with the -I option (see Invocation).

This is exactly what we do.

> #include "file"
> This variant is used for header files of your own program. It searches
> for a file named file first in the directory containing the current
> file, then in the quote directories and then the same directories used
> for . You can prepend directories to the list of quote directories
> with the -iquote option.

Since we do not use -iquote, "" just adds the current directory.


> > As generated files are not in the search directory (unless the build
> > directory happens to match the source directory), it does not make sense
> > to include them with "" - doing so is merely more work for preprocessor
> > and a source or errors if a stale file happens to exist in the source
> > directory.
> 
> I agree there is a problem with stale files. But linux, for instance,
> asks for a "make mrproper" to avoid this.

Using <> we avoid the problem completely and create slightly
less work for the preprocessor.

> > This changes include directives for all generated files, across the
> > tree. The idea is to avoid sending a huge amount of email.  But when
> > merging, the changes will be split with one commit per file, e.g. for
> > ease of bisect in case of build failures, and to ease merging.
> > 
> > Note that should some generated files be missed by this tree-wide
> > refactoring, it isn't a big deal - this merely maintains the status quo,
> > and this can be addressed by a separate patch on top.
> > 
> > Signed-off-by: Michael S. Tsirkin 
> 
> I think your idea conflicts with what Markus has started to do:
> 
> commit d8e39b70625d4ba1e998439d1a077b4b978930e7
> Author: Markus Armbruster 
> Date:   Thu Feb 1 12:18:28 2018 +0100
> 
> Use #include "..." for our own headers, <...> for others
> 
> System headers should be included with <...>, our own headers with
> "...".  Offenders tracked down with an ugly, brittle and probably
> buggy Perl script.  Previous iteration was commit a9c94277f0.
> 
> Delete inclusions of "string.h" and "strings.h" instead of fixing them
> to  and , because we always include these via
> osdep.h.
> 
> Put the cleaned up system header includes first.
> 
> While there, separate #include from file comment with exactly one
> blank line.
> 
> commit a9c94277f07d19d3eb14f199c3e93491aa3eae0e
> Author: Markus Armbruster 
> Date:   Wed Jun 22 19:11:19 2016 +0200
> 
> Use #include "..." for our own headers, <...> for others
> 
> Tracked down with an ugly, brittle and probably buggy Perl script.
> 
> Also move includes converted to <...> up so they get included before
> ours where that's obviously okay.
> 
> Thanks,
> Laurent

I suspect we previously actually did have headers in the
same directory as source so it was somewhat helpful.
They all have been moved out to include now.

-- 
MST

Re: [Qemu-block] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 10:27:19AM +, Daniel P. Berrangé wrote:
> On Tue, Mar 20, 2018 at 10:01:24AM +, Peter Maydell wrote:
> > On 20 March 2018 at 09:44, Daniel P. Berrangé  wrote:
> > > We can follow what autoconf does, and add a check to configure to see if
> > > there are generated files left in the source dir, when configuring with
> > > builddir != srcdir, and exit with error, telling user to clean their
> > > src dir first.
> > 
> > We already do this in our makefile...it just doesn't check every
> > single generated file.
> 
> Ah yes, indeed:
> 
> $ make
> Makefile:59: *** This is an out of tree build but your source tree
> (/home/berrange/src/virt/qemu) seems to have been used for an in-tree
> build. You can fix this by running "make distclean && rm -rf *-linux-user
>  *-softmmu" in your source tree.  Stop.
> 
> 
> It is checking for existance of config-host.mak.
> 
> We have a convenient list of generated files in $(GENERATED_FILES), so
> I wonder if there's a practical way to check all of those too.
> 
> Regards,
> Daniel

With my patch presence of these files mostly stops being an issue.

> -- 
> |: 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: [Qemu-block] [PATCH] qemu: include generated files with <> and not ""

[Daniel P . Berrangé]

On Tue, Mar 20, 2018 at 02:12:24PM +0200, Michael S. Tsirkin wrote:
> On Tue, Mar 20, 2018 at 09:44:06AM +, Daniel P. Berrangé wrote:
> > On Tue, Mar 20, 2018 at 09:58:23AM +0100, Laurent Vivier wrote:
> > > Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit :
> > > > QEMU coding style at the moment asks for all non-system
> > > > include files to be used with #include "foo.h".
> > > > However this rule actually does not make sense and
> > > > creates issues for when the included file is generated.
> > > 
> > > If you change that, we can have issue when a system include has the same
> > > name as our local include. With "", system header are taken first.
> > 
> > > > In C, include "file" means look in current directory,
> > > > then on include search path. Current directory here
> > > > means the source file directory.
> > > > By comparison include  means look on include search path.
> > > 
> > > Not exactly, there is the notion of "system header" too.
> > > 
> > > https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html
> > > 
> > > #include 
> > > This variant is used for system header files. It searches for a file
> > > named file in a standard list of system directories. You can prepend
> > > directories to this list with the -I option (see Invocation).
> > > 
> > > #include "file"
> > > This variant is used for header files of your own program. It searches
> > > for a file named file first in the directory containing the current
> > > file, then in the quote directories and then the same directories used
> > > for . You can prepend directories to the list of quote directories
> > > with the -iquote option.
> > > 
> > > > As generated files are not in the search directory (unless the build
> > > > directory happens to match the source directory), it does not make sense
> > > > to include them with "" - doing so is merely more work for preprocessor
> > > > and a source or errors if a stale file happens to exist in the source
> > > > directory.
> > > 
> > > I agree there is a problem with stale files. But linux, for instance,
> > > asks for a "make mrproper" to avoid this.
> > 
> > We can follow what autoconf does, and add a check to configure to see if
> > there are generated files left in the source dir, when configuring with
> > builddir != srcdir, and exit with error, telling user to clean their
> > src dir first.
> > 
> > > > This changes include directives for all generated files, across the
> > > > tree. The idea is to avoid sending a huge amount of email.  But when
> > > > merging, the changes will be split with one commit per file, e.g. for
> > > > ease of bisect in case of build failures, and to ease merging.
> > > > 
> > > > Note that should some generated files be missed by this tree-wide
> > > > refactoring, it isn't a big deal - this merely maintains the status quo,
> > > > and this can be addressed by a separate patch on top.
> > > > 
> > > > Signed-off-by: Michael S. Tsirkin 
> > > 
> > > I think your idea conflicts with what Markus has started to do:
> > 
> > Yes, I don't think we should revert what Markus started.   Both ways of
> > referencing QEMU headers have downsides, but I think "..." has fewer
> > downsides that "<">.
> 
> Could you please explain what the advantage of "" is?
> It seems to be gone since we moved headers away from
> source.

We moved *some* headers into the include/ directory tree.

I still count 650+ headers which are alongside the .c files.

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: [Qemu-block] [PATCH] qemu: include generated files with <> and not ""

[Laurent Vivier]

Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit :
> QEMU coding style at the moment asks for all non-system
> include files to be used with #include "foo.h".
> However this rule actually does not make sense and
> creates issues for when the included file is generated.

If you change that, we can have issue when a system include has the same
name as our local include. With "", system header are taken first.

> In C, include "file" means look in current directory,
> then on include search path. Current directory here
> means the source file directory.
> By comparison include  means look on include search path.

Not exactly, there is the notion of "system header" too.

https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html

#include 
This variant is used for system header files. It searches for a file
named file in a standard list of system directories. You can prepend
directories to this list with the -I option (see Invocation).

#include "file"
This variant is used for header files of your own program. It searches
for a file named file first in the directory containing the current
file, then in the quote directories and then the same directories used
for . You can prepend directories to the list of quote directories
with the -iquote option.

> As generated files are not in the search directory (unless the build
> directory happens to match the source directory), it does not make sense
> to include them with "" - doing so is merely more work for preprocessor
> and a source or errors if a stale file happens to exist in the source
> directory.

I agree there is a problem with stale files. But linux, for instance,
asks for a "make mrproper" to avoid this.

> This changes include directives for all generated files, across the
> tree. The idea is to avoid sending a huge amount of email.  But when
> merging, the changes will be split with one commit per file, e.g. for
> ease of bisect in case of build failures, and to ease merging.
> 
> Note that should some generated files be missed by this tree-wide
> refactoring, it isn't a big deal - this merely maintains the status quo,
> and this can be addressed by a separate patch on top.
> 
> Signed-off-by: Michael S. Tsirkin 

I think your idea conflicts with what Markus has started to do:

commit d8e39b70625d4ba1e998439d1a077b4b978930e7
Author: Markus Armbruster 
Date:   Thu Feb 1 12:18:28 2018 +0100

Use #include "..." for our own headers, <...> for others

System headers should be included with <...>, our own headers with
"...".  Offenders tracked down with an ugly, brittle and probably
buggy Perl script.  Previous iteration was commit a9c94277f0.

Delete inclusions of "string.h" and "strings.h" instead of fixing them
to  and , because we always include these via
osdep.h.

Put the cleaned up system header includes first.

While there, separate #include from file comment with exactly one
blank line.

commit a9c94277f07d19d3eb14f199c3e93491aa3eae0e
Author: Markus Armbruster 
Date:   Wed Jun 22 19:11:19 2016 +0200

Use #include "..." for our own headers, <...> for others

Tracked down with an ugly, brittle and probably buggy Perl script.

Also move includes converted to <...> up so they get included before
ours where that's obviously okay.

Thanks,
Laurent

Re: [Qemu-block] [PATCH] qemu: include generated files with <> and not ""

[Peter Maydell]

On 20 March 2018 at 09:44, Daniel P. Berrangé  wrote:
> We can follow what autoconf does, and add a check to configure to see if
> there are generated files left in the source dir, when configuring with
> builddir != srcdir, and exit with error, telling user to clean their
> src dir first.

We already do this in our makefile...it just doesn't check every
single generated file.

thanks
-- PMM

Re: [Qemu-block] [PATCH] qemu: include generated files with <> and not ""

[Daniel P . Berrangé]

On Tue, Mar 20, 2018 at 09:58:23AM +0100, Laurent Vivier wrote:
> Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit :
> > QEMU coding style at the moment asks for all non-system
> > include files to be used with #include "foo.h".
> > However this rule actually does not make sense and
> > creates issues for when the included file is generated.
> 
> If you change that, we can have issue when a system include has the same
> name as our local include. With "", system header are taken first.

> > In C, include "file" means look in current directory,
> > then on include search path. Current directory here
> > means the source file directory.
> > By comparison include  means look on include search path.
> 
> Not exactly, there is the notion of "system header" too.
> 
> https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html
> 
> #include 
> This variant is used for system header files. It searches for a file
> named file in a standard list of system directories. You can prepend
> directories to this list with the -I option (see Invocation).
> 
> #include "file"
> This variant is used for header files of your own program. It searches
> for a file named file first in the directory containing the current
> file, then in the quote directories and then the same directories used
> for . You can prepend directories to the list of quote directories
> with the -iquote option.
> 
> > As generated files are not in the search directory (unless the build
> > directory happens to match the source directory), it does not make sense
> > to include them with "" - doing so is merely more work for preprocessor
> > and a source or errors if a stale file happens to exist in the source
> > directory.
> 
> I agree there is a problem with stale files. But linux, for instance,
> asks for a "make mrproper" to avoid this.

We can follow what autoconf does, and add a check to configure to see if
there are generated files left in the source dir, when configuring with
builddir != srcdir, and exit with error, telling user to clean their
src dir first.

> > This changes include directives for all generated files, across the
> > tree. The idea is to avoid sending a huge amount of email.  But when
> > merging, the changes will be split with one commit per file, e.g. for
> > ease of bisect in case of build failures, and to ease merging.
> > 
> > Note that should some generated files be missed by this tree-wide
> > refactoring, it isn't a big deal - this merely maintains the status quo,
> > and this can be addressed by a separate patch on top.
> > 
> > Signed-off-by: Michael S. Tsirkin 
> 
> I think your idea conflicts with what Markus has started to do:

Yes, I don't think we should revert what Markus started.   Both ways of
referencing QEMU headers have downsides, but I think "..." has fewer
downsides that "<">.

The problem Michael is tackling should be pretty rare, because moist
developers aren't frequently switching between srcdir==builddir and
srcdir!=biulddir setups - they have their preference for which to use
and stick with it. As long as we get ./configure to warn about the
dirty srcdir it should be good enough

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: [Qemu-block] [PATCH 4/5] migration/block: limit the number of parallel I/O requests

[Peter Lieven]

Am 08.03.2018 um 14:30 schrieb Peter Lieven:

Am 08.03.2018 um 13:50 schrieb Juan Quintela:

Peter Lieven  wrote:

the current implementation submits up to 512 I/O requests in parallel
which is much to high especially for a background task.
This patch adds a maximum limit of 16 I/O requests that can
be submitted in parallel to avoid monopolizing the I/O device.

Signed-off-by: Peter Lieven 

This code is already a mess (TM).  It is difficult to understand what we
are doing, so not easy to see if your changes are better/worse than what
we have.

I am not sure that your solution help to improve things here. Let's see
what I understand.

We have three fields (without a single comment):

- submitted: this is the number of blocks that we have asked the block
  device to read asynchronously to main memory, and that
  haven't yet read.  I.e. "blocks_read_pending" would be a
  better name?

- read_done: this is the number of blocks that we have finished read
  asynchronously from this block device.  When we finish, we
  do a submitted -- and a read_done++. blocks_read_finished
  name?


Correct. The names should be adjusted.



- transferred: This is the number of blocks that we have transferred
    since the beginning of migration.  At this point, we do a
    read_done-- and a transferred++

Note also that we do malloc()/free() for each block


Yes, but that is a different story.



So, now that we have defined what our fields mean, we need to know what
is our test.  block_save_pending():

 get_remaining_dirty() + (submitted + read_done) * BLOCK_SIZE

Looks good.

But let's us see what test we do in block_save_iterate() (Yes, I have
been very liberal with reformatting and removal of struct names):

(submitted + read_done) * BLOCK_SIZE < qemu_file_get_rate_limit(f) &&
(submitted + read_done) < MAX_INFLIGHT_IO

The idea of that test is to make sure that we _don't send_ through the
QEMUFile more than qemu_file_get_rate_limit(f).  But there are several
things here:
- we have already issued a flush_blks() call before we enter the loop
   And it is inside possibility that we have already sent too much data at
   this point, but we enter the while loop anyways.

   Notice that flush_blks() does the right thing and test in each loop if
   qemu_file_rate_limit() has been reached and stops sending more data if
   it returns true;

- At this point, we *could* have sent all that can be sent for this
   round, but we enter the loop anyways.  And we test two things:
  - that we haven't read from block devices more than
    qemu_file_get_rate_limit() bytes (notice that this is the maximum
    that we could put through the migration channel, not really
    related  with what we read from block devices).

  - that we haven't read in this round more than MAX_INFLIGHT_IO
    blocks.  That is 512 blocks, at BLOCK_SIZE bytes is 512MB. Why?
    Who knows.


The idea behind this was only to limit the maximum memory that is allocated.
That was not meant as a rate limit for the storage backend.



- Now we exit the while loop, and we have pending blocks to send, the
 minimum between:
    - qemu_file_get_rate_limit()/BLOCK_SIZE, and
    - MAX_INFLIGHT_IO

But not all of them are ready to send, only "read_done" blocks are
ready, the "submitted" ones are still waiting for read completion.


Thats what I tried to address in patch 5.



And we call back flush_blks() that will try to send all the "read_done"
blocks through the migration channel until we hit
qemu_file_rate_limit().

So, looking at the problem from far away:

- how many read requests are we have to have in flight at any moment, is
   that 16 from this series the right number? Notice that each request
   are 1MB, so this is 16MB (I have no clue what is the right value).


I choosed that value because it helped to improved the stalls in the
guest that I have been seeing. Stefan also said that 16 would be a good
value for a background task. 512 is definetly too much.



- how many blocks should we get on each round.  Notice that the reason
   for the 1st patch on this series is because the block layer is not
   sending enough blocks to prevent ram migration to start.  If there are
   enough dirty memory sent from the block layer, we shouldn't ever enter
   the ram stage.
   Notice how we register them in vl.c:

 blk_mig_init();
 ram_mig_init();


The idea of the 1st patch was to skip ram migration until we have completed
the first round of block migration (aka bulk phase) as this will take a long 
time.
After that we are only doing incremental updates. You are right this might still
be too early, but it to start after the bulk phase was an easy choice without
introducing another heuristic.



So, after so long mail, do I have some suggestion?

- should we make the MAX_PARALLEL_IO autotunig?  i.e. if we are 

Re: [Qemu-block] [PATCH v2 0/5] block: Ensure non-protocol drivers can only be selected explicitly

[Kevin Wolf]

Am 12.03.2018 um 23:07 hat Fabiano Rosas geschrieben:
> Block drivers can be selected by either protocol syntax:
> 
>   :[:options]
> 
> or explicitly:
> 
>   driver=[,option=...]
> 
> For the protocol syntax to work, drivers should set the protocol_name
> field of the BlockDriver structure and provide bdrv_file_open and
> bdrv_parse_filename implementations.
> 
> Conversely, block drivers that do not support the protocol syntax
> should instead implement bdrv_open and not have a protocol_name field.
> 
> Some drivers do not currently adhere to this and errors arise when
> trying to select them using the protocol syntax. For instance:
> 
>   $ qemu-img info replication:foo
>   qemu-img: block.c:2401: bdrv_open_inherit: \
>   Assertion `!!(flags & BDRV_O_PROTOCOL) == !!drv->bdrv_file_open' failed.
>   Aborted (core dumped)
> 
> This patch-set ensures that the following drivers are meeting the
> above criteria:
> - blkreplay
> - quorum
> - replication
> - throttle
> 
> Aside from that, documentation was added to make the above more
> explicit.

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [PATCH v1 1/1] iotests: fix test case 185

[Kevin Wolf]

Am 19.03.2018 um 18:53 hat Christian Borntraeger geschrieben:
> 
> 
> On 03/19/2018 05:10 PM, Stefan Hajnoczi wrote:
> > On Mon, Mar 19, 2018 at 9:35 AM, QingFeng Hao  
> > wrote:
> >> Test case 185 failed since commit 4486e89c219 --- "vl: introduce 
> >> vm_shutdown()".
> >> It's because of the newly introduced function vm_shutdown calls 
> >> bdrv_drain_all,
> >> which is called later by bdrv_close_all. bdrv_drain_all resumes the jobs
> >> that doubles the speed and offset is doubled.
> >> Some jobs' status are changed as well.
> >>
> >> Thus, let's not call bdrv_drain_all in vm_shutdown.
> >>
> >> Signed-off-by: QingFeng Hao 
> >> ---
> >>  cpus.c | 5 +++--
> >>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/cpus.c b/cpus.c
> >> index 2e6701795b..ae2962508c 100644
> >> --- a/cpus.c
> >> +++ b/cpus.c
> >> @@ -1006,8 +1006,9 @@ static int do_vm_stop(RunState state, bool send_stop)
> >>  qapi_event_send_stop(_abort);
> >>  }
> >>  }
> >> -
> >> -bdrv_drain_all();
> >> +if (send_stop) {
> >> +bdrv_drain_all();
> >> +}
> > 
> > Thanks for looking into this bug!
> > 
> > This if statement breaks the contract of the function when send_stop
> > is false.  Drain ensures that pending I/O completes and that device
> > emulation finishes before this function returns.  This is important
> > for live migration, where there must be no more guest-related activity
> > after vm_stop().
> > 
> > This patch relies on the caller invoking bdrv_close_all() immediately
> > after do_vm_stop(), which is not documented and could lead to more
> > bugs in the future.
> > 
> > I suggest a different solution:
> > 
> > Test 185 relies on internal QEMU behavior which can change from time
> > to time.  Buffer sizes and block job iteration counts are
> > implementation details that are not part of qapi-schema.json or other
> > documented behavior.
> > 
> > In fact, the existing test case was already broken in IOThread mode
> > since iothread_stop_all() -> bdrv_set_aio_context() also includes a
> > bdrv_drain() with the side-effect of an extra blockjob iteration.
> > 
> > After 4486e89c219 ("vl: introduce vm_shutdown()") both IOThread and
> > non-IOThread mode perform the drain.  This has caused the test
> > failure.
> > 
> > Instead of modifying QEMU to keep the same arbitrary internal behavior
> > as before, please send a patch that updates tests/qemu-iotests/185.out
> > with the latest output.
> 
> Wouldnt it be better if the test actually masks out the values the are not
> really part of the "agreed upon" behaviour? Wouldnt block_job_offset from
> common.filter be what we want?

The test case has the following note:

# Note that the reference output intentionally includes the 'offset' field in
# BLOCK_JOB_CANCELLED events for all of the following block jobs. They are
# predictable and any change in the offsets would hint at a bug in the job
# throttling code.

Now the question is if this particular change is okay rather than
hinting at a bug and we should update the reference output or whether we
need to change qemu again.

I think the change isn't really bad, but we are doing more useless work
now than we used to do before, and we're exiting slower because we're
spawning additional I/O that we have to wait for. So the better state
was certainly the old one.

Kevin