[Qemu-commits] [qemu/qemu] f0e0c4: qapi-block-core: Clean up blockdev-snapshot-intern...

[Richard Henderson]

  Branch: refs/heads/staging
  Home:   https://github.com/qemu/qemu
  Commit: f0e0c46309dac41a8420bcb379b1cffa7da0f62c
      
https://github.com/qemu/qemu/commit/f0e0c46309dac41a8420bcb379b1cffa7da0f62c
  Author: Markus Armbruster <armbru@redhat.com>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M qapi/block-core.json

  Log Message:
  -----------
  qapi-block-core: Clean up blockdev-snapshot-internal-sync doc

BlockdevSnapshotInternal is the arguments type of command
blockdev-snapshot-internal-sync.  Its doc comment contains this note:

    # .. note:: In a transaction, if @name is empty or any snapshot matching
    #    @name exists, the operation will fail.  Only some image formats
    #    support it; for example, qcow2, and rbd.

"In a transaction" is misleading, and "if @name is empty or any
snapshot matching @name exists, the operation will fail" is redundant
with the command's Errors documentation.  Drop.

The remainder is fine.  Move it to the command's doc comment, where it
is more prominently visible, with a slight rephrasing for clarity.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-ID: <20240718123609.3063055-1-armbru@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>


  Commit: d5f6cbb263662af585b866481382c3f5ba5cd638
      
https://github.com/qemu/qemu/commit/d5f6cbb263662af585b866481382c3f5ba5cd638
  Author: Kevin Wolf <kwolf@redhat.com>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M block/block-copy.c

  Log Message:
  -----------
  block-copy: Fix missing graph lock

The graph lock needs to be held when calling bdrv_co_pdiscard(). Fix
block_copy_task_entry() to take it for the call.

WITH_GRAPH_RDLOCK_GUARD() was implemented in a weak way because of
limitations in clang's Thread Safety Analysis at the time, so that it
only asserts that the lock is held (which allows calling functions that
require the lock), but we never deal with the unlocking (so even after
the scope of the guard, the compiler assumes that the lock is still
held). This is why the compiler didn't catch this locking error.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20240627181245.281403-2-kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>


  Commit: 7e1711164683b1036f7da5f87c5f66c17a043ab1
      
https://github.com/qemu/qemu/commit/7e1711164683b1036f7da5f87c5f66c17a043ab1
  Author: Kevin Wolf <kwolf@redhat.com>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M include/block/graph-lock.h
    M meson.build

  Log Message:
  -----------
  block/graph-lock: Make WITH_GRAPH_RDLOCK_GUARD() fully checked

Upstream clang 18 (and backports to clang 17 in Fedora and RHEL)
implemented support for __attribute__((cleanup())) in its Thread Safety
Analysis, so we can now actually have a proper implementation of
WITH_GRAPH_RDLOCK_GUARD() that understands when we acquire and when we
release the lock.

-Wthread-safety is now only enabled if the compiler is new enough to
understand this pattern. In theory, we could have used some #ifdefs to
keep the existing basic checks on old compilers, but as long as someone
runs a newer compiler (and our CI does), we will catch locking problems,
so it's probably not worth keeping multiple implementations for this.

The implementation can't use g_autoptr any more because the glib macros
define wrapper functions that don't have the right TSA attributes, so
the compiler would complain about them. Just use the cleanup attribute
directly instead.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20240627181245.281403-3-kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>


  Commit: cfe0880835cd364b590ffd27ef8dbd2ad8838bc5
      
https://github.com/qemu/qemu/commit/cfe0880835cd364b590ffd27ef8dbd2ad8838bc5
  Author: Kevin Wolf <kwolf@redhat.com>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M hw/scsi/scsi-disk.c

  Log Message:
  -----------
  scsi-disk: Use positive return value for status in dma_readv/writev

In some error cases, scsi_block_sgio_complete() never calls the passed
callback, but directly completes the request. This leads to bugs because
its error paths are not exact copies of what the callback would normally
do.

In preparation to fix this, allow passing positive return values to the
callbacks that represent the status code that should be used to complete
the request.

scsi_handle_rw_error() already handles positive values for its ret
parameter because scsi_block_sgio_complete() calls directly into it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <20240731123207.27636-2-kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>


  Commit: 622a70161ac258e4a166a7dca4b5be267e0652d9
      
https://github.com/qemu/qemu/commit/622a70161ac258e4a166a7dca4b5be267e0652d9
  Author: Kevin Wolf <kwolf@redhat.com>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M hw/scsi/scsi-disk.c

  Log Message:
  -----------
  scsi-block: Don't skip callback for sgio error status/driver_status

Instead of calling into scsi_handle_rw_error() directly from
scsi_block_sgio_complete() and skipping the normal callback, go through
the normal cleanup path by calling the callback with a positive error
value.

