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

[QingFeng Hao]

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.

The fix is to not resume the jobs that are already yielded and also change
185.out accordingly.

Suggested-by: Stefan Hajnoczi 
Signed-off-by: QingFeng Hao 
---
 blockjob.c | 10 +-
 include/block/blockjob.h   |  5 +
 tests/qemu-iotests/185.out | 11 +--
 3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index ef3ed69ff1..fa9838ac97 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -206,11 +206,16 @@ void block_job_txn_add_job(BlockJobTxn *txn, BlockJob 
*job)
 
 static void block_job_pause(BlockJob *job)
 {
-job->pause_count++;
+if (!job->yielded) {
+job->pause_count++;
+}
 }
 
 static void block_job_resume(BlockJob *job)
 {
+if (job->yielded) {
+return;
+}
 assert(job->pause_count > 0);
 job->pause_count--;
 if (job->pause_count) {
@@ -371,6 +376,7 @@ static void block_job_sleep_timer_cb(void *opaque)
 BlockJob *job = opaque;
 
 block_job_enter(job);
+job->yielded = false;
 }
 
 void block_job_start(BlockJob *job)
@@ -935,6 +941,7 @@ void *block_job_create(const char *job_id, const 
BlockJobDriver *driver,
 job->cb= cb;
 job->opaque= opaque;
 job->busy  = false;
+job->yielded   = false;
 job->paused= true;
 job->pause_count   = 1;
 job->refcnt= 1;
@@ -1034,6 +1041,7 @@ static void block_job_do_yield(BlockJob *job, uint64_t ns)
 timer_mod(>sleep_timer, ns);
 }
 job->busy = false;
+job->yielded = true;
 block_job_unlock();
 qemu_coroutine_yield();
 
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index fc645dac68..f8f208bbcf 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -99,6 +99,11 @@ typedef struct BlockJob {
 bool ready;
 
 /**
+ * Set to true when the job is yielded.
+ */
+bool yielded;
+
+/**
  * Set to true when the job has deferred work to the main loop.
  */
 bool deferred_to_main_loop;
diff --git a/tests/qemu-iotests/185.out b/tests/qemu-iotests/185.out
index 57eaf8d699..798282e196 100644
--- a/tests/qemu-iotests/185.out
+++ b/tests/qemu-iotests/185.out
@@ -7,6 +7,7 @@ Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
 
 === Creating backing chain ===
 
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"RESUME"}
 Formatting 'TEST_DIR/t.qcow2.mid', fmt=qcow2 size=67108864 
backing_file=TEST_DIR/t.qcow2.base backing_fmt=qcow2 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
 {"return": {}}
 wrote 4194304/4194304 bytes at offset 0
@@ -25,23 +26,28 @@ Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 
backing_file=TEST_DIR/t.q
 === Start active commit job and exit qemu ===
 
 {"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"RESUME"}
 {"return": {}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 4194304, "offset": 
4194304, "speed": 65536, "type": "commit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_READY", "data": {"device": "disk", "len": 4194304, "offset": 
4194304, "speed": 65536, "type": "commit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_COMPLETED", "data": {"device": "disk", "len": 4194304, "offset": 
4194304, "speed": 65536, "type": "commit"}}
 
 === Start mirror job and exit qemu ===
 
 {"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"RESUME"}
 Formatting 'TEST_DIR/t.qcow2.copy', fmt=qcow2 size=67108864 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
 {"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_READY", "data": {"device": "disk", "len": 4194304, "offset": 
4194304, "speed": 65536, "type": "mirror"}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 4194304, "offset": 
4194304, "speed": 65536, "type": "mirror"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_COMPLETED", "data": {"device": "disk", "len": 4194304, "offset": 
4194304, "speed": 65536, "type": 

[Qemu-block] [PATCH v2 0/1] iotests: fix test case 185

[QingFeng Hao]

Hi,
This patch is to fix iotest case 185 and based on the latest commit
d522e0bd18364 --- "gitmodules: Use the QEMU mirror of qemu-palcode".
Thanks!

Change Log (v2):
* Recover the change on the call to bdrv_drain_all but don't resume the
  yielded jobs according to Stefan Hajnoczi's comment.
* Change 185.out accordingly as job's status is changed as well.

Change Log (v1):
* Remove the call to bdrv_drain_all in vm_shutdown to make case 185 passed.

QingFeng Hao (1):
  iotests: fix test case 185

 blockjob.c | 10 +-
 include/block/blockjob.h   |  5 +
 tests/qemu-iotests/185.out | 11 +--
 3 files changed, 23 insertions(+), 3 deletions(-)

-- 
2.13.5

Re: [Qemu-block] [PATCH 4/4] Remove unnecessary variables for function return value

[David Gibson]

On Thu, Mar 22, 2018 at 05:12:26PM +0100, Laurent Vivier wrote:
> Re-run Coccinelle script scripts/coccinelle/return_directly.cocci
> 
> Signed-off-by: Laurent Vivier 
> ---
>  accel/tcg/translate-all.c  |  5 +-
>  block/quorum.c |  6 +--
>  hw/arm/exynos4210.c|  7 +--
>  hw/block/vhost-user-blk.c  |  5 +-
>  hw/hppa/dino.c |  5 +-
>  hw/misc/mos6522.c  |  6 +--
>  hw/net/ftgmac100.c |  5 +-
>  hw/ppc/pnv_lpc.c   | 14 +-
>  io/net-listener.c  |  6 +--
>  target/i386/hax-darwin.c   | 10 ++--
>  target/mips/dsp_helper.c   | 15 ++
>  target/xtensa/core-dc232b/xtensa-modules.c | 56 
> ++
>  target/xtensa/core-dc233c/xtensa-modules.c | 56 
> ++
>  target/xtensa/core-de212/xtensa-modules.c  | 48 +--
>  target/xtensa/core-fsf/xtensa-modules.c| 32 -
>  .../xtensa/core-sample_controller/xtensa-modules.c | 24 +++---
>  target/xtensa/translate.c  |  5 +-
>  tests/m48t59-test.c|  6 +--
>  tests/test-thread-pool.c   |  6 +--
>  util/uri.c |  5 +-
>  20 files changed, 75 insertions(+), 247 deletions(-)

ppc part

Acked-by: David Gibson 

> 
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 5ad1b919bc..55d822d410 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -644,11 +644,8 @@ static inline void *alloc_code_gen_buffer(void)
>  static inline void *alloc_code_gen_buffer(void)
>  {
>  size_t size = tcg_ctx->code_gen_buffer_size;
> -void *buf;
> -
> -buf = VirtualAlloc(NULL, size, MEM_RESERVE | MEM_COMMIT,
> +return VirtualAlloc(NULL, size, MEM_RESERVE | MEM_COMMIT,
>  PAGE_EXECUTE_READWRITE);
> -return buf;
>  }
>  #else
>  static inline void *alloc_code_gen_buffer(void)
> diff --git a/block/quorum.c b/block/quorum.c
> index 14333c18aa..304442ef65 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -608,7 +608,7 @@ static void read_quorum_children_entry(void *opaque)
>  static int read_quorum_children(QuorumAIOCB *acb)
>  {
>  BDRVQuorumState *s = acb->bs->opaque;
> -int i, ret;
> +int i;
>  
>  acb->children_read = s->num_children;
>  for (i = 0; i < s->num_children; i++) {
> @@ -643,9 +643,7 @@ static int read_quorum_children(QuorumAIOCB *acb)
>  qemu_coroutine_yield();
>  }
>  
> -ret = acb->vote_ret;
> -
> -return ret;
> +return acb->vote_ret;
>  }
>  
>  static int read_fifo_child(QuorumAIOCB *acb)
> diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
> index 06f9d1ffa4..d5ce275b21 100644
> --- a/hw/arm/exynos4210.c
> +++ b/hw/arm/exynos4210.c
> @@ -156,12 +156,7 @@ void exynos4210_write_secondary(ARMCPU *cpu,
>  
>  static uint64_t exynos4210_calc_affinity(int cpu)
>  {
> -uint64_t mp_affinity;
> -
> -/* Exynos4210 has 0x9 as cluster ID */
> -mp_affinity = (0x9 << ARM_AFF1_SHIFT) | cpu;
> -
> -return mp_affinity;
> +return (0x9 << ARM_AFF1_SHIFT) | cpu;
>  }
>  
>  Exynos4210State *exynos4210_init(MemoryRegion *system_mem)
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index f840f07dfe..3f41ca9e26 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -196,7 +196,6 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice 
> *vdev,
>  Error **errp)
>  {
>  VHostUserBlk *s = VHOST_USER_BLK(vdev);
> -uint64_t get_features;
>  
>  /* Turn on pre-defined features */
>  virtio_add_feature(, VIRTIO_BLK_F_SEG_MAX);
> @@ -215,9 +214,7 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice 
> *vdev,
>  virtio_add_feature(, VIRTIO_BLK_F_MQ);
>  }
>  
> -get_features = vhost_get_features(>dev, user_feature_bits, features);
> -
> -return get_features;
> +return vhost_get_features(>dev, user_feature_bits, features);
>  }
>  
>  static void vhost_user_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> diff --git a/hw/hppa/dino.c b/hw/hppa/dino.c
> index 15aefde09c..c5dcf3104d 100644
> --- a/hw/hppa/dino.c
> +++ b/hw/hppa/dino.c
> @@ -403,13 +403,10 @@ static void dino_set_irq(void *opaque, int irq, int 
> level)
>  static int dino_pci_map_irq(PCIDevice *d, int irq_num)
>  {
>  int slot = d->devfn >> 3;
> -int local_irq;
>  
>  assert(irq_num >= 0 && irq_num <= 3);
>  
> -local_irq = slot & 0x03;
> -
> -return local_irq;
> +return slot & 0x03;
>  }
>  
>  static void dino_set_timer_irq(void 

Re: [Qemu-block] [Qemu-arm] [PATCH 4/4] Remove unnecessary variables for function return value

[Philippe Mathieu-Daudé]

Hi Laurent,

On 03/22/2018 01:12 PM, Laurent Vivier wrote:
> Re-run Coccinelle script scripts/coccinelle/return_directly.cocci
> 
> Signed-off-by: Laurent Vivier 
> ---
>  accel/tcg/translate-all.c  |  5 +-
>  block/quorum.c |  6 +--
>  hw/arm/exynos4210.c|  7 +--
>  hw/block/vhost-user-blk.c  |  5 +-
>  hw/hppa/dino.c |  5 +-
>  hw/misc/mos6522.c  |  6 +--
>  hw/net/ftgmac100.c |  5 +-
>  hw/ppc/pnv_lpc.c   | 14 +-
>  io/net-listener.c  |  6 +--
>  target/i386/hax-darwin.c   | 10 ++--
>  target/mips/dsp_helper.c   | 15 ++
>  target/xtensa/core-dc232b/xtensa-modules.c | 56 
> ++
>  target/xtensa/core-dc233c/xtensa-modules.c | 56 
> ++
>  target/xtensa/core-de212/xtensa-modules.c  | 48 +--
>  target/xtensa/core-fsf/xtensa-modules.c| 32 -
>  .../xtensa/core-sample_controller/xtensa-modules.c | 24 +++---
>  target/xtensa/translate.c  |  5 +-
>  tests/m48t59-test.c|  6 +--
>  tests/test-thread-pool.c   |  6 +--
>  util/uri.c |  5 +-
>  20 files changed, 75 insertions(+), 247 deletions(-)
> 
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 5ad1b919bc..55d822d410 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -644,11 +644,8 @@ static inline void *alloc_code_gen_buffer(void)
>  static inline void *alloc_code_gen_buffer(void)
>  {
>  size_t size = tcg_ctx->code_gen_buffer_size;
> -void *buf;
> -
> -buf = VirtualAlloc(NULL, size, MEM_RESERVE | MEM_COMMIT,
> +return VirtualAlloc(NULL, size, MEM_RESERVE | MEM_COMMIT,
>  PAGE_EXECUTE_READWRITE);
> -return buf;
>  }
>  #else
>  static inline void *alloc_code_gen_buffer(void)
> diff --git a/block/quorum.c b/block/quorum.c
> index 14333c18aa..304442ef65 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -608,7 +608,7 @@ static void read_quorum_children_entry(void *opaque)
>  static int read_quorum_children(QuorumAIOCB *acb)
>  {
>  BDRVQuorumState *s = acb->bs->opaque;
> -int i, ret;
> +int i;
>  
>  acb->children_read = s->num_children;
>  for (i = 0; i < s->num_children; i++) {
> @@ -643,9 +643,7 @@ static int read_quorum_children(QuorumAIOCB *acb)
>  qemu_coroutine_yield();
>  }
>  
> -ret = acb->vote_ret;
> -
> -return ret;
> +return acb->vote_ret;
>  }
>  
>  static int read_fifo_child(QuorumAIOCB *acb)
> diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
> index 06f9d1ffa4..d5ce275b21 100644
> --- a/hw/arm/exynos4210.c
> +++ b/hw/arm/exynos4210.c
> @@ -156,12 +156,7 @@ void exynos4210_write_secondary(ARMCPU *cpu,
>  
>  static uint64_t exynos4210_calc_affinity(int cpu)
>  {
> -uint64_t mp_affinity;
> -
> -/* Exynos4210 has 0x9 as cluster ID */
> -mp_affinity = (0x9 << ARM_AFF1_SHIFT) | cpu;
> -
> -return mp_affinity;
> +return (0x9 << ARM_AFF1_SHIFT) | cpu;

You should run spatch with --keep-comments.

>  }
>  
>  Exynos4210State *exynos4210_init(MemoryRegion *system_mem)
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index f840f07dfe..3f41ca9e26 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -196,7 +196,6 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice 
> *vdev,
>  Error **errp)
>  {
>  VHostUserBlk *s = VHOST_USER_BLK(vdev);
> -uint64_t get_features;
>  
>  /* Turn on pre-defined features */
>  virtio_add_feature(, VIRTIO_BLK_F_SEG_MAX);
> @@ -215,9 +214,7 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice 
> *vdev,
>  virtio_add_feature(, VIRTIO_BLK_F_MQ);
>  }
>  
> -get_features = vhost_get_features(>dev, user_feature_bits, features);
> -
> -return get_features;
> +return vhost_get_features(>dev, user_feature_bits, features);
>  }
>  
>  static void vhost_user_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> diff --git a/hw/hppa/dino.c b/hw/hppa/dino.c
> index 15aefde09c..c5dcf3104d 100644
> --- a/hw/hppa/dino.c
> +++ b/hw/hppa/dino.c
> @@ -403,13 +403,10 @@ static void dino_set_irq(void *opaque, int irq, int 
> level)
>  static int dino_pci_map_irq(PCIDevice *d, int irq_num)
>  {
>  int slot = d->devfn >> 3;
> -int local_irq;
>  
>  assert(irq_num >= 0 && irq_num <= 3);
>  
> -local_irq = slot & 0x03;
> -
> -return local_irq;
> +return slot & 0x03;
>  }
>  
>  static void dino_set_timer_irq(void *opaque, int irq, int 

Re: [Qemu-block] [PATCH v2] qemu: replace "" with <> in headers

[Paolo Bonzini]

On 22/03/2018 20:29, Michael S. Tsirkin wrote:
> On Wed, Mar 21, 2018 at 05:22:03PM +0100, Kevin Wolf wrote:
>>> It's all still very much a non-standard convention and so less robust
>>> than prefixing file name with a project-specifix prefix.
>> I've always had the impression that it's by far the most common
>> convention, to the point that I'd blindly assume it when joining a new
>> project.
> 
> Any examples?

GCC - https://github.com/gcc-mirror/gcc/blob/master/gcc/reload.c
Libvirt - https://github.com/libvirt/libvirt/blob/master/src/util/virprocess.c
SDL - https://github.com/SDL-mirror/SDL/blob/master/src/core/unix/SDL_poll.c

Anything but Linux really.

I find  verbose and unnecessary.  The only advantage
of your proposal is that files included from the source directory would be
clearly noticeable.  That said, it's easy to add a checkpatch.pl rule that
detects when ".../..." is used on a file not under include/.

Paolo

Re: [Qemu-block] [PATCH 1/1] block/file-posix.c: fix not reopened lock file descriptor

[Dion Bosschieter]

Yeah I have a use case, before a last sync on a storage migration we suspend a 
VM -> send the last diffs -> mount the new storage server and after that we 
change a symlink -> call reopen -> check if all file descriptors are changed 
before resuming the VM.

Dion

> Op 22 mrt. 2018 om 18:39 heeft Kevin Wolf  het volgende 
> geschreven:
> 
> [ Cc: qemu-block ]
> 
> Am 22.03.2018 um 18:20 hat Dion Bosschieter geschrieben:
>> In commit 244a5668106297378391b768e7288eb157616f64 another
>> file descriptor to BDRVRawState is added. When we try to issue the
>> reopen command only s->fd is reopened; lock_fd could still hold an old
>> file descriptor "possibly" pointing to another file.
>> 
>> - change raw_reopen_prepare so it checks use_lock from BDRVRawState and
>> tries to reopen lock_fd accordingly
>> - change raw_reopen_commit so it closes the old lock_fd on use_lock
>> 
>> Signed-off-by: Dion Bosschieter 
> 
> bdrv_reopen() is not meant for opening a different file, it is meant to
> change the flags and options of the same file. Do you have a use case
> where you would actually need to switch to a different file?
> 
> As far as I know, lock_fd was specifically introduced _because_ it stays
> the same across reopen, so we don't need a racy release/reacquire pair.
> Fam (CCed) should know more.
> 
> In any case, doesn't your patch drop all the locks without reacquiring
> them on the new lock_fd?
> 
> Kevin
> 
>> block/file-posix.c | 25 +
>> 1 file changed, 25 insertions(+)
>> 
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index d7fb772c14..16d83fc49e 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -167,6 +167,7 @@ typedef struct BDRVRawState {
>> 
>> typedef struct BDRVRawReopenState {
>> int fd;
>> +int lock_fd;
>> int open_flags;
>> } BDRVRawReopenState;
>> 
>> @@ -795,6 +796,7 @@ static int raw_reopen_prepare(BDRVReopenState *state,
>> raw_parse_flags(state->flags, >open_flags);
>> 
>> rs->fd = -1;
>> +rs->lock_fd = -1;
>> 
>> int fcntl_flags = O_APPEND | O_NONBLOCK;
>> #ifdef O_NOATIME
>> @@ -820,6 +822,17 @@ static int raw_reopen_prepare(BDRVReopenState *state,
>> rs->fd = -1;
>> }
>> }
>> +
>> +if (s->use_lock) {
>> +rs->lock_fd = qemu_dup(s->lock_fd);
>> +if (rs->lock_fd >= 0) {
>> +ret = fcntl_setfl(rs->lock_fd, rs->open_flags);
>> +if (ret) {
>> +qemu_close(rs->lock_fd);
>> +rs->lock_fd = -1;
>> +}
>> +}
>> +}
>> }
>> 
>> /* If we cannot use fcntl, or fcntl failed, fall back to qemu_open() */
>> @@ -835,6 +848,14 @@ static int raw_reopen_prepare(BDRVReopenState *state,
>> error_setg_errno(errp, errno, "Could not reopen file");
>> ret = -1;
>> }
>> +
>> +if (s->use_lock) {
>> +rs->lock_fd = qemu_open(normalized_filename, 
>> rs->open_flags);
>> +if (rs->lock_fd == -1) {
>> +error_setg_errno(errp, errno, "Could not reopen file 
>> for locking");
>> +ret = -1;
>> +}
>> +}
>> }
>> }
>> 
>> @@ -861,7 +882,11 @@ static void raw_reopen_commit(BDRVReopenState *state)
>> s->open_flags = rs->open_flags;
>> 
>> qemu_close(s->fd);
>> +if (s->use_lock) {
>> +qemu_close(s->lock_fd);
>> +}
>> s->fd = rs->fd;
>> +s->lock_fd = rs->lock_fd;
>> 
>> g_free(state->opaque);
>> state->opaque = NULL;
>> -- 
>> 2.14.2
>> 

Re: [Qemu-block] [PATCH 4/4] Remove unnecessary variables for function return value

[Max Filippov]

On Thu, Mar 22, 2018 at 9:12 AM, Laurent Vivier  wrote:
> Re-run Coccinelle script scripts/coccinelle/return_directly.cocci
>
> Signed-off-by: Laurent Vivier 
> ---
>  target/xtensa/core-dc232b/xtensa-modules.c | 56 
> ++
>  target/xtensa/core-dc233c/xtensa-modules.c | 56 
> ++
>  target/xtensa/core-de212/xtensa-modules.c  | 48 +--
>  target/xtensa/core-fsf/xtensa-modules.c| 32 -
>  .../xtensa/core-sample_controller/xtensa-modules.c | 24 +++---

These files were autogenerated, fixing them doesn't make much sense.

-- 
Thanks.
-- Max

Re: [Qemu-block] [PATCH 4/4] Remove unnecessary variables for function return value

[Max Filippov]

On Thu, Mar 22, 2018 at 10:23 AM, Peter Maydell
 wrote:
> On 22 March 2018 at 17:19, Max Filippov  wrote:
>> Ok, I can add a fixup that changes #include  to #include
>> "xtensa-isa.h".
>> Adding #include "qemu/osdep.h" there seems pointless to me.
>
> Every top level .c file must start with including osdep.h.
> Other headers that it might include rely on that.
> It looks like these files aren't actually top level .c files,
> but are only compiled by being #included from some other file.
> Our convention for that kind of thing is to give the file
> a .inc.c extension. If you follow that then clean-includes
> will skip the file rather than trying to apply the requirements
> for a top level .c file to it.

Ok, thanks. Will do that too.

-- 
Thanks.
-- Max

Re: [Qemu-block] [PATCH 4/4] Remove unnecessary variables for function return value

[Max Filippov]

On Thu, Mar 22, 2018 at 9:59 AM, Eric Blake  wrote:
> On 03/22/2018 11:51 AM, Max Filippov wrote:
>>
>> On Thu, Mar 22, 2018 at 9:12 AM, Laurent Vivier 
>> wrote:
>>>
>>> Re-run Coccinelle script scripts/coccinelle/return_directly.cocci
>>>
>>> Signed-off-by: Laurent Vivier 
>>> ---
>>>   target/xtensa/core-dc232b/xtensa-modules.c | 56
>>> ++
>>>   target/xtensa/core-dc233c/xtensa-modules.c | 56
>>> ++
>>>   target/xtensa/core-de212/xtensa-modules.c  | 48
>>> +--
>>>   target/xtensa/core-fsf/xtensa-modules.c| 32 -
>>>   .../xtensa/core-sample_controller/xtensa-modules.c | 24 +++---
>>
>>
>> These files were autogenerated, fixing them doesn't make much sense.
>
>
> How frequently is the generator rerun?  Is it something where we are likely
> to revert the change because it needs to be rerun soon?  If so, then is it
> worth fixing the generator to output more concise code?

They were generated once and are not supposed to be regenerated.

> Conversely, if they were generated up front, but likely to remain unchanged
> into the future, then fixing them (even though the fix differs from the
> generator) will mean they no longer show up as false positives in future
> runs of the Coccinelle script.

Ok.

> I'm also fine removing the changes to these files as part of preparing the
> PULL request, if that's what you would prefer.

The changes are fine, if they make maintenance easier they should stay.

-- 
Thanks.
-- Max

Re: [Qemu-block] [PATCH 4/4] Remove unnecessary variables for function return value

[Max Filippov]

On Thu, Mar 22, 2018 at 9:58 AM, Laurent Vivier  wrote:
> On 22/03/2018 17:51, Max Filippov wrote:
>> On Thu, Mar 22, 2018 at 9:12 AM, Laurent Vivier  wrote:
>>> Re-run Coccinelle script scripts/coccinelle/return_directly.cocci
>>>
>>> Signed-off-by: Laurent Vivier 
>>> ---
>>>  target/xtensa/core-dc232b/xtensa-modules.c | 56 
>>> ++
>>>  target/xtensa/core-dc233c/xtensa-modules.c | 56 
>>> ++
>>>  target/xtensa/core-de212/xtensa-modules.c  | 48 +--
>>>  target/xtensa/core-fsf/xtensa-modules.c| 32 -
>>>  .../xtensa/core-sample_controller/xtensa-modules.c | 24 +++---
>>
>> These files were autogenerated, fixing them doesn't make much sense.
>
> Good to know. They have been already updated by a couple of patches:
>
> 8f0a3716e4 Clean up includes
> d8e39b7062 Use #include "..." for our own headers, <...> for others
>
> Perhaps import_core.sh can be updated?

Ok, I can add a fixup that changes #include  to #include
"xtensa-isa.h".
Adding #include "qemu/osdep.h" there seems pointless to me.

-- 
Thanks.
-- Max

Re: [Qemu-block] [PATCH v2 1/2] make: move generated headers to qemu-build/

[Michael S. Tsirkin]

On Thu, Mar 22, 2018 at 02:42:55PM -0500, Eric Blake wrote:
> On 03/22/2018 02:27 PM, Michael S. Tsirkin wrote:
> > Make sure all generated files go into qemu-build subdirectory.
> > We can then include them like this:
> >   #include "qemu-build/trace.h"
> > 
> > This serves two purposes:
> > - make it easy to detect which files are in the source
> >directory (a bit more work for writers, easier for readers)
> > - reduce chances of conflicts with possible stale files in source
> >directory (which could be left over from e.g. old patches, etc)
> > 
> > This patch needs to be merged with patch 2  of series updating all
> > files: sending it separately to avoid spamming the list.
> > 
> > Signed-off-by: Michael S. Tsirkin 
> > ---
> 
> > +++ b/Makefile
> > @@ -89,102 +89,102 @@ endif
> >   include $(SRC_PATH)/rules.mak
> > -GENERATED_FILES = qemu-version.h config-host.h qemu-options.def
> > -GENERATED_FILES += qapi/qapi-builtin-types.h qapi/qapi-builtin-types.c
> 
> Uggh - I really need to follow up on my threat to make smarter use of make
> variables and string manipulation to cut down on the boilerplate involved
> here.  Sadly, I'm not convinced that doing so is a 2.12 bugfix priority, so
> it isn't at the top of my work queue.
> 
> Overall, the patch is an interesting idea.  I'm still not 100% sold on it
> (as you say, it's now slightly more work for writers), but I'm not coming up
> with any solid reasons why it should not be applied (at least, for 2.13 -
> doing it during freeze for 2.12 is a bit harder to justify).

It's up to Peter really: it helps reduce conflicts if we apply patches
like this during freeze.  But with enough effort on Pater's part it's
not a huge deal.

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

Re: [Qemu-block] [PATCH v2 1/2] make: move generated headers to qemu-build/

[Eric Blake]

On 03/22/2018 02:27 PM, Michael S. Tsirkin wrote:

Make sure all generated files go into qemu-build subdirectory.
We can then include them like this:
  #include "qemu-build/trace.h"

This serves two purposes:
- make it easy to detect which files are in the source
   directory (a bit more work for writers, easier for readers)
- reduce chances of conflicts with possible stale files in source
   directory (which could be left over from e.g. old patches, etc)

This patch needs to be merged with patch 2  of series updating all
files: sending it separately to avoid spamming the list.

Signed-off-by: Michael S. Tsirkin 
---



+++ b/Makefile
@@ -89,102 +89,102 @@ endif
  
  include $(SRC_PATH)/rules.mak
  
-GENERATED_FILES = qemu-version.h config-host.h qemu-options.def

-GENERATED_FILES += qapi/qapi-builtin-types.h qapi/qapi-builtin-types.c


Uggh - I really need to follow up on my threat to make smarter use of 
make variables and string manipulation to cut down on the boilerplate 
involved here.  Sadly, I'm not convinced that doing so is a 2.12 bugfix 
priority, so it isn't at the top of my work queue.


Overall, the patch is an interesting idea.  I'm still not 100% sold on 
it (as you say, it's now slightly more work for writers), but I'm not 
coming up with any solid reasons why it should not be applied (at least, 
for 2.13 - doing it during freeze for 2.12 is a bit harder to justify).


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

Re: [Qemu-block] [PATCH v2] qemu: replace "" with <> in headers

[Michael S. Tsirkin]

On Wed, Mar 21, 2018 at 05:22:03PM +0100, Kevin Wolf wrote:
> > It's all still very much a non-standard convention and so less robust
> > than prefixing file name with a project-specifix prefix.
> 
> I've always had the impression that it's by far the most common
> convention, to the point that I'd blindly assume it when joining a new
> project.

Any examples?

> > > > As another example of problems, a header by the same name in the source
> > > > directory will always be picked up first - before any headers in
> > > > the include directory.
> > > > 
> > > > Let's change the scheme: make sure all headers that are not
> > > > in the source directory are included through a path
> > > > starting with qemu/ , thus:
> > > > 
> > > >  #include <>
> > > > 
> > > > headers in the same directory as source are included with
> > > > 
> > > >  #include ""
> > > > 
> > > > as per standard.
> > > > 
> > > > This (untested) patch is just to start the discussion and does not
> > > > change all of the codebase. If there's agreement, this will be
> > > > run on all code to converting code to this scheme.
> > > 
> > > Renaming files is always painful. If that's the fix, the cure might be
> > > worse than the disease. As far as I know, the conflict is only
> > > theoretical, so in that case I'd say: If it ain't broke, don't fix it.
> > > 
> > > Kevin
> > 
> > It's broke I think, it's very hard for new people to contribute to QEMU.
> > Look e.g. at rdma which all has messed up includes - and that's from an
> > experienced conributor who just isn't an experienced maintainer.
> 
> I don't think the problem is that the convention is hard to apply (it's
> definitely not). It's knowing about the convention. This problem isn't
> going away by switching to a different, less common convention. We're
> only going to see more offenders then.

Not if we have some automatic tools to catch violators.

> > Amount of time spent on teaching new people trivia about our
> > conventions just isn't funny. They should be self-documenting
> > and violations should cause the build to fail.
> 
> Yes, but your proposal doesn't achieve this. You can still use
> "qemu/foo.h" instead of  and it will build successfully.
> That's something we can't change, as far as I know, because the include
> path for "foo.h" is always a superset of .

If the rule is that "" is only for files in the current directory
then we can easily code up a checkpatch script to catch violators.

> If anything, this means that we should prefer "foo.h" for local headers
> (i.e. the way it currently is) because we can let the compiler enforce
> it:  for "foo.h" can become a build error, and does so with your
> -iquote patch, but the other way round doesn't work.
> 
> Then it's only system headers that you can possibly get wrong, but for
> those everyone should be used to using  anyway.
> 
> Kevin

If my proposal to prefix all include directories with qemu/
is accepted, then we can solve the stale file problem
by prohibiting a directory named qemu everywhere in source.


-- 
MST

[Qemu-block] [PATCH v2 1/2] make: move generated headers to qemu-build/

[Michael S. Tsirkin]

Make sure all generated files go into qemu-build subdirectory.
We can then include them like this:
 #include "qemu-build/trace.h"

This serves two purposes:
- make it easy to detect which files are in the source
  directory (a bit more work for writers, easier for readers)
- reduce chances of conflicts with possible stale files in source
  directory (which could be left over from e.g. old patches, etc)

This patch needs to be merged with patch 2  of series updating all
files: sending it separately to avoid spamming the list.

Signed-off-by: Michael S. Tsirkin 
---
 configure   |   6 +-
 Makefile| 412 +++-
 rules.mak   |   5 +-
 .gitignore  |   1 +
 Makefile.objs   | 144 +-
 Makefile.target |  21 +--
 trace/Makefile.objs |  15 +-
 7 files changed, 313 insertions(+), 291 deletions(-)

diff --git a/configure b/configure
index 23a4f3b..7b0a183 100755
--- a/configure
+++ b/configure
@@ -6638,6 +6638,8 @@ if test "$gcov" = "yes" ; then
   echo "GCOV=$gcov_tool" >> $config_host_mak
 fi
 
+mkdir -p qemu-build
+
 # use included Linux headers
 if test "$linux" = "yes" ; then
   mkdir -p linux-headers
@@ -7046,10 +7048,10 @@ echo "QEMU_CFLAGS+=$cflags" >> $config_target_mak
 done # for target in $targets
 
 if [ "$dtc_internal" = "yes" ]; then
-  echo "config-host.h: subdir-dtc" >> $config_host_mak
+  echo "qemu-build/config-host.h: subdir-dtc" >> $config_host_mak
 fi
 if [ "$capstone" = "git" -o "$capstone" = "internal" ]; then
-  echo "config-host.h: subdir-capstone" >> $config_host_mak
+  echo "qemu-build/config-host.h: subdir-capstone" >> $config_host_mak
 fi
 if test -n "$LIBCAPSTONE"; then
   echo "LIBCAPSTONE=$LIBCAPSTONE" >> $config_host_mak
diff --git a/Makefile b/Makefile
index f799390..6fd90a8 100644
--- a/Makefile
+++ b/Makefile
@@ -89,102 +89,102 @@ endif
 
 include $(SRC_PATH)/rules.mak
 
-GENERATED_FILES = qemu-version.h config-host.h qemu-options.def
-GENERATED_FILES += qapi/qapi-builtin-types.h qapi/qapi-builtin-types.c
-GENERATED_FILES += qapi/qapi-types.h qapi/qapi-types.c
-GENERATED_FILES += qapi/qapi-types-block-core.h qapi/qapi-types-block-core.c
-GENERATED_FILES += qapi/qapi-types-block.h qapi/qapi-types-block.c
-GENERATED_FILES += qapi/qapi-types-char.h qapi/qapi-types-char.c
-GENERATED_FILES += qapi/qapi-types-common.h qapi/qapi-types-common.c
-GENERATED_FILES += qapi/qapi-types-crypto.h qapi/qapi-types-crypto.c
-GENERATED_FILES += qapi/qapi-types-introspect.h qapi/qapi-types-introspect.c
-GENERATED_FILES += qapi/qapi-types-migration.h qapi/qapi-types-migration.c
-GENERATED_FILES += qapi/qapi-types-misc.h qapi/qapi-types-misc.c
-GENERATED_FILES += qapi/qapi-types-net.h qapi/qapi-types-net.c
-GENERATED_FILES += qapi/qapi-types-rocker.h qapi/qapi-types-rocker.c
-GENERATED_FILES += qapi/qapi-types-run-state.h qapi/qapi-types-run-state.c
-GENERATED_FILES += qapi/qapi-types-sockets.h qapi/qapi-types-sockets.c
-GENERATED_FILES += qapi/qapi-types-tpm.h qapi/qapi-types-tpm.c
-GENERATED_FILES += qapi/qapi-types-trace.h qapi/qapi-types-trace.c
-GENERATED_FILES += qapi/qapi-types-transaction.h qapi/qapi-types-transaction.c
-GENERATED_FILES += qapi/qapi-types-ui.h qapi/qapi-types-ui.c
-GENERATED_FILES += qapi/qapi-builtin-visit.h qapi/qapi-builtin-visit.c
-GENERATED_FILES += qapi/qapi-visit.h qapi/qapi-visit.c
-GENERATED_FILES += qapi/qapi-visit-block-core.h qapi/qapi-visit-block-core.c
-GENERATED_FILES += qapi/qapi-visit-block.h qapi/qapi-visit-block.c
-GENERATED_FILES += qapi/qapi-visit-char.h qapi/qapi-visit-char.c
-GENERATED_FILES += qapi/qapi-visit-common.h qapi/qapi-visit-common.c
-GENERATED_FILES += qapi/qapi-visit-crypto.h qapi/qapi-visit-crypto.c
-GENERATED_FILES += qapi/qapi-visit-introspect.h qapi/qapi-visit-introspect.c
-GENERATED_FILES += qapi/qapi-visit-migration.h qapi/qapi-visit-migration.c
-GENERATED_FILES += qapi/qapi-visit-misc.h qapi/qapi-visit-misc.c
-GENERATED_FILES += qapi/qapi-visit-net.h qapi/qapi-visit-net.c
-GENERATED_FILES += qapi/qapi-visit-rocker.h qapi/qapi-visit-rocker.c
-GENERATED_FILES += qapi/qapi-visit-run-state.h qapi/qapi-visit-run-state.c
-GENERATED_FILES += qapi/qapi-visit-sockets.h qapi/qapi-visit-sockets.c
-GENERATED_FILES += qapi/qapi-visit-tpm.h qapi/qapi-visit-tpm.c
-GENERATED_FILES += qapi/qapi-visit-trace.h qapi/qapi-visit-trace.c
-GENERATED_FILES += qapi/qapi-visit-transaction.h qapi/qapi-visit-transaction.c
-GENERATED_FILES += qapi/qapi-visit-ui.h qapi/qapi-visit-ui.c
-GENERATED_FILES += qapi/qapi-commands.h qapi/qapi-commands.c
-GENERATED_FILES += qapi/qapi-commands-block-core.h 
qapi/qapi-commands-block-core.c
-GENERATED_FILES += qapi/qapi-commands-block.h qapi/qapi-commands-block.c
-GENERATED_FILES += qapi/qapi-commands-char.h qapi/qapi-commands-char.c
-GENERATED_FILES += qapi/qapi-commands-common.h qapi/qapi-commands-common.c
-GENERATED_FILES += qapi/qapi-commands-crypto.h qapi/qapi-commands-crypto.c
-GENERATED_FILES += 

Re: [Qemu-block] [Qemu-devel] [PATCH 0/4] coccinelle: re-run scripts from scripts/coccinelle

[Peter Maydell]

On 22 March 2018 at 19:12, Eric Blake  wrote:
> Or if we don't patch the false negative, you can bypass checkpatch with an
> ugly hack:
>
> return 0 + (...) | (...);
>
> (I'm NOT going to do that bypass - it's too ugly for my taste)

Yeah, that's definitely not something we should be doing.
checkpatch has plenty of false positives anyway, ignoring one
more is no big deal.

thanks
-- PMM

Re: [Qemu-block] [Qemu-devel] [PATCH 0/4] coccinelle: re-run scripts from scripts/coccinelle

[Eric Blake]

On 03/22/2018 01:00 PM, Peter Maydell wrote:

On 22 March 2018 at 17:57,   wrote:

Checking PATCH 4/4: Remove unnecessary variables for function return value...
ERROR: return is not a function, parentheses are not required
#251: FILE: target/mips/dsp_helper.c:3281:
+return (temp[1] << 63) | (temp[0] >> 1);


This looks like a bug in checkpatch. I guess to fix it you'd need
to make checkpatch count opening and closing parens in the line
to see if it goes to 0 somewhere other than just before the ';'...


Or if we don't patch the false negative, you can bypass checkpatch with 
an ugly hack:


return 0 + (...) | (...);

(I'm NOT going to do that bypass - it's too ugly for my taste)

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

Re: [Qemu-block] [Qemu-devel] [PATCH 0/4] coccinelle: re-run scripts from scripts/coccinelle

[Peter Maydell]

On 22 March 2018 at 17:57,   wrote:
> Checking PATCH 4/4: Remove unnecessary variables for function return value...
> ERROR: return is not a function, parentheses are not required
> #251: FILE: target/mips/dsp_helper.c:3281:
> +return (temp[1] << 63) | (temp[0] >> 1);

This looks like a bug in checkpatch. I guess to fix it you'd need
to make checkpatch count opening and closing parens in the line
to see if it goes to 0 somewhere other than just before the ';'...

thanks
-- PMM

Re: [Qemu-block] [Qemu-devel] [PATCH 0/4] coccinelle: re-run scripts from scripts/coccinelle

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180322161226.29796-1-lviv...@redhat.com
Subject: [Qemu-devel] [PATCH 0/4] coccinelle: re-run scripts from 
scripts/coccinelle

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]patchew/20180321144107.22770-1-kw...@redhat.com -> 
patchew/20180321144107.22770-1-kw...@redhat.com
Switched to a new branch 'test'
a1c7275b57 Remove unnecessary variables for function return value
a530fa5a72 qdict: remove useless cast
c1bfe26b11 error: Remove NULL checks on error_propagate() calls
db3d55e58e error: Strip trailing '\n' from error string arguments (again again)

=== OUTPUT BEGIN ===
Checking PATCH 1/4: error: Strip trailing '\n' from error string arguments 
(again again)...
Checking PATCH 2/4: error: Remove NULL checks on error_propagate() calls...
Checking PATCH 3/4: qdict: remove useless cast...
Checking PATCH 4/4: Remove unnecessary variables for function return value...
ERROR: return is not a function, parentheses are not required
#251: FILE: target/mips/dsp_helper.c:3281:
+return (temp[1] << 63) | (temp[0] >> 1);

ERROR: return is not a function, parentheses are not required
#270: FILE: target/mips/dsp_helper.c:3308:
+return (temp[1] << 63) | (temp[0] >> 1);

ERROR: return is not a function, parentheses are not required
#289: FILE: target/mips/dsp_helper.c:3341:
+return (temp[1] << 63) | (temp[0] >> 1);

total: 3 errors, 0 warnings, 813 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

Re: [Qemu-block] [PATCH 4/4] Remove unnecessary variables for function return value

[Peter Maydell]

On 22 March 2018 at 17:30, Eric Blake  wrote:
> I'm less certain of whether our Coccinelle scripts have easy ways to exclude
> specific files.  We already have scripts/cocci-macro-file.h to help
> Coccinelle not choke on some our existing files, but I'm not sure if
> Coccinelle has a config-file like way that is easy to maintain as a data
> file in-tree for blacklist files to leave alone (right now, when I run
> Coccinelle, I have to manually remember to pass a long command line cribbed
> out of the commit message of an earlier run to pick up things like
> cocci-macro-file.h, instead of an easy formula that points to a single
> config file to pull in all the usual options).

Coccinelle itself doesn't seem to have a config file mechanism.
We could probably approximate one by using a wrapper script
that skips some files and run the passed spatch file on the rest;
scripts/clean-includes has some "skip files we don't want to try
to run on" code that could perhaps be generalized.

thanks
-- PMM

Re: [Qemu-block] [PATCH 1/1] block/file-posix.c: fix not reopened lock file descriptor

[Kevin Wolf]

[ Cc: qemu-block ]

Am 22.03.2018 um 18:20 hat Dion Bosschieter geschrieben:
> In commit 244a5668106297378391b768e7288eb157616f64 another
> file descriptor to BDRVRawState is added. When we try to issue the
> reopen command only s->fd is reopened; lock_fd could still hold an old
> file descriptor "possibly" pointing to another file.
> 
> - change raw_reopen_prepare so it checks use_lock from BDRVRawState and
> tries to reopen lock_fd accordingly
> - change raw_reopen_commit so it closes the old lock_fd on use_lock
> 
> Signed-off-by: Dion Bosschieter 

bdrv_reopen() is not meant for opening a different file, it is meant to
change the flags and options of the same file. Do you have a use case
where you would actually need to switch to a different file?

As far as I know, lock_fd was specifically introduced _because_ it stays
the same across reopen, so we don't need a racy release/reacquire pair.
Fam (CCed) should know more.

In any case, doesn't your patch drop all the locks without reacquiring
them on the new lock_fd?

Kevin

>  block/file-posix.c | 25 +
>  1 file changed, 25 insertions(+)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index d7fb772c14..16d83fc49e 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -167,6 +167,7 @@ typedef struct BDRVRawState {
>  
>  typedef struct BDRVRawReopenState {
>  int fd;
> +int lock_fd;
>  int open_flags;
>  } BDRVRawReopenState;
>  
> @@ -795,6 +796,7 @@ static int raw_reopen_prepare(BDRVReopenState *state,
>  raw_parse_flags(state->flags, >open_flags);
>  
>  rs->fd = -1;
> +rs->lock_fd = -1;
>  
>  int fcntl_flags = O_APPEND | O_NONBLOCK;
>  #ifdef O_NOATIME
> @@ -820,6 +822,17 @@ static int raw_reopen_prepare(BDRVReopenState *state,
>  rs->fd = -1;
>  }
>  }
> +
> +if (s->use_lock) {
> +rs->lock_fd = qemu_dup(s->lock_fd);
> +if (rs->lock_fd >= 0) {
> +ret = fcntl_setfl(rs->lock_fd, rs->open_flags);
> +if (ret) {
> +qemu_close(rs->lock_fd);
> +rs->lock_fd = -1;
> +}
> +}
> +}
>  }
>  
>  /* If we cannot use fcntl, or fcntl failed, fall back to qemu_open() */
> @@ -835,6 +848,14 @@ static int raw_reopen_prepare(BDRVReopenState *state,
>  error_setg_errno(errp, errno, "Could not reopen file");
>  ret = -1;
>  }
> +
> +if (s->use_lock) {
> +rs->lock_fd = qemu_open(normalized_filename, rs->open_flags);
> +if (rs->lock_fd == -1) {
> +error_setg_errno(errp, errno, "Could not reopen file for 
> locking");
> +ret = -1;
> +}
> +}
>  }
>  }
>  
> @@ -861,7 +882,11 @@ static void raw_reopen_commit(BDRVReopenState *state)
>  s->open_flags = rs->open_flags;
>  
>  qemu_close(s->fd);
> +if (s->use_lock) {
> +qemu_close(s->lock_fd);
> +}
>  s->fd = rs->fd;
> +s->lock_fd = rs->lock_fd;
>  
>  g_free(state->opaque);
>  state->opaque = NULL;
> -- 
> 2.14.2
> 

Re: [Qemu-block] [PATCH 4/4] Remove unnecessary variables for function return value

[Eric Blake]

On 03/22/2018 12:19 PM, Max Filippov wrote:


These files were autogenerated, fixing them doesn't make much sense.


Good to know. They have been already updated by a couple of patches:

8f0a3716e4 Clean up includes
d8e39b7062 Use #include "..." for our own headers, <...> for others

Perhaps import_core.sh can be updated?


Ok, I can add a fixup that changes #include  to #include
"xtensa-isa.h".
Adding #include "qemu/osdep.h" there seems pointless to me.


Or we can add more exceptions to our tooling to recognize the files as 
generated.  The scripts/clean-includes script knows to leave certain 
files alone, so add your files to that list.


I'm less certain of whether our Coccinelle scripts have easy ways to 
exclude specific files.  We already have scripts/cocci-macro-file.h to 
help Coccinelle not choke on some our existing files, but I'm not sure 
if Coccinelle has a config-file like way that is easy to maintain as a 
data file in-tree for blacklist files to leave alone (right now, when I 
run Coccinelle, I have to manually remember to pass a long command line 
cribbed out of the commit message of an earlier run to pick up things 
like cocci-macro-file.h, instead of an easy formula that points to a 
single config file to pull in all the usual options).


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

Re: [Qemu-block] [PATCH 4/4] Remove unnecessary variables for function return value

[Peter Maydell]

On 22 March 2018 at 17:19, Max Filippov  wrote:
> Ok, I can add a fixup that changes #include  to #include
> "xtensa-isa.h".
> Adding #include "qemu/osdep.h" there seems pointless to me.

Every top level .c file must start with including osdep.h.
Other headers that it might include rely on that.
It looks like these files aren't actually top level .c files,
but are only compiled by being #included from some other file.
Our convention for that kind of thing is to give the file
a .inc.c extension. If you follow that then clean-includes
will skip the file rather than trying to apply the requirements
for a top level .c file to it.

thanks
-- PMM

Re: [Qemu-block] [PATCH 4/4] Remove unnecessary variables for function return value

[Eric Blake]

On 03/22/2018 11:51 AM, Max Filippov wrote:

On Thu, Mar 22, 2018 at 9:12 AM, Laurent Vivier  wrote:

Re-run Coccinelle script scripts/coccinelle/return_directly.cocci

Signed-off-by: Laurent Vivier 
---
  target/xtensa/core-dc232b/xtensa-modules.c | 56 ++
  target/xtensa/core-dc233c/xtensa-modules.c | 56 ++
  target/xtensa/core-de212/xtensa-modules.c  | 48 +--
  target/xtensa/core-fsf/xtensa-modules.c| 32 -
  .../xtensa/core-sample_controller/xtensa-modules.c | 24 +++---


These files were autogenerated, fixing them doesn't make much sense.


How frequently is the generator rerun?  Is it something where we are 
likely to revert the change because it needs to be rerun soon?  If so, 
then is it worth fixing the generator to output more concise code?


Conversely, if they were generated up front, but likely to remain 
unchanged into the future, then fixing them (even though the fix differs 
from the generator) will mean they no longer show up as false positives 
in future runs of the Coccinelle script.


I'm also fine removing the changes to these files as part of preparing 
the PULL request, if that's what you would prefer.


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

Re: [Qemu-block] [PATCH 4/4] Remove unnecessary variables for function return value

[Laurent Vivier]

On 22/03/2018 17:51, Max Filippov wrote:
> On Thu, Mar 22, 2018 at 9:12 AM, Laurent Vivier  wrote:
>> Re-run Coccinelle script scripts/coccinelle/return_directly.cocci
>>
>> Signed-off-by: Laurent Vivier 
>> ---
>>  target/xtensa/core-dc232b/xtensa-modules.c | 56 
>> ++
>>  target/xtensa/core-dc233c/xtensa-modules.c | 56 
>> ++
>>  target/xtensa/core-de212/xtensa-modules.c  | 48 +--
>>  target/xtensa/core-fsf/xtensa-modules.c| 32 -
>>  .../xtensa/core-sample_controller/xtensa-modules.c | 24 +++---
> 
> These files were autogenerated, fixing them doesn't make much sense.

Good to know. They have been already updated by a couple of patches:

8f0a3716e4 Clean up includes
d8e39b7062 Use #include "..." for our own headers, <...> for others

Perhaps import_core.sh can be updated?

Thanks,
Laurent

Re: [Qemu-block] [PATCH 0/4] coccinelle: re-run scripts from scripts/coccinelle

[Eric Blake]

On 03/22/2018 11:12 AM, Laurent Vivier wrote:

I've re-run some scripts from the coccinelle directory,
and they have found some problems.

This series fixes them.

Laurent Vivier (4):
   error: Strip trailing '\n' from error string arguments (again again)
   error: Remove NULL checks on error_propagate() calls
   qdict: remove useless cast
   Remove unnecessary variables for function return value


I've gone through all the patches to double-check for no surprises; and 
found some formatting nits on 4 that can be touched up on pull request. 
Series:

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/4] Remove unnecessary variables for function return value

[Eric Blake]

On 03/22/2018 11:12 AM, Laurent Vivier wrote:

Re-run Coccinelle script scripts/coccinelle/return_directly.cocci

Signed-off-by: Laurent Vivier 
---



+++ b/hw/arm/exynos4210.c
@@ -156,12 +156,7 @@ void exynos4210_write_secondary(ARMCPU *cpu,
  
  static uint64_t exynos4210_calc_affinity(int cpu)

  {
-uint64_t mp_affinity;
-
-/* Exynos4210 has 0x9 as cluster ID */
-mp_affinity = (0x9 << ARM_AFF1_SHIFT) | cpu;
-
-return mp_affinity;
+return (0x9 << ARM_AFF1_SHIFT) | cpu;


Unchanged by this rewrite, but since this is converting a signed 32-bit 
int to an unsigned 64-bit value, are we sure that the upper 32 bits are 
always set correctly?  (Using unsigned values earlier in the expression 
would require less head-scratching on whether it is correct).  Any 
changes should be a separate fix by the file's maintainer.




+++ b/hw/misc/mos6522.c
@@ -176,12 +176,8 @@ static void mos6522_set_sr_int(MOS6522State *s)
  
  static uint64_t mos6522_get_counter_value(MOS6522State *s, MOS6522Timer *ti)

  {
-uint64_t d;
-
-d = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - ti->load_time,
+return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - ti->load_time,
   ti->frequency, NANOSECONDS_PER_SECOND);


Coccinelle missed that indentation is now off here.


+++ b/hw/ppc/pnv_lpc.c
@@ -125,25 +125,15 @@ static int pnv_lpc_dt_xscom(PnvXScomInterface *dev, void 
*fdt, int xscom_offset)
  static bool opb_read(PnvLpcController *lpc, uint32_t addr, uint8_t *data,
   int sz)
  {
-bool success;
-
-/* XXX Handle access size limits and FW read caching here */
-success = !address_space_rw(>opb_as, addr, MEMTXATTRS_UNSPECIFIED,
+return !address_space_rw(>opb_as, addr, MEMTXATTRS_UNSPECIFIED,
  data, sz, false);


and here.


+++ b/target/xtensa/translate.c
@@ -1272,11 +1272,8 @@ XtensaOpcodeOps *
  xtensa_find_opcode_ops(const XtensaOpcodeTranslators *t,
 const char *name)
  {
-XtensaOpcodeOps *ops;
-
-ops = bsearch(name, t->opcode, t->num_opcodes,
+return bsearch(name, t->opcode, t->num_opcodes,
sizeof(XtensaOpcodeOps), compare_opcode_ops);


and here


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

Re: [Qemu-block] [PATCH 4/4] qapi: new qmp command nbd-server-add-bitmap

[Vladimir Sementsov-Ogievskiy]

22.03.2018 19:19, Eric Blake wrote:

On 03/22/2018 10:43 AM, Vladimir Sementsov-Ogievskiy wrote:


+# Returns: error on one of the following conditions:
+#   - the server is not running
+#   - export is not found
+#   - bitmap is not found
+#   - bitmap is disabled
+#   - bitmap is locked


Do we really need to list all the error conditions?  My worry is 
that a list this specific might go stale, compared to the obvious 
default that the command succeeds only if it was able to expose the 
bitmap and that the error message is specific enough for a human to 
figure out what to fix if it failed.



Hmm, I've just doing it similar with other commands in the file. Is 
there any requirements on this part of qapi documentation? I can 
write only "# Returns: nothing on success" line, is it appropriate? 
blockdev-mirror do so, but other commands tries to describe errors. 
Looks like we lack some specified format for return code description 
like we have for parameters..


Yeah, for returns, it's been very ad hoc.  My personal feel (although 
it's not very well documented and certainly not enforced): all 
commands can reasonably return errors, presumably for a good reason; 
but exhaustively auditing WHICH errors is a huge task with little 
benefits. A few commands return non-generic errors, but if all error 
paths used error_setg(), there's nothing that a machine can do to tell 
the difference between the errors, so documenting different error 
reasons doesn't add much.


Thus, if a command returns nothing on success, I'm fine with omitting 
'Returns:' entirely, and the doc generator permits that. But if you 
have bothered to list Returns: for certain errors, I'm not going to 
blindly throw away the documentation work, even though the list may 
become incomplete over time.




So, only something interesting worth documenting.

Hmm, interesting: consider for example bloc-dirty-bitmap-remove. It says:
# Returns: nothing on success
#  If @node is not a valid block device or node, DeviceNotFound

But the code uses bdrv_lookup_bs, which uses simple error_setg, so it 
will return GenericError, isn't it? And, it's not the only place..


--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH 0/3] coroutine: avoid co_queue_wakeup recursion

[Paolo Bonzini]

On 22/03/2018 16:28, Stefan Hajnoczi wrote:
> co_queue_wakeup is currently implemented in a recursive fashion.  Pathological
> patterns of aio_co_enter() between coroutines can cause stack exhaustion.
> 
> This patch series implements co_queue_wakeup iteratively and avoids stack
> exhaustion.
> 
> This issue was originally reported with qemu-img convert but I don't have a
> good reproducer.  See Patch 3 for a test-aio test case instead.
> 
> Stefan Hajnoczi (3):
>   queue: add QSIMPLEQ_PREPEND()
>   coroutine: avoid co_queue_wakeup recursion
>   coroutine: add test-aio coroutine queue chaining test case
> 
>  include/qemu/coroutine_int.h |   1 -
>  include/qemu/queue.h |   8 
>  block/io.c   |   3 +-
>  tests/test-aio.c |  65 -
>  util/qemu-coroutine-lock.c   |  34 -
>  util/qemu-coroutine.c| 110 
> +++
>  6 files changed, 120 insertions(+), 101 deletions(-)
> 

Reviewed-by: Paolo Bonzini 

I was a little surprised by the disappearing of the "do not use co
anymore" comments, but they seem unnecessary indeed with the new code.

Paolo

Re: [Qemu-block] [PATCH 0/4] coccinelle: re-run scripts from scripts/coccinelle

[Eric Blake]

On 03/22/2018 11:12 AM, Laurent Vivier wrote:

I've re-run some scripts from the coccinelle directory,
and they have found some problems.

This series fixes them.

Laurent Vivier (4):
   error: Strip trailing '\n' from error string arguments (again again)


This is user-visible, so appropriate during freeze as a bug fix.  It's 
closes to the error/qapi code that Markus normally maintains (and where 
I'm filling in while he is out), so I'll queue this one on my QAPI tree.



   error: Remove NULL checks on error_propagate() calls
   qdict: remove useless cast
   Remove unnecessary variables for function return value


These are cosmetic; they should be no-ops.  2 and 3 are again best 
through error/qapi, while 4 is more generic, but there's little reason 
to split up the series just because 4 is not as closely related.  Unless 
anyone has an opinion otherwise, I'm planning to also include them in my 
QAPI tree for 2.12.


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

Re: [Qemu-block] [PATCH 3/4] qdict: remove useless cast

[Dr. David Alan Gilbert]

* Laurent Vivier (lviv...@redhat.com) wrote:
> Re-run Coccinelle script scripts/coccinelle/qobject.cocci
> 
> Signed-off-by: Laurent Vivier 
> ---
>  block/nvme.c | 11 +--
>  monitor.c|  2 +-
>  2 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 8bca57aae6..c4f3a7bc94 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -695,12 +695,11 @@ static void nvme_parse_filename(const char *filename, 
> QDict *options,
>  unsigned long ns;
>  const char *slash = strchr(tmp, '/');
>  if (!slash) {
> -qdict_put(options, NVME_BLOCK_OPT_DEVICE,
> -  qstring_from_str(tmp));
> +qdict_put_str(options, NVME_BLOCK_OPT_DEVICE, tmp);
>  return;
>  }
>  device = g_strndup(tmp, slash - tmp);
> -qdict_put(options, NVME_BLOCK_OPT_DEVICE, qstring_from_str(device));
> +qdict_put_str(options, NVME_BLOCK_OPT_DEVICE, device);
>  g_free(device);
>  namespace = slash + 1;
>  if (*namespace && qemu_strtoul(namespace, NULL, 10, )) {
> @@ -708,8 +707,8 @@ static void nvme_parse_filename(const char *filename, 
> QDict *options,
> namespace);
>  return;
>  }
> -qdict_put(options, NVME_BLOCK_OPT_NAMESPACE,
> -  qstring_from_str(*namespace ? namespace : "1"));
> +qdict_put_str(options, NVME_BLOCK_OPT_NAMESPACE,
> +  *namespace ? namespace : "1");
>  }
>  }
>  
> @@ -1082,7 +1081,7 @@ static void nvme_refresh_filename(BlockDriverState *bs, 
> QDict *opts)
>   bs->drv->format_name);
>  }
>  
> -qdict_put(opts, "driver", qstring_from_str(bs->drv->format_name));
> +qdict_put_str(opts, "driver", bs->drv->format_name);
>  bs->full_open_options = opts;
>  }
>  
> diff --git a/monitor.c b/monitor.c
> index 6ccd2fc089..6e7667d82f 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4316,7 +4316,7 @@ static QObject *get_qmp_greeting(Monitor *mon)
>  /* Monitors that are not using IOThread won't support OOB */
>  continue;
>  }
> -qlist_append(cap_list, qstring_from_str(QMPCapability_str(cap)));
> +qlist_append_str(cap_list, QMPCapability_str(cap));
>  }
>  
>  return qobject_from_jsonf("{'QMP': {'version': %p, 'capabilities': %p}}",

For monitor:

Acked-by: Dr. David Alan Gilbert 

> -- 
> 2.14.3
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-block] [PATCH 4/4] qapi: new qmp command nbd-server-add-bitmap

[Eric Blake]

On 03/22/2018 10:43 AM, Vladimir Sementsov-Ogievskiy wrote:


+# Returns: error on one of the following conditions:
+#   - the server is not running
+#   - export is not found
+#   - bitmap is not found
+#   - bitmap is disabled
+#   - bitmap is locked


Do we really need to list all the error conditions?  My worry is that 
a list this specific might go stale, compared to the obvious default 
that the command succeeds only if it was able to expose the bitmap and 
that the error message is specific enough for a human to figure out 
what to fix if it failed.



Hmm, I've just doing it similar with other commands in the file. Is 
there any requirements on this part of qapi documentation? I can write 
only "# Returns: nothing on success" line, is it appropriate? 
blockdev-mirror do so, but other commands tries to describe errors. 
Looks like we lack some specified format for return code description 
like we have for parameters..


Yeah, for returns, it's been very ad hoc.  My personal feel (although 
it's not very well documented and certainly not enforced): all commands 
can reasonably return errors, presumably for a good reason; but 
exhaustively auditing WHICH errors is a huge task with little benefits. 
A few commands return non-generic errors, but if all error paths used 
error_setg(), there's nothing that a machine can do to tell the 
difference between the errors, so documenting different error reasons 
doesn't add much.


Thus, if a command returns nothing on success, I'm fine with omitting 
'Returns:' entirely, and the doc generator permits that.  But if you 
have bothered to list Returns: for certain errors, I'm not going to 
blindly throw away the documentation work, even though the list may 
become incomplete over time.


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

[Qemu-block] [PATCH 3/4] qdict: remove useless cast

[Laurent Vivier]

Re-run Coccinelle script scripts/coccinelle/qobject.cocci

Signed-off-by: Laurent Vivier 
---
 block/nvme.c | 11 +--
 monitor.c|  2 +-
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 8bca57aae6..c4f3a7bc94 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -695,12 +695,11 @@ static void nvme_parse_filename(const char *filename, 
QDict *options,
 unsigned long ns;
 const char *slash = strchr(tmp, '/');
 if (!slash) {
-qdict_put(options, NVME_BLOCK_OPT_DEVICE,
-  qstring_from_str(tmp));
+qdict_put_str(options, NVME_BLOCK_OPT_DEVICE, tmp);
 return;
 }
 device = g_strndup(tmp, slash - tmp);
-qdict_put(options, NVME_BLOCK_OPT_DEVICE, qstring_from_str(device));
+qdict_put_str(options, NVME_BLOCK_OPT_DEVICE, device);
 g_free(device);
 namespace = slash + 1;
 if (*namespace && qemu_strtoul(namespace, NULL, 10, )) {
@@ -708,8 +707,8 @@ static void nvme_parse_filename(const char *filename, QDict 
*options,
namespace);
 return;
 }
-qdict_put(options, NVME_BLOCK_OPT_NAMESPACE,
-  qstring_from_str(*namespace ? namespace : "1"));
+qdict_put_str(options, NVME_BLOCK_OPT_NAMESPACE,
+  *namespace ? namespace : "1");
 }
 }
 
@@ -1082,7 +1081,7 @@ static void nvme_refresh_filename(BlockDriverState *bs, 
QDict *opts)
  bs->drv->format_name);
 }
 
-qdict_put(opts, "driver", qstring_from_str(bs->drv->format_name));
+qdict_put_str(opts, "driver", bs->drv->format_name);
 bs->full_open_options = opts;
 }
 
