Re: [Qemu-block] [Qemu-devel] [RFC] Require Python 3 for building QEMU
On 15 October 2018 at 19:01, Eduardo Habkost wrote: > On Mon, Oct 15, 2018 at 05:55:27PM +0100, Peter Maydell wrote: >> I also suspect "a few months" is an underestimate. My guess >> would be we're going to want to keep Python 2 support for >> at least the next year, maybe two. > > Python 2.7 will die in less than 15 months[1]. I really want us > to stop reviewing and maintaining Python 2 code in QEMU in less > than 1 year. Preferably in less than 6 months. Well, as with all our dependencies it depends on what the default python is in the various LTS distro versions we support. Once the last LTS distro with python 2 falls off our this-is-what-we-support list we can drop 2 support, possibly with a deprecate-and-warn-then-drop cycle. thanks -- PMM
[Qemu-block] [PATCH] crypto: initialize sector size even when opening with no IO flag
The qcow2 block driver expects to see a valid sector size even when it
has opened the crypto layer with QCRYPTO_BLOCK_OPEN_NO_IO.
Signed-off-by: Daniel P. Berrangé
---
crypto/block-qcow.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/crypto/block-qcow.c b/crypto/block-qcow.c
index 4284e05167..7606231e79 100644
--- a/crypto/block-qcow.c
+++ b/crypto/block-qcow.c
@@ -102,6 +102,8 @@ qcrypto_block_qcow_open(QCryptoBlock *block,
Error **errp)
{
if (flags & QCRYPTO_BLOCK_OPEN_NO_IO) {
+block->sector_size = QCRYPTO_BLOCK_QCOW_SECTOR_SIZE;
+block->payload_offset = 0;
return 0;
} else {
if (!options->u.qcow.key_secret) {
--
2.17.2
Re: [Qemu-block] [PATCH] qcow2: Get the request alignment for encrypted images from QCryptoBlock
On Mon, Oct 15, 2018 at 06:38:14PM +0200, Max Reitz wrote: > On 11.10.18 12:58, Alberto Garcia wrote: > > This doesn't have any practical effect at the moment because the > > values of BDRV_SECTOR_SIZE, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE and > > QCRYPTO_BLOCK_QCOW_SECTOR_SIZE are all the same (512 bytes), but > > future encryption methods could have different requirements. > > > > Signed-off-by: Alberto Garcia > > --- > > block/qcow2.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > This breaks non-LUKS encryption: > > $ ./qemu-img create -f qcow2 -o encryption=on,encrypt.key-secret=secret > --object secret,id=secret,data=foo foo.qcow2 64M > Formatting 'foo.qcow2', fmt=qcow2 size=67108864 encryption=on > encrypt.key-secret=secret cluster_size=65536 lazy_refcounts=off > refcount_bits=16 > qemu-img: block.c:1248: bdrv_open_driver: Assertion > `is_power_of_2(bs->bl.request_alignment)' failed. > [1]13589 abort (core dumped) ./qemu-img create -f qcow2 -o > encryption=on,encrypt.key-secret=secret --objec > > (As seen in iotests 049, 087, 134, and 158.) We failed to set sector size in the crypto backend when opening with the NO_IO flag set. I just sent a fix for that. Feel free to queue it as part of the block layer rather than waiting for my next crypto layer pull request. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-block] [PATCH] crypto: initialize sector size even when opening with no IO flag
On Tue 16 Oct 2018 12:31:05 PM CEST, Daniel P. Berrangé wrote: > The qcow2 block driver expects to see a valid sector size even when it > has opened the crypto layer with QCRYPTO_BLOCK_OPEN_NO_IO. > > Signed-off-by: Daniel P. Berrangé I was actually preparing a patch along these lines :-) Reviewed-by: Alberto Garcia Berto
Re: [Qemu-block] [PATCH] crypto: initialize sector size even when opening with no IO flag
Am 16.10.2018 um 12:31 hat Daniel P. Berrangé geschrieben: > The qcow2 block driver expects to see a valid sector size even when it > has opened the crypto layer with QCRYPTO_BLOCK_OPEN_NO_IO. > > Signed-off-by: Daniel P. Berrangé Thanks, applied to the block branch (and staged before the other patch that exposed the bug). Kevin
Re: [Qemu-block] [PATCH 1/5] option: Make option help nicer to read
Am 15.10.2018 um 19:28 hat Max Reitz geschrieben: > This adds some whitespace into the option help (including indentation) > and replaces '=' by ': ' (not least because '=' should be used for > values, not types). Furthermore, the list name is no longer printed as > part of every line, but only once in advance, and only if the caller did > not print a caption already. > > Signed-off-by: Max Reitz If this isn't a bike shedding series, then what is? So... > --- a/tests/qemu-iotests/082.out > +++ b/tests/qemu-iotests/082.out > @@ -44,171 +44,171 @@ cluster_size: 8192 > > Testing: create -f qcow2 -o help TEST_DIR/t.qcow2 128M > Supported options: > -size Virtual disk size > -compat Compatibility level (0.10 or 1.1) > -backing_file File name of a base image > -backing_fmt Image format of the base image > -encryption Encrypt the image with format 'aes'. (Deprecated in favor > of encrypt.format=aes) > -encrypt.format Encrypt the image, format choices: 'aes', 'luks' > -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase > -encrypt.cipher-alg Name of encryption cipher algorithm > -encrypt.cipher-mode Name of encryption cipher mode > -encrypt.ivgen-alg Name of IV generator algorithm > -encrypt.ivgen-hash-alg Name of IV generator hash algorithm > -encrypt.hash-alg Name of encryption hash algorithm > -encrypt.iter-time Time to spend in PBKDF in milliseconds > -cluster_size qcow2 cluster size > -preallocationPreallocation mode (allowed values: off, metadata, falloc, > full) > -lazy_refcounts Postpone refcount updates > -refcount_bitsWidth of a reference count entry in bits > -nocowTurn off copy-on-write (valid only on btrfs) > + backing_file: str - File name of a base image > + backing_fmt: str - Image format of the base image > + cluster_size: size - qcow2 cluster size > + compat: str - Compatibility level (0.10 or 1.1) > + encrypt.cipher-alg: str - Name of encryption cipher algorithm > + encrypt.cipher-mode: str - Name of encryption cipher mode > + encrypt.format: str - Encrypt the image, format choices: 'aes', 'luks' > + encrypt.hash-alg: str - Name of encryption hash algorithm > + encrypt.iter-time: num - Time to spend in PBKDF in milliseconds > + encrypt.ivgen-alg: str - Name of IV generator algorithm > + encrypt.ivgen-hash-alg: str - Name of IV generator hash algorithm > + encrypt.key-secret: str - ID of secret providing qcow AES key or LUKS > passphrase > + encryption: bool (on/off) - Encrypt the image with format 'aes'. > (Deprecated in favor of encrypt.format=aes) > + lazy_refcounts: bool (on/off) - Postpone refcount updates > + nocow: bool (on/off) - Turn off copy-on-write (valid only on btrfs) > + preallocation: str - Preallocation mode (allowed values: off, metadata, > falloc, full) > + refcount_bits: num - Width of a reference count entry in bits > + size: size - Virtual disk size I like the result of this series better than what we have in current master. Looking at this patch, however, I must also say that I like the original state better than both. How about aligning the description to the same column again to combine the best of both worlds? Kevin
Re: [Qemu-block] [RFC PATCH] block: Clean up bdrv_img_create()'s error reporting
Am 16.10.2018 um 08:41 hat Markus Armbruster geschrieben:
> bdrv_img_create() takes an Error ** argument and used it in the
> conventional way, except for one place: when qemu_opts_do_parse()
> fails, it first reports its error to stderr or the HMP monitor with
> error_report_err(), then error_setg()'s a generic error. When the
> caller reports that second error similarly, this produces two
> consecutive error messages on stderr or the HMP monitor. When the
> caller does something else with it, such as send it via QMP, the first
> error still goes to stderr or the HMP monitor. Not good.
>
> Turn the first message into a prefix for the second.
>
> Signed-off-by: Markus Armbruster
> ---
>
> This is RFC because I didn't check whether "caller does something else
> with it" can actually happen with current code, and I'm not sure the
> prefix is wanted. I hope we'll answer both questions during review.
The only caller that passes non-NULL options and can therefore even run
into this error path is qemu-img.c. It passes any Error it receives to
error_reportf_err().
Current behaviour:
$ ./qemu-img create -f qcow2 -o foo=bar /tmp/test.qcow2 64M
qemu-img: Invalid parameter 'foo'
qemu-img: /tmp/test.qcow2: Invalid options for file format 'qcow2'
Assumed behaviour with your patch (can't test because I don't have
error_propagate_prepend() yet and its addition is not a separate patch):
$ ./qemu-img create -f qcow2 -o foo=bar /tmp/test.qcow2 64M
qemu-img: /tmp/test.qcow2: Invalid options for file format 'qcow2': Invalid
parameter 'foo'
Combining two redundant messages into a single line is nice. Keeping
the redundant information in it ('invalid options'/'invalid parameter')
isn't perfect, though.
If instead of prepending, we just propagate the error, would this
actually lack any important information?
$ ./qemu-img create -f qcow2 -o foo=bar /tmp/test.qcow2 64M
qemu-img: /tmp/test.qcow2: Invalid parameter 'foo'
Kevin
Re: [Qemu-block] [RFC PATCH] iotests: make 083 specific to raw
Am 15.10.2018 um 22:03 hat Cleber Rosa geschrieben: > While testing the Python 3 changes which touch the 083 test, I noticed > that it would fail with qcow2. Expanding the testing, I noticed it > had nothing to do with the Python 3 changes, and in fact, it would not > pass on anything but raw: > > raw: pass > bochs: not generic > cloop: not generic > parallels: fail > qcow: fail > qcow2: fail > qed: fail > vdi: fail > vhdx: fail > vmdk: fail > vpc: fail > luks: fail > > The errors are a mixture I/O and "image not in xxx format", such as: > > === Check disconnect before data === > > Unexpected end-of-file before all bytes were read > -read failed: Input/output error > +can't open device nbd+tcp://127.0.0.1:PORT/foo: Could not open > 'nbd://127.0.0.1:PORT/foo': Input/output error > > === Check disconnect after data === > > -read 512/512 bytes at offset 0 > -512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > +can't open device nbd+tcp://127.0.0.1:PORT/foo: Image not in qcow format > > I'm not aware if there's a quick fix, so, for the time being, it looks > like the honest approach is to make the test known to work on raw > only. > > Signed-off-by: Cleber Rosa Yes, that makes sense to me. Thanks, applied to the block branch. Kevin
[Qemu-block] [PATCH v2] Introduce attributes to qemu timer subsystem
Attributes are simple flags, associated with individual timers for their whole
lifetime.
They intended to be used to mark individual timers for special handling by
various qemu features operating at qemu core level.
New/init functions family in timer interface updated and their comments
improved to avoid info duplication.
Also existing aio interface extended with attribute-enabled variants of
functions, which create/initialize timers.
Signed-off-by: Artem Pisarenko
---
Notes:
v2:
- timer creation/initialize functions reworked and and their unnecessary
variants removed (as Paolo Bonzini suggested)
- also their comments improved to avoid info duplication
include/block/aio.h | 53 ++-
include/qemu/timer.h | 126 ++
tests/ptimer-test-stubs.c | 7 +--
util/qemu-timer.c | 12 +++--
4 files changed, 136 insertions(+), 62 deletions(-)
diff --git a/include/block/aio.h b/include/block/aio.h
index f08630c..07c291a 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -407,10 +407,38 @@ static inline QEMUTimer *aio_timer_new(AioContext *ctx,
QEMUClockType type,
int scale,
QEMUTimerCB *cb, void *opaque)
{
-return timer_new_tl(ctx->tlg.tl[type], scale, cb, opaque);
+return timer_new_full(type, ctx->tlg.tl[type],
+ scale, 0, cb, opaque);
}
/**
+ * aio_timer_new_with_attrs:
+ * @ctx: the aio context
+ * @type: the clock type
+ * @scale: the scale
+ * @attributes: 0, or one to multiple OR'ed QEMU_TIMER_ATTR(id) values to
assign
+ * @cb: the callback to call on timer expiry
+ * @opaque: the opaque pointer to pass to the callback
+ *
+ * Allocate a new timer (with attributes) attached to the context @ctx.
+ * The function is responsible for memory allocation.
+ *
+ * The preferred interface is aio_timer_init. Use that
+ * unless you really need dynamic memory allocation.
+ *
+ * Returns: a pointer to the new timer
+ */
+static inline QEMUTimer *aio_timer_new_with_attrs(AioContext *ctx,
+ QEMUClockType type,
+ int scale, int attributes,
+ QEMUTimerCB *cb, void
*opaque)
+{
+return timer_new_full(type, ctx->tlg.tl[type],
+ scale, attributes, cb, opaque);
+}
+
+
+/**
* aio_timer_init:
* @ctx: the aio context
* @ts: the timer
@@ -427,7 +455,28 @@ static inline void aio_timer_init(AioContext *ctx,
int scale,
QEMUTimerCB *cb, void *opaque)
{
-timer_init_tl(ts, ctx->tlg.tl[type], scale, cb, opaque);
+timer_init_full(ts, ctx->tlg.tl[type], scale, 0, cb, opaque);
+}
+
+/**
+ * aio_timer_init_with_attrs:
+ * @ctx: the aio context
+ * @ts: the timer
+ * @type: the clock type
+ * @scale: the scale
+ * @attributes: 0, or one to multiple OR'ed QEMU_TIMER_ATTR(id) values to
assign
+ * @cb: the callback to call on timer expiry
+ * @opaque: the opaque pointer to pass to the callback
+ *
+ * Initialise a new timer (with attributes) attached to the context @ctx.
+ * The caller is responsible for memory allocation.
+ */
+static inline void aio_timer_init_with_attrs(AioContext *ctx,
+ QEMUTimer *ts, QEMUClockType type,
+ int scale, int attributes,
+ QEMUTimerCB *cb, void *opaque)
+{
+timer_init_full(ts, ctx->tlg.tl[type], scale, attributes, cb, opaque);
}
/**
diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index 39ea907..64a84ea 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -52,6 +52,28 @@ typedef enum {
QEMU_CLOCK_MAX
} QEMUClockType;
+/**
+ * QEMU Timer attributes:
+ *
+ * An individual timer may be assigned with one or multiple attributes when
+ * initialized.
+ * Attribute is a static flag, meaning that timer has corresponding property.
+ * Attributes are defined in QEMUTimerAttrBit enum and encoded to bit set,
+ * which used to initialize timer, stored to 'attributes' member and can be
+ * retrieved externally with timer_get_attributes() call.
+ * Values of QEMUTimerAttrBit aren't used directly,
+ * instead each attribute in bit set accessed with QEMU_TIMER_ATTR_ macro,
+ * where is a unique part of attribute identifier.
+ *
+ * No attributes defined currently.
+ */
+
+typedef enum {
+QEMU_TIMER_ATTRBIT__NONE
+} QEMUTimerAttrBit;
+
+#define QEMU_TIMER_ATTR__NONE (1 << QEMU_TIMER_ATTRBIT__NONE)
+
typedef struct QEMUTimerList QEMUTimerList;
struct QEMUTimerListGroup {
@@ -67,6 +89,7 @@ struct QEMUTimer {
QEMUTimerCB *cb;
void *opaque;
QEMUTimer *next;
+int attributes;
int scale;
};
@@ -418,22 +441,24 @@ int64_t timerlistgroup_deadline_ns(QEMUTimerLi
[Qemu-block] [PATCH] migration/block-dirty-bitmap: fix Coverity CID1390625
Theoretically possible that we finish the skipping loop with bs = NULL
and the following code will crash trying to dereference it. Fix that.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
migration/block-dirty-bitmap.c | 4
1 file changed, 4 insertions(+)
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 477826330c..6de808f95f 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -288,6 +288,10 @@ static int init_dirty_bitmap_migration(void)
bs = backing_bs(bs);
}
+if (!bs || bs->implicit) {
+continue;
+}
+
for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap;
bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
{
--
2.18.0
Re: [Qemu-block] [PATCH v2] Introduce attributes to qemu timer subsystem
On 16/10/2018 15:03, Artem Pisarenko wrote:
> +if (!timer_list) {
> +timer_list = main_loop_tlg.tl[type];
> +}
> +timer_init_full(ts, timer_list, scale, attributes, cb, opaque);
Please move this "if" to timer_init_full, so that here you can just pass
timer_list. timer_init_full will then take a QEMUTimerListGroup* (NULL
defaults to &main_loop_tlg, aio_timer_new passes &ctx->tlg) and a
QEMUClockType instead of a QEMUTimerList*.
Paolo
Re: [Qemu-block] [PATCH v2 3/8] block: Require auto-read-only for existing fallbacks
Am 12.10.2018 um 19:02 hat Eric Blake geschrieben:
> On 10/12/18 6:55 AM, Kevin Wolf wrote:
> > Some block drivers have traditionally changed their node to read-only
> > mode without asking the user. This behaviour has been marked deprecated
> > since 2.11, expecting users to provide an explicit read-only=on option.
> >
> > Now that we have auto-read-only=on, enable these drivers to make use of
> > the option.
> >
> > This is the only use of bdrv_set_read_only(), so we can make it a bit
> > more specific and turn it into a bdrv_apply_auto_read_only() that is
> > more convenient for drivers to use.
> >
> > Signed-off-by: Kevin Wolf
> > ---
>
> > +++ b/block.c
> > @@ -266,27 +266,36 @@ int bdrv_can_set_read_only(BlockDriverState *bs, bool
> > read_only,
> > return 0;
> > }
> > -/* TODO Remove (deprecated since 2.11)
> > - * Block drivers are not supposed to automatically change bs->read_only.
> > - * Instead, they should just check whether they can provide what the user
> > - * explicitly requested and error out if read-write is requested, but they
> > can
> > - * only provide read-only access. */
> > -int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> > +/*
> > + * Called by a driver that can only provide a read-only image.
> > + *
> > + * Returns 0 if the node is already read-only or it could switch the node
> > to
> > + * read-only because BDRV_O_AUTO_RDONLY is set.
> > + *
> > + * Returns -EACCES if the node is read-write and BDRV_O_AUTO_RDONLY is not
> > set.
> > + * If @errmsg is not NULL, it is used as the error message for the Error
> > + * object.
>
> I like it.
>
> Worth documenting the -EINVAL (copy-on-read prevents setting read-only)
> failure as well? (The -EPERM failure of bdrv_can_set_read_only() is not
> reachable, since this new function never clears readonly).
In fact, -EINVAL and the error string from bdrv_can_set_read_only() may
be confusing because the user didn't explicitly request a read-only
image. Maybe it would be better to just turn this case into -EACCES with
the same error message.
What do you think?
> > + */
> > +int bdrv_apply_auto_read_only(BlockDriverState *bs, const char *errmsg,
> > + Error **errp)
> > {
> > int ret = 0;
> > -ret = bdrv_can_set_read_only(bs, read_only, false, errp);
> > +if (!(bs->open_flags & BDRV_O_RDWR)) {
> > +return 0;
> > +}
> > +if (!(bs->open_flags & BDRV_O_AUTO_RDONLY)) {
> > +error_setg(errp, "%s", errmsg ?: "Image is read-only");
> > +return -EACCES;
> > +}
> > +
> > +ret = bdrv_can_set_read_only(bs, true, false, errp);
> > if (ret < 0) {
> > return ret;
> > }
>
> Makes sense.
>
> > +++ b/block/vvfat.c
> > @@ -1262,16 +1262,10 @@ static int vvfat_open(BlockDriverState *bs, QDict
> > *options, int flags,
> > "Unable to set VVFAT to 'rw' when drive is
> > read-only");
> > goto fail;
> > }
> > -} else if (!bdrv_is_read_only(bs)) {
> > -error_report("Opening non-rw vvfat images without an explicit "
> > - "read-only=on option is deprecated. Future versions "
> > - "will refuse to open the image instead of "
> > - "automatically marking the image read-only.");
> > -/* read only is the default for safety */
> > -ret = bdrv_set_read_only(bs, true, &local_err);
> > +} else {
> > +ret = bdrv_apply_auto_read_only(bs, NULL, errp);
> > if (ret < 0) {
> > -error_propagate(errp, local_err);
> > -goto fail;
> > +return ret;
>
> Don't you still need the goto fail, to avoid leaking opts?
Yes, I do. Thanks.
Kevin
Re: [Qemu-block] [Qemu-devel] [RFC PATCH] block: Clean up bdrv_img_create()'s error reporting
Kevin Wolf writes:
> Am 16.10.2018 um 08:41 hat Markus Armbruster geschrieben:
>> bdrv_img_create() takes an Error ** argument and used it in the
>> conventional way, except for one place: when qemu_opts_do_parse()
>> fails, it first reports its error to stderr or the HMP monitor with
>> error_report_err(), then error_setg()'s a generic error. When the
>> caller reports that second error similarly, this produces two
>> consecutive error messages on stderr or the HMP monitor. When the
>> caller does something else with it, such as send it via QMP, the first
>> error still goes to stderr or the HMP monitor. Not good.
>>
>> Turn the first message into a prefix for the second.
>>
>> Signed-off-by: Markus Armbruster
>> ---
>>
>> This is RFC because I didn't check whether "caller does something else
>> with it" can actually happen with current code, and I'm not sure the
>> prefix is wanted. I hope we'll answer both questions during review.
>
> The only caller that passes non-NULL options and can therefore even run
> into this error path is qemu-img.c. It passes any Error it receives to
> error_reportf_err().
>
> Current behaviour:
>
> $ ./qemu-img create -f qcow2 -o foo=bar /tmp/test.qcow2 64M
> qemu-img: Invalid parameter 'foo'
> qemu-img: /tmp/test.qcow2: Invalid options for file format 'qcow2'
>
> Assumed behaviour with your patch (can't test because I don't have
> error_propagate_prepend() yet and its addition is not a separate patch):
Sorry, forgot
Based-on: <20181015115309.17089-1-arm...@redhat.com>
Don't bother testing, my patch is buggy.
> $ ./qemu-img create -f qcow2 -o foo=bar /tmp/test.qcow2 64M
> qemu-img: /tmp/test.qcow2: Invalid options for file format 'qcow2':
> Invalid parameter 'foo'
>
> Combining two redundant messages into a single line is nice. Keeping
> the redundant information in it ('invalid options'/'invalid parameter')
> isn't perfect, though.
>
> If instead of prepending, we just propagate the error, would this
> actually lack any important information?
>
> $ ./qemu-img create -f qcow2 -o foo=bar /tmp/test.qcow2 64M
> qemu-img: /tmp/test.qcow2: Invalid parameter 'foo'
Works for me. Thanks!
[Qemu-block] [PATCH] block/vhdx: Don't take address of fields in packed structs
Taking the address of a field in a packed struct is a bad idea, because
it might not be actually aligned enough for that pointer type (and
thus cause a crash on dereference on some host architectures). Newer
versions of clang warn about this. Avoid the bug by not using the
"modify in place" byte swapping functions.
There are a few places where the in-place swap function is
used on something other than a packed struct field; we convert
those anyway, for consistency.
Patch produced with scripts/coccinelle/inplace-byteswaps.cocci.
Signed-off-by: Peter Maydell
---
Usual disclaimer: produced with "make check" only, but purely
automated conversion should be safe.
block/vhdx.h| 12 ++---
block/vhdx-endian.c | 118 ++--
block/vhdx-log.c| 4 +-
block/vhdx.c| 18 +++
4 files changed, 76 insertions(+), 76 deletions(-)
diff --git a/block/vhdx.h b/block/vhdx.h
index 7003ab7a795..3a5f5293adc 100644
--- a/block/vhdx.h
+++ b/block/vhdx.h
@@ -420,16 +420,16 @@ int vhdx_log_write_and_flush(BlockDriverState *bs,
BDRVVHDXState *s,
static inline void leguid_to_cpus(MSGUID *guid)
{
-le32_to_cpus(&guid->data1);
-le16_to_cpus(&guid->data2);
-le16_to_cpus(&guid->data3);
+guid->data1 = le32_to_cpu(guid->data1);
+guid->data2 = le16_to_cpu(guid->data2);
+guid->data3 = le16_to_cpu(guid->data3);
}
static inline void cpu_to_leguids(MSGUID *guid)
{
-cpu_to_le32s(&guid->data1);
-cpu_to_le16s(&guid->data2);
-cpu_to_le16s(&guid->data3);
+guid->data1 = cpu_to_le32(guid->data1);
+guid->data2 = cpu_to_le16(guid->data2);
+guid->data3 = cpu_to_le16(guid->data3);
}
void vhdx_header_le_import(VHDXHeader *h);
diff --git a/block/vhdx-endian.c b/block/vhdx-endian.c
index 41fbdd2b8f0..ebfa33cb8a4 100644
--- a/block/vhdx-endian.c
+++ b/block/vhdx-endian.c
@@ -35,18 +35,18 @@ void vhdx_header_le_import(VHDXHeader *h)
{
assert(h != NULL);
-le32_to_cpus(&h->signature);
-le32_to_cpus(&h->checksum);
-le64_to_cpus(&h->sequence_number);
+h->signature = le32_to_cpu(h->signature);
+h->checksum = le32_to_cpu(h->checksum);
+h->sequence_number = le64_to_cpu(h->sequence_number);
leguid_to_cpus(&h->file_write_guid);
leguid_to_cpus(&h->data_write_guid);
leguid_to_cpus(&h->log_guid);
-le16_to_cpus(&h->log_version);
-le16_to_cpus(&h->version);
-le32_to_cpus(&h->log_length);
-le64_to_cpus(&h->log_offset);
+h->log_version = le16_to_cpu(h->log_version);
+h->version = le16_to_cpu(h->version);
+h->log_length = le32_to_cpu(h->log_length);
+h->log_offset = le64_to_cpu(h->log_offset);
}
void vhdx_header_le_export(VHDXHeader *orig_h, VHDXHeader *new_h)
@@ -80,68 +80,68 @@ void vhdx_log_desc_le_import(VHDXLogDescriptor *d)
{
assert(d != NULL);
-le32_to_cpus(&d->signature);
-le64_to_cpus(&d->file_offset);
-le64_to_cpus(&d->sequence_number);
+d->signature = le32_to_cpu(d->signature);
+d->file_offset = le64_to_cpu(d->file_offset);
+d->sequence_number = le64_to_cpu(d->sequence_number);
}
void vhdx_log_desc_le_export(VHDXLogDescriptor *d)
{
assert(d != NULL);
-cpu_to_le32s(&d->signature);
-cpu_to_le32s(&d->trailing_bytes);
-cpu_to_le64s(&d->leading_bytes);
-cpu_to_le64s(&d->file_offset);
-cpu_to_le64s(&d->sequence_number);
+d->signature = cpu_to_le32(d->signature);
+d->trailing_bytes = cpu_to_le32(d->trailing_bytes);
+d->leading_bytes = cpu_to_le64(d->leading_bytes);
+d->file_offset = cpu_to_le64(d->file_offset);
+d->sequence_number = cpu_to_le64(d->sequence_number);
}
void vhdx_log_data_le_import(VHDXLogDataSector *d)
{
assert(d != NULL);
-le32_to_cpus(&d->data_signature);
-le32_to_cpus(&d->sequence_high);
-le32_to_cpus(&d->sequence_low);
+d->data_signature = le32_to_cpu(d->data_signature);
+d->sequence_high = le32_to_cpu(d->sequence_high);
+d->sequence_low = le32_to_cpu(d->sequence_low);
}
void vhdx_log_data_le_export(VHDXLogDataSector *d)
{
assert(d != NULL);
-cpu_to_le32s(&d->data_signature);
-cpu_to_le32s(&d->sequence_high);
-cpu_to_le32s(&d->sequence_low);
+d->data_signature = cpu_to_le32(d->data_signature);
+d->sequence_high = cpu_to_le32(d->sequence_high);
+d->sequence_low = cpu_to_le32(d->sequence_low);
}
void vhdx_log_entry_hdr_le_import(VHDXLogEntryHeader *hdr)
{
assert(hdr != NULL);
-le32_to_cpus(&hdr->signature);
-le32_to_cpus(&hdr->checksum);
-le32_to_cpus(&hdr->entry_length);
-le32_to_cpus(&hdr->tail);
-le64_to_cpus(&hdr->sequence_number);
-le32_to_cpus(&hdr->descriptor_count);
+hdr->signature = le32_to_cpu(hdr->signature);
+hdr->checksum = le32_to_cpu(hdr->checksum);
+hdr->entry_length = le32_to_cpu(hdr->entry_length);
+hdr->tail = le32_to_cpu(hdr->tail);
+hdr->sequence_number = le64_to_cpu(hdr->sequence_number);
+hdr->descriptor_count
[Qemu-block] [PATCH] block/vdi: Don't take address of fields in packed structs
Taking the address of a field in a packed struct is a bad idea, because
it might not be actually aligned enough for that pointer type (and
thus cause a crash on dereference on some host architectures). Newer
versions of clang warn about this. Avoid the bug by not using the
"modify in place" byte swapping functions.
There are a few places where the in-place swap function is
used on something other than a packed struct field; we convert
those anyway, for consistency.
Patch produced with scripts/coccinelle/inplace-byteswaps.cocci.
There are other places where we take the address of a packed member
in this file for other purposes than passing it to a byteswap
function (all the calls to qemu_uuid_*()); we leave those for now.
Signed-off-by: Peter Maydell
---
Another "tested with make check" auto-conversion patch. In this
case, as noted above, it doesn't fix all the warnings for the file,
but we might as well put the easy part of the fix in. I'm not sure
what to do with the qemu_uuid_*() calls. Something like
QemuUUID uuid_link = header->uuid_link;
and then using "qemu_uuid_is_null(uuid_link)" rather than
"qemu_uuid_is_null(&header.uuid_link)" ?
Or we could macroise the relevant qemu_uuid functions (notably
qemu_uuid_bswap()) or turn them into functions taking a QemuUUID
rather than a QemuUUID*. Suggestions ?
block/vdi.c | 64 ++---
1 file changed, 32 insertions(+), 32 deletions(-)
diff --git a/block/vdi.c b/block/vdi.c
index 6555cffb886..0ff1ead736c 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -187,22 +187,22 @@ typedef struct {
static void vdi_header_to_cpu(VdiHeader *header)
{
-le32_to_cpus(&header->signature);
-le32_to_cpus(&header->version);
-le32_to_cpus(&header->header_size);
-le32_to_cpus(&header->image_type);
-le32_to_cpus(&header->image_flags);
-le32_to_cpus(&header->offset_bmap);
-le32_to_cpus(&header->offset_data);
-le32_to_cpus(&header->cylinders);
-le32_to_cpus(&header->heads);
-le32_to_cpus(&header->sectors);
-le32_to_cpus(&header->sector_size);
-le64_to_cpus(&header->disk_size);
-le32_to_cpus(&header->block_size);
-le32_to_cpus(&header->block_extra);
-le32_to_cpus(&header->blocks_in_image);
-le32_to_cpus(&header->blocks_allocated);
+header->signature = le32_to_cpu(header->signature);
+header->version = le32_to_cpu(header->version);
+header->header_size = le32_to_cpu(header->header_size);
+header->image_type = le32_to_cpu(header->image_type);
+header->image_flags = le32_to_cpu(header->image_flags);
+header->offset_bmap = le32_to_cpu(header->offset_bmap);
+header->offset_data = le32_to_cpu(header->offset_data);
+header->cylinders = le32_to_cpu(header->cylinders);
+header->heads = le32_to_cpu(header->heads);
+header->sectors = le32_to_cpu(header->sectors);
+header->sector_size = le32_to_cpu(header->sector_size);
+header->disk_size = le64_to_cpu(header->disk_size);
+header->block_size = le32_to_cpu(header->block_size);
+header->block_extra = le32_to_cpu(header->block_extra);
+header->blocks_in_image = le32_to_cpu(header->blocks_in_image);
+header->blocks_allocated = le32_to_cpu(header->blocks_allocated);
qemu_uuid_bswap(&header->uuid_image);
qemu_uuid_bswap(&header->uuid_last_snap);
qemu_uuid_bswap(&header->uuid_link);
@@ -211,22 +211,22 @@ static void vdi_header_to_cpu(VdiHeader *header)
static void vdi_header_to_le(VdiHeader *header)
{
-cpu_to_le32s(&header->signature);
-cpu_to_le32s(&header->version);
-cpu_to_le32s(&header->header_size);
-cpu_to_le32s(&header->image_type);
-cpu_to_le32s(&header->image_flags);
-cpu_to_le32s(&header->offset_bmap);
-cpu_to_le32s(&header->offset_data);
-cpu_to_le32s(&header->cylinders);
-cpu_to_le32s(&header->heads);
-cpu_to_le32s(&header->sectors);
-cpu_to_le32s(&header->sector_size);
-cpu_to_le64s(&header->disk_size);
-cpu_to_le32s(&header->block_size);
-cpu_to_le32s(&header->block_extra);
-cpu_to_le32s(&header->blocks_in_image);
-cpu_to_le32s(&header->blocks_allocated);
+header->signature = cpu_to_le32(header->signature);
+header->version = cpu_to_le32(header->version);
+header->header_size = cpu_to_le32(header->header_size);
+header->image_type = cpu_to_le32(header->image_type);
+header->image_flags = cpu_to_le32(header->image_flags);
+header->offset_bmap = cpu_to_le32(header->offset_bmap);
+header->offset_data = cpu_to_le32(header->offset_data);
+header->cylinders = cpu_to_le32(header->cylinders);
+header->heads = cpu_to_le32(header->heads);
+header->sectors = cpu_to_le32(header->sectors);
+header->sector_size = cpu_to_le32(header->sector_size);
+header->disk_size = cpu_to_le64(header->disk_size);
+header->block_size = cpu_to_le32(header->block_size);
+header->block_extra = cpu_to_le32(header->block_extra);
+header->blocks_in_image
Re: [Qemu-block] [Qemu-devel] [PATCH 1/9] iotests: Make nbd-fault-injector flush
On 10/15/18 9:14 AM, Max Reitz wrote:
When closing a connection, make the nbd-fault-injector flush the socket.
Without this, the output is a bit unreliable with Python 3.
Signed-off-by: Max Reitz
---
tests/qemu-iotests/083.out | 9 +
tests/qemu-iotests/nbd-fault-injector.py | 1 +
2 files changed, 10 insertions(+)
I already had a complaint that the error message in 083.out should NOT
be printing a message (whether the server is python 2 and auto-flushes,
or python 3 and needs an explicit flush, the message itself is
pointless, and the test is racy as a result). We may need to revisit
this patch when that thread is resolved.
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg01041.html
That said, I'm not opposed to this patch, if it gets iotests to be more
useful in the meantime.
diff --git a/tests/qemu-iotests/083.out b/tests/qemu-iotests/083.out
index be6079d27e..f9af8bb691 100644
--- a/tests/qemu-iotests/083.out
+++ b/tests/qemu-iotests/083.out
@@ -41,6 +41,7 @@ can't open device nbd+tcp://127.0.0.1:PORT/foo
=== Check disconnect after neg2 ===
+Unable to read from socket: Connection reset by peer
Connection closed
read failed: Input/output error
@@ -54,6 +55,7 @@ can't open device nbd+tcp://127.0.0.1:PORT/foo
=== Check disconnect before request ===
+Unable to read from socket: Connection reset by peer
Connection closed
read failed: Input/output error
@@ -116,6 +118,7 @@ can't open device nbd+tcp://127.0.0.1:PORT/
=== Check disconnect after neg-classic ===
+Unable to read from socket: Connection reset by peer
Connection closed
read failed: Input/output error
@@ -161,6 +164,8 @@ can't open device nbd+unix:///foo?socket=TEST_DIR/nbd.sock
=== Check disconnect after neg2 ===
+Unable to read from socket: Connection reset by peer
+Connection closed
read failed: Input/output error
=== Check disconnect 8 neg2 ===
@@ -173,6 +178,8 @@ can't open device nbd+unix:///foo?socket=TEST_DIR/nbd.sock
=== Check disconnect before request ===
+Unable to read from socket: Connection reset by peer
+Connection closed
read failed: Input/output error
=== Check disconnect after request ===
@@ -234,6 +241,8 @@ can't open device nbd+unix:///?socket=TEST_DIR/nbd.sock
=== Check disconnect after neg-classic ===
+Unable to read from socket: Connection reset by peer
+Connection closed
read failed: Input/output error
*** done
diff --git a/tests/qemu-iotests/nbd-fault-injector.py
b/tests/qemu-iotests/nbd-fault-injector.py
index f9193c0fae..439a090eb6 100755
--- a/tests/qemu-iotests/nbd-fault-injector.py
+++ b/tests/qemu-iotests/nbd-fault-injector.py
@@ -112,6 +112,7 @@ class FaultInjectionSocket(object):
if rule.match(event, io):
if rule.when == 0 or bufsize is None:
print('Closing connection on rule match %s' % rule.name)
+self.sock.flush()
sys.exit(0)
if rule.when != -1:
return rule.when
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [Qemu-block] [PATCH v2 2/8] block: Add auto-read-only option
On 10/15/18 4:37 AM, Kevin Wolf wrote:
Am 12.10.2018 um 18:47 hat Eric Blake geschrieben:
On 10/12/18 6:55 AM, Kevin Wolf wrote:
If a management application builds the block graph node by node, the
protocol layer doesn't inherit its read-only option from the format
layer any more, so it must be set explicitly.
Bike-shedding: Do we really want to ignore @read-only? Here's the table of 9
combinations ('t'rue, 'f'alse, 'o'mitted), with '*' on the rows that must be
preserved for back-compat:
RO Auto effect
oo *open for write, fail if not possible
fo *open for write, fail if not possible
to *open for read, no conversion to write
of open for write, fail if not possible
ff open for write, fail if not possible
tf open for read, no conversion to write
ot attempt write but graceful fall back to read
ft attempt write but graceful fall back to read
tt ignore RO flag, attempt write anyway
That last row is weird, why not make it an explicit error instead of
ignoring the implied difference in semantics between the two?
You're right that the description allows this. In practice,
auto-read-only can only make a node go from rw to ro, not the other way
round.
So our options are to document the current behaviour (auto-read-only has
no effect when the image is already read-only) or to make it an error.
Ah, that's different. I was reading it as "auto-read-only true lets you
write if possible, overriding an explicit readonly request", while you
are reading it as "auto-read-only true allows graceful fallback to
read-only, and is thus a no-op if you already requested readonly"
I like yours better, so it's just a matter of coming up with the correct
documentation wording.
One thought I had is that for convenience options like -hda (or in fact
-drive), auto-read-only=on could be the default, and only -blockdev and
blockdev-add would disable it by default. That would suggest that we
don't want to make it an error.
Yes, having convenience options set auto-read-only would not be too
terrible (since those are already magic and designed for short-hand
human use), as long as the low-level QMP commands don't add the magic
(explicit control is better at the low levels).
Or, another idea: is it worth trying to support a single tri-state member
(via an alternative between bool and enum, since the existing code uses a
JSON bool):
"read-only": false (open for write, fail if not possible)
"read-only": true (open read-only, no later switching)
"read-only": "auto" (switch as needed; or for initial implementation attempt
for write with graceful fallback to read)
omitting read-only: same as "read-only":false for back-compat
If read-only were new, I would probably make it an enum, but adding it
now isn't very practical. I did actually start with an alternate and it
just wasn't very nice. One thing I remember is places that directly
accessed the options QDict, for which you could now have either a bool, a
string, an int or not present. It becomes a bit too much.
Fair enough. Maybe it's worth a commit message note that we at least
considered and rejected alternate implementations.
As read-only is optional, we could make it true/false/absent without
introducing an alternate and the additional int/string options, but I
don't like that very much either.
No, that way is not introspectible. Adding auto-read-only is much
friendlier.
While we're talking about the schema, another thing I considered was
making auto-read-only an option only for the specific drivers that
support it so introspection could tell the management tool whether the
functionality is available. However, if we do this, we can't parse it in
block.c code and use a flag any more, but need to parse it in each
driver individually. Maybe it would be a better design anyway?
Which drivers do you have in mind? Ones like file-posix, gluster, and
NBD that actually have a notion of opening either read-write or
read-only, or others that are read-only no matter what?
I'm still not convinced that a per-driver option is smart, and am
reasonably happy with you adding it globally.
@@ -1328,6 +1338,11 @@ QemuOptsList bdrv_runtime_opts = {
.type = QEMU_OPT_BOOL,
.help = "Node is opened in read-only mode",
},
+{
+.name = BDRV_OPT_AUTO_READ_ONLY,
+.type = QEMU_OPT_BOOL,
+.help = "Node can become read-only if opening read-write fails",
+},
If we keep your current approach, is it worth mentioning that
auto-read-only true overrides read-only true?
This help text is never printed anywhere anyway... Maybe we should just
delete it. What we refer to is the QAPI documentation anyway.
Are you sure it never gets printed, with some of the recent patches
around trying to improve help output?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Re: [Qemu-block] [PATCH v2 3/8] block: Require auto-read-only for existing fallbacks
On 10/16/18 9:12 AM, Kevin Wolf wrote: Am 12.10.2018 um 19:02 hat Eric Blake geschrieben: On 10/12/18 6:55 AM, Kevin Wolf wrote: Some block drivers have traditionally changed their node to read-only mode without asking the user. This behaviour has been marked deprecated since 2.11, expecting users to provide an explicit read-only=on option. Now that we have auto-read-only=on, enable these drivers to make use of the option. This is the only use of bdrv_set_read_only(), so we can make it a bit more specific and turn it into a bdrv_apply_auto_read_only() that is more convenient for drivers to use. Signed-off-by: Kevin Wolf --- Worth documenting the -EINVAL (copy-on-read prevents setting read-only) failure as well? (The -EPERM failure of bdrv_can_set_read_only() is not reachable, since this new function never clears readonly). In fact, -EINVAL and the error string from bdrv_can_set_read_only() may be confusing because the user didn't explicitly request a read-only image. Maybe it would be better to just turn this case into -EACCES with the same error message. What do you think? So, how would it trigger in practice? The user requests a copy-on-read action with the BDS as destination (thus the BDS must be writable, and can't be set to readonly); they omitted read-only (because they know they want copy-on-read); they supplied auto-read-only=true (because they are lazy and want to always use that flag if it is available); but the particular BDS they selected is not writable (whether read-only file system, read-only NBD server, etc). In short, we can't grant them read-write to begin with, and can't gracefully fall back to read-only because it would violate their request for copy-on-read, so as long as we give them a sane error message about their request being impossible, we're good. Yes, -EACCES sounds reasonable, if you want to code that in. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-block] [Qemu-devel] [PATCH 1/5] option: Make option help nicer to read
Max Reitz writes: > This adds some whitespace into the option help (including indentation) > and replaces '=' by ': ' (not least because '=' should be used for > values, not types). Furthermore, the list name is no longer printed as > part of every line, but only once in advance, and only if the caller did > not print a caption already. > > Signed-off-by: Max Reitz > --- > include/qemu/option.h | 2 +- > qemu-img.c | 4 +- > util/qemu-option.c | 31 +- > tests/qemu-iotests/082.out | 956 ++--- > 4 files changed, 505 insertions(+), 488 deletions(-) 082 fails for me in master (dddb37495b8). If this patch fixes the failure, its commit message should mention it. 091 and 142 also fail, but look unrelated.