The important difference here is not only that the code path is cleaner,
but that the callbacks set r->req.aiocb = NULL. If we skip setting this
and the error action is BLOCK_ERROR_ACTION_STOP, resuming the VM runs
into an assertion failure in scsi_read_data() or scsi_write_data()
because the dangling aiocb pointer is unexpected.

Fixes: a108557bbf ("scsi: inline sg_io_sense_from_errno() into the callers.")
Buglink: https://issues.redhat.com/browse/RHEL-50000
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <20240731123207.27636-3-kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>


  Commit: 8a0495624f23f8f01dfb1484f367174f3b3572e8
      
https://github.com/qemu/qemu/commit/8a0495624f23f8f01dfb1484f367174f3b3572e8
  Author: Kevin Wolf <kwolf@redhat.com>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M hw/scsi/scsi-disk.c

  Log Message:
  -----------
  scsi-disk: Add warning comments that host_status errors take a shortcut

scsi_block_sgio_complete() has surprising behaviour in that there are
error cases in which it directly completes the request and never calls
the passed callback. In the current state of the code, this doesn't seem
to result in bugs, but with future code changes, we must be careful to
never rely on the callback doing some cleanup until this code smell is
fixed. For now, just add warnings to make people aware of the trap.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <20240731123207.27636-4-kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>


  Commit: 9da6bd39f92434f55573acd017841b195c60188f
      
https://github.com/qemu/qemu/commit/9da6bd39f92434f55573acd017841b195c60188f
  Author: Kevin Wolf <kwolf@redhat.com>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M hw/scsi/scsi-disk.c

  Log Message:
  -----------
  scsi-disk: Always report RESERVATION_CONFLICT to guest

In the case of scsi-block, RESERVATION_CONFLICT is not a backend error,
but indicates that the guest tried to make a request that it isn't
allowed to execute. Pass the error to the guest so that it can decide
what to do with it.

Without this, if we stop the VM in response to a RESERVATION_CONFLICT
(as is the default policy in management software such as oVirt or
KubeVirt), it can happen that the VM cannot be resumed any more because
every attempt to resume it immediately runs into the same error and
stops the VM again.

One case that expects RESERVATION_CONFLICT errors to be visible in the
guest is running the validation tests in Windows 2019's Failover Cluster
Manager, which intentionally tries to execute invalid requests to see if
they are properly rejected.

Buglink: https://issues.redhat.com/browse/RHEL-50000
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <20240731123207.27636-5-kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>


  Commit: b881cf00c99e03bc8a3648581f97736ff275b18b
      
https://github.com/qemu/qemu/commit/b881cf00c99e03bc8a3648581f97736ff275b18b
  Author: Amjad Alsharafi <amjadsharafi10@gmail.com>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M block/vvfat.c

  Log Message:
  -----------
  vvfat: Fix bug in writing to middle of file

Before this commit, the behavior when calling `commit_one_file` for
example with `offset=0x2000` (second cluster), what will happen is that
we won't fetch the next cluster from the fat, and instead use the first
cluster for the read operation.

This is due to off-by-one error here, where `i=0x2000 !< offset=0x2000`,
thus not fetching the next cluster.

Signed-off-by: Amjad Alsharafi <amjadsharafi10@gmail.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Tested-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: 
<b97c1e1f1bc2f776061ae914f95d799d124fcd73.1721470238.git.amjadsharafi10@gmail.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>


  Commit: 21b25a0e466a5bba0a45600bb8100ab564202ed1
      
https://github.com/qemu/qemu/commit/21b25a0e466a5bba0a45600bb8100ab564202ed1
  Author: Amjad Alsharafi <amjadsharafi10@gmail.com>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M block/vvfat.c

  Log Message:
  -----------
  vvfat: Fix usage of `info.file.offset`

The field is marked as "the offset in the file (in clusters)", but it
was being used like this
`cluster_size*(nums)+mapping->info.file.offset`, which is incorrect.

Signed-off-by: Amjad Alsharafi <amjadsharafi10@gmail.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: 
<72f19a7903886dda1aa78bcae0e17702ee939262.1721470238.git.amjadsharafi10@gmail.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>


  Commit: f60a6f7e17bf2a2a0f0a08265ac9b077fce42858
      
https://github.com/qemu/qemu/commit/f60a6f7e17bf2a2a0f0a08265ac9b077fce42858
  Author: Amjad Alsharafi <amjadsharafi10@gmail.com>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M block/vvfat.c

  Log Message:
  -----------
  vvfat: Fix wrong checks for cluster mappings invariant

How this `abort` was intended to check for was:
- if the `mapping->first_mapping_index` is not the same as
  `first_mapping_index`, which **should** happen only in one case,
  when we are handling the first mapping, in that case
  `mapping->first_mapping_index == -1`, in all other cases, the other
  mappings after the first should have the condition `true`.
