Re: [PATCH] docs: Prefer 'cookie' over 'handle'
On Fri, Mar 03, 2023 at 04:15:03PM -0600, Eric Blake wrote: > In libnbd, we quickly learned that distinguishing between 'handle' > (verb for acting on an object) and 'handle' (noun describing which > object to act on) could get confusing; we solved it by renaming the > latter to 'cookie'. Copy that approach into the NBD spec, and make it > obvious that a cookie is opaque data from the point of view of the > server. Makes no difference to implementations (other than older code > still using 'handle' may be slightly harder to tie back to the spec). > > Suggested-by: Richard W.M. Jones > Signed-off-by: Eric Blake > --- > doc/proto.md | 18 +- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/doc/proto.md b/doc/proto.md > index 3a96703..abb23e8 100644 > --- a/doc/proto.md > +++ b/doc/proto.md > @@ -301,11 +301,11 @@ may be handled by the server asynchronously), and > structured reply > chunks from one request may be interleaved with reply messages from > other requests; however, there may be constraints that prevent > arbitrary reordering of structured reply chunks within a given reply. > -Clients SHOULD use a handle that is distinct from all other currently > -pending transactions, but MAY reuse handles that are no longer in > -flight; handles need not be consecutive. In each reply message > +Clients SHOULD use a cookie that is distinct from all other currently > +pending transactions, but MAY reuse cookies that are no longer in > +flight; cookies need not be consecutive. In each reply message > (whether simple or structured), the server MUST use the same value for > -handle as was sent by the client in the corresponding request. In > +cookie as was sent by the client in the corresponding request. In > this way, the client can correlate which request is receiving a > response. > > @@ -349,7 +349,7 @@ The request message, sent by the client, looks as follows: > C: 32 bits, 0x25609513, magic (`NBD_REQUEST_MAGIC`) > C: 16 bits, command flags > C: 16 bits, type > -C: 64 bits, handle > +C: 64 bits, cookie > C: 64 bits, offset (unsigned) > C: 32 bits, length (unsigned) > C: (*length* bytes of data if the request is of type `NBD_CMD_WRITE`) > @@ -366,7 +366,7 @@ follows: > S: 32 bits, 0x67446698, magic (`NBD_SIMPLE_REPLY_MAGIC`; used to be > `NBD_REPLY_MAGIC`) > S: 32 bits, error (MAY be zero) > -S: 64 bits, handle > +S: 64 bits, cookie > S: (*length* bytes of data if the request is of type `NBD_CMD_READ` and > *error* is zero) > > @@ -381,7 +381,7 @@ server must initiate a hard disconnect). Second, there > is no way to > efficiently skip over portions of a sparse file that are known to > contain all zeroes. Finally, it is not possible to reliably decode > the server traffic without also having context of what pending read > -requests were sent by the client, to see which *handle* values will > +requests were sent by the client, to see which *cookie* values will > have accompanying payload on success. Therefore structured replies > are also permitted if negotiated. > > @@ -398,7 +398,7 @@ sending errors via a structured reply, as the error can > then be > accompanied by a string payload to present to a human user. > > A structured reply MAY occupy multiple structured chunk messages > -(all with the same value for "handle"), and the > +(all with the same value for "cookie"), and the > `NBD_REPLY_FLAG_DONE` reply flag is used to identify the final > chunk. Unless further documented by individual requests below, > the chunks MAY be sent in any order, except that the chunk with > @@ -418,7 +418,7 @@ A structured reply chunk message looks as follows: > S: 32 bits, 0x668e33ef, magic (`NBD_STRUCTURED_REPLY_MAGIC`) > S: 16 bits, flags > S: 16 bits, type > -S: 64 bits, handle > +S: 64 bits, cookie > S: 32 bits, length of payload (unsigned) > S: *length* bytes of payload data (if *length* is nonzero) Acked-by: Richard W.M. Jones I think this change is very sensible. libnbd has always called this a cookie, because "handle" is a very generic term often used to refer to things inside the program. Leading to this code: https://gitlab.com/nbdkit/libnbd/-/blob/d169661119f66893e2c3050ce25f76554c519b59/generator/states-issue-command.c#L47 nbdkit uses "handle" but we should change that (whether or not this goes upstream in fact). Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
Re: [Libguestfs] [PATCH] docs: Prefer 'cookie' over 'handle'
On Sat, Mar 4, 2023 at 12:15 AM Eric Blake wrote: > > In libnbd, we quickly learned that distinguishing between 'handle' > (verb for acting on an object) and 'handle' (noun describing which > object to act on) could get confusing; we solved it by renaming the > latter to 'cookie'. Copy that approach into the NBD spec, and make it > obvious that a cookie is opaque data from the point of view of the > server. Good change, will make it easier to search code. But the actual text does not make it clear that a cookie is opaque data from point of view of the client. Maybe make this more clear? > Makes no difference to implementations (other than older code > still using 'handle' may be slightly harder to tie back to the spec). To avoid confusion with older code that carefully used "handle" to match the spec, maybe add a note that "cookie" was named "handle" before? Nir
Re: [PATCH] configure: Disable thread-safety warnings on macOS
On Wed, 1 Mar 2023 at 13:33, Kevin Wolf wrote: > > Am 01.03.2023 um 12:34 hat Thomas Huth geschrieben: > > The enablement of -Wthread-safety broke compilation on macOS (if > > -Werror is enabled, like in our CI). Disable it there by default > > until the problems are resolved. > > > > Signed-off-by: Thomas Huth > > This is simpler than what I attempted (test compiling something using > the same TSA features as the failing code), but didn't actually work. > Since I don't have access to macOS, it's hard for me to improve the > configure test. So I'm fine with just doing this instead. > > Acked-by: Kevin Wolf I've applied this to master because it fixes the CI job, but we should probably look more closely at what's going on, because it seems plausible to me that it's something that we could hit on Linux too with either a newer or older version of clang. thanks -- PMM
Re: [PULL 03/38] pflash: Only read non-zero parts of backend image
On 8.02.2023 12:19, Cédric Le Goater wrote:
On 2/7/23 13:48, Kevin Wolf wrote:
Am 07.02.2023 um 10:19 hat Cédric Le Goater geschrieben:
On 2/7/23 09:38, Kevin Wolf wrote:
Am 06.02.2023 um 16:54 hat Cédric Le Goater geschrieben:
On 1/20/23 13:25, Kevin Wolf wrote:
From: Xiang Zheng
Currently we fill the VIRT_FLASH memory space with two 64MB NOR images
when using persistent UEFI variables on virt board. Actually we only use
a very small(non-zero) part of the memory while the rest significant
large(zero) part of memory is wasted.
So this patch checks the block status and only writes the non-zero part
into memory. This requires pflash devices to use sparse files for
backends.
Signed-off-by: Xiang Zheng
[ kraxel: rebased to latest master ]
Signed-off-by: Gerd Hoffmann
Message-Id: <20221220084246.1984871-1-kra...@redhat.com>
Reviewed-by: Daniel P. Berrangé
Reviewed-by: Kevin Wolf
Signed-off-by: Kevin Wolf
This newly merged patch introduces a "regression" when booting an Aspeed
machine. The following extra m25p80 patch (not yet merged) is required
for the issue to show:
https://lore.kernel.org/qemu-devel/20221115151000.2080833-1-...@kaod.org/
U-Boot fails to find the filesystem in that case.
It can be easily reproduced with the witherspoon-bmc machine and seems
to be related to the use of a UBI filesystem. Other Aspeed machines not
using UBI are not impacted.
Here is a tentative fix. I don't know enough the block layer to explain
what is happening :/
I was puzzled for a moment, but...
@@ -39,7 +39,7 @@ static int blk_pread_nonzeroes(BlockBack
return ret;
}
if (!(ret & BDRV_BLOCK_ZERO)) {
- ret = bdrv_pread(bs->file, offset, bytes,
'bs->file' rather than 'bs' really looks wrong. I think replacing that
would already fix the bug you're seeing.
Just to be sure, how did you configure the block backend? bs->file would
happen to work more or less with raw over file-posix (which is probably
what Gerd tested), but I think it breaks with anything else.
The command is :
$ qemu-system-arm -M witherspoon-bmc -net user \
-drive file=/path/to/file.mtd,format=raw,if=mtd \
-nographic -serial mon:stdio -snapshot
If I remove '-snapshot', all works fine.
Ok, that makes sense then. -snapshot creates a temporary qcow2 overlay,
and then what your guest sees with bs->file is not the virtual disk
content of the qcow2 image, but the qcow2 file itself.
yes. Same symptom with pflash devices, TCG and KVM. The guest hangs with
-snapshot.
C.
qemu-system-aarch64 -M virt -smp 2 -cpu max -accel tcg,thread=multi -nographic
-m 2G -drive
if=pflash,format=raw,file=/usr/share/edk2/aarch64/QEMU_EFI-silent-pflash.raw,readonly=on
-drive if=pflash,format=raw,file=rhel9-varstore.img -device
virtio-net,netdev=net0,bus=pcie.0,addr=0x3 -netdev user,id=net0 -drive
file=rhel9-arm64.qcow2,if=none,id=disk,format=qcow2,cache=none -device
virtio-blk-device,drive=disk -serial mon:stdio -snapshot
+1 here for QEMU + KVM/x86: OVMF CODE file fails to load (is all zeroes)
with either "-snapshot" QEMU command line option or even with just "snapshot=on"
setting enabled on pflash0.
Reverting this patch seems to fix the issue.
Thanks,
Maciej
