date:20230921

[PATCH v2] hw/sd/sdhci: Block Size Register bits [14:12] is lost

2023-09-21 Thread Lu Gao

Block Size Register bits [14:12] is SDMA Buffer Boundary, it is missed
in register write, but it is needed in SDMA transfer. e.g. it will be
used in sdhci_sdma_transfer_multi_blocks to calculate boundary_ variables.

Missing this field will cause wrong operation for different SDMA Buffer
Boundary settings.

Fixes: d7dfca0807 ("hw/sdhci: introduce standard SD host controller")
Fixes: dfba99f17f ("hw/sdhci: Fix DMA Transfer Block Size field")

Signed-off-by: Lu Gao 
Signed-off-by: Jianxian Wen 
Reviewed-by: Philippe Mathieu-Daudé? 
---
v2:
 - Add fixes information and reviewed-by information

 hw/sd/sdhci.c | 15 +++
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 5564765a9b..40473b0db0 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -321,6 +321,8 @@ static void sdhci_poweron_reset(DeviceState *dev)
 
 static void sdhci_data_transfer(void *opaque);
 
+#define BLOCK_SIZE_MASK (4 * KiB - 1)
+
 static void sdhci_send_command(SDHCIState *s)
 {
 SDRequest request;
@@ -371,7 +373,8 @@ static void sdhci_send_command(SDHCIState *s)
 
 sdhci_update_irq(s);
 
-if (!timeout && s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
+if (!timeout && (s->blksize & BLOCK_SIZE_MASK) &&
+(s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
 s->data_count = 0;
 sdhci_data_transfer(s);
 }
@@ -406,7 +409,6 @@ static void sdhci_end_transfer(SDHCIState *s)
 /*
  * Programmed i/o data transfer
  */
-#define BLOCK_SIZE_MASK (4 * KiB - 1)
 
 /* Fill host controller's read buffer with BLKSIZE bytes of data from card */
 static void sdhci_read_block_from_card(SDHCIState *s)
@@ -1154,7 +1156,8 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, 
unsigned size)
 s->sdmasysad = (s->sdmasysad & mask) | value;
 MASKED_WRITE(s->sdmasysad, mask, value);
 /* Writing to last byte of sdmasysad might trigger transfer */
-if (!(mask & 0xFF00) && s->blkcnt && s->blksize &&
+if (!(mask & 0xFF00) && s->blkcnt &&
+(s->blksize & BLOCK_SIZE_MASK) &&
 SDHC_DMA_TYPE(s->hostctl1) == SDHC_CTRL_SDMA) {
 if (s->trnmod & SDHC_TRNS_MULTI) {
 sdhci_sdma_transfer_multi_blocks(s);
@@ -1168,7 +1171,11 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, 
unsigned size)
 if (!TRANSFERRING_DATA(s->prnsts)) {
 uint16_t blksize = s->blksize;
 
-MASKED_WRITE(s->blksize, mask, extract32(value, 0, 12));
+/*
+ * [14:12] SDMA Buffer Boundary
+ * [11:00] Transfer Block Size
+ */
+MASKED_WRITE(s->blksize, mask, extract32(value, 0, 15));
 MASKED_WRITE(s->blkcnt, mask >> 16, value >> 16);
 
 /* Limit block size to the maximum buffer size */
-- 
2.17.1

Re: [PATCH 0/5] file-posix: Clean up and fix zoned checks

2023-09-21 Thread Michael Tokarev


21.09.2023 21:21, Michael Tokarev wrote:
..

Is this stable-worthy (at least 1-3)?  From the bug description it smells
like it should be in 8.1.x, or maybe whole series.


N/M, this whole patchset has been Cc'd qemu-stable already.

Thanks,

/mjt

Re: [PATCH 0/5] file-posix: Clean up and fix zoned checks

2023-09-21 Thread Michael Tokarev


24.08.2023 18:53, Hanna Czenczek wrote:

Hi,

As presented in [1] there is a bug in the zone code in raw_co_prw(),
specifically we don’t check whether there actually is zone information
before running code that assumes there is (and thus we run into a
division by zero).  This has now also been reported in [2].

I believe the solution [1] is incomplete, though, which is why I’m
sending this separate series: I don’t think checking bs->wps and/or
bs->bl.zone_size to determine whether there is zone information is
right; for example, we do not have raw_refresh_zoned_limits() clear
those values if on a refresh, zone information were to disappear.

It is also weird that we separate checking bs->wps and bs->bl.zone_size
at all; raw_refresh_zoned_limits() seems to intend to ensure that either
we have information with non-NULL bs->wps and non-zero bs->bl.zone_size,
or we don’t.

I think we should have a single flag that tells whether we have valid
information or not, and it looks to me like bs->bl.zoned != BLK_Z_NONE
is the condition that fits best.

Patch 1 ensures that raw_refresh_zoned_limits() will set bs->bl.zoned to
BLK_Z_NONE on error, so that we can actually be sure that this condition
tells us whether we have valid information or not.

Patch 2 unifies all conditional checks for zone information to use
bs->bl.zoned != BLK_Z_NONE.

Patch 3 is the I/O error path fix, which is not really different from
[1].

Patch 4 does a bit of clean-up.

Patch 5 adds a regression test.


[1] https://lists.nongnu.org/archive/html/qemu-devel/2023-06/msg01742.html
[2] https://bugzilla.redhat.com/show_bug.cgi?id=2234374


Is this stable-worthy (at least 1-3)?  From the bug description it smells
like it should be in 8.1.x, or maybe whole series.

Thanks,

/mjt

Re: [PATCH v3 0/2] qemu-img: map: implement support for compressed clusters

2023-09-21 Thread Andrey Drobyshev

On 9/14/23 16:34, Kevin Wolf wrote:
> Am 07.09.2023 um 23:02 hat Andrey Drobyshev via geschrieben:
>> v2 --> v3:
>>   * Make "compressed" field mandatory, not optional;
>>   * Adjust field description in qapi/block-core.json;
>>   * Squash patch 3 into patch 2 so that failing tests don't break bisect;
>>   * Update even more tests' outputs now that the field is mandatory.
>>
>> v2: https://lists.nongnu.org/archive/html/qemu-block/2023-07/msg00106.html
> 
> Thanks, applied to the block branch.
> 
> Kevin
> 

Hi Kevin,

In the patches applied to master something went wrong and my email isn't
displayed correctly:

> commit 2848289168fbbd9a6855c84ec8fde8929a2b042b
> Author: Andrey Drobyshev via<
> Date:   Fri Sep 8 00:02:25 2023 +0300
> 
> block: add BDRV_BLOCK_COMPRESSED flag for bdrv_block_status()

> commit 52b10c9c0c68e90f9503ba578f2eaf8975c1977f
> Author: Andrey Drobyshev via<
> Date:   Fri Sep 8 00:02:26 2023 +0300
> 
> qemu-img: map: report compressed data blocks

There's probably no way to fix that in master, but just giving you a
heads-up for maybe there's a bug in your scripts.

Andrey

[PATCH 1/1] hw/ide/core: terminate in-flight DMA on IDE bus reset

2023-09-21 Thread Simon Rowe

When an IDE controller is reset, its internal state is being cleared
before any outstanding I/O is cancelled. If a response to DMA is
received in this window, the aio callback will incorrectly continue
with the next part of the transfer (now using sector 0 from
the cleared controller state).

For a write operation, this results in user data being written to the
MBR, replacing the first stage bootloader and/or partition table. A
malicious user could exploit this bug to first read the MBR and then
rewrite it with user-controller bootloader code.

This addresses the bug by checking if DRQ_STAT is still set in the DMA
callback (as it is otherwise cleared at the start of the bus
reset). If it is not, treat the transfer as ended.

This only appears to affect SATA controllers, plain IDE does not use
aio.

Fixes: CVE-2023-5088
Signed-off-by: Simon Rowe 
Cc: Felipe Franciosi 
---
 hw/ide/core.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index b5e0dcd29b..826b7eaeeb 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -906,8 +906,12 @@ static void ide_dma_cb(void *opaque, int ret)
 s->nsector -= n;
 }
 
-/* end of transfer ? */
-if (s->nsector == 0) {
+/*
+ * End of transfer ?
+ * If a bus reset occurs immediately before the callback is invoked the
+ * bus state will have been cleared. Terminate the transfer.
+ */
+if (s->nsector == 0 || !(s->status & DRQ_STAT)) {
 s->status = READY_STAT | SEEK_STAT;
 ide_bus_set_irq(s->bus);
 goto eot;
-- 
2.22.3

[PATCH 0/1] CVE-2023-5088

2023-09-21 Thread Simon Rowe

The attached patch fixes CVE-2023-5088 in which a bug in QEMU could
cause a guest I/O operation otherwise addressed to an arbitrary disk
offset to be targeted to offset 0 instead (potentially overwriting the
VM's boot code). This could be used, for example, by L2 guests with a
virtual disk (vdiskL2) stored on a virtual disk of an L1 (vdiskL1)
hypervisor to read and/or write data to LBA 0 of vdiskL1, potentially
gaining control of L1 at its next reboot.

The symptoms behind this bug have been previously reported as MBR
corruptions and an independent fix has been posted by Fiona Ebner in:

https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg03883.html

Simon Rowe (1):
  hw/ide/core: terminate in-flight DMA on IDE bus reset

 hw/ide/core.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

-- 
2.22.3

Re: [PULL v2 00/28] Block layer patches

2023-09-21 Thread Stefan Hajnoczi

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/8.2 for any 
user-visible changes.


signature.asc
Description: PGP signature

Re: [PULL v2 00/22] implement discard operation for Parallels images

2023-09-21 Thread Stefan Hajnoczi

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/8.2 for any 
user-visible changes.


signature.asc
Description: PGP signature

Re: [PATCH v2 07/10] virtiofsd: Use qemu_get_runtime_dir()

2023-09-21 Thread Akihiko Odaki


On 2023/09/21 21:58, Stefan Hajnoczi wrote:

On Thu, Nov 10, 2022 at 07:06:26PM +0900, Akihiko Odaki wrote:

qemu_get_runtime_dir() is used to construct the path to a lock file.

Signed-off-by: Akihiko Odaki 
---
  tools/virtiofsd/fuse_virtio.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 9368e292e4..b9eeed85e6 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -901,12 +901,12 @@ static bool fv_socket_lock(struct fuse_session *se)
  {
  g_autofree gchar *sk_name = NULL;
  g_autofree gchar *pidfile = NULL;
-g_autofree gchar *state = NULL;
+g_autofree gchar *run = NULL;
  g_autofree gchar *dir = NULL;
  Error *local_err = NULL;
  
-state = qemu_get_local_state_dir();

-dir = g_build_filename(state, "run", "virtiofsd", NULL);
+run = qemu_get_runtime_dir();
+dir = g_build_filename(run, "virtiofsd", NULL);
  
  if (g_mkdir_with_parents(dir, S_IRWXU) < 0) {

  fuse_log(FUSE_LOG_ERR, "%s: Failed to create directory %s: %s\n",


tools/virtiofsd/ no longer exists. Which version of QEMU did you develop 
against?

commit e0dc2631ec4ac718ebe22ddea0ab25524eb37b0e
Author: Dr. David Alan Gilbert 
Date:   Wed Jan 18 12:11:51 2023 +

 virtiofsd: Remove source

Stefan


It is an old version of the series. You can find the latest version at:
https://patchew.org/QEMU/20230921075425.16738-1-akihiko.od...@daynix.com/

Regards,
Akihiko Odaki

Re: [PATCH v2 7/7] qobject atomics osdep: Make a few macros more hygienic

2023-09-21 Thread Markus Armbruster

Kevin Wolf  writes:

> Am 20.09.2023 um 20:31 hat Markus Armbruster geschrieben:

[...]

>> diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
>> index 9003b71fd3..d36cc97805 100644
>> --- a/include/qapi/qmp/qobject.h
>> +++ b/include/qapi/qmp/qobject.h
>> @@ -45,10 +45,17 @@ struct QObject {
>>  struct QObjectBase_ base;
>>  };
>>  
>> -#define QOBJECT(obj) ({ \
>> +/*
>> + * Preprocessory sorcery ahead: use a different identifier for the
>> + * local variable in each expansion, so we can nest macro calls
>> + * without shadowing variables.
>> + */
>> +#define QOBJECT_INTERNAL(obj, _obj) ({  \
>>  typeof(obj) _obj = (obj);   \
>> -_obj ? container_of(&(_obj)->base, QObject, base) : NULL;   \
>> +_obj\
>> +? container_of(&(_obj)->base, QObject, base) : NULL;\
>
> What happened here? The code in this line (or now two lines) seems to be
> unchanged apart from a strange looking newline.

Accident, will fix, thanks!

>>  })
>> +#define QOBJECT(obj) QOBJECT_INTERNAL((obj), MAKE_IDENTFIER(_obj))
>
> Kevin

[PATCH v3 2/7] migration: Clean up local variable shadowing

2023-09-21 Thread Markus Armbruster

Local variables shadowing other local variables or parameters make the
code needlessly hard to understand.  Tracked down with -Wshadow=local.
Clean up: delete inner declarations when they are actually redundant,
else rename variables.

Signed-off-by: Markus Armbruster 
Reviewed-by: Peter Xu 
Reviewed-by: Li Zhijian 
---
 migration/block.c   | 4 ++--
 migration/ram.c | 8 +++-
 migration/rdma.c| 8 +---
 migration/vmstate.c | 2 +-
 4 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/migration/block.c b/migration/block.c
index 86c2256a2b..eb6aafeb9e 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -440,8 +440,8 @@ static int init_blk_migration(QEMUFile *f)
 /* Can only insert new BDSes now because doing so while iterating block
  * devices may end up in a deadlock (iterating the new BDSes, too). */
 for (i = 0; i < num_bs; i++) {
-BlkMigDevState *bmds = bmds_bs[i].bmds;
-BlockDriverState *bs = bmds_bs[i].bs;
+bmds = bmds_bs[i].bmds;
+bs = bmds_bs[i].bs;
 
 if (bmds) {
 ret = blk_insert_bs(bmds->blk, bs, &local_err);
diff --git a/migration/ram.c b/migration/ram.c
index 9040d66e61..0c202f8109 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3517,8 +3517,6 @@ int colo_init_ram_cache(void)
 * we use the same name 'ram_bitmap' as for migration.
 */
 if (ram_bytes_total()) {
-RAMBlock *block;
-
 RAMBLOCK_FOREACH_NOT_IGNORED(block) {
 unsigned long pages = block->max_length >> TARGET_PAGE_BITS;
 block->bmap = bitmap_new(pages);
@@ -3998,12 +3996,12 @@ static int ram_load_precopy(QEMUFile *f)
 }
 }
 if (migrate_ignore_shared()) {
-hwaddr addr = qemu_get_be64(f);
+hwaddr addr2 = qemu_get_be64(f);
 if (migrate_ram_is_ignored(block) &&
-block->mr->addr != addr) {
+block->mr->addr != addr2) {
 error_report("Mismatched GPAs for block %s "
  "%" PRId64 "!= %" PRId64,
- id, (uint64_t)addr,
+ id, (uint64_t)addr2,
  (uint64_t)block->mr->addr);
 ret = -EINVAL;
 }
diff --git a/migration/rdma.c b/migration/rdma.c
index 3915d1d7c9..c78ddfcb74 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -1902,9 +1902,11 @@ static int qemu_rdma_exchange_send(RDMAContext *rdma, 
RDMAControlHeader *head,
  * by waiting for a READY message.
  */
 if (rdma->control_ready_expected) {
-RDMAControlHeader resp;
-ret = qemu_rdma_exchange_get_response(rdma,
-&resp, RDMA_CONTROL_READY, 
RDMA_WRID_READY);
+RDMAControlHeader resp_ignored;
+
+ret = qemu_rdma_exchange_get_response(rdma, &resp_ignored,
+  RDMA_CONTROL_READY,
+  RDMA_WRID_READY);
 if (ret < 0) {
 return ret;
 }
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 31842c3afb..438ea77cfa 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -97,7 +97,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription 
*vmsd,
 return -EINVAL;
 }
 if (vmsd->pre_load) {
-int ret = vmsd->pre_load(opaque);
+ret = vmsd->pre_load(opaque);
 if (ret) {
 return ret;
 }
-- 
2.41.0

[PATCH v3 6/7] block: Clean up local variable shadowing

2023-09-21 Thread Markus Armbruster

Local variables shadowing other local variables or parameters make the
code needlessly hard to understand.  Tracked down with -Wshadow=local.
Clean up: delete inner declarations when they are actually redundant,
else rename variables.

Signed-off-by: Markus Armbruster 
Reviewed-by: Stefan Hajnoczi 
Acked-by: Anthony PERARD 
Acked-by: Ilya Dryomov 
Reviewed-by: Kevin Wolf 
---
 block.c  |  9 +
 block/rbd.c  |  2 +-
 block/stream.c   |  1 -
 block/vvfat.c| 35 ++-
 hw/block/xen-block.c |  6 +++---
 5 files changed, 27 insertions(+), 26 deletions(-)

diff --git a/block.c b/block.c
index 8da89aaa62..bb5dd17e9c 100644
--- a/block.c
+++ b/block.c
@@ -3035,18 +3035,19 @@ static BdrvChild 
*bdrv_attach_child_common(BlockDriverState *child_bs,
   &local_err);
 
 if (ret < 0 && child_class->change_aio_ctx) {
-Transaction *tran = tran_new();
+Transaction *aio_ctx_tran = tran_new();
 GHashTable *visited = g_hash_table_new(NULL, NULL);
 bool ret_child;
 
 g_hash_table_add(visited, new_child);
 ret_child = child_class->change_aio_ctx(new_child, child_ctx,
-visited, tran, NULL);
+visited, aio_ctx_tran,
+NULL);
 if (ret_child == true) {
 error_free(local_err);
 ret = 0;
 }
-tran_finalize(tran, ret_child == true ? 0 : -1);
+tran_finalize(aio_ctx_tran, ret_child == true ? 0 : -1);
 g_hash_table_destroy(visited);
 }
 
@@ -6077,12 +6078,12 @@ void bdrv_iterate_format(void (*it)(void *opaque, const 
char *name),
 QLIST_FOREACH(drv, &bdrv_drivers, list) {
 if (drv->format_name) {
 bool found = false;
-int i = count;
 
 if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, read_only)) {
 continue;
 }
 
+i = count;
 while (formats && i && !found) {
 found = !strcmp(formats[--i], drv->format_name);
 }
diff --git a/block/rbd.c b/block/rbd.c
index 978671411e..472ca05cba 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -1290,7 +1290,7 @@ static int coroutine_fn 
qemu_rbd_start_co(BlockDriverState *bs,
  * operations that exceed the current size.
  */
 if (offset + bytes > s->image_size) {
-int r = qemu_rbd_resize(bs, offset + bytes);
+r = qemu_rbd_resize(bs, offset + bytes);
 if (r < 0) {
 return r;
 }
diff --git a/block/stream.c b/block/stream.c
index e522bbdec5..007253880b 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -282,7 +282,6 @@ void stream_start(const char *job_id, BlockDriverState *bs,
 /* Make sure that the image is opened in read-write mode */
 bs_read_only = bdrv_is_read_only(bs);
 if (bs_read_only) {
-int ret;
 /* Hold the chain during reopen */
 if (bdrv_freeze_backing_chain(bs, above_base, errp) < 0) {
 return;
diff --git a/block/vvfat.c b/block/vvfat.c
index 0ddc91fc09..856b479c91 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -777,7 +777,6 @@ static int read_directory(BDRVVVFATState* s, int 
mapping_index)
 while((entry=readdir(dir))) {
 unsigned int length=strlen(dirname)+2+strlen(entry->d_name);
 char* buffer;
-direntry_t* direntry;
 struct stat st;
 int is_dot=!strcmp(entry->d_name,".");
 int is_dotdot=!strcmp(entry->d_name,"..");
@@ -857,7 +856,7 @@ static int read_directory(BDRVVVFATState* s, int 
mapping_index)
 
 /* fill with zeroes up to the end of the cluster */
 while(s->directory.next%(0x10*s->sectors_per_cluster)) {
-direntry_t* direntry=array_get_next(&(s->directory));
+direntry = array_get_next(&(s->directory));
 memset(direntry,0,sizeof(direntry_t));
 }
 
@@ -1962,24 +1961,24 @@ get_cluster_count_for_direntry(BDRVVVFATState* s, 
direntry_t* direntry, const ch
  * This is horribly inefficient, but that is okay, since
  * it is rarely executed, if at all.
  */
-int64_t offset = cluster2sector(s, cluster_num);
+int64_t offs = cluster2sector(s, cluster_num);
 
 vvfat_close_current_file(s);
 for (i = 0; i < s->sectors_per_cluster; i++) {
 int res;
 
 res = bdrv_is_allocated(s->qcow->bs,
-(offset + i) * BDRV_SECTOR_SIZE,
+(offs + i) * BDRV_SECTOR_SIZE,
 BDRV_SECTOR_SIZE, NULL);
 if (res < 0) {

[PATCH v3 0/7] Steps towards enabling -Wshadow=local

2023-09-21 Thread Markus Armbruster

Local variables shadowing other local variables or parameters make the
code needlessly hard to understand.  Bugs love to hide in such code.
Evidence: PATCH 1.

Enabling -Wshadow would prevent bugs like this one.  But we'd have to
clean up all the offenders first.  We got a lot of them.

Enabling -Wshadow=local should be less work for almost as much gain.
I took a stab at it.  There's a small, exciting part, and a large,
boring part.

The exciting part is dark preprocessor sorcery to let us nest macro
calls without shadowing: PATCH 7.

The boring part is cleaning up all the other warnings.  I did some
[PATCH 2-6], but ran out of steam long before finishing the job.  Some
160 unique warnings remain.

To see them, enable -Wshadow=local like so:

diff --git a/meson.build b/meson.build
index 98e68ef0b1..9fc4c7ac9d 100644
--- a/meson.build
+++ b/meson.build
@@ -466,6 +466,9 @@ warn_flags = [
   '-Wno-tautological-type-limit-compare',
   '-Wno-psabi',
   '-Wno-gnu-variable-sized-type-not-at-end',
+  '-Wshadow=local',
+  '-Wno-error=shadow=local',
+  '-Wno-error=shadow=compatible-local',
 ]
 
 if targetos != 'darwin'

You may want to drop the -Wno-error lines.

v3:
* PATCH 7: Comment typo [Eric], peel off a pair of parenthesis [Eric],
  revert accidental line breaks [Kevin]

v2:
* PATCH 3+6: Mollify checkpatch
* PATCH 4: Redo for clearer code, R-bys dropped [Kevin]
* PATCH 5: Rename tweaked [Kevin]
* PATCH 6: Rename local @tran instead of the parameter [Kevin]
* PATCH 7: Drop PASTE(), use glue() instead [Richard]; pass
  identifiers instead of __COUNTER__ for readability [Eric]; add
  comments

Markus Armbruster (7):
  migration/rdma: Fix save_page method to fail on polling error
  migration: Clean up local variable shadowing
  ui: Clean up local variable shadowing
  block/dirty-bitmap: Clean up local variable shadowing
  block/vdi: Clean up local variable shadowing
  block: Clean up local variable shadowing
  qobject atomics osdep: Make a few macros more hygienic

 include/qapi/qmp/qobject.h  | 10 --
 include/qemu/atomic.h   | 17 +++-
 include/qemu/compiler.h |  3 +++
 include/qemu/osdep.h| 27 ++---
 block.c |  9 +
 block/monitor/bitmap-qmp-cmds.c | 19 +-
 block/qcow2-bitmap.c|  3 +--
 block/rbd.c |  2 +-
 block/stream.c  |  1 -
 block/vdi.c |  7 +++
 block/vvfat.c   | 35 +
 hw/block/xen-block.c|  6 +++---
 migration/block.c   |  4 ++--
 migration/ram.c |  8 +++-
 migration/rdma.c| 14 -
 migration/vmstate.c |  2 +-
 ui/gtk.c| 14 ++---
 ui/spice-display.c  |  9 +
 ui/vnc-palette.c|  2 --
 ui/vnc.c| 12 +--
 ui/vnc-enc-zrle.c.inc   |  9 -
 21 files changed, 121 insertions(+), 92 deletions(-)

-- 
2.41.0

[PATCH v3 3/7] ui: Clean up local variable shadowing

2023-09-21 Thread Markus Armbruster

Local variables shadowing other local variables or parameters make the
code needlessly hard to understand.  Tracked down with -Wshadow=local.
Clean up: delete inner declarations when they are actually redundant,
else rename variables.

Signed-off-by: Markus Armbruster 
Reviewed-by: Peter Maydell 
---
 ui/gtk.c  | 14 +++---
 ui/spice-display.c|  9 +
 ui/vnc-palette.c  |  2 --
 ui/vnc.c  | 12 ++--
 ui/vnc-enc-zrle.c.inc |  9 -
 5 files changed, 22 insertions(+), 24 deletions(-)

diff --git a/ui/gtk.c b/ui/gtk.c
index e09f97a86b..3373427c9b 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -930,8 +930,8 @@ static gboolean gd_motion_event(GtkWidget *widget, 
GdkEventMotion *motion,
 GdkMonitor *monitor = gdk_display_get_monitor_at_window(dpy, win);
 GdkRectangle geometry;
 
-int x = (int)motion->x_root;
-int y = (int)motion->y_root;
+int xr = (int)motion->x_root;
+int yr = (int)motion->y_root;
 
 gdk_monitor_get_geometry(monitor, &geometry);
 
@@ -942,13 +942,13 @@ static gboolean gd_motion_event(GtkWidget *widget, 
GdkEventMotion *motion,
  * may still be only half way across the screen. Without
  * this warp, the server pointer would thus appear to hit
  * an invisible wall */
-if (x <= geometry.x || x - geometry.x >= geometry.width - 1 ||
-y <= geometry.y || y - geometry.y >= geometry.height - 1) {
+if (xr <= geometry.x || xr - geometry.x >= geometry.width - 1 ||
+yr <= geometry.y || yr - geometry.y >= geometry.height - 1) {
 GdkDevice *dev = gdk_event_get_device((GdkEvent *)motion);
-x = geometry.x + geometry.width / 2;
-y = geometry.y + geometry.height / 2;
+xr = geometry.x + geometry.width / 2;
+yr = geometry.y + geometry.height / 2;
 
-gdk_device_warp(dev, screen, x, y);
+gdk_device_warp(dev, screen, xr, yr);
 s->last_set = FALSE;
 return FALSE;
 }
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 5cc47bd668..6eb98a5a5c 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -1081,15 +1081,16 @@ static void qemu_spice_gl_update(DisplayChangeListener 
*dcl,
 }
 
 if (render_cursor) {
-int x, y;
+int ptr_x, ptr_y;
+
 qemu_mutex_lock(&ssd->lock);
-x = ssd->ptr_x;
-y = ssd->ptr_y;
+ptr_x = ssd->ptr_x;
+ptr_y = ssd->ptr_y;
 qemu_mutex_unlock(&ssd->lock);
 egl_texture_blit(ssd->gls, &ssd->blit_fb, &ssd->guest_fb,
  !y_0_top);
 egl_texture_blend(ssd->gls, &ssd->blit_fb, &ssd->cursor_fb,
-  !y_0_top, x, y, 1.0, 1.0);
+  !y_0_top, ptr_x, ptr_y, 1.0, 1.0);
 glFlush();
 }
 
diff --git a/ui/vnc-palette.c b/ui/vnc-palette.c
index dc7c0ba997..4e88c412f0 100644
--- a/ui/vnc-palette.c
+++ b/ui/vnc-palette.c
@@ -86,8 +86,6 @@ int palette_put(VncPalette *palette, uint32_t color)
 return 0;
 }
 if (!entry) {
-VncPaletteEntry *entry;
-
 entry = &palette->pool[palette->size];
 entry->color = color;
 entry->idx = idx;
diff --git a/ui/vnc.c b/ui/vnc.c
index 6fd86996a5..ecb75ff8c8 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1584,15 +1584,15 @@ static void vnc_jobs_bh(void *opaque)
  */
 static int vnc_client_read(VncState *vs)
 {
-size_t ret;
+size_t sz;
 
 #ifdef CONFIG_VNC_SASL
 if (vs->sasl.conn && vs->sasl.runSSF)
-ret = vnc_client_read_sasl(vs);
+sz = vnc_client_read_sasl(vs);
 else
 #endif /* CONFIG_VNC_SASL */
-ret = vnc_client_read_plain(vs);
-if (!ret) {
+sz = vnc_client_read_plain(vs);
+if (!sz) {
 if (vs->disconnecting) {
 vnc_disconnect_finish(vs);
 return -1;
@@ -3118,8 +3118,8 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
 cmp_bytes = MIN(VNC_DIRTY_PIXELS_PER_BIT * VNC_SERVER_FB_BYTES,
 server_stride);
 if (vd->guest.format != VNC_SERVER_FB_FORMAT) {
-int width = pixman_image_get_width(vd->server);
-tmpbuf = qemu_pixman_linebuf_create(VNC_SERVER_FB_FORMAT, width);
+int w = pixman_image_get_width(vd->server);
+tmpbuf = qemu_pixman_linebuf_create(VNC_SERVER_FB_FORMAT, w);
 } else {
 int guest_bpp =
 PIXMAN_FORMAT_BPP(pixman_image_get_format(vd->guest.fb));
diff --git a/ui/vnc-enc-zrle.c.inc b/ui/vnc-enc-zrle.c.inc
index a8ca37d05e..2ef7501d52 100644
--- a/ui/vnc-enc-zrle.c.inc
+++ b/ui/vnc-enc-zrle.c.inc
@@ -153,11 +153,12 @@ static void ZRLE_ENCODE_TILE(VncState *vs, ZRLE_PIXEL 
*data, int w, int h,
 }
 
 if (use_rle) {
-ZRLE_PIXEL *ptr = data;
-ZRLE_PIXEL *end = ptr + w * h;
 ZRLE_PIXEL *run_start;
 ZRLE_PIXEL pix;
 
+ptr = data;
+end = ptr + w * h;
+
 while (p

[PATCH v3 5/7] block/vdi: Clean up local variable shadowing

2023-09-21 Thread Markus Armbruster

Local variables shadowing other local variables or parameters make the
code needlessly hard to understand.  Tracked down with -Wshadow=local.
Clean up: delete inner declarations when they are actually redundant,
else rename variables.

Signed-off-by: Markus Armbruster 
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Kevin Wolf 
---
 block/vdi.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 6c35309e04..934e1b849b 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -634,7 +634,6 @@ vdi_co_pwritev(BlockDriverState *bs, int64_t offset, 
int64_t bytes,
 bmap_entry = le32_to_cpu(s->bmap[block_index]);
 if (!VDI_IS_ALLOCATED(bmap_entry)) {
 /* Allocate new block and write to it. */
-uint64_t data_offset;
 qemu_co_rwlock_upgrade(&s->bmap_lock);
 bmap_entry = le32_to_cpu(s->bmap[block_index]);
 if (VDI_IS_ALLOCATED(bmap_entry)) {
@@ -700,7 +699,7 @@ nonallocating_write:
 /* One or more new blocks were allocated. */
 VdiHeader *header;
 uint8_t *base;
-uint64_t offset;
+uint64_t bmap_offset;
 uint32_t n_sectors;
 
 g_free(block);
@@ -723,11 +722,11 @@ nonallocating_write:
 bmap_first /= (SECTOR_SIZE / sizeof(uint32_t));
 bmap_last /= (SECTOR_SIZE / sizeof(uint32_t));
 n_sectors = bmap_last - bmap_first + 1;
-offset = s->bmap_sector + bmap_first;
+bmap_offset = s->bmap_sector + bmap_first;
 base = ((uint8_t *)&s->bmap[0]) + bmap_first * SECTOR_SIZE;
 logout("will write %u block map sectors starting from entry %u\n",
n_sectors, bmap_first);
-ret = bdrv_co_pwrite(bs->file, offset * SECTOR_SIZE,
+ret = bdrv_co_pwrite(bs->file, bmap_offset * SECTOR_SIZE,
  n_sectors * SECTOR_SIZE, base, 0);
 }
 
-- 
2.41.0

Re: [PATCH v2 07/10] virtiofsd: Use qemu_get_runtime_dir()

2023-09-21 Thread Stefan Hajnoczi

On Thu, Nov 10, 2022 at 07:06:26PM +0900, Akihiko Odaki wrote:
> qemu_get_runtime_dir() is used to construct the path to a lock file.
> 
> Signed-off-by: Akihiko Odaki 
> ---
>  tools/virtiofsd/fuse_virtio.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> index 9368e292e4..b9eeed85e6 100644
> --- a/tools/virtiofsd/fuse_virtio.c
> +++ b/tools/virtiofsd/fuse_virtio.c
> @@ -901,12 +901,12 @@ static bool fv_socket_lock(struct fuse_session *se)
>  {
>  g_autofree gchar *sk_name = NULL;
>  g_autofree gchar *pidfile = NULL;
> -g_autofree gchar *state = NULL;
> +g_autofree gchar *run = NULL;
>  g_autofree gchar *dir = NULL;
>  Error *local_err = NULL;
>  
> -state = qemu_get_local_state_dir();
> -dir = g_build_filename(state, "run", "virtiofsd", NULL);
> +run = qemu_get_runtime_dir();
> +dir = g_build_filename(run, "virtiofsd", NULL);
>  
>  if (g_mkdir_with_parents(dir, S_IRWXU) < 0) {
>  fuse_log(FUSE_LOG_ERR, "%s: Failed to create directory %s: %s\n",

tools/virtiofsd/ no longer exists. Which version of QEMU did you develop 
against?

commit e0dc2631ec4ac718ebe22ddea0ab25524eb37b0e
Author: Dr. David Alan Gilbert 
Date:   Wed Jan 18 12:11:51 2023 +

virtiofsd: Remove source

Stefan


signature.asc
Description: PGP signature

[PATCH v3 7/7] qobject atomics osdep: Make a few macros more hygienic

2023-09-21 Thread Markus Armbruster

Variables declared in macros can shadow other variables.  Much of the
time, this is harmless, e.g.:

#define _FDT(exp)  \
do {   \
int ret = (exp);   \
if (ret < 0) { \
error_report("error creating device tree: %s: %s",   \
#exp, fdt_strerror(ret));  \
exit(1);   \
}  \
} while (0)

Harmless shadowing in h_client_architecture_support():

target_ulong ret;

[...]

ret = do_client_architecture_support(cpu, spapr, vec, fdt_bufsize);
if (ret == H_SUCCESS) {
_FDT((fdt_pack(spapr->fdt_blob)));
[...]
}

return ret;

However, we can get in trouble when the shadowed variable is used in a
macro argument:

#define QOBJECT(obj) ({ \
typeof(obj) o = (obj);  \
o ? container_of(&(o)->base, QObject, base) : NULL; \
 })

QOBJECT(o) expands into

({
--->typeof(o) o = (o);
o ? container_of(&(o)->base, QObject, base) : NULL;
})

Unintended variable name capture at --->.  We'd be saved by
-Winit-self.  But I could certainly construct more elaborate death
traps that don't trigger it.

To reduce the risk of trapping ourselves, we use variable names in
macros that no sane person would use elsewhere.  Here's our actual
definition of QOBJECT():

#define QOBJECT(obj) ({ \
typeof(obj) _obj = (obj);   \
_obj ? container_of(&(_obj)->base, QObject, base) : NULL;   \
})

Works well enough until we nest macro calls.  For instance, with

#define qobject_ref(obj) ({ \
typeof(obj) _obj = (obj);   \
qobject_ref_impl(QOBJECT(_obj));\
_obj;   \
})

the expression qobject_ref(obj) expands into

({
typeof(obj) _obj = (obj);
qobject_ref_impl(
({
--->typeof(_obj) _obj = (_obj);
_obj ? container_of(&(_obj)->base, QObject, base) : NULL;
}));
_obj;
})

Unintended variable name capture at --->.

The only reliable way to prevent unintended variable name capture is
-Wshadow.

One blocker for enabling it is shadowing hiding in function-like
macros like

 qdict_put(dict, "name", qobject_ref(...))

qdict_put() wraps its last argument in QOBJECT(), and the last
argument here contains another QOBJECT().

Use dark preprocessor sorcery to make the macros that give us this
problem use different variable names on every call.

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
 include/qapi/qmp/qobject.h | 10 --
 include/qemu/atomic.h  | 17 -
 include/qemu/compiler.h|  3 +++
 include/qemu/osdep.h   | 27 ---
 4 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index 9003b71fd3..89b97d88bc 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -45,10 +45,16 @@ struct QObject {
 struct QObjectBase_ base;
 };
 
-#define QOBJECT(obj) ({ \
+/*
+ * Preprocessor sorcery ahead: use a different identifier for the
+ * local variable in each expansion, so we can nest macro calls
+ * without shadowing variables.
+ */
+#define QOBJECT_INTERNAL(obj, _obj) ({  \
 typeof(obj) _obj = (obj);   \
-_obj ? container_of(&(_obj)->base, QObject, base) : NULL;   \
+_obj ? container_of(&_obj->base, QObject, base) : NULL; \
 })
+#define QOBJECT(obj) QOBJECT_INTERNAL((obj), MAKE_IDENTFIER(_obj))
 
 /* Required for qobject_to() */
 #define QTYPE_CAST_TO_QNull QTYPE_QNULL
diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index d95612f7a0..f1d3d1702a 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -157,13 +157,20 @@
 smp_read_barrier_depends();
 #endif
 
-#define qatomic_rcu_read(ptr)  \
-({ \
+/*
+ * Preprocessor sorcery ahead: use a different identifier for the
+ * local variable in each expansion, so we can nest macro calls
+ * without shadowing variables.
+ */
+#define qatomic_rcu_read_internal(ptr, _val)\
+({  \
 qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \
-typeof_strip_qual(*ptr) _val;  \
-qatomic_rcu_read__nocheck(ptr, &_val

[PATCH v3 1/7] migration/rdma: Fix save_page method to fail on polling error

2023-09-21 Thread Markus Armbruster

qemu_rdma_save_page() reports polling error with error_report(), then
succeeds anyway.  This is because the variable holding the polling
status *shadows* the variable the function returns.  The latter
remains zero.

Broken since day one, and duplicated more recently.

Fixes: 2da776db4846 (rdma: core logic)
Fixes: b390afd8c50b (migration/rdma: Fix out of order wrid)
Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
Reviewed-by: Peter Xu 
Reviewed-by: Li Zhijian 
---
 migration/rdma.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index a2a3db35b1..3915d1d7c9 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -3282,7 +3282,8 @@ static size_t qemu_rdma_save_page(QEMUFile *f,
  */
 while (1) {
 uint64_t wr_id, wr_id_in;
-int ret = qemu_rdma_poll(rdma, rdma->recv_cq, &wr_id_in, NULL);
+ret = qemu_rdma_poll(rdma, rdma->recv_cq, &wr_id_in, NULL);
+
 if (ret < 0) {
 error_report("rdma migration: polling error! %d", ret);
 goto err;
@@ -3297,7 +3298,8 @@ static size_t qemu_rdma_save_page(QEMUFile *f,
 
 while (1) {
 uint64_t wr_id, wr_id_in;
-int ret = qemu_rdma_poll(rdma, rdma->send_cq, &wr_id_in, NULL);
+ret = qemu_rdma_poll(rdma, rdma->send_cq, &wr_id_in, NULL);
+
 if (ret < 0) {
 error_report("rdma migration: polling error! %d", ret);
 goto err;
-- 
2.41.0

[PATCH v3 4/7] block/dirty-bitmap: Clean up local variable shadowing

2023-09-21 Thread Markus Armbruster

Local variables shadowing other local variables or parameters make the
code needlessly hard to understand.  Tracked down with -Wshadow=local.
Clean up: rename both the pair of parameters and the pair of local
variables.  While there, move the local variables to function scope.

Suggested-by: Kevin Wolf 
Signed-off-by: Markus Armbruster 
Reviewed-by: Kevin Wolf 
---
 block/monitor/bitmap-qmp-cmds.c | 19 ++-
 block/qcow2-bitmap.c|  3 +--
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/block/monitor/bitmap-qmp-cmds.c b/block/monitor/bitmap-qmp-cmds.c
index 55f778f5af..70d01a3776 100644
--- a/block/monitor/bitmap-qmp-cmds.c
+++ b/block/monitor/bitmap-qmp-cmds.c
@@ -258,37 +258,38 @@ void qmp_block_dirty_bitmap_disable(const char *node, 
const char *name,
 bdrv_disable_dirty_bitmap(bitmap);
 }
 
-BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
+BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *dst_node,
+  const char *dst_bitmap,
   BlockDirtyBitmapOrStrList *bms,
   HBitmap **backup, Error **errp)
 {
 BlockDriverState *bs;
 BdrvDirtyBitmap *dst, *src;
 BlockDirtyBitmapOrStrList *lst;
+const char *src_node, *src_bitmap;
 HBitmap *local_backup = NULL;
 
 GLOBAL_STATE_CODE();
 
-dst = block_dirty_bitmap_lookup(node, target, &bs, errp);
+dst = block_dirty_bitmap_lookup(dst_node, dst_bitmap, &bs, errp);
 if (!dst) {
 return NULL;
 }
 
 for (lst = bms; lst; lst = lst->next) {
 switch (lst->value->type) {
-const char *name, *node;
 case QTYPE_QSTRING:
-name = lst->value->u.local;
-src = bdrv_find_dirty_bitmap(bs, name);
+src_bitmap = lst->value->u.local;
+src = bdrv_find_dirty_bitmap(bs, src_bitmap);
 if (!src) {
-error_setg(errp, "Dirty bitmap '%s' not found", name);
+error_setg(errp, "Dirty bitmap '%s' not found", src_bitmap);
 goto fail;
 }
 break;
 case QTYPE_QDICT:
-node = lst->value->u.external.node;
-name = lst->value->u.external.name;
-src = block_dirty_bitmap_lookup(node, name, NULL, errp);
+src_node = lst->value->u.external.node;
+src_bitmap = lst->value->u.external.name;
+src = block_dirty_bitmap_lookup(src_node, src_bitmap, NULL, errp);
 if (!src) {
 goto fail;
 }
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 037fa2d435..ffd5cd3b23 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1555,7 +1555,6 @@ bool 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
 FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
 const char *name = bdrv_dirty_bitmap_name(bitmap);
 uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
-Qcow2Bitmap *bm;
 
 if (!bdrv_dirty_bitmap_get_persistence(bitmap) ||
 bdrv_dirty_bitmap_inconsistent(bitmap)) {
@@ -1625,7 +1624,7 @@ bool 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
 
 /* allocate clusters and store bitmaps */
 QSIMPLEQ_FOREACH(bm, bm_list, entry) {
-BdrvDirtyBitmap *bitmap = bm->dirty_bitmap;
+bitmap = bm->dirty_bitmap;
 
 if (bitmap == NULL || bdrv_dirty_bitmap_readonly(bitmap)) {
 continue;
-- 
2.41.0

Re: [PATCH v3 7/7] qobject atomics osdep: Make a few macros more hygienic

2023-09-21 Thread Philippe Mathieu-Daudé


On 21/9/23 14:13, Markus Armbruster wrote:

Variables declared in macros can shadow other variables.  Much of the
time, this is harmless, e.g.:

 #define _FDT(exp)  \
 do {   \
 int ret = (exp);   \
 if (ret < 0) { \
 error_report("error creating device tree: %s: %s",   \
 #exp, fdt_strerror(ret));  \
 exit(1);   \
 }  \
 } while (0)

Harmless shadowing in h_client_architecture_support():

 target_ulong ret;

 [...]

 ret = do_client_architecture_support(cpu, spapr, vec, fdt_bufsize);
 if (ret == H_SUCCESS) {
 _FDT((fdt_pack(spapr->fdt_blob)));
 [...]
 }

 return ret;

However, we can get in trouble when the shadowed variable is used in a
macro argument:

 #define QOBJECT(obj) ({ \
 typeof(obj) o = (obj);  \
 o ? container_of(&(o)->base, QObject, base) : NULL; \
  })

QOBJECT(o) expands into

 ({
--->typeof(o) o = (o);
 o ? container_of(&(o)->base, QObject, base) : NULL;
 })

Unintended variable name capture at --->.  We'd be saved by
-Winit-self.  But I could certainly construct more elaborate death
traps that don't trigger it.

To reduce the risk of trapping ourselves, we use variable names in
macros that no sane person would use elsewhere.  Here's our actual
definition of QOBJECT():

 #define QOBJECT(obj) ({ \
 typeof(obj) _obj = (obj);   \
 _obj ? container_of(&(_obj)->base, QObject, base) : NULL;   \
 })

Works well enough until we nest macro calls.  For instance, with

 #define qobject_ref(obj) ({ \
 typeof(obj) _obj = (obj);   \
 qobject_ref_impl(QOBJECT(_obj));\
 _obj;   \
 })

the expression qobject_ref(obj) expands into

 ({
 typeof(obj) _obj = (obj);
 qobject_ref_impl(
 ({
--->typeof(_obj) _obj = (_obj);
 _obj ? container_of(&(_obj)->base, QObject, base) : NULL;
 }));
 _obj;
 })

Unintended variable name capture at --->.

The only reliable way to prevent unintended variable name capture is
-Wshadow.

One blocker for enabling it is shadowing hiding in function-like
macros like

  qdict_put(dict, "name", qobject_ref(...))

qdict_put() wraps its last argument in QOBJECT(), and the last
argument here contains another QOBJECT().

Use dark preprocessor sorcery to make the macros that give us this
problem use different variable names on every call.

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
  include/qapi/qmp/qobject.h | 10 --
  include/qemu/atomic.h  | 17 -
  include/qemu/compiler.h|  3 +++
  include/qemu/osdep.h   | 27 ---
  4 files changed, 43 insertions(+), 14 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé

Re: [PATCH v2 01/10] qga: Remove platform GUID definitions

2023-09-21 Thread Konstantin Kostiuk

Hi Akihiko,

Thanks for ping.
I will merge this commit with other qga fixes.

Best Regards,
Konstantin Kostiuk.


On Thu, Sep 21, 2023 at 10:58 AM Akihiko Odaki 
wrote:

> On 2022/11/17 18:45, Konstantin Kostiuk wrote:
> > Reviewed-by: Konstantin Kostiuk  > >
> >
> > Will merge this patch in QGA series
> >
> > On Thu, Nov 10, 2022 at 12:06 PM Akihiko Odaki  > > wrote:
> >
> > GUID_DEVINTERFACE_DISK and GUID_DEVINTERFACE_STORAGEPORT are already
> > defined by MinGW-w64. They are not only unnecessary, but can lead to
> > duplicate definition errors at link time with some unknown condition.
> >
> > Signed-off-by: Akihiko Odaki  > >
> > ---
> >   qga/commands-win32.c | 7 ---
> >   1 file changed, 7 deletions(-)
> >
> > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > index ec9f55b453..dde5d401bb 100644
> > --- a/qga/commands-win32.c
> > +++ b/qga/commands-win32.c
> > @@ -506,13 +506,6 @@ static GuestDiskBusType
> > find_bus_type(STORAGE_BUS_TYPE bus)
> >   return win2qemu[(int)bus];
> >   }
> >
> > -DEFINE_GUID(GUID_DEVINTERFACE_DISK,
> > -0x53f56307L, 0xb6bf, 0x11d0, 0x94, 0xf2,
> > -0x00, 0xa0, 0xc9, 0x1e, 0xfb, 0x8b);
> > -DEFINE_GUID(GUID_DEVINTERFACE_STORAGEPORT,
> > -0x2accfe60L, 0xc130, 0x11d2, 0xb0, 0x82,
> > -0x00, 0xa0, 0xc9, 0x1e, 0xfb, 0x8b);
> > -
> >   static void get_pci_address_for_device(GuestPCIAddress *pci,
> >  HDEVINFO dev_info)
> >   {
> > --
> > 2.38.1
> >
>
> Hi Konstantin,
>
> This patch seems missed since then. Can you merge it?
>
> Regards,
> Akihiko Odaki
>
>

Re: [PATCH v2 7/7] qobject atomics osdep: Make a few macros more hygienic

2023-09-21 Thread Kevin Wolf

Am 20.09.2023 um 20:31 hat Markus Armbruster geschrieben:
> Variables declared in macros can shadow other variables.  Much of the
> time, this is harmless, e.g.:
> 
> #define _FDT(exp)  \
> do {   \
> int ret = (exp);   \
> if (ret < 0) { \
> error_report("error creating device tree: %s: %s",   \
> #exp, fdt_strerror(ret));  \
> exit(1);   \
> }  \
> } while (0)
> 
> Harmless shadowing in h_client_architecture_support():
> 
> target_ulong ret;
> 
> [...]
> 
> ret = do_client_architecture_support(cpu, spapr, vec, fdt_bufsize);
> if (ret == H_SUCCESS) {
> _FDT((fdt_pack(spapr->fdt_blob)));
> [...]
> }
> 
> return ret;
> 
> However, we can get in trouble when the shadowed variable is used in a
> macro argument:
> 
> #define QOBJECT(obj) ({ \
> typeof(obj) o = (obj);  \
> o ? container_of(&(o)->base, QObject, base) : NULL; \
>  })
> 
> QOBJECT(o) expands into
> 
> ({
> --->typeof(o) o = (o);
> o ? container_of(&(o)->base, QObject, base) : NULL;
> })
> 
> Unintended variable name capture at --->.  We'd be saved by
> -Winit-self.  But I could certainly construct more elaborate death
> traps that don't trigger it.
> 
> To reduce the risk of trapping ourselves, we use variable names in
> macros that no sane person would use elsewhere.  Here's our actual
> definition of QOBJECT():
> 
> #define QOBJECT(obj) ({ \
> typeof(obj) _obj = (obj);   \
> _obj ? container_of(&(_obj)->base, QObject, base) : NULL;   \
> })
> 
> Works well enough until we nest macro calls.  For instance, with
> 
> #define qobject_ref(obj) ({ \
> typeof(obj) _obj = (obj);   \
> qobject_ref_impl(QOBJECT(_obj));\
> _obj;   \
> })
> 
> the expression qobject_ref(obj) expands into
> 
> ({
> typeof(obj) _obj = (obj);
> qobject_ref_impl(
> ({
> --->typeof(_obj) _obj = (_obj);
> _obj ? container_of(&(_obj)->base, QObject, base) : NULL;
> }));
> _obj;
> })
> 
> Unintended variable name capture at --->.
> 
> The only reliable way to prevent unintended variable name capture is
> -Wshadow.
> 
> One blocker for enabling it is shadowing hiding in function-like
> macros like
> 
>  qdict_put(dict, "name", qobject_ref(...))
> 
> qdict_put() wraps its last argument in QOBJECT(), and the last
> argument here contains another QOBJECT().
> 
> Use dark preprocessor sorcery to make the macros that give us this
> problem use different variable names on every call.
> 
> Signed-off-by: Markus Armbruster 
> Reviewed-by: Eric Blake 
> ---
>  include/qapi/qmp/qobject.h | 11 +--
>  include/qemu/atomic.h  | 17 -
>  include/qemu/compiler.h|  3 +++
>  include/qemu/osdep.h   | 31 +++
>  4 files changed, 47 insertions(+), 15 deletions(-)
> 
> diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
> index 9003b71fd3..d36cc97805 100644
> --- a/include/qapi/qmp/qobject.h
> +++ b/include/qapi/qmp/qobject.h
> @@ -45,10 +45,17 @@ struct QObject {
>  struct QObjectBase_ base;
>  };
>  
> -#define QOBJECT(obj) ({ \
> +/*
> + * Preprocessory sorcery ahead: use a different identifier for the
> + * local variable in each expansion, so we can nest macro calls
> + * without shadowing variables.
> + */
> +#define QOBJECT_INTERNAL(obj, _obj) ({  \
>  typeof(obj) _obj = (obj);   \
> -_obj ? container_of(&(_obj)->base, QObject, base) : NULL;   \
> +_obj\
> +? container_of(&(_obj)->base, QObject, base) : NULL;\

What happened here? The code in this line (or now two lines) seems to be
unchanged apart from a strange looking newline.

>  })
> +#define QOBJECT(obj) QOBJECT_INTERNAL((obj), MAKE_IDENTFIER(_obj))

Kevin

Re: [PULL 4/5] hw/ufs: Support for UFS logical unit

2023-09-21 Thread Jeuk Kim




On 2023-09-15 4:59 PM, Paolo Bonzini wrote:

On 9/15/23 00:19, Jeuk Kim wrote:
First, ufs-lu has a feature called "unit descriptor". This feature 
shows the status of the ufs-lu


and only works with UFS-specific "query request" commands, not SCSI 
commands.


This looks like something that can be implemented in the UFS subsystem.

UFS also has something called a well-known lu. Unlike typical SCSI 
devices, where each lu is independent,

UFS can control other lu's through the well-known lu.


This can also be implemented in UfsBus.

Finally, UFS-LU will have features that SCSI-HD does not have, such 
as the zone block command.


These should be implemented in scsi-hd as well.

In addition to this, I wanted some scsi commands to behave 
differently from scsi-hd, for example,

the Inquiry command should read "QEMU UFS" instead of "QEMU HARDDISK",
and the mode_sense_page command should have a different result.


Some of these don't have much justification, and others (such as the 
control page) could be done in scsi-hd as well.


We should look into cleaning this up and making ufs-lu share a lot 
more code with scsi-hd; possibly even supporting -device scsi-hd with 
UFS devices.  I am not going to ask you for a revert, but if this is 
not done before 8.2 is out, I will ask you to disable it by default in 
hw/ufs/Kconfig.


In the future, please Cc the SCSI maintainers for UFS patches.

Paolo


Dear Paolo

Hi. I've been looking into how ufs-lu can share code with scsi-hd.

I have verified that ufs-lu can use scsi-hd's code, and I would like to modify 
it to do so.

I've validated two possible fixes.
I'd like to hear your thoughts.

Option1.
As you mentioned, using ufsbus, which inherits from scsibus, removes the ufs-lu 
device type and use scsi-hd. (like -device ufs,id=ufs0 -device scsi-hd,bus=ufs0)
I've verified that this method is implementable, except for one problem.
Because we are using the scsi-hd type instead of the ufs-lu type, the ufs has 
to manage all the ufs-lu specific data (such as the unit descriptor).
However, since there is no ufs_lu_realize() function, we need a way to notify 
the ufs when a new scsi-hd device is added.
Would there be a way to let the ufs know that a new scsi-hd has been added at 
scsi_hd_realize() time?

Option 2.
Use qdev_new() & qdev_realize() to make ufs-lu have a virtual scsi bus and 
scsi-hd.
The ufs-lu can pass through SCSI commands to the virtual scsi-hd.
This is similar to the method used by the device "usb-storage".

With this method, I can keep the ufs-lu device type (ufs_lu_realize() makes it 
convenient to manage ufs-lu related data) and avoid duplicating code with 
scsi-hd.
So I prefer this approach, but the annotation for "usb-storage" is marked with a 
"Hack alert", so I'm not sure if this is the right way.
The code can be found in usb_msd_storage_realize() 
(hw/usb/dev-storage-classic.c:51).

I am wondering if you could give me some advice on this and your preferred 
direction for fixing it.

Thank you so much.

Jeuk

Re: [PATCH v2 4/7] block/dirty-bitmap: Clean up local variable shadowing

2023-09-21 Thread Kevin Wolf

Am 20.09.2023 um 20:31 hat Markus Armbruster geschrieben:
> Local variables shadowing other local variables or parameters make the
> code needlessly hard to understand.  Tracked down with -Wshadow=local.
> Clean up: rename both the pair of parameters and the pair of local
> variables.  While there, move the local variables to function scope.
> 
> Suggested-by: Kevin Wolf 
> Signed-off-by: Markus Armbruster 

Reviewed-by: Kevin Wolf

Re: [PATCH v2 6/7] block: Clean up local variable shadowing

2023-09-21 Thread Kevin Wolf

Am 20.09.2023 um 20:31 hat Markus Armbruster geschrieben:
> Local variables shadowing other local variables or parameters make the
> code needlessly hard to understand.  Tracked down with -Wshadow=local.
> Clean up: delete inner declarations when they are actually redundant,
> else rename variables.
> 
> Signed-off-by: Markus Armbruster 
> Reviewed-by: Stefan Hajnoczi 
> Acked-by: Anthony PERARD 
> Acked-by: Ilya Dryomov 

Reviewed-by: Kevin Wolf

Re: [PATCH v2 5/7] block/vdi: Clean up local variable shadowing

2023-09-21 Thread Kevin Wolf

Am 20.09.2023 um 20:31 hat Markus Armbruster geschrieben:
> Local variables shadowing other local variables or parameters make the
> code needlessly hard to understand.  Tracked down with -Wshadow=local.
> Clean up: delete inner declarations when they are actually redundant,
> else rename variables.
> 
> Signed-off-by: Markus Armbruster 
> Reviewed-by: Stefan Hajnoczi 

Reviewed-by: Kevin Wolf

[PULL v2 20/22] tests: extend test 131 to cover availability of the discard operation

2023-09-21 Thread Denis V. Lunev

This patch contains test which minimally tests discard and new cluster
allocation logic.

The following checks are added:
* write 2 clusters, discard the first allocated
* write another cluster, check that the hole is filled
* write 2 clusters, discard the first allocated, write 1 cluster at
  non-aligned to cluster offset (2 new clusters should be allocated)

Signed-off-by: Denis V. Lunev 
Reviewed-by: Alexander Ivanov 
---
 tests/qemu-iotests/131 | 31 +++
 tests/qemu-iotests/131.out | 38 ++
 2 files changed, 69 insertions(+)

diff --git a/tests/qemu-iotests/131 b/tests/qemu-iotests/131
index 304bbb3f61..324008b3f6 100755
--- a/tests/qemu-iotests/131
+++ b/tests/qemu-iotests/131
@@ -74,6 +74,37 @@ poke_file "$TEST_IMG" "$inuse_offset" "\x59\x6e\x6f\x74"
 echo "== read corrupted image with repairing =="
 { $QEMU_IO -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | 
_filter_qemu_io | _filter_testdir
 
+echo "== check discard =="
+
+# Clear image
+_make_test_img $size
+
+{ $QEMU_IO -c "write -P 0x11 0 $CLUSTER_DBL_SIZE" "$TEST_IMG"; } 2>&1 | 
_filter_qemu_io | _filter_testdir
+{ $QEMU_IMG map "$TEST_IMG"; } 2>&1 | _filter_qemu_img_map
+{ $QEMU_IO -c "discard 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io 
| _filter_testdir
+{ $QEMU_IMG map "$TEST_IMG"; } 2>&1 | _filter_qemu_img_map
+{ $QEMU_IO -c "read -P 0 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | 
_filter_qemu_io | _filter_testdir
+
+echo "== check simple allocation over the discarded hole =="
+
+{ $QEMU_IO -c "write -P 0x11 $CLUSTER_DBL_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 
2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IMG map "$TEST_IMG"; } 2>&1 | _filter_qemu_img_map
+{ $QEMU_IO -c "read -P 0x11 $CLUSTER_DBL_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 
2>&1 | _filter_qemu_io | _filter_testdir
+
+echo "== check more complex allocation over the discard hole =="
+
+# Clear image
+_make_test_img $size
+
+{ $QEMU_IO -c "write -P 0x11 $CLUSTER_DBL_SIZE $CLUSTER_DBL_SIZE" "$TEST_IMG"; 
} 2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "discard $CLUSTER_DBL_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | 
_filter_qemu_io | _filter_testdir
+# There is 1 cluster hole. Fill it fully and allocate 1 cluster at the end
+{ $QEMU_IO -c "write -P 0x12 $CLUSTER_HALF_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 
2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IMG map "$TEST_IMG"; } 2>&1 | _filter_qemu_img_map
+{ $QEMU_IO -c "read -P 0x12 $CLUSTER_HALF_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 
2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0 0 $CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 | 
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0 $((CLUSTER_SIZE + CLUSTER_HALF_SIZE)) 
$CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+
 echo "== allocate with backing =="
 # Verify that allocating clusters works fine even when there is a backing 
image.
 # Regression test for a bug where we would pass a buffer read from the backing
diff --git a/tests/qemu-iotests/131.out b/tests/qemu-iotests/131.out
index d2904578df..27df91ca97 100644
--- a/tests/qemu-iotests/131.out
+++ b/tests/qemu-iotests/131.out
@@ -26,6 +26,44 @@ read 524288/524288 bytes at offset 0
 Repairing image was not closed correctly
 read 1048576/1048576 bytes at offset 1048576
 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check discard ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 2097152/2097152 bytes at offset 0
+2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset  Length  File
+0   0x20TEST_DIR/t.IMGFMT
+discard 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset  Length  File
+0x100x10TEST_DIR/t.IMGFMT
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check simple allocation over the discarded hole ==
+wrote 1048576/1048576 bytes at offset 2097152
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset  Length  File
+0x100x10TEST_DIR/t.IMGFMT
+0x200x10TEST_DIR/t.IMGFMT
+read 1048576/1048576 bytes at offset 2097152
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check more complex allocation over the discard hole ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 2097152/2097152 bytes at offset 2097152
+2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+discard 1048576/1048576 bytes at offset 2097152
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1048576/1048576 bytes at offset 524288
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset  Length  File
+0   0x10TEST_DIR/t.IMGFMT
+0x100x10TEST_DIR/t.IMGFMT
+0x300x10TEST_DIR/t.IMGFMT
+read 1048576/1048576 bytes at

[PULL v2 21/22] parallels: naive implementation of parallels_co_pwrite_zeroes

2023-09-21 Thread Denis V. Lunev

The zero flag is missed in the Parallels format specification. We can
resort to discard if we have no backing file.

Signed-off-by: Denis V. Lunev 
Reviewed-by: Alexander Ivanov 
---
 block/parallels.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/block/parallels.c b/block/parallels.c
index 1808029f14..d026ce9e2f 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -582,6 +582,19 @@ done:
 return ret;
 }
 
+static int coroutine_fn GRAPH_RDLOCK
+parallels_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int64_t bytes,
+   BdrvRequestFlags flags)
+{
+/*
+ * The zero flag is missed in the Parallels format specification. We can
+ * resort to discard if we have no backing file (this condition is checked
+ * inside parallels_co_pdiscard().
+ */
+return parallels_co_pdiscard(bs, offset, bytes);
+}
+
+
 static void parallels_check_unclean(BlockDriverState *bs,
 BdrvCheckResult *res,
 BdrvCheckMode fix)
@@ -1463,6 +1476,7 @@ static BlockDriver bdrv_parallels = {
 .bdrv_co_create_opts= parallels_co_create_opts,
 .bdrv_co_check  = parallels_co_check,
 .bdrv_co_pdiscard   = parallels_co_pdiscard,
+.bdrv_co_pwrite_zeroes  = parallels_co_pwrite_zeroes,
 };
 
 static void bdrv_parallels_init(void)
-- 
2.34.1

[PULL v2 07/22] parallels: refactor path when we need to re-check image in parallels_open

2023-09-21 Thread Denis V. Lunev

More conditions follows thus the check should be more scalable.

Signed-off-by: Denis V. Lunev 
Reviewed-by: Alexander Ivanov 
---
 block/parallels.c | 21 ++---
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index bd26c8db63..af3b4894d7 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1071,7 +1071,7 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
 int ret, size, i;
 int64_t file_nb_sectors, sector;
 uint32_t data_start;
-bool data_off_is_correct;
+bool need_check = false;
 
 ret = parallels_opts_prealloc(bs, options, errp);
 if (ret < 0) {
@@ -1139,11 +1139,14 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
 s->bat_bitmap = (uint32_t *)(s->header + 1);
 
 if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
-s->header_unclean = true;
+need_check = s->header_unclean = true;
+}
+
+{
+bool ok = parallels_test_data_off(s, file_nb_sectors, &data_start);
+need_check = need_check || !ok;
 }
 
-data_off_is_correct = parallels_test_data_off(s, file_nb_sectors,
-  &data_start);
 s->data_start = data_start;
 s->data_end = s->data_start;
 if (s->data_end < (s->header_size >> BDRV_SECTOR_BITS)) {
@@ -1200,6 +1203,7 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
 s->data_end = sector + s->tracks;
 }
 }
+need_check = need_check || s->data_end > file_nb_sectors;
 
 /*
  * We don't repair the image here if it's opened for checks. Also we don't
@@ -1209,12 +1213,8 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
 return 0;
 }
 
-/*
- * Repair the image if it's dirty or
- * out-of-image corruption was detected.
- */
-if (s->data_end > file_nb_sectors || s->header_unclean
-|| !data_off_is_correct) {
+/* Repair the image if corruption was detected. */
+if (need_check) {
 BdrvCheckResult res;
 ret = bdrv_check(bs, &res, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
 if (ret < 0) {
@@ -1223,7 +1223,6 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
 goto fail;
 }
 }
-
 return 0;
 
 fail_format:
-- 
2.34.1

[PULL v2 11/22] parallels: add test which will validate data_off fixes through repair

2023-09-21 Thread Denis V. Lunev

We have only check through self-repair and that proven to be not enough.

Signed-off-by: Denis V. Lunev 
Reviewed-by: Alexander Ivanov 
---
 tests/qemu-iotests/tests/parallels-checks | 17 +
 tests/qemu-iotests/tests/parallels-checks.out | 18 ++
 2 files changed, 35 insertions(+)

diff --git a/tests/qemu-iotests/tests/parallels-checks 
b/tests/qemu-iotests/tests/parallels-checks
index 5917ee079d..f4ca50295e 100755
--- a/tests/qemu-iotests/tests/parallels-checks
+++ b/tests/qemu-iotests/tests/parallels-checks
@@ -140,6 +140,23 @@ poke_file "$TEST_IMG" "$DATA_OFF_OFFSET" "\xff\xff\xff\xff"
 echo "== check first cluster =="
 { $QEMU_IO -c "read -P 0x55 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | 
_filter_qemu_io | _filter_testdir
 
+# Clear image
+_make_test_img $SIZE
+
+echo "== TEST DATA_OFF THROUGH REPAIR =="
+
+echo "== write pattern to first cluster =="
+{ $QEMU_IO -c "write -P 0x55 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | 
_filter_qemu_io | _filter_testdir
+
+echo "== spoil data_off field =="
+poke_file "$TEST_IMG" "$DATA_OFF_OFFSET" "\xff\xff\xff\xff"
+
+echo "== repair image =="
+_check_test_img -r all
+
+echo "== check first cluster =="
+{ $QEMU_IO -r -c "read -P 0x55 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | 
_filter_qemu_io | _filter_testdir
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/tests/parallels-checks.out 
b/tests/qemu-iotests/tests/parallels-checks.out
index 98a3a7f55e..74a5e29260 100644
--- a/tests/qemu-iotests/tests/parallels-checks.out
+++ b/tests/qemu-iotests/tests/parallels-checks.out
@@ -72,4 +72,22 @@ wrote 1048576/1048576 bytes at offset 0
 Repairing data_off field has incorrect value
 read 1048576/1048576 bytes at offset 0
 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=4194304
+== TEST DATA_OFF THROUGH REPAIR ==
+== write pattern to first cluster ==
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== spoil data_off field ==
+== repair image ==
+Repairing data_off field has incorrect value
+The following inconsistencies were found and repaired:
+
+0 leaked clusters
+1 corruptions
+
+Double checking the fixed image now...
+No errors were found on the image.
+== check first cluster ==
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 *** done
-- 
2.34.1

[PULL v2 17/22] parallels: naive implementation of allocate_clusters with used bitmap

2023-09-21 Thread Denis V. Lunev

The access to the bitmap is not optimized completely.

Signed-off-by: Denis V. Lunev 
Reviewed-by: Alexander Ivanov 
---
 block/parallels.c | 51 ---
 1 file changed, 39 insertions(+), 12 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index ec35237119..ebcdff8736 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -253,7 +253,7 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
 {
 int ret = 0;
 BDRVParallelsState *s = bs->opaque;
-int64_t pos, space, idx, to_allocate, i, len;
+int64_t i, pos, idx, to_allocate, first_free, host_off;
 
 pos = block_status(s, sector_num, nb_sectors, pnum);
 if (pos > 0) {
@@ -276,15 +276,13 @@ allocate_clusters(BlockDriverState *bs, int64_t 
sector_num,
  */
 assert(idx < s->bat_size && idx + to_allocate <= s->bat_size);
 
-space = to_allocate * s->tracks;
-len = bdrv_co_getlength(bs->file->bs);
-if (len < 0) {
-return len;
-}
-if (s->data_end + space > (len >> BDRV_SECTOR_BITS)) {
+first_free = find_first_zero_bit(s->used_bmap, s->used_bmap_size);
+if (first_free == s->used_bmap_size) {
 uint32_t new_usedsize;
+int64_t space = to_allocate * s->tracks + s->prealloc_size;
+
+host_off = s->data_end * BDRV_SECTOR_SIZE;
 
-space += s->prealloc_size;
 /*
  * We require the expanded size to read back as zero. If the
  * user permitted truncation, we try that; but if it fails, we
@@ -313,6 +311,32 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
 s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size,
   new_usedsize);
 s->used_bmap_size = new_usedsize;
+} else {
+int64_t next_used;
+next_used = find_next_bit(s->used_bmap, s->used_bmap_size, first_free);
+
+/* Not enough continuous clusters in the middle, adjust the size */
+if (next_used - first_free < to_allocate) {
+to_allocate = next_used - first_free;
+*pnum = (idx + to_allocate) * s->tracks - sector_num;
+}
+
+host_off = s->data_start * BDRV_SECTOR_SIZE;
+host_off += first_free * s->cluster_size;
+
+/*
+ * No need to preallocate if we are using tail area from the above
+ * branch. In the other case we are likely re-using hole. Preallocate
+ * the space if required by the prealloc_mode.
+ */
+if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE &&
+host_off < s->data_end * BDRV_SECTOR_SIZE) {
+ret = bdrv_co_pwrite_zeroes(bs->file, host_off,
+s->cluster_size * to_allocate, 0);
+if (ret < 0) {
+return ret;
+}
+}
 }
 
 /*
@@ -344,15 +368,18 @@ allocate_clusters(BlockDriverState *bs, int64_t 
sector_num,
 }
 }
 
-ret = mark_used(bs, s->used_bmap, s->used_bmap_size,
-s->data_end << BDRV_SECTOR_BITS, to_allocate);
+ret = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off, 
to_allocate);
 if (ret < 0) {
 /* Image consistency is broken. Alarm! */
 return ret;
 }
 for (i = 0; i < to_allocate; i++) {
-parallels_set_bat_entry(s, idx + i, s->data_end / s->off_multiplier);
-s->data_end += s->tracks;
+parallels_set_bat_entry(s, idx + i,
+host_off / BDRV_SECTOR_SIZE / s->off_multiplier);
+host_off += s->cluster_size;
+}
+if (host_off > s->data_end * BDRV_SECTOR_SIZE) {
+s->data_end = host_off / BDRV_SECTOR_SIZE;
 }
 
 return bat2sect(s, idx) + sector_num % s->tracks;
-- 
2.34.1

[PULL v2 15/22] parallels: accept multiple clusters in mark_used()

2023-09-21 Thread Denis V. Lunev

This would be useful in the next patch in allocate_clusters(). This
change would not imply serious performance drawbacks as usually image
is full of data or are at the end of the bitmap.

Signed-off-by: Denis V. Lunev 
Reviewed-by: Alexander Ivanov 
---
 block/parallels.c | 18 ++
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index a997880c34..3df73aa8a0 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -178,18 +178,20 @@ static void parallels_set_bat_entry(BDRVParallelsState *s,
 bitmap_set(s->bat_dirty_bmap, bat_entry_off(index) / s->bat_dirty_block, 
1);
 }
 
-static int mark_used(BlockDriverState *bs,
- unsigned long *bitmap, uint32_t bitmap_size, int64_t off)
+static int mark_used(BlockDriverState *bs, unsigned long *bitmap,
+ uint32_t bitmap_size, int64_t off, uint32_t count)
 {
 BDRVParallelsState *s = bs->opaque;
 uint32_t cluster_index = host_cluster_index(s, off);
-if (cluster_index >= bitmap_size) {
+unsigned long next_used;
+if (cluster_index + count > bitmap_size) {
 return -E2BIG;
 }
-if (test_bit(cluster_index, bitmap)) {
+next_used = find_next_bit(bitmap, bitmap_size, cluster_index);
+if (next_used < cluster_index + count) {
 return -EBUSY;
 }
-bitmap_set(bitmap, cluster_index, 1);
+bitmap_set(bitmap, cluster_index, count);
 return 0;
 }
 
@@ -230,7 +232,7 @@ static int parallels_fill_used_bitmap(BlockDriverState *bs)
 continue;
 }
 
-err2 = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off);
+err2 = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off, 1);
 if (err2 < 0 && err == 0) {
 err = err2;
 }
@@ -732,7 +734,7 @@ parallels_check_duplicate(BlockDriverState *bs, 
BdrvCheckResult *res,
 continue;
 }
 
-ret = mark_used(bs, bitmap, bitmap_size, host_off);
+ret = mark_used(bs, bitmap, bitmap_size, host_off, 1);
 assert(ret != -E2BIG);
 if (ret == 0) {
 continue;
@@ -792,7 +794,7 @@ parallels_check_duplicate(BlockDriverState *bs, 
BdrvCheckResult *res,
  * considered, and the bitmap size doesn't change. This specifically
  * means that -E2BIG is OK.
  */
-ret = mark_used(bs, bitmap, bitmap_size, host_off);
+ret = mark_used(bs, bitmap, bitmap_size, host_off, 1);
 if (ret == -EBUSY) {
 res->check_errors++;
 goto out_repair_bat;
-- 
2.34.1

[PULL v2 10/22] parallels: fix broken parallels_check_data_off()

2023-09-21 Thread Denis V. Lunev

Once we have repaired data_off field in the header we should update
s->data_start which is calculated on the base of it.

Signed-off-by: Denis V. Lunev 
Reviewed-by: Alexander Ivanov 
---
 block/parallels.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/parallels.c b/block/parallels.c
index 66c86d87e3..2b5f2b54a0 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -531,6 +531,7 @@ parallels_check_data_off(BlockDriverState *bs, 
BdrvCheckResult *res,
 res->corruptions++;
 if (fix & BDRV_FIX_ERRORS) {
 s->header->data_off = cpu_to_le32(data_off);
+s->data_start = data_off;
 res->corruptions_fixed++;
 }
 
-- 
2.34.1

[PATCH v3 2/8] ivshmem-server: Use qemu_get_runtime_dir()

2023-09-21 Thread Akihiko Odaki

qemu_get_runtime_dir() is used to construct the default PID file path.

Signed-off-by: Akihiko Odaki 
---
 contrib/ivshmem-server/main.c | 20 
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/contrib/ivshmem-server/main.c b/contrib/ivshmem-server/main.c
index 5901f17707..313124dd45 100644
--- a/contrib/ivshmem-server/main.c
+++ b/contrib/ivshmem-server/main.c
@@ -14,7 +14,6 @@
 
 #define IVSHMEM_SERVER_DEFAULT_VERBOSE0
 #define IVSHMEM_SERVER_DEFAULT_FOREGROUND 0
-#define IVSHMEM_SERVER_DEFAULT_PID_FILE   "/var/run/ivshmem-server.pid"
 #define IVSHMEM_SERVER_DEFAULT_UNIX_SOCK_PATH "/tmp/ivshmem_socket"
 #define IVSHMEM_SERVER_DEFAULT_SHM_PATH   "ivshmem"
 #define IVSHMEM_SERVER_DEFAULT_SHM_SIZE   (4 * 1024 * 1024)
@@ -35,15 +34,23 @@ typedef struct IvshmemServerArgs {
 unsigned n_vectors;
 } IvshmemServerArgs;
 
+static char *ivshmem_server_get_default_pid_file(void)
+{
+g_autofree char *run = qemu_get_runtime_dir();
+return g_build_filename(run, "ivshmem-server.pid", NULL);
+}
+
 static void
 ivshmem_server_usage(const char *progname)
 {
+g_autofree char *pid_file = ivshmem_server_get_default_pid_file();
+
 printf("Usage: %s [OPTION]...\n"
"  -h: show this help\n"
"  -v: verbose mode\n"
"  -F: foreground mode (default is to daemonize)\n"
"  -p : path to the PID file (used in daemon mode only)\n"
-   " default " IVSHMEM_SERVER_DEFAULT_PID_FILE "\n"
+   " default %s\n"
"  -S : path to the unix socket to listen to\n"
" default " IVSHMEM_SERVER_DEFAULT_UNIX_SOCK_PATH "\n"
"  -M : POSIX shared memory object to use\n"
@@ -54,7 +61,7 @@ ivshmem_server_usage(const char *progname)
" default %u\n"
"  -n : number of vectors\n"
" default %u\n",
-   progname, IVSHMEM_SERVER_DEFAULT_SHM_SIZE,
+   progname, pid_file, IVSHMEM_SERVER_DEFAULT_SHM_SIZE,
IVSHMEM_SERVER_DEFAULT_N_VECTORS);
 }
 
@@ -189,10 +196,10 @@ main(int argc, char *argv[])
 {
 IvshmemServer server;
 struct sigaction sa, sa_quit;
+g_autofree char *default_pid_file = NULL;
 IvshmemServerArgs args = {
 .verbose = IVSHMEM_SERVER_DEFAULT_VERBOSE,
 .foreground = IVSHMEM_SERVER_DEFAULT_FOREGROUND,
-.pid_file = IVSHMEM_SERVER_DEFAULT_PID_FILE,
 .unix_socket_path = IVSHMEM_SERVER_DEFAULT_UNIX_SOCK_PATH,
 .shm_path = IVSHMEM_SERVER_DEFAULT_SHM_PATH,
 .use_shm_open = true,
@@ -207,6 +214,11 @@ main(int argc, char *argv[])
  */
 printf("*** Example code, do not use in production ***\n");
 
+qemu_init_exec_dir(argv[0]);
+
+default_pid_file = ivshmem_server_get_default_pid_file();
+args.pid_file = default_pid_file;
+
 /* parse arguments, will exit on error */
 ivshmem_server_parse_args(&args, argc, argv);
 
-- 
2.41.0

[PULL v2 16/22] parallels: update used bitmap in allocate_cluster

2023-09-21 Thread Denis V. Lunev

We should extend the bitmap if the file is extended and set the bit in
the image used bitmap once the cluster is allocated. Sanity check at
that moment also looks like a good idea.

Signed-off-by: Denis V. Lunev 
Reviewed-by: Alexander Ivanov 
---
 block/parallels.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/block/parallels.c b/block/parallels.c
index 3df73aa8a0..ec35237119 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -282,6 +282,8 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
 return len;
 }
 if (s->data_end + space > (len >> BDRV_SECTOR_BITS)) {
+uint32_t new_usedsize;
+
 space += s->prealloc_size;
 /*
  * We require the expanded size to read back as zero. If the
@@ -305,6 +307,12 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
 if (ret < 0) {
 return ret;
 }
+
+new_usedsize = s->used_bmap_size +
+   (space << BDRV_SECTOR_BITS) / s->cluster_size;
+s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size,
+  new_usedsize);
+s->used_bmap_size = new_usedsize;
 }
 
 /*
@@ -336,6 +344,12 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
 }
 }
 
+ret = mark_used(bs, s->used_bmap, s->used_bmap_size,
+s->data_end << BDRV_SECTOR_BITS, to_allocate);
+if (ret < 0) {
+/* Image consistency is broken. Alarm! */
+return ret;
+}
 for (i = 0; i < to_allocate; i++) {
 parallels_set_bat_entry(s, idx + i, s->data_end / s->off_multiplier);
 s->data_end += s->tracks;
-- 
2.34.1

[PULL v2 12/22] parallels: collect bitmap of used clusters at open

2023-09-21 Thread Denis V. Lunev

If the operation is failed, we need to check image consistency if the
problem is not about memory allocation.

Bitmap adjustments in allocate_cluster are not performed yet.
They worth to be separate. This was proven useful during debug of this
series. Kept as is for future bissecting.

It should be specifically noted that used bitmap must be recalculated
if data_off has been fixed during image consistency check.

Signed-off-by: Denis V. Lunev 
Reviewed-by: Alexander Ivanov 
---
 block/parallels.c | 73 +++
 block/parallels.h |  3 ++
 2 files changed, 76 insertions(+)

diff --git a/block/parallels.c b/block/parallels.c
index 2b5f2b54a0..a997880c34 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -193,6 +193,58 @@ static int mark_used(BlockDriverState *bs,
 return 0;
 }
 
+/*
+ * Collect used bitmap. The image can contain errors, we should fill the
+ * bitmap anyway, as much as we can. This information will be used for
+ * error resolution.
+ */
+static int parallels_fill_used_bitmap(BlockDriverState *bs)
+{
+BDRVParallelsState *s = bs->opaque;
+int64_t payload_bytes;
+uint32_t i;
+int err = 0;
+
+payload_bytes = bdrv_getlength(bs->file->bs);
+if (payload_bytes < 0) {
+return payload_bytes;
+}
+payload_bytes -= s->data_start * BDRV_SECTOR_SIZE;
+if (payload_bytes < 0) {
+return -EINVAL;
+}
+
+s->used_bmap_size = DIV_ROUND_UP(payload_bytes, s->cluster_size);
+if (s->used_bmap_size == 0) {
+return 0;
+}
+s->used_bmap = bitmap_try_new(s->used_bmap_size);
+if (s->used_bmap == NULL) {
+return -ENOMEM;
+}
+
+for (i = 0; i < s->bat_size; i++) {
+int err2;
+int64_t host_off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+if (host_off == 0) {
+continue;
+}
+
+err2 = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off);
+if (err2 < 0 && err == 0) {
+err = err2;
+}
+}
+return err;
+}
+
+static void parallels_free_used_bitmap(BlockDriverState *bs)
+{
+BDRVParallelsState *s = bs->opaque;
+s->used_bmap_size = 0;
+g_free(s->used_bmap);
+}
+
 static int64_t coroutine_fn GRAPH_RDLOCK
 allocate_clusters(BlockDriverState *bs, int64_t sector_num,
   int nb_sectors, int *pnum)
@@ -530,8 +582,17 @@ parallels_check_data_off(BlockDriverState *bs, 
BdrvCheckResult *res,
 
 res->corruptions++;
 if (fix & BDRV_FIX_ERRORS) {
+int err;
 s->header->data_off = cpu_to_le32(data_off);
 s->data_start = data_off;
+
+parallels_free_used_bitmap(bs);
+err = parallels_fill_used_bitmap(bs);
+if (err == -ENOMEM) {
+res->check_errors++;
+return err;
+}
+
 res->corruptions_fixed++;
 }
 
@@ -1222,6 +1283,14 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
 }
 need_check = need_check || s->data_end > file_nb_sectors;
 
+if (!need_check) {
+ret = parallels_fill_used_bitmap(bs);
+if (ret == -ENOMEM) {
+goto fail;
+}
+need_check = need_check || ret < 0; /* These are correctable errors */
+}
+
 /*
  * We don't repair the image here if it's opened for checks. Also we don't
  * want to change inactive images and can't change readonly images.
@@ -1251,6 +1320,8 @@ fail:
  * "s" object was allocated by g_malloc0 so we can safely
  * try to free its fields even they were not allocated.
  */
+parallels_free_used_bitmap(bs);
+
 error_free(s->migration_blocker);
 g_free(s->bat_dirty_bmap);
 qemu_vfree(s->header);
@@ -1271,6 +1342,8 @@ static void parallels_close(BlockDriverState *bs)
   PREALLOC_MODE_OFF, 0, NULL);
 }
 
+parallels_free_used_bitmap(bs);
+
 g_free(s->bat_dirty_bmap);
 qemu_vfree(s->header);
 
diff --git a/block/parallels.h b/block/parallels.h
index 4e53e9572d..6b199443cf 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -72,6 +72,9 @@ typedef struct BDRVParallelsState {
 unsigned long *bat_dirty_bmap;
 unsigned int  bat_dirty_block;
 
+unsigned long *used_bmap;
+unsigned long used_bmap_size;
+
 uint32_t *bat_bitmap;
 unsigned int bat_size;
 
-- 
2.34.1

[PULL v2 13/22] tests: fix broken deduplication check in parallels format test

2023-09-21 Thread Denis V. Lunev

Original check is broken as supposed reading from 2 different clusters
results in read from the same file offset twice. This is definitely
wrong.

We should be sure that
* the content of both clusters is correct after repair
* clusters are at the different offsets after repair
In order to check the latter we write some content into the first one
and validate that fact.

Signed-off-by: Denis V. Lunev 
Reviewed-by: Alexander Ivanov 
---
 tests/qemu-iotests/tests/parallels-checks | 14 ++
 tests/qemu-iotests/tests/parallels-checks.out | 16 
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/tests/qemu-iotests/tests/parallels-checks 
b/tests/qemu-iotests/tests/parallels-checks
index f4ca50295e..df99558486 100755
--- a/tests/qemu-iotests/tests/parallels-checks
+++ b/tests/qemu-iotests/tests/parallels-checks
@@ -117,14 +117,20 @@ echo "== check second cluster =="
 echo "== repair image =="
 _check_test_img -r all
 
+echo "== check the first cluster =="
+{ $QEMU_IO -r -c "read -P 0x11 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | 
_filter_qemu_io | _filter_testdir
+
 echo "== check second cluster =="
 { $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 
2>&1 | _filter_qemu_io | _filter_testdir
 
-echo "== check first cluster on host =="
-printf "content: 0x%02x\n" `peek_file_le $TEST_IMG $(($CLUSTER_SIZE)) 1`
+echo "== write another pattern to the first clusters =="
+{ $QEMU_IO -c "write -P 0x66 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | 
_filter_qemu_io | _filter_testdir
+
+echo "== check the first cluster =="
+{ $QEMU_IO -r -c "read -P 0x66 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | 
_filter_qemu_io | _filter_testdir
 
-echo "== check second cluster on host =="
-printf "content: 0x%02x\n" `peek_file_le $TEST_IMG $(($CLUSTER_SIZE)) 1`
+echo "== check the second cluster (deduplicated) =="
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 
2>&1 | _filter_qemu_io | _filter_testdir
 
 # Clear image
 _make_test_img $SIZE
diff --git a/tests/qemu-iotests/tests/parallels-checks.out 
b/tests/qemu-iotests/tests/parallels-checks.out
index 74a5e29260..1325d2b611 100644
--- a/tests/qemu-iotests/tests/parallels-checks.out
+++ b/tests/qemu-iotests/tests/parallels-checks.out
@@ -55,13 +55,21 @@ The following inconsistencies were found and repaired:
 
 Double checking the fixed image now...
 No errors were found on the image.
+== check the first cluster ==
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 == check second cluster ==
 read 1048576/1048576 bytes at offset 1048576
 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-== check first cluster on host ==
-content: 0x11
-== check second cluster on host ==
-content: 0x11
+== write another pattern to the first clusters ==
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check the first cluster ==
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check the second cluster (deduplicated) ==
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=4194304
 == TEST DATA_OFF CHECK ==
 == write pattern to first cluster ==
-- 
2.34.1

[PULL v2 22/22] tests: extend test 131 to cover availability of the write-zeroes

2023-09-21 Thread Denis V. Lunev

This patch contains test which minimally tests write-zeroes on top of
working discard.

The following checks are added:
* write 2 clusters, write-zero to the first allocated cluster
* write 2 cluster, write-zero to the half the first allocated cluster

Signed-off-by: Denis V. Lunev 
Reviewed-by: Alexander Ivanov 
---
 tests/qemu-iotests/131 | 21 +
 tests/qemu-iotests/131.out | 22 ++
 2 files changed, 43 insertions(+)

diff --git a/tests/qemu-iotests/131 b/tests/qemu-iotests/131
index 324008b3f6..3119100e78 100755
--- a/tests/qemu-iotests/131
+++ b/tests/qemu-iotests/131
@@ -105,6 +105,27 @@ _make_test_img $size
 { $QEMU_IO -c "read -P 0 0 $CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 | 
_filter_qemu_io | _filter_testdir
 { $QEMU_IO -c "read -P 0 $((CLUSTER_SIZE + CLUSTER_HALF_SIZE)) 
$CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
 
+echo "== check write-zeroes =="
+
+# Clear image
+_make_test_img $size
+
+{ $QEMU_IO -c "write -P 0x11 0 $CLUSTER_DBL_SIZE" "$TEST_IMG"; } 2>&1 | 
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "write -z 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io 
| _filter_testdir
+{ $QEMU_IMG map "$TEST_IMG"; } 2>&1 | _filter_qemu_img_map
+{ $QEMU_IO -c "read -P 0 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | 
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | 
_filter_qemu_io | _filter_testdir
+
+echo "== check cluster-partial write-zeroes =="
+
+# Clear image
+_make_test_img $size
+
+{ $QEMU_IO -c "write -P 0x11 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | 
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "write -z 0 $CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 | 
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0 0 $CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 | 
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0x11 $CLUSTER_HALF_SIZE $CLUSTER_HALF_SIZE" 
"$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+
 echo "== allocate with backing =="
 # Verify that allocating clusters works fine even when there is a backing 
image.
 # Regression test for a bug where we would pass a buffer read from the backing
diff --git a/tests/qemu-iotests/131.out b/tests/qemu-iotests/131.out
index 27df91ca97..86a2d2a49b 100644
--- a/tests/qemu-iotests/131.out
+++ b/tests/qemu-iotests/131.out
@@ -64,6 +64,28 @@ read 524288/524288 bytes at offset 0
 512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 524288/524288 bytes at offset 1572864
 512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check write-zeroes ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 2097152/2097152 bytes at offset 0
+2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset  Length  File
+0x100x10TEST_DIR/t.IMGFMT
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check cluster-partial write-zeroes ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 524288/524288 bytes at offset 0
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 524288/524288 bytes at offset 0
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 524288/524288 bytes at offset 524288
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 == allocate with backing ==
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
-- 
2.34.1

[PATCH v3 6/8] module: Use qemu_get_runtime_dir()

2023-09-21 Thread Akihiko Odaki

qemu_get_runtime_dir() is used to construct the path to module upgrades.

Signed-off-by: Akihiko Odaki 
---
 util/module.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/util/module.c b/util/module.c
index 32e263163c..580658edf4 100644
--- a/util/module.c
+++ b/util/module.c
@@ -242,7 +242,8 @@ int module_load(const char *prefix, const char *name, Error 
**errp)
 version_dir = g_strcanon(g_strdup(QEMU_PKGVERSION),
  G_CSET_A_2_Z G_CSET_a_2_z G_CSET_DIGITS "+-.~",
  '_');
-dirs[n_dirs++] = g_strdup_printf("/var/run/qemu/%s", version_dir);
+g_autofree char *run = qemu_get_runtime_dir();
+dirs[n_dirs++] = g_build_filename(run, "qemu", version_dir, NULL);
 #endif
 assert(n_dirs <= ARRAY_SIZE(dirs));
 
-- 
2.41.0

Re: [PATCH v2 01/10] qga: Remove platform GUID definitions

2023-09-21 Thread Akihiko Odaki


On 2022/11/17 18:45, Konstantin Kostiuk wrote:
Reviewed-by: Konstantin Kostiuk >


Will merge this patch in QGA series

On Thu, Nov 10, 2022 at 12:06 PM Akihiko Odaki > wrote:


GUID_DEVINTERFACE_DISK and GUID_DEVINTERFACE_STORAGEPORT are already
defined by MinGW-w64. They are not only unnecessary, but can lead to
duplicate definition errors at link time with some unknown condition.

Signed-off-by: Akihiko Odaki mailto:akihiko.od...@daynix.com>>
---
  qga/commands-win32.c | 7 ---
  1 file changed, 7 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index ec9f55b453..dde5d401bb 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -506,13 +506,6 @@ static GuestDiskBusType
find_bus_type(STORAGE_BUS_TYPE bus)
      return win2qemu[(int)bus];
  }

-DEFINE_GUID(GUID_DEVINTERFACE_DISK,
-        0x53f56307L, 0xb6bf, 0x11d0, 0x94, 0xf2,
-        0x00, 0xa0, 0xc9, 0x1e, 0xfb, 0x8b);
-DEFINE_GUID(GUID_DEVINTERFACE_STORAGEPORT,
-        0x2accfe60L, 0xc130, 0x11d2, 0xb0, 0x82,
-        0x00, 0xa0, 0xc9, 0x1e, 0xfb, 0x8b);
-
  static void get_pci_address_for_device(GuestPCIAddress *pci,
                                         HDEVINFO dev_info)
  {
-- 
2.38.1




Hi Konstantin,

This patch seems missed since then. Can you merge it?

Regards,
Akihiko Odaki

[PULL v2 06/22] parallels: return earlier from parallels_open() function on error

2023-09-21 Thread Denis V. Lunev

At the beginning of the function we can return immediately until we
really allocate s->header.

Signed-off-by: Denis V. Lunev 
Reviewed-by: Alexander Ivanov 
---
 block/parallels.c | 14 +-
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 12f38cf70b..bd26c8db63 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1090,7 +1090,7 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
 
 ret = bdrv_pread(bs->file, 0, sizeof(ph), &ph, 0);
 if (ret < 0) {
-goto fail;
+return ret;
 }
 
 bs->total_sectors = le64_to_cpu(ph.nb_sectors);
@@ -1110,13 +1110,11 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
 s->tracks = le32_to_cpu(ph.tracks);
 if (s->tracks == 0) {
 error_setg(errp, "Invalid image: Zero sectors per track");
-ret = -EINVAL;
-goto fail;
+return -EINVAL;
 }
 if (s->tracks > INT32_MAX/513) {
 error_setg(errp, "Invalid image: Too big cluster");
-ret = -EFBIG;
-goto fail;
+return -EFBIG;
 }
 s->prealloc_size = MAX(s->tracks, s->prealloc_size);
 s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
@@ -1124,16 +1122,14 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
 s->bat_size = le32_to_cpu(ph.bat_entries);
 if (s->bat_size > INT_MAX / sizeof(uint32_t)) {
 error_setg(errp, "Catalog too large");
-ret = -EFBIG;
-goto fail;
+return -EFBIG;
 }
 
 size = bat_entry_off(s->bat_size);
 s->header_size = ROUND_UP(size, bdrv_opt_mem_align(bs->file->bs));
 s->header = qemu_try_blockalign(bs->file->bs, s->header_size);
 if (s->header == NULL) {
-ret = -ENOMEM;
-goto fail;
+return -ENOMEM;
 }
 
 ret = bdrv_pread(bs->file, 0, s->header_size, s->header, 0);
-- 
2.34.1

[PULL v2 18/22] parallels: improve readability of allocate_clusters

2023-09-21 Thread Denis V. Lunev

Replace 'space' representing the amount of data to preallocate with
'bytes'.

Rationale:
* 'space' at each place is converted to bytes
* the unit is more close to the variable name

Signed-off-by: Denis V. Lunev 
Reviewed-by: Alexander Ivanov 
---
 block/parallels.c | 13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index ebcdff8736..a97fb8b506 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -279,7 +279,8 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
 first_free = find_first_zero_bit(s->used_bmap, s->used_bmap_size);
 if (first_free == s->used_bmap_size) {
 uint32_t new_usedsize;
-int64_t space = to_allocate * s->tracks + s->prealloc_size;
+int64_t bytes = to_allocate * s->cluster_size;
+bytes += s->prealloc_size * BDRV_SECTOR_SIZE;
 
 host_off = s->data_end * BDRV_SECTOR_SIZE;
 
@@ -289,8 +290,7 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
  * force the safer-but-slower fallocate.
  */
 if (s->prealloc_mode == PRL_PREALLOC_MODE_TRUNCATE) {
-ret = bdrv_co_truncate(bs->file,
-   (s->data_end + space) << BDRV_SECTOR_BITS,
+ret = bdrv_co_truncate(bs->file, host_off + bytes,
false, PREALLOC_MODE_OFF,
BDRV_REQ_ZERO_WRITE, NULL);
 if (ret == -ENOTSUP) {
@@ -298,16 +298,13 @@ allocate_clusters(BlockDriverState *bs, int64_t 
sector_num,
 }
 }
 if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
-ret = bdrv_co_pwrite_zeroes(bs->file,
-s->data_end << BDRV_SECTOR_BITS,
-space << BDRV_SECTOR_BITS, 0);
+ret = bdrv_co_pwrite_zeroes(bs->file, host_off, bytes, 0);
 }
 if (ret < 0) {
 return ret;
 }
 
-new_usedsize = s->used_bmap_size +
-   (space << BDRV_SECTOR_BITS) / s->cluster_size;
+new_usedsize = s->used_bmap_size + bytes / s->cluster_size;
 s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size,
   new_usedsize);
 s->used_bmap_size = new_usedsize;
-- 
2.34.1

[PULL v2 19/22] parallels: naive implementation of parallels_co_pdiscard

2023-09-21 Thread Denis V. Lunev

* Discarding with backing stores is not supported by the format.
* There is no buffering/queueing of the discard operation.
* Only operations aligned to the cluster are supported.

Signed-off-by: Denis V. Lunev 
Reviewed-by: Alexander Ivanov 
---
 block/parallels.c | 46 ++
 1 file changed, 46 insertions(+)

diff --git a/block/parallels.c b/block/parallels.c
index a97fb8b506..1808029f14 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -537,6 +537,51 @@ parallels_co_readv(BlockDriverState *bs, int64_t 
sector_num, int nb_sectors,
 return ret;
 }
 
+
+static int coroutine_fn GRAPH_RDLOCK
+parallels_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes)
+{
+int ret = 0;
+uint32_t cluster, count;
+BDRVParallelsState *s = bs->opaque;
+
+/*
+ * The image does not support ZERO mark inside the BAT, which means that
+ * stale data could be exposed from the backing file.
+ */
+if (bs->backing) {
+return -ENOTSUP;
+}
+
+if (!QEMU_IS_ALIGNED(offset, s->cluster_size)) {
+return -ENOTSUP;
+} else if (!QEMU_IS_ALIGNED(bytes, s->cluster_size)) {
+return -ENOTSUP;
+}
+
+cluster = offset / s->cluster_size;
+count = bytes / s->cluster_size;
+
+qemu_co_mutex_lock(&s->lock);
+for (; count > 0; cluster++, count--) {
+int64_t host_off = bat2sect(s, cluster) << BDRV_SECTOR_BITS;
+if (host_off == 0) {
+continue;
+}
+
+ret = bdrv_co_pdiscard(bs->file, host_off, s->cluster_size);
+if (ret < 0) {
+goto done;
+}
+
+parallels_set_bat_entry(s, cluster, 0);
+bitmap_clear(s->used_bmap, host_cluster_index(s, host_off), 1);
+}
+done:
+qemu_co_mutex_unlock(&s->lock);
+return ret;
+}
+
 static void parallels_check_unclean(BlockDriverState *bs,
 BdrvCheckResult *res,
 BdrvCheckMode fix)
@@ -1417,6 +1462,7 @@ static BlockDriver bdrv_parallels = {
 .bdrv_co_create = parallels_co_create,
 .bdrv_co_create_opts= parallels_co_create_opts,
 .bdrv_co_check  = parallels_co_check,
+.bdrv_co_pdiscard   = parallels_co_pdiscard,
 };
 
 static void bdrv_parallels_init(void)
-- 
2.34.1

[PULL v2 09/22] tests: ensure that image validation will not cure the corruption

2023-09-21 Thread Denis V. Lunev

Since
commit cfce1091d55322789582480798a891cbaf66924e
Author: Alexander Ivanov 
Date:   Tue Jul 18 12:44:29 2023 +0200
parallels: Image repairing in parallels_open()
there is a potential pit fall with calling
qemu-io -c "read"
The image is opened in read-write mode and thus could be potentially
repaired. This could ruin testing process.

The patch forces read-only opening for reads. In that case repairing
is impossible.

Signed-off-by: Denis V. Lunev 
Reviewed-by: Alexander Ivanov 
---
 tests/qemu-iotests/tests/parallels-checks | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/tests/parallels-checks 
b/tests/qemu-iotests/tests/parallels-checks
index a7a1b357b5..5917ee079d 100755
--- a/tests/qemu-iotests/tests/parallels-checks
+++ b/tests/qemu-iotests/tests/parallels-checks
@@ -91,7 +91,7 @@ file_size=`stat --printf="%s" "$TEST_IMG"`
 echo "file size: $file_size"
 
 echo "== check last cluster =="
-{ $QEMU_IO -c "read -P 0x11 $LAST_CLUSTER_OFF $CLUSTER_SIZE" "$TEST_IMG"; } 
2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IO -r -c "read -P 0x11 $LAST_CLUSTER_OFF $CLUSTER_SIZE" "$TEST_IMG"; } 
2>&1 | _filter_qemu_io | _filter_testdir
 
 # Clear image
 _make_test_img $SIZE
@@ -105,19 +105,20 @@ echo "== write another pattern to second cluster =="
 { $QEMU_IO -c "write -P 0x55 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 
| _filter_qemu_io | _filter_testdir
 
 echo "== check second cluster =="
-{ $QEMU_IO -c "read -P 0x55 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | 
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -r -c "read -P 0x55 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 
2>&1 | _filter_qemu_io | _filter_testdir
+
 
 echo "== corrupt image =="
 poke_file "$TEST_IMG" "$(($BAT_OFFSET + 4))" "\x01\x00\x00\x00"
 
 echo "== check second cluster =="
-{ $QEMU_IO -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | 
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 
2>&1 | _filter_qemu_io | _filter_testdir
 
 echo "== repair image =="
 _check_test_img -r all
 
 echo "== check second cluster =="
-{ $QEMU_IO -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | 
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 
2>&1 | _filter_qemu_io | _filter_testdir
 
 echo "== check first cluster on host =="
 printf "content: 0x%02x\n" `peek_file_le $TEST_IMG $(($CLUSTER_SIZE)) 1`
-- 
2.34.1

[PULL v2 08/22] parallels: create mark_used() helper which sets bit in used bitmap

2023-09-21 Thread Denis V. Lunev

This functionality is used twice already and next patch will add more
code with it.

Signed-off-by: Denis V. Lunev 
Reviewed-by: Alexander Ivanov 
---
 block/parallels.c | 34 +-
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index af3b4894d7..66c86d87e3 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -178,6 +178,21 @@ static void parallels_set_bat_entry(BDRVParallelsState *s,
 bitmap_set(s->bat_dirty_bmap, bat_entry_off(index) / s->bat_dirty_block, 
1);
 }
 
+static int mark_used(BlockDriverState *bs,
+ unsigned long *bitmap, uint32_t bitmap_size, int64_t off)
+{
+BDRVParallelsState *s = bs->opaque;
+uint32_t cluster_index = host_cluster_index(s, off);
+if (cluster_index >= bitmap_size) {
+return -E2BIG;
+}
+if (test_bit(cluster_index, bitmap)) {
+return -EBUSY;
+}
+bitmap_set(bitmap, cluster_index, 1);
+return 0;
+}
+
 static int64_t coroutine_fn GRAPH_RDLOCK
 allocate_clusters(BlockDriverState *bs, int64_t sector_num,
   int nb_sectors, int *pnum)
@@ -621,7 +636,7 @@ parallels_check_duplicate(BlockDriverState *bs, 
BdrvCheckResult *res,
 BDRVParallelsState *s = bs->opaque;
 int64_t host_off, host_sector, guest_sector;
 unsigned long *bitmap;
-uint32_t i, bitmap_size, cluster_index, bat_entry;
+uint32_t i, bitmap_size, bat_entry;
 int n, ret = 0;
 uint64_t *buf = NULL;
 bool fixed = false;
@@ -655,10 +670,9 @@ parallels_check_duplicate(BlockDriverState *bs, 
BdrvCheckResult *res,
 continue;
 }
 
-cluster_index = host_cluster_index(s, host_off);
-assert(cluster_index < bitmap_size);
-if (!test_bit(cluster_index, bitmap)) {
-bitmap_set(bitmap, cluster_index, 1);
+ret = mark_used(bs, bitmap, bitmap_size, host_off);
+assert(ret != -E2BIG);
+if (ret == 0) {
 continue;
 }
 
@@ -713,11 +727,13 @@ parallels_check_duplicate(BlockDriverState *bs, 
BdrvCheckResult *res,
  * consistent for the new allocated clusters too.
  *
  * Note, clusters allocated outside the current image are not
- * considered, and the bitmap size doesn't change.
+ * considered, and the bitmap size doesn't change. This specifically
+ * means that -E2BIG is OK.
  */
-cluster_index = host_cluster_index(s, host_off);
-if (cluster_index < bitmap_size) {
-bitmap_set(bitmap, cluster_index, 1);
+ret = mark_used(bs, bitmap, bitmap_size, host_off);
+if (ret == -EBUSY) {
+res->check_errors++;
+goto out_repair_bat;
 }
 
 fixed = true;
-- 
2.34.1

[PULL v2 05/22] parallels: return earler in fail_format branch in parallels_open()

2023-09-21 Thread Denis V. Lunev

We do not need to perform any deallocation/cleanup if wrong format is
detected.

Signed-off-by: Denis V. Lunev 
Reviewed-by: Alexander Ivanov 
---
 block/parallels.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/parallels.c b/block/parallels.c
index ae006e7fc7..12f38cf70b 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1232,7 +1232,8 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
 
 fail_format:
 error_setg(errp, "Image not in Parallels format");
-ret = -EINVAL;
+return -EINVAL;
+
 fail:
 /*
  * "s" object was allocated by g_malloc0 so we can safely
-- 
2.34.1

[PULL v2 01/22] parallels: fix formatting in bdrv_parallels initialization

2023-09-21 Thread Denis V. Lunev

Old code is ugly and contains tabulations. There are no functional
changes in this patch.

Signed-off-by: Denis V. Lunev 
Reviewed-by: Alexander Ivanov 
---
 block/parallels.c | 36 +++-
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 48c32d6821..2ebd8e1301 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1249,23 +1249,25 @@ static void parallels_close(BlockDriverState *bs)
 }
 
 static BlockDriver bdrv_parallels = {
-.format_name   = "parallels",
-.instance_size = sizeof(BDRVParallelsState),
-.bdrv_probe= parallels_probe,
-.bdrv_open = parallels_open,
-.bdrv_close= parallels_close,
-.bdrv_child_perm  = bdrv_default_perms,
-.bdrv_co_block_status = parallels_co_block_status,
-.bdrv_has_zero_init   = bdrv_has_zero_init_1,
-.bdrv_co_flush_to_os  = parallels_co_flush_to_os,
-.bdrv_co_readv  = parallels_co_readv,
-.bdrv_co_writev = parallels_co_writev,
-.is_format  = true,
-.supports_backing = true,
-.bdrv_co_create  = parallels_co_create,
-.bdrv_co_create_opts = parallels_co_create_opts,
-.bdrv_co_check  = parallels_co_check,
-.create_opts= ¶llels_create_opts,
+.format_name= "parallels",
+.instance_size  = sizeof(BDRVParallelsState),
+.create_opts= ¶llels_create_opts,
+.is_format  = true,
+.supports_backing   = true,
+
+.bdrv_has_zero_init = bdrv_has_zero_init_1,
+
+.bdrv_probe = parallels_probe,
+.bdrv_open  = parallels_open,
+.bdrv_close = parallels_close,
+.bdrv_child_perm= bdrv_default_perms,
+.bdrv_co_block_status   = parallels_co_block_status,
+.bdrv_co_flush_to_os= parallels_co_flush_to_os,
+.bdrv_co_readv  = parallels_co_readv,
+.bdrv_co_writev = parallels_co_writev,
+.bdrv_co_create = parallels_co_create,
+.bdrv_co_create_opts= parallels_co_create_opts,
+.bdrv_co_check  = parallels_co_check,
 };
 
 static void bdrv_parallels_init(void)
-- 
2.34.1

[PATCH v3 5/8] scsi: Use qemu_get_runtime_dir()

2023-09-21 Thread Akihiko Odaki

qemu_get_runtime_dir() is used to construct the default paths.

Signed-off-by: Akihiko Odaki 
---
 scsi/qemu-pr-helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index c6c6347e9b..507f23357f 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -77,10 +77,10 @@ static int gid = -1;
 
 static void compute_default_paths(void)
 {
-g_autofree char *state = qemu_get_local_state_dir();
+g_autofree char *run = qemu_get_runtime_dir();
 
-socket_path = g_build_filename(state, "run", "qemu-pr-helper.sock", NULL);
-pidfile = g_build_filename(state, "run", "qemu-pr-helper.pid", NULL);
+socket_path = g_build_filename(run, "qemu-pr-helper.sock", NULL);
+pidfile = g_build_filename(run, "qemu-pr-helper.pid", NULL);
 }
 
 static void usage(const char *name)
-- 
2.41.0

[PULL v2 04/22] parallels: invent parallels_opts_prealloc() helper to parse prealloc opts

2023-09-21 Thread Denis V. Lunev

This patch creates above mentioned helper and moves its usage to the
beginning of parallels_open(). This simplifies parallels_open() a bit.

The patch also ensures that we store prealloc_size on block driver state
always in sectors. This makes code cleaner and avoids wrong opinion at
the assignment that the value is in bytes.

Signed-off-by: Denis V. Lunev 
Reviewed-by: Alexander Ivanov 
---
 block/parallels.c | 72 +--
 1 file changed, 44 insertions(+), 28 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index af7be427c9..ae006e7fc7 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1025,6 +1025,44 @@ static int parallels_update_header(BlockDriverState *bs)
 return bdrv_pwrite_sync(bs->file, 0, size, s->header, 0);
 }
 
+
+static int parallels_opts_prealloc(BlockDriverState *bs, QDict *options,
+   Error **errp)
+{
+int err;
+char *buf;
+int64_t bytes;
+BDRVParallelsState *s = bs->opaque;
+Error *local_err = NULL;
+QemuOpts *opts = qemu_opts_create(¶llels_runtime_opts, NULL, 0, errp);
+if (!opts) {
+return -ENOMEM;
+}
+
+err = -EINVAL;
+if (!qemu_opts_absorb_qdict(opts, options, errp)) {
+goto done;
+}
+
+bytes = qemu_opt_get_size_del(opts, PARALLELS_OPT_PREALLOC_SIZE, 0);
+s->prealloc_size = bytes >> BDRV_SECTOR_BITS;
+buf = qemu_opt_get_del(opts, PARALLELS_OPT_PREALLOC_MODE);
+/* prealloc_mode can be downgraded later during allocate_clusters */
+s->prealloc_mode = qapi_enum_parse(&prealloc_mode_lookup, buf,
+   PRL_PREALLOC_MODE_FALLOCATE,
+   &local_err);
+g_free(buf);
+if (local_err != NULL) {
+error_propagate(errp, local_err);
+goto done;
+}
+err = 0;
+
+done:
+qemu_opts_del(opts);
+return err;
+}
+
 static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
   Error **errp)
 {
@@ -1033,11 +1071,13 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
 int ret, size, i;
 int64_t file_nb_sectors, sector;
 uint32_t data_start;
-QemuOpts *opts = NULL;
-Error *local_err = NULL;
-char *buf;
 bool data_off_is_correct;
 
+ret = parallels_opts_prealloc(bs, options, errp);
+if (ret < 0) {
+return ret;
+}
+
 ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
 if (ret < 0) {
 return ret;
@@ -1078,6 +1118,7 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
 ret = -EFBIG;
 goto fail;
 }
+s->prealloc_size = MAX(s->tracks, s->prealloc_size);
 s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
 
 s->bat_size = le32_to_cpu(ph.bat_entries);
@@ -1117,29 +1158,6 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
 s->header_size = size;
 }
 
-opts = qemu_opts_create(¶llels_runtime_opts, NULL, 0, errp);
-if (!opts) {
-goto fail_options;
-}
-
-if (!qemu_opts_absorb_qdict(opts, options, errp)) {
-goto fail_options;
-}
-
-s->prealloc_size =
-qemu_opt_get_size_del(opts, PARALLELS_OPT_PREALLOC_SIZE, 0);
-s->prealloc_size = MAX(s->tracks, s->prealloc_size >> BDRV_SECTOR_BITS);
-buf = qemu_opt_get_del(opts, PARALLELS_OPT_PREALLOC_MODE);
-/* prealloc_mode can be downgraded later during allocate_clusters */
-s->prealloc_mode = qapi_enum_parse(&prealloc_mode_lookup, buf,
-   PRL_PREALLOC_MODE_FALLOCATE,
-   &local_err);
-g_free(buf);
-if (local_err != NULL) {
-error_propagate(errp, local_err);
-goto fail_options;
-}
-
 if (ph.ext_off) {
 if (flags & BDRV_O_RDWR) {
 /*
@@ -1214,10 +1232,8 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
 
 fail_format:
 error_setg(errp, "Image not in Parallels format");
-fail_options:
 ret = -EINVAL;
 fail:
-qemu_opts_del(opts);
 /*
  * "s" object was allocated by g_malloc0 so we can safely
  * try to free its fields even they were not allocated.
-- 
2.34.1

[PULL v2 14/22] tests: test self-cure of parallels image with duplicated clusters

2023-09-21 Thread Denis V. Lunev

The test is quite similar with the original one for duplicated clusters.
There is the only difference in the operation which should fix the
image.

Signed-off-by: Denis V. Lunev 
Reviewed-by: Alexander Ivanov 
---
 tests/qemu-iotests/tests/parallels-checks | 36 +++
 tests/qemu-iotests/tests/parallels-checks.out | 31 
 2 files changed, 67 insertions(+)

diff --git a/tests/qemu-iotests/tests/parallels-checks 
b/tests/qemu-iotests/tests/parallels-checks
index df99558486..b281246a42 100755
--- a/tests/qemu-iotests/tests/parallels-checks
+++ b/tests/qemu-iotests/tests/parallels-checks
@@ -135,6 +135,42 @@ echo "== check the second cluster (deduplicated) =="
 # Clear image
 _make_test_img $SIZE
 
+echo "== TEST DUPLICATION SELF-CURE =="
+
+echo "== write pattern to whole image =="
+{ $QEMU_IO -c "write -P 0x11 0 $SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | 
_filter_testdir
+
+echo "== write another pattern to second cluster =="
+{ $QEMU_IO -c "write -P 0x55 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 
| _filter_qemu_io | _filter_testdir
+
+echo "== check second cluster =="
+{ $QEMU_IO -r -c "read -P 0x55 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 
2>&1 | _filter_qemu_io | _filter_testdir
+
+
+echo "== corrupt image =="
+poke_file "$TEST_IMG" "$(($BAT_OFFSET + 4))" "\x01\x00\x00\x00"
+
+echo "== check second cluster =="
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 
2>&1 | _filter_qemu_io | _filter_testdir
+
+echo "== check the first cluster with self-repair =="
+{ $QEMU_IO -c "read -P 0x11 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | 
_filter_qemu_io | _filter_testdir
+
+echo "== check second cluster =="
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 
2>&1 | _filter_qemu_io | _filter_testdir
+
+echo "== write another pattern to the first clusters =="
+{ $QEMU_IO -c "write -P 0x66 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | 
_filter_qemu_io | _filter_testdir
+
+echo "== check the first cluster =="
+{ $QEMU_IO -r -c "read -P 0x66 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | 
_filter_qemu_io | _filter_testdir
+
+echo "== check the second cluster (deduplicated) =="
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 
2>&1 | _filter_qemu_io | _filter_testdir
+
+# Clear image
+_make_test_img $SIZE
+
 echo "== TEST DATA_OFF CHECK =="
 
 echo "== write pattern to first cluster =="
diff --git a/tests/qemu-iotests/tests/parallels-checks.out 
b/tests/qemu-iotests/tests/parallels-checks.out
index 1325d2b611..9793423111 100644
--- a/tests/qemu-iotests/tests/parallels-checks.out
+++ b/tests/qemu-iotests/tests/parallels-checks.out
@@ -71,6 +71,37 @@ read 1048576/1048576 bytes at offset 0
 read 1048576/1048576 bytes at offset 1048576
 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=4194304
+== TEST DUPLICATION SELF-CURE ==
+== write pattern to whole image ==
+wrote 4194304/4194304 bytes at offset 0
+4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== write another pattern to second cluster ==
+wrote 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check second cluster ==
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== corrupt image ==
+== check second cluster ==
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check the first cluster with self-repair ==
+Repairing duplicate offset in BAT entry 1
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check second cluster ==
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== write another pattern to the first clusters ==
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check the first cluster ==
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check the second cluster (deduplicated) ==
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=4194304
 == TEST DATA_OFF CHECK ==
 == write pattern to first cluster ==
 wrote 1048576/1048576 bytes at offset 0
-- 
2.34.1

[PATCH v3 7/8] util: Remove qemu_get_local_state_dir()

2023-09-21 Thread Akihiko Odaki

There are no users of the function anymore.

Signed-off-by: Akihiko Odaki 
---
 include/qemu/osdep.h |  8 
 util/oslib-posix.c   |  6 --
 util/oslib-win32.c   | 10 --
 3 files changed, 24 deletions(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index bb857c910f..cc585447ef 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -628,14 +628,6 @@ ssize_t qemu_write_full(int fd, const void *buf, size_t 
count)
 
 void qemu_set_cloexec(int fd);
 
-/* Return a dynamically allocated directory path that is appropriate for 
storing
- * local state.
- *
- * The caller is responsible for releasing the value returned with g_free()
- * after use.
- */
-char *qemu_get_local_state_dir(void);
-
 /**
  * qemu_get_runtime_dir:
  *
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 0c82717be5..f3054ad2cd 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -267,12 +267,6 @@ int qemu_socketpair(int domain, int type, int protocol, 
int sv[2])
 return ret;
 }
 
-char *
-qemu_get_local_state_dir(void)
-{
-return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR);
-}
-
 char *
 qemu_get_runtime_dir(void)
 {
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 38df7b57b5..f93c3bff8e 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -229,16 +229,6 @@ int qemu_get_thread_id(void)
 return GetCurrentThreadId();
 }
 
-char *
-qemu_get_local_state_dir(void)
-{
-const char * const *data_dirs = g_get_system_data_dirs();
-
-g_assert(data_dirs && data_dirs[0]);
-
-return g_strdup(data_dirs[0]);
-}
-
 char *
 qemu_get_runtime_dir(void)
 {
-- 
2.41.0

[PATCH v3 3/8] contrib/rdmacm-mux: Use qemu_get_runtime_dir()

2023-09-21 Thread Akihiko Odaki

qemu_get_runtime_dir() is used to construct the default Unix socket
path.

Signed-off-by: Akihiko Odaki 
---
 contrib/rdmacm-mux/main.c  | 22 ++
 contrib/rdmacm-mux/meson.build |  2 +-
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/contrib/rdmacm-mux/main.c b/contrib/rdmacm-mux/main.c
index 771ca01e03..00c14031ca 100644
--- a/contrib/rdmacm-mux/main.c
+++ b/contrib/rdmacm-mux/main.c
@@ -14,6 +14,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
 #include 
 #include 
 #include 
@@ -40,8 +41,6 @@
 #define CM_REQ_DGID_POS  80
 #define CM_SIDR_REQ_DGID_POS 44
 
-/* The below can be override by command line parameter */
-#define UNIX_SOCKET_PATH "/var/run/rdmacm-mux"
 /* Has format %s-%s-%d" -- */
 #define SOCKET_PATH_MAX (PATH_MAX - NAME_MAX - sizeof(int) - 2)
 #define RDMA_PORT_NUM 1
@@ -77,7 +76,13 @@ typedef struct RdmaCmServer {
 
 static RdmaCMServer server = {0};
 
-static void usage(const char *progname)
+static char *get_default_unix_socket_path(void)
+{
+g_autofree char *run = qemu_get_runtime_dir();
+return g_build_filename(run, "rdmacm-mux", NULL);
+}
+
+static void usage(const char *progname, const char *default_unix_socket_path)
 {
 printf("Usage: %s [OPTION]...\n"
"Start a RDMA-CM multiplexer\n"
@@ -86,7 +91,7 @@ static void usage(const char *progname)
"\t-d rdma-device-name   Name of RDMA device to register with\n"
"\t-s unix-socket-path   Path to unix socket to listen on (default 
%s)\n"
"\t-p rdma-device-port   Port number of RDMA device to register 
with (default %d)\n",
-   progname, UNIX_SOCKET_PATH, RDMA_PORT_NUM);
+   progname, default_unix_socket_path, RDMA_PORT_NUM);
 }
 
 static void help(const char *progname)
@@ -97,16 +102,16 @@ static void help(const char *progname)
 static void parse_args(int argc, char *argv[])
 {
 int c;
-char unix_socket_path[SOCKET_PATH_MAX];
+g_autofree char *default_unix_socket_path = get_default_unix_socket_path();
+char *unix_socket_path = default_unix_socket_path;
 
 strcpy(server.args.rdma_dev_name, "");
-strcpy(unix_socket_path, UNIX_SOCKET_PATH);
 server.args.rdma_port_num = RDMA_PORT_NUM;
 
 while ((c = getopt(argc, argv, "hs:d:p:")) != -1) {
 switch (c) {
 case 'h':
-usage(argv[0]);
+usage(argv[0], default_unix_socket_path);
 exit(0);
 
 case 'd':
@@ -115,7 +120,7 @@ static void parse_args(int argc, char *argv[])
 
 case 's':
 /* This is temporary, final name will build below */
-strncpy(unix_socket_path, optarg, SOCKET_PATH_MAX - 1);
+unix_socket_path = optarg;
 break;
 
 case 'p':
@@ -811,6 +816,7 @@ int main(int argc, char *argv[])
 {
 int rc;
 
+qemu_init_exec_dir(argv[0]);
 memset(&server, 0, sizeof(server));
 
 parse_args(argc, argv);
diff --git a/contrib/rdmacm-mux/meson.build b/contrib/rdmacm-mux/meson.build
index 36c9c89630..59f60f9cac 100644
--- a/contrib/rdmacm-mux/meson.build
+++ b/contrib/rdmacm-mux/meson.build
@@ -1,7 +1,7 @@
 if have_pvrdma
   # FIXME: broken on big endian architectures
   executable('rdmacm-mux', files('main.c'), genh,
- dependencies: [glib, libumad],
+ dependencies: [glib, libumad, qemuutil],
  build_by_default: false,
  install: false)
 endif
-- 
2.41.0

[PATCH v3 1/8] util: Introduce qemu_get_runtime_dir()

2023-09-21 Thread Akihiko Odaki

qemu_get_runtime_dir() returns a dynamically allocated directory path
that is appropriate for storing runtime files. It corresponds to "run"
directory in Unix.

With a tree-wide search, it was found that there are several cases
where such a functionality is implemented so let's have one as a common
utlity function.

A notable feature of qemu_get_runtime_dir() is that it uses
$XDG_RUNTIME_DIR if available. While the function is often called by
executables which requires root privileges, it is still possible that
they are called from a user without privilege to write the system
runtime directory. In fact, I decided to write this patch when I ran
virtiofsd in a Linux namespace created by a normal user and realized
it tries to write the system runtime directory, not writable in this
case. $XDG_RUNTIME_DIR should provide a writable directory in such
cases.

This function does not use qemu_get_local_state_dir() or its logic
for Windows. Actually the implementation of qemu_get_local_state_dir()
for Windows seems not right as it calls g_get_system_data_dirs(),
which refers to $XDG_DATA_DIRS. In Unix terminology, it is basically
"/usr/share", not "/var", which qemu_get_local_state_dir() is intended
to provide. Instead, this function try to use the following in order:
- $XDG_RUNTIME_DIR
- LocalAppData folder
- get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")

This function does not use g_get_user_runtime_dir() either as it
falls back to g_get_user_cache_dir() when $XDG_DATA_DIRS is not
available. In the case, we rather use:
get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")

Signed-off-by: Akihiko Odaki 
---
 include/qemu/osdep.h | 12 
 util/oslib-posix.c   | 11 +++
 util/oslib-win32.c   | 26 ++
 3 files changed, 49 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 2897720fac..bb857c910f 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -636,6 +636,18 @@ void qemu_set_cloexec(int fd);
  */
 char *qemu_get_local_state_dir(void);
 
+/**
+ * qemu_get_runtime_dir:
+ *
+ * Return a dynamically allocated directory path that is appropriate for 
storing
+ * runtime files. It corresponds to "run" directory in Unix, and uses
+ * $XDG_RUNTIME_DIR if available.
+ *
+ * The caller is responsible for releasing the value returned with g_free()
+ * after use.
+ */
+char *qemu_get_runtime_dir(void);
+
 /**
  * qemu_getauxval:
  * @type: the auxiliary vector key to lookup
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index e86fd64e09..0c82717be5 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -273,6 +273,17 @@ qemu_get_local_state_dir(void)
 return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR);
 }
 
+char *
+qemu_get_runtime_dir(void)
+{
+char *env = getenv("XDG_RUNTIME_DIR");
+if (env) {
+return g_strdup(env);
+}
+
+return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run");
+}
+
 void qemu_set_tty_echo(int fd, bool echo)
 {
 struct termios tty;
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 19a0ea7fbe..38df7b57b5 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -27,6 +27,8 @@
  */
 
 #include "qemu/osdep.h"
+#include 
+#include 
 #include 
 #include "qapi/error.h"
 #include "qemu/main-loop.h"
@@ -237,6 +239,30 @@ qemu_get_local_state_dir(void)
 return g_strdup(data_dirs[0]);
 }
 
+char *
+qemu_get_runtime_dir(void)
+{
+size_t size = GetEnvironmentVariableA("XDG_RUNTIME_DIR", NULL, 0);
+if (size) {
+char *env = g_malloc(size);
+GetEnvironmentVariableA("XDG_RUNTIME_DIR", env, size);
+return env;
+}
+
+PWSTR wpath;
+const wchar_t *cwpath;
+if (!SHGetKnownFolderPath(&FOLDERID_LocalAppData, KF_FLAG_DEFAULT, NULL, 
&wpath)) {
+cwpath = wpath;
+size = wcsrtombs(NULL, &cwpath, 0, &(mbstate_t){0}) + 1;
+char *path = g_malloc(size);
+wcsrtombs(path, &cwpath, size, &(mbstate_t){0});
+CoTaskMemFree(wpath);
+return path;
+}
+
+return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run");
+}
+
 void qemu_set_tty_echo(int fd, bool echo)
 {
 HANDLE handle = (HANDLE)_get_osfhandle(fd);
-- 
2.41.0

[PATCH v3 8/8] spice-app: Use qemu_get_runtime_dir()

2023-09-21 Thread Akihiko Odaki

qemu_get_runtime_dir() provides QEMU-specific fallback of runtime
directory.

Signed-off-by: Akihiko Odaki 
---
 ui/spice-app.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ui/spice-app.c b/ui/spice-app.c
index 405fb7f9f5..f6c2343213 100644
--- a/ui/spice-app.c
+++ b/ui/spice-app.c
@@ -151,8 +151,8 @@ static void spice_app_display_early_init(DisplayOptions 
*opts)
 atexit(spice_app_atexit);
 
 if (qemu_name) {
-app_dir = g_build_filename(g_get_user_runtime_dir(),
-   "qemu", qemu_name, NULL);
+g_autofree char *run = qemu_get_runtime_dir();
+app_dir = g_build_filename(run, "qemu", qemu_name, NULL);
 if (g_mkdir_with_parents(app_dir, S_IRWXU) < -1) {
 error_report("Failed to create directory %s: %s",
  app_dir, strerror(errno));
-- 
2.41.0

[PATCH v3 4/8] qga: Use qemu_get_runtime_dir()

2023-09-21 Thread Akihiko Odaki

qemu_get_runtime_dir() is used to construct the default state directory.

Signed-off-by: Akihiko Odaki 
---
 qga/main.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index 8668b9f3d3..145ee02df3 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -45,12 +45,11 @@
 #define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent.0"
 #endif /* CONFIG_BSD */
 #define QGA_SERIAL_PATH_DEFAULT "/dev/ttyS0"
-#define QGA_STATE_RELATIVE_DIR  "run"
 #else
 #define QGA_VIRTIO_PATH_DEFAULT ".\\Global\\org.qemu.guest_agent.0"
-#define QGA_STATE_RELATIVE_DIR  "qemu-ga"
 #define QGA_SERIAL_PATH_DEFAULT "COM1"
 #endif
+#define QGA_STATE_RELATIVE_DIR  "qemu-ga"
 #ifdef CONFIG_FSFREEZE
 #define QGA_FSFREEZE_HOOK_DEFAULT CONFIG_QEMU_CONFDIR "/fsfreeze-hook"
 #endif
@@ -129,12 +128,12 @@ static void stop_agent(GAState *s, bool requested);
 static void
 init_dfl_pathnames(void)
 {
-g_autofree char *state = qemu_get_local_state_dir();
+g_autofree char *run = qemu_get_runtime_dir();
 
 g_assert(dfl_pathnames.state_dir == NULL);
 g_assert(dfl_pathnames.pidfile == NULL);
-dfl_pathnames.state_dir = g_build_filename(state, QGA_STATE_RELATIVE_DIR, 
NULL);
-dfl_pathnames.pidfile = g_build_filename(state, QGA_STATE_RELATIVE_DIR, 
"qemu-ga.pid", NULL);
+dfl_pathnames.state_dir = g_build_filename(run, QGA_STATE_RELATIVE_DIR, 
NULL);
+dfl_pathnames.pidfile = g_build_filename(run, QGA_STATE_RELATIVE_DIR, 
"qemu-ga.pid", NULL);
 }
 
 static void quit_handler(int sig)
-- 
2.41.0

[PULL v2 03/22] parallels: fix memory leak in parallels_open()

2023-09-21 Thread Denis V. Lunev

We should free opts allocated through qemu_opts_create() at the end.

Signed-off-by: Denis V. Lunev 
Reviewed-by: Alexander Ivanov 
---
 block/parallels.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/parallels.c b/block/parallels.c
index 428f72de1c..af7be427c9 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1217,6 +1217,7 @@ fail_format:
 fail_options:
 ret = -EINVAL;
 fail:
+qemu_opts_del(opts);
 /*
  * "s" object was allocated by g_malloc0 so we can safely
  * try to free its fields even they were not allocated.
-- 
2.34.1

[PULL v2 00/22] implement discard operation for Parallels images

2023-09-21 Thread Denis V. Lunev

The following changes since commit 4907644841e3200aea6475c0f72d3d987e9f3d93:

  Merge tag 'mem-2023-09-19' of https://github.com/davidhildenbrand/qemu into 
staging (2023-09-19 13:22:19 -0400)

are available in the Git repository at:

  https://src.openvz.org/scm/~den/qemu.git tags/pull-parallels-2023-09-20-v2

for you to fetch changes up to 1dba99e34d1bcb3c0de69c4270f9587b01e225fe:

  tests: extend test 131 to cover availability of the write-zeroes (2023-09-21 
08:49:28 +0200)


Parallels format driver:
* regular calculation of cluster used bitmap of the image file
* cluster allocation on the base of that bitmap (effectively allocation of
  new clusters could be done inside the image if that offset space is unused)
* support of DISCARD and WRITE_ZEROES operations
* image check bugfixes
* unit tests fixes
* unit tests covering new functionality


Changes from v1:
* patch 12: bdrv_co_getlength -> bdrv_getlength in parallels_fill_used_bitmap
  as called from open()
* patch 19: added GRAPH_RDLOCK specifier for parallels_co_pdiscard
* patch 21: added GRAPH_RDLOCK specifier for parallels_co_pwrite_zeroes


Denis V. Lunev (22):
  parallels: fix formatting in bdrv_parallels initialization
  parallels: mark driver as supporting CBT
  parallels: fix memory leak in parallels_open()
  parallels: invent parallels_opts_prealloc() helper to parse prealloc opts
  parallels: return earler in fail_format branch in parallels_open()
  parallels: return earlier from parallels_open() function on error
  parallels: refactor path when we need to re-check image in parallels_open
  parallels: create mark_used() helper which sets bit in used bitmap
  tests: ensure that image validation will not cure the corruption
  parallels: fix broken parallels_check_data_off()
  parallels: add test which will validate data_off fixes through repair
  parallels: collect bitmap of used clusters at open
  tests: fix broken deduplication check in parallels format test
  tests: test self-cure of parallels image with duplicated clusters
  parallels: accept multiple clusters in mark_used()
  parallels: update used bitmap in allocate_cluster
  parallels: naive implementation of allocate_clusters with used bitmap
  parallels: improve readability of allocate_clusters
  parallels: naive implementation of parallels_co_pdiscard
  tests: extend test 131 to cover availability of the discard operation
  parallels: naive implementation of parallels_co_pwrite_zeroes
  tests: extend test 131 to cover availability of the write-zeroes

 block/parallels.c | 389 --
 block/parallels.h |   3 +
 tests/qemu-iotests/131|  52 
 tests/qemu-iotests/131.out|  60 
 tests/qemu-iotests/tests/parallels-checks |  76 -
 tests/qemu-iotests/tests/parallels-checks.out |  65 -
 6 files changed, 544 insertions(+), 101 deletions(-)

-- 
2.34.1

[PULL v2 02/22] parallels: mark driver as supporting CBT

2023-09-21 Thread Denis V. Lunev

Parallels driver indeed support Parallels Dirty Bitmap Feature in
read-only mode. The patch adds bdrv_supports_persistent_dirty_bitmap()
callback which always return 1 to indicate that.

This will allow to copy CBT from Parallels image with qemu-img.

Note: read-write support is signalled through
bdrv_co_can_store_new_dirty_bitmap() and is different.

Signed-off-by: Denis V. Lunev 
Reviewed-by: Alexander Ivanov 
---
 block/parallels.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/block/parallels.c b/block/parallels.c
index 2ebd8e1301..428f72de1c 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1248,6 +1248,11 @@ static void parallels_close(BlockDriverState *bs)
 error_free(s->migration_blocker);
 }
 
+static bool parallels_is_support_dirty_bitmaps(BlockDriverState *bs)
+{
+return 1;
+}
+
 static BlockDriver bdrv_parallels = {
 .format_name= "parallels",
 .instance_size  = sizeof(BDRVParallelsState),
@@ -1256,6 +1261,7 @@ static BlockDriver bdrv_parallels = {
 .supports_backing   = true,
 
 .bdrv_has_zero_init = bdrv_has_zero_init_1,
+.bdrv_supports_persistent_dirty_bitmap = 
parallels_is_support_dirty_bitmaps,
 
 .bdrv_probe = parallels_probe,
 .bdrv_open  = parallels_open,
-- 
2.34.1

[PATCH v3 0/8] util: Introduce qemu_get_runtime_dir()

2023-09-21 Thread Akihiko Odaki

qemu_get_runtime_dir() returns a dynamically allocated directory path
that is appropriate for storing runtime files. It corresponds to "run"
directory in Unix.

With a tree-wide search, it was found that there are several cases
where such a functionality is implemented so let's have one as a common
utlity function.

A notable feature of qemu_get_runtime_dir() is that it uses
$XDG_RUNTIME_DIR if available. While the function is often called by
executables which requires root privileges, it is still possible that
they are called from a user without privilege to write the system
runtime directory. In fact, I decided to write this patch when I ran
virtiofsd in a Linux namespace created by a normal user and realized
it tries to write the system runtime directory, not writable in this
case. $XDG_RUNTIME_DIR should provide a writable directory in such
cases.

This function does not use qemu_get_local_state_dir() or its logic
for Windows. Actually the implementation of qemu_get_local_state_dir()
for Windows seems not right as it calls g_get_system_data_dirs(),
which refers to $XDG_DATA_DIRS. In Unix terminology, it is basically
"/usr/share", not "/var", which qemu_get_local_state_dir() is intended
to provide. Instead, this function try to use the following in order:
- $XDG_RUNTIME_DIR
- LocalAppData folder
- get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")

This function does not use g_get_user_runtime_dir() either as it
falls back to g_get_user_cache_dir() when $XDG_DATA_DIRS is not
available. In the case, we rather use:
get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")

V2 -> V3:
  Rebase to the current master.
  Dropped patch "qga: Remove platform GUID definitions" since it is
  irrelevant.

V1 -> V2:
  Rebased to the current master since Patchew complains.

Akihiko Odaki (8):
  util: Introduce qemu_get_runtime_dir()
  ivshmem-server: Use qemu_get_runtime_dir()
  contrib/rdmacm-mux: Use qemu_get_runtime_dir()
  qga: Use qemu_get_runtime_dir()
  scsi: Use qemu_get_runtime_dir()
  module: Use qemu_get_runtime_dir()
  util: Remove qemu_get_local_state_dir()
  spice-app: Use qemu_get_runtime_dir()

 include/qemu/osdep.h   | 10 +++---
 contrib/ivshmem-server/main.c  | 20 
 contrib/rdmacm-mux/main.c  | 22 ++
 qga/main.c |  9 -
 scsi/qemu-pr-helper.c  |  6 +++---
 ui/spice-app.c |  4 ++--
 util/module.c  |  3 ++-
 util/oslib-posix.c |  9 +++--
 util/oslib-win32.c | 24 
 contrib/rdmacm-mux/meson.build |  2 +-
 10 files changed, 76 insertions(+), 33 deletions(-)

-- 
2.41.0

Re: [PATCH v6 1/3] hw/i2c: add smbus pec utility function

2023-09-21 Thread Andrew Jeffery

On Thu, 2023-09-14 at 11:53 +0200, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> Add i2c_smbus_pec() to calculate the SMBus Packet Error Code for a
> message.
> 
> Reviewed-by: Jonathan Cameron 
> Signed-off-by: Klaus Jensen 

It at least looks a lot like the linux implementation :)

Reviewed-by: Andrew Jeffery 

> ---
>  hw/i2c/smbus_master.c | 26 ++
>  include/hw/i2c/smbus_master.h |  2 ++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/hw/i2c/smbus_master.c b/hw/i2c/smbus_master.c
> index 6a53c34e70b7..01a8e4700222 100644
> --- a/hw/i2c/smbus_master.c
> +++ b/hw/i2c/smbus_master.c
> @@ -15,6 +15,32 @@
>  #include "hw/i2c/i2c.h"
>  #include "hw/i2c/smbus_master.h"
>  
> +static uint8_t crc8(uint16_t data)
> +{
> +int i;
> +
> +for (i = 0; i < 8; i++) {
> +if (data & 0x8000) {
> +data ^= 0x1070U << 3;
> +}
> +
> +data <<= 1;
> +}
> +
> +return (uint8_t)(data >> 8);
> +}
> +
> +uint8_t i2c_smbus_pec(uint8_t crc, uint8_t *buf, size_t len)
> +{
> +int i;
> +
> +for (i = 0; i < len; i++) {
> +crc = crc8((crc ^ buf[i]) << 8);
> +}
> +
> +return crc;
> +}
> +
>  /* Master device commands.  */
>  int smbus_quick_command(I2CBus *bus, uint8_t addr, int read)
>  {
> diff --git a/include/hw/i2c/smbus_master.h b/include/hw/i2c/smbus_master.h
> index bb13bc423c22..d90f81767d86 100644
> --- a/include/hw/i2c/smbus_master.h
> +++ b/include/hw/i2c/smbus_master.h
> @@ -27,6 +27,8 @@
>  
>  #include "hw/i2c/i2c.h"
>  
> +uint8_t i2c_smbus_pec(uint8_t crc, uint8_t *buf, size_t len);
> +
>  /* Master device commands.  */
>  int smbus_quick_command(I2CBus *bus, uint8_t addr, int read);
>  int smbus_receive_byte(I2CBus *bus, uint8_t addr);
>

Re: [PATCH v6 2/3] hw/i2c: add mctp core

2023-09-21 Thread Andrew Jeffery

On Thu, 2023-09-14 at 11:53 +0200, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> Add an abstract MCTP over I2C endpoint model. This implements MCTP
> control message handling as well as handling the actual I2C transport
> (packetization).
> 
> Devices are intended to derive from this and implement the class
> methods.
> 
> Parts of this implementation is inspired by code[1] previously posted by
> Jonathan Cameron.
> 
> Squashed a fix[2] from Matt Johnston.
> 
>   [1]: 
> https://lore.kernel.org/qemu-devel/20220520170128.4436-1-jonathan.came...@huawei.com/
>   [2]: 
> https://lore.kernel.org/qemu-devel/20221121080445.ga29...@codeconstruct.com.au/
> 
> Tested-by: Jonathan Cameron 
> Reviewed-by: Jonathan Cameron 
> Signed-off-by: Klaus Jensen 

Nice!

Reviewed-by: Andrew Jeffery

Re: [PATCH v6 3/3] hw/nvme: add nvme management interface model

2023-09-21 Thread Andrew Jeffery

On Thu, 2023-09-14 at 11:53 +0200, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> Add the 'nmi-i2c' device that emulates an NVMe Management Interface
> controller.
> 
> Initial support is very basic (Read NMI DS, Configuration Get).
> 
> This is based on previously posted code by Padmakar Kalghatgi, Arun
> Kumar Agasar and Saurav Kumar.
> 
> Reviewed-by: Jonathan Cameron 
> Signed-off-by: Klaus Jensen 
> ---
>  hw/nvme/Kconfig  |   4 +
>  hw/nvme/meson.build  |   1 +
>  hw/nvme/nmi-i2c.c| 407 
> +++
>  hw/nvme/trace-events |   6 +
>  4 files changed, 418 insertions(+)
> 
> diff --git a/hw/nvme/Kconfig b/hw/nvme/Kconfig
> index cfa2ab0f9d5a..e1f6360c0f4b 100644
> --- a/hw/nvme/Kconfig
> +++ b/hw/nvme/Kconfig
> @@ -2,3 +2,7 @@ config NVME_PCI
>  bool
>  default y if PCI_DEVICES || PCIE_DEVICES
>  depends on PCI
> +
> +config NVME_NMI_I2C
> +bool
> +default y if I2C_MCTP
> diff --git a/hw/nvme/meson.build b/hw/nvme/meson.build
> index 1a6a2ca2f307..7bc85f31c149 100644
> --- a/hw/nvme/meson.build
> +++ b/hw/nvme/meson.build
> @@ -1 +1,2 @@
>  system_ss.add(when: 'CONFIG_NVME_PCI', if_true: files('ctrl.c', 'dif.c', 
> 'ns.c', 'subsys.c'))
> +system_ss.add(when: 'CONFIG_NVME_NMI_I2C', if_true: files('nmi-i2c.c'))
> diff --git a/hw/nvme/nmi-i2c.c b/hw/nvme/nmi-i2c.c
> new file mode 100644
> index ..bf4648db0457
> --- /dev/null
> +++ b/hw/nvme/nmi-i2c.c
> @@ -0,0 +1,407 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * SPDX-FileCopyrightText: Copyright (c) 2023 Samsung Electronics Co., Ltd.
> + *
> + * SPDX-FileContributor: Padmakar Kalghatgi 
> + * SPDX-FileContributor: Arun Kumar Agasar 
> + * SPDX-FileContributor: Saurav Kumar 
> + * SPDX-FileContributor: Klaus Jensen 
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/crc32c.h"
> +#include "hw/registerfields.h"
> +#include "hw/i2c/i2c.h"
> +#include "hw/i2c/mctp.h"
> +#include "net/mctp.h"
> +#include "trace.h"
> +
> +/* NVM Express Management Interface 1.2c, Section 3.1 */
> +#define NMI_MAX_MESSAGE_LENGTH 4224
> +
> +#define TYPE_NMI_I2C_DEVICE "nmi-i2c"
> +OBJECT_DECLARE_SIMPLE_TYPE(NMIDevice, NMI_I2C_DEVICE)
> +
> +typedef struct NMIDevice {
> +MCTPI2CEndpoint mctp;
> +
> +uint8_t buffer[NMI_MAX_MESSAGE_LENGTH];
> +uint8_t scratch[NMI_MAX_MESSAGE_LENGTH];
> +
> +size_t  len;
> +int64_t pos;
> +} NMIDevice;
> +
> +FIELD(NMI_MCTPD, MT, 0, 7)
> +FIELD(NMI_MCTPD, IC, 7, 1)
> +
> +#define NMI_MCTPD_MT_NMI 0x4
> +#define NMI_MCTPD_IC_ENABLED 0x1
> +
> +FIELD(NMI_NMP, ROR, 7, 1)
> +FIELD(NMI_NMP, NMIMT, 3, 4)
> +
> +#define NMI_NMP_NMIMT_NVME_MI 0x1
> +#define NMI_NMP_NMIMT_NVME_ADMIN 0x2
> +
> +typedef struct NMIMessage {
> +uint8_t mctpd;
> +uint8_t nmp;
> +uint8_t rsvd2[2];
> +uint8_t payload[]; /* includes the Message Integrity Check */
> +} NMIMessage;
> +
> +typedef struct NMIRequest {
> +   uint8_t opc;
> +   uint8_t rsvd1[3];
> +   uint32_t dw0;
> +   uint32_t dw1;
> +   uint32_t mic;
> +} NMIRequest;
> +
> +FIELD(NMI_CMD_READ_NMI_DS_DW0, DTYP, 24, 8)
> +
> +typedef enum NMIReadDSType {
> +NMI_CMD_READ_NMI_DS_SUBSYSTEM   = 0x0,
> +NMI_CMD_READ_NMI_DS_PORTS   = 0x1,
> +NMI_CMD_READ_NMI_DS_CTRL_LIST   = 0x2,
> +NMI_CMD_READ_NMI_DS_CTRL_INFO   = 0x3,
> +NMI_CMD_READ_NMI_DS_OPT_CMD_SUPPORT = 0x4,
> +NMI_CMD_READ_NMI_DS_MEB_CMD_SUPPORT = 0x5,
> +} NMIReadDSType;
> +
> +#define NMI_STATUS_INVALID_PARAMETER 0x4
> +
> +static void nmi_scratch_append(NMIDevice *nmi, const void *buf, size_t count)
> +{
> +assert(nmi->pos + count <= NMI_MAX_MESSAGE_LENGTH);
> +
> +memcpy(nmi->scratch + nmi->pos, buf, count);
> +nmi->pos += count;
> +}
> +
> +static void nmi_set_parameter_error(NMIDevice *nmi, uint8_t bit, uint16_t 
> byte)
> +{
> +/* NVM Express Management Interface 1.2c, Figure 30 */
> +struct resp {
> +uint8_t  status;
> +uint8_t  bit;
> +uint16_t byte;
> +};
> +
> +struct resp buf = {
> +.status = NMI_STATUS_INVALID_PARAMETER,
> +.bit = bit & 0x3,
> +.byte = byte,
> +};
> +
> +nmi_scratch_append(nmi, &buf, sizeof(buf));
> +}
> +
> +static void nmi_set_error(NMIDevice *nmi, uint8_t status)
> +{
> +const uint8_t buf[4] = {status,};
> +
> +nmi_scratch_append(nmi, buf, sizeof(buf));
> +}
> +
> +static void nmi_handle_mi_read_nmi_ds(NMIDevice *nmi, NMIRequest *request)
> +{
> +I2CSlave *i2c = I2C_SLAVE(nmi);
> +
> +uint32_t dw0 = le32_to_cpu(request->dw0);
> +uint8_t dtyp = FIELD_EX32(dw0, NMI_CMD_READ_NMI_DS_DW0, DTYP);
> +
> +trace_nmi_handle_mi_read_nmi_ds(dtyp);
> +
> +static const uint8_t nmi_ds_subsystem[36] = {
> +0x00,   /* success */
> +0x20, 0x00, /* response data length */
> +0x00,   /* reserved */
> +0x00,   /* number of ports */
> +0x01,   /* major version */
> +0x01,   /* minor version */

62 matches

Mail list logo