diff --git a/monitor.c b/monitor.c
index 6ccd2fc089..6e7667d82f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4316,7 +4316,7 @@ static QObject *get_qmp_greeting(Monitor *mon)
 /* Monitors that are not using IOThread won't support OOB */
 continue;
 }
-qlist_append(cap_list, qstring_from_str(QMPCapability_str(cap)));
+qlist_append_str(cap_list, QMPCapability_str(cap));
 }
 
 return qobject_from_jsonf("{'QMP': {'version': %p, 'capabilities': %p}}",
-- 
2.14.3

[Qemu-block] [PATCH 4/4] Remove unnecessary variables for function return value

[Laurent Vivier]

Re-run Coccinelle script scripts/coccinelle/return_directly.cocci

Signed-off-by: Laurent Vivier 
---
 accel/tcg/translate-all.c  |  5 +-
 block/quorum.c |  6 +--
 hw/arm/exynos4210.c|  7 +--
 hw/block/vhost-user-blk.c  |  5 +-
 hw/hppa/dino.c |  5 +-
 hw/misc/mos6522.c  |  6 +--
 hw/net/ftgmac100.c |  5 +-
 hw/ppc/pnv_lpc.c   | 14 +-
 io/net-listener.c  |  6 +--
 target/i386/hax-darwin.c   | 10 ++--
 target/mips/dsp_helper.c   | 15 ++
 target/xtensa/core-dc232b/xtensa-modules.c | 56 ++
 target/xtensa/core-dc233c/xtensa-modules.c | 56 ++
 target/xtensa/core-de212/xtensa-modules.c  | 48 +--
 target/xtensa/core-fsf/xtensa-modules.c| 32 -
 .../xtensa/core-sample_controller/xtensa-modules.c | 24 +++---
 target/xtensa/translate.c  |  5 +-
 tests/m48t59-test.c|  6 +--
 tests/test-thread-pool.c   |  6 +--
 util/uri.c |  5 +-
 20 files changed, 75 insertions(+), 247 deletions(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 5ad1b919bc..55d822d410 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -644,11 +644,8 @@ static inline void *alloc_code_gen_buffer(void)
 static inline void *alloc_code_gen_buffer(void)
 {
 size_t size = tcg_ctx->code_gen_buffer_size;
-void *buf;
-
-buf = VirtualAlloc(NULL, size, MEM_RESERVE | MEM_COMMIT,
+return VirtualAlloc(NULL, size, MEM_RESERVE | MEM_COMMIT,
 PAGE_EXECUTE_READWRITE);
-return buf;
 }
 #else
 static inline void *alloc_code_gen_buffer(void)
diff --git a/block/quorum.c b/block/quorum.c
index 14333c18aa..304442ef65 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -608,7 +608,7 @@ static void read_quorum_children_entry(void *opaque)
 static int read_quorum_children(QuorumAIOCB *acb)
 {
 BDRVQuorumState *s = acb->bs->opaque;
-int i, ret;
+int i;
 
 acb->children_read = s->num_children;
 for (i = 0; i < s->num_children; i++) {
@@ -643,9 +643,7 @@ static int read_quorum_children(QuorumAIOCB *acb)
 qemu_coroutine_yield();
 }
 
-ret = acb->vote_ret;
-
-return ret;
+return acb->vote_ret;
 }
 
 static int read_fifo_child(QuorumAIOCB *acb)
diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
index 06f9d1ffa4..d5ce275b21 100644
--- a/hw/arm/exynos4210.c
+++ b/hw/arm/exynos4210.c
@@ -156,12 +156,7 @@ void exynos4210_write_secondary(ARMCPU *cpu,
 
 static uint64_t exynos4210_calc_affinity(int cpu)
 {
-uint64_t mp_affinity;
-
-/* Exynos4210 has 0x9 as cluster ID */
-mp_affinity = (0x9 << ARM_AFF1_SHIFT) | cpu;
-
-return mp_affinity;
+return (0x9 << ARM_AFF1_SHIFT) | cpu;
 }
 
 Exynos4210State *exynos4210_init(MemoryRegion *system_mem)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index f840f07dfe..3f41ca9e26 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -196,7 +196,6 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice 
*vdev,
 Error **errp)
 {
 VHostUserBlk *s = VHOST_USER_BLK(vdev);
-uint64_t get_features;
 
 /* Turn on pre-defined features */
 virtio_add_feature(, VIRTIO_BLK_F_SEG_MAX);
@@ -215,9 +214,7 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice 
*vdev,
 virtio_add_feature(, VIRTIO_BLK_F_MQ);
 }
 
-get_features = vhost_get_features(>dev, user_feature_bits, features);
-
-return get_features;
+return vhost_get_features(>dev, user_feature_bits, features);
 }
 
 static void vhost_user_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
diff --git a/hw/hppa/dino.c b/hw/hppa/dino.c
index 15aefde09c..c5dcf3104d 100644
--- a/hw/hppa/dino.c
+++ b/hw/hppa/dino.c
@@ -403,13 +403,10 @@ static void dino_set_irq(void *opaque, int irq, int level)
 static int dino_pci_map_irq(PCIDevice *d, int irq_num)
 {
 int slot = d->devfn >> 3;
-int local_irq;
 
 assert(irq_num >= 0 && irq_num <= 3);
 
-local_irq = slot & 0x03;
-
-return local_irq;
+return slot & 0x03;
 }
 
 static void dino_set_timer_irq(void *opaque, int irq, int level)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 8ad9fc831e..606532aa65 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -176,12 +176,8 @@ static void mos6522_set_sr_int(MOS6522State *s)
 
 static uint64_t mos6522_get_counter_value(MOS6522State *s, MOS6522Timer *ti)
 {
-uint64_t d;
-
-d = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - 

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

[Laurent Vivier]

Re-run Coccinelle patch
scripts/coccinelle/error_propagate_null.cocci

Signed-off-by: Laurent Vivier 
---
 io/channel-websock.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/io/channel-websock.c b/io/channel-websock.c
index ec48a305f0..e6608b969d 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -586,9 +586,7 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel 
*ioc,
 return TRUE;
 }
 
-if (err) {
-error_propagate(>io_err, err);
-}
+error_propagate(>io_err, err);
 
 trace_qio_channel_websock_handshake_reply(ioc);
 qio_channel_add_watch(
-- 
2.14.3

[Qemu-block] [PATCH 1/4] error: Strip trailing '\n' from error string arguments (again again)

[Laurent Vivier]

Re-run Coccinelle script scripts/coccinelle/err-bad-newline.cocci,
and found new error_report() occurences with '\n'.

Signed-off-by: Laurent Vivier 
---
 target/i386/hvf/hvf.c | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index 15870a4f36..c36753954b 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -86,25 +86,25 @@ static void assert_hvf_ok(hv_return_t ret)
 
 switch (ret) {
 case HV_ERROR:
-error_report("Error: HV_ERROR\n");
+error_report("Error: HV_ERROR");
 break;
 case HV_BUSY:
-error_report("Error: HV_BUSY\n");
+error_report("Error: HV_BUSY");
 break;
 case HV_BAD_ARGUMENT:
-error_report("Error: HV_BAD_ARGUMENT\n");
+error_report("Error: HV_BAD_ARGUMENT");
 break;
 case HV_NO_RESOURCES:
-error_report("Error: HV_NO_RESOURCES\n");
+error_report("Error: HV_NO_RESOURCES");
 break;
 case HV_NO_DEVICE:
-error_report("Error: HV_NO_DEVICE\n");
+error_report("Error: HV_NO_DEVICE");
 break;
 case HV_UNSUPPORTED:
-error_report("Error: HV_UNSUPPORTED\n");
+error_report("Error: HV_UNSUPPORTED");
 break;
 default:
-error_report("Unknown Error\n");
+error_report("Unknown Error");
 }
 
 abort();
@@ -191,7 +191,7 @@ void hvf_set_phys_mem(MemoryRegionSection *section, bool 
add)
 if (mem) {
 mem->size = 0;
 if (do_hvf_set_memory(mem)) {
-error_report("Failed to reset overlapping slot\n");
+error_report("Failed to reset overlapping slot");
 abort();
 }
 }
@@ -211,7 +211,7 @@ void hvf_set_phys_mem(MemoryRegionSection *section, bool 
add)
 }
 
 if (x == hvf_state->num_slots) {
-error_report("No free slots\n");
+error_report("No free slots");
 abort();
 }
 
@@ -221,7 +221,7 @@ void hvf_set_phys_mem(MemoryRegionSection *section, bool 
add)
 mem->region = area;
 
 if (do_hvf_set_memory(mem)) {
-error_report("Error registering new memory slot\n");
+error_report("Error registering new memory slot");
 abort();
 }
 }
@@ -884,7 +884,7 @@ int hvf_vcpu_exec(CPUState *cpu)
 break;
 }
 default:
