Re: [PATCH 2/4] migration: Add block-bitmap-mapping parameter
On 01.07.20 16:34, Vladimir Sementsov-Ogievskiy wrote: > 30.06.2020 11:45, Max Reitz wrote: >> This migration parameter allows mapping block node names and bitmap >> names to aliases for the purpose of block dirty bitmap migration. >> >> This way, management tools can use different node and bitmap names on >> the source and destination and pass the mapping of how bitmaps are to be >> transferred to qemu (on the source, the destination, or even both with >> arbitrary aliases in the migration stream). >> >> Suggested-by: Vladimir Sementsov-Ogievskiy >> Signed-off-by: Max Reitz >> --- >> qapi/migration.json | 83 +++- >> migration/migration.h | 3 + >> migration/block-dirty-bitmap.c | 372 - >> migration/migration.c | 29 +++ >> 4 files changed, 432 insertions(+), 55 deletions(-) >> >> diff --git a/qapi/migration.json b/qapi/migration.json >> index d5000558c6..5aeae9bea8 100644 >> --- a/qapi/migration.json >> +++ b/qapi/migration.json >> @@ -507,6 +507,44 @@ >> 'data': [ 'none', 'zlib', >> { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] } >> +## >> +# @BitmapMigrationBitmapAlias: >> +# >> +# @name: The name of the bitmap. >> +# >> +# @alias: An alias name for migration (for example the bitmap name on >> +# the opposite site). >> +# >> +# Since: 5.1 >> +## >> +{ 'struct': 'BitmapMigrationBitmapAlias', >> + 'data': { >> + 'name': 'str', >> + 'alias': 'str' >> + } } >> + >> +## >> +# @BitmapMigrationNodeAlias: >> +# >> +# Maps a block node name and the bitmaps it has to aliases for dirty >> +# bitmap migration. >> +# >> +# @node-name: A block node name. >> +# >> +# @alias: An alias block node name for migration (for example the >> +# node name on the opposite site). >> +# >> +# @bitmaps: Mappings for the bitmaps on this node. >> +# >> +# Since: 5.1 >> +## >> +{ 'struct': 'BitmapMigrationNodeAlias', >> + 'data': { >> + 'node-name': 'str', >> + 'alias': 'str', >> + 'bitmaps': [ 'BitmapMigrationBitmapAlias' ] > > So, we still can't migrate bitmaps from one node to different nodes, but we > also don't know a usecase for it, so it seems OK. But with such scheme we > can select and rename bitmaps in-flight, and Peter said about corresponding > use-case. > > I'm OK with this, still, just an idea in my mind: > > we could instead just have a list of > > BitmapMigrationAlias: { > node-name > bitmap-name > node-alias > bitmap-alias > } > > so, mapping is set for each bitmap in separate. Well, OK, but why? >> + } } >> + >> ## >> # @MigrationParameter: >> # >> @@ -641,6 +679,18 @@ >> # will consume more CPU. >> # Defaults to 1. (Since 5.0) >> # >> +# @block-bitmap-mapping: Maps block nodes and bitmaps on them to >> +# aliases for the purpose of dirty bitmap migration. Such >> +# aliases may for example be the corresponding names on the >> +# opposite site. >> +# The mapping must be one-to-one and complete: On the source, > > I've recently faced the case where incomplete mapping make sence: shared > migration of read-only bitmaps in backing files: it seems better to not > migrate them through migration channel, they are migrating through > shared storage automatically (yes, we'll pay the time to load them on > destination, but I'm going to improve it by implementing lazy load of > read-only and disabled bitmaps, so this will not be a problem). > > So, now I think that it would be good just ignore all the bitmap not > described by mapping OK. >> +# migrating a bitmap from a node when either is not mapped >> +# will result in an error. On the destination, similarly, >> +# receiving a bitmap (by alias) from a node (by alias) when >> +# either alias is not mapped will result in an error. >> +# By default, all node names and bitmap names are mapped to >> +# themselves. (Since 5.1) > > This sentense is unclear, want means "by default" - if the mapping is > not specified at all or if some nodes/bitmaps are not covered. It is clear. > Still, > tha latter will conflict with previous sentencies, so "by default" is > about when mapping is not set at all. Precisely. > I suggest to word it directly: > "If mapping is not set (the command never called, or mapping was > removed". The mapping cannot be removed. It’s also technically clear because mentioning a default for some parameter always means that that particular parameter will have that default. So in this case “by default” refers to block-bitmap-mapping, not anything nested in it. (Defaults for stuff nested in its value would be described in the respective structs’ definitions.) I’d be OK with “By default (when this parameter has never been set)…”. > And, actual behavior without mapping is not as simple: it actually tries > to use blk names if possible and node-names if not, and this veries > from
Re: [PATCH 1/4] migration: Prevent memleak by ...params_test_apply
On 01.07.20 16:38, Eric Blake wrote: > On 6/30/20 3:45 AM, Max Reitz wrote: >> The created structure is not really a proper QAPI object, so we cannot >> and will not free its members. Strings therein should therefore not be >> duplicated, or we will leak them. > > This seems fragile to me; having to code QAPI usage differently > depending on whether the containing struct was malloc'd or not (and > therefore whether someone will call qapi_free_MigrateSetParameters or > not) looks awkward to maintain. I don’t think that’s the point. The point is that it’s just a temporary object to run some check function on. This is a very... special use case. > We have > visit_type_MigrateSetParameters_members, could that be used as a cleaner > way to free all members of the struct without freeing the struct itself? > Should the QAPI generator start generating qapi_free_FOO_members to > make such cleanup easier? The whole code is a mess, in my opinion. The real question is why don’t we just drop migrate_params_test_apply() and let qmp_migrate_set_parameters() invoke migrate_params_check() directly on @params. I think there was some reason why I didn’t do that, but unfortunately I don’t remember it off the top of my head (if there was a reason). In any case, I don’t think any of this is the QAPI generator’s fault. >> Signed-off-by: Max Reitz >> --- >> migration/migration.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/migration/migration.c b/migration/migration.c >> index 481a590f72..47c7da4e55 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -1336,12 +1336,12 @@ static void >> migrate_params_test_apply(MigrateSetParameters *params, >> if (params->has_tls_creds) { >> assert(params->tls_creds->type == QTYPE_QSTRING); >> - dest->tls_creds = g_strdup(params->tls_creds->u.s); >> + dest->tls_creds = params->tls_creds->u.s; >> } >> if (params->has_tls_hostname) { >> assert(params->tls_hostname->type == QTYPE_QSTRING); >> - dest->tls_hostname = g_strdup(params->tls_hostname->u.s); >> + dest->tls_hostname = params->tls_hostname->u.s; >> } >> if (params->has_max_bandwidth) { >> > signature.asc Description: OpenPGP digital signature
Re: [PATCH 2/4] migration: Add block-bitmap-mapping parameter
02.07.2020 11:09, Max Reitz wrote: On 01.07.20 16:34, Vladimir Sementsov-Ogievskiy wrote: 30.06.2020 11:45, Max Reitz wrote: This migration parameter allows mapping block node names and bitmap names to aliases for the purpose of block dirty bitmap migration. This way, management tools can use different node and bitmap names on the source and destination and pass the mapping of how bitmaps are to be transferred to qemu (on the source, the destination, or even both with arbitrary aliases in the migration stream). Suggested-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Max Reitz --- qapi/migration.json | 83 +++- migration/migration.h | 3 + migration/block-dirty-bitmap.c | 372 - migration/migration.c | 29 +++ 4 files changed, 432 insertions(+), 55 deletions(-) diff --git a/qapi/migration.json b/qapi/migration.json index d5000558c6..5aeae9bea8 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -507,6 +507,44 @@ 'data': [ 'none', 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] } +## +# @BitmapMigrationBitmapAlias: +# +# @name: The name of the bitmap. +# +# @alias: An alias name for migration (for example the bitmap name on +# the opposite site). +# +# Since: 5.1 +## +{ 'struct': 'BitmapMigrationBitmapAlias', + 'data': { + 'name': 'str', + 'alias': 'str' + } } + +## +# @BitmapMigrationNodeAlias: +# +# Maps a block node name and the bitmaps it has to aliases for dirty +# bitmap migration. +# +# @node-name: A block node name. +# +# @alias: An alias block node name for migration (for example the +# node name on the opposite site). +# +# @bitmaps: Mappings for the bitmaps on this node. +# +# Since: 5.1 +## +{ 'struct': 'BitmapMigrationNodeAlias', + 'data': { + 'node-name': 'str', + 'alias': 'str', + 'bitmaps': [ 'BitmapMigrationBitmapAlias' ] So, we still can't migrate bitmaps from one node to different nodes, but we also don't know a usecase for it, so it seems OK. But with such scheme we can select and rename bitmaps in-flight, and Peter said about corresponding use-case. I'm OK with this, still, just an idea in my mind: we could instead just have a list of BitmapMigrationAlias: { node-name bitmap-name node-alias bitmap-alias } so, mapping is set for each bitmap in separate. Well, OK, but why? But why not :) Just thinking out loud. May be someone will imaging good reasons for it. + } } + ## # @MigrationParameter: # @@ -641,6 +679,18 @@ # will consume more CPU. # Defaults to 1. (Since 5.0) # +# @block-bitmap-mapping: Maps block nodes and bitmaps on them to +# aliases for the purpose of dirty bitmap migration. Such +# aliases may for example be the corresponding names on the +# opposite site. +# The mapping must be one-to-one and complete: On the source, I've recently faced the case where incomplete mapping make sence: shared migration of read-only bitmaps in backing files: it seems better to not migrate them through migration channel, they are migrating through shared storage automatically (yes, we'll pay the time to load them on destination, but I'm going to improve it by implementing lazy load of read-only and disabled bitmaps, so this will not be a problem). So, now I think that it would be good just ignore all the bitmap not described by mapping OK. +# migrating a bitmap from a node when either is not mapped +# will result in an error. On the destination, similarly, +# receiving a bitmap (by alias) from a node (by alias) when +# either alias is not mapped will result in an error. +# By default, all node names and bitmap names are mapped to +# themselves. (Since 5.1) This sentense is unclear, want means "by default" - if the mapping is not specified at all or if some nodes/bitmaps are not covered. It is clear. Still, tha latter will conflict with previous sentencies, so "by default" is about when mapping is not set at all. Precisely. I suggest to word it directly: "If mapping is not set (the command never called, or mapping was removed". The mapping cannot be removed. It’s also technically clear because mentioning a default for some parameter always means that that particular parameter will have that default. So in this case “by default” refers to block-bitmap-mapping, not anything nested in it. (Defaults for stuff nested in its value would be described in the respective structs’ definitions.) Reasonable. > I’d be OK with “By default (when this parameter has never been set)…”. Yes, this is absolutely clear. And, actual behavior without mapping is not as simple: it actually tries to use blk names if possible and node-names if not, and this veries from version to version. So, I think not worth to document it in detail, just note, that without mapping the
Re: [PATCH 10/46] qemu-option: Check return value instead of @err where convenient
01.07.2020 11:02, Markus Armbruster wrote: Vladimir Sementsov-Ogievskiy writes: 24.06.2020 19:43, Markus Armbruster wrote: Convert uses like opts = qemu_opts_create(..., &err); if (err) { ... } to opts = qemu_opts_create(..., &err); if (!opts) { ... } Eliminate error_propagate() that are now unnecessary. Delete @err that are now unused. Signed-off-by: Markus Armbruster --- block/parallels.c | 4 ++-- blockdev.c | 5 ++--- qdev-monitor.c | 6 ++ util/qemu-config.c | 10 -- util/qemu-option.c | 12 5 files changed, 14 insertions(+), 23 deletions(-) diff --git a/block/parallels.c b/block/parallels.c index 860dbb80a2..b15c9ac28d 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -823,8 +823,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, } } -opts = qemu_opts_create(¶llels_runtime_opts, NULL, 0, &local_err); -if (local_err != NULL) { +opts = qemu_opts_create(¶llels_runtime_opts, NULL, 0, errp); +if (!opts) { goto fail_options; } Honestly, I don't like this hunk. as already complicated code (crossing gotos) becomes more complicated (add one more pattern to fail_options path: no-op error_propagate). At least, we'll need a follow-up patch, refactoring parallels_open() to drop "fail_options" label completely. For what it's worth, this is how it looks at the end of the series: static int parallels_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { BDRVParallelsState *s = bs->opaque; ParallelsHeader ph; int ret, size, i; QemuOpts *opts = NULL; Error *local_err = NULL; char *buf; bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds, BDRV_CHILD_IMAGE, false, errp); if (!bs->file) { return -EINVAL; } ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph)); if (ret < 0) { goto fail; } bs->total_sectors = le64_to_cpu(ph.nb_sectors); if (le32_to_cpu(ph.version) != HEADER_VERSION) { goto fail_format; } if (!memcmp(ph.magic, HEADER_MAGIC, 16)) { s->off_multiplier = 1; bs->total_sectors = 0x & bs->total_sectors; } else if (!memcmp(ph.magic, HEADER_MAGIC2, 16)) { s->off_multiplier = le32_to_cpu(ph.tracks); } else { goto fail_format; } s->tracks = le32_to_cpu(ph.tracks); if (s->tracks == 0) { error_setg(errp, "Invalid image: Zero sectors per track"); ret = -EINVAL; goto fail; } if (s->tracks > INT32_MAX/513) { error_setg(errp, "Invalid image: Too big cluster"); ret = -EFBIG; goto fail; } 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; } 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; } s->data_end = le32_to_cpu(ph.data_off); if (s->data_end == 0) { s->data_end = ROUND_UP(bat_entry_off(s->bat_size), BDRV_SECTOR_SIZE); } if (s->data_end < s->header_size) { /* there is not enough unused space to fit to block align between BAT and actual data. We can't avoid read-modify-write... */ s->header_size = size; } ret = bdrv_pread(bs->file, 0, s->header, s->header_size); if (ret < 0) { goto fail; } s->bat_bitmap = (uint32_t *)(s->header + 1); for (i = 0; i < s->bat_size; i++) { int64_t off = bat2sect(s, i); if (off >= s->data_end) { s->data_end = off + s->tracks; } } if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) { /* Image was not closed correctly. The check is mandatory */ s->header_unclean = true; if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_CHECK)) { error_setg(errp, "parallels: Image was not closed correctly; " "cannot be opened read/write"); ret = -EACCES; goto fail; } } opts = qemu_opts_create(¶llels_runtime_opts, NULL, 0, errp); if (!opts) { goto fail_options; } if (!qemu_opts_absorb_qdict
Re: [PATCH 2/3] util: support detailed error reporting for qemu_open
On Thu, Jul 02, 2020 at 07:13:08AM +0200, Markus Armbruster wrote: > Daniel P. Berrangé writes: > > > Create a "qemu_open_err" method which does the same as "qemu_open", > > but with a "Error **errp" for error reporting. There should be no > > behavioural difference for existing callers at this stage. > > > > Signed-off-by: Daniel P. Berrangé > > --- > > include/qemu/osdep.h | 1 + > > util/osdep.c | 71 +++- > > 2 files changed, 58 insertions(+), 14 deletions(-) > > > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > > index 0d26a1b9bd..e41701a308 100644 > > --- a/include/qemu/osdep.h > > +++ b/include/qemu/osdep.h > > @@ -483,6 +483,7 @@ int qemu_madvise(void *addr, size_t len, int advice); > > int qemu_mprotect_rwx(void *addr, size_t size); > > int qemu_mprotect_none(void *addr, size_t size); > > > > +int qemu_open_err(const char *name, int flags, Error **errp, ...); > > int qemu_open(const char *name, int flags, ...); > > int qemu_close(int fd); > > int qemu_unlink(const char *name); > > diff --git a/util/osdep.c b/util/osdep.c > > index 4bdbe81cec..450b3a5da3 100644 > > --- a/util/osdep.c > > +++ b/util/osdep.c > > @@ -22,6 +22,7 @@ > > * THE SOFTWARE. > > */ > > #include "qemu/osdep.h" > > +#include "qapi/error.h" > > > > /* Needed early for CONFIG_BSD etc. */ > > > > @@ -282,7 +283,7 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t > > len, bool exclusive) > > /* > > * Opens a file with FD_CLOEXEC set > > */ > > -int qemu_open(const char *name, int flags, ...) > > +static int qemu_openv(const char *name, int flags, Error **errp, va_list > > ap) > > { > > int ret; > > int mode = 0; > > @@ -297,24 +298,31 @@ int qemu_open(const char *name, int flags, ...) > > > > fdset_id = qemu_parse_fdset(fdset_id_str); > > if (fdset_id == -1) { > > +error_setg(errp, "Unable to parse fdset %s", name); > > errno = EINVAL; > > return -1; > > } > > > > fd = monitor_fdset_get_fd(fdset_id, flags); > > if (fd < 0) { > > +error_setg_errno(errp, -fd, "Unable acquire FD for %s flags > > %x", > > + name, flags); > > errno = -fd; > > return -1; > > } > > > > dupfd = qemu_dup_flags(fd, flags); > > if (dupfd == -1) { > > +error_setg_errno(errp, errno, "Unable dup FD for %s flags %x", > > + name, flags); > > return -1; > > } > > > > ret = monitor_fdset_dup_fd_add(fdset_id, dupfd); > > if (ret == -1) { > > close(dupfd); > > +error_setg(errp, "Unable save FD for %s flags %x", > > + name, flags); > > errno = EINVAL; > > return -1; > > } > > @@ -324,11 +332,7 @@ int qemu_open(const char *name, int flags, ...) > > #endif > > > > if (flags & O_CREAT) { > > -va_list ap; > > - > > -va_start(ap, flags); > > mode = va_arg(ap, int); > > -va_end(ap); > > } > > > > #ifdef O_CLOEXEC > > @@ -340,25 +344,64 @@ int qemu_open(const char *name, int flags, ...) > > } > > #endif > > > > +if (ret == -1) { > > +const char *action = "open"; > > +if (flags & O_CREAT) { > > +action = "create"; > > +} > > #ifdef O_DIRECT > > -if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) { > > -int newflags = flags & ~O_DIRECT; > > +if (errno == EINVAL && (flags & O_DIRECT)) { > > +int newflags = flags & ~O_DIRECT; > > # ifdef O_CLOEXEC > > -ret = open(name, newflags | O_CLOEXEC, mode); > > +ret = open(name, newflags | O_CLOEXEC, mode); > > # else > > -ret = open(name, newflags, mode); > > +ret = open(name, newflags, mode); > > # endif > > -if (ret != -1) { > > -close(ret); > > -error_report("file system does not support O_DIRECT"); > > -errno = EINVAL; > > +if (ret != -1) { > > +close(ret); > > +error_setg(errp, "Unable to %s '%s' flags 0x%x: " > > + "filesystem does not support O_DIRECT", > > + action, name, flags); > > +if (!errp) { > > If the caller ignores errors, ... > > > +error_report("file system does not support O_DIRECT"); > > ... we report this error to stderr (but not any of the other ones). > This is weird. I figure you do it here to reproduce the weirdness of > qemu_open() before the patch. Goes back to > > commit a5813077aac7865f69b7ee46a594f3705429f7cd > Author: Stefan Hajnoczi > Date: Thu Aug 22 11:29:03 2013 +0200 > > osdep: warn if open(O_DIRECT) on fails with EINVAL > > Print a warning when opening a file O_DIRECT fai
Re: [PATCH 2/4] migration: Add block-bitmap-mapping parameter
On 02.07.20 11:19, Vladimir Sementsov-Ogievskiy wrote: > 02.07.2020 11:09, Max Reitz wrote: >> On 01.07.20 16:34, Vladimir Sementsov-Ogievskiy wrote: >>> 30.06.2020 11:45, Max Reitz wrote: This migration parameter allows mapping block node names and bitmap names to aliases for the purpose of block dirty bitmap migration. This way, management tools can use different node and bitmap names on the source and destination and pass the mapping of how bitmaps are to be transferred to qemu (on the source, the destination, or even both with arbitrary aliases in the migration stream). Suggested-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Max Reitz --- qapi/migration.json | 83 +++- migration/migration.h | 3 + migration/block-dirty-bitmap.c | 372 - migration/migration.c | 29 +++ 4 files changed, 432 insertions(+), 55 deletions(-) diff --git a/qapi/migration.json b/qapi/migration.json index d5000558c6..5aeae9bea8 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -507,6 +507,44 @@ 'data': [ 'none', 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] } +## +# @BitmapMigrationBitmapAlias: +# +# @name: The name of the bitmap. +# +# @alias: An alias name for migration (for example the bitmap name on +# the opposite site). +# +# Since: 5.1 +## +{ 'struct': 'BitmapMigrationBitmapAlias', + 'data': { + 'name': 'str', + 'alias': 'str' + } } + +## +# @BitmapMigrationNodeAlias: +# +# Maps a block node name and the bitmaps it has to aliases for dirty +# bitmap migration. +# +# @node-name: A block node name. +# +# @alias: An alias block node name for migration (for example the +# node name on the opposite site). +# +# @bitmaps: Mappings for the bitmaps on this node. +# +# Since: 5.1 +## +{ 'struct': 'BitmapMigrationNodeAlias', + 'data': { + 'node-name': 'str', + 'alias': 'str', + 'bitmaps': [ 'BitmapMigrationBitmapAlias' ] >>> >>> So, we still can't migrate bitmaps from one node to different nodes, >>> but we >>> also don't know a usecase for it, so it seems OK. But with such >>> scheme we >>> can select and rename bitmaps in-flight, and Peter said about >>> corresponding >>> use-case. >>> >>> I'm OK with this, still, just an idea in my mind: >>> >>> we could instead just have a list of >>> >>> BitmapMigrationAlias: { >>> node-name >>> bitmap-name >>> node-alias >>> bitmap-alias >>> } >>> >>> so, mapping is set for each bitmap in separate. >> >> Well, OK, but why? > > But why not :) Just thinking out loud. May be someone will imaging good > reasons for it. The reason for “Why not” is that this code now exists. ;) + } + } + + g_hash_table_insert(bitmaps_map, + g_strdup(bmap_map_from), g_strdup(bmap_map_to)); + } + + amin = g_new(AliasMapInnerNode, 1); + *amin = (AliasMapInnerNode){ >>> >>> style: space before '{' >> >> Is that required? > > If checkpatch doesn't complain, than not.. O:) + .string = g_strdup(node_map_to), + .subtree = bitmaps_map, + }; + + g_hash_table_insert(alias_map, g_strdup(node_map_from), amin); + } + + return alias_map; + +fail: + g_hash_table_destroy(alias_map); + return NULL; +} + +/** + * Run construct_alias_map() in both directions to check whether @bbm + * is valid. + * (This function is to be used by migration/migration.c to validate + * the user-specified block-bitmap-mapping migration parameter.) + * + * Returns true if and only if the mapping is valid. + */ +bool check_dirty_bitmap_mig_alias_map(const BitmapMigrationNodeAliasList *bbm, + Error **errp) +{ + GHashTable *alias_map; + + alias_map = construct_alias_map(bbm, true, errp); + if (!alias_map) { + return false; + } + g_hash_table_destroy(alias_map); + + alias_map = construct_alias_map(bbm, false, errp); + if (!alias_map) { + return false; + } + g_hash_table_destroy(alias_map); + + return true; +} + void init_dirty_bitmap_incoming_migration(void) { qemu_mutex_init(&finish_lock); @@ -191,11 +351,11 @@ static void send_bitmap_header(QEMUFile *f, DirtyBitmapMigBitmapState *dbms, qemu_put_bitmap_flags(f, flags); if (flags & D
Re: [PATCH v9 14/34] qcow2: Add QCow2SubclusterType and qcow2_get_subcluster_type()
On 01.07.20 18:26, Alberto Garcia wrote: > On Wed 01 Jul 2020 02:52:14 PM CEST, Max Reitz wrote: >>> if (l2_entry & QCOW_OFLAG_COMPRESSED) { >>> return QCOW2_CLUSTER_COMPRESSED; >>> -} else if (l2_entry & QCOW_OFLAG_ZERO) { >>> +} else if ((l2_entry & QCOW_OFLAG_ZERO) && !has_subclusters(s)) { >> >> OK, so now qcow2_get_cluster_type() reports zero clusters to be normal >> or unallocated clusters when there are subclusters. Seems weird to >> me, because zero clusters are invalid clusters then. > > I'm actually hesitant about this. > > In extended L2 entries QCOW_OFLAG_ZERO does not have any meaning so > technically it doesn't need to be checked any more than the other > reserved bits (1 to 8). Good point. That convinces me. > The reason why we would want to check it is, of course, because that bit > does have a meaning in regular L2 entries. > > But that bit is ignored in images with subclusters so the only reason > why we would check it is to report corruption, not because we need to > know its value. Sure. But isn’t that the whole point of having QCOW2_SUBCLUSTER_INVALID in the first place? > It's true that we do check it in v2 images, although in that case the > entries are otherwise identical and there is a way to convert between > both types. > >> I preferred just reporting them as zero clusters and letting the >> caller deal with it, because it does mean an error in the image and so >> it should be reported. > > Another alternative would be to add QCOW2_CLUSTER_INVALID and we could > even include there other cases like unaligned offsets and things like > that. But that would also affect the code that repairs corrupted images. Interesting. Well, and that’d be definitely too much for this series, as you already said. So: Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [PATCH 1/3] util: validate whether O_DIRECT is supported after failure
On 7/1/20 6:05 PM, Daniel P. Berrangé wrote: > Currently we suggest that a filesystem may not support O_DIRECT after > seeing an EINVAL. Other things can cause EINVAL though, so it is better > to do an explicit check, and then report a definitive error message. > > Signed-off-by: Daniel P. Berrangé > --- > util/osdep.c | 13 +++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/util/osdep.c b/util/osdep.c > index 4829c07ff6..4bdbe81cec 100644 > --- a/util/osdep.c > +++ b/util/osdep.c > @@ -342,8 +342,17 @@ int qemu_open(const char *name, int flags, ...) > > #ifdef O_DIRECT > if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) { > -error_report("file system may not support O_DIRECT"); > -errno = EINVAL; /* in case it was clobbered */ > +int newflags = flags & ~O_DIRECT; > +# ifdef O_CLOEXEC > +ret = open(name, newflags | O_CLOEXEC, mode); > +# else > +ret = open(name, newflags, mode); > +# endif Can we use this simpler form (easier to set a breakpoint)? #ifdef O_CLOEXEC newflags |= O_CLOEXEC; #endif ret = open(name, newflags, mode); Whichever you prefer: Reviewed-by: Philippe Mathieu-Daudé > +if (ret != -1) { > +close(ret); > +error_report("file system does not support O_DIRECT"); > +errno = EINVAL; > +} > } > #endif /* O_DIRECT */ > >
Re: [PATCH v4 2/2] nvme: allow cmb and pmr to be enabled on same device
On Jul 1 14:48, Andrzej Jakowski wrote: > So far it was not possible to have CMB and PMR emulated on the same > device, because BAR2 was used exclusively either of PMR or CMB. This > patch places CMB at BAR4 offset so it not conflicts with MSI-X vectors. > Linux craps out when I test this (1MB CMB): Misaligned __add_pages start: 0xfdd00 end: #fdeff I tracked it down to check_pfn_span (mm/memory_hotplug.c) failing because it's not on a 2MB boundary. I then tried to monkey patch the cmb_offset to be 2MB instead and it succeeds in registering the cmb: [8.384565] memmap_init_zone_device initialised 512 pages in 0ms [8.385715] nvme :03:00.0: added peer-to-peer DMA memory [mem 0xfd20-0xfd3f 64bit pref] [8.419372] nvme nvme0: 1/0/0 default/read/poll queues But the kernel then continues to really crap out with a kernel panic: [8.440891] DMAR: DRHD: handling fault status reg 2 [8.440934] BUG: kernel NULL pointer dereference, address: 0120 [8.441713] DMAR: [DMA Read] Request device [03:00.0] PASID fault addr fd20 [faul t reason 06] PTE Read access is not set [8.442630] #PF: supervisor write access in kernel mode [8.444972] #PF: error_code(0x0002) - not-present page [8.445640] PGD 0 P4D 0 [8.445965] Oops: 0002 [#1] PREEMPT SMP PTI [8.446499] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 5.8.0-rc1-00034-gb6cf9836d07f-dirty #19 [8.447547] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-0-gf21b5a4aeb0 2-prebuilt.qemu.org 04/01/2014 [8.448898] RIP: 0010:nvme_process_cq+0xc4/0x200 [nvme] [8.449525] Code: cf 00 00 00 48 8b 57 70 48 8b 7c c2 f8 e8 14 e9 32 c1 49 89 c7 0f 1f 44 00 0 0 41 0f b7 44 24 0e 49 8b 14 24 4c 89 ff 66 d1 e8 <49> 89 97 20 01 00 00 66 41 89 87 2a 01 00 00 e8 28 04 33 c1 0f b7 [8.451662] RSP: 0018:c915cf20 EFLAGS: 00010803 [8.452321] RAX: 400b RBX: 88826faad0c0 RCX: [8.453293] RDX: RSI: RDI: [8.454312] RBP: 8882725d38e4 R08: 0001f71e225c R09: [8.455319] R10: R11: R12: 888270bb [8.456334] R13: R14: c915cfac R15: [8.457311] FS: () GS:888277d8() knlGS: [8.458441] CS: 0010 DS: ES: CR0: 80050033 [8.459380] CR2: 0120 CR3: 000271c8c006 CR4: 00360ee0 [8.460507] Call Trace: [8.460906] [8.461272] nvme_irq+0x10/0x20 [nvme] [8.461951] __handle_irq_event_percpu+0x45/0x1b0 [8.462803] ? handle_fasteoi_irq+0x210/0x210 [8.463585] handle_irq_event+0x58/0xb0 [8.464312] handle_edge_irq+0xae/0x270 [8.465027] asm_call_on_stack+0x12/0x20 [8.465686] [8.466053] common_interrupt+0x120/0x1f0 [8.466717] asm_common_interrupt+0x1e/0x40 [8.467429] RIP: 0010:default_idle+0x21/0x170 [8.468140] Code: eb a6 e8 82 27 ff ff cc cc 0f 1f 44 00 00 41 54 55 53 e8 e2 2e ff ff 0f 1f 4 4 00 00 e9 07 00 00 00 0f 00 2d f3 d4 44 00 fb f4 ca 2e ff ff 89 c5 0f 1f 44 00 00 5b 5d 41 5c c3 89 c5 65 8b 05 [8.471286] RSP: 0018:c909fec8 EFLAGS: 0282 [8.472202] RAX: 0003 RBX: 888276ff RCX: 0001 [8.473405] RDX: 0001 RSI: 8212355f RDI: 8212d699 [8.474571] RBP: 0003 R08: 888277d9e4a0 R09: 0020 [8.475717] R10: R11: R12: [8.476921] R13: R14: R15: [8.478110] ? default_idle+0xe/0x170 [8.478728] do_idle+0x1e1/0x240 [8.479283] ? _raw_spin_lock_irqsave+0x19/0x40 [8.480040] cpu_startup_entry+0x19/0x20 [8.480705] start_secondary+0x153/0x190 [8.481400] secondary_startup_64+0xb6/0xc0 [8.482114] Modules linked in: libata nvme nvme_core scsi_mod t10_pi crc_t10dif crct10dif_gene ric crct10dif_common virtio_net net_failover failover virtio_rng rng_core [8.484675] CR2: 0120 [8.485264] ---[ end trace ff1849437c76af12 ]--- [8.486065] RIP: 0010:nvme_process_cq+0xc4/0x200 [nvme] [8.486953] Code: cf 00 00 00 48 8b 57 70 48 8b 7c c2 f8 e8 14 e9 32 c1 49 89 c7 0f 1f 44 00 0 0 41 0f b7 44 24 0e 49 8b 14 24 4c 89 ff 66 d1 e8 <49> 89 97 20 01 00 00 66 41 89 87 2a 01 00 00 e8 28 04 33 c1 0f b7 [8.490234] RSP: 0018:c915cf20 EFLAGS: 00010803 [8.491144] RAX: 400b RBX: 88826faad0c0 RCX: [8.492445] RDX: RSI: RDI: [8.493681] RBP: 8882725d38e4 R08: 0001f71e225c R09: [8.494907] R10: R11: R12: 888270bb [8.496130] R13: R14: c915cfac R15: [8.497363] FS: () G
Re: [PATCH v4 2/2] nvme: allow cmb and pmr to be enabled on same device
On Jul 2 12:13, Klaus Jensen wrote: > On Jul 1 14:48, Andrzej Jakowski wrote: > > So far it was not possible to have CMB and PMR emulated on the same > > device, because BAR2 was used exclusively either of PMR or CMB. This > > patch places CMB at BAR4 offset so it not conflicts with MSI-X vectors. > > > > Linux craps out when I test this (1MB CMB): > > Misaligned __add_pages start: 0xfdd00 end: #fdeff > > I tracked it down to check_pfn_span (mm/memory_hotplug.c) failing > because it's not on a 2MB boundary. I then tried to monkey patch the > cmb_offset to be 2MB instead and it succeeds in registering the cmb: > > [8.384565] memmap_init_zone_device initialised 512 pages in 0ms > [8.385715] nvme :03:00.0: added peer-to-peer DMA memory [mem > 0xfd20-0xfd3f 64bit > pref] > [8.419372] nvme nvme0: 1/0/0 default/read/poll queues > > But the kernel then continues to really crap out with a kernel panic: > > [8.440891] DMAR: DRHD: handling fault status reg 2 > [8.440934] BUG: kernel NULL pointer dereference, address: 0120 > [8.441713] DMAR: [DMA Read] Request device [03:00.0] PASID fault > addr fd20 [faul > t reason 06] PTE Read access is not set > [8.442630] #PF: supervisor write access in kernel mode > [8.444972] #PF: error_code(0x0002) - not-present page > [8.445640] PGD 0 P4D 0 > [8.445965] Oops: 0002 [#1] PREEMPT SMP PTI > [8.446499] CPU: 3 PID: 0 Comm: swapper/3 Not tainted > 5.8.0-rc1-00034-gb6cf9836d07f-dirty #19 > [8.447547] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS > rel-1.13.0-0-gf21b5a4aeb0 > 2-prebuilt.qemu.org 04/01/2014 > [8.448898] RIP: 0010:nvme_process_cq+0xc4/0x200 [nvme] > [8.449525] Code: cf 00 00 00 48 8b 57 70 48 8b 7c c2 f8 e8 14 e9 32 c1 49 > 89 c7 0f 1f 44 00 0 > 0 41 0f b7 44 24 0e 49 8b 14 24 4c 89 ff 66 d1 e8 <49> 89 97 20 01 00 00 66 > 41 89 87 2a 01 00 00 > e8 28 04 33 c1 0f b7 > [8.451662] RSP: 0018:c915cf20 EFLAGS: 00010803 > [8.452321] RAX: 400b RBX: 88826faad0c0 RCX: > > [8.453293] RDX: RSI: RDI: > > [8.454312] RBP: 8882725d38e4 R08: 0001f71e225c R09: > > [8.455319] R10: R11: R12: > 888270bb > [8.456334] R13: R14: c915cfac R15: > > [8.457311] FS: () GS:888277d8() > knlGS: > [8.458441] CS: 0010 DS: ES: CR0: 80050033 > [8.459380] CR2: 0120 CR3: 000271c8c006 CR4: > 00360ee0 > [8.460507] Call Trace: > [8.460906] > [8.461272] nvme_irq+0x10/0x20 [nvme] > [8.461951] __handle_irq_event_percpu+0x45/0x1b0 > [8.462803] ? handle_fasteoi_irq+0x210/0x210 > [8.463585] handle_irq_event+0x58/0xb0 > [8.464312] handle_edge_irq+0xae/0x270 > [8.465027] asm_call_on_stack+0x12/0x20 > [8.465686] > [8.466053] common_interrupt+0x120/0x1f0 > [8.466717] asm_common_interrupt+0x1e/0x40 > [8.467429] RIP: 0010:default_idle+0x21/0x170 > [8.468140] Code: eb a6 e8 82 27 ff ff cc cc 0f 1f 44 00 00 41 54 55 53 e8 > e2 2e ff ff 0f 1f 4 > 4 00 00 e9 07 00 00 00 0f 00 2d f3 d4 44 00 fb f4 ca 2e ff ff 89 c5 0f > 1f 44 00 00 5b 5d 41 > 5c c3 89 c5 65 8b 05 > [8.471286] RSP: 0018:c909fec8 EFLAGS: 0282 > [8.472202] RAX: 0003 RBX: 888276ff RCX: > 0001 > [8.473405] RDX: 0001 RSI: 8212355f RDI: > 8212d699 > [8.474571] RBP: 0003 R08: 888277d9e4a0 R09: > 0020 > [8.475717] R10: R11: R12: > > [8.476921] R13: R14: R15: > > [8.478110] ? default_idle+0xe/0x170 > [8.478728] do_idle+0x1e1/0x240 > [8.479283] ? _raw_spin_lock_irqsave+0x19/0x40 > [8.480040] cpu_startup_entry+0x19/0x20 > [8.480705] start_secondary+0x153/0x190 > [8.481400] secondary_startup_64+0xb6/0xc0 > [8.482114] Modules linked in: libata nvme nvme_core scsi_mod t10_pi > crc_t10dif crct10dif_gene > ric crct10dif_common virtio_net net_failover failover virtio_rng rng_core > [8.484675] CR2: 0120 > [8.485264] ---[ end trace ff1849437c76af12 ]--- > [8.486065] RIP: 0010:nvme_process_cq+0xc4/0x200 [nvme] > [8.486953] Code: cf 00 00 00 48 8b 57 70 48 8b 7c c2 f8 e8 14 e9 32 c1 49 > 89 c7 0f 1f 44 00 0 > 0 41 0f b7 44 24 0e 49 8b 14 24 4c 89 ff 66 d1 e8 <49> 89 97 20 01 00 00 66 > 41 89 87 2a 01 00 00 > e8 28 04 33 c1 0f b7 > [8.490234] RSP: 0018:c915cf20 EFLAGS: 00010803 > [8.491144] RAX: 400b RBX: 88826faad0c0 RCX: > > [8.492445] RDX: RSI: RDI: > > [8.493681] RBP:
Re: [PATCH 2/4] migration: Add block-bitmap-mapping parameter
02.07.2020 12:41, Max Reitz wrote: On 02.07.20 11:19, Vladimir Sementsov-Ogievskiy wrote: 02.07.2020 11:09, Max Reitz wrote: On 01.07.20 16:34, Vladimir Sementsov-Ogievskiy wrote: 30.06.2020 11:45, Max Reitz wrote: This migration parameter allows mapping block node names and bitmap names to aliases for the purpose of block dirty bitmap migration. This way, management tools can use different node and bitmap names on the source and destination and pass the mapping of how bitmaps are to be transferred to qemu (on the source, the destination, or even both with arbitrary aliases in the migration stream). Suggested-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Max Reitz --- qapi/migration.json | 83 +++- migration/migration.h | 3 + migration/block-dirty-bitmap.c | 372 - migration/migration.c | 29 +++ 4 files changed, 432 insertions(+), 55 deletions(-) diff --git a/qapi/migration.json b/qapi/migration.json index d5000558c6..5aeae9bea8 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -507,6 +507,44 @@ 'data': [ 'none', 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] } +## +# @BitmapMigrationBitmapAlias: +# +# @name: The name of the bitmap. +# +# @alias: An alias name for migration (for example the bitmap name on +# the opposite site). +# +# Since: 5.1 +## +{ 'struct': 'BitmapMigrationBitmapAlias', + 'data': { + 'name': 'str', + 'alias': 'str' + } } + +## +# @BitmapMigrationNodeAlias: +# +# Maps a block node name and the bitmaps it has to aliases for dirty +# bitmap migration. +# +# @node-name: A block node name. +# +# @alias: An alias block node name for migration (for example the +# node name on the opposite site). +# +# @bitmaps: Mappings for the bitmaps on this node. +# +# Since: 5.1 +## +{ 'struct': 'BitmapMigrationNodeAlias', + 'data': { + 'node-name': 'str', + 'alias': 'str', + 'bitmaps': [ 'BitmapMigrationBitmapAlias' ] So, we still can't migrate bitmaps from one node to different nodes, but we also don't know a usecase for it, so it seems OK. But with such scheme we can select and rename bitmaps in-flight, and Peter said about corresponding use-case. I'm OK with this, still, just an idea in my mind: we could instead just have a list of BitmapMigrationAlias: { node-name bitmap-name node-alias bitmap-alias } so, mapping is set for each bitmap in separate. Well, OK, but why? But why not :) Just thinking out loud. May be someone will imaging good reasons for it. The reason for “Why not” is that this code now exists. ;) Exactly :) But another arguments may appear, who knows. If not - great. Actual reason is more flexible interface: you can migrate any bitmap to any node with any name (except for conflicts of course). But do we need it, I don't know. I'd like to hear Peter's opinion, if he don't have preference, then I don't care too. -- Best regards, Vladimir
Re: [PATCH 2/4] migration: Add block-bitmap-mapping parameter
02.07.2020 12:41, Max Reitz wrote: I don’t know if doing it differently would actually be beneficial for anyone, but OTOH naively it seems like a more invasive code change. I don't see real benefits, we can go either way, so, not worth rewriting the patch. === I feel like a stupid reviewer:) Huh? If anything, a stupid review on a design-changing patch would be a plain “R-b” without having actually considered the impact. You do consider the impact and question it in all places. I don’t think I need to mention this, but that’s a very good and important thing to do, because it forces me to reason why we’d want this or that design. Without being questioned, I wouldn’t have to reason about that. (Which may be a problem in our patch workflow – authors don’t need to reason unless questioned.[1]) Sorry if I gave the impression of dismissing your comments. It should be my burden to reason why I took certain design decisions. No problem. I should better describe reasons for my suggestions as well, or if I have no one, mark it as "thinking-out-loud" instead of recommended change. -- Best regards, Vladimir
Re: [PATCH 0/4] migration: Add block-bitmap-mapping parameter
Hmm, seems, you didn't use scripts/get_maintainer.pl, as neither Eric nor John are in Cc. Add them. 30.06.2020 11:45, Max Reitz wrote: RFC v1: https://lists.nongnu.org/archive/html/qemu-block/2020-05/msg00912.html RFC v2: https://lists.nongnu.org/archive/html/qemu-block/2020-05/msg00915.html Branch: https://github.com/XanClic/qemu.git migration-bitmap-mapping-v1 Branch: https://git.xanclic.moe/XanClic/qemu.git migration-bitmap-mapping-v1 Hi, This new migration parameter allows mapping block node names and bitmap names to aliases for the purpose of block dirty bitmap migration. This way, management tools can use different node names on the source and destination and pass the mapping of how bitmaps are to be transferred to qemu (on the source, the destination, or even both with arbitrary aliases in the migration stream). v1 (as opposed to the RFC): - Added an iotest - Allow mapping of not only node names, but also of bitmap names to aliases - Make this a migration parameter instead of adding a whole new QMP command - Added patch 1 for good measure Max Reitz (4): migration: Prevent memleak by ...params_test_apply migration: Add block-bitmap-mapping parameter iotests.py: Add wait_for_runstate() iotests: Test node/bitmap aliases during migration qapi/migration.json| 83 +- migration/migration.h | 3 + migration/block-dirty-bitmap.c | 372 + migration/migration.c | 33 ++- tests/qemu-iotests/300 | 487 + tests/qemu-iotests/300.out | 5 + tests/qemu-iotests/group | 1 + tests/qemu-iotests/iotests.py | 4 + 8 files changed, 931 insertions(+), 57 deletions(-) create mode 100755 tests/qemu-iotests/300 create mode 100644 tests/qemu-iotests/300.out -- Best regards, Vladimir
Re: [PATCH 2/4] migration: Add block-bitmap-mapping parameter
* Max Reitz (mre...@redhat.com) wrote: > On 30.06.20 12:51, Dr. David Alan Gilbert wrote: > > * Max Reitz (mre...@redhat.com) wrote: > >> This migration parameter allows mapping block node names and bitmap > >> names to aliases for the purpose of block dirty bitmap migration. > >> > >> This way, management tools can use different node and bitmap names on > >> the source and destination and pass the mapping of how bitmaps are to be > >> transferred to qemu (on the source, the destination, or even both with > >> arbitrary aliases in the migration stream). > >> > >> Suggested-by: Vladimir Sementsov-Ogievskiy > >> Signed-off-by: Max Reitz > >> --- > >> qapi/migration.json| 83 +++- > >> migration/migration.h | 3 + > >> migration/block-dirty-bitmap.c | 372 - > >> migration/migration.c | 29 +++ > >> 4 files changed, 432 insertions(+), 55 deletions(-) > >> > >> diff --git a/qapi/migration.json b/qapi/migration.json > >> index d5000558c6..5aeae9bea8 100644 > >> --- a/qapi/migration.json > >> +++ b/qapi/migration.json > >> @@ -507,6 +507,44 @@ > >>'data': [ 'none', 'zlib', > >> { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] } > >> > >> +## > >> +# @BitmapMigrationBitmapAlias: > >> +# > >> +# @name: The name of the bitmap. > >> +# > >> +# @alias: An alias name for migration (for example the bitmap name on > >> +# the opposite site). > >> +# > >> +# Since: 5.1 > >> +## > >> +{ 'struct': 'BitmapMigrationBitmapAlias', > >> + 'data': { > >> + 'name': 'str', > >> + 'alias': 'str' > >> + } } > >> + > >> +## > >> +# @BitmapMigrationNodeAlias: > >> +# > >> +# Maps a block node name and the bitmaps it has to aliases for dirty > >> +# bitmap migration. > >> +# > >> +# @node-name: A block node name. > >> +# > >> +# @alias: An alias block node name for migration (for example the > >> +# node name on the opposite site). > >> +# > >> +# @bitmaps: Mappings for the bitmaps on this node. > >> +# > >> +# Since: 5.1 > >> +## > >> +{ 'struct': 'BitmapMigrationNodeAlias', > >> + 'data': { > >> + 'node-name': 'str', > >> + 'alias': 'str', > >> + 'bitmaps': [ 'BitmapMigrationBitmapAlias' ] > >> + } } > >> + > >> ## > >> # @MigrationParameter: > >> # > >> @@ -641,6 +679,18 @@ > >> # will consume more CPU. > >> # Defaults to 1. (Since 5.0) > >> # > >> +# @block-bitmap-mapping: Maps block nodes and bitmaps on them to > >> +# aliases for the purpose of dirty bitmap migration. Such > >> +# aliases may for example be the corresponding names on the > >> +# opposite site. > >> +# The mapping must be one-to-one and complete: On the source, > >> +# migrating a bitmap from a node when either is not mapped > >> +# will result in an error. On the destination, similarly, > >> +# receiving a bitmap (by alias) from a node (by alias) when > >> +# either alias is not mapped will result in an error. > >> +# By default, all node names and bitmap names are mapped to > >> +# themselves. (Since 5.1) > >> +# > >> # Since: 2.4 > >> ## > >> { 'enum': 'MigrationParameter', > >> @@ -655,7 +705,8 @@ > >> 'multifd-channels', > >> 'xbzrle-cache-size', 'max-postcopy-bandwidth', > >> 'max-cpu-throttle', 'multifd-compression', > >> - 'multifd-zlib-level' ,'multifd-zstd-level' ] } > >> + 'multifd-zlib-level' ,'multifd-zstd-level', > >> + 'block-bitmap-mapping' ] } > >> > >> ## > >> # @MigrateSetParameters: > >> @@ -781,6 +832,18 @@ > >> # will consume more CPU. > >> # Defaults to 1. (Since 5.0) > >> # > >> +# @block-bitmap-mapping: Maps block nodes and bitmaps on them to > >> +# aliases for the purpose of dirty bitmap migration. Such > >> +# aliases may for example be the corresponding names on the > >> +# opposite site. > >> +# The mapping must be one-to-one and complete: On the source, > >> +# migrating a bitmap from a node when either is not mapped > >> +# will result in an error. On the destination, similarly, > >> +# receiving a bitmap (by alias) from a node (by alias) when > >> +# either alias is not mapped will result in an error. > >> +# By default, all node names and bitmap names are mapped to > >> +# themselves. (Since 5.1) > >> +# > >> # Since: 2.4 > >> ## > >> # TODO either fuse back into MigrationParameters, or make > >> @@ -811,7 +874,8 @@ > >> '*max-cpu-throttle': 'int', > >> '*multifd-compression': 'MultiFDCompression', > >> '*multifd-zlib-level': 'int', > >> -'*multifd-zstd-level': 'int' } } > >> +'*multifd-zstd-level': 'int', > >> +'*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } } > > > > That's a hairy type for a migration parame
Re: [PATCH v9 20/34] qcow2: Add subcluster support to calculate_l2_meta()
On 28.06.20 13:02, Alberto Garcia wrote: > If an image has subclusters then there are more copy-on-write > scenarios that we need to consider. Let's say we have a write request > from the middle of subcluster #3 until the end of the cluster: > > 1) If we are writing to a newly allocated cluster then we need >copy-on-write. The previous contents of subclusters #0 to #3 must >be copied to the new cluster. We can optimize this process by >skipping all leading unallocated or zero subclusters (the status of >those skipped subclusters will be reflected in the new L2 bitmap). > > 2) If we are overwriting an existing cluster: > >2.1) If subcluster #3 is unallocated or has the all-zeroes bit set > then we need copy-on-write (on subcluster #3 only). > >2.2) If subcluster #3 was already allocated then there is no need > for any copy-on-write. However we still need to update the L2 > bitmap to reflect possible changes in the allocation status of > subclusters #4 to #31. Because of this, this function checks > if all the overwritten subclusters are already allocated and > in this case it returns without creating a new QCowL2Meta > structure. > > After all these changes l2meta_cow_start() and l2meta_cow_end() > are not necessarily cluster-aligned anymore. We need to update the > calculation of old_start and old_end in handle_dependencies() to > guarantee that no two requests try to write on the same cluster. > > Signed-off-by: Alberto Garcia > Reviewed-by: Eric Blake > --- > block/qcow2-cluster.c | 163 +- > 1 file changed, 131 insertions(+), 32 deletions(-) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [PATCH 33/46] qom: Crash more nicely on object_property_get_link() failure
Markus Armbruster writes: > Eric Blake writes: > >> On 6/24/20 11:43 AM, Markus Armbruster wrote: >>> Pass &error_abort instead of NULL where the returned value is >>> dereferenced or asserted to be non-null. >>> >>> Signed-off-by: Markus Armbruster >>> --- >> >>> @@ -63,8 +64,8 @@ hwaddr platform_bus_get_mmio_addr(PlatformBusDevice >>> *pbus, SysBusDevice *sbdev, >>> return -1; >>> } >>> -parent_mr = object_property_get_link(OBJECT(sbdev_mr), >>> "container", NULL); >>> - >>> +parent_mr = object_property_get_link(OBJECT(sbdev_mr), "container", >>> + &error_abort); >>> assert(parent_mr); >> >> Do we still need to keep the assert? > > Not really, I guess. > >>> +++ b/hw/ppc/spapr_pci_nvlink2.c >>> @@ -141,9 +141,10 @@ static void spapr_phb_pci_collect_nvgpu(PCIBus *bus, >>> PCIDevice *pdev, >>> if (tgt) { >>> Error *local_err = NULL; >>> SpaprPhbPciNvGpuConfig *nvgpus = opaque; >>> -Object *mr_gpu = object_property_get_link(po, "nvlink2-mr[0]", >>> NULL); >>> +Object *mr_gpu = object_property_get_link(po, "nvlink2-mr[0]", >>> + &error_abort); >>> Object *mr_npu = object_property_get_link(po, >>> "nvlink2-atsd-mr[0]", >>> - NULL); >>> + &error_abort); >>> g_assert(mr_gpu || mr_npu); >> >> Likewise. > > I'll drop both unless somebody objects. The second hunk needs to be dropped instead: either of the two object_property_get_link() may fail, just not both.
Re: [PATCH v9 21/34] qcow2: Add subcluster support to qcow2_get_host_offset()
On 28.06.20 13:02, Alberto Garcia wrote: > The logic of this function remains pretty much the same, except that > it uses count_contiguous_subclusters(), which combines the logic of > count_contiguous_clusters() / count_contiguous_clusters_unallocated() > and checks individual subclusters. > > qcow2_cluster_to_subcluster_type() is not necessary as a separate > function anymore so it's inlined into its caller. > > Signed-off-by: Alberto Garcia > Reviewed-by: Eric Blake > --- > block/qcow2.h | 38 --- > block/qcow2-cluster.c | 150 ++ > 2 files changed, 92 insertions(+), 96 deletions(-) [...] > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index 59dd9bda29..2f3bd3a882 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -426,66 +426,66 @@ static int > qcow2_get_subcluster_range_type(BlockDriverState *bs, [...] > +static int count_contiguous_subclusters(BlockDriverState *bs, int > nb_clusters, > +unsigned sc_index, uint64_t > *l2_slice, > +unsigned *l2_index) > { > BDRVQcow2State *s = bs->opaque; > -int i; > -QCow2ClusterType first_cluster_type; > -uint64_t mask = stop_flags | L2E_OFFSET_MASK | QCOW_OFLAG_COMPRESSED; > -uint64_t first_entry = get_l2_entry(s, l2_slice, l2_index); > -uint64_t offset = first_entry & mask; > +int i, count = 0; > +bool check_offset; > +uint64_t expected_offset; > +QCow2SubclusterType expected_type, type; > > -first_cluster_type = qcow2_get_cluster_type(bs, first_entry); > -if (first_cluster_type == QCOW2_CLUSTER_UNALLOCATED) { > -return 0; > -} > - > -/* must be allocated */ > -assert(first_cluster_type == QCOW2_CLUSTER_NORMAL || > - first_cluster_type == QCOW2_CLUSTER_ZERO_ALLOC); > +assert(*l2_index + nb_clusters <= s->l2_size); Not l2_slice_size? > > for (i = 0; i < nb_clusters; i++) { > -uint64_t l2_entry = get_l2_entry(s, l2_slice, l2_index + i) & mask; > -if (offset + (uint64_t) i * cluster_size != l2_entry) { > +unsigned first_sc = (i == 0) ? sc_index : 0; > +uint64_t l2_entry = get_l2_entry(s, l2_slice, *l2_index + i); > +uint64_t l2_bitmap = get_l2_bitmap(s, l2_slice, *l2_index + i); > +int ret = qcow2_get_subcluster_range_type(bs, l2_entry, l2_bitmap, > + first_sc, &type); > +if (ret < 0) { > +*l2_index += i; /* Point to the invalid entry */ > +return -EIO; > +} > +if (i == 0) { > +if (type == QCOW2_SUBCLUSTER_COMPRESSED) { > +/* Compressed clusters are always processed one by one */ > +return ret; > +} > +expected_type = type; > +expected_offset = l2_entry & L2E_OFFSET_MASK; > +check_offset = (type == QCOW2_SUBCLUSTER_NORMAL || > +type == QCOW2_SUBCLUSTER_ZERO_ALLOC || > +type == QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC); > +} else if (type != expected_type) { > break; > +} else if (check_offset) { My gcc (v10.1.1) appears to be a bit daft, and so doesn’t recognize that check_offset must always be initialized before this line is hit. > +expected_offset += s->cluster_size; Same for expected_offset. Would you mind helping it by initializing them in their definition? O:) Max signature.asc Description: OpenPGP digital signature
Re: [PATCH 07/46] error: Avoid more error_propagate() when error is not used here
Markus Armbruster writes: > Vladimir Sementsov-Ogievskiy writes: > >> 24.06.2020 19:43, Markus Armbruster wrote: >>> When all we do with an Error we receive into a local variable is >>> propagating to somewhere else, we can just as well receive it there >>> right away. The previous commit did that for simple cases with >>> Coccinelle. Do it for a few more manually. >>> >>> Signed-off-by: Markus Armbruster >>> --- >>> blockdev.c | 5 + >>> hw/core/numa.c | 44 ++-- >>> qdev-monitor.c | 11 --- >>> 3 files changed, 19 insertions(+), 41 deletions(-) >>> >>> diff --git a/blockdev.c b/blockdev.c >>> index b66863c42a..73736a4eaf 100644 >>> --- a/blockdev.c >>> +++ b/blockdev.c >>> @@ -1009,13 +1009,10 @@ DriveInfo *drive_new(QemuOpts *all_opts, >>> BlockInterfaceType block_default_type, >>> } >>> /* Actual block device init: Functionality shared with >>> blockdev-add */ >>> -blk = blockdev_init(filename, bs_opts, &local_err); >>> +blk = blockdev_init(filename, bs_opts, errp); >>> bs_opts = NULL; >>> if (!blk) { >>> -error_propagate(errp, local_err); >>> goto fail; >>> -} else { >>> -assert(!local_err); >>> } >>> /* Create legacy DriveInfo */ >>> diff --git a/hw/core/numa.c b/hw/core/numa.c >>> index 5f81900f88..aa8c6be210 100644 >>> --- a/hw/core/numa.c >>> +++ b/hw/core/numa.c >>> @@ -449,40 +449,33 @@ void parse_numa_hmat_cache(MachineState *ms, >>> NumaHmatCacheOptions *node, >>> void set_numa_options(MachineState *ms, NumaOptions *object, >>> Error **errp) >>> { >>> -Error *err = NULL; >>> - >>> if (!ms->numa_state) { >>> error_setg(errp, "NUMA is not supported by this machine-type"); >>> -goto end; >>> +return; >>> } >>> switch (object->type) { >>> case NUMA_OPTIONS_TYPE_NODE: >>> -parse_numa_node(ms, &object->u.node, &err); >>> -if (err) { >>> -goto end; >>> -} >>> +parse_numa_node(ms, &object->u.node, errp); >>> break; >> >> Could we use return here and and for other "break" operators here, to >> stress, that we >> are not going to do something more in case of failure (as well as in case of >> success)? To prevent the future addition of some code after the switch >> without >> handling the error carefully here. > > Can do. Second thoughts: I'd prefer not to mess with it now. The sane way to add code after the switch is to make the parse_numa_FOO() return bool, then bail out like this: if (!parse_numa_node(ms, &object->u.node, errp)) { return; } Too much for me right now. I'm having a hard time getting this ready in time of the freeze. We can always improve on top. [...]
[PATCH v2 1/3] util: validate whether O_DIRECT is supported after failure
Currently we suggest that a filesystem may not support O_DIRECT after seeing an EINVAL. Other things can cause EINVAL though, so it is better to do an explicit check, and then report a definitive error message. Signed-off-by: Daniel P. Berrangé --- util/osdep.c | 15 +++ 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/util/osdep.c b/util/osdep.c index 4829c07ff6..e2b7507ee2 100644 --- a/util/osdep.c +++ b/util/osdep.c @@ -332,9 +332,11 @@ int qemu_open(const char *name, int flags, ...) } #ifdef O_CLOEXEC -ret = open(name, flags | O_CLOEXEC, mode); -#else +flags |= O_CLOEXEC; +#endif ret = open(name, flags, mode); + +#ifndef O_CLOEXEC if (ret >= 0) { qemu_set_cloexec(ret); } @@ -342,8 +344,13 @@ int qemu_open(const char *name, int flags, ...) #ifdef O_DIRECT if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) { -error_report("file system may not support O_DIRECT"); -errno = EINVAL; /* in case it was clobbered */ +int newflags = flags & ~O_DIRECT; +ret = open(name, newflags, mode); +if (ret != -1) { +close(ret); +error_report("file system does not support O_DIRECT"); +errno = EINVAL; +} } #endif /* O_DIRECT */ -- 2.26.2
[PATCH v2 2/3] util: support detailed error reporting for qemu_open
Create a "qemu_open_err" method which does the same as "qemu_open", but with a "Error **errp" for error reporting. There should be no behavioural difference for existing callers at this stage. Signed-off-by: Daniel P. Berrangé --- include/qemu/osdep.h | 2 ++ util/osdep.c | 66 +++- 2 files changed, 55 insertions(+), 13 deletions(-) diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h index 0d26a1b9bd..8506a978fa 100644 --- a/include/qemu/osdep.h +++ b/include/qemu/osdep.h @@ -483,6 +483,8 @@ int qemu_madvise(void *addr, size_t len, int advice); int qemu_mprotect_rwx(void *addr, size_t size); int qemu_mprotect_none(void *addr, size_t size); +/* This is preferred over qemu_open for its improved error reporting */ +int qemu_open_err(const char *name, int flags, Error **errp, ...); int qemu_open(const char *name, int flags, ...); int qemu_close(int fd); int qemu_unlink(const char *name); diff --git a/util/osdep.c b/util/osdep.c index e2b7507ee2..3de8bee463 100644 --- a/util/osdep.c +++ b/util/osdep.c @@ -22,6 +22,7 @@ * THE SOFTWARE. */ #include "qemu/osdep.h" +#include "qapi/error.h" /* Needed early for CONFIG_BSD etc. */ @@ -282,7 +283,7 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive) /* * Opens a file with FD_CLOEXEC set */ -int qemu_open(const char *name, int flags, ...) +static int qemu_openv(const char *name, int flags, Error **errp, va_list ap) { int ret; int mode = 0; @@ -297,24 +298,31 @@ int qemu_open(const char *name, int flags, ...) fdset_id = qemu_parse_fdset(fdset_id_str); if (fdset_id == -1) { +error_setg(errp, "Could not parse fdset %s", name); errno = EINVAL; return -1; } fd = monitor_fdset_get_fd(fdset_id, flags); if (fd < 0) { +error_setg_errno(errp, -fd, "Could not acquire FD for %s flags %x", + name, flags); errno = -fd; return -1; } dupfd = qemu_dup_flags(fd, flags); if (dupfd == -1) { +error_setg_errno(errp, errno, "Could not dup FD for %s flags %x", + name, flags); return -1; } ret = monitor_fdset_dup_fd_add(fdset_id, dupfd); if (ret == -1) { close(dupfd); +error_setg(errp, "Could not save FD for %s flags %x", + name, flags); errno = EINVAL; return -1; } @@ -324,11 +332,7 @@ int qemu_open(const char *name, int flags, ...) #endif if (flags & O_CREAT) { -va_list ap; - -va_start(ap, flags); mode = va_arg(ap, int); -va_end(ap); } #ifdef O_CLOEXEC @@ -342,21 +346,57 @@ int qemu_open(const char *name, int flags, ...) } #endif +if (ret == -1) { +const char *action = "open"; +if (flags & O_CREAT) { +action = "create"; +} #ifdef O_DIRECT -if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) { -int newflags = flags & ~O_DIRECT; -ret = open(name, newflags, mode); -if (ret != -1) { -close(ret); -error_report("file system does not support O_DIRECT"); -errno = EINVAL; +if (errno == EINVAL && (flags & O_DIRECT)) { +int newflags = flags & ~O_DIRECT; +ret = open(name, newflags, mode); +if (ret != -1) { +close(ret); +error_setg(errp, "Could not %s '%s' flags 0x%x: " + "filesystem does not support O_DIRECT", + action, name, flags); +errno = EINVAL; +return -1; +} } -} #endif /* O_DIRECT */ +error_setg_errno(errp, errno, "Could not %s '%s' flags 0x%x", + action, name, flags); +} + return ret; } +int qemu_open_err(const char *name, int flags, Error **errp, ...) +{ +va_list ap; +int rv; + +va_start(ap, errp); +rv = qemu_openv(name, flags, errp, ap); +va_end(ap); + +return rv; +} + +int qemu_open(const char *name, int flags, ...) +{ +va_list ap; +int rv; + +va_start(ap, flags); +rv = qemu_openv(name, flags, NULL, ap); +va_end(ap); + +return rv; +} + int qemu_close(int fd) { int64_t fdset_id; -- 2.26.2
[PATCH v2 0/3] block: improve error reporting for unsupported O_DIRECT
v1: https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg00269.html See patch commit messages for rationale Ideally we would convert other callers of qemu_open to use qemu_open_err, and eventually remove qemu_open, renaming qemu_open_err back to qemu_open. Given soft freeze is just days away though, I'm hoping this series is simple enough to get into this release, leaving bigger cleanup for later. Improved in v2: - Mention that qemu_open_err is preferred over qemu_open - Get rid of obsolete error_report call - Simplify O_DIRECT handling - Fixup iotests for changed error message text Daniel P. Berrangé (3): util: validate whether O_DIRECT is supported after failure util: support detailed error reporting for qemu_open block: switch to use qemu_open_err for improved errors block/file-posix.c| 10 ++--- include/qemu/osdep.h | 2 + tests/qemu-iotests/051.out| 4 +- tests/qemu-iotests/051.pc.out | 4 +- tests/qemu-iotests/061.out| 2 +- tests/qemu-iotests/069.out| 2 +- tests/qemu-iotests/082.out| 4 +- tests/qemu-iotests/111.out| 2 +- tests/qemu-iotests/226.out| 6 +-- tests/qemu-iotests/232.out| 12 +++--- tests/qemu-iotests/244.out| 6 +-- util/osdep.c | 69 +-- 12 files changed, 85 insertions(+), 38 deletions(-) -- 2.26.2
[PATCH v2 3/3] block: switch to use qemu_open_err for improved errors
Currently at startup if using cache=none on a filesystem lacking O_DIRECT such as tmpfs, at startup QEMU prints qemu-system-x86_64: -drive file=/tmp/foo.img,cache=none: file system may not support O_DIRECT qemu-system-x86_64: -drive file=/tmp/foo.img,cache=none: Could not open '/tmp/foo.img': Invalid argument while at QMP level the hint is missing, so QEMU reports just "error": { "class": "GenericError", "desc": "Could not open '/tmp/foo.img': Invalid argument" } which is close to useless for the end user trying to figure out what they did wrong With this change at startup QEMU prints qemu-system-x86_64: -drive file=/tmp/foo.img,cache=none: Unable to open '/tmp/foo.img' flags 0x4000: filesystem does not support O_DIRECT while at the QMP level QEMU reports a massively more informative "error": { "class": "GenericError", "desc": "Unable to open '/tmp/foo.img' flags 0x4002: filesystem does not support O_DIRECT" } Signed-off-by: Daniel P. Berrangé --- block/file-posix.c| 10 -- tests/qemu-iotests/051.out| 4 ++-- tests/qemu-iotests/051.pc.out | 4 ++-- tests/qemu-iotests/061.out| 2 +- tests/qemu-iotests/069.out| 2 +- tests/qemu-iotests/082.out| 4 ++-- tests/qemu-iotests/111.out| 2 +- tests/qemu-iotests/226.out| 6 +++--- tests/qemu-iotests/232.out| 12 ++-- tests/qemu-iotests/244.out| 6 +++--- 10 files changed, 25 insertions(+), 27 deletions(-) diff --git a/block/file-posix.c b/block/file-posix.c index 3ab8f5a0fa..2865b789fb 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -574,11 +574,10 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, raw_parse_flags(bdrv_flags, &s->open_flags, false); s->fd = -1; -fd = qemu_open(filename, s->open_flags, 0644); +fd = qemu_open_err(filename, s->open_flags, errp, 0644); ret = fd < 0 ? -errno : 0; if (ret < 0) { -error_setg_file_open(errp, -ret, filename); if (ret == -EROFS) { ret = -EACCES; } @@ -970,9 +969,8 @@ static int raw_reconfigure_getfd(BlockDriverState *bs, int flags, ret = raw_normalize_devicepath(&normalized_filename, errp); if (ret >= 0) { assert(!(*open_flags & O_CREAT)); -fd = qemu_open(normalized_filename, *open_flags); +fd = qemu_open_err(normalized_filename, *open_flags, errp); if (fd == -1) { -error_setg_errno(errp, errno, "Could not reopen file"); return -1; } } @@ -2324,10 +2322,10 @@ raw_co_create(BlockdevCreateOptions *options, Error **errp) } /* Create file */ -fd = qemu_open(file_opts->filename, O_RDWR | O_CREAT | O_BINARY, 0644); +fd = qemu_open_err(file_opts->filename, O_RDWR | O_CREAT | O_BINARY, + errp, 0644); if (fd < 0) { result = -errno; -error_setg_errno(errp, -result, "Could not create file"); goto out; } diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out index 554c5ca90a..1a80eac0ce 100644 --- a/tests/qemu-iotests/051.out +++ b/tests/qemu-iotests/051.out @@ -363,7 +363,7 @@ Testing: -drive file=foo:bar QEMU_PROG: -drive file=foo:bar: Unknown protocol 'foo' Testing: -drive file.filename=foo:bar -QEMU_PROG: -drive file.filename=foo:bar: Could not open 'foo:bar': No such file or directory +QEMU_PROG: -drive file.filename=foo:bar: Could not open 'foo:bar' flags 0x08000: No such file or directory Testing: -hda file:TEST_DIR/t.qcow2 QEMU X.Y.Z monitor - type 'help' for more information @@ -374,7 +374,7 @@ QEMU X.Y.Z monitor - type 'help' for more information (qemu) quit Testing: -drive file.filename=file:TEST_DIR/t.qcow2 -QEMU_PROG: -drive file.filename=file:TEST_DIR/t.qcow2: Could not open 'file:TEST_DIR/t.qcow2': No such file or directory +QEMU_PROG: -drive file.filename=file:TEST_DIR/t.qcow2: Could not open 'file:TEST_DIR/t.qcow2' flags 0x8: No such file or directory === Snapshot mode === diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out index 0ea80d35f0..bf8f96287b 100644 --- a/tests/qemu-iotests/051.pc.out +++ b/tests/qemu-iotests/051.pc.out @@ -463,7 +463,7 @@ Testing: -drive file=foo:bar QEMU_PROG: -drive file=foo:bar: Unknown protocol 'foo' Testing: -drive file.filename=foo:bar -QEMU_PROG: -drive file.filename=foo:bar: Could not open 'foo:bar': No such file or directory +QEMU_PROG: -drive file.filename=foo:bar: Could not open 'foo:bar' flags 0x8: No such file or directory Testing: -hda file:TEST_DIR/t.qcow2 QEMU X.Y.Z monitor - type 'help' for more information @@ -474,7 +474,7 @@ QEMU X.Y.Z monitor - type 'help' for more information (qemu) quit Testing: -drive file.filename=file:TEST_DIR/t.qcow2 -QEMU_PROG: -drive file.filename=file:TEST_DIR/t.qcow2: Could not open 'file:TEST_DIR/t.qcow2': No such file or directory +QEMU_PROG: -drive file
Re: [PATCH v9 22/34] qcow2: Add subcluster support to zero_in_l2_slice()
On 28.06.20 13:02, Alberto Garcia wrote: > The QCOW_OFLAG_ZERO bit that indicates that a cluster reads as > zeroes is only used in standard L2 entries. Extended L2 entries use > individual 'all zeroes' bits for each subcluster. > > This must be taken into account when updating the L2 entry and also > when deciding that an existing entry does not need to be updated. > > Signed-off-by: Alberto Garcia > Reviewed-by: Eric Blake > --- > block/qcow2-cluster.c | 36 +++- > 1 file changed, 19 insertions(+), 17 deletions(-) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [PATCH 2/4] migration: Add block-bitmap-mapping parameter
02.07.2020 12:41, Max Reitz wrote: As I've said before, it may be reasonable to ignore bitmaps not referenced in the hash-table. No problem with that. We just decided on this behavior when we discussed the RFC. Sorry for that. The reason for my changed opinion is a recent bug from customers about bitmap migration. No problem. (My original proposal was different still, where non-specified mappings would default to the identity mapping.) And, remembering, what my series "[PATCH v2 00/22] Fix error handling during bitmap postcopy" is for, I have one more reason for non-strict behavior around bitmap migration in general: Bitmaps are migrating postcopy. We can't rollback in general, so we should ignore any error during bitmaps migration, as it's non critical data, and we must not break already running target due to some bitmap issue. Failed bitmap migration should never be a problem, the worst consequence is necessity to create a full backup instead of incremental. -- Best regards, Vladimir
Re: [PATCH v9 23/34] qcow2: Add subcluster support to discard_in_l2_slice()
On 28.06.20 13:02, Alberto Garcia wrote: > Two things need to be taken into account here: > > 1) With full_discard == true the L2 entry must be cleared completely. >This also includes the L2 bitmap if the image has extended L2 >entries. > > 2) With full_discard == false we have to make the discarded cluster >read back as zeroes. With normal L2 entries this is done with the >QCOW_OFLAG_ZERO bit, whereas with extended L2 entries this is done >with the individual 'all zeroes' bits for each subcluster. > >Note however that QCOW_OFLAG_ZERO is not supported in v2 qcow2 >images so, if there is a backing file, discard cannot guarantee >that the image will read back as zeroes. If this is important for >the caller it should forbid it as qcow2_co_pdiscard() does (see >80f5c01183 for more details). > > Signed-off-by: Alberto Garcia > Reviewed-by: Eric Blake > --- > block/qcow2-cluster.c | 52 +++ > 1 file changed, 23 insertions(+), 29 deletions(-) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [PATCH v9 24/34] qcow2: Add subcluster support to check_refcounts_l2()
On 28.06.20 13:02, Alberto Garcia wrote: > The offset field of an uncompressed cluster's L2 entry must be aligned > to the cluster size, otherwise it is invalid. If the cluster has no > data then it means that the offset points to a preallocation, so we > can clear the offset field without affecting the guest-visible data. > This is what 'qemu-img check' does when run in repair mode. > > On traditional qcow2 images this can only happen when QCOW_OFLAG_ZERO > is set, and repairing such entries turns the clusters from ZERO_ALLOC > into ZERO_PLAIN. > > Extended L2 entries have no ZERO_ALLOC clusters and no QCOW_OFLAG_ZERO > but the idea is the same: if none of the subclusters are allocated > then we can clear the offset field and leave the bitmap untouched. > > Signed-off-by: Alberto Garcia > --- > block/qcow2-refcount.c | 16 +++- > tests/qemu-iotests/060.out | 2 +- > 2 files changed, 12 insertions(+), 6 deletions(-) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [PATCH v9 25/34] qcow2: Update L2 bitmap in qcow2_alloc_cluster_link_l2()
On 28.06.20 13:02, Alberto Garcia wrote: > The L2 bitmap needs to be updated after each write to indicate what > new subclusters are now allocated. This needs to happen even if the > cluster was already allocated and the L2 entry was otherwise valid. > > In some cases however a write operation doesn't need change the L2 > bitmap (because all affected subclusters were already allocated). This > is detected in calculate_l2_meta(), and qcow2_alloc_cluster_link_l2() > is never called in those cases. > > Signed-off-by: Alberto Garcia > Reviewed-by: Eric Blake > --- > block/qcow2-cluster.c | 18 ++ > 1 file changed, 18 insertions(+) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [PATCH v9 28/34] qcow2: Add subcluster support to qcow2_co_pwrite_zeroes()
On 28.06.20 13:02, Alberto Garcia wrote: > This works now at the subcluster level and pwrite_zeroes_alignment is > updated accordingly. > > qcow2_cluster_zeroize() is turned into qcow2_subcluster_zeroize() with > the following changes: > >- The request can now be subcluster-aligned. > >- The cluster-aligned body of the request is still zeroized using > zero_in_l2_slice() as before. > >- The subcluster-aligned head and tail of the request are zeroized > with the new zero_l2_subclusters() function. > > There is just one thing to take into account for a possible future > improvement: compressed clusters cannot be partially zeroized so > zero_l2_subclusters() on the head or the tail can return -ENOTSUP. > This makes the caller repeat the *complete* request and write actual > zeroes to disk. This is sub-optimal because > >1) if the head area was compressed we would still be able to use > the fast path for the body and possibly the tail. > >2) if the tail area was compressed we are writing zeroes to the > head and the body areas, which are already zeroized. > > Signed-off-by: Alberto Garcia > Reviewed-by: Eric Blake > --- > block/qcow2.h | 4 +-- > block/qcow2-cluster.c | 80 +++ > block/qcow2.c | 27 --- > 3 files changed, 90 insertions(+), 21 deletions(-) [...] > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index deff838fe8..1641976028 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -2015,12 +2015,58 @@ static int zero_in_l2_slice(BlockDriverState *bs, > uint64_t offset, > return nb_clusters; > } > > -int qcow2_cluster_zeroize(BlockDriverState *bs, uint64_t offset, > - uint64_t bytes, int flags) > +static int zero_l2_subclusters(BlockDriverState *bs, uint64_t offset, > + unsigned nb_subclusters) > +{ > +BDRVQcow2State *s = bs->opaque; > +uint64_t *l2_slice; > +uint64_t old_l2_bitmap, l2_bitmap; > +int l2_index, ret, sc = offset_to_sc_index(s, offset); > + > +/* For full clusters use zero_in_l2_slice() instead */ > +assert(nb_subclusters > 0 && nb_subclusters < > s->subclusters_per_cluster); > +assert(sc + nb_subclusters <= s->subclusters_per_cluster); Maybe we should also assert that @offset is aligned to the subcluster size. [...] > diff --git a/block/qcow2.c b/block/qcow2.c > index 86258fbc22..4edc3c72b9 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c [...] > @@ -4367,12 +4367,13 @@ static int coroutine_fn > qcow2_co_truncate(BlockDriverState *bs, int64_t offset, > uint64_t zero_start = QEMU_ALIGN_UP(old_length, s->cluster_size); Can we instead align this to just subclusters? > > /* > - * Use zero clusters as much as we can. qcow2_cluster_zeroize() > + * Use zero clusters as much as we can. qcow2_subcluster_zeroize() > * requires a cluster-aligned start. The end may be unaligned if it > is s/cluster/subcluster/? Max > * at the end of the image (which it is here). > */ > if (offset > zero_start) { > -ret = qcow2_cluster_zeroize(bs, zero_start, offset - zero_start, > 0); > +ret = qcow2_subcluster_zeroize(bs, zero_start, offset - > zero_start, > + 0); > if (ret < 0) { > error_setg_errno(errp, -ret, "Failed to zero out new > clusters"); > goto fail; > signature.asc Description: OpenPGP digital signature
Re: [PATCH v9 30/34] qcow2: Add prealloc field to QCowL2Meta
On 28.06.20 13:02, Alberto Garcia wrote: > This field allows us to indicate that the L2 metadata update does not > come from a write request with actual data but from a preallocation > request. > > For traditional images this does not make any difference, but for > images with extended L2 entries this means that the clusters are > allocated normally in the L2 table but individual subclusters are > marked as unallocated. > > This will allow preallocating images that have a backing file. > > There is one special case: when we resize an existing image we can > also request that the new clusters are preallocated. If the image > already had a backing file then we have to hide any possible stale > data and zero out the new clusters (see commit 955c7d6687 for more > details). > > In this case the subclusters cannot be left as unallocated so the L2 > bitmap must be updated. > > Signed-off-by: Alberto Garcia > Reviewed-by: Eric Blake > --- > block/qcow2.h | 8 > block/qcow2-cluster.c | 2 +- > block/qcow2.c | 6 ++ > 3 files changed, 15 insertions(+), 1 deletion(-) Sounds good, but I’m just not quite sure about the details on falloc/full allocation: With .prealloc = true, writing to the preallocated subclusters will require a COW operation. That’s not ideal, and avoiding those COWs may be a reason to do preallocation in the first place. Now, with backing files, it’s entirely correct. You need a COW operation, because that’s the point of having a backing file. But without a backing file I wonder if it wouldn’t be better to set .prealloc = false to avoid that COW. Of course, if we did that, you couldn’t create the overlay separately from the backing file, preallocate it, and only then attach the backing file to the overlay. But is that a problem? (Or are there other problems to consider?) Max signature.asc Description: OpenPGP digital signature
Re: [PATCH 03/46] qdev: Smooth error checking of qdev_realize() & friends
Markus Armbruster writes: > Vladimir Sementsov-Ogievskiy writes: > >> 24.06.2020 19:43, Markus Armbruster wrote: >>> Convert >>> >>> foo(..., &err); >>> if (err) { >>> ... >>> } >>> >>> to >>> >>> if (!foo(..., &err)) { >>> ... >>> } >>> >>> for qdev_realize(), qdev_realize_and_unref(), qbus_realize() and their >>> wrappers isa_realize_and_unref(), pci_realize_and_unref(), >>> sysbus_realize(), sysbus_realize_and_unref(), usb_realize_and_unref(). >>> Coccinelle script: >> >> Please, also mention a command to run the script >> >>> >>> @@ >>> identifier fun = {isa_realize_and_unref, pci_realize_and_unref, >>> qbus_realize, qdev_realize, qdev_realize_and_unref, sysbus_realize, >>> sysbus_realize_and_unref, usb_realize_and_unref}; >>> expression list args, args2; >>> typedef Error; >>> Error *err; >>> identifier errp; >>> @@ >>> - fun(args, &err, args2); >>> - if (err) { >>> + if (!fun(args, errp, args2)) { >>>... when != err >>> - error_propagate(errp, err); >>>... >>>} >>> >>> @@ >>> identifier fun = {isa_realize_and_unref, pci_realize_and_unref, >>> qbus_realize, qdev_realize, qdev_realize_and_unref, sysbus_realize, >>> sysbus_realize_and_unref, usb_realize_and_unref}; >>> expression list args, args2; >>> typedef Error; >>> Error *err; >>> @@ >>> - fun(args, &err, args2); >>> - if (err) { >>> + if (!fun(args, &err, args2)) { >>>... >>>} >>> >>> Fails to convert hw/arm/armsse.c, because Coccinelle gets confused by >>> ARMSSE being used both as typedef and function-like macro there. >>> Convert manually. >>> >>> Eliminate error_propagate() that are now unnecessary. Delete @err >>> that are now unused. Clean up whitespace. >>> >>> Signed-off-by: Markus Armbruster >>> --- >>> hw/arm/allwinner-a10.c | 21 ++- >>> hw/arm/armsse.c | 104 >>> hw/arm/armv7m.c | 12 +--- >>> hw/arm/aspeed_ast2600.c | 68 ++--- >>> hw/arm/aspeed_soc.c | 60 +- >>> hw/arm/bcm2835_peripherals.c| 60 +- >>> hw/arm/bcm2836.c| 12 +--- >>> hw/arm/cubieboard.c | 3 +- >>> hw/arm/digic.c | 12 +--- >>> hw/arm/digic_boards.c | 3 +- >>> hw/arm/fsl-imx25.c | 44 -- >>> hw/arm/fsl-imx31.c | 32 +++--- >>> hw/arm/fsl-imx6.c | 48 --- >>> hw/arm/msf2-soc.c | 21 ++- >>> hw/arm/nrf51_soc.c | 24 ++-- >>> hw/arm/stm32f205_soc.c | 29 +++-- >>> hw/arm/stm32f405_soc.c | 32 +++--- >>> hw/arm/xlnx-zynqmp.c| 61 +-- >>> hw/block/fdc.c | 4 +- >>> hw/block/xen-block.c| 3 +- >>> hw/char/serial-pci-multi.c | 5 +- >>> hw/char/serial-pci.c| 5 +- >>> hw/char/serial.c| 10 +-- >>> hw/core/cpu.c | 3 +- >>> hw/cpu/a15mpcore.c | 5 +- >>> hw/cpu/a9mpcore.c | 21 ++- >>> hw/cpu/arm11mpcore.c| 17 ++ >>> hw/cpu/realview_mpcore.c| 9 +-- >>> hw/display/virtio-gpu-pci.c | 6 +- >>> hw/display/virtio-vga.c | 5 +- >>> hw/intc/armv7m_nvic.c | 9 +-- >>> hw/intc/pnv_xive.c | 8 +-- >>> hw/intc/realview_gic.c | 5 +- >>> hw/intc/spapr_xive.c| 8 +-- >>> hw/intc/xics.c | 5 +- >>> hw/intc/xive.c | 3 +- >>> hw/isa/piix4.c | 5 +- >>> hw/microblaze/xlnx-zynqmp-pmu.c | 9 +-- >>> hw/mips/cps.c | 17 ++ >>> hw/misc/macio/cuda.c| 5 +- >>> hw/misc/macio/macio.c | 25 ++-- >>> hw/misc/macio/pmu.c | 5 +- >>> hw/pci-host/pnv_phb3.c | 13 +--- >>> hw/pci-host/pnv_phb4.c | 5 +- >>> hw/pci-host/pnv_phb4_pec.c | 5 +- >>> hw/ppc/e500.c | 5 +- >>> hw/ppc/pnv.c| 53 >>> hw/ppc/pnv_core.c | 4 +- >>> hw/ppc/pnv_psi.c| 9 +-- >>> hw/ppc/spapr_cpu_core.c | 3 +- >>> hw/ppc/spapr_irq.c | 5 +- >>> hw/riscv/opentitan.c| 9 +-- >>> hw/riscv/sifive_e.c | 6 +- >>> hw/riscv/sifive_u.c | 5 +- >>> hw/s390x/event-facility.c | 13 ++-- >>> hw/s390x/s390-pci-bus.c | 3 +- >>> hw/s390x/sclp.c | 3 +- >>> hw/s390x/virtio-ccw-crypto.c| 5 +- >>> hw/s390x/virtio-ccw-rng.c | 5 +- >>> hw/scsi/scsi-bus.c | 4 +- >>> hw/sd/aspeed_sdhci.c
Re: [PATCH v9 30/34] qcow2: Add prealloc field to QCowL2Meta
On 7/2/20 9:50 AM, Max Reitz wrote: On 28.06.20 13:02, Alberto Garcia wrote: This field allows us to indicate that the L2 metadata update does not come from a write request with actual data but from a preallocation request. For traditional images this does not make any difference, but for images with extended L2 entries this means that the clusters are allocated normally in the L2 table but individual subclusters are marked as unallocated. This will allow preallocating images that have a backing file. There is one special case: when we resize an existing image we can also request that the new clusters are preallocated. If the image already had a backing file then we have to hide any possible stale data and zero out the new clusters (see commit 955c7d6687 for more details). In this case the subclusters cannot be left as unallocated so the L2 bitmap must be updated. Signed-off-by: Alberto Garcia Reviewed-by: Eric Blake --- block/qcow2.h | 8 block/qcow2-cluster.c | 2 +- block/qcow2.c | 6 ++ 3 files changed, 15 insertions(+), 1 deletion(-) Sounds good, but I’m just not quite sure about the details on falloc/full allocation: With .prealloc = true, writing to the preallocated subclusters will require a COW operation. That’s not ideal, and avoiding those COWs may be a reason to do preallocation in the first place. I'm not sure I follow the complaint. If a cluster is preallocated but the subcluster is marked unallocated, then doing a partial write to that subcluster must provide the correct contents for the rest of the subcluster (either filling with zero, or reading from a backing file) - but this COW can be limited to just the portion of the subcluster, and is no different than the COW you have to perform without subclusters when doing a write to a preallocated cluster in general. Now, with backing files, it’s entirely correct. You need a COW operation, because that’s the point of having a backing file. But without a backing file I wonder if it wouldn’t be better to set .prealloc = false to avoid that COW. Without a backing file, there is no read required - writing to an unallocated subcluster within a preallocated cluster merely has to provide zeros to the rest of the write. And depending on whether we can intelligently guarantee that the underlying protocol already reads as zeroes when preallocated, we even have an optimization where even that is not necessary. We can still lump it in the "COW" terminology, in that our write is more complex than merely writing in place, but it isn't a true copy-on-write operation as there is nothing to be copied. Of course, if we did that, you couldn’t create the overlay separately from the backing file, preallocate it, and only then attach the backing file to the overlay. But is that a problem? (Or are there other problems to consider?) Max -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v4 2/2] nvme: allow cmb and pmr to be enabled on same device
On 7/2/20 3:31 AM, Klaus Jensen wrote: > Aight, an update here. This only happens when QEMU is run with a virtual > IOMMU. Otherwise, the kernel is happy. > > With the vIOMMU, qemu also craps out a bit: > > qemu-system-x86_64: vtd_iova_to_slpte: detected slpte permission error > (iova=0xfd20, level=0x2, slpte=0x0, write=0) > qemu-system-x86_64: vtd_iommu_translate: detected translation failure > (dev=03:00:00, iova=0xfd20) > > So I think we are back in QEMU land for the bug. Can you share command line for that?
Re: [PATCH v9 30/34] qcow2: Add prealloc field to QCowL2Meta
On 02.07.20 16:58, Eric Blake wrote: > On 7/2/20 9:50 AM, Max Reitz wrote: >> On 28.06.20 13:02, Alberto Garcia wrote: >>> This field allows us to indicate that the L2 metadata update does not >>> come from a write request with actual data but from a preallocation >>> request. >>> >>> For traditional images this does not make any difference, but for >>> images with extended L2 entries this means that the clusters are >>> allocated normally in the L2 table but individual subclusters are >>> marked as unallocated. >>> >>> This will allow preallocating images that have a backing file. >>> >>> There is one special case: when we resize an existing image we can >>> also request that the new clusters are preallocated. If the image >>> already had a backing file then we have to hide any possible stale >>> data and zero out the new clusters (see commit 955c7d6687 for more >>> details). >>> >>> In this case the subclusters cannot be left as unallocated so the L2 >>> bitmap must be updated. >>> >>> Signed-off-by: Alberto Garcia >>> Reviewed-by: Eric Blake >>> --- >>> block/qcow2.h | 8 >>> block/qcow2-cluster.c | 2 +- >>> block/qcow2.c | 6 ++ >>> 3 files changed, 15 insertions(+), 1 deletion(-) >> >> Sounds good, but I’m just not quite sure about the details on >> falloc/full allocation: With .prealloc = true, writing to the >> preallocated subclusters will require a COW operation. That’s not >> ideal, and avoiding those COWs may be a reason to do preallocation in >> the first place. > > I'm not sure I follow the complaint. If a cluster is preallocated but > the subcluster is marked unallocated, then doing a partial write to that > subcluster must provide the correct contents for the rest of the > subcluster (either filling with zero, or reading from a backing file) - > but this COW can be limited to just the portion of the subcluster, and > is no different than the COW you have to perform without subclusters > when doing a write to a preallocated cluster in general. It was my impression that falloc/full preallocation would create normal data clusters, not zero clusters, so no COW was necessary when writing to them. >> Now, with backing files, it’s entirely correct. You need a COW >> operation, because that’s the point of having a backing file. >> >> But without a backing file I wonder if it wouldn’t be better to set >> .prealloc = false to avoid that COW. > > Without a backing file, there is no read required - writing to an > unallocated subcluster within a preallocated cluster merely has to > provide zeros to the rest of the write. And depending on whether we can > intelligently guarantee that the underlying protocol already reads as > zeroes when preallocated, we even have an optimization where even that > is not necessary. We can still lump it in the "COW" terminology, in > that our write is more complex than merely writing in place, but it > isn't a true copy-on-write operation as there is nothing to be copied. The term “COW” specifically in the qcow2 driver also refers to having to write zeroes to an area that isn’t written to by the guest as part of the process of having to allocate a (sub)cluster. (Of course there is no COW from a backing file if there is no backing file.) Max signature.asc Description: OpenPGP digital signature
Re: [PATCH v9 31/34] qcow2: Add the 'extended_l2' option and the QCOW2_INCOMPAT_EXTL2 bit
On 28.06.20 13:02, Alberto Garcia wrote: > Now that the implementation of subclusters is complete we can finally > add the necessary options to create and read images with this feature, > which we call "extended L2 entries". > > Signed-off-by: Alberto Garcia > Reviewed-by: Eric Blake > --- > qapi/block-core.json | 7 +++ > block/qcow2.h| 8 ++- > include/block/block_int.h| 1 + > block/qcow2.c| 74 -- > tests/qemu-iotests/031.out | 8 +-- > tests/qemu-iotests/036.out | 4 +- > tests/qemu-iotests/049.out | 102 +++ > tests/qemu-iotests/060.out | 1 + > tests/qemu-iotests/061.out | 20 +++--- > tests/qemu-iotests/065 | 12 ++-- > tests/qemu-iotests/082.out | 48 --- > tests/qemu-iotests/085.out | 38 ++-- > tests/qemu-iotests/144.out | 4 +- > tests/qemu-iotests/182.out | 2 +- > tests/qemu-iotests/185.out | 8 +-- > tests/qemu-iotests/198.out | 2 + > tests/qemu-iotests/206.out | 4 ++ > tests/qemu-iotests/242.out | 5 ++ > tests/qemu-iotests/255.out | 8 +-- > tests/qemu-iotests/274.out | 49 --- > tests/qemu-iotests/280.out | 2 +- > tests/qemu-iotests/291.out | 6 +- > tests/qemu-iotests/common.filter | 1 + > 23 files changed, 272 insertions(+), 142 deletions(-) Looks OK. Reviewed-by: Max Reitz [...] > diff --git a/block/qcow2.c b/block/qcow2.c > index 003f166024..37bfae823c 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c [...] > @@ -5491,6 +5539,14 @@ static int qcow2_amend_options(BlockDriverState *bs, > QemuOpts *opts, > "is not supported"); > return -ENOTSUP; > } > +} else if (!strcmp(desc->name, BLOCK_OPT_EXTL2)) { > +bool extended_l2 = qemu_opt_get_bool(opts, BLOCK_OPT_EXTL2, > + has_subclusters(s)); > +if (extended_l2 != has_subclusters(s)) { > +error_setg(errp, "Toggling extended L2 entries " > + "is not supported"); > +return -EINVAL; Just a note: I think ENOTSUP would fit better. (Then again, Maxim’s “block/amend: refactor qcow2 amend options” removes all of this code anyway.) signature.asc Description: OpenPGP digital signature
Re: [PATCH v2 1/3] util: validate whether O_DIRECT is supported after failure
On 7/2/20 2:56 PM, Daniel P. Berrangé wrote: > Currently we suggest that a filesystem may not support O_DIRECT after > seeing an EINVAL. Other things can cause EINVAL though, so it is better > to do an explicit check, and then report a definitive error message. > > Signed-off-by: Daniel P. Berrangé > --- > util/osdep.c | 15 +++ > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/util/osdep.c b/util/osdep.c > index 4829c07ff6..e2b7507ee2 100644 > --- a/util/osdep.c > +++ b/util/osdep.c > @@ -332,9 +332,11 @@ int qemu_open(const char *name, int flags, ...) > } > > #ifdef O_CLOEXEC > -ret = open(name, flags | O_CLOEXEC, mode); > -#else > +flags |= O_CLOEXEC; > +#endif > ret = open(name, flags, mode); > + > +#ifndef O_CLOEXEC > if (ret >= 0) { > qemu_set_cloexec(ret); > } > @@ -342,8 +344,13 @@ int qemu_open(const char *name, int flags, ...) Too bad the git-diff removed 2 lines of context to add a comment instead. In case it helps other reviewers, what was stripped out of the context is this single line: #endif Reviewed-by: Philippe Mathieu-Daudé > > #ifdef O_DIRECT > if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) { > -error_report("file system may not support O_DIRECT"); > -errno = EINVAL; /* in case it was clobbered */ > +int newflags = flags & ~O_DIRECT; > +ret = open(name, newflags, mode); > +if (ret != -1) { > +close(ret); > +error_report("file system does not support O_DIRECT"); > +errno = EINVAL; > +} > } > #endif /* O_DIRECT */ > >
[PATCH v2 02/44] error: Document Error API usage rules
This merely codifies existing practice, with one exception: the rule advising against returning void, where existing practice is mixed. When the Error API was created, we adopted the (unwritten) rule to return void when the function returns no useful value on success, unlike GError, which recommends to return true on success and false on error then. When a function returns a distinct error value, say false, a checked call that passes the error up looks like if (!frobnicate(..., errp)) { handle the error... } When it returns void, we need Error *err = NULL; frobnicate(..., &err); if (err) { handle the error... error_propagate(errp, err); } Not only is this more verbose, it also creates an Error object even when @errp is null, &error_abort or &error_fatal. People got tired of the additional boilerplate, and started to ignore the unwritten rule. The result is confusion among developers about the preferred usage. The written rule will hopefully reduce the confusion. The remainder of this series will update a substantial amount of code to honor the rule. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Greg Kurz --- include/qapi/error.h | 26 ++ 1 file changed, 26 insertions(+) diff --git a/include/qapi/error.h b/include/qapi/error.h index 3e64324b7a..5ceb3ace06 100644 --- a/include/qapi/error.h +++ b/include/qapi/error.h @@ -15,6 +15,32 @@ /* * Error reporting system loosely patterned after Glib's GError. * + * Rules: + * + * - Functions that use Error to report errors have an Error **errp + * parameter. It should be the last parameter, except for functions + * taking variable arguments. + * + * - You may pass NULL to not receive the error, &error_abort to abort + * on error, &error_fatal to exit(1) on error, or a pointer to a + * variable containing NULL to receive the error. + * + * - The value of @errp should not affect control flow. + * + * - On success, the function should not use @errp. On failure, it + * should set a new error, e.g. with error_setg(errp, ...), or + * propagate an existing one, e.g. with error_propagate(errp, ...). + * + * - Whenever practical, also return a value that indicates success / + * failure. This can make the error checking more concise, and can + * avoid useless error object creation and destruction. Note that + * we still have many functions returning void. We recommend + * • bool-valued functions return true on success / false on failure, + * • pointer-valued functions return non-null / null pointer, and + * • integer-valued functions return non-negative / negative. + * + * How to: + * * Create an error: * error_setg(errp, "situation normal, all fouled up"); * -- 2.26.2
[PATCH v2 21/44] qom: Use error_reportf_err() instead of g_printerr() in examples
Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- include/qom/object.h | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/include/qom/object.h b/include/qom/object.h index 94a61ccc3f..b70edd8cd9 100644 --- a/include/qom/object.h +++ b/include/qom/object.h @@ -671,8 +671,7 @@ Object *object_new(const char *typename); * NULL); * * if (!obj) { - * g_printerr("Cannot create memory backend: %s\n", - *error_get_pretty(err)); + * error_reportf_err(err, "Cannot create memory backend: "); * } * * @@ -739,8 +738,7 @@ void object_apply_compat_props(Object *obj); * NULL); * * if (!obj) { - * g_printerr("Cannot set properties: %s\n", - *error_get_pretty(err)); + * error_reportf_err(err, "Cannot set properties: "); * } * * -- 2.26.2
[PATCH v2 08/44] qemu-option: Factor out helper find_default_by_name()
Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy --- util/qemu-option.c | 47 ++ 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/util/qemu-option.c b/util/qemu-option.c index 1df55bc881..14e211ddd8 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -142,6 +142,13 @@ static const QemuOptDesc *find_desc_by_name(const QemuOptDesc *desc, return NULL; } +static const char *find_default_by_name(QemuOpts *opts, const char *name) +{ +const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name); + +return desc ? desc->def_value_str : NULL; +} + void parse_option_size(const char *name, const char *value, uint64_t *ret, Error **errp) { @@ -270,7 +277,7 @@ static void qemu_opt_del_all(QemuOpts *opts, const char *name) const char *qemu_opt_get(QemuOpts *opts, const char *name) { QemuOpt *opt; -const QemuOptDesc *desc; +const char *def_val; if (opts == NULL) { return NULL; @@ -278,9 +285,9 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name) opt = qemu_opt_find(opts, name); if (!opt) { -desc = find_desc_by_name(opts->list->desc, name); -if (desc && desc->def_value_str) { -return desc->def_value_str; +def_val = find_default_by_name(opts, name); +if (def_val) { +return def_val; } } return opt ? opt->str : NULL; @@ -312,7 +319,7 @@ const char *qemu_opt_iter_next(QemuOptsIter *iter) char *qemu_opt_get_del(QemuOpts *opts, const char *name) { QemuOpt *opt; -const QemuOptDesc *desc; +const char *def_val; char *str = NULL; if (opts == NULL) { @@ -321,9 +328,9 @@ char *qemu_opt_get_del(QemuOpts *opts, const char *name) opt = qemu_opt_find(opts, name); if (!opt) { -desc = find_desc_by_name(opts->list->desc, name); -if (desc && desc->def_value_str) { -str = g_strdup(desc->def_value_str); +def_val = find_default_by_name(opts, name); +if (def_val) { +str = g_strdup(def_val); } return str; } @@ -349,7 +356,7 @@ static bool qemu_opt_get_bool_helper(QemuOpts *opts, const char *name, bool defval, bool del) { QemuOpt *opt; -const QemuOptDesc *desc; +const char *def_val; bool ret = defval; if (opts == NULL) { @@ -358,9 +365,9 @@ static bool qemu_opt_get_bool_helper(QemuOpts *opts, const char *name, opt = qemu_opt_find(opts, name); if (opt == NULL) { -desc = find_desc_by_name(opts->list->desc, name); -if (desc && desc->def_value_str) { -parse_option_bool(name, desc->def_value_str, &ret, &error_abort); +def_val = find_default_by_name(opts, name); +if (def_val) { +parse_option_bool(name, def_val, &ret, &error_abort); } return ret; } @@ -386,7 +393,7 @@ static uint64_t qemu_opt_get_number_helper(QemuOpts *opts, const char *name, uint64_t defval, bool del) { QemuOpt *opt; -const QemuOptDesc *desc; +const char *def_val; uint64_t ret = defval; if (opts == NULL) { @@ -395,9 +402,9 @@ static uint64_t qemu_opt_get_number_helper(QemuOpts *opts, const char *name, opt = qemu_opt_find(opts, name); if (opt == NULL) { -desc = find_desc_by_name(opts->list->desc, name); -if (desc && desc->def_value_str) { -parse_option_number(name, desc->def_value_str, &ret, &error_abort); +def_val = find_default_by_name(opts, name); +if (def_val) { +parse_option_number(name, def_val, &ret, &error_abort); } return ret; } @@ -424,7 +431,7 @@ static uint64_t qemu_opt_get_size_helper(QemuOpts *opts, const char *name, uint64_t defval, bool del) { QemuOpt *opt; -const QemuOptDesc *desc; +const char *def_val; uint64_t ret = defval; if (opts == NULL) { @@ -433,9 +440,9 @@ static uint64_t qemu_opt_get_size_helper(QemuOpts *opts, const char *name, opt = qemu_opt_find(opts, name); if (opt == NULL) { -desc = find_desc_by_name(opts->list->desc, name); -if (desc && desc->def_value_str) { -parse_option_size(name, desc->def_value_str, &ret, &error_abort); +def_val = find_default_by_name(opts, name); +if (def_val) { +parse_option_size(name, def_val, &ret, &error_abort); } return ret; } -- 2.26.2
[PATCH v2 04/44] macio: Tidy up error handling in macio_newworld_realize()
macio_newworld_realize() effectively ignores ns->gpio realization errors, leaking the Error object. Fortunately, macio_gpio_realize() can't actually fail. Tidy up. Cc: Mark Cave-Ayland Cc: David Gibson Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Acked-by: David Gibson Reviewed-by: Vladimir Sementsov-Ogievskiy --- hw/misc/macio/macio.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c index 42414797e2..be66bb7758 100644 --- a/hw/misc/macio/macio.c +++ b/hw/misc/macio/macio.c @@ -334,7 +334,9 @@ static void macio_newworld_realize(PCIDevice *d, Error **errp) &error_abort); memory_region_add_subregion(&s->bar, 0x50, sysbus_mmio_get_region(sysbus_dev, 0)); -qdev_realize(DEVICE(&ns->gpio), BUS(&s->macio_bus), &err); +if (!qdev_realize(DEVICE(&ns->gpio), BUS(&s->macio_bus), errp)) { +return; +} /* PMU */ object_initialize_child(OBJECT(s), "pmu", &s->pmu, TYPE_VIA_PMU); -- 2.26.2
[PATCH v2 11/44] qemu-option: Replace opt_set() by cleaner opt_validate()
opt_set() frees its argument @value on failure. Slightly unclean; functions ideally do nothing on failure. To tidy this up, move opt_create() from opt_set() into its callers, along with the cleanup. Rename opt_set() to opt_validate(), noting its similarity to qemu_opts_validate(). Drop redundant parameter @opts; use opt->opts instead. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- util/qemu-option.c | 35 +++ 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/util/qemu-option.c b/util/qemu-option.c index 1023fe7527..d8233b3b35 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -516,36 +516,39 @@ static QemuOpt *opt_create(QemuOpts *opts, const char *name, char *value, return opt; } -static void opt_set(QemuOpts *opts, const char *name, char *value, -bool prepend, bool *help_wanted, Error **errp) +static bool opt_validate(QemuOpt *opt, bool *help_wanted, + Error **errp) { -QemuOpt *opt; const QemuOptDesc *desc; Error *local_err = NULL; -desc = find_desc_by_name(opts->list->desc, name); -if (!desc && !opts_accepts_any(opts)) { -g_free(value); -error_setg(errp, QERR_INVALID_PARAMETER, name); -if (help_wanted && is_help_option(name)) { +desc = find_desc_by_name(opt->opts->list->desc, opt->name); +if (!desc && !opts_accepts_any(opt->opts)) { +error_setg(errp, QERR_INVALID_PARAMETER, opt->name); +if (help_wanted && is_help_option(opt->name)) { *help_wanted = true; } -return; +return false; } -opt = opt_create(opts, name, value, prepend); opt->desc = desc; qemu_opt_parse(opt, &local_err); if (local_err) { error_propagate(errp, local_err); -qemu_opt_del(opt); +return false; } + +return true; } void qemu_opt_set(QemuOpts *opts, const char *name, const char *value, Error **errp) { -opt_set(opts, name, g_strdup(value), false, NULL, errp); +QemuOpt *opt = opt_create(opts, name, g_strdup(value), false); + +if (!opt_validate(opt, NULL, errp)) { +qemu_opt_del(opt); +} } void qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val, @@ -817,9 +820,9 @@ static void opts_do_parse(QemuOpts *opts, const char *params, const char *firstname, bool prepend, bool *help_wanted, Error **errp) { -Error *local_err = NULL; char *option, *value; const char *p; +QemuOpt *opt; for (p = params; *p;) { p = get_opt_name_value(p, firstname, &option, &value); @@ -831,10 +834,10 @@ static void opts_do_parse(QemuOpts *opts, const char *params, continue; } -opt_set(opts, option, value, prepend, help_wanted, &local_err); +opt = opt_create(opts, option, value, prepend); g_free(option); -if (local_err) { -error_propagate(errp, local_err); +if (!opt_validate(opt, help_wanted, errp)) { +qemu_opt_del(opt); return; } } -- 2.26.2
[PATCH v2 01/44] error: Improve examples in error.h's big comment
Show errp instead of &err where &err is actually unusual. Add a missing declaration. Add a second error pileup example. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Greg Kurz --- include/qapi/error.h | 19 +++ 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/include/qapi/error.h b/include/qapi/error.h index ad5b6e896d..3e64324b7a 100644 --- a/include/qapi/error.h +++ b/include/qapi/error.h @@ -16,15 +16,15 @@ * Error reporting system loosely patterned after Glib's GError. * * Create an error: - * error_setg(&err, "situation normal, all fouled up"); + * error_setg(errp, "situation normal, all fouled up"); * * Create an error and add additional explanation: - * error_setg(&err, "invalid quark"); - * error_append_hint(&err, "Valid quarks are up, down, strange, " + * error_setg(errp, "invalid quark"); + * error_append_hint(errp, "Valid quarks are up, down, strange, " * "charm, top, bottom.\n"); * * Do *not* contract this to - * error_setg(&err, "invalid quark\n" + * error_setg(errp, "invalid quark\n" // WRONG! *"Valid quarks are up, down, strange, charm, top, bottom."); * * Report an error to the current monitor if we have one, else stderr: @@ -108,12 +108,23 @@ * } * * Do *not* "optimize" this to + * Error *err = NULL; * foo(arg, &err); * bar(arg, &err); // WRONG! * if (err) { * handle the error... * } * because this may pass a non-null err to bar(). + * + * Likewise, do *not* + * Error *err = NULL; + * if (cond1) { + * error_setg(&err, ...); + * } + * if (cond2) { + * error_setg(&err, ...); // WRONG! + * } + * because this may pass a non-null err to error_setg(). */ #ifndef ERROR_H -- 2.26.2
[PATCH v2 24/44] qom: Don't handle impossible object_property_get_link() failure
Don't handle object_property_get_link() failure that can't happen unless the programmer screwed up, pass &error_abort. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Philippe Mathieu-Daudé --- hw/arm/bcm2835_peripherals.c | 7 +-- hw/arm/bcm2836.c | 7 +-- hw/display/bcm2835_fb.c | 8 +--- hw/dma/bcm2835_dma.c | 9 + hw/gpio/bcm2835_gpio.c | 15 ++- hw/intc/nios2_iic.c | 8 +--- hw/misc/bcm2835_mbox.c | 9 + hw/misc/bcm2835_property.c | 17 ++--- hw/usb/hcd-dwc2.c| 9 + 9 files changed, 11 insertions(+), 78 deletions(-) diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c index 2df81168e4..beade39e41 100644 --- a/hw/arm/bcm2835_peripherals.c +++ b/hw/arm/bcm2835_peripherals.c @@ -134,12 +134,7 @@ static void bcm2835_peripherals_realize(DeviceState *dev, Error **errp) uint64_t ram_size, vcram_size; int n; -obj = object_property_get_link(OBJECT(dev), "ram", &err); -if (obj == NULL) { -error_setg(errp, "%s: required ram link not found: %s", - __func__, error_get_pretty(err)); -return; -} +obj = object_property_get_link(OBJECT(dev), "ram", &error_abort); ram = MEMORY_REGION(obj); ram_size = memory_region_size(ram); diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c index 1a7560ef30..70ca2f0d9a 100644 --- a/hw/arm/bcm2836.c +++ b/hw/arm/bcm2836.c @@ -77,12 +77,7 @@ static void bcm2836_realize(DeviceState *dev, Error **errp) /* common peripherals from bcm2835 */ -obj = object_property_get_link(OBJECT(dev), "ram", &err); -if (obj == NULL) { -error_setg(errp, "%s: required ram link not found: %s", - __func__, error_get_pretty(err)); -return; -} +obj = object_property_get_link(OBJECT(dev), "ram", &error_abort); object_property_add_const_link(OBJECT(&s->peripherals), "ram", obj); diff --git a/hw/display/bcm2835_fb.c b/hw/display/bcm2835_fb.c index c6263808a2..c4bfed2740 100644 --- a/hw/display/bcm2835_fb.c +++ b/hw/display/bcm2835_fb.c @@ -401,7 +401,6 @@ static void bcm2835_fb_reset(DeviceState *dev) static void bcm2835_fb_realize(DeviceState *dev, Error **errp) { BCM2835FBState *s = BCM2835_FB(dev); -Error *err = NULL; Object *obj; if (s->vcram_base == 0) { @@ -409,12 +408,7 @@ static void bcm2835_fb_realize(DeviceState *dev, Error **errp) return; } -obj = object_property_get_link(OBJECT(dev), "dma-mr", &err); -if (obj == NULL) { -error_setg(errp, "%s: required dma-mr link not found: %s", - __func__, error_get_pretty(err)); -return; -} +obj = object_property_get_link(OBJECT(dev), "dma-mr", &error_abort); /* Fill in the parts of initial_config that are not set by QOM properties */ s->initial_config.xres_virtual = s->initial_config.xres; diff --git a/hw/dma/bcm2835_dma.c b/hw/dma/bcm2835_dma.c index 4cd9dab745..eb0002a2b9 100644 --- a/hw/dma/bcm2835_dma.c +++ b/hw/dma/bcm2835_dma.c @@ -376,16 +376,9 @@ static void bcm2835_dma_reset(DeviceState *dev) static void bcm2835_dma_realize(DeviceState *dev, Error **errp) { BCM2835DMAState *s = BCM2835_DMA(dev); -Error *err = NULL; Object *obj; -obj = object_property_get_link(OBJECT(dev), "dma-mr", &err); -if (obj == NULL) { -error_setg(errp, "%s: required dma-mr link not found: %s", - __func__, error_get_pretty(err)); -return; -} - +obj = object_property_get_link(OBJECT(dev), "dma-mr", &error_abort); s->dma_mr = MEMORY_REGION(obj); address_space_init(&s->dma_as, s->dma_mr, TYPE_BCM2835_DMA "-memory"); diff --git a/hw/gpio/bcm2835_gpio.c b/hw/gpio/bcm2835_gpio.c index 91ce3d10cc..abdddbc67c 100644 --- a/hw/gpio/bcm2835_gpio.c +++ b/hw/gpio/bcm2835_gpio.c @@ -312,22 +312,11 @@ static void bcm2835_gpio_realize(DeviceState *dev, Error **errp) { BCM2835GpioState *s = BCM2835_GPIO(dev); Object *obj; -Error *err = NULL; -obj = object_property_get_link(OBJECT(dev), "sdbus-sdhci", &err); -if (obj == NULL) { -error_setg(errp, "%s: required sdhci link not found: %s", -__func__, error_get_pretty(err)); -return; -} +obj = object_property_get_link(OBJECT(dev), "sdbus-sdhci", &error_abort); s->sdbus_sdhci = SD_BUS(obj); -obj = object_property_get_link(OBJECT(dev), "sdbus-sdhost", &err); -if (obj == NULL) { -error_setg(errp, "%s: required sdhost link not found: %s", -__func__, error_get_pretty(err)); -return; -} +obj = object_property_get_link(OBJECT(dev), "sdbus-sdhost", &error_abort); s->sdbus_sdhost = SD_BUS(obj); } diff --git a/hw/intc/nios2_iic.c b/hw/intc/nios2_iic.c index 3a5d86c2a4..1a5df8c89a 100644 --- a/hw/intc/nios2_iic.c +++ b/hw/intc/nios2_iic.c @@ -66,14
[PATCH v2 09/44] qemu-option: Simplify around find_default_by_name()
Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy --- util/qemu-option.c | 18 +- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/util/qemu-option.c b/util/qemu-option.c index 14e211ddd8..e7b540a21b 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -277,7 +277,6 @@ static void qemu_opt_del_all(QemuOpts *opts, const char *name) const char *qemu_opt_get(QemuOpts *opts, const char *name) { QemuOpt *opt; -const char *def_val; if (opts == NULL) { return NULL; @@ -285,12 +284,10 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name) opt = qemu_opt_find(opts, name); if (!opt) { -def_val = find_default_by_name(opts, name); -if (def_val) { -return def_val; -} +return find_default_by_name(opts, name); } -return opt ? opt->str : NULL; + +return opt->str; } void qemu_opt_iter_init(QemuOptsIter *iter, QemuOpts *opts, const char *name) @@ -319,8 +316,7 @@ const char *qemu_opt_iter_next(QemuOptsIter *iter) char *qemu_opt_get_del(QemuOpts *opts, const char *name) { QemuOpt *opt; -const char *def_val; -char *str = NULL; +char *str; if (opts == NULL) { return NULL; @@ -328,11 +324,7 @@ char *qemu_opt_get_del(QemuOpts *opts, const char *name) opt = qemu_opt_find(opts, name); if (!opt) { -def_val = find_default_by_name(opts, name); -if (def_val) { -str = g_strdup(def_val); -} -return str; +return g_strdup(find_default_by_name(opts, name)); } str = opt->str; opt->str = NULL; -- 2.26.2
[PATCH v2 28/44] qom: Use returned bool to check for failure, Coccinelle part
The previous commit enables conversion of foo(..., &err); if (err) { ... } to if (!foo(..., errp)) { ... } for QOM functions that now return true / false on success / error. Coccinelle script: @@ identifier fun = {object_apply_global_props, object_initialize_child_with_props, object_initialize_child_with_propsv, object_property_get, object_property_get_bool, object_property_parse, object_property_set, object_property_set_bool, object_property_set_int, object_property_set_link, object_property_set_qobject, object_property_set_str, object_property_set_uint, object_set_props, object_set_propv, user_creatable_add_dict, user_creatable_complete, user_creatable_del}; expression list args, args2; typedef Error; Error *err; @@ -fun(args, &err, args2); -if (err) +if (!fun(args, &err, args2)) { ... } Fails to convert hw/arm/armsse.c, because Coccinelle gets confused by ARMSSE being used both as typedef and function-like macro there. Convert manually. Line breaks tidied up manually. Signed-off-by: Markus Armbruster --- hw/arm/armsse.c | 52 +++- hw/arm/armv7m.c | 20 ++ hw/arm/aspeed_ast2600.c | 5 ++-- hw/arm/aspeed_soc.c | 5 ++-- hw/arm/bcm2835_peripherals.c | 5 ++-- hw/arm/bcm2836.c | 12 - hw/arm/cubieboard.c | 11 hw/arm/digic.c | 4 +-- hw/arm/nrf51_soc.c | 8 +++--- hw/arm/stm32f405_soc.c | 7 +++-- hw/arm/xlnx-zynqmp.c | 14 +- hw/block/xen-block.c | 15 +-- hw/core/qdev.c | 3 +-- hw/i386/x86.c| 3 +-- hw/ppc/pnv_psi.c | 4 +-- hw/s390x/s390-pci-bus.c | 3 +-- hw/s390x/s390-virtio-ccw.c | 3 +-- hw/scsi/scsi-bus.c | 3 +-- hw/sd/aspeed_sdhci.c | 8 +++--- hw/sd/ssi-sd.c | 3 +-- hw/virtio/virtio-rng.c | 5 ++-- qdev-monitor.c | 3 +-- qom/object.c | 15 --- qom/object_interfaces.c | 6 ++--- softmmu/vl.c | 4 +-- target/arm/monitor.c | 3 +-- target/i386/cpu.c| 11 +++- target/s390x/cpu_models.c| 3 +-- 28 files changed, 96 insertions(+), 142 deletions(-) diff --git a/hw/arm/armsse.c b/hw/arm/armsse.c index f8822f9cbd..e8d489a7e7 100644 --- a/hw/arm/armsse.c +++ b/hw/arm/armsse.c @@ -535,22 +535,20 @@ static void armsse_realize(DeviceState *dev, Error **errp) * later if necessary. */ if (extract32(info->cpuwait_rst, i, 1)) { -object_property_set_bool(cpuobj, "start-powered-off", true, &err); -if (err) { +if (!object_property_set_bool(cpuobj, "start-powered-off", true, + &err)) { error_propagate(errp, err); return; } } if (!s->cpu_fpu[i]) { -object_property_set_bool(cpuobj, "vfp", false, &err); -if (err) { +if (!object_property_set_bool(cpuobj, "vfp", false, &err)) { error_propagate(errp, err); return; } } if (!s->cpu_dsp[i]) { -object_property_set_bool(cpuobj, "dsp", false, &err); -if (err) { +if (!object_property_set_bool(cpuobj, "dsp", false, &err)) { error_propagate(errp, err); return; } @@ -605,9 +603,8 @@ static void armsse_realize(DeviceState *dev, Error **errp) DeviceState *devs = DEVICE(splitter); int cpunum; -object_property_set_int(splitter, "num-lines", info->num_cpus, -&err); -if (err) { +if (!object_property_set_int(splitter, "num-lines", + info->num_cpus, &err)) { error_propagate(errp, err); return; } @@ -659,9 +656,8 @@ static void armsse_realize(DeviceState *dev, Error **errp) * multiple lines, one for each of the PPCs within the ARMSSE and one * that will be an output from the ARMSSE to the system. */ -object_property_set_int(OBJECT(&s->sec_resp_splitter), "num-lines", 3, -&err); -if (err) { +if (!object_property_set_int(OBJECT(&s->sec_resp_splitter), + "num-lines", 3, &err)) { error_propagate(errp, err); return; } @@ -703,9 +699,9 @@ static void armsse_realize(DeviceState *dev, Error **errp) } /* We must OR together lines from the MPC splitters to go to the NVIC */ -object_property_set_int(OBJECT(&s->mpc_irq_orgate), "num-lines", -IOTS_NUM_EXP_MPC + info->sram_bank
[PATCH v2 03/44] qdev: Use returned bool to check for qdev_realize() etc. failure
Convert foo(..., &err); if (err) { ... } to if (!foo(..., &err)) { ... } for qdev_realize(), qdev_realize_and_unref(), qbus_realize() and their wrappers isa_realize_and_unref(), pci_realize_and_unref(), sysbus_realize(), sysbus_realize_and_unref(), usb_realize_and_unref(). Coccinelle script: @@ identifier fun = {isa_realize_and_unref, pci_realize_and_unref, qbus_realize, qdev_realize, qdev_realize_and_unref, sysbus_realize, sysbus_realize_and_unref, usb_realize_and_unref}; expression list args, args2; typedef Error; Error *err; @@ -fun(args, &err, args2); -if (err) +if (!fun(args, &err, args2)) { ... } Chokes on hw/arm/musicpal.c's lcd_refresh() with the unhelpful error message "no position information". Nothing to convert there; skipped. Fails to convert hw/arm/armsse.c, because Coccinelle gets confused by ARMSSE being used both as typedef and function-like macro there. Converted manually. A few line breaks tidied up manually. Signed-off-by: Markus Armbruster --- hw/arm/allwinner-a10.c | 15 +++ hw/arm/armsse.c | 78 +++-- hw/arm/armv7m.c | 9 ++-- hw/arm/aspeed_ast2600.c | 51 +++-- hw/arm/aspeed_soc.c | 45 +++ hw/arm/bcm2835_peripherals.c| 45 +++ hw/arm/bcm2836.c| 9 ++-- hw/arm/cubieboard.c | 3 +- hw/arm/digic.c | 9 ++-- hw/arm/digic_boards.c | 3 +- hw/arm/fsl-imx25.c | 33 +- hw/arm/fsl-imx31.c | 24 -- hw/arm/fsl-imx6.c | 36 +-- hw/arm/msf2-soc.c | 15 +++ hw/arm/nrf51_soc.c | 18 +++- hw/arm/stm32f205_soc.c | 21 +++-- hw/arm/stm32f405_soc.c | 24 -- hw/arm/xlnx-zynqmp.c| 45 +++ hw/block/fdc.c | 3 +- hw/block/xen-block.c| 3 +- hw/char/serial-pci-multi.c | 3 +- hw/char/serial-pci.c| 3 +- hw/char/serial.c| 6 +-- hw/core/cpu.c | 3 +- hw/cpu/a15mpcore.c | 3 +- hw/cpu/a9mpcore.c | 15 +++ hw/cpu/arm11mpcore.c| 12 ++--- hw/cpu/realview_mpcore.c| 6 +-- hw/display/virtio-gpu-pci.c | 4 +- hw/display/virtio-vga.c | 3 +- hw/intc/armv7m_nvic.c | 6 +-- hw/intc/pnv_xive.c | 6 +-- hw/intc/realview_gic.c | 3 +- hw/intc/spapr_xive.c| 6 +-- hw/intc/xics.c | 3 +- hw/intc/xive.c | 3 +- hw/isa/piix4.c | 3 +- hw/microblaze/xlnx-zynqmp-pmu.c | 6 +-- hw/mips/cps.c | 12 ++--- hw/misc/macio/cuda.c| 3 +- hw/misc/macio/macio.c | 18 +++- hw/misc/macio/pmu.c | 3 +- hw/pci-host/pnv_phb3.c | 9 ++-- hw/pci-host/pnv_phb4.c | 3 +- hw/pci-host/pnv_phb4_pec.c | 3 +- hw/ppc/e500.c | 3 +- hw/ppc/pnv.c| 39 ++--- hw/ppc/pnv_core.c | 3 +- hw/ppc/pnv_psi.c| 6 +-- hw/ppc/spapr_cpu_core.c | 3 +- hw/ppc/spapr_irq.c | 3 +- hw/riscv/opentitan.c| 6 +-- hw/riscv/sifive_e.c | 3 +- hw/riscv/sifive_u.c | 3 +- hw/s390x/event-facility.c | 10 ++--- hw/s390x/s390-pci-bus.c | 3 +- hw/s390x/sclp.c | 3 +- hw/s390x/virtio-ccw-crypto.c| 3 +- hw/s390x/virtio-ccw-rng.c | 3 +- hw/scsi/scsi-bus.c | 3 +- hw/sd/aspeed_sdhci.c| 3 +- hw/sd/ssi-sd.c | 3 +- hw/usb/bus.c| 3 +- hw/virtio/virtio-rng-pci.c | 3 +- qdev-monitor.c | 3 +- 65 files changed, 248 insertions(+), 495 deletions(-) diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c index 52e0d83760..e1acffe5f6 100644 --- a/hw/arm/allwinner-a10.c +++ b/hw/arm/allwinner-a10.c @@ -74,14 +74,12 @@ static void aw_a10_realize(DeviceState *dev, Error **errp) SysBusDevice *sysbusdev; Error *err = NULL; -qdev_realize(DEVICE(&s->cpu), NULL, &err); -if (err != NULL) { +if (!qdev_realize(DEVICE(&s->cpu), NULL, &err)) { error_propagate(errp, err); return; } -sysbus_realize(SYS_BUS_DEVICE(&s->intc), &err); -if (err != NULL) { +if (!sysbus_realize(SYS_BUS_DEVICE(&s->intc), &err)) { error_propagate(errp, err); return; } @@ -93,8 +91,7 @@ static void aw_a10_realize(DeviceState *dev, Error **errp) qdev_get_gpio_in(DEVICE(&s->cpu), ARM_CPU_FIQ)); qdev_pass_gpios(DEVICE(&s->intc), dev, NULL); -sysbus_realize(SYS_BUS_DEVICE(&s->timer), &err); -if (err != NULL
[PATCH v2 13/44] qemu-option: Use returned bool to check for failure
The previous commit enables conversion of foo(..., &err); if (err) { ... } to if (!foo(..., &err)) { ... } for QemuOpts functions that now return true / false on success / error. Coccinelle script: @@ identifier fun = {opts_do_parse, parse_option_bool, parse_option_number, parse_option_size, qemu_opt_parse, qemu_opt_rename, qemu_opt_set, qemu_opt_set_bool, qemu_opt_set_number, qemu_opts_absorb_qdict, qemu_opts_do_parse, qemu_opts_from_qdict_entry, qemu_opts_set, qemu_opts_validate}; expression list args, args2; typedef Error; Error *err; @@ -fun(args, &err, args2); -if (err) +if (!fun(args, &err, args2)) { ... } A few line breaks tidied up manually. Signed-off-by: Markus Armbruster --- block.c | 16 ++-- block/blkdebug.c | 3 +-- block/blklogwrites.c | 3 +-- block/blkverify.c | 3 +-- block/crypto.c| 3 +-- block/curl.c | 3 +-- block/file-posix.c| 6 ++ block/file-win32.c| 6 ++ block/gluster.c | 15 +-- block/iscsi.c | 3 +-- block/nbd.c | 3 +-- block/parallels.c | 3 +-- block/qcow2.c | 3 +-- block/quorum.c| 3 +-- block/raw-format.c| 3 +-- block/replication.c | 3 +-- block/sheepdog.c | 3 +-- block/ssh.c | 3 +-- block/throttle.c | 3 +-- block/vpc.c | 3 +-- block/vvfat.c | 3 +-- block/vxhs.c | 6 ++ blockdev.c| 11 --- chardev/char.c| 6 ++ contrib/ivshmem-server/main.c | 4 ++-- hw/net/virtio-net.c | 5 ++--- hw/smbios/smbios.c| 24 qapi/string-input-visitor.c | 3 +-- qemu-img.c| 19 +++ tpm.c | 3 +-- util/qemu-config.c| 12 util/qemu-option.c| 16 ++-- 32 files changed, 71 insertions(+), 132 deletions(-) diff --git a/block.c b/block.c index 6dbcb7e083..8d478bdc51 100644 --- a/block.c +++ b/block.c @@ -1629,8 +1629,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file, assert(options != NULL && bs->options != options); opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort); -qemu_opts_absorb_qdict(opts, options, &local_err); -if (local_err) { +if (!qemu_opts_absorb_qdict(opts, options, &local_err)) { error_propagate(errp, local_err); ret = -EINVAL; goto fail_opts; @@ -4091,8 +4090,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, /* Process generic block layer options */ opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort); -qemu_opts_absorb_qdict(opts, reopen_state->options, &local_err); -if (local_err) { +if (!qemu_opts_absorb_qdict(opts, reopen_state->options, &local_err)) { error_propagate(errp, local_err); ret = -EINVAL; goto error; @@ -6078,8 +6076,7 @@ void bdrv_img_create(const char *filename, const char *fmt, /* Parse -o options */ if (options) { -qemu_opts_do_parse(opts, options, NULL, &local_err); -if (local_err) { +if (!qemu_opts_do_parse(opts, options, NULL, &local_err)) { goto out; } } @@ -6092,8 +6089,8 @@ void bdrv_img_create(const char *filename, const char *fmt, } if (base_filename) { -qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename, &local_err); -if (local_err) { +if (!qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename, + &local_err)) { error_setg(errp, "Backing file not supported for file format '%s'", fmt); goto out; @@ -6101,8 +6098,7 @@ void bdrv_img_create(const char *filename, const char *fmt, } if (base_fmt) { -qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt, &local_err); -if (local_err) { +if (!qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt, &local_err)) { error_setg(errp, "Backing file format not supported for file " "format '%s'", fmt); goto out; diff --git a/block/blkdebug.c b/block/blkdebug.c index 7194bc7f06..d473dcf8c7 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -472,8 +472,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags, uint64_t align; opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); -qemu_opts_absorb_qdict(opts, options, &local_err); -if (local_err) { +if (!qemu_opts_absorb_qdict(opts, options, &local_err
[PATCH v2 05/44] virtio-crypto-pci: Tidy up virtio_crypto_pci_realize()
virtio_crypto_pci_realize() continues after realization of its "virtio-crypto-device" fails. Only an object_property_set_link() follows; looks harmless to me. Tidy up anyway: return after failure, just like virtio_rng_pci_realize() does. Cc: "Gonglei (Arei)" Cc: Michael S. Tsirkin Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Gonglei < arei.gong...@huawei.com> --- hw/virtio/virtio-crypto-pci.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/hw/virtio/virtio-crypto-pci.c b/hw/virtio/virtio-crypto-pci.c index 72be531c95..0755722288 100644 --- a/hw/virtio/virtio-crypto-pci.c +++ b/hw/virtio/virtio-crypto-pci.c @@ -54,7 +54,9 @@ static void virtio_crypto_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) } virtio_pci_force_virtio_1(vpci_dev); -qdev_realize(vdev, BUS(&vpci_dev->bus), errp); +if (!qdev_realize(vdev, BUS(&vpci_dev->bus), errp)) { +return; +} object_property_set_link(OBJECT(vcrypto), OBJECT(vcrypto->vdev.conf.cryptodev), "cryptodev", NULL); -- 2.26.2
[PATCH v2 41/44] error: Avoid error_propagate() after migrate_add_blocker()
When migrate_add_blocker(blocker, &errp) is followed by error_propagate(errp, err), we can often just as well do migrate_add_blocker(..., errp). Do that with this Coccinelle script: @@ expression blocker, err, errp; expression ret; @@ -ret = migrate_add_blocker(blocker, &err); -if (err) { +ret = migrate_add_blocker(blocker, errp); +if (ret < 0) { ... when != err; -error_propagate(errp, err); ... } @@ expression blocker, err, errp; @@ -migrate_add_blocker(blocker, &err); -if (err) { +if (migrate_add_blocker(blocker, errp) < 0) { ... when != err; -error_propagate(errp, err); ... } Double-check @err is not used afterwards. Dereferencing it would be use after free, but checking whether it's null would be legitimate. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy --- block/parallels.c| 5 ++--- block/qcow.c | 6 ++ block/vdi.c | 6 ++ block/vhdx.c | 5 ++--- block/vmdk.c | 6 ++ block/vpc.c | 5 ++--- block/vvfat.c| 6 ++ hw/display/virtio-gpu-base.c | 5 + hw/intc/arm_gic_kvm.c| 4 +--- hw/intc/arm_gicv3_its_kvm.c | 5 + hw/intc/arm_gicv3_kvm.c | 4 +--- hw/misc/ivshmem.c| 4 +--- hw/scsi/vhost-scsi.c | 4 +--- 13 files changed, 20 insertions(+), 45 deletions(-) diff --git a/block/parallels.c b/block/parallels.c index f489c0d4ba..3c22dfdc9d 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -859,9 +859,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, error_setg(&s->migration_blocker, "The Parallels format used by node '%s' " "does not support live migration", bdrv_get_device_or_node_name(bs)); -ret = migrate_add_blocker(s->migration_blocker, &local_err); -if (local_err) { -error_propagate(errp, local_err); +ret = migrate_add_blocker(s->migration_blocker, errp); +if (ret < 0) { error_free(s->migration_blocker); goto fail; } diff --git a/block/qcow.c b/block/qcow.c index c22d1bf6b8..1e134f3445 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -121,7 +121,6 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, unsigned int len, i, shift; int ret; QCowHeader header; -Error *local_err = NULL; QCryptoBlockOpenOptions *crypto_opts = NULL; unsigned int cflags = 0; QDict *encryptopts = NULL; @@ -314,9 +313,8 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, error_setg(&s->migration_blocker, "The qcow format used by node '%s' " "does not support live migration", bdrv_get_device_or_node_name(bs)); -ret = migrate_add_blocker(s->migration_blocker, &local_err); -if (local_err) { -error_propagate(errp, local_err); +ret = migrate_add_blocker(s->migration_blocker, errp); +if (ret < 0) { error_free(s->migration_blocker); goto fail; } diff --git a/block/vdi.c b/block/vdi.c index 0a60be773e..3a3df45f84 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -375,7 +375,6 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, VdiHeader header; size_t bmap_size; int ret; -Error *local_err = NULL; QemuUUID uuid_link, uuid_parent; bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds, @@ -496,9 +495,8 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, error_setg(&s->migration_blocker, "The vdi format used by node '%s' " "does not support live migration", bdrv_get_device_or_node_name(bs)); -ret = migrate_add_blocker(s->migration_blocker, &local_err); -if (local_err) { -error_propagate(errp, local_err); +ret = migrate_add_blocker(s->migration_blocker, errp); +if (ret < 0) { error_free(s->migration_blocker); goto fail_free_bmap; } diff --git a/block/vhdx.c b/block/vhdx.c index eed9c3b860..1f9fca837d 100644 --- a/block/vhdx.c +++ b/block/vhdx.c @@ -1089,9 +1089,8 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags, error_setg(&s->migration_blocker, "The vhdx format used by node '%s' " "does not support live migration", bdrv_get_device_or_node_name(bs)); -ret = migrate_add_blocker(s->migration_blocker, &local_err); -if (local_err) { -error_propagate(errp, local_err); +ret = migrate_add_blocker(s->migration_blocker, errp); +if (ret < 0) { error_free(s->migration_blocker); goto fail; } diff --git a/block/vmdk.c b/block/vmdk.c index 9a09193f3b..28cec50f38 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -
[PATCH v2 25/44] qom: Use return values to check for error where that's simpler
When using the Error object to check for error, we need to receive it into a local variable, then propagate() it to @errp. Using the return value permits allows receiving it straight to @errp. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- qom/object.c | 16 +--- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/qom/object.c b/qom/object.c index 0808da2767..56d858b6a5 100644 --- a/qom/object.c +++ b/qom/object.c @@ -549,8 +549,7 @@ void object_initialize_child_with_propsv(Object *parentobj, object_initialize(childobj, size, type); obj = OBJECT(childobj); -object_set_propv(obj, &local_err, vargs); -if (local_err) { +if (object_set_propv(obj, errp, vargs) < 0) { goto out; } @@ -743,7 +742,7 @@ Object *object_new_with_propv(const char *typename, } obj = object_new_with_type(klass->type); -if (object_set_propv(obj, &local_err, vargs) < 0) { +if (object_set_propv(obj, errp, vargs) < 0) { goto error; } @@ -1767,14 +1766,17 @@ static void object_set_link_property(Object *obj, Visitor *v, char *path = NULL; visit_type_str(v, name, &path, &local_err); +if (local_err) { +error_propagate(errp, local_err); +return; +} -if (!local_err && strcmp(path, "") != 0) { -new_target = object_resolve_link(obj, name, path, &local_err); +if (strcmp(path, "") != 0) { +new_target = object_resolve_link(obj, name, path, errp); } g_free(path); -if (local_err) { -error_propagate(errp, local_err); +if (!new_target) { return; } -- 2.26.2
[PATCH v2 14/44] block: Avoid error accumulation in bdrv_img_create()
When creating an image fails because the format doesn't support option "backing_file" or "backing_fmt", bdrv_img_create() first has qemu_opt_set() put a generic error into @local_err, then puts the real error into @errp with error_setg(), and then propagates the former to the latter, which throws away the generic error. A bit complicated, but works. Not that qemu_opt_set() returns a useful value, we can simply ignore the generic error instead. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- block.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/block.c b/block.c index 8d478bdc51..b396f32a51 100644 --- a/block.c +++ b/block.c @@ -6090,7 +6090,7 @@ void bdrv_img_create(const char *filename, const char *fmt, if (base_filename) { if (!qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename, - &local_err)) { + NULL)) { error_setg(errp, "Backing file not supported for file format '%s'", fmt); goto out; @@ -6098,7 +6098,7 @@ void bdrv_img_create(const char *filename, const char *fmt, } if (base_fmt) { -if (!qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt, &local_err)) { +if (!qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt, NULL)) { error_setg(errp, "Backing file format not supported for file " "format '%s'", fmt); goto out; -- 2.26.2
[PATCH v2 29/44] qom: Use returned bool to check for failure, manual part
The previous commit used Coccinelle to convert from checking the Error object to checking the return value. Convert a few more manually. Signed-off-by: Markus Armbruster --- hw/core/bus.c | 6 +- hw/core/qdev.c | 7 +-- hw/s390x/s390-virtio-ccw.c | 13 +++-- 3 files changed, 9 insertions(+), 17 deletions(-) diff --git a/hw/core/bus.c b/hw/core/bus.c index 00d1d31762..6b987b6946 100644 --- a/hw/core/bus.c +++ b/hw/core/bus.c @@ -166,11 +166,7 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam bool qbus_realize(BusState *bus, Error **errp) { -Error *err = NULL; - -object_property_set_bool(OBJECT(bus), "realized", true, &err); -error_propagate(errp, err); -return !err; +return object_property_set_bool(OBJECT(bus), "realized", true, errp); } void qbus_unrealize(BusState *bus) diff --git a/hw/core/qdev.c b/hw/core/qdev.c index d6416fb894..17bd8fc2ec 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -386,8 +386,6 @@ void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev, */ bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp) { -Error *err = NULL; - assert(!dev->realized && !dev->parent_bus); if (bus) { @@ -396,10 +394,7 @@ bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp) assert(!DEVICE_GET_CLASS(dev)->bus_type); } -if (!object_property_set_bool(OBJECT(dev), "realized", true, &err)) { -error_propagate(errp, err); -} -return !err; +return object_property_set_bool(OBJECT(dev), "realized", true, errp); } /* diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 62af349c31..f7a68343ef 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -70,19 +70,20 @@ static S390CPU *s390x_new_cpu(const char *typename, uint32_t core_id, { S390CPU *cpu = S390_CPU(object_new(typename)); Error *err = NULL; +S390CPU *ret = NULL; if (!object_property_set_int(OBJECT(cpu), "core-id", core_id, &err)) { goto out; } -qdev_realize(DEVICE(cpu), NULL, &err); +if (!qdev_realize(DEVICE(cpu), NULL, &err)) { +goto out; +} +ret = cpu; out: object_unref(OBJECT(cpu)); -if (err) { -error_propagate(errp, err); -cpu = NULL; -} -return cpu; +error_propagate(errp, err); +return ret; } static void s390_init_cpus(MachineState *machine) -- 2.26.2
[PATCH v2 17/44] qapi: Use returned bool to check for failure, Coccinelle part
The previous commit enables conversion of visit_foo(..., &err); if (err) { ... } to if (!visit_foo(..., errp)) { ... } for visitor functions that now return true / false on success / error. Coccinelle script: @@ identifier fun =~ "check_list|input_type_enum|lv_start_struct|lv_type_bool|lv_type_int64|lv_type_str|lv_type_uint64|output_type_enum|parse_type_bool|parse_type_int64|parse_type_null|parse_type_number|parse_type_size|parse_type_str|parse_type_uint64|print_type_bool|print_type_int64|print_type_null|print_type_number|print_type_size|print_type_str|print_type_uint64|qapi_clone_start_alternate|qapi_clone_start_list|qapi_clone_start_struct|qapi_clone_type_bool|qapi_clone_type_int64|qapi_clone_type_null|qapi_clone_type_number|qapi_clone_type_str|qapi_clone_type_uint64|qapi_dealloc_start_list|qapi_dealloc_start_struct|qapi_dealloc_type_anything|qapi_dealloc_type_bool|qapi_dealloc_type_int64|qapi_dealloc_type_null|qapi_dealloc_type_number|qapi_dealloc_type_str|qapi_dealloc_type_uint64|qobject_input_check_list|qobject_input_check_struct|qobject_input_start_alternate|qobject_input_start_list|qobject_input_start_struct|qobject_input_type_any|qobject_input_type_bool|qobject_input_type_bool_keyval|qobject_input_type_int64|qobject_input_type_int64_keyval|qobject_input_type_null|qobject_input_type_number|qobject_input_type_number_keyval|qobject_input_type_size_keyval|qobject_input_type_str|qobject_input_type_str_keyval|qobject_input_type_uint64|qobject_input_type_uint64_keyval|qobject_output_start_list|qobject_output_start_struct|qobject_output_type_any|qobject_output_type_bool|qobject_output_type_int64|qobject_output_type_null|qobject_output_type_number|qobject_output_type_str|qobject_output_type_uint64|start_list|visit_check_list|visit_check_struct|visit_start_alternate|visit_start_list|visit_start_struct|visit_type_.*"; expression list args; typedef Error; Error *err; @@ -fun(args, &err); -if (err) +if (!fun(args, &err)) { ... } A few line breaks tidied up manually. Signed-off-by: Markus Armbruster --- accel/kvm/kvm-all.c | 3 +-- accel/tcg/tcg-all.c | 3 +-- backends/cryptodev.c | 3 +-- backends/hostmem-file.c | 3 +-- backends/hostmem-memfd.c | 3 +-- backends/hostmem.c | 6 ++--- backends/tpm/tpm_util.c | 3 +-- block/blkdebug.c | 3 +-- block/nbd.c | 3 +-- block/sheepdog.c | 3 +-- block/throttle-groups.c | 6 ++--- bootdevice.c | 3 +-- hw/block/xen-block.c | 3 +-- hw/core/machine.c| 3 +-- hw/core/qdev-properties-system.c | 12 +++-- hw/core/qdev-properties.c| 38 +++-- hw/cpu/core.c| 6 ++--- hw/gpio/aspeed_gpio.c| 3 +-- hw/i386/pc.c | 3 +-- hw/ide/qdev.c| 3 +-- hw/intc/apic_common.c| 3 +-- hw/mem/nvdimm.c | 6 ++--- hw/misc/aspeed_sdmc.c| 3 +-- hw/misc/pca9552.c| 3 +-- hw/misc/tmp105.c | 3 +-- hw/misc/tmp421.c | 3 +-- hw/net/ne2000-isa.c | 3 +-- hw/ppc/spapr_caps.c | 9 +++ hw/ppc/spapr_drc.c | 10 +++- hw/s390x/css.c | 3 +-- hw/usb/dev-storage.c | 3 +-- hw/vfio/pci-quirks.c | 3 +-- hw/virtio/virtio-balloon.c | 15 iothread.c | 3 +-- monitor/hmp-cmds.c | 3 +-- net/colo-compare.c | 6 ++--- net/dump.c | 3 +-- net/filter-buffer.c | 3 +-- qom/object.c | 42 +++- qom/object_interfaces.c | 3 +-- target/arm/cpu64.c | 9 +++ target/arm/monitor.c | 3 +-- target/i386/cpu.c| 15 target/ppc/compat.c | 3 +-- target/s390x/cpu_models.c| 9 +++ target/sparc/cpu.c | 3 +-- 46 files changed, 97 insertions(+), 188 deletions(-) diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index d54a8701d8..397669231d 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -3116,8 +3116,7 @@ static void kvm_set_kvm_shadow_mem(Object *obj, Visitor *v, Error *error = NULL; int64_t value; -visit_type_int(v, name, &value, &error); -if (error) { +if (!visit_type_int(v, name, &value, &error)) { error_propagate(errp, error); return; } diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c index 3b4fda5640..d6b3d7fc07 100644 --- a/accel/tcg/tcg-all.c +++ b/accel/tcg/tcg-all.c @@ -185,8 +185,7 @@ static void tcg_set_tb_size(Object *obj, Visitor *v, Error *error = NULL; ui
[PATCH v2 23/44] qom: Crash more nicely on object_property_get_link() failure
Pass &error_abort instead of NULL where the returned value is dereferenced or asserted to be non-null. Drop a now redundant assertion. Signed-off-by: Markus Armbruster --- hw/core/platform-bus.c | 6 +++--- hw/ppc/spapr_drc.c | 3 ++- hw/ppc/spapr_hcall.c | 3 ++- hw/ppc/spapr_pci_nvlink2.c | 3 ++- ui/vnc.c | 2 +- 5 files changed, 10 insertions(+), 7 deletions(-) diff --git a/hw/core/platform-bus.c b/hw/core/platform-bus.c index d494e5cec1..5037ca265e 100644 --- a/hw/core/platform-bus.c +++ b/hw/core/platform-bus.c @@ -22,6 +22,7 @@ #include "qemu/osdep.h" #include "hw/platform-bus.h" #include "hw/qdev-properties.h" +#include "qapi/error.h" #include "qemu/error-report.h" #include "qemu/module.h" @@ -63,9 +64,8 @@ hwaddr platform_bus_get_mmio_addr(PlatformBusDevice *pbus, SysBusDevice *sbdev, return -1; } -parent_mr = object_property_get_link(OBJECT(sbdev_mr), "container", NULL); - -assert(parent_mr); +parent_mr = object_property_get_link(OBJECT(sbdev_mr), "container", + &error_abort); if (parent_mr != pbus_mr_obj) { /* MMIO region is not mapped on platform bus */ return -1; diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index d10193f39e..1f18b79348 100644 --- a/hw/ppc/spapr_drc.c +++ b/hw/ppc/spapr_drc.c @@ -870,7 +870,8 @@ int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t drc_type_mask) continue; } -obj = object_property_get_link(root_container, prop->name, NULL); +obj = object_property_get_link(root_container, prop->name, + &error_abort); drc = SPAPR_DR_CONNECTOR(obj); drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c index 0f54988f2e..c1d01228c6 100644 --- a/hw/ppc/spapr_hcall.c +++ b/hw/ppc/spapr_hcall.c @@ -1655,7 +1655,8 @@ static void spapr_handle_transient_dev_before_cas(SpaprMachineState *spapr) continue; } drc = SPAPR_DR_CONNECTOR(object_property_get_link(drc_container, - prop->name, NULL)); + prop->name, + &error_abort)); if (spapr_drc_transient(drc)) { spapr_drc_reset(drc); diff --git a/hw/ppc/spapr_pci_nvlink2.c b/hw/ppc/spapr_pci_nvlink2.c index 8332d5694e..dd8cd6db96 100644 --- a/hw/ppc/spapr_pci_nvlink2.c +++ b/hw/ppc/spapr_pci_nvlink2.c @@ -358,7 +358,8 @@ void spapr_phb_nvgpu_ram_populate_dt(SpaprPhbState *sphb, void *fdt) for (i = 0; i < sphb->nvgpus->num; ++i) { SpaprPhbPciNvGpuSlot *nvslot = &sphb->nvgpus->slots[i]; Object *nv_mrobj = object_property_get_link(OBJECT(nvslot->gpdev), -"nvlink2-mr[0]", NULL); +"nvlink2-mr[0]", +&error_abort); uint32_t associativity[] = { cpu_to_be32(0x4), SPAPR_GPU_NUMA_ID, diff --git a/ui/vnc.c b/ui/vnc.c index 527ad25124..f006aa1afd 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -568,7 +568,7 @@ VncInfo2List *qmp_query_vnc_servers(Error **errp) &info->vencrypt, &info->has_vencrypt); if (vd->dcl.con) { dev = DEVICE(object_property_get_link(OBJECT(vd->dcl.con), - "device", NULL)); + "device", &error_abort)); info->has_display = true; info->display = g_strdup(dev->id); } -- 2.26.2
[PATCH v2 36/44] error: Eliminate error_propagate() manually
When all we do with an Error we receive into a local variable is propagating to somewhere else, we can just as well receive it there right away. The previous two commits did that for sufficiently simple cases with Coccinelle. Do it for several more manually. Signed-off-by: Markus Armbruster --- block/replication.c| 4 +--- blockdev.c | 16 -- bootdevice.c | 6 ++ dump/dump.c| 7 ++ hw/block/fdc.c | 8 +++ hw/core/numa.c | 44 -- hw/i386/x86.c | 6 ++ hw/intc/xive.c | 12 +++ hw/ppc/spapr_cpu_core.c| 14 hw/s390x/s390-pci-bus.c| 4 +--- hw/s390x/s390-virtio-ccw.c | 6 ++ hw/s390x/sclp.c| 12 --- hw/usb/bus.c | 4 +--- qdev-monitor.c | 11 -- qga/commands-posix.c | 4 +--- qom/object.c | 9 ++-- qom/qom-qobject.c | 5 + target/i386/cpu.c | 19 +--- 18 files changed, 56 insertions(+), 135 deletions(-) diff --git a/block/replication.c b/block/replication.c index b844a09eb1..dcd430624e 100644 --- a/block/replication.c +++ b/block/replication.c @@ -367,7 +367,6 @@ static void reopen_backing_file(BlockDriverState *bs, bool writable, { BDRVReplicationState *s = bs->opaque; BlockReopenQueue *reopen_queue = NULL; -Error *local_err = NULL; if (writable) { s->orig_hidden_read_only = bdrv_is_read_only(s->hidden_disk->bs); @@ -392,8 +391,7 @@ static void reopen_backing_file(BlockDriverState *bs, bool writable, } if (reopen_queue) { -bdrv_reopen_multiple(reopen_queue, &local_err); -error_propagate(errp, local_err); +bdrv_reopen_multiple(reopen_queue, errp); } bdrv_subtree_drained_end(s->hidden_disk->bs); diff --git a/blockdev.c b/blockdev.c index 1f254e7110..59b0b8ffaf 100644 --- a/blockdev.c +++ b/blockdev.c @@ -793,7 +793,6 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type, bool read_only = false; bool copy_on_read; const char *filename; -Error *local_err = NULL; int i; /* Change legacy command line options into QMP ones */ @@ -1003,13 +1002,10 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type, } /* Actual block device init: Functionality shared with blockdev-add */ -blk = blockdev_init(filename, bs_opts, &local_err); +blk = blockdev_init(filename, bs_opts, errp); bs_opts = NULL; if (!blk) { -error_propagate(errp, local_err); goto fail; -} else { -assert(!local_err); } /* Create legacy DriveInfo */ @@ -3141,9 +3137,8 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp) arg->has_copy_mode, arg->copy_mode, arg->has_auto_finalize, arg->auto_finalize, arg->has_auto_dismiss, arg->auto_dismiss, - &local_err); + errp); bdrv_unref(target_bs); -error_propagate(errp, local_err); out: aio_context_release(aio_context); } @@ -3171,7 +3166,6 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id, AioContext *aio_context; AioContext *old_context; BlockMirrorBackingMode backing_mode = MIRROR_LEAVE_BACKING_CHAIN; -Error *local_err = NULL; bool zero_target; int ret; @@ -3213,8 +3207,7 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id, has_copy_mode, copy_mode, has_auto_finalize, auto_finalize, has_auto_dismiss, auto_dismiss, - &local_err); -error_propagate(errp, local_err); + errp); out: aio_context_release(aio_context); } @@ -3433,8 +3426,7 @@ void qmp_change_backing_file(const char *device, } if (ro) { -bdrv_reopen_set_read_only(image_bs, true, &local_err); -error_propagate(errp, local_err); +bdrv_reopen_set_read_only(image_bs, true, errp); } out: diff --git a/bootdevice.c b/bootdevice.c index 8185402a5a..add4e3d2d1 100644 --- a/bootdevice.c +++ b/bootdevice.c @@ -303,15 +303,13 @@ static void device_set_bootindex(Object *obj, Visitor *v, const char *name, /* check whether bootindex is present in fw_boot_order list */ check_boot_index(boot_index, &local_err); if (local_err) { -goto out; +error_propagate(errp, local_err); +return; } /* change bootindex to a new one */ *prop->bootindex = boot_index; add_boot_device_path(*prop->bootindex, prop->dev, prop->suffix); - -out: -error_propagate(errp, local_err); } static void property_release_bootindex(Object *obj, const char *name, diff --git a/dump/dump.c b/du
[PATCH v2 43/44] qdev: Ignore Error objects where the return value suffices
Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- hw/core/qdev-properties.c | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c index e1ad147339..8eb4283a56 100644 --- a/hw/core/qdev-properties.c +++ b/hw/core/qdev-properties.c @@ -653,7 +653,6 @@ static void set_pci_devfn(Object *obj, Visitor *v, const char *name, Property *prop = opaque; int32_t value, *ptr = qdev_get_prop_ptr(dev, prop); unsigned int slot, fn, n; -Error *local_err = NULL; char *str; if (dev->realized) { @@ -661,9 +660,7 @@ static void set_pci_devfn(Object *obj, Visitor *v, const char *name, return; } -if (!visit_type_str(v, name, &str, &local_err)) { -error_free(local_err); -local_err = NULL; +if (!visit_type_str(v, name, &str, NULL)) { if (!visit_type_int32(v, name, &value, errp)) { return; } -- 2.26.2
[PATCH v2 20/44] s390x/pci: Fix harmless mistake in zpci's property fid's setter
s390_pci_set_fid() sets zpci->fid_defined to true even when visit_type_uint32() failed. Reproducer: "-device zpci,fid=junk". Harmless in practice, because qdev_device_add() then fails, throwing away @zpci. Fix it anyway. Cc: Matthew Rosato Cc: Cornelia Huck Signed-off-by: Markus Armbruster Reviewed-by: Matthew Rosato Reviewed-by: Cornelia Huck --- hw/s390x/s390-pci-bus.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c index 0517901024..07e1e4d7a3 100644 --- a/hw/s390x/s390-pci-bus.c +++ b/hw/s390x/s390-pci-bus.c @@ -1267,7 +1267,9 @@ static void s390_pci_set_fid(Object *obj, Visitor *v, const char *name, return; } -visit_type_uint32(v, name, ptr, errp); +if (!visit_type_uint32(v, name, ptr, errp)) { +return; +} zpci->fid_defined = true; } -- 2.26.2
[PATCH v2 12/44] qemu-option: Make functions taking Error ** return bool, not void
See recent commit "error: Document Error API usage rules" for rationale. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy --- include/qemu/option.h | 16 blockdev.c| 5 ++- util/qemu-option.c| 92 +-- 3 files changed, 64 insertions(+), 49 deletions(-) diff --git a/include/qemu/option.h b/include/qemu/option.h index eb4097889d..2d77b10f90 100644 --- a/include/qemu/option.h +++ b/include/qemu/option.h @@ -30,7 +30,7 @@ const char *get_opt_value(const char *p, char **value); -void parse_option_size(const char *name, const char *value, +bool parse_option_size(const char *name, const char *value, uint64_t *ret, Error **errp); bool has_help_option(const char *param); @@ -80,11 +80,11 @@ uint64_t qemu_opt_get_number_del(QemuOpts *opts, const char *name, uint64_t qemu_opt_get_size_del(QemuOpts *opts, const char *name, uint64_t defval); int qemu_opt_unset(QemuOpts *opts, const char *name); -void qemu_opt_set(QemuOpts *opts, const char *name, const char *value, +bool qemu_opt_set(QemuOpts *opts, const char *name, const char *value, Error **errp); -void qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val, +bool qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val, Error **errp); -void qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val, +bool qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val, Error **errp); typedef int (*qemu_opt_loopfunc)(void *opaque, const char *name, const char *value, @@ -106,13 +106,13 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id, int fail_if_exists, Error **errp); void qemu_opts_reset(QemuOptsList *list); void qemu_opts_loc_restore(QemuOpts *opts); -void qemu_opts_set(QemuOptsList *list, const char *id, +bool qemu_opts_set(QemuOptsList *list, const char *id, const char *name, const char *value, Error **errp); const char *qemu_opts_id(QemuOpts *opts); void qemu_opts_set_id(QemuOpts *opts, char *id); void qemu_opts_del(QemuOpts *opts); -void qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc, Error **errp); -void qemu_opts_do_parse(QemuOpts *opts, const char *params, +bool qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc, Error **errp); +bool qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname, Error **errp); QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params, bool permit_abbrev); @@ -125,7 +125,7 @@ QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict, QDict *qemu_opts_to_qdict_filtered(QemuOpts *opts, QDict *qdict, QemuOptsList *list, bool del); QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict); -void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp); +bool qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp); typedef int (*qemu_opts_loopfunc)(void *opaque, QemuOpts *opts, Error **errp); int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, diff --git a/blockdev.c b/blockdev.c index b52ed9de86..39e12a62b3 100644 --- a/blockdev.c +++ b/blockdev.c @@ -705,7 +705,7 @@ BlockDriverState *bdrv_next_monitor_owned(BlockDriverState *bs) : QTAILQ_FIRST(&monitor_bdrv_states); } -static void qemu_opt_rename(QemuOpts *opts, const char *from, const char *to, +static bool qemu_opt_rename(QemuOpts *opts, const char *from, const char *to, Error **errp) { const char *value; @@ -715,7 +715,7 @@ static void qemu_opt_rename(QemuOpts *opts, const char *from, const char *to, if (qemu_opt_find(opts, to)) { error_setg(errp, "'%s' and its alias '%s' can't be used at the " "same time", to, from); -return; +return false; } } @@ -724,6 +724,7 @@ static void qemu_opt_rename(QemuOpts *opts, const char *from, const char *to, qemu_opt_set(opts, to, value, &error_abort); qemu_opt_unset(opts, from); } +return true; } QemuOptsList qemu_legacy_drive_opts = { diff --git a/util/qemu-option.c b/util/qemu-option.c index d8233b3b35..2f4fb62120 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -96,7 +96,7 @@ const char *get_opt_value(const char *p, char **value) return offset; } -static void parse_option_bool(const char *name, const char *value, bool *ret, +static bool parse_option_bool(const char *name, const char *value, bool *ret, Error **errp) { if (!strcmp(value, "on")) { @@ -106,10 +106,12 @@ static void parse_option_bool(const char *name, const char *value, bool *ret
[PATCH v2 31/44] qdev: Make functions taking Error ** return bool, not void
See recent commit "error: Document Error API usage rules" for rationale. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- include/hw/qdev-properties.h | 4 ++-- hw/core/qdev-properties-system.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h index 49c6cd2460..f12ab9e6bc 100644 --- a/include/hw/qdev-properties.h +++ b/include/hw/qdev-properties.h @@ -236,8 +236,8 @@ extern const PropertyInfo qdev_prop_pcie_link_width; /* * Set properties between creation and realization. */ -void qdev_prop_set_drive_err(DeviceState *dev, const char *name, - BlockBackend *value, Error **errp); +bool qdev_prop_set_drive_err(DeviceState *dev, const char *name, + BlockBackend *value, Error **errp); /* * Set properties between creation and realization. diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c index 810831b1df..7d2387f22c 100644 --- a/hw/core/qdev-properties-system.c +++ b/hw/core/qdev-properties-system.c @@ -421,7 +421,7 @@ const PropertyInfo qdev_prop_audiodev = { .set = set_audiodev, }; -void qdev_prop_set_drive_err(DeviceState *dev, const char *name, +bool qdev_prop_set_drive_err(DeviceState *dev, const char *name, BlockBackend *value, Error **errp) { const char *ref = ""; @@ -436,7 +436,7 @@ void qdev_prop_set_drive_err(DeviceState *dev, const char *name, } } -object_property_set_str(OBJECT(dev), name, ref, errp); +return object_property_set_str(OBJECT(dev), name, ref, errp); } void qdev_prop_set_drive(DeviceState *dev, const char *name, -- 2.26.2
[PATCH v2 32/44] qdev: Use returned bool to check for failure, Coccinelle part
The previous commit enables conversion of qdev_prop_set_drive_err(..., &err); if (err) { ... } to if (!qdev_prop_set_drive_err(..., errp)) { ... } Coccinelle script: @@ identifier fun = qdev_prop_set_drive_err; expression list args; typedef Error; Error *err; @@ -fun(args, &err); -if (err) +if (!fun(args, &err)) { ... } One line break tidied up manually. Signed-off-by: Markus Armbruster --- hw/scsi/scsi-bus.c | 3 +-- hw/sd/sd.c | 3 +-- hw/sd/ssi-sd.c | 5 ++--- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c index a83939f7d0..38b66a2f45 100644 --- a/hw/scsi/scsi-bus.c +++ b/hw/scsi/scsi-bus.c @@ -277,8 +277,7 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockBackend *blk, if (serial && object_property_find(OBJECT(dev), "serial", NULL)) { qdev_prop_set_string(dev, "serial", serial); } -qdev_prop_set_drive_err(dev, "drive", blk, &err); -if (err) { +if (!qdev_prop_set_drive_err(dev, "drive", blk, &err)) { error_propagate(errp, err); object_unparent(OBJECT(dev)); return NULL; diff --git a/hw/sd/sd.c b/hw/sd/sd.c index 97a9d32964..5137168d66 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -706,8 +706,7 @@ SDState *sd_init(BlockBackend *blk, bool is_spi) obj = object_new(TYPE_SD_CARD); dev = DEVICE(obj); -qdev_prop_set_drive_err(dev, "drive", blk, &err); -if (err) { +if (!qdev_prop_set_drive_err(dev, "drive", blk, &err)) { error_reportf_err(err, "sd_init failed: "); return NULL; } diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c index 4d91f603fa..e0fb9f3093 100644 --- a/hw/sd/ssi-sd.c +++ b/hw/sd/ssi-sd.c @@ -254,9 +254,8 @@ static void ssi_sd_realize(SSISlave *d, Error **errp) dinfo = drive_get_next(IF_SD); carddev = qdev_new(TYPE_SD_CARD); if (dinfo) { -qdev_prop_set_drive_err(carddev, "drive", blk_by_legacy_dinfo(dinfo), -&err); -if (err) { +if (!qdev_prop_set_drive_err(carddev, "drive", + blk_by_legacy_dinfo(dinfo), &err)) { goto fail; } } -- 2.26.2
[PATCH v2 00/44] Less clumsy error checking
When the Error API was created, we adopted the (unwritten) rule to return void when the function returns no useful value on success, unlike GError, which recommends to return true on success and false on error then. When a function returns a distinct error value, say false, a checked call that passes the error up looks like if (!frobnicate(..., errp)) { handle the error... } When it returns void, we need Error *err = NULL; frobnicate(..., &err); if (err) { handle the error... error_propagate(errp, err); } Not only is this more verbose, it also creates an Error object even when @errp is null, &error_abort or &error_fatal. People got tired of the additional boilerplate, and started to ignore the unwritten rule. The result is confusion among developers about the preferred usage. This series adopts the GError rule (in writing), and updates a substantial amount of code to honor the rule. Cuts the number of error_propagate() calls nearly by half. The diffstat speaks for itself. Based on my "[PULL 00/28] Error reporting patches patches for 2020-07-02". Also available from my public repository https://repo.or.cz/qemu/armbru.git on branch error-smooth. Based-on: Message-Id: <20200702110931.2953148-1-arm...@redhat.com> v2: * Rebased * Coccinelle scripts reworked, patches sliced and diced for reviewability: Old PATCH 03+17+23-24+35+38-39+42 split into "Use returned bool to check for failure" parts [PATCH 03+13+17-18+25+28-29+42], and "Eliminate error_propagate()" parts. The latter combined with old PATCH 06-07+18-19+29+43 are now [PATCH 33-37]. The end result is almost identical. Some R-bys dropped; sorry! * Drop variables as they become unused [Vladimir] * PATCH 01: Comment typos fixed [Greg] * PATCH 09: Simplify further by cutting out variables [Eric] * PATCH 11: opt_set() renamed to opt_validate(), redundant parameter dropped [Vladimir], R-by kept * PATCH 16: Comment typos fixed, indentation tidied up [Eric] * PATCH 23: Assertion dropped [Eric], incorrect hunk backed out * PATCH 26: Regenerated after rebase; note additional Coccinelle hiccup in the commit message * PATCH 27: Indentation tiedied up [Eric] * Left for later: followup to fix nearby typos and such [Eric] Markus Armbruster (44): error: Improve examples in error.h's big comment error: Document Error API usage rules qdev: Use returned bool to check for qdev_realize() etc. failure macio: Tidy up error handling in macio_newworld_realize() virtio-crypto-pci: Tidy up virtio_crypto_pci_realize() qemu-option: Check return value instead of @err where convenient qemu-option: Make uses of find_desc_by_name() more similar qemu-option: Factor out helper find_default_by_name() qemu-option: Simplify around find_default_by_name() qemu-option: Factor out helper opt_create() qemu-option: Replace opt_set() by cleaner opt_validate() qemu-option: Make functions taking Error ** return bool, not void qemu-option: Use returned bool to check for failure block: Avoid error accumulation in bdrv_img_create() hmp: Eliminate a variable in hmp_migrate_set_parameter() qapi: Make visitor functions taking Error ** return bool, not void qapi: Use returned bool to check for failure, Coccinelle part qapi: Use returned bool to check for failure, manual part block/parallels: Simplify parallels_open() after previous commit s390x/pci: Fix harmless mistake in zpci's property fid's setter qom: Use error_reportf_err() instead of g_printerr() in examples qom: Rename qdev_get_type() to object_get_type() qom: Crash more nicely on object_property_get_link() failure qom: Don't handle impossible object_property_get_link() failure qom: Use return values to check for error where that's simpler qom: Put name parameter before value / visitor parameter qom: Make functions taking Error ** return bool, not void qom: Use returned bool to check for failure, Coccinelle part qom: Use returned bool to check for failure, manual part qom: Make functions taking Error ** return bool, not 0/-1 qdev: Make functions taking Error ** return bool, not void qdev: Use returned bool to check for failure, Coccinelle part error: Avoid unnecessary error_propagate() after error_setg() error: Eliminate error_propagate() with Coccinelle, part 1 error: Eliminate error_propagate() with Coccinelle, part 2 error: Eliminate error_propagate() manually error: Reduce unnecessary error propagation qapi: Smooth another visitor error checking pattern qapi: Smooth visitor error checking in generated code qapi: Purge error_propagate() from QAPI core error: Avoid error_propagate() after migrate_add_blocker() qemu-img: Ignore Error objects where the return value suffices qdev: Ignore Error objects where the return value suffices hmp: Ignore Error objects where the return value suffices docs/devel/qapi-code-gen.txt | 103 - include/hw/audio/pcspk.h
[PATCH v2 44/44] hmp: Ignore Error objects where the return value suffices
qdev_print_props() receives and throws away Error objects just to check for object_property_get_str() and object_property_print() failure. Unnecessary, both return suitable values, so use those instead. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- qdev-monitor.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/qdev-monitor.c b/qdev-monitor.c index 2db38b18af..7ebb97cf0d 100644 --- a/qdev-monitor.c +++ b/qdev-monitor.c @@ -695,22 +695,22 @@ static void qdev_print_props(Monitor *mon, DeviceState *dev, Property *props, if (!props) return; for (; props->name; props++) { -Error *err = NULL; char *value; char *legacy_name = g_strdup_printf("legacy-%s", props->name); + if (object_property_get_type(OBJECT(dev), legacy_name, NULL)) { -value = object_property_get_str(OBJECT(dev), legacy_name, &err); +value = object_property_get_str(OBJECT(dev), legacy_name, NULL); } else { -value = object_property_print(OBJECT(dev), props->name, true, &err); +value = object_property_print(OBJECT(dev), props->name, true, + NULL); } g_free(legacy_name); -if (err) { -error_free(err); +if (!value) { continue; } qdev_printf("%s = %s\n", props->name, -value && *value ? value : ""); +*value ? value : ""); g_free(value); } } -- 2.26.2
[PATCH v2 22/44] qom: Rename qdev_get_type() to object_get_type()
Commit 2f262e06f0 lifted qdev_get_type() from qdev to object without renaming it accordingly. Do that now. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Philippe Mathieu-Daudé --- qom/object.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/qom/object.c b/qom/object.c index 87ee0b5a81..0808da2767 100644 --- a/qom/object.c +++ b/qom/object.c @@ -2365,7 +2365,7 @@ object_class_property_add_tm(ObjectClass *klass, const char *name, NULL, NULL, prop); } -static char *qdev_get_type(Object *obj, Error **errp) +static char *object_get_type(Object *obj, Error **errp) { return g_strdup(object_get_typename(obj)); } @@ -2716,7 +2716,7 @@ void object_class_property_set_description(ObjectClass *klass, static void object_class_init(ObjectClass *klass, void *data) { -object_class_property_add_str(klass, "type", qdev_get_type, +object_class_property_add_str(klass, "type", object_get_type, NULL); } -- 2.26.2
[PATCH v2 15/44] hmp: Eliminate a variable in hmp_migrate_set_parameter()
Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- monitor/hmp-cmds.c | 8 ++-- 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index 2b0b58a336..d7810cb564 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -1247,7 +1247,6 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) MigrateSetParameters *p = g_new0(MigrateSetParameters, 1); uint64_t valuebw = 0; uint64_t cache_size; -MultiFDCompression compress_type; Error *err = NULL; int val, ret; @@ -1343,11 +1342,8 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) break; case MIGRATION_PARAMETER_MULTIFD_COMPRESSION: p->has_multifd_compression = true; -visit_type_MultiFDCompression(v, param, &compress_type, &err); -if (err) { -break; -} -p->multifd_compression = compress_type; +visit_type_MultiFDCompression(v, param, &p->multifd_compression, + &err); break; case MIGRATION_PARAMETER_MULTIFD_ZLIB_LEVEL: p->has_multifd_zlib_level = true; -- 2.26.2
[PATCH v2 35/44] error: Eliminate error_propagate() with Coccinelle, part 2
When all we do with an Error we receive into a local variable is propagating to somewhere else, we can just as well receive it there right away. The previous commit did that with a Coccinelle script I consider fairly trustworthy. This commit uses the same script with the matching of return taken out, i.e. we convert if (!foo(..., &err)) { ... error_propagate(errp, err); ... } to if (!foo(..., errp)) { ... ... } This is unsound: @err could still be read between afterwards. I don't know how to express "no read of @err without an intervening write" in Coccinelle. Instead, I manually double-checked for uses of @err. Suboptimal line breaks tweaked manually. qdev_realize() simplified further to placate scripts/checkpatch.pl. Signed-off-by: Markus Armbruster --- block.c | 6 ++ block/blkdebug.c | 7 ++- block/blklogwrites.c | 3 +-- block/blkverify.c| 3 +-- block/crypto.c | 4 +--- block/file-posix.c | 6 ++ block/file-win32.c | 6 ++ block/gluster.c | 4 +--- block/iscsi.c| 3 +-- block/nbd.c | 8 ++-- block/qcow2.c| 13 - block/raw-format.c | 4 +--- block/sheepdog.c | 8 ++-- block/ssh.c | 4 +--- block/throttle.c | 4 +--- block/vmdk.c | 4 +--- block/vpc.c | 3 +-- block/vvfat.c| 3 +-- blockdev.c | 3 +-- hw/intc/xics.c | 4 +--- hw/vfio/pci.c| 3 +-- net/tap.c| 3 +-- qom/object.c | 4 +--- 23 files changed, 32 insertions(+), 78 deletions(-) diff --git a/block.c b/block.c index 7f3091da63..60d2945c2c 100644 --- a/block.c +++ b/block.c @@ -1629,8 +1629,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file, assert(options != NULL && bs->options != options); opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort); -if (!qemu_opts_absorb_qdict(opts, options, &local_err)) { -error_propagate(errp, local_err); +if (!qemu_opts_absorb_qdict(opts, options, errp)) { ret = -EINVAL; goto fail_opts; } @@ -4090,8 +4089,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, /* Process generic block layer options */ opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort); -if (!qemu_opts_absorb_qdict(opts, reopen_state->options, &local_err)) { -error_propagate(errp, local_err); +if (!qemu_opts_absorb_qdict(opts, reopen_state->options, errp)) { ret = -EINVAL; goto error; } diff --git a/block/blkdebug.c b/block/blkdebug.c index 3c0a9d45cc..9c08d8a005 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -359,7 +359,6 @@ static int blkdebug_parse_perm_list(uint64_t *dest, QDict *options, QObject *crumpled_subqdict = NULL; Visitor *v = NULL; BlockPermissionList *perm_list = NULL, *element; -Error *local_err = NULL; *dest = 0; @@ -375,8 +374,7 @@ static int blkdebug_parse_perm_list(uint64_t *dest, QDict *options, } v = qobject_input_visitor_new(crumpled_subqdict); -if (!visit_type_BlockPermissionList(v, NULL, &perm_list, &local_err)) { -error_propagate(errp, local_err); +if (!visit_type_BlockPermissionList(v, NULL, &perm_list, errp)) { ret = -EINVAL; goto out; } @@ -471,8 +469,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags, uint64_t align; opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); -if (!qemu_opts_absorb_qdict(opts, options, &local_err)) { -error_propagate(errp, local_err); +if (!qemu_opts_absorb_qdict(opts, options, errp)) { ret = -EINVAL; goto out; } diff --git a/block/blklogwrites.c b/block/blklogwrites.c index 0c93e926b1..57315f56b4 100644 --- a/block/blklogwrites.c +++ b/block/blklogwrites.c @@ -149,9 +149,8 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags, bool log_append; opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); -if (!qemu_opts_absorb_qdict(opts, options, &local_err)) { +if (!qemu_opts_absorb_qdict(opts, options, errp)) { ret = -EINVAL; -error_propagate(errp, local_err); goto fail; } diff --git a/block/blkverify.c b/block/blkverify.c index 666d626c57..4aed53ab59 100644 --- a/block/blkverify.c +++ b/block/blkverify.c @@ -116,8 +116,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags, int ret; opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); -if (!qemu_opts_absorb_qdict(opts, options, &local_err)) { -error_propagate(errp, local_err); +if (!qemu_opts_absorb_qdict(opts, options, errp)) { ret = -EINVAL; goto fail; } diff --git a/block/crypto.c b/block/crypto.c index 301fa4da8e..5b2b97ea2f 100644 ---
[PATCH v2 42/44] qemu-img: Ignore Error objects where the return value suffices
Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- qemu-img.c | 8 ++-- 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/qemu-img.c b/qemu-img.c index 27bf94e7ae..c11bfe0268 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -464,22 +464,18 @@ static int add_old_style_options(const char *fmt, QemuOpts *opts, const char *base_filename, const char *base_fmt) { -Error *err = NULL; - if (base_filename) { if (!qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename, - &err)) { + NULL)) { error_report("Backing file not supported for file format '%s'", fmt); -error_free(err); return -1; } } if (base_fmt) { -if (!qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt, &err)) { +if (!qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt, NULL)) { error_report("Backing file format not supported for file " "format '%s'", fmt); -error_free(err); return -1; } } -- 2.26.2
[PATCH v2 06/44] qemu-option: Check return value instead of @err where convenient
Convert uses like opts = qemu_opts_create(..., &err); if (err) { ... } to opts = qemu_opts_create(..., &err); if (!opts) { ... } Eliminate error_propagate() that are now unnecessary. Delete @err that are now unused. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy --- block/parallels.c | 4 ++-- blockdev.c | 5 ++--- qdev-monitor.c | 5 ++--- util/qemu-config.c | 10 -- util/qemu-option.c | 12 5 files changed, 14 insertions(+), 22 deletions(-) diff --git a/block/parallels.c b/block/parallels.c index 63a1cde8af..f26f03c926 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -824,8 +824,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, } } -opts = qemu_opts_create(¶llels_runtime_opts, NULL, 0, &local_err); -if (local_err != NULL) { +opts = qemu_opts_create(¶llels_runtime_opts, NULL, 0, errp); +if (!opts) { goto fail_options; } diff --git a/blockdev.c b/blockdev.c index 31d5eaf6bf..b52ed9de86 100644 --- a/blockdev.c +++ b/blockdev.c @@ -504,9 +504,8 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, /* Check common options by copying from bs_opts to opts, all other options * stay in bs_opts for processing by bdrv_open(). */ id = qdict_get_try_str(bs_opts, "id"); -opts = qemu_opts_create(&qemu_common_drive_opts, id, 1, &error); -if (error) { -error_propagate(errp, error); +opts = qemu_opts_create(&qemu_common_drive_opts, id, 1, errp); +if (!opts) { goto err_no_opts; } diff --git a/qdev-monitor.c b/qdev-monitor.c index 13a13a811a..079cb6001e 100644 --- a/qdev-monitor.c +++ b/qdev-monitor.c @@ -799,9 +799,8 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp) QemuOpts *opts; DeviceState *dev; -opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err); -if (local_err) { -error_propagate(errp, local_err); +opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, errp); +if (!opts) { return; } if (!monitor_cur_is_qmp() && qdev_device_help(opts)) { diff --git a/util/qemu-config.c b/util/qemu-config.c index 772f5a219e..c0d0e9b8ef 100644 --- a/util/qemu-config.c +++ b/util/qemu-config.c @@ -493,9 +493,8 @@ static void config_parse_qdict_section(QDict *options, QemuOptsList *opts, goto out; } -subopts = qemu_opts_create(opts, NULL, 0, &local_err); -if (local_err) { -error_propagate(errp, local_err); +subopts = qemu_opts_create(opts, NULL, 0, errp); +if (!subopts) { goto out; } @@ -538,10 +537,9 @@ static void config_parse_qdict_section(QDict *options, QemuOptsList *opts, } opt_name = g_strdup_printf("%s.%u", opts->name, i++); -subopts = qemu_opts_create(opts, opt_name, 1, &local_err); +subopts = qemu_opts_create(opts, opt_name, 1, errp); g_free(opt_name); -if (local_err) { -error_propagate(errp, local_err); +if (!subopts) { goto out; } diff --git a/util/qemu-option.c b/util/qemu-option.c index 0ebfd97a98..fd1fd23521 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -670,11 +670,9 @@ void qemu_opts_set(QemuOptsList *list, const char *id, const char *name, const char *value, Error **errp) { QemuOpts *opts; -Error *local_err = NULL; -opts = qemu_opts_create(list, id, 1, &local_err); -if (local_err) { -error_propagate(errp, local_err); +opts = qemu_opts_create(list, id, 1, errp); +if (!opts) { return; } qemu_opt_set(opts, name, value, errp); @@ -1012,10 +1010,8 @@ QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict, QemuOpts *opts; const QDictEntry *entry; -opts = qemu_opts_create(list, qdict_get_try_str(qdict, "id"), 1, -&local_err); -if (local_err) { -error_propagate(errp, local_err); +opts = qemu_opts_create(list, qdict_get_try_str(qdict, "id"), 1, errp); +if (!opts) { return NULL; } -- 2.26.2
[PATCH v2 19/44] block/parallels: Simplify parallels_open() after previous commit
Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- block/parallels.c | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/block/parallels.c b/block/parallels.c index 32d0ecd398..e0ec819550 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -843,6 +843,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, &local_err); g_free(buf); if (local_err != NULL) { +error_propagate(errp, local_err); goto fail_options; } @@ -873,15 +874,11 @@ 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_vfree(s->header); return ret; - -fail_options: -error_propagate(errp, local_err); -ret = -EINVAL; -goto fail; } -- 2.26.2
[PATCH v2 10/44] qemu-option: Factor out helper opt_create()
There is just one use so far. The next commit will add more. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy --- util/qemu-option.c | 27 ++- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/util/qemu-option.c b/util/qemu-option.c index e7b540a21b..1023fe7527 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -499,6 +499,23 @@ int qemu_opt_unset(QemuOpts *opts, const char *name) } } +static QemuOpt *opt_create(QemuOpts *opts, const char *name, char *value, + bool prepend) +{ +QemuOpt *opt = g_malloc0(sizeof(*opt)); + +opt->name = g_strdup(name); +opt->str = value; +opt->opts = opts; +if (prepend) { +QTAILQ_INSERT_HEAD(&opts->head, opt, next); +} else { +QTAILQ_INSERT_TAIL(&opts->head, opt, next); +} + +return opt; +} + static void opt_set(QemuOpts *opts, const char *name, char *value, bool prepend, bool *help_wanted, Error **errp) { @@ -516,16 +533,8 @@ static void opt_set(QemuOpts *opts, const char *name, char *value, return; } -opt = g_malloc0(sizeof(*opt)); -opt->name = g_strdup(name); -opt->opts = opts; -if (prepend) { -QTAILQ_INSERT_HEAD(&opts->head, opt, next); -} else { -QTAILQ_INSERT_TAIL(&opts->head, opt, next); -} +opt = opt_create(opts, name, value, prepend); opt->desc = desc; -opt->str = value; qemu_opt_parse(opt, &local_err); if (local_err) { error_propagate(errp, local_err); -- 2.26.2
[PATCH v2 16/44] qapi: Make visitor functions taking Error ** return bool, not void
See recent commit "error: Document Error API usage rules" for rationale. Signed-off-by: Markus Armbruster --- docs/devel/qapi-code-gen.txt | 51 +-- include/qapi/clone-visitor.h | 8 +- include/qapi/visitor-impl.h | 26 +++--- include/qapi/visitor.h| 102 - audio/audio_legacy.c | 15 ++-- qapi/opts-visitor.c | 58 +++- qapi/qapi-clone-visitor.c | 67 -- qapi/qapi-dealloc-visitor.c | 27 -- qapi/qapi-visit-core.c| 165 ++ qapi/qobject-input-visitor.c | 109 +- qapi/qobject-output-visitor.c | 27 -- qapi/string-input-visitor.c | 62 +++-- qapi/string-output-visitor.c | 32 --- scripts/qapi/visit.py | 58 +--- 14 files changed, 452 insertions(+), 355 deletions(-) diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt index a7794ef658..9bfc57063c 100644 --- a/docs/devel/qapi-code-gen.txt +++ b/docs/devel/qapi-code-gen.txt @@ -1408,42 +1408,38 @@ Example: #include "example-qapi-types.h" -void visit_type_UserDefOne_members(Visitor *v, UserDefOne *obj, Error **errp); -void visit_type_UserDefOne(Visitor *v, const char *name, UserDefOne **obj, Error **errp); -void visit_type_UserDefOneList(Visitor *v, const char *name, UserDefOneList **obj, Error **errp); +bool visit_type_UserDefOne_members(Visitor *v, UserDefOne *obj, Error **errp); +bool visit_type_UserDefOne(Visitor *v, const char *name, UserDefOne **obj, Error **errp); +bool visit_type_UserDefOneList(Visitor *v, const char *name, UserDefOneList **obj, Error **errp); -void visit_type_q_obj_my_command_arg_members(Visitor *v, q_obj_my_command_arg *obj, Error **errp); +bool visit_type_q_obj_my_command_arg_members(Visitor *v, q_obj_my_command_arg *obj, Error **errp); #endif /* EXAMPLE_QAPI_VISIT_H */ $ cat qapi-generated/example-qapi-visit.c [Uninteresting stuff omitted...] -void visit_type_UserDefOne_members(Visitor *v, UserDefOne *obj, Error **errp) +bool visit_type_UserDefOne_members(Visitor *v, UserDefOne *obj, Error **errp) { Error *err = NULL; -visit_type_int(v, "integer", &obj->integer, &err); -if (err) { -goto out; +if (!visit_type_int(v, "integer", &obj->integer, errp)) { +return false; } if (visit_optional(v, "string", &obj->has_string)) { -visit_type_str(v, "string", &obj->string, &err); -if (err) { -goto out; +if (!visit_type_str(v, "string", &obj->string, errp)) { +return false; } } - -out: error_propagate(errp, err); +return !err; } -void visit_type_UserDefOne(Visitor *v, const char *name, UserDefOne **obj, Error **errp) +bool visit_type_UserDefOne(Visitor *v, const char *name, UserDefOne **obj, Error **errp) { Error *err = NULL; -visit_start_struct(v, name, (void **)obj, sizeof(UserDefOne), &err); -if (err) { -goto out; +if (!visit_start_struct(v, name, (void **)obj, sizeof(UserDefOne), errp)) { +return false; } if (!*obj) { /* incomplete */ @@ -1461,19 +1457,18 @@ Example: qapi_free_UserDefOne(*obj); *obj = NULL; } -out: error_propagate(errp, err); +return !err; } -void visit_type_UserDefOneList(Visitor *v, const char *name, UserDefOneList **obj, Error **errp) +bool visit_type_UserDefOneList(Visitor *v, const char *name, UserDefOneList **obj, Error **errp) { Error *err = NULL; UserDefOneList *tail; size_t size = sizeof(**obj); -visit_start_list(v, name, (GenericList **)obj, size, &err); -if (err) { -goto out; +if (!visit_start_list(v, name, (GenericList **)obj, size, errp)) { +return false; } for (tail = *obj; tail; @@ -1492,21 +1487,19 @@ Example: qapi_free_UserDefOneList(*obj); *obj = NULL; } -out: error_propagate(errp, err); +return !err; } -void visit_type_q_obj_my_command_arg_members(Visitor *v, q_obj_my_command_arg *obj, Error **errp) +bool visit_type_q_obj_my_command_arg_members(Visitor *v, q_obj_my_command_arg *obj, Error **errp) { Error *err = NULL; -visit_type_UserDefOneList(v, "arg1", &obj->arg1, &err); -if (err) { -goto out; +if (!visit_type_UserDefOneList(v, "arg1", &obj->arg1, errp)) { +return false; } - -out: error_propagate(errp, err); +return !err; } [Uninteresting stuff omitted...] diff --git a/include/qapi/clone-visitor.h b/include/qapi/clone-visitor.h index 5b665ee38c..adf9a788e2 100644 --- a/inclu
[PATCH v2 33/44] error: Avoid unnecessary error_propagate() after error_setg()
Replace error_setg(&err, ...); error_propagate(errp, err); by error_setg(errp, ...); Related pattern: if (...) { error_setg(&err, ...); goto out; } ... out: error_propagate(errp, err); return; When all paths to label out are that way, replace by if (...) { error_setg(errp, ...); return; } and delete the label along with the error_propagate(). When we have at most one other path that actually needs to propagate, and maybe one at the end that where propagation is unnecessary, e.g. foo(..., &err); if (err) { goto out; } ... bar(..., &err); out: error_propagate(errp, err); return; move the error_propagate() to where it's needed, like if (...) { foo(..., &err); error_propagate(errp, err); return; } ... bar(..., errp); return; and transform the error_setg() as above. Bonus: the elimination of gotos will make later patches in this series easier to review. Candidates for conversion tracked down with this Coccinelle script: @@ identifier err, errp; expression list args; @@ -error_setg(&err, args); +error_setg(errp, args); ... when != err error_propagate(errp, err); Signed-off-by: Markus Armbruster --- backends/cryptodev.c| 11 +++--- backends/hostmem-file.c | 19 +++--- backends/hostmem-memfd.c| 15 backends/hostmem.c | 27 ++ block/quorum.c | 16 block/replication.c | 11 +++--- block/throttle-groups.c | 22 +-- block/vxhs.c| 9 ++--- hw/acpi/core.c | 15 +++- hw/hyperv/vmbus.c | 5 +-- hw/i386/pc.c| 35 ++ hw/mem/nvdimm.c | 17 - hw/mem/pc-dimm.c| 14 +++ hw/misc/aspeed_sdmc.c | 3 +- hw/ppc/rs6000_mc.c | 9 ++--- hw/ppc/spapr.c | 73 - hw/ppc/spapr_pci.c | 14 +++ hw/s390x/ipl.c | 23 +--- hw/xen/xen_pt_config_init.c | 3 +- iothread.c | 12 +++--- net/colo-compare.c | 20 -- net/dump.c | 10 ++--- net/filter-buffer.c | 10 ++--- qga/commands-win32.c| 19 +++--- 24 files changed, 169 insertions(+), 243 deletions(-) diff --git a/backends/cryptodev.c b/backends/cryptodev.c index 72b7077475..4de378532b 100644 --- a/backends/cryptodev.c +++ b/backends/cryptodev.c @@ -158,16 +158,15 @@ cryptodev_backend_set_queues(Object *obj, Visitor *v, const char *name, uint32_t value; if (!visit_type_uint32(v, name, &value, &local_err)) { -goto out; +error_propagate(errp, local_err); +return; } if (!value) { -error_setg(&local_err, "Property '%s.%s' doesn't take value '%" - PRIu32 "'", object_get_typename(obj), name, value); -goto out; +error_setg(errp, "Property '%s.%s' doesn't take value '%" PRIu32 "'", + object_get_typename(obj), name, value); +return; } backend->conf.peers.queues = value; -out: -error_propagate(errp, local_err); } static void diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c index 320dffbaa9..a44f5a61ac 100644 --- a/backends/hostmem-file.c +++ b/backends/hostmem-file.c @@ -114,18 +114,16 @@ static void file_memory_backend_set_align(Object *o, Visitor *v, uint64_t val; if (host_memory_backend_mr_inited(backend)) { -error_setg(&local_err, "cannot change property '%s' of %s", - name, object_get_typename(o)); -goto out; +error_setg(errp, "cannot change property '%s' of %s", name, + object_get_typename(o)); +return; } if (!visit_type_size(v, name, &val, &local_err)) { -goto out; +error_propagate(errp, local_err); +return; } fb->align = val; - - out: -error_propagate(errp, local_err); } static bool file_memory_backend_get_pmem(Object *o, Error **errp) @@ -139,7 +137,6 @@ static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp) HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o); if (host_memory_backend_mr_inited(backend)) { - error_setg(errp, "cannot change property 'pmem' of %s.", object_get_typename(o)); return; @@ -147,13 +144,9 @@ static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp) #ifndef CONFIG_LIBPMEM if (value) { -Error *local_err = NULL; - -error_setg(&local_err, - "Lack of libpmem support while setting the 'pmem=on'" +error_setg(errp, "Lack of libpmem support while setting the 'pmem=on'" " of %s. We can't ensure data persistence.", object_get_typename(o));
Re: [PATCH v2 00/44] Less clumsy error checking
diff -w between v1 rebased and v2, with: diff --git a/include/qapi/error.h b/include/qapi/error.h index c3d84d610a..5ceb3ace06 100644 --- a/include/qapi/error.h +++ b/include/qapi/error.h @@ -145,10 +145,10 @@ * Likewise, do *not* * Error *err = NULL; * if (cond1) { - * error_setg(err, ...); + * error_setg(&err, ...); * } * if (cond2) { - * error_setg(err, ...); // WRONG! + * error_setg(&err, ...); // WRONG! * } * because this may pass a non-null err to error_setg(). */ diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h index 7d1b8d579c..ebc19ede7f 100644 --- a/include/qapi/visitor.h +++ b/include/qapi/visitor.h @@ -284,7 +284,7 @@ void visit_free(Visitor *v); * On failure, set *@obj to NULL and store an error through @errp. * Can happen only when @v is an input visitor. * - * Return true on succes, false on failure. + * Return true on success, false on failure. * * After visit_start_struct() succeeds, the caller may visit its * members one after the other, passing the member's name and address @@ -304,7 +304,7 @@ bool visit_start_struct(Visitor *v, const char *name, void **obj, * On failure, store an error through @errp. Can happen only when @v * is an input visitor. * - * Return true on succes, false on failure. + * Return true on success, false on failure. * * Should be called prior to visit_end_struct() if all other * intermediate visit steps were successful, to allow the visitor one @@ -343,7 +343,7 @@ void visit_end_struct(Visitor *v, void **obj); * On failure, set *@list to NULL and store an error through @errp. * Can happen only when @v is an input visitor. * - * Return true on succes, false on failure. + * Return true on success, false on failure. * * After visit_start_list() succeeds, the caller may visit its members * one after the other. A real visit (where @list is non-NULL) uses @@ -380,7 +380,7 @@ GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size); * On failure, store an error through @errp. Can happen only when @v * is an input visitor. * - * Return true on succes, false on failure. + * Return true on success, false on failure. * * Should be called prior to visit_end_list() if all other * intermediate visit steps were successful, to allow the visitor one @@ -418,7 +418,7 @@ void visit_end_list(Visitor *v, void **list); * On failure, set *@obj to NULL and store an error through @errp. * Can happen only when @v is an input visitor. * - * Return true on succes, false on failure. + * Return true on success, false on failure. * * If successful, this must be paired with visit_end_alternate() with * the same @obj to clean up, even if visiting the contents of the @@ -476,7 +476,7 @@ bool visit_optional(Visitor *v, const char *name, bool *present); * On failure, store an error through @errp. Can happen only when @v * is an input visitor. * - * Return true on succes, false on failure. + * Return true on success, false on failure. * * May call visit_type_str() under the hood, and the enum visit may * fail even if the corresponding string visit succeeded; this implies @@ -510,7 +510,7 @@ bool visit_is_dealloc(Visitor *v); * On failure, store an error through @errp. Can happen only when @v * is an input visitor. * - * Return true on succes, false on failure. + * Return true on success, false on failure. */ bool visit_type_int(Visitor *v, const char *name, int64_t *obj, Error **errp); @@ -591,7 +591,7 @@ bool visit_type_size(Visitor *v, const char *name, uint64_t *obj, * On failure, store an error through @errp. Can happen only when @v * is an input visitor. * - * Return true on succes, false on failure. + * Return true on success, false on failure. */ bool visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp); @@ -612,7 +612,7 @@ bool visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp); * On failure, set *@obj to NULL and store an error through @errp. * Can happen only when @v is an input visitor. * - * Return true on succes, false on failure. + * Return true on success, false on failure. * * FIXME: Callers that try to output NULL *obj should not be allowed. */ @@ -631,7 +631,7 @@ bool visit_type_str(Visitor *v, const char *name, char **obj, Error **errp); * On failure, store an error through @errp. Can happen only when @v * is an input visitor. * - * Return true on succes, false on failure. + * Return true on success, false on failure. */ bool visit_type_number(Visitor *v, const char *name, double *obj, Error **errp); @@ -649,7 +649,7 @@ bool visit_type_number(Visitor *v, const char *name, double *obj, * On failure, set *@obj to NULL and store an error through @errp. * Can happen only when @v is an input visitor. * - * Return true on succes, false on failure. + * Return true on success, false on failure. * *
[PATCH v2 07/44] qemu-option: Make uses of find_desc_by_name() more similar
This is to make the next commit easier to review. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy --- util/qemu-option.c | 32 ++-- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/util/qemu-option.c b/util/qemu-option.c index fd1fd23521..1df55bc881 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -270,6 +270,7 @@ static void qemu_opt_del_all(QemuOpts *opts, const char *name) const char *qemu_opt_get(QemuOpts *opts, const char *name) { QemuOpt *opt; +const QemuOptDesc *desc; if (opts == NULL) { return NULL; @@ -277,7 +278,7 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name) opt = qemu_opt_find(opts, name); if (!opt) { -const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name); +desc = find_desc_by_name(opts->list->desc, name); if (desc && desc->def_value_str) { return desc->def_value_str; } @@ -348,6 +349,7 @@ static bool qemu_opt_get_bool_helper(QemuOpts *opts, const char *name, bool defval, bool del) { QemuOpt *opt; +const QemuOptDesc *desc; bool ret = defval; if (opts == NULL) { @@ -356,7 +358,7 @@ static bool qemu_opt_get_bool_helper(QemuOpts *opts, const char *name, opt = qemu_opt_find(opts, name); if (opt == NULL) { -const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name); +desc = find_desc_by_name(opts->list->desc, name); if (desc && desc->def_value_str) { parse_option_bool(name, desc->def_value_str, &ret, &error_abort); } @@ -384,6 +386,7 @@ static uint64_t qemu_opt_get_number_helper(QemuOpts *opts, const char *name, uint64_t defval, bool del) { QemuOpt *opt; +const QemuOptDesc *desc; uint64_t ret = defval; if (opts == NULL) { @@ -392,7 +395,7 @@ static uint64_t qemu_opt_get_number_helper(QemuOpts *opts, const char *name, opt = qemu_opt_find(opts, name); if (opt == NULL) { -const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name); +desc = find_desc_by_name(opts->list->desc, name); if (desc && desc->def_value_str) { parse_option_number(name, desc->def_value_str, &ret, &error_abort); } @@ -421,6 +424,7 @@ static uint64_t qemu_opt_get_size_helper(QemuOpts *opts, const char *name, uint64_t defval, bool del) { QemuOpt *opt; +const QemuOptDesc *desc; uint64_t ret = defval; if (opts == NULL) { @@ -429,7 +433,7 @@ static uint64_t qemu_opt_get_size_helper(QemuOpts *opts, const char *name, opt = qemu_opt_find(opts, name); if (opt == NULL) { -const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name); +desc = find_desc_by_name(opts->list->desc, name); if (desc && desc->def_value_str) { parse_option_size(name, desc->def_value_str, &ret, &error_abort); } @@ -540,18 +544,18 @@ void qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val, Error **errp) { QemuOpt *opt; -const QemuOptDesc *desc = opts->list->desc; +const QemuOptDesc *desc; -opt = g_malloc0(sizeof(*opt)); -opt->desc = find_desc_by_name(desc, name); -if (!opt->desc && !opts_accepts_any(opts)) { +desc = find_desc_by_name(opts->list->desc, name); +if (!desc && !opts_accepts_any(opts)) { error_setg(errp, QERR_INVALID_PARAMETER, name); -g_free(opt); return; } +opt = g_malloc0(sizeof(*opt)); opt->name = g_strdup(name); opt->opts = opts; +opt->desc = desc; opt->value.boolean = !!val; opt->str = g_strdup(val ? "on" : "off"); QTAILQ_INSERT_TAIL(&opts->head, opt, next); @@ -561,18 +565,18 @@ void qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val, Error **errp) { QemuOpt *opt; -const QemuOptDesc *desc = opts->list->desc; +const QemuOptDesc *desc; -opt = g_malloc0(sizeof(*opt)); -opt->desc = find_desc_by_name(desc, name); -if (!opt->desc && !opts_accepts_any(opts)) { +desc = find_desc_by_name(opts->list->desc, name); +if (!desc && !opts_accepts_any(opts)) { error_setg(errp, QERR_INVALID_PARAMETER, name); -g_free(opt); return; } +opt = g_malloc0(sizeof(*opt)); opt->name = g_strdup(name); opt->opts = opts; +opt->desc = desc; opt->value.uint = val; opt->str = g_strdup_printf("%" PRId64, val); QTAILQ_INSERT_TAIL(&opts->head, opt, next); -- 2.26.2
[PATCH v2 18/44] qapi: Use returned bool to check for failure, manual part
The previous commit used Coccinelle to convert from checking the Error object to checking the return value. Convert a few more manually. Also tweak control flow in places to conform to the conventional "if error bail out" pattern. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- accel/kvm/kvm-all.c | 50 ++- block/throttle-groups.c | 5 ++-- bootdevice.c | 4 ++-- hw/core/qdev-properties.c | 12 +- hw/ide/qdev.c | 4 ++-- hw/mem/nvdimm.c | 9 +++ hw/net/ne2000-isa.c | 4 ++-- hw/usb/dev-storage.c | 4 ++-- net/net.c | 8 ++- 9 files changed, 44 insertions(+), 56 deletions(-) diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index 397669231d..83f03b2018 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -3128,37 +3128,33 @@ static void kvm_set_kernel_irqchip(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { -Error *err = NULL; KVMState *s = KVM_STATE(obj); OnOffSplit mode; -visit_type_OnOffSplit(v, name, &mode, &err); -if (err) { -error_propagate(errp, err); +if (!visit_type_OnOffSplit(v, name, &mode, errp)) { return; -} else { -switch (mode) { -case ON_OFF_SPLIT_ON: -s->kernel_irqchip_allowed = true; -s->kernel_irqchip_required = true; -s->kernel_irqchip_split = ON_OFF_AUTO_OFF; -break; -case ON_OFF_SPLIT_OFF: -s->kernel_irqchip_allowed = false; -s->kernel_irqchip_required = false; -s->kernel_irqchip_split = ON_OFF_AUTO_OFF; -break; -case ON_OFF_SPLIT_SPLIT: -s->kernel_irqchip_allowed = true; -s->kernel_irqchip_required = true; -s->kernel_irqchip_split = ON_OFF_AUTO_ON; -break; -default: -/* The value was checked in visit_type_OnOffSplit() above. If - * we get here, then something is wrong in QEMU. - */ -abort(); -} +} +switch (mode) { +case ON_OFF_SPLIT_ON: +s->kernel_irqchip_allowed = true; +s->kernel_irqchip_required = true; +s->kernel_irqchip_split = ON_OFF_AUTO_OFF; +break; +case ON_OFF_SPLIT_OFF: +s->kernel_irqchip_allowed = false; +s->kernel_irqchip_required = false; +s->kernel_irqchip_split = ON_OFF_AUTO_OFF; +break; +case ON_OFF_SPLIT_SPLIT: +s->kernel_irqchip_allowed = true; +s->kernel_irqchip_required = true; +s->kernel_irqchip_split = ON_OFF_AUTO_ON; +break; +default: +/* The value was checked in visit_type_OnOffSplit() above. If + * we get here, then something is wrong in QEMU. + */ +abort(); } } diff --git a/block/throttle-groups.c b/block/throttle-groups.c index bb242fde1a..e411051160 100644 --- a/block/throttle-groups.c +++ b/block/throttle-groups.c @@ -895,8 +895,8 @@ static void throttle_group_set_limits(Object *obj, Visitor *v, ThrottleLimits *argp; Error *local_err = NULL; -if (!visit_type_ThrottleLimits(v, name, &argp, &local_err)) { -goto ret; +if (!visit_type_ThrottleLimits(v, name, &argp, errp)) { +return; } qemu_mutex_lock(&tg->lock); throttle_get_config(&tg->ts, &cfg); @@ -908,7 +908,6 @@ static void throttle_group_set_limits(Object *obj, Visitor *v, unlock: qemu_mutex_unlock(&tg->lock); -ret: qapi_free_ThrottleLimits(argp); error_propagate(errp, local_err); return; diff --git a/bootdevice.c b/bootdevice.c index fb09d3c668..769f40c77d 100644 --- a/bootdevice.c +++ b/bootdevice.c @@ -297,8 +297,8 @@ static void device_set_bootindex(Object *obj, Visitor *v, const char *name, int32_t boot_index; Error *local_err = NULL; -if (!visit_type_int32(v, name, &boot_index, &local_err)) { -goto out; +if (!visit_type_int32(v, name, &boot_index, errp)) { +return; } /* check whether bootindex is present in fw_boot_order list */ check_boot_index(boot_index, &local_err); diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c index c1a0f910d8..e4dd9ff6b2 100644 --- a/hw/core/qdev-properties.c +++ b/hw/core/qdev-properties.c @@ -672,15 +672,15 @@ static void set_pci_devfn(Object *obj, Visitor *v, const char *name, if (!visit_type_str(v, name, &str, &local_err)) { error_free(local_err); local_err = NULL; -visit_type_int32(v, name, &value, &local_err); -if (local_err) { -error_propagate(errp, local_err); -} else if (value < -1 || value > 255) { +if (!visit_type_int32(v, name, &value, errp)) { +return; +} +if (value < -1 || value > 255) { error_setg(errp, QERR_IN
[PATCH v2 37/44] error: Reduce unnecessary error propagation
When all we do with an Error we receive into a local variable is propagating to somewhere else, we can just as well receive it there right away, even when we need to keep error_propagate() for other error paths. Signed-off-by: Markus Armbruster --- block.c | 2 +- block/gluster.c | 8 block/parallels.c | 2 +- block/quorum.c | 2 +- block/replication.c | 3 +-- block/vxhs.c| 4 ++-- hw/core/qdev.c | 2 +- hw/net/virtio-net.c | 4 ++-- 8 files changed, 13 insertions(+), 14 deletions(-) diff --git a/block.c b/block.c index 60d2945c2c..2dcf9afd61 100644 --- a/block.c +++ b/block.c @@ -6073,7 +6073,7 @@ void bdrv_img_create(const char *filename, const char *fmt, /* Parse -o options */ if (options) { -if (!qemu_opts_do_parse(opts, options, NULL, &local_err)) { +if (!qemu_opts_do_parse(opts, options, NULL, errp)) { goto out; } } diff --git a/block/gluster.c b/block/gluster.c index c620880f27..4f1448e2bc 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -523,7 +523,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf, /* create opts info from runtime_json_opts list */ opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort); -if (!qemu_opts_absorb_qdict(opts, options, &local_err)) { +if (!qemu_opts_absorb_qdict(opts, options, errp)) { goto out; } @@ -554,7 +554,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf, /* create opts info from runtime_type_opts list */ opts = qemu_opts_create(&runtime_type_opts, NULL, 0, &error_abort); -if (!qemu_opts_absorb_qdict(opts, backing_options, &local_err)) { +if (!qemu_opts_absorb_qdict(opts, backing_options, errp)) { goto out; } @@ -584,7 +584,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf, if (gsconf->type == SOCKET_ADDRESS_TYPE_INET) { /* create opts info from runtime_inet_opts list */ opts = qemu_opts_create(&runtime_inet_opts, NULL, 0, &error_abort); -if (!qemu_opts_absorb_qdict(opts, backing_options, &local_err)) { +if (!qemu_opts_absorb_qdict(opts, backing_options, errp)) { goto out; } @@ -632,7 +632,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf, } else { /* create opts info from runtime_unix_opts list */ opts = qemu_opts_create(&runtime_unix_opts, NULL, 0, &error_abort); -if (!qemu_opts_absorb_qdict(opts, backing_options, &local_err)) { +if (!qemu_opts_absorb_qdict(opts, backing_options, errp)) { goto out; } diff --git a/block/parallels.c b/block/parallels.c index 324085ccea..c259799111 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -827,7 +827,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, goto fail_options; } -if (!qemu_opts_absorb_qdict(opts, options, &local_err)) { +if (!qemu_opts_absorb_qdict(opts, options, errp)) { goto fail_options; } diff --git a/block/quorum.c b/block/quorum.c index 5d52e605db..6df9449fc2 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -921,7 +921,7 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags, } opts = qemu_opts_create(&quorum_runtime_opts, NULL, 0, &error_abort); -if (!qemu_opts_absorb_qdict(opts, options, &local_err)) { +if (!qemu_opts_absorb_qdict(opts, options, errp)) { ret = -EINVAL; goto exit; } diff --git a/block/replication.c b/block/replication.c index dcd430624e..0c70215784 100644 --- a/block/replication.c +++ b/block/replication.c @@ -85,7 +85,6 @@ static int replication_open(BlockDriverState *bs, QDict *options, { int ret; BDRVReplicationState *s = bs->opaque; -Error *local_err = NULL; QemuOpts *opts = NULL; const char *mode; const char *top_id; @@ -99,7 +98,7 @@ static int replication_open(BlockDriverState *bs, QDict *options, ret = -EINVAL; opts = qemu_opts_create(&replication_runtime_opts, NULL, 0, &error_abort); -if (!qemu_opts_absorb_qdict(opts, options, &local_err)) { +if (!qemu_opts_absorb_qdict(opts, options, errp)) { goto fail; } diff --git a/block/vxhs.c b/block/vxhs.c index fecaeb82c9..dc0e254730 100644 --- a/block/vxhs.c +++ b/block/vxhs.c @@ -318,7 +318,7 @@ static int vxhs_open(BlockDriverState *bs, QDict *options, opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); tcp_opts = qemu_opts_create(&runtime_tcp_opts, NULL, 0, &error_abort); -if (!qemu_opts_absorb_qdict(opts, options, &local_err)) { +if (!qemu_opts_absorb_qdict(opts, options, errp)) { ret = -EINVAL; goto out; } @@ -345,7 +345,7 @@ static int vxhs_open(BlockDriverState *bs, QDict *options, /* get th
[PATCH v2 27/44] qom: Make functions taking Error ** return bool, not void
See recent commit "error: Document Error API usage rules" for rationale. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- include/qom/object.h| 42 ++ include/qom/object_interfaces.h | 12 +++- include/qom/qom-qobject.h | 4 +- qom/object.c| 98 - qom/object_interfaces.c | 21 --- qom/qom-qobject.c | 6 +- 6 files changed, 121 insertions(+), 62 deletions(-) diff --git a/include/qom/object.h b/include/qom/object.h index 7ef9c8d0cc..b2d2558245 100644 --- a/include/qom/object.h +++ b/include/qom/object.h @@ -703,7 +703,7 @@ Object *object_new_with_propv(const char *typename, Error **errp, va_list vargs); -void object_apply_global_props(Object *obj, const GPtrArray *props, +bool object_apply_global_props(Object *obj, const GPtrArray *props, Error **errp); void object_set_machine_compat_props(GPtrArray *compat_props); void object_set_accelerator_compat_props(GPtrArray *compat_props); @@ -798,8 +798,10 @@ void object_initialize(void *obj, size_t size, const char *typename); * strings. The propname of %NULL indicates the end of the property list. * If the object implements the user creatable interface, the object will * be marked complete once all the properties have been processed. + * + * Returns: %true on success, %false on failure. */ -void object_initialize_child_with_props(Object *parentobj, +bool object_initialize_child_with_props(Object *parentobj, const char *propname, void *childobj, size_t size, const char *type, Error **errp, ...) QEMU_SENTINEL; @@ -815,8 +817,10 @@ void object_initialize_child_with_props(Object *parentobj, * @vargs: list of property names and values * * See object_initialize_child() for documentation. + * + * Returns: %true on success, %false on failure. */ -void object_initialize_child_with_propsv(Object *parentobj, +bool object_initialize_child_with_propsv(Object *parentobj, const char *propname, void *childobj, size_t size, const char *type, Error **errp, va_list vargs); @@ -1197,8 +1201,10 @@ void object_unparent(Object *obj); * @errp: returns an error if this function fails * * Reads a property from a object. + * + * Returns: %true on success, %false on failure. */ -void object_property_get(Object *obj, const char *name, Visitor *v, +bool object_property_get(Object *obj, const char *name, Visitor *v, Error **errp); /** @@ -1208,8 +1214,10 @@ void object_property_get(Object *obj, const char *name, Visitor *v, * @errp: returns an error if this function fails * * Writes a string value to a property. + * + * Returns: %true on success, %false on failure. */ -void object_property_set_str(Object *obj, +bool object_property_set_str(Object *obj, const char *name, const char *value, Error **errp); @@ -1238,8 +1246,9 @@ char *object_property_get_str(Object *obj, const char *name, * OBJ_PROP_LINK_STRONG bit, the old target object is * unreferenced, and a reference is added to the new target object. * + * Returns: %true on success, %false on failure. */ -void object_property_set_link(Object *obj, const char *name, Object *value, +bool object_property_set_link(Object *obj, const char *name, Object *value, Error **errp); /** @@ -1262,8 +1271,10 @@ Object *object_property_get_link(Object *obj, const char *name, * @errp: returns an error if this function fails * * Writes a bool value to a property. + * + * Returns: %true on success, %false on failure. */ -void object_property_set_bool(Object *obj, const char *name, bool value, +bool object_property_set_bool(Object *obj, const char *name, bool value, Error **errp); /** @@ -1285,8 +1296,10 @@ bool object_property_get_bool(Object *obj, const char *name, * @errp: returns an error if this function fails * * Writes an integer value to a property. + * + * Returns: %true on success, %false on failure. */ -void object_property_set_int(Object *obj, const char *name, int64_t value, +bool object_property_set_int(Object *obj, const char *name, int64_t value, Error **errp); /** @@ -1308,8 +1321,10 @@ int64_t object_property_get_int(Object *obj, const char *name, * @errp: returns an error if this function fails * * Writes an unsigned integer value to a property. + * + * Returns: %true on success, %false on failure. */ -void object_property_set_uint(Object *obj, const char *name, uint64_t value, +bool object_property_set_uint(Object *obj, const char *name, uint64_t value, E
[PATCH v2 30/44] qom: Make functions taking Error ** return bool, not 0/-1
Just for consistency. Also fix the example in object_set_props()'s documentation. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- include/qom/object.h | 28 +++- qom/object.c | 14 +++--- 2 files changed, 18 insertions(+), 24 deletions(-) diff --git a/include/qom/object.h b/include/qom/object.h index b2d2558245..d74ede4eac 100644 --- a/include/qom/object.h +++ b/include/qom/object.h @@ -729,15 +729,13 @@ void object_apply_compat_props(Object *obj); * Error *err = NULL; * Object *obj = ...get / create object...; * - * obj = object_set_props(obj, - * &err, - * "share", "yes", - * "mem-path", "/dev/shm/somefile", - * "prealloc", "yes", - * "size", "1048576", - * NULL); - * - * if (!obj) { + * if (!object_set_props(obj, + * &err, + * "share", "yes", + * "mem-path", "/dev/shm/somefile", + * "prealloc", "yes", + * "size", "1048576", + * NULL)) { * error_reportf_err(err, "Cannot set properties: "); * } * @@ -746,11 +744,9 @@ void object_apply_compat_props(Object *obj); * The returned object will have one stable reference maintained * for as long as it is present in the object hierarchy. * - * Returns: -1 on error, 0 on success + * Returns: %true on success, %false on error. */ -int object_set_props(Object *obj, - Error **errp, - ...) QEMU_SENTINEL; +bool object_set_props(Object *obj, Error **errp, ...) QEMU_SENTINEL; /** * object_set_propv: @@ -760,11 +756,9 @@ int object_set_props(Object *obj, * * See object_set_props() for documentation. * - * Returns: -1 on error, 0 on success + * Returns: %true on success, %false on error. */ -int object_set_propv(Object *obj, - Error **errp, - va_list vargs); +bool object_set_propv(Object *obj, Error **errp, va_list vargs); /** * object_initialize: diff --git a/qom/object.c b/qom/object.c index ebfe6e6c51..bd8c86ec6f 100644 --- a/qom/object.c +++ b/qom/object.c @@ -557,7 +557,7 @@ bool object_initialize_child_with_propsv(Object *parentobj, object_initialize(childobj, size, type); obj = OBJECT(childobj); -if (object_set_propv(obj, errp, vargs) < 0) { +if (!object_set_propv(obj, errp, vargs)) { goto out; } @@ -752,7 +752,7 @@ Object *object_new_with_propv(const char *typename, } obj = object_new_with_type(klass->type); -if (object_set_propv(obj, errp, vargs) < 0) { +if (!object_set_propv(obj, errp, vargs)) { goto error; } @@ -780,12 +780,12 @@ Object *object_new_with_propv(const char *typename, } -int object_set_props(Object *obj, +bool object_set_props(Object *obj, Error **errp, ...) { va_list vargs; -int ret; +bool ret; va_start(vargs, errp); ret = object_set_propv(obj, errp, vargs); @@ -795,7 +795,7 @@ int object_set_props(Object *obj, } -int object_set_propv(Object *obj, +bool object_set_propv(Object *obj, Error **errp, va_list vargs) { @@ -809,12 +809,12 @@ int object_set_propv(Object *obj, g_assert(value != NULL); if (!object_property_parse(obj, propname, value, &local_err)) { error_propagate(errp, local_err); -return -1; +return false; } propname = va_arg(vargs, char *); } -return 0; +return true; } -- 2.26.2
[PATCH v2 39/44] qapi: Smooth visitor error checking in generated code
Use visitor functions' return values to check for failure. Eliminate error_propagate() that are now unnecessary. Delete @err that are now unused. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- docs/devel/qapi-code-gen.txt | 60 ++-- scripts/qapi/commands.py | 22 ++--- scripts/qapi/visit.py| 57 ++ 3 files changed, 55 insertions(+), 84 deletions(-) diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt index 9bfc57063c..69eede6c28 100644 --- a/docs/devel/qapi-code-gen.txt +++ b/docs/devel/qapi-code-gen.txt @@ -1420,8 +1420,6 @@ Example: bool visit_type_UserDefOne_members(Visitor *v, UserDefOne *obj, Error **errp) { -Error *err = NULL; - if (!visit_type_int(v, "integer", &obj->integer, errp)) { return false; } @@ -1430,13 +1428,12 @@ Example: return false; } } -error_propagate(errp, err); -return !err; +return true; } bool visit_type_UserDefOne(Visitor *v, const char *name, UserDefOne **obj, Error **errp) { -Error *err = NULL; +bool ok = false; if (!visit_start_struct(v, name, (void **)obj, sizeof(UserDefOne), errp)) { return false; @@ -1446,24 +1443,22 @@ Example: assert(visit_is_dealloc(v)); goto out_obj; } -visit_type_UserDefOne_members(v, *obj, &err); -if (err) { +if (!visit_type_UserDefOne_members(v, *obj, errp)) { goto out_obj; } -visit_check_struct(v, &err); +ok = visit_check_struct(v, errp); out_obj: visit_end_struct(v, (void **)obj); -if (err && visit_is_input(v)) { +if (!ok && visit_is_input(v)) { qapi_free_UserDefOne(*obj); *obj = NULL; } -error_propagate(errp, err); -return !err; +return ok; } bool visit_type_UserDefOneList(Visitor *v, const char *name, UserDefOneList **obj, Error **errp) { -Error *err = NULL; +bool ok = false; UserDefOneList *tail; size_t size = sizeof(**obj); @@ -1473,33 +1468,27 @@ Example: for (tail = *obj; tail; tail = (UserDefOneList *)visit_next_list(v, (GenericList *)tail, size)) { -visit_type_UserDefOne(v, NULL, &tail->value, &err); -if (err) { -break; +if (!visit_type_UserDefOne(v, NULL, &tail->value, errp)) { +goto out_obj; } } -if (!err) { -visit_check_list(v, &err); -} +ok = visit_check_list(v, errp); +out_obj: visit_end_list(v, (void **)obj); -if (err && visit_is_input(v)) { +if (!ok && visit_is_input(v)) { qapi_free_UserDefOneList(*obj); *obj = NULL; } -error_propagate(errp, err); -return !err; +return ok; } bool visit_type_q_obj_my_command_arg_members(Visitor *v, q_obj_my_command_arg *obj, Error **errp) { -Error *err = NULL; - if (!visit_type_UserDefOneList(v, "arg1", &obj->arg1, errp)) { return false; } -error_propagate(errp, err); -return !err; +return true; } [Uninteresting stuff omitted...] @@ -1554,15 +1543,12 @@ Example: static void qmp_marshal_output_UserDefOne(UserDefOne *ret_in, QObject **ret_out, Error **errp) { -Error *err = NULL; Visitor *v; v = qobject_output_visitor_new(ret_out); -visit_type_UserDefOne(v, "unused", &ret_in, &err); -if (!err) { +if (visit_type_UserDefOne(v, "unused", &ret_in, errp)) { visit_complete(v, ret_out); } -error_propagate(errp, err); visit_free(v); v = qapi_dealloc_visitor_new(); visit_type_UserDefOne(v, "unused", &ret_in, NULL); @@ -1572,33 +1558,32 @@ Example: void qmp_marshal_my_command(QDict *args, QObject **ret, Error **errp) { Error *err = NULL; +bool ok = false; Visitor *v; UserDefOne *retval; q_obj_my_command_arg arg = {0}; v = qobject_input_visitor_new(QOBJECT(args)); -visit_start_struct(v, NULL, NULL, 0, &err); -if (err) { +if (!visit_start_struct(v, NULL, NULL, 0, errp)) { goto out; } -visit_type_q_obj_my_command_arg_members(v, &arg, &err); -if (!err) { -visit_check_struct(v, &err); +if (visit_type_q_obj_my_command_arg_members(v, &arg, errp)) { +ok = visit_check_struct(v, errp); } visit_end_struct(v, NULL); -if (err) { +if (!ok) { goto out; } retval = qmp_my_command(arg.arg1, &err); +error_propagate(errp, err);
[PATCH v2 38/44] qapi: Smooth another visitor error checking pattern
Convert visit_type_FOO(v, ..., &ptr, &err); ... if (err) { ... } to visit_type_FOO(v, ..., &ptr, errp); ... if (!ptr) { ... } for functions that set @ptr to non-null / null on success / error. Eliminate error_propagate() that are now unnecessary. Delete @err that are now unused. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- block/nfs.c | 7 ++- block/parallels.c| 6 ++ block/qcow.c | 6 ++ block/qcow2.c| 7 ++- block/qed.c | 6 ++ block/rbd.c | 7 ++- block/sheepdog.c | 6 ++ block/ssh.c | 6 ++ block/vdi.c | 7 ++- block/vhdx.c | 6 ++ block/vpc.c | 6 ++ hw/acpi/core.c | 4 ++-- hw/block/xen-block.c | 6 ++ hw/core/numa.c | 7 +++ monitor/monitor.c| 21 +++-- 15 files changed, 36 insertions(+), 72 deletions(-) diff --git a/block/nfs.c b/block/nfs.c index b1718d125a..61a249a9fc 100644 --- a/block/nfs.c +++ b/block/nfs.c @@ -563,18 +563,15 @@ static BlockdevOptionsNfs *nfs_options_qdict_to_qapi(QDict *options, BlockdevOptionsNfs *opts = NULL; Visitor *v; const QDictEntry *e; -Error *local_err = NULL; v = qobject_input_visitor_new_flat_confused(options, errp); if (!v) { return NULL; } -visit_type_BlockdevOptionsNfs(v, NULL, &opts, &local_err); +visit_type_BlockdevOptionsNfs(v, NULL, &opts, errp); visit_free(v); - -if (local_err) { -error_propagate(errp, local_err); +if (!opts) { return NULL; } diff --git a/block/parallels.c b/block/parallels.c index c259799111..f489c0d4ba 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -667,11 +667,9 @@ static int coroutine_fn parallels_co_create_opts(BlockDriver *drv, goto done; } -visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err); +visit_type_BlockdevCreateOptions(v, NULL, &create_options, errp); visit_free(v); - -if (local_err) { -error_propagate(errp, local_err); +if (!create_options) { ret = -EINVAL; goto done; } diff --git a/block/qcow.c b/block/qcow.c index 7913d12d50..c22d1bf6b8 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -994,11 +994,9 @@ static int coroutine_fn qcow_co_create_opts(BlockDriver *drv, goto fail; } -visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err); +visit_type_BlockdevCreateOptions(v, NULL, &create_options, errp); visit_free(v); - -if (local_err) { -error_propagate(errp, local_err); +if (!create_options) { ret = -EINVAL; goto fail; } diff --git a/block/qcow2.c b/block/qcow2.c index a9137a535b..9ed2396c88 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -3683,7 +3683,6 @@ static int coroutine_fn qcow2_co_create_opts(BlockDriver *drv, Visitor *v; BlockDriverState *bs = NULL; BlockDriverState *data_bs = NULL; -Error *local_err = NULL; const char *val; int ret; @@ -3779,11 +3778,9 @@ static int coroutine_fn qcow2_co_create_opts(BlockDriver *drv, goto finish; } -visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err); +visit_type_BlockdevCreateOptions(v, NULL, &create_options, errp); visit_free(v); - -if (local_err) { -error_propagate(errp, local_err); +if (!create_options) { ret = -EINVAL; goto finish; } diff --git a/block/qed.c b/block/qed.c index 3d5ddd86f2..7fa7f880f6 100644 --- a/block/qed.c +++ b/block/qed.c @@ -770,11 +770,9 @@ static int coroutine_fn bdrv_qed_co_create_opts(BlockDriver *drv, goto fail; } -visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err); +visit_type_BlockdevCreateOptions(v, NULL, &create_options, errp); visit_free(v); - -if (local_err) { -error_propagate(errp, local_err); +if (!create_options) { ret = -EINVAL; goto fail; } diff --git a/block/rbd.c b/block/rbd.c index 617553b022..688074c64b 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -681,7 +681,6 @@ static int qemu_rbd_convert_options(QDict *options, BlockdevOptionsRbd **opts, Error **errp) { Visitor *v; -Error *local_err = NULL; /* Convert the remaining options into a QAPI object */ v = qobject_input_visitor_new_flat_confused(options, errp); @@ -689,11 +688,9 @@ static int qemu_rbd_convert_options(QDict *options, BlockdevOptionsRbd **opts, return -EINVAL; } -visit_type_BlockdevOptionsRbd(v, NULL, opts, &local_err); +visit_type_BlockdevOptionsRbd(v, NULL, opts, errp); visit_free(v); - -if (local_err) { -error_propagate(errp, local_err); +if (!opts) { return -EINVAL; } diff --git a/block/sheepdog.c b/block/sheepdog.c inde
Re: [PATCH v2 03/44] qdev: Use returned bool to check for qdev_realize() etc. failure
On 7/2/20 10:49 AM, Markus Armbruster wrote: Convert foo(..., &err); if (err) { ... } to if (!foo(..., &err)) { ... } for qdev_realize(), qdev_realize_and_unref(), qbus_realize() and their wrappers isa_realize_and_unref(), pci_realize_and_unref(), sysbus_realize(), sysbus_realize_and_unref(), usb_realize_and_unref(). Coccinelle script: @@ identifier fun = {isa_realize_and_unref, pci_realize_and_unref, qbus_realize, qdev_realize, qdev_realize_and_unref, sysbus_realize, sysbus_realize_and_unref, usb_realize_and_unref}; expression list args, args2; typedef Error; Error *err; @@ -fun(args, &err, args2); -if (err) +if (!fun(args, &err, args2)) { ... } Chokes on hw/arm/musicpal.c's lcd_refresh() with the unhelpful error message "no position information". Nothing to convert there; skipped. Fails to convert hw/arm/armsse.c, because Coccinelle gets confused by ARMSSE being used both as typedef and function-like macro there. Converted manually. A few line breaks tidied up manually. Signed-off-by: Markus Armbruster --- The conversion is quite straight-forward. The patch is big, but correct, and you documented where it is not mechanical. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
[PATCH v2 40/44] qapi: Purge error_propagate() from QAPI core
Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- qapi/qapi-visit-core.c | 40 +++- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index 5a9c47aabf..7e5f40e7f0 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -39,19 +39,18 @@ void visit_free(Visitor *v) bool visit_start_struct(Visitor *v, const char *name, void **obj, size_t size, Error **errp) { -Error *err = NULL; +bool ok; trace_visit_start_struct(v, name, obj, size); if (obj) { assert(size); assert(!(v->type & VISITOR_OUTPUT) || *obj); } -v->start_struct(v, name, obj, size, &err); +ok = v->start_struct(v, name, obj, size, errp); if (obj && (v->type & VISITOR_INPUT)) { -assert(!err != !*obj); +assert(ok != !*obj); } -error_propagate(errp, err); -return !err; +return ok; } bool visit_check_struct(Visitor *v, Error **errp) @@ -69,16 +68,15 @@ void visit_end_struct(Visitor *v, void **obj) bool visit_start_list(Visitor *v, const char *name, GenericList **list, size_t size, Error **errp) { -Error *err = NULL; +bool ok; assert(!list || size >= sizeof(GenericList)); trace_visit_start_list(v, name, list, size); -v->start_list(v, name, list, size, &err); +ok = v->start_list(v, name, list, size, errp); if (list && (v->type & VISITOR_INPUT)) { -assert(!(err && *list)); +assert(ok || !*list); } -error_propagate(errp, err); -return !err; +return ok; } GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size) @@ -104,19 +102,20 @@ bool visit_start_alternate(Visitor *v, const char *name, GenericAlternate **obj, size_t size, Error **errp) { -Error *err = NULL; +bool ok; assert(obj && size >= sizeof(GenericAlternate)); assert(!(v->type & VISITOR_OUTPUT) || *obj); trace_visit_start_alternate(v, name, obj, size); -if (v->start_alternate) { -v->start_alternate(v, name, obj, size, &err); +if (!v->start_alternate) { +assert(!(v->type & VISITOR_INPUT)); +return true; } +ok = v->start_alternate(v, name, obj, size, errp); if (v->type & VISITOR_INPUT) { -assert(v->start_alternate && !err != !*obj); +assert(ok != !*obj); } -error_propagate(errp, err); -return !err; +return ok; } void visit_end_alternate(Visitor *v, void **obj) @@ -309,7 +308,7 @@ bool visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp) bool visit_type_str(Visitor *v, const char *name, char **obj, Error **errp) { -Error *err = NULL; +bool ok; assert(obj); /* TODO: Fix callers to not pass NULL when they mean "", so that we @@ -317,12 +316,11 @@ bool visit_type_str(Visitor *v, const char *name, char **obj, Error **errp) assert(!(v->type & VISITOR_OUTPUT) || *obj); */ trace_visit_type_str(v, name, obj); -v->type_str(v, name, obj, &err); +ok = v->type_str(v, name, obj, errp); if (v->type & VISITOR_INPUT) { -assert(!err != !*obj); +assert(ok != !*obj); } -error_propagate(errp, err); -return !err; +return ok; } bool visit_type_number(Visitor *v, const char *name, double *obj, -- 2.26.2
Re: [PATCH v2 13/44] qemu-option: Use returned bool to check for failure
On 7/2/20 10:49 AM, Markus Armbruster wrote: The previous commit enables conversion of foo(..., &err); if (err) { ... } to if (!foo(..., &err)) { ... } for QemuOpts functions that now return true / false on success / error. Coccinelle script: @@ identifier fun = {opts_do_parse, parse_option_bool, parse_option_number, parse_option_size, qemu_opt_parse, qemu_opt_rename, qemu_opt_set, qemu_opt_set_bool, qemu_opt_set_number, qemu_opts_absorb_qdict, qemu_opts_do_parse, qemu_opts_from_qdict_entry, qemu_opts_set, qemu_opts_validate}; expression list args, args2; typedef Error; Error *err; @@ -fun(args, &err, args2); -if (err) +if (!fun(args, &err, args2)) { ... } A few line breaks tidied up manually. Signed-off-by: Markus Armbruster --- Similar results to the script in 3/44. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 14/44] block: Avoid error accumulation in bdrv_img_create()
On 7/2/20 10:49 AM, Markus Armbruster wrote: When creating an image fails because the format doesn't support option "backing_file" or "backing_fmt", bdrv_img_create() first has qemu_opt_set() put a generic error into @local_err, then puts the real error into @errp with error_setg(), and then propagates the former to the latter, which throws away the generic error. A bit complicated, but works. Not that qemu_opt_set() returns a useful value, we can simply ignore s/Not/Now/ the generic error instead. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- block.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 00/44] Less clumsy error checking
Patchew URL: https://patchew.org/QEMU/20200702155000.3455325-1-arm...@redhat.com/ Hi, This series seems to have some coding style problems. See output below for more information: Subject: [PATCH v2 00/44] Less clumsy error checking Type: series Message-id: 20200702155000.3455325-1-arm...@redhat.com === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 581316d hmp: Ignore Error objects where the return value suffices 382f9cc qdev: Ignore Error objects where the return value suffices 2d54031 qemu-img: Ignore Error objects where the return value suffices c1da551 error: Avoid error_propagate() after migrate_add_blocker() 584033a qapi: Purge error_propagate() from QAPI core d5c18e7 qapi: Smooth visitor error checking in generated code a0b2eb7 qapi: Smooth another visitor error checking pattern 59fbe03 error: Reduce unnecessary error propagation d5401ed error: Eliminate error_propagate() manually 28da452 error: Eliminate error_propagate() with Coccinelle, part 2 249518d error: Eliminate error_propagate() with Coccinelle, part 1 79d26b3 error: Avoid unnecessary error_propagate() after error_setg() 69f21ee qdev: Use returned bool to check for failure, Coccinelle part cc10b4f qdev: Make functions taking Error ** return bool, not void 937e007 qom: Make functions taking Error ** return bool, not 0/-1 c13008d qom: Use returned bool to check for failure, manual part a76e6b8a qom: Use returned bool to check for failure, Coccinelle part 5831b19 qom: Make functions taking Error ** return bool, not void ee89136 qom: Put name parameter before value / visitor parameter cca92e7 qom: Use return values to check for error where that's simpler b3e4fbd qom: Don't handle impossible object_property_get_link() failure 64c3868 qom: Crash more nicely on object_property_get_link() failure 1045bbb qom: Rename qdev_get_type() to object_get_type() a90aa52 qom: Use error_reportf_err() instead of g_printerr() in examples 042c119 s390x/pci: Fix harmless mistake in zpci's property fid's setter 7556520 block/parallels: Simplify parallels_open() after previous commit 67c3627 qapi: Use returned bool to check for failure, manual part d461f88 qapi: Use returned bool to check for failure, Coccinelle part 61c3c84 qapi: Make visitor functions taking Error ** return bool, not void e11841d hmp: Eliminate a variable in hmp_migrate_set_parameter() b11147d block: Avoid error accumulation in bdrv_img_create() 5ddfdab qemu-option: Use returned bool to check for failure 132ed75 qemu-option: Make functions taking Error ** return bool, not void 48a09ea qemu-option: Replace opt_set() by cleaner opt_validate() 7ea945d qemu-option: Factor out helper opt_create() 378230d qemu-option: Simplify around find_default_by_name() 419b8e9 qemu-option: Factor out helper find_default_by_name() 066fe51 qemu-option: Make uses of find_desc_by_name() more similar 5ce56fe qemu-option: Check return value instead of @err where convenient 367ec25 virtio-crypto-pci: Tidy up virtio_crypto_pci_realize() 8df3b8f macio: Tidy up error handling in macio_newworld_realize() 0e00e46 qdev: Use returned bool to check for qdev_realize() etc. failure 15a41a5 error: Document Error API usage rules 6cc06a1 error: Improve examples in error.h's big comment === OUTPUT BEGIN === 1/44 Checking commit 6cc06a1b48e4 (error: Improve examples in error.h's big comment) ERROR: Error messages should not contain newlines #37: FILE: include/qapi/error.h:27: + * error_setg(errp, "invalid quark\n" // WRONG! total: 1 errors, 0 warnings, 42 lines checked Patch 1/44 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 2/44 Checking commit 15a41a5ba273 (error: Document Error API usage rules) 3/44 Checking commit 0e00e463e690 (qdev: Use returned bool to check for qdev_realize() etc. failure) 4/44 Checking commit 8df3b8f872d3 (macio: Tidy up error handling in macio_newworld_realize()) 5/44 Checking commit 367ec250efbc (virtio-crypto-pci: Tidy up virtio_crypto_pci_realize()) 6/44 Checking commit 5ce56fec2a32 (qemu-option: Check return value instead of @err where convenient) 7/44 Checking commit 066fe5182eff (qemu-option: Make uses of find_desc_by_name() more similar) 8/44 Checking commit 419b8e946ee0 (qemu-option: Factor out helper find_default_by_name()) 9/44 Checking commit 378230df0dc2 (qemu-option: Simplify around find_default_by_name()) 10/44 Checking commit 7ea945df03f9 (qemu-option: Factor out helper opt_create()) 11/44 Checking commit 48a09ea2ba72 (qemu-option: Replace opt_set() by cleaner opt_validate()) 12/44 Checking commit 132ed750d145 (qemu-option: Make functions taking Error ** return bool, not void) 13/44 Check
Re: [PATCH v2 00/44] Less clumsy error checking
Patchew URL: https://patchew.org/QEMU/20200702155000.3455325-1-arm...@redhat.com/ Hi, This series failed the docker-quick@centos7 build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash make docker-image-centos7 V=1 NETWORK=1 time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200702155000.3455325-1-arm...@redhat.com/testing.docker-quick@centos7/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH v2 16/44] qapi: Make visitor functions taking Error ** return bool, not void
On 7/2/20 10:49 AM, Markus Armbruster wrote: See recent commit "error: Document Error API usage rules" for rationale. Signed-off-by: Markus Armbruster --- Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 17/44] qapi: Use returned bool to check for failure, Coccinelle part
On 7/2/20 10:49 AM, Markus Armbruster wrote: The previous commit enables conversion of visit_foo(..., &err); if (err) { ... } to if (!visit_foo(..., errp)) { ... } for visitor functions that now return true / false on success / error. Coccinelle script: @@ identifier fun =~ "check_list|input_type_enum|lv_start_struct|lv_type_bool|lv_type_int64|lv_type_str|lv_type_uint64|output_type_enum|parse_type_bool|parse_type_int64|parse_type_null|parse_type_number|parse_type_size|parse_type_str|parse_type_uint64|print_type_bool|print_type_int64|print_type_null|print_type_number|print_type_size|print_type_str|print_type_uint64|qapi_clone_start_alternate|qapi_clone_start_list|qapi_clone_start_struct|qapi_clone_type_bool|qapi_clone_type_int64|qapi_clone_type_null|qapi_clone_type_number|qapi_clone_type_str|qapi_clone_type_uint64|qapi_dealloc_start_list|qapi_dealloc_start_struct|qapi_dealloc_type_anything|qapi_dealloc_type_bool|qapi_dealloc_type_int64|qapi_dealloc_type_null|qapi_dealloc_type_number|qapi_dealloc_type_str|qapi_dealloc_type_uint64|qobject_input_check_list|qobject_input_check_struct|qobject_input_start_alternate|qobject_input_start_list|qobject_input_start_struct|qobject_input_type_any|qobject_input_type_bool|qobject_input_type_bool_keyval|qobject_input_type_int64|qobject_input_type_int64_keyval|qobject_input_type_null|qobject_input_type_number|qobject_input_type_number_keyval|qobject_input_type_size_keyval|qobject_input_type_str|qobject_input_type_str_keyval|qobject_input_type_uint64|qobject_input_type_uint64_keyval|qobject_output_start_list|qobject_output_start_struct|qobject_output_type_any|qobject_output_type_bool|qobject_output_type_int64|qobject_output_type_null|qobject_output_type_number|qobject_output_type_str|qobject_output_type_uint64|start_list|visit_check_list|visit_check_struct|visit_start_alternate|visit_start_list|visit_start_struct|visit_type_.*"; Long line, but tolerable expression list args; typedef Error; Error *err; @@ -fun(args, &err); -if (err) +if (!fun(args, &err)) { ... } A few line breaks tidied up manually. Signed-off-by: Markus Armbruster --- Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 23/44] qom: Crash more nicely on object_property_get_link() failure
On 7/2/20 10:49 AM, Markus Armbruster wrote: Pass &error_abort instead of NULL where the returned value is dereferenced or asserted to be non-null. Drop a now redundant assertion. Signed-off-by: Markus Armbruster --- hw/core/platform-bus.c | 6 +++--- Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 28/44] qom: Use returned bool to check for failure, Coccinelle part
On 7/2/20 10:49 AM, Markus Armbruster wrote: The previous commit enables conversion of foo(..., &err); if (err) { ... } to if (!foo(..., errp)) { ... } for QOM functions that now return true / false on success / error. Coccinelle script: @@ identifier fun = {object_apply_global_props, object_initialize_child_with_props, object_initialize_child_with_propsv, object_property_get, object_property_get_bool, object_property_parse, object_property_set, object_property_set_bool, object_property_set_int, object_property_set_link, object_property_set_qobject, object_property_set_str, object_property_set_uint, object_set_props, object_set_propv, user_creatable_add_dict, user_creatable_complete, user_creatable_del}; expression list args, args2; typedef Error; Error *err; @@ -fun(args, &err, args2); -if (err) +if (!fun(args, &err, args2)) { ... } Fails to convert hw/arm/armsse.c, because Coccinelle gets confused by ARMSSE being used both as typedef and function-like macro there. Convert manually. Line breaks tidied up manually. Signed-off-by: Markus Armbruster --- Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 29/44] qom: Use returned bool to check for failure, manual part
On 7/2/20 10:49 AM, Markus Armbruster wrote: The previous commit used Coccinelle to convert from checking the Error object to checking the return value. Convert a few more manually. Signed-off-by: Markus Armbruster --- hw/core/bus.c | 6 +- hw/core/qdev.c | 7 +-- hw/s390x/s390-virtio-ccw.c | 13 +++-- 3 files changed, 9 insertions(+), 17 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 32/44] qdev: Use returned bool to check for failure, Coccinelle part
On 7/2/20 10:49 AM, Markus Armbruster wrote: The previous commit enables conversion of qdev_prop_set_drive_err(..., &err); if (err) { ... } to if (!qdev_prop_set_drive_err(..., errp)) { ... } Coccinelle script: Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 33/44] error: Avoid unnecessary error_propagate() after error_setg()
On 7/2/20 10:49 AM, Markus Armbruster wrote: Replace error_setg(&err, ...); error_propagate(errp, err); by error_setg(errp, ...); Candidates for conversion tracked down with this Coccinelle script: @@ identifier err, errp; expression list args; @@ -error_setg(&err, args); +error_setg(errp, args); ... when != err error_propagate(errp, err); Signed-off-by: Markus Armbruster --- +++ b/backends/cryptodev.c @@ -158,16 +158,15 @@ cryptodev_backend_set_queues(Object *obj, Visitor *v, const char *name, uint32_t value; if (!visit_type_uint32(v, name, &value, &local_err)) { -goto out; +error_propagate(errp, local_err); +return; } Looks like this error_propgate is spurious if you just use if (!visit_type_uint32(..., errp)) { Oh - that's not the pattern you flagged, and a later patch then addresses it. It might help if the commit message for this patch mentions that further cleanups are still forthcoming. +++ b/backends/hostmem-file.c @@ -114,18 +114,16 @@ static void file_memory_backend_set_align(Object *o, Visitor *v, uint64_t val; if (host_memory_backend_mr_inited(backend)) { -error_setg(&local_err, "cannot change property '%s' of %s", - name, object_get_typename(o)); -goto out; +error_setg(errp, "cannot change property '%s' of %s", name, + object_get_typename(o)); +return; } if (!visit_type_size(v, name, &val, &local_err)) { -goto out; +error_propagate(errp, local_err); +return; } Another case where the first 'if' matches the subject of this patch, and the second 'if' can avoid local_err but that the change will be done in a later patch. And several more later on, but this is how far it took me to realize that you intentionally saved them for later. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v4 2/2] nvme: allow cmb and pmr to be enabled on same device
On Jul 2 08:07, Andrzej Jakowski wrote: > On 7/2/20 3:31 AM, Klaus Jensen wrote: > > Aight, an update here. This only happens when QEMU is run with a virtual > > IOMMU. Otherwise, the kernel is happy. > > > > With the vIOMMU, qemu also craps out a bit: > > > > qemu-system-x86_64: vtd_iova_to_slpte: detected slpte permission error > > (iova=0xfd20, level=0x2, slpte=0x0, write=0) > > qemu-system-x86_64: vtd_iommu_translate: detected translation failure > > (dev=03:00:00, iova=0xfd20) > > > > So I think we are back in QEMU land for the bug. > > Can you share command line for that? > > qemu-system-x86_64 \ -nodefaults \ -display none \ -device intel-iommu,pt,intremap=on,device-iotlb=on \ -machine type=q35,accel=kvm,kernel_irqchip=split \ -cpu host \ -smp 4 \ -m 8G \ -nic user,model=virtio-net-pci,hostfwd=tcp::-:22 \ -device virtio-rng-pci \ -drive id=boot,file=/home/kbj/work/src/vmctl/state/pmr/boot.qcow2,format=qcow2,if=virtio,discard=on,detect-zeroes=unmap \ -device pcie-root-port,id=pcie_root_port1,chassis=1,slot=0 \ -device x3130-upstream,id=pcie_upstream1,bus=pcie_root_port1 \ -device xio3130-downstream,id=pcie_downstream1,bus=pcie_upstream1,chassis=1,slot=1 \ -drive id=nvme0n1,file=/home/kbj/work/src/vmctl/state/pmr/nvme0n1.img,format=raw,if=none,discard=on,detect-zeroes=unmap \ -object memory-backend-file,id=pmr,share=on,mem-path=pmr.bin,size=1M \ -device nvme,id=nvme0,serial=deadbeef,bus=pcie_downstream1,drive=nvme0n1,msix_qsize=1,pmrdev=pmr,cmb_size_mb=2 \ -pidfile /home/kbj/work/src/vmctl/run/pmr/pidfile \ -kernel /home/kbj/work/src/kernel/linux/arch/x86_64/boot/bzImage \ -append root=/dev/vda1 console=ttyS0,115200 audit=0 nokaslr \ -virtfs local,path=/home/kbj/work/src/kernel/linux,security_model=none,readonly,mount_tag=modules \ -serial mon:stdio \ -trace pci_nvme*
[PATCH 6/6] migration: support picking vmstate disk in QMP snapshot commands
This wires up support for a new "vmstate" parameter to the QMP commands for snapshots (savevm, loadvm). This parameter accepts block driver state node name. One use case for this would be a VM using OVMF firmware where the variables store is the first snapshottable disk image. The vmstate snapshot usually wants to be stored in the primary root disk of the VM, not the firmeware varstore. Thus there needs to be a mechanism to override the default choice of disk. Signed-off-by: Daniel P. Berrangé --- include/migration/snapshot.h | 8 ++-- migration/savevm.c | 16 ++-- monitor/hmp-cmds.c | 6 -- qapi/migration.json | 6 ++ replay/replay-snapshot.c | 4 ++-- softmmu/vl.c | 2 +- 6 files changed, 29 insertions(+), 13 deletions(-) diff --git a/include/migration/snapshot.h b/include/migration/snapshot.h index dffb84dbe5..721147d3c1 100644 --- a/include/migration/snapshot.h +++ b/include/migration/snapshot.h @@ -17,7 +17,11 @@ #include "qapi/qapi-builtin-types.h" -int save_snapshot(const char *name, strList *exclude, Error **errp); -int load_snapshot(const char *name, strList *exclude, Error **errp); +int save_snapshot(const char *name, + const char *vmstate, strList *exclude, + Error **errp); +int load_snapshot(const char *name, + const char *vmstate, strList *exclude, + Error **errp); #endif diff --git a/migration/savevm.c b/migration/savevm.c index 4b040676f7..5fd593e475 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2624,7 +2624,8 @@ int qemu_load_device_state(QEMUFile *f) return 0; } -int save_snapshot(const char *name, strList *exclude, Error **errp) +int save_snapshot(const char *name, const char *vmstate, + strList *exclude, Error **errp) { BlockDriverState *bs, *bs1; QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1; @@ -2662,7 +2663,7 @@ int save_snapshot(const char *name, strList *exclude, Error **errp) } } -bs = bdrv_all_find_vmstate_bs(NULL, exclude, errp); +bs = bdrv_all_find_vmstate_bs(vmstate, exclude, errp); if (bs == NULL) { return ret; } @@ -2827,7 +2828,8 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp) migration_incoming_state_destroy(); } -int load_snapshot(const char *name, strList *exclude, Error **errp) +int load_snapshot(const char *name, const char *vmstate, + strList *exclude, Error **errp) { BlockDriverState *bs, *bs_vm_state; QEMUSnapshotInfo sn; @@ -2856,7 +2858,7 @@ int load_snapshot(const char *name, strList *exclude, Error **errp) return ret; } -bs_vm_state = bdrv_all_find_vmstate_bs(NULL, exclude, errp); +bs_vm_state = bdrv_all_find_vmstate_bs(vmstate, exclude, errp); if (!bs_vm_state) { return -ENOTSUP; } @@ -2943,13 +2945,15 @@ bool vmstate_check_only_migratable(const VMStateDescription *vmsd) } void qmp_savevm(const char *tag, +bool has_vmstate, const char *vmstate, bool has_exclude, strList *exclude, Error **errp) { -save_snapshot(tag, exclude, errp); +save_snapshot(tag, vmstate, exclude, errp); } void qmp_loadvm(const char *tag, +bool has_vmstate, const char *vmstate, bool has_exclude, strList *exclude, Error **errp) { @@ -2957,7 +2961,7 @@ void qmp_loadvm(const char *tag, vm_stop(RUN_STATE_RESTORE_VM); -if (load_snapshot(tag, exclude, errp) == 0 && saved_vm_running) { +if (load_snapshot(tag, vmstate, exclude, errp) == 0 && saved_vm_running) { vm_start(); } } diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index fcde649100..586676e179 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -1091,7 +1091,8 @@ void hmp_loadvm(Monitor *mon, const QDict *qdict) { Error *err = NULL; -qmp_loadvm(qdict_get_str(qdict, "name"), false, NULL, &err); +qmp_loadvm(qdict_get_str(qdict, "name"), + false, NULL, false, NULL, &err); hmp_handle_error(mon, err); } @@ -1099,7 +1100,8 @@ void hmp_savevm(Monitor *mon, const QDict *qdict) { Error *err = NULL; -qmp_savevm(qdict_get_try_str(qdict, "name"), false, NULL, &err); +qmp_savevm(qdict_get_try_str(qdict, "name"), + false, NULL, false, NULL, &err); hmp_handle_error(mon, err); } diff --git a/qapi/migration.json b/qapi/migration.json index 2388664077..91173f5b06 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -1630,6 +1630,7 @@ # @tag: name of the snapshot to create. If it already # exists it will be replaced. # @exclude: list of block device node names to exclude +# @vmstate: block device node name to save vmstate to # # Note that execution of the VM will be paused during the time # it takes to save the snapshot @@ -1641,6 +1642,7 @@
[PATCH 1/6] migration: improve error reporting of block driver state name
With blockdev, a BlockDriverState may not have an device name, so using a node name is required as an alternative. Signed-off-by: Daniel P. Berrangé --- migration/savevm.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/migration/savevm.c b/migration/savevm.c index b979ea6e7f..72dbad95ed 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2648,7 +2648,7 @@ int save_snapshot(const char *name, Error **errp) if (!bdrv_all_can_snapshot(&bs)) { error_setg(errp, "Device '%s' is writable but does not support " - "snapshots", bdrv_get_device_name(bs)); + "snapshots", bdrv_get_device_or_node_name(bs)); return ret; } @@ -2657,7 +2657,7 @@ int save_snapshot(const char *name, Error **errp) ret = bdrv_all_delete_snapshot(name, &bs1, errp); if (ret < 0) { error_prepend(errp, "Error while deleting snapshot on device " - "'%s': ", bdrv_get_device_name(bs1)); + "'%s': ", bdrv_get_device_or_node_name(bs1)); return ret; } } @@ -2728,7 +2728,7 @@ int save_snapshot(const char *name, Error **errp) ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, &bs); if (ret < 0) { error_setg(errp, "Error while creating snapshot on '%s'", - bdrv_get_device_name(bs)); + bdrv_get_device_or_node_name(bs)); goto the_end; } @@ -2846,14 +2846,14 @@ int load_snapshot(const char *name, Error **errp) if (!bdrv_all_can_snapshot(&bs)) { error_setg(errp, "Device '%s' is writable but does not support snapshots", - bdrv_get_device_name(bs)); + bdrv_get_device_or_node_name(bs)); return -ENOTSUP; } ret = bdrv_all_find_snapshot(name, &bs); if (ret < 0) { error_setg(errp, "Device '%s' does not have the requested snapshot '%s'", - bdrv_get_device_name(bs), name); + bdrv_get_device_or_node_name(bs), name); return ret; } @@ -2882,7 +2882,7 @@ int load_snapshot(const char *name, Error **errp) ret = bdrv_all_goto_snapshot(name, &bs, errp); if (ret < 0) { error_prepend(errp, "Could not load snapshot '%s' on '%s': ", - name, bdrv_get_device_name(bs)); + name, bdrv_get_device_or_node_name(bs)); goto err_drain; } -- 2.26.2
[PATCH 0/6] migration: bring savevm/loadvm/delvm over to QMP
When QMP was first introduced some 10+ years ago now, the snapshot related commands (savevm/loadvm/delvm) were not converted. This was primarily because their implementation causes blocking of the thread running the monitor commands. This was (and still is) considered undesirable behaviour both in HMP and QMP. In theory someone was supposed to fix this flaw at some point in the past 10 years and bring them into the QMP world. Sadly, thus far it hasn't happened as people always had more important things to work on. Enterprise apps were much more interested in external snapshots than internal snapshots as they have many more features. Meanwhile users still want to use internal snapshots as there is a certainly simplicity in having everything self-contained in one image, even though it has limitations. Thus the apps that end up executing the savevm/loadvm/delvm via the "human-monitor-command" QMP command. IOW, the problematic blocking behaviour that was one of the reasons for not having savevm/loadvm/delvm in QMP is experienced by applications regardless. By not portting the commands to QMP due to one design flaw, we've forced apps and users to suffer from other design flaws of HMP ( bad error reporting, strong type checking of args, no introspection) for an additional 10 years. This feels rather sub-optimal :-( In practice users don't appear to care strongly about the fact that these commands block the VM while they run. I might have seen one bug report about it, but it certainly isn't something that comes up as a frequent topic except among us QEMU maintainers. Users do care about having access to the snapshot feature. Where I am seeing frequent complaints is wrt the use of OVMF combined with snapshots which has some serious pain points. This is getting worse as the push to ditch legacy BIOS in favour of UEFI gain momentum both across OS vendors and mgmt apps. Solving it requires new parameters to the commands, but doing this in HMP is super unappealing. After 10 years, I think it is time for us to be a little pragmatic about our handling of snapshots commands. My desire is that libvirt should never use "human-monitor-command" under any circumstances, because of the inherant flaws in HMP as a protocol for machine consumption. If there are flaws in QMP commands that's fine. If we fix them in future, we can deprecate the current QMP commands and remove them not too long after, without being locked in forever. Thus in this series I'm proposing a direct 1-1 mapping of the existing HMP commands for savevm/loadvm/delvm into QMP as a first step. This does not solve the blocking thread problem, but it does eliminate the error reporting, type checking and introspection problems inherant to HMP. We're winning on 3 out of the 4 long term problems. If someone can suggest a easy way to fix the thread blocking problem too, I'd be interested to hear it. If it involves a major refactoring then I think user are better served by unlocking what look like easy wins today. With a QMP variant, we reasonably deal with the problems related to OVMF: - The logic to pick which disk to store the vmstate in is not satsifactory. The first block driver state cannot be assumed to be the root disk image, it might be OVMF varstore and we don't want to store vmstate in there. - The logic to decide which disks must be snapshotted is hardwired to all disks which are writable Again with OVMF there might be a writable varstore, but this can be raw rather than qcow2 format, and thus unable to be snapshotted. While users might wish to snapshot their varstore, in some/many/most cases it is entirely uneccessary. Users are blocked from snapshotting their VM though due to this varstore. These are solved by adding two parameters to the commands. The first is a block device node name that identifies the image to store vmstate in, and the second is a list of node names to exclude from snapshots. In the block code I've only dealt with node names for block devices, as IIUC, this is all that libvirt should need in the -blockdev world it now lives in. IOW, I've made not attempt to cope with people wanting to use these QMP commands in combination with -drive args. I've done some minimal work in libvirt to start to make use of the new commands to validate their functionality, but this isn't finished yet. My ultimate goal is to make the GNOME Boxes maintainer happy again by having internal snapshots work with OVMF: https://gitlab.gnome.org/GNOME/gnome-boxes/-/commit/c486da262f6566326fbcb5e= f45c5f64048f16a6e Daniel P. Berrang=C3=A9 (6): migration: improve error reporting of block driver state name migration: introduce savevm, loadvm, delvm QMP commands block: add ability to filter out blockdevs during snapshot block: allow specifying name of block device for vmstate storage migration: support excluding block devs in QMP snapshot commands migration: support picking vmstate disk in QMP snapshot
[PATCH 2/6] migration: introduce savevm, loadvm, delvm QMP commands
savevm, loadvm and delvm are some of the few commands that have never been converted to use QMP. The primary reason for this lack of conversion is that they block execution of the thread for as long as they run. Despite this downside, however, libvirt and applications using libvirt has used these commands for as long as QMP has existed, via the "human-monitor-command" passthrough command. IOW, while it is clearly desirable to be able to fix the blocking problem, this is not an immediate obstacle to real world usage. Meanwhile there is a need for other features which involve adding new parameters to the commands. This is possible with HMP passthrough, but it provides no reliable way for apps to introspect features, so using QAPI modelling is highly desirable. This patch thus introduces trival savevm, loadvm, delvm commands to QMP that are functionally identical to the HMP counterpart, including the blocking problem. Signed-off-by: Daniel P. Berrangé --- migration/savevm.c | 27 monitor/hmp-cmds.c | 18 ++- qapi/migration.json | 76 + 3 files changed, 106 insertions(+), 15 deletions(-) diff --git a/migration/savevm.c b/migration/savevm.c index 72dbad95ed..53586a6406 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2943,3 +2943,30 @@ bool vmstate_check_only_migratable(const VMStateDescription *vmsd) return !(vmsd && vmsd->unmigratable); } + +void qmp_savevm(const char *tag, Error **errp) +{ +save_snapshot(tag, errp); +} + +void qmp_loadvm(const char *tag, Error **errp) +{ +int saved_vm_running = runstate_is_running(); + +vm_stop(RUN_STATE_RESTORE_VM); + +if (load_snapshot(tag, errp) == 0 && saved_vm_running) { +vm_start(); +} +} + +void qmp_delvm(const char *tag, Error **errp) +{ +BlockDriverState *bs; + +if (bdrv_all_delete_snapshot(tag, &bs, errp) < 0) { +error_prepend(errp, + "deleting snapshot on device '%s': ", + bdrv_get_device_or_node_name(bs)); +} +} diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index 2b0b58a336..26a5a1a701 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -1089,15 +1089,9 @@ void hmp_balloon(Monitor *mon, const QDict *qdict) void hmp_loadvm(Monitor *mon, const QDict *qdict) { -int saved_vm_running = runstate_is_running(); -const char *name = qdict_get_str(qdict, "name"); Error *err = NULL; -vm_stop(RUN_STATE_RESTORE_VM); - -if (load_snapshot(name, &err) == 0 && saved_vm_running) { -vm_start(); -} +qmp_loadvm(qdict_get_str(qdict, "name"), &err); hmp_handle_error(mon, err); } @@ -1105,21 +1099,15 @@ void hmp_savevm(Monitor *mon, const QDict *qdict) { Error *err = NULL; -save_snapshot(qdict_get_try_str(qdict, "name"), &err); +qmp_savevm(qdict_get_try_str(qdict, "name"), &err); hmp_handle_error(mon, err); } void hmp_delvm(Monitor *mon, const QDict *qdict) { -BlockDriverState *bs; Error *err = NULL; -const char *name = qdict_get_str(qdict, "name"); -if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) { -error_prepend(&err, - "deleting snapshot on device '%s': ", - bdrv_get_device_name(bs)); -} +qmp_delvm(qdict_get_str(qdict, "name"), &err); hmp_handle_error(mon, err); } diff --git a/qapi/migration.json b/qapi/migration.json index d5000558c6..849de38fb0 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -1621,3 +1621,79 @@ ## { 'event': 'UNPLUG_PRIMARY', 'data': { 'device-id': 'str' } } + +## +# @savevm: +# +# Save a VM snapshot +# +# @tag: name of the snapshot to create. If it already +# exists it will be replaced. +# +# Note that execution of the VM will be paused during the time +# it takes to save the snapshot +# +# Returns: nothing +# +# Example: +# +# -> { "execute": "savevm", +# "data": { +# "tag": "my-snap" +# } +#} +# <- { "return": { } } +# +# Since: 5.2 +## +{ 'command': 'savevm', + 'data': { 'tag': 'str' } } + +## +# @loadvm: +# +# Load a VM snapshot +# +# @tag: name of the snapshot to load. +# +# Returns: nothing +# +# Example: +# +# -> { "execute": "loadvm", +# "data": { +# "tag": "my-snap" +# } +#} +# <- { "return": { } } +# +# Since: 5.2 +## +{ 'command': 'loadvm', + 'data': { 'tag': 'str' } } + +## +# @delvm: +# +# Delete a VM snapshot +# +# @tag: name of the snapshot to delete. +# +# Note that execution of the VM will be paused during the time +# it takes to delete the snapshot +# +# Returns: nothing +# +# Example: +# +# -> { "execute": "delvm", +# "data": { +# "tag": "my-snap" +# } +#} +# <- { "return": { } } +# +# Since: 5.2 +## +{ 'command': 'delvm', + 'data': { 'tag': 'str' } } -- 2.26.2
[PATCH 5/6] migration: support excluding block devs in QMP snapshot commands
This wires up support for a new "exclude" parameter to the QMP commands for snapshots (savevm, loadvm, delvm). This parameter accepts a list of block driver state node names. One use case for this would be a VM using OVMF firmware where the variables store is a raw disk image. Ideally the variable store would be qcow2, allowing its contents to be included in the snapshot, but typically today the variable store is raw. It is still useful to be able to snapshot VMs using OVMF, even if the varstore is excluded, as the main OS disk content is usually the stuff the user cares about. Signed-off-by: Daniel P. Berrangé --- include/migration/snapshot.h | 6 -- migration/savevm.c | 38 +--- monitor/hmp-cmds.c | 6 +++--- qapi/migration.json | 21 ++-- replay/replay-snapshot.c | 4 ++-- softmmu/vl.c | 2 +- 6 files changed, 47 insertions(+), 30 deletions(-) diff --git a/include/migration/snapshot.h b/include/migration/snapshot.h index c85b6ec75b..dffb84dbe5 100644 --- a/include/migration/snapshot.h +++ b/include/migration/snapshot.h @@ -15,7 +15,9 @@ #ifndef QEMU_MIGRATION_SNAPSHOT_H #define QEMU_MIGRATION_SNAPSHOT_H -int save_snapshot(const char *name, Error **errp); -int load_snapshot(const char *name, Error **errp); +#include "qapi/qapi-builtin-types.h" + +int save_snapshot(const char *name, strList *exclude, Error **errp); +int load_snapshot(const char *name, strList *exclude, Error **errp); #endif diff --git a/migration/savevm.c b/migration/savevm.c index b11c6a882d..4b040676f7 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2624,7 +2624,7 @@ int qemu_load_device_state(QEMUFile *f) return 0; } -int save_snapshot(const char *name, Error **errp) +int save_snapshot(const char *name, strList *exclude, Error **errp) { BlockDriverState *bs, *bs1; QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1; @@ -2646,7 +2646,7 @@ int save_snapshot(const char *name, Error **errp) return ret; } -if (!bdrv_all_can_snapshot(NULL, &bs)) { +if (!bdrv_all_can_snapshot(exclude, &bs)) { error_setg(errp, "Device '%s' is writable but does not support " "snapshots", bdrv_get_device_or_node_name(bs)); return ret; @@ -2654,7 +2654,7 @@ int save_snapshot(const char *name, Error **errp) /* Delete old snapshots of the same name */ if (name) { -ret = bdrv_all_delete_snapshot(name, NULL, &bs1, errp); +ret = bdrv_all_delete_snapshot(name, exclude, &bs1, errp); if (ret < 0) { error_prepend(errp, "Error while deleting snapshot on device " "'%s': ", bdrv_get_device_or_node_name(bs1)); @@ -2662,7 +2662,7 @@ int save_snapshot(const char *name, Error **errp) } } -bs = bdrv_all_find_vmstate_bs(NULL, NULL, errp); +bs = bdrv_all_find_vmstate_bs(NULL, exclude, errp); if (bs == NULL) { return ret; } @@ -2724,7 +2724,7 @@ int save_snapshot(const char *name, Error **errp) aio_context_release(aio_context); aio_context = NULL; -ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, NULL, &bs); +ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, exclude, &bs); if (ret < 0) { error_setg(errp, "Error while creating snapshot on '%s'", bdrv_get_device_or_node_name(bs)); @@ -2827,7 +2827,7 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp) migration_incoming_state_destroy(); } -int load_snapshot(const char *name, Error **errp) +int load_snapshot(const char *name, strList *exclude, Error **errp) { BlockDriverState *bs, *bs_vm_state; QEMUSnapshotInfo sn; @@ -2842,13 +2842,13 @@ int load_snapshot(const char *name, Error **errp) return -EINVAL; } -if (!bdrv_all_can_snapshot(NULL, &bs)) { +if (!bdrv_all_can_snapshot(exclude, &bs)) { error_setg(errp, "Device '%s' is writable but does not support snapshots", bdrv_get_device_or_node_name(bs)); return -ENOTSUP; } -ret = bdrv_all_find_snapshot(name, NULL, &bs); +ret = bdrv_all_find_snapshot(name, exclude, &bs); if (ret < 0) { error_setg(errp, "Device '%s' does not have the requested snapshot '%s'", @@ -2856,7 +2856,7 @@ int load_snapshot(const char *name, Error **errp) return ret; } -bs_vm_state = bdrv_all_find_vmstate_bs(NULL, NULL, errp); +bs_vm_state = bdrv_all_find_vmstate_bs(NULL, exclude, errp); if (!bs_vm_state) { return -ENOTSUP; } @@ -2877,7 +2877,7 @@ int load_snapshot(const char *name, Error **errp) /* Flush all IO requests so they don't interfere with the new state. */ bdrv_drain_all_begin(); -ret = bdrv_all_goto_snapshot(name, NULL, &bs, errp); +ret = bdrv_all_goto_snapshot(name, exclude, &bs, err
Re: [PATCH v2 34/44] error: Eliminate error_propagate() with Coccinelle, part 1
On 7/2/20 10:49 AM, Markus Armbruster wrote: When all we do with an Error we receive into a local variable is propagating to somewhere else, we can just as well receive it there right away. Convert if (!foo(..., &err)) { ... error_propagate(errp, err); ... return ... } to if (!foo(..., errp)) { ... ... return ... } Not exactly elegant, I'm afraid. True, but it is still reasonable enough to use. The "when != goto lbl;" is necessary to avoid transforming spelled "when != lbl:" above in rule1 and rule2 if (fun(args, &err)) { goto out } ... out: error_propagate(errp, err); even though other paths to label out still need the error_propagate(). For an actual example, see sclp_realize(). Without the "when strict", Coccinelle transforms vfio_msix_setup(), incorrectly. I don't know what exactly "when strict" does, only that it helps here. The match of return is narrower than what I want, but I can't figure out how to express "return where the operand doesn't use @err". For an example where it's too narrow, see vfio_intx_enable(). Silently fails to convert hw/arm/armsse.c, because Coccinelle gets confused by ARMSSE being used both as typedef and function-like macro there. Converted manually. Line breaks tidied up manually. One nested declaration of @local_err deleted manually. Preexisting unwanted blank line dropped in hw/riscv/sifive_e.c. Signed-off-by: Markus Armbruster --- 114 files changed, 376 insertions(+), 884 deletions(-) Big, but sane. Picks up the spots I noticed in 33/44, and then some ;) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
[PATCH 3/6] block: add ability to filter out blockdevs during snapshot
When executing snapshot logic, currently blockdevs are filtered out if they are read-only or if there is no media present. In order to support snapshotting when UEFI firmware is in use, we need the ability to filter out the blockdev used for the firmware vars store. This can be achieved by having a list of node names that should be excluded from consideration for snapshots. Signed-off-by: Daniel P. Berrangé --- block/monitor/block-hmp-cmds.c | 4 ++-- block/snapshot.c | 41 ++ include/block/snapshot.h | 19 +--- migration/savevm.c | 18 +++ 4 files changed, 49 insertions(+), 33 deletions(-) diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c index 4c8c375172..0ee6e7a4be 100644 --- a/block/monitor/block-hmp-cmds.c +++ b/block/monitor/block-hmp-cmds.c @@ -899,7 +899,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict) ImageEntry *image_entry, *next_ie; SnapshotEntry *snapshot_entry; -bs = bdrv_all_find_vmstate_bs(); +bs = bdrv_all_find_vmstate_bs(NULL); if (!bs) { monitor_printf(mon, "No available block device supports snapshots\n"); return; @@ -951,7 +951,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict) total = 0; for (i = 0; i < nb_sns; i++) { SnapshotEntry *next_sn; -if (bdrv_all_find_snapshot(sn_tab[i].name, &bs1) == 0) { +if (bdrv_all_find_snapshot(sn_tab[i].name, NULL, &bs1) == 0) { global_snapshots[total] = i; total++; QTAILQ_FOREACH(image_entry, &image_list, next) { diff --git a/block/snapshot.c b/block/snapshot.c index bd9fb01817..c8950b0b86 100644 --- a/block/snapshot.c +++ b/block/snapshot.c @@ -385,12 +385,22 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs, return ret; } -static bool bdrv_all_snapshots_includes_bs(BlockDriverState *bs) +static bool bdrv_all_snapshots_includes_bs(BlockDriverState *bs, + strList *exclude_bs) { +const char *node_name = bdrv_get_node_name(bs); + if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) { return false; } +while (exclude_bs) { +if (g_str_equal(node_name, exclude_bs->value)) { +return false; +} +exclude_bs = exclude_bs->next; +} + /* Include all nodes that are either in use by a BlockBackend, or that * aren't attached to any node, but owned by the monitor. */ return bdrv_has_blk(bs) || QLIST_EMPTY(&bs->parents); @@ -400,7 +410,7 @@ static bool bdrv_all_snapshots_includes_bs(BlockDriverState *bs) * These functions will properly handle dataplane (take aio_context_acquire * when appropriate for appropriate block drivers) */ -bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs) +bool bdrv_all_can_snapshot(strList *exclude_bs, BlockDriverState **first_bad_bs) { bool ok = true; BlockDriverState *bs; @@ -410,7 +420,7 @@ bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs) AioContext *ctx = bdrv_get_aio_context(bs); aio_context_acquire(ctx); -if (bdrv_all_snapshots_includes_bs(bs)) { +if (bdrv_all_snapshots_includes_bs(bs, exclude_bs)) { ok = bdrv_can_snapshot(bs); } aio_context_release(ctx); @@ -425,8 +435,8 @@ fail: return ok; } -int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs, - Error **errp) +int bdrv_all_delete_snapshot(const char *name, strList *exclude_bs, + BlockDriverState **first_bad_bs, Error **errp) { int ret = 0; BlockDriverState *bs; @@ -437,7 +447,7 @@ int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs, AioContext *ctx = bdrv_get_aio_context(bs); aio_context_acquire(ctx); -if (bdrv_all_snapshots_includes_bs(bs) && +if (bdrv_all_snapshots_includes_bs(bs, exclude_bs) && bdrv_snapshot_find(bs, snapshot, name) >= 0) { ret = bdrv_snapshot_delete(bs, snapshot->id_str, @@ -456,8 +466,8 @@ fail: } -int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs, - Error **errp) +int bdrv_all_goto_snapshot(const char *name, strList *exclude_bs, + BlockDriverState **first_bad_bs, Error **errp) { int ret = 0; BlockDriverState *bs; @@ -467,7 +477,7 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs, AioContext *ctx = bdrv_get_aio_context(bs); aio_context_acquire(ctx); -if (bdrv_all_snapshots_includes_bs(bs)) { +if (bdrv_all_snapshots_includes_bs(bs, exclude_bs)) { ret = bdrv_snapshot_goto(bs, name, errp); } aio_context_release(ctx); @@ -482,7 +492,8 @@ fail: return ret; } -int
[PATCH 4/6] block: allow specifying name of block device for vmstate storage
Currently the vmstate will be stored in the first block device that supports snapshots. Historically this would have usually been the root device, but with UEFI it might be the variable store. There needs to be a way to override the choice of block device to store the state in. Signed-off-by: Daniel P. Berrangé --- block/monitor/block-hmp-cmds.c | 2 +- block/snapshot.c | 33 + include/block/snapshot.h | 4 +++- migration/savevm.c | 6 ++ 4 files changed, 35 insertions(+), 10 deletions(-) diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c index 0ee6e7a4be..a698fac027 100644 --- a/block/monitor/block-hmp-cmds.c +++ b/block/monitor/block-hmp-cmds.c @@ -899,7 +899,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict) ImageEntry *image_entry, *next_ie; SnapshotEntry *snapshot_entry; -bs = bdrv_all_find_vmstate_bs(NULL); +bs = bdrv_all_find_vmstate_bs(NULL, NULL, NULL); if (!bs) { monitor_printf(mon, "No available block device supports snapshots\n"); return; diff --git a/block/snapshot.c b/block/snapshot.c index c8950b0b86..4774a1890d 100644 --- a/block/snapshot.c +++ b/block/snapshot.c @@ -552,7 +552,9 @@ fail: return err; } -BlockDriverState *bdrv_all_find_vmstate_bs(strList *exclude_bs) +BlockDriverState *bdrv_all_find_vmstate_bs(const char *vmstate_bs, + strList *exclude_bs, + Error **errp) { BlockDriverState *bs; BdrvNextIterator it; @@ -560,16 +562,39 @@ BlockDriverState *bdrv_all_find_vmstate_bs(strList *exclude_bs) for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) { AioContext *ctx = bdrv_get_aio_context(bs); bool found; +Error *err = NULL; aio_context_acquire(ctx); -found = bdrv_all_snapshots_includes_bs(bs, exclude_bs) && -bdrv_can_snapshot(bs); +if (vmstate_bs == NULL) { +found = bdrv_all_snapshots_includes_bs(bs, exclude_bs) && +bdrv_can_snapshot(bs); +} else { +if (g_str_equal(vmstate_bs, bdrv_get_node_name(bs))) { +found = bdrv_all_snapshots_includes_bs(bs, exclude_bs) && +bdrv_can_snapshot(bs); +if (!found) { +error_setg(&err, + "block device '%s' cannot accept snapshots", + vmstate_bs); +} +} else { +found = false; +} +} aio_context_release(ctx); -if (found) { +if (found || err) { bdrv_next_cleanup(&it); +if (err) { +error_propagate(errp, err); +return NULL; +} break; } } + +if (!bs) { +error_setg(errp, "No block device can accept snapshots"); +} return bs; } diff --git a/include/block/snapshot.h b/include/block/snapshot.h index f20986ca37..ff627df5cb 100644 --- a/include/block/snapshot.h +++ b/include/block/snapshot.h @@ -90,6 +90,8 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn, strList *exclude_bs, BlockDriverState **first_bad_bs); -BlockDriverState *bdrv_all_find_vmstate_bs(strList *exclude_bs); +BlockDriverState *bdrv_all_find_vmstate_bs(const char *vmstate_bs, + strList *exclude_bs, + Error **errp); #endif diff --git a/migration/savevm.c b/migration/savevm.c index 4251aa0dde..b11c6a882d 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2662,9 +2662,8 @@ int save_snapshot(const char *name, Error **errp) } } -bs = bdrv_all_find_vmstate_bs(NULL); +bs = bdrv_all_find_vmstate_bs(NULL, NULL, errp); if (bs == NULL) { -error_setg(errp, "No block device can accept snapshots"); return ret; } aio_context = bdrv_get_aio_context(bs); @@ -2857,9 +2856,8 @@ int load_snapshot(const char *name, Error **errp) return ret; } -bs_vm_state = bdrv_all_find_vmstate_bs(NULL); +bs_vm_state = bdrv_all_find_vmstate_bs(NULL, NULL, errp); if (!bs_vm_state) { -error_setg(errp, "No block device supports snapshots"); return -ENOTSUP; } aio_context = bdrv_get_aio_context(bs_vm_state); -- 2.26.2