- From above, we know that this is the first mapping, so if the offset
  is not `0`, then abort, since this is an invalid state.

The issue was that `first_mapping_index` is not set if we are
checking from the middle, the variable `first_mapping_index` is
only set if we passed through the check `cluster_was_modified` with the
first mapping, and in the same function call we checked the other
mappings.

One approach is to go into the loop even if `cluster_was_modified`
is not true so that we will be able to set `first_mapping_index` for the
first mapping, but since `first_mapping_index` is only used here,
another approach is to just check manually for the
`mapping->first_mapping_index != -1` since we know that this is the
value for the only entry where `offset == 0` (i.e. first mapping).

Signed-off-by: Amjad Alsharafi <amjadsharafi10@gmail.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: 
<b0fbca3ee208c565885838f6a7deeaeb23f4f9c2.1721470238.git.amjadsharafi10@gmail.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>


  Commit: 5eed3db336506b529b927ba221fe0d836e5b8819
      
https://github.com/qemu/qemu/commit/5eed3db336506b529b927ba221fe0d836e5b8819
  Author: Amjad Alsharafi <amjadsharafi10@gmail.com>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M block/vvfat.c

  Log Message:
  -----------
  vvfat: Fix reading files with non-continuous clusters

When reading with `read_cluster` we get the `mapping` with
`find_mapping_for_cluster` and then we call `open_file` for this
mapping.
The issue appear when its the same file, but a second cluster that is
not immediately after it, imagine clusters `500 -> 503`, this will give
us 2 mappings one has the range `500..501` and another `503..504`, both
point to the same file, but different offsets.

When we don't open the file since the path is the same, we won't assign
`s->current_mapping` and thus accessing way out of bound of the file.