-error_report("Unrecognized CR %d\n", cr);
+error_report("Unrecognized CR %d", cr);
 abort();
 }
 RIP(env) += ins_len;
@@ -930,7 +930,7 @@ int hvf_vcpu_exec(CPUState *cpu)
 env->error_code = 0;
 break;
 default:
-error_report("%llx: unhandled exit %llx\n", rip, exit_reason);
+error_report("%llx: unhandled exit %llx", rip, exit_reason);
 }
 } while (ret == 0);
 
-- 
2.14.3

[Qemu-block] [PATCH 0/4] coccinelle: re-run scripts from scripts/coccinelle

[Laurent Vivier]

I've re-run some scripts from the coccinelle directory,
and they have found some problems.

This series fixes them.

Laurent Vivier (4):
  error: Strip trailing '\n' from error string arguments (again again)
  error: Remove NULL checks on error_propagate() calls
  qdict: remove useless cast
  Remove unnecessary variables for function return value

 accel/tcg/translate-all.c  |  5 +-
 block/nvme.c   | 11 ++---
 block/quorum.c |  6 +--
 hw/arm/exynos4210.c|  7 +--
 hw/block/vhost-user-blk.c  |  5 +-
 hw/hppa/dino.c |  5 +-
 hw/misc/mos6522.c  |  6 +--
 hw/net/ftgmac100.c |  5 +-
 hw/ppc/pnv_lpc.c   | 14 +-
 io/channel-websock.c   |  4 +-
 io/net-listener.c  |  6 +--
 monitor.c  |  2 +-
 target/i386/hax-darwin.c   | 10 ++--
 target/i386/hvf/hvf.c  | 24 +-
 target/mips/dsp_helper.c   | 15 ++
 target/xtensa/core-dc232b/xtensa-modules.c | 56 ++
 target/xtensa/core-dc233c/xtensa-modules.c | 56 ++
 target/xtensa/core-de212/xtensa-modules.c  | 48 +--
 target/xtensa/core-fsf/xtensa-modules.c| 32 -
 .../xtensa/core-sample_controller/xtensa-modules.c | 24 +++---
 target/xtensa/translate.c  |  5 +-
 tests/m48t59-test.c|  6 +--
 tests/test-thread-pool.c   |  6 +--
 util/uri.c |  5 +-
 24 files changed, 94 insertions(+), 269 deletions(-)

