From 8c04093c8ce13fb67122b4ecedaa9857dc8ada98 Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Wed, 7 Nov 2018 11:09:59 -0200 Subject: block/snapshot: remove bdrv_snapshot_delete_by_id_or_name After the previous patch, the only instance of this function left is inside qemu-img.c. qemu-img is using it inside the 'img_snapshot' function to delete snapshots in the SNAPSHOT_DELETE case, based on a "snapshot_name" string that refers to the tag, not ID, of the QEMUSnapshotInfo struct. This can be verified by checking the SNAPSHOT_CREATE case that comes shortly before SNAPSHOT_DELETE. In that case, the same "snapshot_name" variable is being strcpy to the 'name' field of the QEMUSnapshotInfo struct sn: pstrcpy(sn.name, sizeof(sn.name), snapshot_name); Based on that, it is unlikely that "snapshot_name" might contain an "id" in SNAPSHOT_DELETE. This patch changes SNAPSHOT_DELETE to use snapshot_find() and snapshot_delete() instead of bdrv_snapshot_delete_by_id_or_name. After that, there is no instances left of bdrv_snapshot_delete_by_id_or_name in the code, so it is safe to remove it entirely. Suggested-by: Murilo Opsfelder Araujo Signed-off-by: Daniel Henrique Barboza Signed-off-by: Kevin Wolf --- include/block/snapshot.h | 3 --- 1 file changed, 3 deletions(-) (limited to 'include') diff --git a/include/block/snapshot.h b/include/block/snapshot.h index f73d1094af..b5d5084a12 100644 --- a/include/block/snapshot.h +++ b/include/block/snapshot.h @@ -61,9 +61,6 @@ int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id, const char *name, Error **errp); -int bdrv_snapshot_delete_by_id_or_name(BlockDriverState *bs, - const char *id_or_name, - Error **errp); int bdrv_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_info); int bdrv_snapshot_load_tmp(BlockDriverState *bs, -- cgit v1.2.3-55-g7522 From c90e2a9cfd94bd02d92c53b97f04fd595001de7e Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 14 Feb 2019 18:42:44 +0100 Subject: block-backend: Make blk_inc/dec_in_flight public For some users of BlockBackends, just increasing the in_flight counter is easier than implementing separate handlers in BlockDevOps. Make the helper functions for this public. Signed-off-by: Kevin Wolf --- block/block-backend.c | 4 ++-- include/sysemu/block-backend.h | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) (limited to 'include') diff --git a/block/block-backend.c b/block/block-backend.c index f6ea824308..0219555f89 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -1262,12 +1262,12 @@ int blk_make_zero(BlockBackend *blk, BdrvRequestFlags flags) return bdrv_make_zero(blk->root, flags); } -static void blk_inc_in_flight(BlockBackend *blk) +void blk_inc_in_flight(BlockBackend *blk) { atomic_inc(&blk->in_flight); } -static void blk_dec_in_flight(BlockBackend *blk) +void blk_dec_in_flight(BlockBackend *blk) { atomic_dec(&blk->in_flight); aio_wait_kick(); diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h index 832a4bf168..e2066eb06b 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -156,6 +156,8 @@ int blk_co_pdiscard(BlockBackend *blk, int64_t offset, int bytes); int blk_co_flush(BlockBackend *blk); int blk_flush(BlockBackend *blk); int blk_commit_all(void); +void blk_inc_in_flight(BlockBackend *blk); +void blk_dec_in_flight(BlockBackend *blk); void blk_drain(BlockBackend *blk); void blk_drain_all(void); void blk_set_on_error(BlockBackend *blk, BlockdevOnError on_read_error, -- cgit v1.2.3-55-g7522 From 6886ceaf61c2399419258246a064485e9b1e51ac Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 18 Feb 2019 14:09:32 +0100 Subject: io: Make qio_channel_yield() interruptible Similar to how qemu_co_sleep_ns() allows preemption from an external coroutine entry, allow reentering qio_channel_yield() early. Signed-off-by: Kevin Wolf --- include/io/channel.h | 9 ++++++--- io/channel.c | 10 ++++++++++ 2 files changed, 16 insertions(+), 3 deletions(-) (limited to 'include') diff --git a/include/io/channel.h b/include/io/channel.h index da2f138200..59460cb1ec 100644 --- a/include/io/channel.h +++ b/include/io/channel.h @@ -739,10 +739,13 @@ void qio_channel_detach_aio_context(QIOChannel *ioc); * addition, no two coroutine can be waiting on the same condition * and channel at the same time. * - * This must only be called from coroutine context + * This must only be called from coroutine context. It is safe to + * reenter the coroutine externally while it is waiting; in this + * case the function will return even if @condition is not yet + * available. */ -void qio_channel_yield(QIOChannel *ioc, - GIOCondition condition); +void coroutine_fn qio_channel_yield(QIOChannel *ioc, + GIOCondition condition); /** * qio_channel_wait: diff --git a/io/channel.c b/io/channel.c index 8dd0684f5d..303376e08d 100644 --- a/io/channel.c +++ b/io/channel.c @@ -469,6 +469,16 @@ void coroutine_fn qio_channel_yield(QIOChannel *ioc, } qio_channel_set_aio_fd_handlers(ioc); qemu_coroutine_yield(); + + /* Allow interrupting the operation by reentering the coroutine other than + * through the aio_fd_handlers. */ + if (condition == G_IO_IN && ioc->read_coroutine) { + ioc->read_coroutine = NULL; + qio_channel_set_aio_fd_handlers(ioc); + } else if (condition == G_IO_OUT && ioc->write_coroutine) { + ioc->write_coroutine = NULL; + qio_channel_set_aio_fd_handlers(ioc); + } } -- cgit v1.2.3-55-g7522 From a7b78fc944b20b953d68426b7db2c81fc6a5b5af Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 18 Feb 2019 14:38:15 +0100 Subject: nbd: Move nbd_read_eof() to nbd/client.c The only caller of nbd_read_eof() is nbd_receive_reply(), so it doesn't have to live in the header file, but can move next to its caller. Also add the missing coroutine_fn to the function and its caller. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- include/block/nbd.h | 3 ++- nbd/client.c | 22 +++++++++++++++++++++- nbd/nbd-internal.h | 19 ------------------- 3 files changed, 23 insertions(+), 21 deletions(-) (limited to 'include') diff --git a/include/block/nbd.h b/include/block/nbd.h index 96cfb1d7d5..cad975e00c 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -300,7 +300,8 @@ int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds, int nbd_init(int fd, QIOChannelSocket *sioc, NBDExportInfo *info, Error **errp); int nbd_send_request(QIOChannel *ioc, NBDRequest *request); -int nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp); +int coroutine_fn nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, + Error **errp); int nbd_client(int fd); int nbd_disconnect(int fd); int nbd_errno_to_system_errno(int err); diff --git a/nbd/client.c b/nbd/client.c index 10a52ad7d0..28d174c0f3 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -1387,12 +1387,32 @@ static int nbd_receive_structured_reply_chunk(QIOChannel *ioc, return 0; } +/* nbd_read_eof + * Tries to read @size bytes from @ioc. + * Returns 1 on success + * 0 on eof, when no data was read (errp is not set) + * negative errno on failure (errp is set) + */ +static inline int coroutine_fn +nbd_read_eof(QIOChannel *ioc, void *buffer, size_t size, Error **errp) +{ + int ret; + + assert(size); + ret = qio_channel_read_all_eof(ioc, buffer, size, errp); + if (ret < 0) { + ret = -EIO; + } + return ret; +} + /* nbd_receive_reply * Returns 1 on success * 0 on eof, when no data was read (errp is not set) * negative errno on failure (errp is set) */ -int nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp) +int coroutine_fn nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, + Error **errp) { int ret; const char *type; diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h index 82aa221227..049f83df77 100644 --- a/nbd/nbd-internal.h +++ b/nbd/nbd-internal.h @@ -64,25 +64,6 @@ #define NBD_SET_TIMEOUT _IO(0xab, 9) #define NBD_SET_FLAGS _IO(0xab, 10) -/* nbd_read_eof - * Tries to read @size bytes from @ioc. - * Returns 1 on success - * 0 on eof, when no data was read (errp is not set) - * negative errno on failure (errp is set) - */ -static inline int nbd_read_eof(QIOChannel *ioc, void *buffer, size_t size, - Error **errp) -{ - int ret; - - assert(size); - ret = qio_channel_read_all_eof(ioc, buffer, size, errp); - if (ret < 0) { - ret = -EIO; - } - return ret; -} - /* nbd_write * Writes @size bytes to @ioc. Returns 0 on success. */ -- cgit v1.2.3-55-g7522 From d3bd5b90890f6715bcee38e00745112157dfbe59 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 18 Feb 2019 14:56:01 +0100 Subject: nbd: Use low-level QIOChannel API in nbd_read_eof() Instead of using the convenience wrapper qio_channel_read_all_eof(), use the lower level QIOChannel API. This means duplicating some code, but we'll need this because this coroutine yield is special: We want it to be interruptible so that nbd_client_attach_aio_context() can correctly reenter the coroutine. This moves the bdrv_dec/inc_in_flight() pair into nbd_read_eof(), so that connection_co will always sit in this exact qio_channel_yield() call when bdrv_drain() returns. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- block/nbd-client.c | 8 +------- include/block/nbd.h | 4 ++-- nbd/client.c | 46 +++++++++++++++++++++++++++++++++++++--------- 3 files changed, 40 insertions(+), 18 deletions(-) (limited to 'include') diff --git a/block/nbd-client.c b/block/nbd-client.c index 5ce4aa9520..60f38f0320 100644 --- a/block/nbd-client.c +++ b/block/nbd-client.c @@ -84,15 +84,9 @@ static coroutine_fn void nbd_connection_entry(void *opaque) * * Therefore we keep an additional in_flight reference all the time and * only drop it temporarily here. - * - * FIXME This is not safe because the QIOChannel could wake up the - * coroutine for a second time; it is not prepared for coroutine - * resumption from external code. */ - bdrv_dec_in_flight(s->bs); assert(s->reply.handle == 0); - ret = nbd_receive_reply(s->ioc, &s->reply, &local_err); - bdrv_inc_in_flight(s->bs); + ret = nbd_receive_reply(s->bs, s->ioc, &s->reply, &local_err); if (local_err) { trace_nbd_read_reply_entry_fail(ret, error_get_pretty(local_err)); diff --git a/include/block/nbd.h b/include/block/nbd.h index cad975e00c..c6ef1ef42e 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -300,8 +300,8 @@ int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds, int nbd_init(int fd, QIOChannelSocket *sioc, NBDExportInfo *info, Error **errp); int nbd_send_request(QIOChannel *ioc, NBDRequest *request); -int coroutine_fn nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, - Error **errp); +int coroutine_fn nbd_receive_reply(BlockDriverState *bs, QIOChannel *ioc, + NBDReply *reply, Error **errp); int nbd_client(int fd); int nbd_disconnect(int fd); int nbd_errno_to_system_errno(int err); diff --git a/nbd/client.c b/nbd/client.c index 28d174c0f3..de7da48246 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -1394,30 +1394,58 @@ static int nbd_receive_structured_reply_chunk(QIOChannel *ioc, * negative errno on failure (errp is set) */ static inline int coroutine_fn -nbd_read_eof(QIOChannel *ioc, void *buffer, size_t size, Error **errp) +nbd_read_eof(BlockDriverState *bs, QIOChannel *ioc, void *buffer, size_t size, + Error **errp) { - int ret; + bool partial = false; assert(size); - ret = qio_channel_read_all_eof(ioc, buffer, size, errp); - if (ret < 0) { - ret = -EIO; + while (size > 0) { + struct iovec iov = { .iov_base = buffer, .iov_len = size }; + ssize_t len; + + len = qio_channel_readv(ioc, &iov, 1, errp); + if (len == QIO_CHANNEL_ERR_BLOCK) { + bdrv_dec_in_flight(bs); + qio_channel_yield(ioc, G_IO_IN); + bdrv_inc_in_flight(bs); + continue; + } else if (len < 0) { + return -EIO; + } else if (len == 0) { + if (partial) { + error_setg(errp, + "Unexpected end-of-file before all bytes were read"); + return -EIO; + } else { + return 0; + } + } + + partial = true; + size -= len; + buffer = (uint8_t*) buffer + len; } - return ret; + return 1; } /* nbd_receive_reply + * + * Decreases bs->in_flight while waiting for a new reply. This yield is where + * we wait indefinitely and the coroutine must be able to be safely reentered + * for nbd_client_attach_aio_context(). + * * Returns 1 on success * 0 on eof, when no data was read (errp is not set) * negative errno on failure (errp is set) */ -int coroutine_fn nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, - Error **errp) +int coroutine_fn nbd_receive_reply(BlockDriverState *bs, QIOChannel *ioc, + NBDReply *reply, Error **errp) { int ret; const char *type; - ret = nbd_read_eof(ioc, &reply->magic, sizeof(reply->magic), errp); + ret = nbd_read_eof(bs, ioc, &reply->magic, sizeof(reply->magic), errp); if (ret <= 0) { return ret; } -- cgit v1.2.3-55-g7522 From f962e96150e9c6a41e26caeaf93a65ec5b755607 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Sat, 23 Feb 2019 22:20:40 +0300 Subject: block: fix bdrv_check_perm for non-tree subgraph bdrv_check_perm in it's recursion checks each node in context of new permissions for one parent, because of nature of DFS. It works well, while children subgraph of top-most updated node is a tree, i.e. it doesn't have any kind of loops. But if we have a loop (not oriented, of course), i.e. we have two different ways from top-node to some child-node, then bdrv_check_perm will do wrong thing: top | \ | | v v A B | | v v node It will once check new permissions of node in context of new A permissions and old B permissions and once visa-versa. It's a wrong way and may lead to corruption of permission system. We may start with no-permissions and all-shared for both A->node and B->node relations and finish up with non shared write permission for both ways. The following commit will add a test, which shows this bug. To fix this situation, let's really set BdrvChild permissions during bdrv_check_perm procedure. And we are happy here, as check-perm is already written in transaction manner, so we just need to restore backed-up permissions in _abort. Signed-off-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Kevin Wolf --- block.c | 27 ++++++++++++++++++++++++++- include/block/block_int.h | 5 +++++ 2 files changed, 31 insertions(+), 1 deletion(-) (limited to 'include') diff --git a/block.c b/block.c index bb4bf1237c..16d59e0b32 100644 --- a/block.c +++ b/block.c @@ -1954,13 +1954,32 @@ static int bdrv_child_check_perm(BdrvChild *c, BlockReopenQueue *q, ret = bdrv_check_update_perm(c->bs, q, perm, shared, ignore_children, errp); g_slist_free(ignore_children); - return ret; + if (ret < 0) { + return ret; + } + + if (!c->has_backup_perm) { + c->has_backup_perm = true; + c->backup_perm = c->perm; + c->backup_shared_perm = c->shared_perm; + } + /* + * Note: it's OK if c->has_backup_perm was already set, as we can find the + * same child twice during check_perm procedure + */ + + c->perm = perm; + c->shared_perm = shared; + + return 0; } static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared) { uint64_t cumulative_perms, cumulative_shared_perms; + c->has_backup_perm = false; + c->perm = perm; c->shared_perm = shared; @@ -1971,6 +1990,12 @@ static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared) static void bdrv_child_abort_perm_update(BdrvChild *c) { + if (c->has_backup_perm) { + c->perm = c->backup_perm; + c->shared_perm = c->backup_shared_perm; + c->has_backup_perm = false; + } + bdrv_abort_perm_update(c->bs); } diff --git a/include/block/block_int.h b/include/block/block_int.h index 0075bafd10..8437df85a2 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -662,6 +662,11 @@ struct BdrvChild { */ uint64_t shared_perm; + /* backup of permissions during permission update procedure */ + bool has_backup_perm; + uint64_t backup_perm; + uint64_t backup_shared_perm; + QLIST_ENTRY(BdrvChild) next; QLIST_ENTRY(BdrvChild) next_parent; }; -- cgit v1.2.3-55-g7522 From f30c66ba6e417a07e68ad6e0bc5da27561a3beea Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 1 Feb 2019 20:29:05 +0100 Subject: block: Use bdrv_refresh_filename() to pull Before this patch, bdrv_refresh_filename() is used in a pushing manner: Whenever the BDS graph is modified, the parents of the modified edges are supposed to be updated (recursively upwards). However, that is nonviable, considering that we want child changes not to concern parents. Also, in the long run we want a pull model anyway: Here, we would have a bdrv_filename() function which returns a BDS's filename, freshly constructed. This patch is an intermediate step. It adds bdrv_refresh_filename() calls before every place a BDS.filename value is used. The only exceptions are protocol drivers that use their own filename, which clearly would not profit from refreshing that filename before. Also, bdrv_get_encrypted_filename() is removed along the way (as a user of BDS.filename), since it is completely unused. In turn, all of the calls to bdrv_refresh_filename() before this patch are removed, because we no longer have to call this function on graph changes. Signed-off-by: Max Reitz Message-id: 20190201192935.18394-2-mreitz@redhat.com Reviewed-by: Eric Blake Signed-off-by: Max Reitz --- block.c | 31 +++++++++++++++---------------- block/qapi.c | 4 ++++ block/raw-format.c | 1 + block/replication.c | 2 -- block/vhdx-log.c | 1 + block/vmdk.c | 6 ++++++ blockdev.c | 8 ++++++++ include/block/block.h | 1 - qemu-img.c | 11 +++++++++-- 9 files changed, 44 insertions(+), 21 deletions(-) (limited to 'include') diff --git a/block.c b/block.c index 16d59e0b32..88aa687a68 100644 --- a/block.c +++ b/block.c @@ -323,8 +323,11 @@ void bdrv_get_full_backing_filename_from_filename(const char *backed, void bdrv_get_full_backing_filename(BlockDriverState *bs, char *dest, size_t sz, Error **errp) { - char *backed = bs->exact_filename[0] ? bs->exact_filename : bs->filename; + char *backed; + bdrv_refresh_filename(bs); + + backed = bs->exact_filename[0] ? bs->exact_filename : bs->filename; bdrv_get_full_backing_filename_from_filename(backed, bs->backing_file, dest, sz, errp); } @@ -1004,6 +1007,8 @@ static void bdrv_backing_attach(BdrvChild *c) "node is used as backing hd of '%s'", bdrv_get_device_or_node_name(parent)); + bdrv_refresh_filename(backing_hd); + parent->open_flags &= ~BDRV_O_NO_BACKING; pstrcpy(parent->backing_file, sizeof(parent->backing_file), backing_hd->filename); @@ -1413,6 +1418,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file, } if (file != NULL) { + bdrv_refresh_filename(blk_bs(file)); filename = blk_bs(file)->filename; } else { /* @@ -2334,8 +2340,6 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd, bdrv_unref(backing_hd); } - bdrv_refresh_filename(bs); - out: bdrv_refresh_limits(bs, NULL); } @@ -2864,8 +2868,6 @@ static BlockDriverState *bdrv_open_inherit(const char *filename, g_free(child_key_dot); } - bdrv_refresh_filename(bs); - /* Check if any unknown options were used */ if (qdict_size(options) != 0) { const QDictEntry *entry = qdict_first(options); @@ -3310,6 +3312,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, if (local_err != NULL) { error_propagate(errp, local_err); } else { + bdrv_refresh_filename(reopen_state->bs); error_setg(errp, "failed while preparing to reopen image '%s'", reopen_state->bs->filename); } @@ -3937,7 +3940,10 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base, /* success - we can delete the intermediate states, and link top->base */ /* TODO Check graph modification op blockers (BLK_PERM_GRAPH_MOD) once * we've figured out how they should work. */ - backing_file_str = backing_file_str ? backing_file_str : base->filename; + if (!backing_file_str) { + bdrv_refresh_filename(base); + backing_file_str = base->filename; + } QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) { /* Check whether we are allowed to switch c from top to base */ @@ -4485,16 +4491,6 @@ bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs) return bs->supported_zero_flags & BDRV_REQ_MAY_UNMAP; } -const char *bdrv_get_encrypted_filename(BlockDriverState *bs) -{ - if (bs->backing && bs->backing->bs->encrypted) - return bs->backing_file; - else if (bs->encrypted) - return bs->filename; - else - return NULL; -} - void bdrv_get_backing_filename(BlockDriverState *bs, char *filename, int filename_size) { @@ -4615,6 +4611,9 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs, is_protocol = path_has_protocol(backing_file); + /* This will recursively refresh everything in the backing chain */ + bdrv_refresh_filename(bs); + for (curr_bs = bs; curr_bs->backing; curr_bs = curr_bs->backing->bs) { /* If either of the filename paths is actually a protocol, then diff --git a/block/qapi.c b/block/qapi.c index 00291f9105..4623de1d7b 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -51,6 +51,8 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk, return NULL; } + bdrv_refresh_filename(bs); + info = g_malloc0(sizeof(*info)); info->file = g_strdup(bs->filename); info->ro = bs->read_only; @@ -264,6 +266,8 @@ void bdrv_query_image_info(BlockDriverState *bs, goto out; } + bdrv_refresh_filename(bs); + info = g_new0(ImageInfo, 1); info->filename = g_strdup(bs->filename); info->format = g_strdup(bdrv_get_format_name(bs)); diff --git a/block/raw-format.c b/block/raw-format.c index 6f6dc99b2c..d07bcdae62 100644 --- a/block/raw-format.c +++ b/block/raw-format.c @@ -436,6 +436,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags, bs->file->bs->supported_zero_flags); if (bs->probed && !bdrv_is_read_only(bs)) { + bdrv_refresh_filename(bs->file->bs); fprintf(stderr, "WARNING: Image format was not specified for '%s' and probing " "guessed raw.\n" diff --git a/block/replication.c b/block/replication.c index e70dd95001..9b332002ee 100644 --- a/block/replication.c +++ b/block/replication.c @@ -616,8 +616,6 @@ static void replication_done(void *opaque, int ret) if (ret == 0) { s->stage = BLOCK_REPLICATION_DONE; - /* refresh top bs's filename */ - bdrv_refresh_filename(bs); s->active_disk = NULL; s->secondary_disk = NULL; s->hidden_disk = NULL; diff --git a/block/vhdx-log.c b/block/vhdx-log.c index ecd64266c5..3149ff08d8 100644 --- a/block/vhdx-log.c +++ b/block/vhdx-log.c @@ -803,6 +803,7 @@ int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed, if (logs.valid) { if (bs->read_only) { + bdrv_refresh_filename(bs); ret = -EPERM; error_setg(errp, "VHDX image file '%s' opened read-only, but " diff --git a/block/vmdk.c b/block/vmdk.c index 096e8eb662..117dc6adfe 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -479,6 +479,7 @@ static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent, extent->l1_table, l1_size); if (ret < 0) { + bdrv_refresh_filename(extent->file->bs); error_setg_errno(errp, -ret, "Could not read l1 table from extent '%s'", extent->file->bs->filename); @@ -499,6 +500,7 @@ static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent, extent->l1_backup_table, l1_size); if (ret < 0) { + bdrv_refresh_filename(extent->file->bs); error_setg_errno(errp, -ret, "Could not read l1 backup table from extent '%s'", extent->file->bs->filename); @@ -530,6 +532,7 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs, ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header)); if (ret < 0) { + bdrv_refresh_filename(file->bs); error_setg_errno(errp, -ret, "Could not read header from file '%s'", file->bs->filename); @@ -607,6 +610,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header)); if (ret < 0) { + bdrv_refresh_filename(file->bs); error_setg_errno(errp, -ret, "Could not read header from file '%s'", file->bs->filename); @@ -861,6 +865,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs, if (!path_is_absolute(fname) && !path_has_protocol(fname) && !desc_file_path[0]) { + bdrv_refresh_filename(bs->file->bs); error_setg(errp, "Cannot use relative extent paths with VMDK " "descriptor file '%s'", bs->file->bs->filename); return -EINVAL; @@ -2470,6 +2475,7 @@ static ImageInfo *vmdk_get_extent_info(VmdkExtent *extent) { ImageInfo *info = g_new0(ImageInfo, 1); + bdrv_refresh_filename(extent->file->bs); *info = (ImageInfo){ .filename = g_strdup(extent->file->bs->filename), .format = g_strdup(extent->type), diff --git a/blockdev.c b/blockdev.c index 8714ad2702..7e6bf9955c 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1627,6 +1627,7 @@ static void external_snapshot_prepare(BlkActionState *common, error_setg_errno(errp, -size, "bdrv_getlength failed"); goto out; } + bdrv_refresh_filename(state->old_bs); bdrv_img_create(new_image_file, format, state->old_bs->filename, state->old_bs->drv->format_name, @@ -3230,6 +3231,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, goto out; } assert(bdrv_get_aio_context(base_bs) == aio_context); + bdrv_refresh_filename(base_bs); base_name = base_bs->filename; } @@ -3349,6 +3351,10 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device, goto out; } } else if (has_top && top) { + /* This strcmp() is just a shortcut, there is no need to + * refresh @bs's filename. If it mismatches, + * bdrv_find_backing_image() will do the refresh and may still + * return @bs. */ if (strcmp(bs->filename, top) != 0) { top_bs = bdrv_find_backing_image(bs, top); } @@ -3509,6 +3515,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn, if (backup->mode != NEW_IMAGE_MODE_EXISTING) { assert(backup->format); if (source) { + bdrv_refresh_filename(source); bdrv_img_create(backup->target, backup->format, source->filename, source->drv->format_name, NULL, size, flags, false, &local_err); @@ -3889,6 +3896,7 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp) break; case NEW_IMAGE_MODE_ABSOLUTE_PATHS: /* create new image with backing file */ + bdrv_refresh_filename(source); bdrv_img_create(arg->target, format, source->filename, source->drv->format_name, diff --git a/include/block/block.h b/include/block/block.h index 73357c6c25..aaae900925 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -485,7 +485,6 @@ void bdrv_round_to_clusters(BlockDriverState *bs, int64_t *cluster_offset, int64_t *cluster_bytes); -const char *bdrv_get_encrypted_filename(BlockDriverState *bs); void bdrv_get_backing_filename(BlockDriverState *bs, char *filename, int filename_size); void bdrv_get_full_backing_filename(BlockDriverState *bs, diff --git a/qemu-img.c b/qemu-img.c index eb5045c742..d2fc28c987 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -2790,6 +2790,7 @@ static int get_block_status(BlockDriverState *bs, int64_t offset, BlockDriverState *file; bool has_offset; int64_t map; + char *filename = NULL; /* As an optimization, we could cache the current range of unallocated * clusters in each file of the chain, and avoid querying the same @@ -2817,6 +2818,11 @@ static int get_block_status(BlockDriverState *bs, int64_t offset, has_offset = !!(ret & BDRV_BLOCK_OFFSET_VALID); + if (file && has_offset) { + bdrv_refresh_filename(file); + filename = file->filename; + } + *e = (MapEntry) { .start = offset, .length = bytes, @@ -2825,8 +2831,8 @@ static int get_block_status(BlockDriverState *bs, int64_t offset, .offset = map, .has_offset = has_offset, .depth = depth, - .has_filename = file && has_offset, - .filename = file && has_offset ? file->filename : NULL, + .has_filename = filename, + .filename = filename, }; return 0; @@ -3334,6 +3340,7 @@ static int img_rebase(int argc, char **argv) qdict_put_bool(options, BDRV_OPT_FORCE_SHARE, true); } + bdrv_refresh_filename(bs); overlay_filename = bs->exact_filename[0] ? bs->exact_filename : bs->filename; out_real_path = g_malloc(PATH_MAX); -- cgit v1.2.3-55-g7522 From 998c201923a5e082f362566d234dfd6057e4a19a Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 1 Feb 2019 20:29:08 +0100 Subject: block: Add BDS.auto_backing_file If the backing file is overridden, this most probably does change the guest-visible data of a BDS. Therefore, we will need to consider this in bdrv_refresh_filename(). To see whether it has been overridden, we might want to compare bs->backing_file and bs->backing->bs->filename. However, bs->backing_file is changed by bdrv_set_backing_hd() (which is just used to change the backing child at runtime, without modifying the image header), so bs->backing_file most of the time simply contains a copy of bs->backing->bs->filename anyway, so it is useless for such a comparison. This patch adds an auto_backing_file BDS field which contains the backing file path as indicated by the image header, which is not changed by bdrv_set_backing_hd(). Because of bdrv_refresh_filename() magic, however, a BDS's filename may differ from what has been specified during bdrv_open(). Then, the comparison between bs->auto_backing_file and bs->backing->bs->filename may fail even though bs->backing was opened from bs->auto_backing_file. To mitigate this, we can copy the real BDS's filename (after the whole bdrv_open() and bdrv_refresh_filename() process) into bs->auto_backing_file, if we know the former has been opened based on the latter. This is only possible if no options modifying the backing file's behavior have been specified, though. To simplify things, this patch only copies the filename from the backing file if no options have been specified for it at all. Furthermore, there are cases where an overlay is created by qemu which already contains a BDS's filename (e.g. in blockdev-snapshot-sync). We do not need to worry about updating the overlay's bs->auto_backing_file there, because we actually wrote a post-bdrv_refresh_filename() filename into the image header. So all in all, there will be false negatives where (as of a future patch) bdrv_refresh_filename() will assume that the backing file differs from what was specified in the image header, even though it really does not. However, these cases should be limited to where (1) the user actually did override something in the backing chain (e.g. by specifying options for the backing file), or (2) the user executed a QMP command to change some node's backing file (e.g. change-backing-file or block-commit with @backing-file given) where the given filename does not happen to coincide with qemu's idea of the backing BDS's filename. Then again, (1) really is limited to -drive. With -blockdev or blockdev-add, you have to adhere to the schema, so a user cannot give partial "unimportant" options (e.g. by just setting backing.node-name and leaving the rest to the image header). Therefore, trying to fix this would mean trying to fix something for -drive only. To improve on (2), we would need a full infrastructure to "canonicalize" an arbitrary filename (+ options), so it can be compared against another. That seems a bit over the top, considering that filenames nowadays are there mostly for the user's entertainment. Signed-off-by: Max Reitz Reviewed-by: Eric Blake Reviewed-by: Alberto Garcia Message-id: 20190201192935.18394-5-mreitz@redhat.com Signed-off-by: Max Reitz --- block.c | 19 +++++++++++++++++++ block/qcow.c | 7 +++++-- block/qcow2.c | 10 +++++++--- block/qed.c | 7 +++++-- block/vmdk.c | 6 ++++-- include/block/block_int.h | 4 ++++ 6 files changed, 44 insertions(+), 9 deletions(-) (limited to 'include') diff --git a/block.c b/block.c index 31e4664a42..21395b546c 100644 --- a/block.c +++ b/block.c @@ -2361,6 +2361,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options, char *bdref_key_dot; const char *reference = NULL; int ret = 0; + bool implicit_backing = false; BlockDriverState *backing_hd; QDict *options; QDict *tmp_parent_options = NULL; @@ -2396,6 +2397,16 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options, qobject_unref(options); goto free_exit; } else { + if (qdict_size(options) == 0) { + /* If the user specifies options that do not modify the + * backing file's behavior, we might still consider it the + * implicit backing file. But it's easier this way, and + * just specifying some of the backing BDS's options is + * only possible with -drive anyway (otherwise the QAPI + * schema forces the user to specify everything). */ + implicit_backing = !strcmp(bs->auto_backing_file, bs->backing_file); + } + bdrv_get_full_backing_filename(bs, backing_filename, PATH_MAX, &local_err); if (local_err) { @@ -2429,6 +2440,12 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options, } bdrv_set_aio_context(backing_hd, bdrv_get_aio_context(bs)); + if (implicit_backing) { + bdrv_refresh_filename(backing_hd); + pstrcpy(bs->auto_backing_file, sizeof(bs->auto_backing_file), + backing_hd->filename); + } + /* Hook up the backing file link; drop our reference, bs owns the * backing_hd reference now */ bdrv_set_backing_hd(bs, backing_hd, &local_err); @@ -3848,6 +3865,8 @@ int bdrv_change_backing_file(BlockDriverState *bs, if (ret == 0) { pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_file ?: ""); pstrcpy(bs->backing_format, sizeof(bs->backing_format), backing_fmt ?: ""); + pstrcpy(bs->auto_backing_file, sizeof(bs->auto_backing_file), + backing_file ?: ""); } return ret; } diff --git a/block/qcow.c b/block/qcow.c index 0a235bf393..d47515d3df 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -31,6 +31,7 @@ #include "qemu/module.h" #include "qemu/option.h" #include "qemu/bswap.h" +#include "qemu/cutils.h" #include #include "qapi/qmp/qdict.h" #include "qapi/qmp/qstring.h" @@ -295,11 +296,13 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, goto fail; } ret = bdrv_pread(bs->file, header.backing_file_offset, - bs->backing_file, len); + bs->auto_backing_file, len); if (ret < 0) { goto fail; } - bs->backing_file[len] = '\0'; + bs->auto_backing_file[len] = '\0'; + pstrcpy(bs->backing_file, sizeof(bs->backing_file), + bs->auto_backing_file); } /* Disable migration when qcow images are used */ diff --git a/block/qcow2.c b/block/qcow2.c index 65a54c9ac6..3826ce7a39 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1474,13 +1474,15 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, goto fail; } ret = bdrv_pread(bs->file, header.backing_file_offset, - bs->backing_file, len); + bs->auto_backing_file, len); if (ret < 0) { error_setg_errno(errp, -ret, "Could not read backing file name"); goto fail; } - bs->backing_file[len] = '\0'; - s->image_backing_file = g_strdup(bs->backing_file); + bs->auto_backing_file[len] = '\0'; + pstrcpy(bs->backing_file, sizeof(bs->backing_file), + bs->auto_backing_file); + s->image_backing_file = g_strdup(bs->auto_backing_file); } /* Internal snapshots */ @@ -2518,6 +2520,8 @@ static int qcow2_change_backing_file(BlockDriverState *bs, return -EINVAL; } + pstrcpy(bs->auto_backing_file, sizeof(bs->auto_backing_file), + backing_file ?: ""); pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_file ?: ""); pstrcpy(bs->backing_format, sizeof(bs->backing_format), backing_fmt ?: ""); diff --git a/block/qed.c b/block/qed.c index 1280870024..81a1bedd41 100644 --- a/block/qed.c +++ b/block/qed.c @@ -454,11 +454,14 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options, } ret = qed_read_string(bs->file, s->header.backing_filename_offset, - s->header.backing_filename_size, bs->backing_file, - sizeof(bs->backing_file)); + s->header.backing_filename_size, + bs->auto_backing_file, + sizeof(bs->auto_backing_file)); if (ret < 0) { return ret; } + pstrcpy(bs->backing_file, sizeof(bs->backing_file), + bs->auto_backing_file); if (s->header.features & QED_F_BACKING_FORMAT_NO_PROBE) { pstrcpy(bs->backing_format, sizeof(bs->backing_format), "raw"); diff --git a/block/vmdk.c b/block/vmdk.c index 117dc6adfe..464b718352 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -386,12 +386,14 @@ static int vmdk_parent_open(BlockDriverState *bs) ret = -EINVAL; goto out; } - if ((end_name - p_name) > sizeof(bs->backing_file) - 1) { + if ((end_name - p_name) > sizeof(bs->auto_backing_file) - 1) { ret = -EINVAL; goto out; } - pstrcpy(bs->backing_file, end_name - p_name + 1, p_name); + pstrcpy(bs->auto_backing_file, end_name - p_name + 1, p_name); + pstrcpy(bs->backing_file, sizeof(bs->backing_file), + bs->auto_backing_file); } out: diff --git a/include/block/block_int.h b/include/block/block_int.h index 8437df85a2..dd7276cde2 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -702,6 +702,10 @@ struct BlockDriverState { char filename[PATH_MAX]; char backing_file[PATH_MAX]; /* if non zero, the image is a diff of this file image */ + /* The backing filename indicated by the image header; if we ever + * open this file, then this is replaced by the resulting BDS's + * filename (i.e. after a bdrv_refresh_filename() run). */ + char auto_backing_file[PATH_MAX]; char backing_format[16]; /* if non-zero and backing_file exists */ QDict *full_open_options; -- cgit v1.2.3-55-g7522 From 009b03aaa233ccf5bd3014404995540158d7dc93 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 1 Feb 2019 20:29:13 +0100 Subject: block: Make path_combine() return the path Besides being safe for arbitrary path lengths, after some follow-up patches all callers will want a freshly allocated buffer anyway. In the meantime, path_combine_deprecated() is added which has the same interface as path_combine() had before this patch. All callers to that function will be converted in follow-up patches. Signed-off-by: Max Reitz Reviewed-by: Alberto Garcia Reviewed-by: Kevin Wolf Message-id: 20190201192935.18394-10-mreitz@redhat.com Signed-off-by: Max Reitz --- block.c | 85 ++++++++++++++++++++++++++++----------------------- block/vmdk.c | 3 +- include/block/block.h | 4 +-- 3 files changed, 49 insertions(+), 43 deletions(-) (limited to 'include') diff --git a/block.c b/block.c index 3b33941c5b..91859a928d 100644 --- a/block.c +++ b/block.c @@ -152,53 +152,62 @@ int path_is_absolute(const char *path) #endif } -/* if filename is absolute, just copy it to dest. Otherwise, build a +/* if filename is absolute, just return its duplicate. Otherwise, build a path to it by considering it is relative to base_path. URL are supported. */ -void path_combine(char *dest, int dest_size, - const char *base_path, - const char *filename) +char *path_combine(const char *base_path, const char *filename) { + const char *protocol_stripped = NULL; const char *p, *p1; + char *result; int len; - if (dest_size <= 0) - return; if (path_is_absolute(filename)) { - pstrcpy(dest, dest_size, filename); - } else { - const char *protocol_stripped = NULL; + return g_strdup(filename); + } - if (path_has_protocol(base_path)) { - protocol_stripped = strchr(base_path, ':'); - if (protocol_stripped) { - protocol_stripped++; - } + if (path_has_protocol(base_path)) { + protocol_stripped = strchr(base_path, ':'); + if (protocol_stripped) { + protocol_stripped++; } - p = protocol_stripped ?: base_path; + } + p = protocol_stripped ?: base_path; - p1 = strrchr(base_path, '/'); + p1 = strrchr(base_path, '/'); #ifdef _WIN32 - { - const char *p2; - p2 = strrchr(base_path, '\\'); - if (!p1 || p2 > p1) - p1 = p2; + { + const char *p2; + p2 = strrchr(base_path, '\\'); + if (!p1 || p2 > p1) { + p1 = p2; } + } #endif - if (p1) - p1++; - else - p1 = base_path; - if (p1 > p) - p = p1; - len = p - base_path; - if (len > dest_size - 1) - len = dest_size - 1; - memcpy(dest, base_path, len); - dest[len] = '\0'; - pstrcat(dest, dest_size, filename); + if (p1) { + p1++; + } else { + p1 = base_path; + } + if (p1 > p) { + p = p1; } + len = p - base_path; + + result = g_malloc(len + strlen(filename) + 1); + memcpy(result, base_path, len); + strcpy(result + len, filename); + + return result; +} + +static void path_combine_deprecated(char *dest, int dest_size, + const char *base_path, + const char *filename) +{ + char *combined = path_combine(base_path, filename); + pstrcpy(dest, dest_size, combined); + g_free(combined); } /* @@ -316,7 +325,7 @@ void bdrv_get_full_backing_filename_from_filename(const char *backed, error_setg(errp, "Cannot use relative backing file names for '%s'", backed); } else { - path_combine(dest, sz, backed, backing); + path_combine_deprecated(dest, sz, backed, backing); } } @@ -4657,8 +4666,8 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs, } else { /* If not an absolute filename path, make it relative to the current * image's filename path */ - path_combine(filename_tmp, PATH_MAX, curr_bs->filename, - backing_file); + path_combine_deprecated(filename_tmp, PATH_MAX, curr_bs->filename, + backing_file); /* We are going to compare absolute pathnames */ if (!realpath(filename_tmp, filename_full)) { @@ -4667,8 +4676,8 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs, /* We need to make sure the backing filename we are comparing against * is relative to the current image filename (or absolute) */ - path_combine(filename_tmp, PATH_MAX, curr_bs->filename, - curr_bs->backing_file); + path_combine_deprecated(filename_tmp, PATH_MAX, curr_bs->filename, + curr_bs->backing_file); if (!realpath(filename_tmp, backing_file_full)) { continue; diff --git a/block/vmdk.c b/block/vmdk.c index 464b718352..32e4e7589a 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -873,8 +873,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs, return -EINVAL; } - extent_path = g_malloc0(PATH_MAX); - path_combine(extent_path, PATH_MAX, desc_file_path, fname); + extent_path = path_combine(desc_file_path, fname); ret = snprintf(extent_opt_prefix, 32, "extents.%d", s->num_extents); assert(ret < 32); diff --git a/include/block/block.h b/include/block/block.h index aaae900925..e233372a3a 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -496,9 +496,7 @@ void bdrv_get_full_backing_filename_from_filename(const char *backed, int path_has_protocol(const char *path); int path_is_absolute(const char *path); -void path_combine(char *dest, int dest_size, - const char *base_path, - const char *filename); +char *path_combine(const char *base_path, const char *filename); int bdrv_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos); int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos); -- cgit v1.2.3-55-g7522 From 645ae7d88e5393a2a67ebe325f4456ecd49e33e5 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 1 Feb 2019 20:29:14 +0100 Subject: block: bdrv_get_full_backing_filename_from_...'s ret. val. Make bdrv_get_full_backing_filename_from_filename() return an allocated string instead of placing the result in a caller-provided buffer. Signed-off-by: Max Reitz Message-id: 20190201192935.18394-11-mreitz@redhat.com Signed-off-by: Max Reitz --- block.c | 53 +++++++++++++++++++++++++++++++++++---------------- block/vmdk.c | 10 +++++----- include/block/block.h | 7 +++---- qemu-img.c | 12 ++++-------- 4 files changed, 49 insertions(+), 33 deletions(-) (limited to 'include') diff --git a/block.c b/block.c index 91859a928d..466d7887cc 100644 --- a/block.c +++ b/block.c @@ -312,20 +312,29 @@ fail: return -EACCES; } -void bdrv_get_full_backing_filename_from_filename(const char *backed, - const char *backing, - char *dest, size_t sz, - Error **errp) +/* + * If @backing is empty, this function returns NULL without setting + * @errp. In all other cases, NULL will only be returned with @errp + * set. + * + * Therefore, a return value of NULL without @errp set means that + * there is no backing file; if @errp is set, there is one but its + * absolute filename cannot be generated. + */ +char *bdrv_get_full_backing_filename_from_filename(const char *backed, + const char *backing, + Error **errp) { - if (backing[0] == '\0' || path_has_protocol(backing) || - path_is_absolute(backing)) - { - pstrcpy(dest, sz, backing); + if (backing[0] == '\0') { + return NULL; + } else if (path_has_protocol(backing) || path_is_absolute(backing)) { + return g_strdup(backing); } else if (backed[0] == '\0' || strstart(backed, "json:", NULL)) { error_setg(errp, "Cannot use relative backing file names for '%s'", backed); + return NULL; } else { - path_combine_deprecated(dest, sz, backed, backing); + return path_combine(backed, backing); } } @@ -333,12 +342,24 @@ void bdrv_get_full_backing_filename(BlockDriverState *bs, char *dest, size_t sz, Error **errp) { char *backed; + char *full_name; + Error *local_error = NULL; bdrv_refresh_filename(bs); backed = bs->exact_filename[0] ? bs->exact_filename : bs->filename; - bdrv_get_full_backing_filename_from_filename(backed, bs->backing_file, - dest, sz, errp); + + full_name = bdrv_get_full_backing_filename_from_filename(backed, + bs->backing_file, + &local_error); + if (full_name) { + pstrcpy(dest, sz, full_name); + g_free(full_name); + } else if (local_error) { + error_propagate(errp, local_error); + } else if (sz > 0) { + *dest = '\0'; + } } void bdrv_register(BlockDriver *bdrv) @@ -5179,17 +5200,17 @@ void bdrv_img_create(const char *filename, const char *fmt, size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, img_size); if (backing_file && !(flags & BDRV_O_NO_BACKING)) { BlockDriverState *bs; - char *full_backing = g_new0(char, PATH_MAX); + char *full_backing; int back_flags; QDict *backing_options = NULL; - bdrv_get_full_backing_filename_from_filename(filename, backing_file, - full_backing, PATH_MAX, - &local_err); + full_backing = + bdrv_get_full_backing_filename_from_filename(filename, backing_file, + &local_err); if (local_err) { - g_free(full_backing); goto out; } + assert(full_backing); /* backing files always opened read-only */ back_flags = flags; diff --git a/block/vmdk.c b/block/vmdk.c index 32e4e7589a..81c506cb69 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -2078,16 +2078,16 @@ static int coroutine_fn vmdk_co_do_create(int64_t size, if (backing_file) { BlockBackend *backing; - char *full_backing = g_new0(char, PATH_MAX); - bdrv_get_full_backing_filename_from_filename(blk_bs(blk)->filename, backing_file, - full_backing, PATH_MAX, - &local_err); + char *full_backing = + bdrv_get_full_backing_filename_from_filename(blk_bs(blk)->filename, + backing_file, + &local_err); if (local_err) { - g_free(full_backing); error_propagate(errp, local_err); ret = -ENOENT; goto exit; } + assert(full_backing); backing = blk_new_open(full_backing, NULL, NULL, BDRV_O_NO_BACKING, errp); diff --git a/include/block/block.h b/include/block/block.h index e233372a3a..5f1650304d 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -489,10 +489,9 @@ void bdrv_get_backing_filename(BlockDriverState *bs, char *filename, int filename_size); void bdrv_get_full_backing_filename(BlockDriverState *bs, char *dest, size_t sz, Error **errp); -void bdrv_get_full_backing_filename_from_filename(const char *backed, - const char *backing, - char *dest, size_t sz, - Error **errp); +char *bdrv_get_full_backing_filename_from_filename(const char *backed, + const char *backing, + Error **errp); int path_has_protocol(const char *path); int path_is_absolute(const char *path); diff --git a/qemu-img.c b/qemu-img.c index d2fc28c987..ae0025926c 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -3343,18 +3343,14 @@ static int img_rebase(int argc, char **argv) bdrv_refresh_filename(bs); overlay_filename = bs->exact_filename[0] ? bs->exact_filename : bs->filename; - out_real_path = g_malloc(PATH_MAX); - - bdrv_get_full_backing_filename_from_filename(overlay_filename, - out_baseimg, - out_real_path, - PATH_MAX, - &local_err); + out_real_path = + bdrv_get_full_backing_filename_from_filename(overlay_filename, + out_baseimg, + &local_err); if (local_err) { error_reportf_err(local_err, "Could not resolve backing filename: "); ret = -1; - g_free(out_real_path); goto out; } -- cgit v1.2.3-55-g7522 From 6b6833c1b4d11d7d838bce34ed4a2d7c42855efe Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 1 Feb 2019 20:29:15 +0100 Subject: block: bdrv_get_full_backing_filename's ret. val. Make bdrv_get_full_backing_filename() return an allocated string instead of placing the result in a caller-provided buffer. Signed-off-by: Max Reitz Reviewed-by: Alberto Garcia Message-id: 20190201192935.18394-12-mreitz@redhat.com Signed-off-by: Max Reitz --- block.c | 48 +++++++++++++++++------------------------------- block/qapi.c | 12 ++---------- include/block/block.h | 3 +-- 3 files changed, 20 insertions(+), 43 deletions(-) (limited to 'include') diff --git a/block.c b/block.c index 466d7887cc..a2203f0cfe 100644 --- a/block.c +++ b/block.c @@ -338,28 +338,16 @@ char *bdrv_get_full_backing_filename_from_filename(const char *backed, } } -void bdrv_get_full_backing_filename(BlockDriverState *bs, char *dest, size_t sz, - Error **errp) +char *bdrv_get_full_backing_filename(BlockDriverState *bs, Error **errp) { char *backed; - char *full_name; - Error *local_error = NULL; bdrv_refresh_filename(bs); backed = bs->exact_filename[0] ? bs->exact_filename : bs->filename; - - full_name = bdrv_get_full_backing_filename_from_filename(backed, - bs->backing_file, - &local_error); - if (full_name) { - pstrcpy(dest, sz, full_name); - g_free(full_name); - } else if (local_error) { - error_propagate(errp, local_error); - } else if (sz > 0) { - *dest = '\0'; - } + return bdrv_get_full_backing_filename_from_filename(backed, + bs->backing_file, + errp); } void bdrv_register(BlockDriver *bdrv) @@ -2387,7 +2375,7 @@ out: int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options, const char *bdref_key, Error **errp) { - char *backing_filename = g_malloc0(PATH_MAX); + char *backing_filename = NULL; char *bdref_key_dot; const char *reference = NULL; int ret = 0; @@ -2422,7 +2410,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options, */ reference = qdict_get_try_str(parent_options, bdref_key); if (reference || qdict_haskey(options, "file.filename")) { - backing_filename[0] = '\0'; + /* keep backing_filename NULL */ } else if (bs->backing_file[0] == '\0' && qdict_size(options) == 0) { qobject_unref(options); goto free_exit; @@ -2437,8 +2425,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options, implicit_backing = !strcmp(bs->auto_backing_file, bs->backing_file); } - bdrv_get_full_backing_filename(bs, backing_filename, PATH_MAX, - &local_err); + backing_filename = bdrv_get_full_backing_filename(bs, &local_err); if (local_err) { ret = -EINVAL; error_propagate(errp, local_err); @@ -2459,9 +2446,8 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options, qdict_put_str(options, "driver", bs->backing_format); } - backing_hd = bdrv_open_inherit(*backing_filename ? backing_filename : NULL, - reference, options, 0, bs, &child_backing, - errp); + backing_hd = bdrv_open_inherit(backing_filename, reference, options, 0, bs, + &child_backing, errp); if (!backing_hd) { bs->open_flags |= BDRV_O_NO_BACKING; error_prepend(errp, "Could not open backing file: "); @@ -4648,7 +4634,6 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs, int is_protocol = 0; BlockDriverState *curr_bs = NULL; BlockDriverState *retval = NULL; - Error *local_error = NULL; if (!bs || !bs->drv || !backing_file) { return NULL; @@ -4668,21 +4653,22 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs, /* If either of the filename paths is actually a protocol, then * compare unmodified paths; otherwise make paths relative */ if (is_protocol || path_has_protocol(curr_bs->backing_file)) { + char *backing_file_full_ret; + if (strcmp(backing_file, curr_bs->backing_file) == 0) { retval = curr_bs->backing->bs; break; } /* Also check against the full backing filename for the image */ - bdrv_get_full_backing_filename(curr_bs, backing_file_full, PATH_MAX, - &local_error); - if (local_error == NULL) { - if (strcmp(backing_file, backing_file_full) == 0) { + backing_file_full_ret = bdrv_get_full_backing_filename(curr_bs, + NULL); + if (backing_file_full_ret) { + bool equal = strcmp(backing_file, backing_file_full_ret) == 0; + g_free(backing_file_full_ret); + if (equal) { retval = curr_bs->backing->bs; break; } - } else { - error_free(local_error); - local_error = NULL; } } else { /* If not an absolute filename path, make it relative to the current diff --git a/block/qapi.c b/block/qapi.c index 4623de1d7b..6002a768f8 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -296,18 +296,10 @@ void bdrv_query_image_info(BlockDriverState *bs, backing_filename = bs->backing_file; if (backing_filename[0] != '\0') { - char *backing_filename2 = g_malloc0(PATH_MAX); + char *backing_filename2; info->backing_filename = g_strdup(backing_filename); info->has_backing_filename = true; - bdrv_get_full_backing_filename(bs, backing_filename2, PATH_MAX, &err); - if (err) { - /* Can't reconstruct the full backing filename, so we must omit - * this field and apply a Best Effort to this query. */ - g_free(backing_filename2); - backing_filename2 = NULL; - error_free(err); - err = NULL; - } + backing_filename2 = bdrv_get_full_backing_filename(bs, NULL); /* Always report the full_backing_filename if present, even if it's the * same as backing_filename. That they are same is useful info. */ diff --git a/include/block/block.h b/include/block/block.h index 5f1650304d..42cc38f598 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -487,8 +487,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs, void bdrv_get_backing_filename(BlockDriverState *bs, char *filename, int filename_size); -void bdrv_get_full_backing_filename(BlockDriverState *bs, - char *dest, size_t sz, Error **errp); +char *bdrv_get_full_backing_filename(BlockDriverState *bs, Error **errp); char *bdrv_get_full_backing_filename_from_filename(const char *backed, const char *backing, Error **errp); -- cgit v1.2.3-55-g7522 From 1e89d0f9bed7e95278d151cf6308cafbba8dae9f Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 1 Feb 2019 20:29:18 +0100 Subject: block: Add bdrv_dirname() This function may be implemented by block drivers to derive a directory name from a BDS. Concatenating this g_free()-able string with a relative filename must result in a valid (not necessarily existing) filename, so this is a function that should generally be not implemented by format drivers, because this is protocol-specific. If a BDS's driver does not implement this function, bdrv_dirname() will fall through to the BDS's file if it exists. If it does not, the exact_filename field will be used to generate a directory name. Signed-off-by: Max Reitz Reviewed-by: Alberto Garcia Message-id: 20190201192935.18394-15-mreitz@redhat.com Signed-off-by: Max Reitz --- block.c | 27 +++++++++++++++++++++++++++ include/block/block.h | 1 + include/block/block_int.h | 7 +++++++ 3 files changed, 35 insertions(+) (limited to 'include') diff --git a/block.c b/block.c index d14ce1c98c..9f655c0fd6 100644 --- a/block.c +++ b/block.c @@ -5715,6 +5715,33 @@ void bdrv_refresh_filename(BlockDriverState *bs) } } +char *bdrv_dirname(BlockDriverState *bs, Error **errp) +{ + BlockDriver *drv = bs->drv; + + if (!drv) { + error_setg(errp, "Node '%s' is ejected", bs->node_name); + return NULL; + } + + if (drv->bdrv_dirname) { + return drv->bdrv_dirname(bs, errp); + } + + if (bs->file) { + return bdrv_dirname(bs->file->bs, errp); + } + + bdrv_refresh_filename(bs); + if (bs->exact_filename[0] != '\0') { + return path_combine(bs->exact_filename, ""); + } + + error_setg(errp, "Cannot generate a base directory for %s nodes", + drv->format_name); + return NULL; +} + /* * Hot add/remove a BDS's child. So the user can take a child offline when * it is broken and take a new child online diff --git a/include/block/block.h b/include/block/block.h index 42cc38f598..5b5cf868df 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -491,6 +491,7 @@ char *bdrv_get_full_backing_filename(BlockDriverState *bs, Error **errp); char *bdrv_get_full_backing_filename_from_filename(const char *backed, const char *backing, Error **errp); +char *bdrv_dirname(BlockDriverState *bs, Error **errp); int path_has_protocol(const char *path); int path_is_absolute(const char *path); diff --git a/include/block/block_int.h b/include/block/block_int.h index dd7276cde2..e4d4817ea6 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -141,6 +141,13 @@ struct BlockDriver { void (*bdrv_refresh_filename)(BlockDriverState *bs, QDict *options); + /* + * Returns an allocated string which is the directory name of this BDS: It + * will be used to make relative filenames absolute by prepending this + * function's return value to them. + */ + char *(*bdrv_dirname)(BlockDriverState *bs, Error **errp); + /* aio */ BlockAIOCB *(*bdrv_aio_preadv)(BlockDriverState *bs, uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags, -- cgit v1.2.3-55-g7522 From 2654267cc163083f4fb9a6d719468d9dd1bea455 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 1 Feb 2019 20:29:25 +0100 Subject: block: Add strong_runtime_opts to BlockDriver This new field can be set by block drivers to list the runtime options they accept that may influence the contents of the respective BDS. As of a follow-up patch, this list will be used by the common bdrv_refresh_filename() implementation to decide which options to put into BDS.full_open_options (and consequently whether a JSON filename has to be created), thus freeing the drivers of having to implement that logic themselves. Additionally, this patch adds the field to all of the block drivers that need it and sets it accordingly. Signed-off-by: Max Reitz Reviewed-by: Alberto Garcia Message-id: 20190201192935.18394-22-mreitz@redhat.com Signed-off-by: Max Reitz --- block/blkdebug.c | 16 ++++++++++++++++ block/blklogwrites.c | 8 ++++++++ block/crypto.c | 8 ++++++++ block/curl.c | 21 +++++++++++++++++++++ block/gluster.c | 19 +++++++++++++++++++ block/iscsi.c | 18 ++++++++++++++++++ block/nbd.c | 14 ++++++++++++++ block/nfs.c | 11 +++++++++++ block/null.c | 9 +++++++++ block/nvme.c | 8 ++++++++ block/qcow.c | 7 +++++++ block/qcow2.c | 7 +++++++ block/quorum.c | 11 +++++++++++ block/raw-format.c | 10 +++++++++- block/rbd.c | 14 ++++++++++++++ block/replication.c | 8 ++++++++ block/sheepdog.c | 12 ++++++++++++ block/ssh.c | 12 ++++++++++++ block/throttle.c | 7 +++++++ block/vpc.c | 7 +++++++ block/vvfat.c | 12 ++++++++++++ block/vxhs.c | 11 +++++++++++ include/block/block_int.h | 7 +++++++ 23 files changed, 256 insertions(+), 1 deletion(-) (limited to 'include') diff --git a/block/blkdebug.c b/block/blkdebug.c index 0759452925..71b4275b98 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -888,6 +888,20 @@ static int blkdebug_reopen_prepare(BDRVReopenState *reopen_state, return 0; } +static const char *const blkdebug_strong_runtime_opts[] = { + "config", + "inject-error.", + "set-state.", + "align", + "max-transfer", + "opt-write-zero", + "max-write-zero", + "opt-discard", + "max-discard", + + NULL +}; + static BlockDriver bdrv_blkdebug = { .format_name = "blkdebug", .protocol_name = "blkdebug", @@ -917,6 +931,8 @@ static BlockDriver bdrv_blkdebug = { = blkdebug_debug_remove_breakpoint, .bdrv_debug_resume = blkdebug_debug_resume, .bdrv_debug_is_suspended = blkdebug_debug_is_suspended, + + .strong_runtime_opts = blkdebug_strong_runtime_opts, }; static void bdrv_blkdebug_init(void) diff --git a/block/blklogwrites.c b/block/blklogwrites.c index 36e3d0f822..5da5df112d 100644 --- a/block/blklogwrites.c +++ b/block/blklogwrites.c @@ -517,6 +517,13 @@ blk_log_writes_co_pdiscard(BlockDriverState *bs, int64_t offset, int count) LOG_DISCARD_FLAG, false); } +static const char *const blk_log_writes_strong_runtime_opts[] = { + "log-append", + "log-sector-size", + + NULL +}; + static BlockDriver bdrv_blk_log_writes = { .format_name = "blklogwrites", .instance_size = sizeof(BDRVBlkLogWritesState), @@ -536,6 +543,7 @@ static BlockDriver bdrv_blk_log_writes = { .bdrv_co_block_status = bdrv_co_block_status_from_file, .is_filter = true, + .strong_runtime_opts = blk_log_writes_strong_runtime_opts, }; static void bdrv_blk_log_writes_init(void) diff --git a/block/crypto.c b/block/crypto.c index d5b1da66a1..fd8c7cfac6 100644 --- a/block/crypto.c +++ b/block/crypto.c @@ -619,6 +619,12 @@ block_crypto_get_specific_info_luks(BlockDriverState *bs, Error **errp) return spec_info; } +static const char *const block_crypto_strong_runtime_opts[] = { + BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET, + + NULL +}; + BlockDriver bdrv_crypto_luks = { .format_name = "luks", .instance_size = sizeof(BlockCrypto), @@ -640,6 +646,8 @@ BlockDriver bdrv_crypto_luks = { .bdrv_getlength = block_crypto_getlength, .bdrv_get_info = block_crypto_get_info_luks, .bdrv_get_specific_info = block_crypto_get_specific_info_luks, + + .strong_runtime_opts = block_crypto_strong_runtime_opts, }; static void block_crypto_init(void) diff --git a/block/curl.c b/block/curl.c index b7ac265d3a..1c9e4f6a64 100644 --- a/block/curl.c +++ b/block/curl.c @@ -947,6 +947,19 @@ static int64_t curl_getlength(BlockDriverState *bs) return s->len; } +static const char *const curl_strong_runtime_opts[] = { + CURL_BLOCK_OPT_URL, + CURL_BLOCK_OPT_SSLVERIFY, + CURL_BLOCK_OPT_COOKIE, + CURL_BLOCK_OPT_COOKIE_SECRET, + CURL_BLOCK_OPT_USERNAME, + CURL_BLOCK_OPT_PASSWORD_SECRET, + CURL_BLOCK_OPT_PROXY_USERNAME, + CURL_BLOCK_OPT_PROXY_PASSWORD_SECRET, + + NULL +}; + static BlockDriver bdrv_http = { .format_name = "http", .protocol_name = "http", @@ -961,6 +974,8 @@ static BlockDriver bdrv_http = { .bdrv_detach_aio_context = curl_detach_aio_context, .bdrv_attach_aio_context = curl_attach_aio_context, + + .strong_runtime_opts = curl_strong_runtime_opts, }; static BlockDriver bdrv_https = { @@ -977,6 +992,8 @@ static BlockDriver bdrv_https = { .bdrv_detach_aio_context = curl_detach_aio_context, .bdrv_attach_aio_context = curl_attach_aio_context, + + .strong_runtime_opts = curl_strong_runtime_opts, }; static BlockDriver bdrv_ftp = { @@ -993,6 +1010,8 @@ static BlockDriver bdrv_ftp = { .bdrv_detach_aio_context = curl_detach_aio_context, .bdrv_attach_aio_context = curl_attach_aio_context, + + .strong_runtime_opts = curl_strong_runtime_opts, }; static BlockDriver bdrv_ftps = { @@ -1009,6 +1028,8 @@ static BlockDriver bdrv_ftps = { .bdrv_detach_aio_context = curl_detach_aio_context, .bdrv_attach_aio_context = curl_attach_aio_context, + + .strong_runtime_opts = curl_strong_runtime_opts, }; static void curl_block_init(void) diff --git a/block/gluster.c b/block/gluster.c index 72891060e3..af64330211 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -1495,6 +1495,21 @@ static int coroutine_fn qemu_gluster_co_block_status(BlockDriverState *bs, } +static const char *const gluster_strong_open_opts[] = { + GLUSTER_OPT_VOLUME, + GLUSTER_OPT_PATH, + GLUSTER_OPT_TYPE, + GLUSTER_OPT_SERVER_PATTERN, + GLUSTER_OPT_HOST, + GLUSTER_OPT_PORT, + GLUSTER_OPT_TO, + GLUSTER_OPT_IPV4, + GLUSTER_OPT_IPV6, + GLUSTER_OPT_SOCKET, + + NULL +}; + static BlockDriver bdrv_gluster = { .format_name = "gluster", .protocol_name = "gluster", @@ -1522,6 +1537,7 @@ static BlockDriver bdrv_gluster = { #endif .bdrv_co_block_status = qemu_gluster_co_block_status, .create_opts = &qemu_gluster_create_opts, + .strong_runtime_opts = gluster_strong_open_opts, }; static BlockDriver bdrv_gluster_tcp = { @@ -1551,6 +1567,7 @@ static BlockDriver bdrv_gluster_tcp = { #endif .bdrv_co_block_status = qemu_gluster_co_block_status, .create_opts = &qemu_gluster_create_opts, + .strong_runtime_opts = gluster_strong_open_opts, }; static BlockDriver bdrv_gluster_unix = { @@ -1580,6 +1597,7 @@ static BlockDriver bdrv_gluster_unix = { #endif .bdrv_co_block_status = qemu_gluster_co_block_status, .create_opts = &qemu_gluster_create_opts, + .strong_runtime_opts = gluster_strong_open_opts, }; /* rdma is deprecated (actually never supported for volfile fetch). @@ -1615,6 +1633,7 @@ static BlockDriver bdrv_gluster_rdma = { #endif .bdrv_co_block_status = qemu_gluster_co_block_status, .create_opts = &qemu_gluster_create_opts, + .strong_runtime_opts = gluster_strong_open_opts, }; static void bdrv_gluster_init(void) diff --git a/block/iscsi.c b/block/iscsi.c index ff473206e6..a0c0084837 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -2448,6 +2448,20 @@ static QemuOptsList iscsi_create_opts = { } }; +static const char *const iscsi_strong_runtime_opts[] = { + "transport", + "portal", + "target", + "user", + "password", + "password-secret", + "lun", + "initiator-name", + "header-digest", + + NULL +}; + static BlockDriver bdrv_iscsi = { .format_name = "iscsi", .protocol_name = "iscsi", @@ -2482,6 +2496,8 @@ static BlockDriver bdrv_iscsi = { .bdrv_detach_aio_context = iscsi_detach_aio_context, .bdrv_attach_aio_context = iscsi_attach_aio_context, + + .strong_runtime_opts = iscsi_strong_runtime_opts, }; #if LIBISCSI_API_VERSION >= (20160603) @@ -2519,6 +2535,8 @@ static BlockDriver bdrv_iser = { .bdrv_detach_aio_context = iscsi_detach_aio_context, .bdrv_attach_aio_context = iscsi_attach_aio_context, + + .strong_runtime_opts = iscsi_strong_runtime_opts, }; #endif diff --git a/block/nbd.c b/block/nbd.c index 83e6e9e442..318a58776c 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -538,6 +538,17 @@ static char *nbd_dirname(BlockDriverState *bs, Error **errp) return NULL; } +static const char *const nbd_strong_runtime_opts[] = { + "path", + "host", + "port", + "export", + "tls-creds", + "server.", + + NULL +}; + static BlockDriver bdrv_nbd = { .format_name = "nbd", .protocol_name = "nbd", @@ -557,6 +568,7 @@ static BlockDriver bdrv_nbd = { .bdrv_refresh_filename = nbd_refresh_filename, .bdrv_co_block_status = nbd_client_co_block_status, .bdrv_dirname = nbd_dirname, + .strong_runtime_opts = nbd_strong_runtime_opts, }; static BlockDriver bdrv_nbd_tcp = { @@ -578,6 +590,7 @@ static BlockDriver bdrv_nbd_tcp = { .bdrv_refresh_filename = nbd_refresh_filename, .bdrv_co_block_status = nbd_client_co_block_status, .bdrv_dirname = nbd_dirname, + .strong_runtime_opts = nbd_strong_runtime_opts, }; static BlockDriver bdrv_nbd_unix = { @@ -599,6 +612,7 @@ static BlockDriver bdrv_nbd_unix = { .bdrv_refresh_filename = nbd_refresh_filename, .bdrv_co_block_status = nbd_client_co_block_status, .bdrv_dirname = nbd_dirname, + .strong_runtime_opts = nbd_strong_runtime_opts, }; static void bdrv_nbd_init(void) diff --git a/block/nfs.c b/block/nfs.c index 19ee07c321..6985a44b89 100644 --- a/block/nfs.c +++ b/block/nfs.c @@ -878,6 +878,15 @@ static void coroutine_fn nfs_co_invalidate_cache(BlockDriverState *bs, } #endif +static const char *nfs_strong_runtime_opts[] = { + "path", + "user", + "group", + "server.", + + NULL +}; + static BlockDriver bdrv_nfs = { .format_name = "nfs", .protocol_name = "nfs", @@ -905,6 +914,8 @@ static BlockDriver bdrv_nfs = { .bdrv_refresh_filename = nfs_refresh_filename, .bdrv_dirname = nfs_dirname, + .strong_runtime_opts = nfs_strong_runtime_opts, + #ifdef LIBNFS_FEATURE_PAGECACHE .bdrv_co_invalidate_cache = nfs_co_invalidate_cache, #endif diff --git a/block/null.c b/block/null.c index d442d3e901..858892f0c4 100644 --- a/block/null.c +++ b/block/null.c @@ -252,6 +252,13 @@ static void null_refresh_filename(BlockDriverState *bs, QDict *opts) bs->full_open_options = qobject_ref(opts); } +static const char *const null_strong_runtime_opts[] = { + BLOCK_OPT_SIZE, + NULL_OPT_ZEROES, + + NULL +}; + static BlockDriver bdrv_null_co = { .format_name = "null-co", .protocol_name = "null-co", @@ -269,6 +276,7 @@ static BlockDriver bdrv_null_co = { .bdrv_co_block_status = null_co_block_status, .bdrv_refresh_filename = null_refresh_filename, + .strong_runtime_opts = null_strong_runtime_opts, }; static BlockDriver bdrv_null_aio = { @@ -288,6 +296,7 @@ static BlockDriver bdrv_null_aio = { .bdrv_co_block_status = null_co_block_status, .bdrv_refresh_filename = null_refresh_filename, + .strong_runtime_opts = null_strong_runtime_opts, }; static void bdrv_null_init(void) diff --git a/block/nvme.c b/block/nvme.c index 6c2ce7dfa5..bf656b2bba 100644 --- a/block/nvme.c +++ b/block/nvme.c @@ -1136,6 +1136,13 @@ static void nvme_unregister_buf(BlockDriverState *bs, void *host) qemu_vfio_dma_unmap(s->vfio, host); } +static const char *const nvme_strong_runtime_opts[] = { + NVME_BLOCK_OPT_DEVICE, + NVME_BLOCK_OPT_NAMESPACE, + + NULL +}; + static BlockDriver bdrv_nvme = { .format_name = "nvme", .protocol_name = "nvme", @@ -1153,6 +1160,7 @@ static BlockDriver bdrv_nvme = { .bdrv_refresh_filename = nvme_refresh_filename, .bdrv_refresh_limits = nvme_refresh_limits, + .strong_runtime_opts = nvme_strong_runtime_opts, .bdrv_detach_aio_context = nvme_detach_aio_context, .bdrv_attach_aio_context = nvme_attach_aio_context, diff --git a/block/qcow.c b/block/qcow.c index d47515d3df..25d2025fd0 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -1186,6 +1186,12 @@ static QemuOptsList qcow_create_opts = { } }; +static const char *const qcow_strong_runtime_opts[] = { + "encrypt." BLOCK_CRYPTO_OPT_QCOW_KEY_SECRET, + + NULL +}; + static BlockDriver bdrv_qcow = { .format_name = "qcow", .instance_size = sizeof(BDRVQcowState), @@ -1209,6 +1215,7 @@ static BlockDriver bdrv_qcow = { .bdrv_get_info = qcow_get_info, .create_opts = &qcow_create_opts, + .strong_runtime_opts = qcow_strong_runtime_opts, }; static void bdrv_qcow_init(void) diff --git a/block/qcow2.c b/block/qcow2.c index 3826ce7a39..92242fb14f 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -4936,6 +4936,12 @@ static QemuOptsList qcow2_create_opts = { } }; +static const char *const qcow2_strong_runtime_opts[] = { + "encrypt." BLOCK_CRYPTO_OPT_QCOW_KEY_SECRET, + + NULL +}; + BlockDriver bdrv_qcow2 = { .format_name = "qcow2", .instance_size = sizeof(BDRVQcow2State), @@ -4984,6 +4990,7 @@ BlockDriver bdrv_qcow2 = { .bdrv_inactivate = qcow2_inactivate, .create_opts = &qcow2_create_opts, + .strong_runtime_opts = qcow2_strong_runtime_opts, .bdrv_co_check = qcow2_co_check, .bdrv_amend_options = qcow2_amend_options, diff --git a/block/quorum.c b/block/quorum.c index a890f21e85..1af6458dc4 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -1104,6 +1104,15 @@ static char *quorum_dirname(BlockDriverState *bs, Error **errp) return NULL; } +static const char *const quorum_strong_runtime_opts[] = { + QUORUM_OPT_VOTE_THRESHOLD, + QUORUM_OPT_BLKVERIFY, + QUORUM_OPT_REWRITE, + QUORUM_OPT_READ_PATTERN, + + NULL +}; + static BlockDriver bdrv_quorum = { .format_name = "quorum", @@ -1128,6 +1137,8 @@ static BlockDriver bdrv_quorum = { .is_filter = true, .bdrv_recurse_is_first_non_filter = quorum_recurse_is_first_non_filter, + + .strong_runtime_opts = quorum_strong_runtime_opts, }; static void bdrv_quorum_init(void) diff --git a/block/raw-format.c b/block/raw-format.c index d07bcdae62..e3e5ba2c8a 100644 --- a/block/raw-format.c +++ b/block/raw-format.c @@ -532,6 +532,13 @@ static int coroutine_fn raw_co_copy_range_to(BlockDriverState *bs, read_flags, write_flags); } +static const char *const raw_strong_runtime_opts[] = { + "offset", + "size", + + NULL +}; + BlockDriver bdrv_raw = { .format_name = "raw", .instance_size = sizeof(BDRVRawState), @@ -561,7 +568,8 @@ BlockDriver bdrv_raw = { .bdrv_lock_medium = &raw_lock_medium, .bdrv_co_ioctl = &raw_co_ioctl, .create_opts = &raw_create_opts, - .bdrv_has_zero_init = &raw_has_zero_init + .bdrv_has_zero_init = &raw_has_zero_init, + .strong_runtime_opts = raw_strong_runtime_opts, }; static void bdrv_raw_init(void) diff --git a/block/rbd.c b/block/rbd.c index 8a1a9f4b6e..0c549c9935 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -1228,6 +1228,18 @@ static QemuOptsList qemu_rbd_create_opts = { } }; +static const char *const qemu_rbd_strong_runtime_opts[] = { + "pool", + "image", + "conf", + "snapshot", + "user", + "server.", + "password-secret", + + NULL +}; + static BlockDriver bdrv_rbd = { .format_name = "rbd", .instance_size = sizeof(BDRVRBDState), @@ -1265,6 +1277,8 @@ static BlockDriver bdrv_rbd = { #ifdef LIBRBD_SUPPORTS_INVALIDATE .bdrv_co_invalidate_cache = qemu_rbd_co_invalidate_cache, #endif + + .strong_runtime_opts = qemu_rbd_strong_runtime_opts, }; static void bdrv_rbd_init(void) diff --git a/block/replication.c b/block/replication.c index 9b332002ee..4c80b54daf 100644 --- a/block/replication.c +++ b/block/replication.c @@ -676,6 +676,13 @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp) aio_context_release(aio_context); } +static const char *const replication_strong_runtime_opts[] = { + REPLICATION_MODE, + REPLICATION_TOP_ID, + + NULL +}; + BlockDriver bdrv_replication = { .format_name = "replication", .instance_size = sizeof(BDRVReplicationState), @@ -692,6 +699,7 @@ BlockDriver bdrv_replication = { .bdrv_recurse_is_first_non_filter = replication_recurse_is_first_non_filter, .has_variable_length = true, + .strong_runtime_opts = replication_strong_runtime_opts, }; static void bdrv_replication_init(void) diff --git a/block/sheepdog.c b/block/sheepdog.c index b916ba07bf..cbdfe9ab6e 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -3203,6 +3203,15 @@ static QemuOptsList sd_create_opts = { } }; +static const char *const sd_strong_runtime_opts[] = { + "vdi", + "snap-id", + "tag", + "server.", + + NULL +}; + static BlockDriver bdrv_sheepdog = { .format_name = "sheepdog", .protocol_name = "sheepdog", @@ -3238,6 +3247,7 @@ static BlockDriver bdrv_sheepdog = { .bdrv_attach_aio_context = sd_attach_aio_context, .create_opts = &sd_create_opts, + .strong_runtime_opts = sd_strong_runtime_opts, }; static BlockDriver bdrv_sheepdog_tcp = { @@ -3275,6 +3285,7 @@ static BlockDriver bdrv_sheepdog_tcp = { .bdrv_attach_aio_context = sd_attach_aio_context, .create_opts = &sd_create_opts, + .strong_runtime_opts = sd_strong_runtime_opts, }; static BlockDriver bdrv_sheepdog_unix = { @@ -3312,6 +3323,7 @@ static BlockDriver bdrv_sheepdog_unix = { .bdrv_attach_aio_context = sd_attach_aio_context, .create_opts = &sd_create_opts, + .strong_runtime_opts = sd_strong_runtime_opts, }; static void bdrv_sheepdog_init(void) diff --git a/block/ssh.c b/block/ssh.c index bbc513e095..190ef95300 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -1254,6 +1254,17 @@ static int coroutine_fn ssh_co_truncate(BlockDriverState *bs, int64_t offset, return ssh_grow_file(s, offset, errp); } +static const char *const ssh_strong_runtime_opts[] = { + "host", + "port", + "path", + "user", + "host_key_check", + "server.", + + NULL +}; + static BlockDriver bdrv_ssh = { .format_name = "ssh", .protocol_name = "ssh", @@ -1270,6 +1281,7 @@ static BlockDriver bdrv_ssh = { .bdrv_co_truncate = ssh_co_truncate, .bdrv_co_flush_to_disk = ssh_co_flush, .create_opts = &ssh_create_opts, + .strong_runtime_opts = ssh_strong_runtime_opts, }; static void bdrv_ssh_init(void) diff --git a/block/throttle.c b/block/throttle.c index 636c9764aa..f64dcc27b9 100644 --- a/block/throttle.c +++ b/block/throttle.c @@ -227,6 +227,12 @@ static void coroutine_fn throttle_co_drain_end(BlockDriverState *bs) atomic_dec(&tgm->io_limits_disabled); } +static const char *const throttle_strong_runtime_opts[] = { + QEMU_OPT_THROTTLE_GROUP_NAME, + + NULL +}; + static BlockDriver bdrv_throttle = { .format_name = "throttle", .instance_size = sizeof(ThrottleGroupMember), @@ -259,6 +265,7 @@ static BlockDriver bdrv_throttle = { .bdrv_co_drain_end = throttle_co_drain_end, .is_filter = true, + .strong_runtime_opts = throttle_strong_runtime_opts, }; static void bdrv_throttle_init(void) diff --git a/block/vpc.c b/block/vpc.c index 52ab717642..a902a4c54d 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -1218,6 +1218,12 @@ static QemuOptsList vpc_create_opts = { } }; +static const char *const vpc_strong_runtime_opts[] = { + VPC_OPT_SIZE_CALC, + + NULL +}; + static BlockDriver bdrv_vpc = { .format_name = "vpc", .instance_size = sizeof(BDRVVPCState), @@ -1238,6 +1244,7 @@ static BlockDriver bdrv_vpc = { .create_opts = &vpc_create_opts, .bdrv_has_zero_init = vpc_has_zero_init, + .strong_runtime_opts = vpc_strong_runtime_opts, }; static void bdrv_vpc_init(void) diff --git a/block/vvfat.c b/block/vvfat.c index b7b61ea8b7..5f66787890 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -3253,6 +3253,16 @@ static void vvfat_close(BlockDriverState *bs) } } +static const char *const vvfat_strong_runtime_opts[] = { + "dir", + "fat-type", + "floppy", + "label", + "rw", + + NULL +}; + static BlockDriver bdrv_vvfat = { .format_name = "vvfat", .protocol_name = "fat", @@ -3267,6 +3277,8 @@ static BlockDriver bdrv_vvfat = { .bdrv_co_preadv = vvfat_co_preadv, .bdrv_co_pwritev = vvfat_co_pwritev, .bdrv_co_block_status = vvfat_co_block_status, + + .strong_runtime_opts = vvfat_strong_runtime_opts, }; static void bdrv_vvfat_init(void) diff --git a/block/vxhs.c b/block/vxhs.c index 0cb0a007e9..2e18229ba4 100644 --- a/block/vxhs.c +++ b/block/vxhs.c @@ -556,6 +556,16 @@ static int64_t vxhs_getlength(BlockDriverState *bs) return vdisk_size; } +static const char *const vxhs_strong_runtime_opts[] = { + VXHS_OPT_VDISK_ID, + "tls-creds", + VXHS_OPT_HOST, + VXHS_OPT_PORT, + VXHS_OPT_SERVER".", + + NULL +}; + static BlockDriver bdrv_vxhs = { .format_name = "vxhs", .protocol_name = "vxhs", @@ -567,6 +577,7 @@ static BlockDriver bdrv_vxhs = { .bdrv_getlength = vxhs_getlength, .bdrv_aio_preadv = vxhs_aio_preadv, .bdrv_aio_pwritev = vxhs_aio_pwritev, + .strong_runtime_opts = vxhs_strong_runtime_opts, }; static void bdrv_vxhs_init(void) diff --git a/include/block/block_int.h b/include/block/block_int.h index e4d4817ea6..160e8cac1f 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -517,6 +517,13 @@ struct BlockDriver { void (*bdrv_register_buf)(BlockDriverState *bs, void *host, size_t size); void (*bdrv_unregister_buf)(BlockDriverState *bs, void *host); QLIST_ENTRY(BlockDriver) list; + + /* Pointer to a NULL-terminated array of names of strong options + * that can be specified for bdrv_open(). A strong option is one + * that changes the data of a BDS. + * If this pointer is NULL, the array is considered empty. + * "filename" and "driver" are always considered strong. */ + const char *const *strong_runtime_opts; }; typedef struct BlockLimits { -- cgit v1.2.3-55-g7522 From abc521a9aa470421dc9285cafe16ff64f3044ac5 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 1 Feb 2019 20:29:26 +0100 Subject: block: Add BlockDriver.bdrv_gather_child_options Some follow-up patches will rework the way bs->full_open_options is refreshed in bdrv_refresh_filename(). The new implementation will remove the need for the block drivers' bdrv_refresh_filename() implementations to set bs->full_open_options; instead, it will be generic and use static information from each block driver. However, by implementing bdrv_gather_child_options(), block drivers will still be able to override the way the full_open_options of their children are incorporated into their own. We need to implement this function for VMDK because we have to prevent the generic implementation from gathering the options of all children: It is not possible to specify options for the extents through the runtime options. For quorum, the child names that would be used by the generic implementation and the ones that we actually (currently) want to use differ. See quorum_gather_child_options() for more information. Note that both of these are cases which are not ideal: In case of VMDK it would probably be nice to be able to specify options for all extents. In case of quorum, the current runtime option structure is simply broken and needs to be fixed (but that is left for another patch). Signed-off-by: Max Reitz Reviewed-by: Alberto Garcia Message-id: 20190201192935.18394-23-mreitz@redhat.com Signed-off-by: Max Reitz --- block/quorum.c | 40 ++++++++++++++++++++++++++++++++++++++++ block/vmdk.c | 19 +++++++++++++++++++ include/block/block_int.h | 24 ++++++++++++++++++++++++ 3 files changed, 83 insertions(+) (limited to 'include') diff --git a/block/quorum.c b/block/quorum.c index 1af6458dc4..3984f0aa4f 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -1094,6 +1094,45 @@ static void quorum_refresh_filename(BlockDriverState *bs, QDict *options) bs->full_open_options = opts; } +static void quorum_gather_child_options(BlockDriverState *bs, QDict *target, + bool backing_overridden) +{ + BDRVQuorumState *s = bs->opaque; + QList *children_list; + int i; + + /* + * The generic implementation for gathering child options in + * bdrv_refresh_filename() would use the names of the children + * as specified for bdrv_open_child() or bdrv_attach_child(), + * which is "children.%u" with %u being a value + * (s->next_child_index) that is incremented each time a new child + * is added (and never decremented). Since children can be + * deleted at runtime, there may be gaps in that enumeration. + * When creating a new quorum BDS and specifying the children for + * it through runtime options, the enumeration used there may not + * have any gaps, though. + * + * Therefore, we have to create a new gap-less enumeration here + * (which we can achieve by simply putting all of the children's + * full_open_options into a QList). + * + * XXX: Note that there are issues with the current child option + * structure quorum uses (such as the fact that children do + * not really have unique permanent names). Therefore, this + * is going to have to change in the future and ideally we + * want quorum to be covered by the generic implementation. + */ + + children_list = qlist_new(); + qdict_put(target, "children", children_list); + + for (i = 0; i < s->num_children; i++) { + qlist_append(children_list, + qobject_ref(s->children[i]->bs->full_open_options)); + } +} + static char *quorum_dirname(BlockDriverState *bs, Error **errp) { /* In general, there are multiple BDSs with different dirnames below this @@ -1121,6 +1160,7 @@ static BlockDriver bdrv_quorum = { .bdrv_open = quorum_open, .bdrv_close = quorum_close, .bdrv_refresh_filename = quorum_refresh_filename, + .bdrv_gather_child_options = quorum_gather_child_options, .bdrv_dirname = quorum_dirname, .bdrv_co_flush_to_disk = quorum_co_flush, diff --git a/block/vmdk.c b/block/vmdk.c index 81c506cb69..91345babb5 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -27,6 +27,7 @@ #include "qapi/error.h" #include "block/block_int.h" #include "sysemu/block-backend.h" +#include "qapi/qmp/qdict.h" #include "qapi/qmp/qerror.h" #include "qemu/error-report.h" #include "qemu/module.h" @@ -2608,6 +2609,23 @@ static int vmdk_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) return 0; } +static void vmdk_gather_child_options(BlockDriverState *bs, QDict *target, + bool backing_overridden) +{ + /* No children but file and backing can be explicitly specified (TODO) */ + qdict_put(target, "file", + qobject_ref(bs->file->bs->full_open_options)); + + if (backing_overridden) { + if (bs->backing) { + qdict_put(target, "backing", + qobject_ref(bs->backing->bs->full_open_options)); + } else { + qdict_put_null(target, "backing"); + } + } +} + static QemuOptsList vmdk_create_opts = { .name = "vmdk-create-opts", .head = QTAILQ_HEAD_INITIALIZER(vmdk_create_opts.head), @@ -2679,6 +2697,7 @@ static BlockDriver bdrv_vmdk = { .bdrv_get_specific_info = vmdk_get_specific_info, .bdrv_refresh_limits = vmdk_refresh_limits, .bdrv_get_info = vmdk_get_info, + .bdrv_gather_child_options = vmdk_gather_child_options, .supports_backing = true, .create_opts = &vmdk_create_opts, diff --git a/include/block/block_int.h b/include/block/block_int.h index 160e8cac1f..ab4cf2df07 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -141,6 +141,30 @@ struct BlockDriver { void (*bdrv_refresh_filename)(BlockDriverState *bs, QDict *options); + /* + * Gathers the open options for all children into @target. + * A simple format driver (without backing file support) might + * implement this function like this: + * + * QINCREF(bs->file->bs->full_open_options); + * qdict_put(target, "file", bs->file->bs->full_open_options); + * + * If not specified, the generic implementation will simply put + * all children's options under their respective name. + * + * @backing_overridden is true when bs->backing seems not to be + * the child that would result from opening bs->backing_file. + * Therefore, if it is true, the backing child's options should be + * gathered; otherwise, there is no need since the backing child + * is the one implied by the image header. + * + * Note that ideally this function would not be needed. Every + * block driver which implements it is probably doing something + * shady regarding its runtime option structure. + */ + void (*bdrv_gather_child_options)(BlockDriverState *bs, QDict *target, + bool backing_overridden); + /* * Returns an allocated string which is the directory name of this BDS: It * will be used to make relative filenames absolute by prepending this -- cgit v1.2.3-55-g7522 From 998b3a1e5a2dd23bf89a853e15fabdaa8d788a72 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 1 Feb 2019 20:29:28 +0100 Subject: block: Purify .bdrv_refresh_filename() Currently, BlockDriver.bdrv_refresh_filename() is supposed to both refresh the filename (BDS.exact_filename) and set BDS.full_open_options. Now that we have generic code in the central bdrv_refresh_filename() for creating BDS.full_open_options, we can drop the latter part from all BlockDriver.bdrv_refresh_filename() implementations. This also means that we can drop all of the existing default code for this from the global bdrv_refresh_filename() itself. Furthermore, we now have to call BlockDriver.bdrv_refresh_filename() after having set BDS.full_open_options, because the block driver's implementation should now be allowed to depend on BDS.full_open_options being set correctly. Finally, with this patch we can drop the @options parameter from BlockDriver.bdrv_refresh_filename(); also, add a comment on this function's purpose in block/block_int.h while touching its interface. This completely obsoletes blklogwrite's implementation of .bdrv_refresh_filename(). Signed-off-by: Max Reitz Message-id: 20190201192935.18394-25-mreitz@redhat.com Signed-off-by: Max Reitz --- block.c | 131 ++++++++------------------------------------- block/blkdebug.c | 54 +++++++------------ block/blklogwrites.c | 22 -------- block/blkverify.c | 16 +----- block/commit.c | 2 +- block/mirror.c | 2 +- block/nbd.c | 23 +------- block/nfs.c | 36 +------------ block/null.c | 22 +++++--- block/nvme.c | 22 +++++--- block/quorum.c | 30 ----------- include/block/block_int.h | 6 ++- tests/qemu-iotests/228 | 4 -- tests/qemu-iotests/228.out | 2 +- 14 files changed, 81 insertions(+), 291 deletions(-) (limited to 'include') diff --git a/block.c b/block.c index a6e523059d..23869623ea 100644 --- a/block.c +++ b/block.c @@ -5621,33 +5621,6 @@ static bool append_strong_runtime_options(QDict *d, BlockDriverState *bs) return found_any; } -static bool append_open_options(QDict *d, BlockDriverState *bs) -{ - const QDictEntry *entry; - QemuOptDesc *desc; - bool found_any = false; - - for (entry = qdict_first(bs->options); entry; - entry = qdict_next(bs->options, entry)) - { - /* Exclude all non-driver-specific options */ - for (desc = bdrv_runtime_opts.desc; desc->name; desc++) { - if (!strcmp(qdict_entry_key(entry), desc->name)) { - break; - } - } - if (desc->name) { - continue; - } - - qdict_put_obj(d, qdict_entry_key(entry), - qobject_ref(qdict_entry_value(entry))); - found_any = true; - } - - return found_any; -} - /* Note: This function may return false positives; it may return true * even if opening the backing file specified by bs's image header * would result in exactly bs->backing. */ @@ -5681,6 +5654,8 @@ void bdrv_refresh_filename(BlockDriverState *bs) BdrvChild *child; QDict *opts; bool backing_overridden; + bool generate_json_filename; /* Whether our default implementation should + fill exact_filename (false) or not (true) */ if (!drv) { return; @@ -5716,90 +5691,10 @@ void bdrv_refresh_filename(BlockDriverState *bs) backing_overridden = false; } - if (drv->bdrv_refresh_filename) { - /* Obsolete information is of no use here, so drop the old file name - * information before refreshing it */ - bs->exact_filename[0] = '\0'; - if (bs->full_open_options) { - qobject_unref(bs->full_open_options); - bs->full_open_options = NULL; - } - - opts = qdict_new(); - append_open_options(opts, bs); - drv->bdrv_refresh_filename(bs, opts); - qobject_unref(opts); - } else if (bs->file) { - /* Try to reconstruct valid information from the underlying file */ - bool has_open_options; - - bs->exact_filename[0] = '\0'; - if (bs->full_open_options) { - qobject_unref(bs->full_open_options); - bs->full_open_options = NULL; - } - - opts = qdict_new(); - has_open_options = append_open_options(opts, bs); - has_open_options |= backing_overridden; - - /* If no specific options have been given for this BDS, the filename of - * the underlying file should suffice for this one as well */ - if (bs->file->bs->exact_filename[0] && !has_open_options) { - strcpy(bs->exact_filename, bs->file->bs->exact_filename); - } - /* Reconstructing the full options QDict is simple for most format block - * drivers, as long as the full options are known for the underlying - * file BDS. The full options QDict of that file BDS should somehow - * contain a representation of the filename, therefore the following - * suffices without querying the (exact_)filename of this BDS. */ - if (bs->file->bs->full_open_options && - (!bs->backing || bs->backing->bs->full_open_options)) - { - qdict_put_str(opts, "driver", drv->format_name); - qdict_put(opts, "file", - qobject_ref(bs->file->bs->full_open_options)); - - if (bs->backing) { - qdict_put(opts, "backing", - qobject_ref(bs->backing->bs->full_open_options)); - } else if (backing_overridden) { - qdict_put_null(opts, "backing"); - } - - bs->full_open_options = opts; - } else { - qobject_unref(opts); - } - } else if (!bs->full_open_options && qdict_size(bs->options)) { - /* There is no underlying file BDS (at least referenced by BDS.file), - * so the full options QDict should be equal to the options given - * specifically for this block device when it was opened (plus the - * driver specification). - * Because those options don't change, there is no need to update - * full_open_options when it's already set. */ - - opts = qdict_new(); - append_open_options(opts, bs); - qdict_put_str(opts, "driver", drv->format_name); - - if (bs->exact_filename[0]) { - /* This may not work for all block protocol drivers (some may - * require this filename to be parsed), but we have to find some - * default solution here, so just include it. If some block driver - * does not support pure options without any filename at all or - * needs some special format of the options QDict, it needs to - * implement the driver-specific bdrv_refresh_filename() function. - */ - qdict_put_str(opts, "filename", bs->exact_filename); - } - - bs->full_open_options = opts; - } - /* Gather the options QDict */ opts = qdict_new(); - append_strong_runtime_options(opts, bs); + generate_json_filename = append_strong_runtime_options(opts, bs); + generate_json_filename |= backing_overridden; if (drv->bdrv_gather_child_options) { /* Some block drivers may not want to present all of their children's @@ -5825,6 +5720,24 @@ void bdrv_refresh_filename(BlockDriverState *bs) qobject_unref(bs->full_open_options); bs->full_open_options = opts; + if (drv->bdrv_refresh_filename) { + /* Obsolete information is of no use here, so drop the old file name + * information before refreshing it */ + bs->exact_filename[0] = '\0'; + + drv->bdrv_refresh_filename(bs); + } else if (bs->file) { + /* Try to reconstruct valid information from the underlying file */ + + bs->exact_filename[0] = '\0'; + + /* If no specific options have been given for this BDS, the filename of + * the underlying file should suffice for this one as well */ + if (bs->file->bs->exact_filename[0] && !generate_json_filename) { + strcpy(bs->exact_filename, bs->file->bs->exact_filename); + } + } + if (bs->exact_filename[0]) { pstrcpy(bs->filename, sizeof(bs->filename), bs->exact_filename); } else { diff --git a/block/blkdebug.c b/block/blkdebug.c index 71b4275b98..1ea835c2b9 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -811,51 +811,37 @@ static int64_t blkdebug_getlength(BlockDriverState *bs) return bdrv_getlength(bs->file->bs); } -static void blkdebug_refresh_filename(BlockDriverState *bs, QDict *options) +static void blkdebug_refresh_filename(BlockDriverState *bs) { BDRVBlkdebugState *s = bs->opaque; - QDict *opts; const QDictEntry *e; - bool force_json = false; - - for (e = qdict_first(options); e; e = qdict_next(options, e)) { - if (strcmp(qdict_entry_key(e), "config") && - strcmp(qdict_entry_key(e), "x-image")) - { - force_json = true; - break; - } - } + int ret; - if (force_json && !bs->file->bs->full_open_options) { - /* The config file cannot be recreated, so creating a plain filename - * is impossible */ + if (!bs->file->bs->exact_filename[0]) { return; } - if (!force_json && bs->file->bs->exact_filename[0]) { - int ret = snprintf(bs->exact_filename, sizeof(bs->exact_filename), - "blkdebug:%s:%s", s->config_file ?: "", - bs->file->bs->exact_filename); - if (ret >= sizeof(bs->exact_filename)) { - /* An overflow makes the filename unusable, so do not report any */ - bs->exact_filename[0] = 0; + for (e = qdict_first(bs->full_open_options); e; + e = qdict_next(bs->full_open_options, e)) + { + /* Real child options are under "image", but "x-image" may + * contain a filename */ + if (strcmp(qdict_entry_key(e), "config") && + strcmp(qdict_entry_key(e), "image") && + strcmp(qdict_entry_key(e), "x-image") && + strcmp(qdict_entry_key(e), "driver")) + { + return; } } - opts = qdict_new(); - qdict_put_str(opts, "driver", "blkdebug"); - - qdict_put(opts, "image", qobject_ref(bs->file->bs->full_open_options)); - - for (e = qdict_first(options); e; e = qdict_next(options, e)) { - if (strcmp(qdict_entry_key(e), "x-image")) { - qdict_put_obj(opts, qdict_entry_key(e), - qobject_ref(qdict_entry_value(e))); - } + ret = snprintf(bs->exact_filename, sizeof(bs->exact_filename), + "blkdebug:%s:%s", + s->config_file ?: "", bs->file->bs->exact_filename); + if (ret >= sizeof(bs->exact_filename)) { + /* An overflow makes the filename unusable, so do not report any */ + bs->exact_filename[0] = 0; } - - bs->full_open_options = opts; } static void blkdebug_refresh_limits(BlockDriverState *bs, Error **errp) diff --git a/block/blklogwrites.c b/block/blklogwrites.c index 5da5df112d..eb2b4901a5 100644 --- a/block/blklogwrites.c +++ b/block/blklogwrites.c @@ -280,27 +280,6 @@ static int64_t blk_log_writes_getlength(BlockDriverState *bs) return bdrv_getlength(bs->file->bs); } -static void blk_log_writes_refresh_filename(BlockDriverState *bs, - QDict *options) -{ - BDRVBlkLogWritesState *s = bs->opaque; - - if (bs->file->bs->full_open_options - && s->log_file->bs->full_open_options) - { - QDict *opts = qdict_new(); - qdict_put_str(opts, "driver", "blklogwrites"); - - qobject_ref(bs->file->bs->full_open_options); - qdict_put(opts, "file", bs->file->bs->full_open_options); - qobject_ref(s->log_file->bs->full_open_options); - qdict_put(opts, "log", s->log_file->bs->full_open_options); - qdict_put_int(opts, "log-sector-size", s->sectorsize); - - bs->full_open_options = opts; - } -} - static void blk_log_writes_child_perm(BlockDriverState *bs, BdrvChild *c, const BdrvChildRole *role, BlockReopenQueue *ro_q, @@ -531,7 +510,6 @@ static BlockDriver bdrv_blk_log_writes = { .bdrv_open = blk_log_writes_open, .bdrv_close = blk_log_writes_close, .bdrv_getlength = blk_log_writes_getlength, - .bdrv_refresh_filename = blk_log_writes_refresh_filename, .bdrv_child_perm = blk_log_writes_child_perm, .bdrv_refresh_limits = blk_log_writes_refresh_limits, diff --git a/block/blkverify.c b/block/blkverify.c index 3c7d4c8729..3ff77ff49a 100644 --- a/block/blkverify.c +++ b/block/blkverify.c @@ -281,24 +281,10 @@ static bool blkverify_recurse_is_first_non_filter(BlockDriverState *bs, return bdrv_recurse_is_first_non_filter(s->test_file->bs, candidate); } -static void blkverify_refresh_filename(BlockDriverState *bs, QDict *options) +static void blkverify_refresh_filename(BlockDriverState *bs) { BDRVBlkverifyState *s = bs->opaque; - if (bs->file->bs->full_open_options - && s->test_file->bs->full_open_options) - { - QDict *opts = qdict_new(); - qdict_put_str(opts, "driver", "blkverify"); - - qdict_put(opts, "raw", - qobject_ref(bs->file->bs->full_open_options)); - qdict_put(opts, "test", - qobject_ref(s->test_file->bs->full_open_options)); - - bs->full_open_options = opts; - } - if (bs->file->bs->exact_filename[0] && s->test_file->bs->exact_filename[0]) { diff --git a/block/commit.c b/block/commit.c index 614a8ca374..385fb98527 100644 --- a/block/commit.c +++ b/block/commit.c @@ -230,7 +230,7 @@ static int coroutine_fn bdrv_commit_top_preadv(BlockDriverState *bs, return bdrv_co_preadv(bs->backing, offset, bytes, qiov, flags); } -static void bdrv_commit_top_refresh_filename(BlockDriverState *bs, QDict *opts) +static void bdrv_commit_top_refresh_filename(BlockDriverState *bs) { pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), bs->backing->bs->filename); diff --git a/block/mirror.c b/block/mirror.c index 031c1aeaeb..726d3c27fb 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -1431,7 +1431,7 @@ static int coroutine_fn bdrv_mirror_top_pdiscard(BlockDriverState *bs, NULL, 0); } -static void bdrv_mirror_top_refresh_filename(BlockDriverState *bs, QDict *opts) +static void bdrv_mirror_top_refresh_filename(BlockDriverState *bs) { if (bs->backing == NULL) { /* we can be here after failed bdrv_attach_child in diff --git a/block/nbd.c b/block/nbd.c index 318a58776c..2e72df528a 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -477,12 +477,9 @@ static void nbd_attach_aio_context(BlockDriverState *bs, nbd_client_attach_aio_context(bs, new_context); } -static void nbd_refresh_filename(BlockDriverState *bs, QDict *options) +static void nbd_refresh_filename(BlockDriverState *bs) { BDRVNBDState *s = bs->opaque; - QDict *opts = qdict_new(); - QObject *saddr_qdict; - Visitor *ov; const char *host = NULL, *port = NULL, *path = NULL; if (s->saddr->type == SOCKET_ADDRESS_TYPE_INET) { @@ -495,8 +492,6 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options) path = s->saddr->u.q_unix.path; } /* else can't represent as pseudo-filename */ - qdict_put_str(opts, "driver", "nbd"); - if (path && s->export) { snprintf(bs->exact_filename, sizeof(bs->exact_filename), "nbd+unix:///%s?socket=%s", s->export, path); @@ -510,22 +505,6 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options) snprintf(bs->exact_filename, sizeof(bs->exact_filename), "nbd://%s:%s", host, port); } - - ov = qobject_output_visitor_new(&saddr_qdict); - visit_type_SocketAddress(ov, NULL, &s->saddr, &error_abort); - visit_complete(ov, &saddr_qdict); - visit_free(ov); - qdict_put_obj(opts, "server", saddr_qdict); - - if (s->export) { - qdict_put_str(opts, "export", s->export); - } - if (s->tlscredsid) { - qdict_put_str(opts, "tls-creds", s->tlscredsid); - } - - qdict_flatten(opts); - bs->full_open_options = opts; } static char *nbd_dirname(BlockDriverState *bs, Error **errp) diff --git a/block/nfs.c b/block/nfs.c index 6985a44b89..531903610b 100644 --- a/block/nfs.c +++ b/block/nfs.c @@ -799,14 +799,9 @@ static int nfs_reopen_prepare(BDRVReopenState *state, return 0; } -static void nfs_refresh_filename(BlockDriverState *bs, QDict *options) +static void nfs_refresh_filename(BlockDriverState *bs) { NFSClient *client = bs->opaque; - QDict *opts = qdict_new(); - QObject *server_qdict; - Visitor *ov; - - qdict_put_str(opts, "driver", "nfs"); if (client->uid && !client->gid) { snprintf(bs->exact_filename, sizeof(bs->exact_filename), @@ -824,35 +819,6 @@ static void nfs_refresh_filename(BlockDriverState *bs, QDict *options) snprintf(bs->exact_filename, sizeof(bs->exact_filename), "nfs://%s%s", client->server->host, client->path); } - - ov = qobject_output_visitor_new(&server_qdict); - visit_type_NFSServer(ov, NULL, &client->server, &error_abort); - visit_complete(ov, &server_qdict); - qdict_put_obj(opts, "server", server_qdict); - qdict_put_str(opts, "path", client->path); - - if (client->uid) { - qdict_put_int(opts, "user", client->uid); - } - if (client->gid) { - qdict_put_int(opts, "group", client->gid); - } - if (client->tcp_syncnt) { - qdict_put_int(opts, "tcp-syn-cnt", client->tcp_syncnt); - } - if (client->readahead) { - qdict_put_int(opts, "readahead-size", client->readahead); - } - if (client->pagecache) { - qdict_put_int(opts, "page-cache-size", client->pagecache); - } - if (client->debug) { - qdict_put_int(opts, "debug", client->debug); - } - - visit_free(ov); - qdict_flatten(opts); - bs->full_open_options = opts; } static char *nfs_dirname(BlockDriverState *bs, Error **errp) diff --git a/block/null.c b/block/null.c index 858892f0c4..1c56a0ef01 100644 --- a/block/null.c +++ b/block/null.c @@ -239,17 +239,23 @@ static int coroutine_fn null_co_block_status(BlockDriverState *bs, return ret; } -static void null_refresh_filename(BlockDriverState *bs, QDict *opts) +static void null_refresh_filename(BlockDriverState *bs) { - qdict_del(opts, "filename"); - - if (!qdict_size(opts)) { - snprintf(bs->exact_filename, sizeof(bs->exact_filename), "%s://", - bs->drv->format_name); + const QDictEntry *e; + + for (e = qdict_first(bs->full_open_options); e; + e = qdict_next(bs->full_open_options, e)) + { + /* These options can be ignored */ + if (strcmp(qdict_entry_key(e), "filename") && + strcmp(qdict_entry_key(e), "driver")) + { + return; + } } - qdict_put_str(opts, "driver", bs->drv->format_name); - bs->full_open_options = qobject_ref(opts); + snprintf(bs->exact_filename, sizeof(bs->exact_filename), "%s://", + bs->drv->format_name); } static const char *const null_strong_runtime_opts[] = { diff --git a/block/nvme.c b/block/nvme.c index bf656b2bba..6b5845644b 100644 --- a/block/nvme.c +++ b/block/nvme.c @@ -1053,17 +1053,23 @@ static int nvme_reopen_prepare(BDRVReopenState *reopen_state, return 0; } -static void nvme_refresh_filename(BlockDriverState *bs, QDict *opts) +static void nvme_refresh_filename(BlockDriverState *bs) { - qdict_del(opts, "filename"); - - if (!qdict_size(opts)) { - snprintf(bs->exact_filename, sizeof(bs->exact_filename), "%s://", - bs->drv->format_name); + const QDictEntry *e; + + for (e = qdict_first(bs->full_open_options); e; + e = qdict_next(bs->full_open_options, e)) + { + /* These options can be ignored */ + if (strcmp(qdict_entry_key(e), "filename") && + strcmp(qdict_entry_key(e), "driver")) + { + return; + } } - qdict_put_str(opts, "driver", bs->drv->format_name); - bs->full_open_options = qobject_ref(opts); + snprintf(bs->exact_filename, sizeof(bs->exact_filename), "%s://", + bs->drv->format_name); } static void nvme_refresh_limits(BlockDriverState *bs, Error **errp) diff --git a/block/quorum.c b/block/quorum.c index 3984f0aa4f..352f729136 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -1065,35 +1065,6 @@ static void quorum_del_child(BlockDriverState *bs, BdrvChild *child, bdrv_drained_end(bs); } -static void quorum_refresh_filename(BlockDriverState *bs, QDict *options) -{ - BDRVQuorumState *s = bs->opaque; - QDict *opts; - QList *children; - int i; - - for (i = 0; i < s->num_children; i++) { - if (!s->children[i]->bs->full_open_options) { - return; - } - } - - children = qlist_new(); - for (i = 0; i < s->num_children; i++) { - qlist_append(children, - qobject_ref(s->children[i]->bs->full_open_options)); - } - - opts = qdict_new(); - qdict_put_str(opts, "driver", "quorum"); - qdict_put_int(opts, QUORUM_OPT_VOTE_THRESHOLD, s->threshold); - qdict_put_bool(opts, QUORUM_OPT_BLKVERIFY, s->is_blkverify); - qdict_put_bool(opts, QUORUM_OPT_REWRITE, s->rewrite_corrupted); - qdict_put(opts, "children", children); - - bs->full_open_options = opts; -} - static void quorum_gather_child_options(BlockDriverState *bs, QDict *target, bool backing_overridden) { @@ -1159,7 +1130,6 @@ static BlockDriver bdrv_quorum = { .bdrv_open = quorum_open, .bdrv_close = quorum_close, - .bdrv_refresh_filename = quorum_refresh_filename, .bdrv_gather_child_options = quorum_gather_child_options, .bdrv_dirname = quorum_dirname, diff --git a/include/block/block_int.h b/include/block/block_int.h index ab4cf2df07..836d67c1ae 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -139,7 +139,11 @@ struct BlockDriver { Error **errp); int (*bdrv_make_empty)(BlockDriverState *bs); - void (*bdrv_refresh_filename)(BlockDriverState *bs, QDict *options); + /* + * Refreshes the bs->exact_filename field. If that is impossible, + * bs->exact_filename has to be left empty. + */ + void (*bdrv_refresh_filename)(BlockDriverState *bs); /* * Gathers the open options for all children into @target. diff --git a/tests/qemu-iotests/228 b/tests/qemu-iotests/228 index 7a57c41e29..9a50afd205 100755 --- a/tests/qemu-iotests/228 +++ b/tests/qemu-iotests/228 @@ -230,10 +230,6 @@ with iotests.FilePath('base.img') as base_img_path, \ overlay='node0') # This should give us the original plain result - # FIXME: Currently, it yields a json:{} filename even though it - # only contains a @driver and a @file entry, so a plain - # filename would obviously suffice. This is fixed by a - # future patch. log_node_info(vm.node_info('node0')) diff --git a/tests/qemu-iotests/228.out b/tests/qemu-iotests/228.out index 393ab09164..4217df24fe 100644 --- a/tests/qemu-iotests/228.out +++ b/tests/qemu-iotests/228.out @@ -74,7 +74,7 @@ bs->backing: (none) {"execute": "blockdev-snapshot", "arguments": {"node": "original-backing", "overlay": "node0"}} {"return": {}} -bs->filename: json:{"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-top.img"}} +bs->filename: TEST_DIR/PID-top.img bs->backing_file: TEST_DIR/PID-base.img bs->backing->bs->filename: TEST_DIR/PID-base.img -- cgit v1.2.3-55-g7522