>From our example above, after `open_file` (that didn't open anything) we
will get the offset into the file with
`s->cluster_size*(cluster_num-s->current_mapping->begin)`, which will
give us `0x2000 * (504-500)`, which is out of bound for this mapping and
will produce some issues.

Signed-off-by: Amjad Alsharafi <amjadsharafi10@gmail.com>
Message-ID: 
<1f3ea115779abab62ba32c788073cdc99f9ad5dd.1721470238.git.amjadsharafi10@gmail.com>
[kwolf: Simplified the patch based on Amjad's analysis and input]
Signed-off-by: Kevin Wolf <kwolf@redhat.com>


  Commit: c8f60bfb4345ea8343a53eaefe88d47b44c53f24
      
https://github.com/qemu/qemu/commit/c8f60bfb4345ea8343a53eaefe88d47b44c53f24
  Author: Amjad Alsharafi <amjadsharafi10@gmail.com>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M tests/qemu-iotests/check
    A tests/qemu-iotests/fat16.py
    M tests/qemu-iotests/testenv.py
    A tests/qemu-iotests/tests/vvfat
    A tests/qemu-iotests/tests/vvfat.out

  Log Message:
  -----------
  iotests: Add `vvfat` tests

Added several tests to verify the implementation of the vvfat driver.

We needed a way to interact with it, so created a basic `fat16.py` driver
that handled writing correct sectors for us.

Added `vvfat` to the non-generic formats, as its not a normal image format.

Signed-off-by: Amjad Alsharafi <amjadsharafi10@gmail.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Tested-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: 
<bb8149c945301aefbdf470a0924c07f69f9c087d.1721470238.git.amjadsharafi10@gmail.com>
[kwolf: Made mypy and pylint happy to unbreak 297]
Signed-off-by: Kevin Wolf <kwolf@redhat.com>


  Commit: ca1dcc913888a97073233dd9142ca5241dab1b7c
      
https://github.com/qemu/qemu/commit/ca1dcc913888a97073233dd9142ca5241dab1b7c
  Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
  Date:   2024-08-06 (Tue, 06 Aug 2024)

  Changed paths:
    M tests/qemu-iotests/024
    M tests/qemu-iotests/024.out

  Log Message:
  -----------
  iotests/024: exclude 'backing file format' field from the output

Apparently 'qemu-img info' doesn't report the backing file format field
for qed (as it does for qcow2):

$ qemu-img create -f qed base.qed 1M && qemu-img create -f qed -b base.qed -F 
qed top.qed 1M
$ qemu-img create -f qcow2 base.qcow2 1M && qemu-img create -f qcow2 -b 
base.qcow2 -F qcow2 top.qcow2 1M
$ qemu-img info top.qed | grep 'backing file format'
$ qemu-img info top.qcow2 | grep 'backing file format'
backing file format: qcow2

This leads to the 024 test failure with -qed.  Let's just filter the
field out and exclude it from the output.

This is a fixup for the commit f93e65ee51 ("iotests/{024, 271}: add
testcases for qemu-img rebase").

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Message-ID: <20240730094701.790624-1-andrey.drobyshev@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>


  Commit: 6d00c6f982562222adbd0613966285792125abe5
      
https://github.com/qemu/qemu/commit/6d00c6f982562222adbd0613966285792125abe5
  Author: Richard Henderson <richard.henderson@linaro.org>
  Date:   2024-08-07 (Wed, 07 Aug 2024)

  Changed paths:
    M block/block-copy.c
    M block/vvfat.c
    M hw/scsi/scsi-disk.c
    M include/block/graph-lock.h
    M meson.build
    M qapi/block-core.json
    M tests/qemu-iotests/024
    M tests/qemu-iotests/024.out
    M tests/qemu-iotests/check
    A tests/qemu-iotests/fat16.py
    M tests/qemu-iotests/testenv.py
    A tests/qemu-iotests/tests/vvfat
    A tests/qemu-iotests/tests/vvfat.out

  Log Message:
  -----------
  Merge tag 'for-upstream' of https://repo.or.cz/qemu/kevin into staging

Block layer patches

- scsi-block: Fix error handling with r/werror=stop
- Depend on newer clang for TSA, make WITH_GRAPH_RDLOCK_GUARD() fully
  checked, fix block-copy to add missing lock
- vvfat: Fix write bugs for large files and add iotests
- Clean up blockdev-snapshot-internal-sync doc
- Fix iotests 024 for qed

# -----BEGIN PGP SIGNATURE-----
#
# iQJFBAABCAAvFiEE3D3rFZqa+V09dFb+fwmycsiPL9YFAmayag4RHGt3b2xmQHJl
# ZGhhdC5jb20ACgkQfwmycsiPL9Y0yhAArDpKYNsOmJerL/abIetchJ84suuR2MHZ
# iziAsTXk1iiSNYrAfXyiGhMsovvApluW1bojB80XLjaWFeN76zCRq0bnYVhv/xeX
# bQddC4JyWkcYGmdASiFpvQ7+p37jBh+OebmxsF557s4uM6b0/QN1xnOyyjBpyJbB
# aBTNgUYaTXmD6RD8h9SscnroNqhckuv6+zm0SX2Z4wRTF2uEmVWdL2yz2I3P8G7W
# dhVfgOCYQmW0cSfTueBQJClaUoHyJeibd4TzHR12hFAKIYobXMGfcE3AhfpBvO3t
# 0SEQ5MUx3zasGVENSJA6UnzVnpHl8HRtdDIFhSWb6yZJJ6RPPGynj7UVvFOK1SXM
# iXzj1kcYzFO/AFO3JxkSr6IHZdzZr4e5wtuFbw8Je6Ai0P5prc53jBDovtbAT0Wt
# +dAP7cnntYLDcAIsJqGUdr2FJfSOh9gApH/I3kF3scDwLRpb6OlWJ60T5b98VcR4
# +J67AXuGN7OXtYEU6GupZpWTQ/nZQ63egrCfJlqL67QduuF1YvcgOo2+TdAwDYkf
# 8nU7AEUgzWox8EcTkof/BXYYabOjn0D6/1+aLc7J7vGGlnKVyQMK9Kn5MRBzkyb3
# iWOtuv8aoNfnxtuMnpwe/Uf2hhOGi8IldnoP2+Yb9urWnFQ3Jbbmnv8Ga7mDQmRs
# ue4gDS51MCc=
# =ouBM
# -----END PGP SIGNATURE-----
# gpg: Signature made Wed 07 Aug 2024 04:23:10 AM AEST
# gpg:                using RSA key DC3DEB159A9AF95D3D7456FE7F09B272C88F2FD6
# gpg:                issuer "kwolf@redhat.com"
# gpg: Good signature from "Kevin Wolf <kwolf@redhat.com>" [full]

* tag 'for-upstream' of https://repo.or.cz/qemu/kevin:
  iotests/024: exclude 'backing file format' field from the output
  iotests: Add `vvfat` tests
  vvfat: Fix reading files with non-continuous clusters
  vvfat: Fix wrong checks for cluster mappings invariant
  vvfat: Fix usage of `info.file.offset`
  vvfat: Fix bug in writing to middle of file
  scsi-disk: Always report RESERVATION_CONFLICT to guest
  scsi-disk: Add warning comments that host_status errors take a shortcut
  scsi-block: Don't skip callback for sgio error status/driver_status
  scsi-disk: Use positive return value for status in dma_readv/writev
  block/graph-lock: Make WITH_GRAPH_RDLOCK_GUARD() fully checked
  block-copy: Fix missing graph lock
  qapi-block-core: Clean up blockdev-snapshot-internal-sync doc

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>


Compare: https://github.com/qemu/qemu/compare/f4bb895a3bb0...6d00c6f98256

To unsubscribe from these emails, change your notification settings at 
https://github.com/qemu/qemu/settings/notifications