-- 
2.14.3

Re: [Qemu-block] [PATCH 4/4] qapi: new qmp command nbd-server-add-bitmap

[Vladimir Sementsov-Ogievskiy]

21.03.2018 20:33, Eric Blake wrote:

On 03/21/2018 07:19 AM, Vladimir Sementsov-Ogievskiy wrote:

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  qapi/block.json | 27 +++
  blockdev-nbd.c  | 23 +++
  2 files changed, 50 insertions(+)

diff --git a/qapi/block.json b/qapi/block.json
index c694524002..4afbbcd7b7 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -269,6 +269,33 @@
    'data': {'name': 'str', '*mode': 'NbdServerRemoveMode'} }
    ##
+# @nbd-server-add-bitmap:
+#
+# Export dirty bitmap through selected export. Bitmaps are searched 
for in
+# device attached to the export and in all its backings. Exported 
bitmap

+# is locked until NBD export is removed.


Expose a dirty bitmap associated with the selected export.  The bitmap 
search starts at the device attached to the export, and includes all 
backing files.  The exported bitmap is then locked until the NBD 
export is removed.



+#
+# @name: Export name.
+#
+# @bitmap: Bitmap name to search.


s/search./search for./


+#
+# @bitmap-export-name: How the bitmap will be seen by nbd clients
+#  (default @bitmap)


Maybe mention that the client must use NBD_OPT_SET_META_CONTEXT with a 
query of "qemu-dirty-bitmap:NAME" (where NAME matches 
bitmap-export-name) to access the exposed bitmap.  (May need to be 
adjusted by my suggestion to use just the namespace "qemu:")


ok




+#
+#
+# Returns: error on one of the following conditions:
+#   - the server is not running
+#   - export is not found
+#   - bitmap is not found
+#   - bitmap is disabled
+#   - bitmap is locked


Do we really need to list all the error conditions?  My worry is that 
a list this specific might go stale, compared to the obvious default 
that the command succeeds only if it was able to expose the bitmap and 
that the error message is specific enough for a human to figure out 
what to fix if it failed.



Hmm, I've just doing it similar with other commands in the file. Is 
there any requirements on this part of qapi documentation? I can write 
only "# Returns: nothing on success" line, is it appropriate? 
blockdev-mirror do so, but other commands tries to describe errors. 
Looks like we lack some specified format for return code description 
like we have for parameters..





+#
+# Since: 2.13
+##
+  { 'command': 'nbd-server-add-bitmap',
+    'data': {'name': 'str', 'bitmap': 'str', '*bitmap-export-name': 
'str'} }

+





--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH 3/4] nbd/server: implement dirty bitmap export

[Vladimir Sementsov-Ogievskiy]

21.03.2018 19:57, Eric Blake wrote:

On 03/21/2018 07:19 AM, Vladimir Sementsov-Ogievskiy wrote:

Signed-off-by: Vladimir Sementsov-Ogievskiy 




[...]




+
+#define NBD_STATE_DIRTY 1


Does this belong better in nbd.h, alongside NBD_STATE_HOLE?  (And 
defining it as (1 << 0) might be nicer, too).


It's not used anywhere else, but it may be worth moving, for consistency 
and for future users. Ok, I'll move.



[...]


  +/* nbd_meta_bitmap_query
+ *
+ * Handle query to 'qemu-dirty-bitmap' namespace.
+ * 'len' is the amount of text remaining to be read from the current 
name, after

+ * the 'qemu-dirty-bitmap:' portion has been stripped.
+ *
+ * Return -errno on I/O error, 0 if option was completely handled by
+ * sending a reply about inconsistent lengths, or 1 on success. */
+static int nbd_meta_bitmap_query(NBDClient *client, 
NBDExportMetaContexts *meta,

+ uint32_t len, Error **errp)
+{
+    if (!client->exp->export_bitmap) {
+    return nbd_opt_skip(client, len, errp);
+    }
+
+    return nbd_meta_single_query(client, 
client->exp->export_bitmap_name, len,

+ >dirty_bitmap, errp);


So if I'm reading this right, a client can do _LIST_ 
'qemu-dirty-bitmap:' (or 'qemu:dirty-bitmap:' if we choose a shorter 
initial namespace) and get back the exported bitmap name; or the user 
already knows the bitmap name and both _LIST_ and _SET_ 
'qemu-dirty-bitmap:name' return the one supported bitmap for that NBD 
server.


yes





  /* nbd_co_send_extents
+ *
+ * NBD_REPLY_FLAG_DONE is not set, don't forget to send it.
+ *
   * @extents should be in big-endian */
  static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
 NBDExtent *extents, unsigned 
nb_extents,


Worth a bool flag that the caller can inform this function whether to 
include FLAG_DONE?


It was simpler to just send it separately, after all BLOCK_STATUS 
replies. So, I didn't need it.



+
+static int nbd_co_send_bitmap(NBDClient *client, uint64_t handle,
+  BdrvDirtyBitmap *bitmap, uint64_t offset,
+  uint64_t length, uint32_t context_id,
+  Error **errp)
+{
+    int ret;
+    unsigned nb_extents;
+    NBDExtent *extents = g_new(NBDExtent, NBD_MAX_BITMAP_EXTENTS);


Is this correct even when the client used NBD_CMD_FLAG_REQ_ONE?



Oops, looks like a bug.

--
Best regards,
Vladimir

[Qemu-block] [PATCH 2/3] coroutine: avoid co_queue_wakeup recursion

[Stefan Hajnoczi]

qemu_aio_coroutine_enter() is (indirectly) called recursively when
processing co_queue_wakeup.  This can lead to stack exhaustion.

This patch rewrites co_queue_wakeup in an iterative fashion (instead of
recursive) with bounded memory usage to prevent stack exhaustion.

qemu_co_queue_run_restart() is inlined into qemu_aio_coroutine_enter()
and the qemu_coroutine_enter() call is turned into a loop to avoid
recursion.

There is one change that is worth mentioning:  Previously, when
coroutine A queued coroutine B, qemu_co_queue_run_restart() entered
coroutine B from coroutine A.  If A was terminating then it would still
stay alive until B yielded.  After this patch B is entered by A's parent
so that a A can be deleted immediately if it is terminating.

It is safe to make this change since B could never interact with A if it
was terminating anyway.

Signed-off-by: Stefan Hajnoczi 
---
 include/qemu/coroutine_int.h |   1 -
 block/io.c   |   3 +-
 util/qemu-coroutine-lock.c   |  34 -
 util/qemu-coroutine.c| 110 +++
 4 files changed, 60 insertions(+), 88 deletions(-)

diff --git a/include/qemu/coroutine_int.h b/include/qemu/coroutine_int.h
index 59e8406398..bd6b0468e1 100644
--- a/include/qemu/coroutine_int.h
+++ b/include/qemu/coroutine_int.h
@@ -68,6 +68,5 @@ Coroutine *qemu_coroutine_new(void);
 void qemu_coroutine_delete(Coroutine *co);
 CoroutineAction qemu_coroutine_switch(Coroutine *from, Coroutine *to,
   CoroutineAction action);
-void coroutine_fn qemu_co_queue_run_restart(Coroutine *co);
 
 #endif
diff --git a/block/io.c b/block/io.c
index 2b09c656d0..bd9a19a9c4 100644
--- a/block/io.c
+++ b/block/io.c
@@ -249,8 +249,7 @@ static void coroutine_fn 
bdrv_co_yield_to_drain(BlockDriverState *bs,
 BdrvCoDrainData data;
 
 /* Calling bdrv_drain() from a BH ensures the current coroutine yields and
- * other coroutines run if they were queued from
- * qemu_co_queue_run_restart(). */
+ * other coroutines run if they were queued by aio_co_enter(). */
 
 assert(qemu_in_coroutine());
 data = (BdrvCoDrainData) {
diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c
index 5a80c10690..27438a1858 100644
--- a/util/qemu-coroutine-lock.c
+++ b/util/qemu-coroutine-lock.c
@@ -68,40 +68,6 @@ void coroutine_fn qemu_co_queue_wait_impl(CoQueue *queue, 
QemuLockable *lock)
 }
 }
 
-/**
- * qemu_co_queue_run_restart:
- *
- * Enter each coroutine that was previously marked for restart by
- * qemu_co_queue_next() or qemu_co_queue_restart_all().  This function is
- * invoked by the core coroutine code when the current coroutine yields or
- * terminates.
- */
-void qemu_co_queue_run_restart(Coroutine *co)
-{
-Coroutine *next;
-QSIMPLEQ_HEAD(, Coroutine) tmp_queue_wakeup =
-QSIMPLEQ_HEAD_INITIALIZER(tmp_queue_wakeup);
-
-trace_qemu_co_queue_run_restart(co);
-
-/* Because "co" has yielded, any coroutine that we wakeup can resume it.
- * If this happens and "co" terminates, co->co_queue_wakeup becomes
- * invalid memory.  Therefore, use a temporary queue and do not touch
- * the "co" coroutine as soon as you enter another one.
- *
- * In its turn resumed "co" can populate "co_queue_wakeup" queue with
- * new coroutines to be woken up.  The caller, who has resumed "co",
- * will be responsible for traversing the same queue, which may cause
- * a different wakeup order but not any missing wakeups.
- */
-QSIMPLEQ_CONCAT(_queue_wakeup, >co_queue_wakeup);
-
-while ((next = QSIMPLEQ_FIRST(_queue_wakeup))) {
-QSIMPLEQ_REMOVE_HEAD(_queue_wakeup, co_queue_next);
-qemu_coroutine_enter(next);
-}
-}
-
 static bool qemu_co_queue_do_restart(CoQueue *queue, bool single)
 {
 Coroutine *next;
diff --git a/util/qemu-coroutine.c b/util/qemu-coroutine.c
index 9eff7fd450..1ba4191b84 100644
--- a/util/qemu-coroutine.c
+++ b/util/qemu-coroutine.c
@@ -104,57 +104,65 @@ static void coroutine_delete(Coroutine *co)
 
 void qemu_aio_coroutine_enter(AioContext *ctx, Coroutine *co)
 {
-Coroutine *self = qemu_coroutine_self();
-CoroutineAction ret;
-
-/* Cannot rely on the read barrier for co in aio_co_wake(), as there are
- * callers outside of aio_co_wake() */
-const char *scheduled = atomic_mb_read(>scheduled);
-
-trace_qemu_aio_coroutine_enter(ctx, self, co, co->entry_arg);
-
-/* if the Coroutine has already been scheduled, entering it again will
- * cause us to enter it twice, potentially even after the coroutine has
- * been deleted */
-if (scheduled) {
-fprintf(stderr,
-"%s: Co-routine was already scheduled in '%s'\n",
-__func__, scheduled);
-abort();
-}
-
-if (co->caller) {
-fprintf(stderr, "Co-routine re-entered recursively\n");
-abort();
-}
-
-co->caller = self;
-

[Qemu-block] [PATCH 1/3] queue: add QSIMPLEQ_PREPEND()

[Stefan Hajnoczi]

QSIMPLEQ_CONCAT(a, b) joins a = a + b.  The new QSIMPLEQ_PREPEND(a, b)
API joins a = b + a.

Signed-off-by: Stefan Hajnoczi 
---
 include/qemu/queue.h | 8 
 1 file changed, 8 insertions(+)

diff --git a/include/qemu/queue.h b/include/qemu/queue.h
index aa270d2b38..59fd1203a1 100644
--- a/include/qemu/queue.h
+++ b/include/qemu/queue.h
@@ -324,6 +324,14 @@ struct {   
 \
 }   \
 } while (/*CONSTCOND*/0)
 
+#define QSIMPLEQ_PREPEND(head1, head2) do { \
+if (!QSIMPLEQ_EMPTY((head2))) { \
+*(head2)->sqh_last = (head1)->sqh_first;\
+(head1)->sqh_first = (head2)->sqh_first;  \
+QSIMPLEQ_INIT((head2)); \
+}   \
+} while (/*CONSTCOND*/0)
+
 #define QSIMPLEQ_LAST(head, type, field)\
 (QSIMPLEQ_EMPTY((head)) ?   \
 NULL :  \
-- 
2.14.3

[Qemu-block] [PATCH 3/3] coroutine: add test-aio coroutine queue chaining test case

[Stefan Hajnoczi]

Check that two coroutines can queue each other repeatedly without
hitting stack exhaustion.

Switch to qemu_init_main_loop() in main() because coroutines use
qemu_get_aio_context() - they don't know about test-aio's ctx variable.

Signed-off-by: Stefan Hajnoczi 
---
 tests/test-aio.c | 65 
 1 file changed, 52 insertions(+), 13 deletions(-)

diff --git a/tests/test-aio.c b/tests/test-aio.c
index 54e20d6ab1..86fb73b3d5 100644
--- a/tests/test-aio.c
+++ b/tests/test-aio.c
@@ -16,6 +16,8 @@
 #include "qemu/timer.h"
 #include "qemu/sockets.h"
 #include "qemu/error-report.h"
+#include "qemu/coroutine.h"
+#include "qemu/main-loop.h"
 
 static AioContext *ctx;
 
@@ -827,24 +829,59 @@ static void test_source_timer_schedule(void)
 timer_del();
 }
 
+/*
+ * Check that aio_co_enter() can chain many times
+ *
+ * Two coroutines should be able to invoke each other via aio_co_enter() many
+ * times without hitting a limit like stack exhaustion.  In other words, the
+ * calls should be chained instead of nested.
+ */
+
+typedef struct {
+Coroutine *other;
+unsigned i;
+unsigned max;
+} ChainData;
+
+static void coroutine_fn chain(void *opaque)
+{
+ChainData *data = opaque;
+
+for (data->i = 0; data->i < data->max; data->i++) {
+/* Queue up the other coroutine... */
+aio_co_enter(ctx, data->other);
+
+/* ...and give control to it */
+qemu_coroutine_yield();
+}
+}
+
+static void test_queue_chaining(void)
+{
+/* This number of iterations hit stack exhaustion in the past: */
+ChainData data_a = { .max = 25000 };
+ChainData data_b = { .max = 25000 };
+
+data_b.other = qemu_coroutine_create(chain, _a);
+data_a.other = qemu_coroutine_create(chain, _b);
+
+qemu_coroutine_enter(data_b.other);
+
+g_assert_cmpint(data_a.i, ==, data_a.max);
+g_assert_cmpint(data_b.i, ==, data_b.max - 1);
+
+/* Allow the second coroutine to terminate */
+qemu_coroutine_enter(data_a.other);
+
+g_assert_cmpint(data_b.i, ==, data_b.max);
+}
 
 /* End of tests.  */
 
 int main(int argc, char **argv)
 {
-Error *local_error = NULL;
-GSource *src;
-
-init_clocks(NULL);
-
-ctx = aio_context_new(_error);
-if (!ctx) {
-error_reportf_err(local_error, "Failed to create AIO Context: ");
-exit(1);
-}
-src = aio_get_g_source(ctx);
-g_source_attach(src, NULL);
-g_source_unref(src);
+qemu_init_main_loop(_fatal);
+ctx = qemu_get_aio_context();
 
 while (g_main_context_iteration(NULL, false));
 
@@ -864,6 +901,8 @@ int main(int argc, char **argv)
 g_test_add_func("/aio/external-client", test_aio_external_client);
 g_test_add_func("/aio/timer/schedule",  test_timer_schedule);
 
+g_test_add_func("/aio/coroutine/queue-chaining", test_queue_chaining);
+
 g_test_add_func("/aio-gsource/flush",   test_source_flush);
 g_test_add_func("/aio-gsource/bh/schedule", 
test_source_bh_schedule);
 g_test_add_func("/aio-gsource/bh/schedule10",   
test_source_bh_schedule10);
-- 
2.14.3

[Qemu-block] [PATCH 0/3] coroutine: avoid co_queue_wakeup recursion

[Stefan Hajnoczi]

co_queue_wakeup is currently implemented in a recursive fashion.  Pathological
patterns of aio_co_enter() between coroutines can cause stack exhaustion.

This patch series implements co_queue_wakeup iteratively and avoids stack
exhaustion.

This issue was originally reported with qemu-img convert but I don't have a
good reproducer.  See Patch 3 for a test-aio test case instead.

Stefan Hajnoczi (3):
  queue: add QSIMPLEQ_PREPEND()
  coroutine: avoid co_queue_wakeup recursion
  coroutine: add test-aio coroutine queue chaining test case

 include/qemu/coroutine_int.h |   1 -
 include/qemu/queue.h |   8 
 block/io.c   |   3 +-
 tests/test-aio.c |  65 -
 util/qemu-coroutine-lock.c   |  34 -
 util/qemu-coroutine.c| 110 +++
 6 files changed, 120 insertions(+), 101 deletions(-)

-- 
2.14.3

Re: [Qemu-block] [PATCH 3/4] nbd/server: implement dirty bitmap export

[Vladimir Sementsov-Ogievskiy]

21.03.2018 19:57, Eric Blake wrote:

On 03/21/2018 07:19 AM, Vladimir Sementsov-Ogievskiy wrote:

Signed-off-by: Vladimir Sementsov-Ogievskiy 


Rather sparse on the details in the commit message; I had to read the 
patch to even learn what the new namespace is.


oh, yes, sorry :(


@@ -92,6 +101,7 @@ typedef struct NBDExportMetaContexts {
  bool valid; /* means that negotiation of the option finished 
without

 errors */
  bool base_allocation; /* export base:allocation context (block 
status) */
+    bool dirty_bitmap; /* export 
qemu-dirty-bitmap: */


That's a LONG namespace name, and it does not lend itself well to 
other qemu extensions; so future qemu BLOCK_STATUS additions would 
require consuming yet another namespace and additional upstream NBD 
registration.  Wouldn't it be better to have the namespace be 'qemu:' 
(which we register with upstream NBD just once), where we are then 
free to document leafnames to look like 'dirty-bitmap:name' or 'foo:bar'?


No doubts. (and this shows, that we may want namespaces in namespaces, 
so we'll parse ':' anyway).


Only one note here (I'm ashamed): 'qemu-dirty-bitmap' namespace is used 
in Virtuozzo for the feature, yes, without 'x-' prefix. It's my fault, 
and it should not influence final solution. The probability of problems 
is near to zero (especially if we decide now to use 'qemu:' namespace 
for all qemu-specific things. But I'm not sure about, should we mention 
this fact in the spec.

[cc NBD list]

--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH 1/4] nbd/server: refactor nbd_negotiate_meta_query for several namespaces

[Vladimir Sementsov-Ogievskiy]

21.03.2018 17:56, Eric Blake wrote:

[adding NBD list]

On 03/21/2018 07:19 AM, Vladimir Sementsov-Ogievskiy wrote:

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  nbd/server.c | 60 
+++-

  1 file changed, 43 insertions(+), 17 deletions(-)



+struct {
+    const char *ns;
+    int (*func)(NBDClient *, NBDExportMetaContexts *, uint32_t, 
Error **);

+} meta_namespace_handlers[] = {
+    /* namespaces should go in non-decreasing order by name length */
+    {.ns = "base:", .func = nbd_meta_base_query},
+};
+


@@ -787,9 +793,12 @@ static int nbd_negotiate_meta_query(NBDClient 
*client,
  NBDExportMetaContexts *meta, 
Error **errp)

  {
  int ret;
-    char query[sizeof("base:") - 1];
-    size_t baselen = strlen("base:");
+    int i;
  uint32_t len;
+    int bytes_done = 0;
+    char *query;
+    int nb_ns = sizeof(meta_namespace_handlers) /
+    sizeof(meta_namespace_handlers[0]);


Use the ARRAY_SIZE() macro here.


+    query = g_malloc(strlen(meta_namespace_handlers[nb_ns - 1].ns));


So this sizes a buffer according to the largest namespace we expect to 
handle,...



  -    len -= baselen;
-    ret = nbd_opt_read(client, query, baselen, errp);
-    if (ret <= 0) {
-    return ret;
-    }
-    if (strncmp(query, "base:", baselen) != 0) {
-    return nbd_opt_skip(client, len, errp);
+    for (i = 0; i < nb_ns; i++) {
+    const char *ns = meta_namespace_handlers[i].ns;
+    int ns_len = strlen(ns);
+    int diff_len = strlen(ns) - bytes_done;
+
+    assert(diff_len >= 0);
+
+    if (diff_len > 0) {
+    if (len < diff_len) {
+    ret = nbd_opt_skip(client, len, errp);
+    goto out;
+    }
+
+    len -= diff_len;
+    ret = nbd_opt_read(client, query + bytes_done, diff_len, 
errp);

+    if (ret <= 0) {
+    goto out;
+    }
+    }
+
+    if (!strncmp(query, ns, ns_len)) {
+    ret = meta_namespace_handlers[i].func(client, meta, len, 
errp);

+    goto out;
+    }



...then you do multiple iterative reads as you step through the list. 
You know, if you encounter a ':' at any point in the iterative reads, 
you don't have to continue through the rest of the handlers (the query 
belongs to a short-named namespace we didn't recognize).


Is it any smarter to just blindly do a single read of MIN(querylen, 
maxlen), then strchr() for ':' (if not found, it's not a namespace we 
recognize), and then look up if the name matches, at which point we 
then read the rest of the query and refactor the namespace handler to 
be passed the already-parsed leafname, rather than having to parse the 
leafname off the wire in the handler?


but what if we get base:very-long-garbage? Only namespace handler knows, 
should we read leafname to the memory or just drop. So, in your case, 
we'll have to refactor it to handle partly read query..

for now, we can do it as simple as:

1. read first letter.
2. if first_letter == 'b' and len == strlen(base:alloction): read, 
check, set meta.base_allocation if match
3. if first_letter == 'q' and len == strlen(qemu-drity-bitmap:) + 
strlen(exp.export_bitmap_name): read, check, set meta.dirty_bitmap if match


and forget about generic code, until we really need it



I'm STILL wondering if the NBD spec should specify namespace and 
leafname as separate fields rather than requiring the server to parse 
for ':'.  We have only a couple of weeks before the qemu 2.12 release 
cements in place an implementation of the BLOCK_STATUS extension.  And 
we've already discussed that if we make a change, we have to consider 
using a different constant for NBD_OPT_SET_META_CONTEXT to play nicely 
with the existing Virtuozzo implementation that currently matches what 
is slated to land in qemu 2.12 if no further qemu patches are 
submitted.  Is it worth me proposing a doc change to demonstrate what 
the difference would look like, along with corresponding qemu changes 
to match, to decide if including it in 2.12 is worth it?




Personally, I'd prefer to avoid it.

--
Best regards,
Vladimir

[Qemu-block] [PATCH v3] vmdk: return ERROR when cluster sector is larger than vmdk limitation

[yuchenlin]

From: yuchenlin 

VMDK has a hard limitation of extent size, which is due to the size of grain
table entry is 32 bits. It means it can only point to a grain located at
offset = 2^32. To avoid writing the user data beyond limitation and record a 
useless offset
in grain table. We should return ERROR here.

Signed-off-by: yuchenlin 
---
v2->v3:
 - use (1ULL << 32) to clearly show the limitation of offset is 2^32.

 thanks

 block/vmdk.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/block/vmdk.c b/block/vmdk.c
index f94c49a9c0..84f8bbe480 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -47,6 +47,8 @@
 #define VMDK4_FLAG_MARKER (1 << 17)
 #define VMDK4_GD_AT_END 0xULL
 
+#define VMDK_EXTENT_MAX_SECTORS (1ULL << 32)
+
 #define VMDK_GTE_ZEROED 0x1
 
 /* VMDK internal error codes */
@@ -1250,6 +1252,10 @@ static int get_cluster_offset(BlockDriverState *bs,
 return zeroed ? VMDK_ZEROED : VMDK_UNALLOC;
 }
 
+if (extent->next_cluster_sector >= VMDK_EXTENT_MAX_SECTORS) {
+return VMDK_ERROR;
+}
+
 cluster_sector = extent->next_cluster_sector;
 extent->next_cluster_sector += extent->cluster_sectors;
 
-- 
2.16.2

Re: [Qemu-block] raw iotest regressions in 2.12.0-rc0

[Peter Xu]

On Wed, Mar 21, 2018 at 05:58:48PM -0400, John Snow wrote:
> ./check -v -raw
> Failures: 109 132 136 148 152 183
> 
> 3fd2457d18edf5736f713dfe1ada9c87a9badab1 is the first bad commit
> commit 3fd2457d18edf5736f713dfe1ada9c87a9badab1
> Author: Peter Xu 
> Date:   Fri Mar 9 17:00:03 2018 +0800
> 
> monitor: enable IO thread for (qmp & !mux) typed
> 
> Start to use dedicate IO thread for QMP monitors that are not using
> MUXed chardev.
> 
> Reviewed-by: Fam Zheng 
> Reviewed-by: Stefan Hajnoczi 
> Signed-off-by: Peter Xu 
> Message-Id: <20180309090006.10018-21-pet...@redhat.com>
> Signed-off-by: Eric Blake 
> 
> 
> The symptom appears to be extra "RESUME" events in the stream that
> weren't expected by the original output for tests 109 and 183; the rest
> are python and I didn't dig yet.
> 
> ./check -v raw
> Failures: 055
> Failed 5 of 5 tests
> 
> 91ad45061af0fe44ac5dadb5bedaf4d7a08077c8 is the first bad commit
> commit 91ad45061af0fe44ac5dadb5bedaf4d7a08077c8
> Author: Peter Xu 
> Date:   Fri Mar 9 17:00:05 2018 +0800
> 
> tests: qmp-test: verify command batching
> 
> OOB introduced DROP event for flow control.  This should not affect old
> QMP clients.  Add a command batching check to make sure of it.
> 
> Reviewed-by: Stefan Hajnoczi 
> Signed-off-by: Peter Xu 
> Message-Id: <20180309090006.10018-23-pet...@redhat.com>
> Reviewed-by: Eric Blake 
> Signed-off-by: Eric Blake 
> 
> 
> 
> Maybe these are known, but I wanted to consolidate them for rc0 for
> something easy to search for. There are others for qcow2 which I'll post
> in a bit...!
> 
> 
> Thanks,
> --js

CCing Max, Fam.

Now I think I know how to solve some of the tests already (109, 132,
148, 152, 183). While I am still working (or, not yet started to work)
on some others (055, 136, 205).

205 is interesting - it won't fail every time, but randomly:

205 1s ... [failed, exit status 1] - output mismatch (see 205.out.bad)
--- /home/peterx/git/qemu/tests/qemu-iotests/205.out2018-03-08 
19:36:27.452220803 +0800
+++ /home/peterx/git/qemu/bin/tests/qemu-iotests/205.out.bad
2018-03-22 21:16:52.727152006 +0800
@@ -1,5 +1,19 @@
-...
+F..
+==
+FAIL: test_connect_after_remove_default (__main__.TestNbdServerRemove)
+--
+Traceback (most recent call last):
+  File "205", line 96, in test_connect_after_remove_default
+self.do_test_connect_after_remove()
+  File "205", line 90, in do_test_connect_after_remove
+self.assert_qmp(result, 'return', {})
+  File "/home/peterx/git/qemu/tests/qemu-iotests/iotests.py", line 
422, in assert_qmp
+result = self.dictpath(d, path)
+  File "/home/peterx/git/qemu/tests/qemu-iotests/iotests.py", line 
381, in dictpath
+self.fail('failed path traversal for "%s" in "%s"' % (path, 
str(d)))
+AssertionError: failed path traversal for "return" in "{u'error': 
{u'class': u'GenericError', u'desc': u"export 'exp' still in use"}}"
+
--
Ran 7 tests

Not digged yet.

For 136, it happens always, this is the error:

136 4s ... [failed, exit status 1] - output mismatch (see 136.out.bad)
--- /home/peterx/git/qemu/tests/qemu-iotests/136.out2018-01-12 
12:46:42.069915434 +0800
+++ /home/peterx/git/qemu/bin/tests/qemu-iotests/136.out.bad
2018-03-22 21:16:13.981116000 +0800
@@ -1,5 +1,125 @@
-...
+.EE.EE.EE.EE.EE
+==
+ERROR: test_read_only (__main__.BlockDeviceStatsTestAccountBoth)
+--
+Traceback (most recent call last):
+  File "136", line 286, in test_read_only
+self.do_test_stats(rd_size = i[0], rd_ops = i[1])
+  File "136", line 278, in do_test_stats
+self.check_values()
+  File "136", line 204, in check_values
+self.assertLess(0, stats['idle_time_ns'])
+KeyError: 'idle_time_ns'
+
+==
+ERROR: test_write_only (__main__.BlockDeviceStatsTestAccountBoth)
+--
+Traceback (most recent call last):
+  File "136", line 294, in test_write_only
+

Re: [Qemu-block] [Qemu-devel] [PATCH v2] vmdk: return ERROR when cluster sector is larger than vmdk limitation

[Eric Blake]

On 03/22/2018 02:41 AM, Fam Zheng wrote:

On Thu, 03/22 14:24, yuchen...@synology.com wrote:

From: yuchenlin 

VMDK has a hard limitation of extent size, which is due to the size of grain
table entry is 32 bits. It means it can only point to a grain located at
offset = 2^32. To avoid writing the user data beyond limitation and record a 
useless offset
in grain table. We should return ERROR here.

Signed-off-by: yuchenlin 
---



+++ b/block/vmdk.c
@@ -47,6 +47,8 @@
  #define VMDK4_FLAG_MARKER (1 << 17)
  #define VMDK4_GD_AT_END 0xULL
  
+#define VMDK_EXTENT_MAX_SECTORS (4294967296)

+


Hmm, with sectors instead of bytes, you don't want to use Phillipe's '2 
* T_BYTE' macro after all; but it would still look better spelled as 
'(1ULL << 32)' or '(4ULL * 1024 * 1024 * 1024)'.  Most importantly, 
since the value does NOT fit in a 32-bit unsigned integer, I think you 
NEED to use the ULL suffix to make your intent clear.



  #define VMDK_GTE_ZEROED 0x1
  
  /* VMDK internal error codes */

@@ -1250,6 +1252,10 @@ static int get_cluster_offset(BlockDriverState *bs,
  return zeroed ? VMDK_ZEROED : VMDK_UNALLOC;
  }
  
+if (extent->next_cluster_sector >= VMDK_EXTENT_MAX_SECTORS) {

+return VMDK_ERROR;
+}
+
  cluster_sector = extent->next_cluster_sector;
  extent->next_cluster_sector += extent->cluster_sectors;
  
--

2.16.2



Cc'ing Kevin and Max who merge block/ patches.


Also, CC'ing qemu-block, as all block-related patches should go through 
there.




Reviewed-by: Fam Zheng 





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

[Qemu-block] [PATCH for-2.12 v4] iotests: Test abnormally large size in compressed cluster descriptor

[Alberto Garcia]

L2 entries for compressed clusters have a field that indicates the
number of sectors used to store the data in the image.

That's however not the size of the compressed data itself, just the
number of sectors where that data is located. The actual data size is
usually not a multiple of the sector size, and therefore cannot be
represented with this field.

The way it works is that QEMU reads all the specified sectors and
starts decompressing the data until there's enough to recover the
original uncompressed cluster. If there are any bytes left that
haven't been decompressed they are simply ignored.

One consequence of this is that even if the size field is larger than
it needs to be QEMU can handle it just fine: it will read more data
from disk but it will ignore the extra bytes.

This test creates an image with two compressed clusters that use 5
sectors (2.5 KB) each, increases the size field to the maximum (8192
sectors, or 4 MB) and verifies that the data can be read without
problems.

This test is important because while the decompressed data takes
exactly one cluster, the maximum value allowed in the compressed size
field is twice the cluster size. So although QEMU won't produce images
with such large values we need to make sure that it can handle them.

Another effect of increasing the size field is that it can make
it include data from the following host cluster(s). In this case
'qemu-img check' will detect that the refcounts are not correct, and
we'll need to rebuild them.

Additionally, this patch also tests that decreasing the size corrupts
the image since the original data can no longer be recovered. In this
case QEMU returns an error when trying to read the compressed data,
but 'qemu-img check' doesn't see anything wrong if the refcounts are
consistent.

One possible task for the future is to make 'qemu-img check' verify
the sizes of the compressed clusters, by trying to decompress the data
and checking that the size stored in the L2 entry is correct.

Signed-off-by: Alberto Garcia 
Reviewed-by: Eric Blake 
---
v4: Resend for 2.12

v3: Add TODO comment, as suggested by Eric.

Corrupt the length of the second compressed cluster as well so the
uncompressed data would span three host clusters.

v2: We now have two scenarios where we make QEMU read data from the
next host cluster and from beyond the end of the image. This
version also runs qemu-img check on the corrupted image.

If the size field is too small, reading fails but qemu-img check
succeeds.

If the size field is too large, reading succeeds but qemu-img
check fails (this can be repaired, though).
---
 tests/qemu-iotests/122 | 45 +
 tests/qemu-iotests/122.out | 31 +++
 2 files changed, 76 insertions(+)

diff --git a/tests/qemu-iotests/122 b/tests/qemu-iotests/122
index 45b359c2ba..5b9593016c 100755
--- a/tests/qemu-iotests/122
+++ b/tests/qemu-iotests/122
@@ -130,6 +130,51 @@ $QEMU_IO -c "read -P 01024k 1022k" "$TEST_IMG" 2>&1 | 
_filter_qemu_io | _fil
 
 
 echo
+echo "=== Corrupted size field in compressed cluster descriptor ==="
+echo
+# Create an empty image, fill half of it with data and compress it.
+# The L2 entries of the two compressed clusters are located at
+# 0x80 and 0x88, their original values are 0x400800a0
+# and 0x400800a00802 (5 sectors for compressed data each).
+TEST_IMG="$TEST_IMG".1 _make_test_img 8M
+$QEMU_IO -c "write -P 0x11 0 4M" "$TEST_IMG".1 2>&1 | _filter_qemu_io | 
_filter_testdir
+$QEMU_IMG convert -c -O qcow2 -o cluster_size=2M "$TEST_IMG".1 "$TEST_IMG"
+
+# Reduce size of compressed data to 4 sectors: this corrupts the image.
+poke_file "$TEST_IMG" $((0x80)) "\x40\x06"
+$QEMU_IO -c "read  -P 0x11 0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | 
_filter_testdir
+
+# 'qemu-img check' however doesn't see anything wrong because it
+# doesn't try to decompress the data and the refcounts are consistent.
+# TODO: update qemu-img so this can be detected
+_check_test_img
+
+# Increase size of compressed data to the maximum (8192 sectors).
+# This makes QEMU read more data (8192 sectors instead of 5, host
+# addresses [0xa0, 0xdf]), but the decompression algorithm
+# stops once we have enough to restore the uncompressed cluster, so
+# the rest of the data is ignored.
+poke_file "$TEST_IMG" $((0x80)) "\x7f\xfe"
+# Do it also for the second compressed cluster (L2 entry at 0x88).
+# In this case the compressed data would span 3 host clusters
+# (host addresses: [0xa00802, 0xe00801])
+poke_file "$TEST_IMG" $((0x88)) "\x7f\xfe"
+
+# Here the image is too small so we're asking QEMU to read beyond the
+# end of the image.
+$QEMU_IO -c "read  -P 0x11  0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | 
_filter_testdir
+# But if we grow the image we won't be reading beyond its end anymore.
+$QEMU_IO -c "write -P 0x22 4M 4M" "$TEST_IMG" 2>&1 |