- 25 2月, 2019 40 次提交
-
-
由 Max Reitz 提交于
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: NMax Reitz <mreitz@redhat.com> Reviewed-by: NAlberto Garcia <berto@igalia.com> Message-id: 20190201192935.18394-15-mreitz@redhat.com Signed-off-by: NMax Reitz <mreitz@redhat.com>
-
由 Max Reitz 提交于
bdrv_find_backing_image() should use bdrv_get_full_backing_filename() or bdrv_make_absolute_filename() instead of trying to do what those functions do by itself. path_combine_deprecated() can now be dropped, so let's do that. Signed-off-by: NMax Reitz <mreitz@redhat.com> Reviewed-by: NAlberto Garcia <berto@igalia.com> Message-id: 20190201192935.18394-14-mreitz@redhat.com Signed-off-by: NMax Reitz <mreitz@redhat.com>
-
由 Max Reitz 提交于
This is a general function for making a filename that is relative to a certain BDS absolute. It calls bdrv_get_full_backing_filename_from_filename() for now, but that will be changed in a follow-up patch. Signed-off-by: NMax Reitz <mreitz@redhat.com> Reviewed-by: NAlberto Garcia <berto@igalia.com> Message-id: 20190201192935.18394-13-mreitz@redhat.com Signed-off-by: NMax Reitz <mreitz@redhat.com>
-
由 Max Reitz 提交于
Make bdrv_get_full_backing_filename() return an allocated string instead of placing the result in a caller-provided buffer. Signed-off-by: NMax Reitz <mreitz@redhat.com> Reviewed-by: NAlberto Garcia <berto@igalia.com> Message-id: 20190201192935.18394-12-mreitz@redhat.com Signed-off-by: NMax Reitz <mreitz@redhat.com>
-
由 Max Reitz 提交于
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: NMax Reitz <mreitz@redhat.com> Message-id: 20190201192935.18394-11-mreitz@redhat.com Signed-off-by: NMax Reitz <mreitz@redhat.com>
-
由 Max Reitz 提交于
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: NMax Reitz <mreitz@redhat.com> Reviewed-by: NAlberto Garcia <berto@igalia.com> Reviewed-by: NKevin Wolf <kwolf@redhat.com> Message-id: 20190201192935.18394-10-mreitz@redhat.com Signed-off-by: NMax Reitz <mreitz@redhat.com>
-
由 Max Reitz 提交于
Signed-off-by: NMax Reitz <mreitz@redhat.com> Message-id: 20190201192935.18394-9-mreitz@redhat.com Signed-off-by: NMax Reitz <mreitz@redhat.com>
-
由 Max Reitz 提交于
This function queries a node; since we cannot do that right now, it executes query-named-block-nodes and returns the matching node's object. Signed-off-by: NMax Reitz <mreitz@redhat.com> Reviewed-by: NJohn Snow <jsnow@redhat.com> Reviewed-by: NAlberto Garcia <berto@igalia.com> Reviewed-by: NEric Blake <eblake@redhat.com> Message-id: 20190201192935.18394-8-mreitz@redhat.com Signed-off-by: NMax Reitz <mreitz@redhat.com>
-
由 Max Reitz 提交于
Signed-off-by: NMax Reitz <mreitz@redhat.com> Message-id: 20190201192935.18394-7-mreitz@redhat.com Reviewed-by: NEric Blake <eblake@redhat.com> Signed-off-by: NMax Reitz <mreitz@redhat.com>
-
由 Max Reitz 提交于
Basically, bdrv_refresh_filename() should respect all children of a BlockDriverState. However, generally those children are driver-specific, so this function cannot handle the general case. On the other hand, there are only few drivers which use other children than @file and @backing (that being vmdk, quorum, and blkverify). Most block drivers only use @file and/or @backing (if they use any children at all). Both can be implemented directly in bdrv_refresh_filename. The user overriding the file's filename is already handled, however, the user overriding the backing file is not. If this is done, opening the BDS with the plain filename of its file will not be correct, so we may not set bs->exact_filename in that case. iotest 051 contains test cases for overriding the backing file, and so its output changes with this patch applied. Signed-off-by: NMax Reitz <mreitz@redhat.com> Reviewed-by: NAlberto Garcia <berto@igalia.com> Message-id: 20190201192935.18394-6-mreitz@redhat.com Signed-off-by: NMax Reitz <mreitz@redhat.com>
-
由 Max Reitz 提交于
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: NMax Reitz <mreitz@redhat.com> Reviewed-by: NEric Blake <eblake@redhat.com> Reviewed-by: NAlberto Garcia <berto@igalia.com> Message-id: 20190201192935.18394-5-mreitz@redhat.com Signed-off-by: NMax Reitz <mreitz@redhat.com>
-
由 Max Reitz 提交于
bdrv_refresh_filename() should simply skip all implicit nodes. They are supposed to be invisible to the user, so they should not appear in filename information. Signed-off-by: NMax Reitz <mreitz@redhat.com> Reviewed-by: NEric Blake <eblake@redhat.com> Reviewed-by: NAlberto Garcia <berto@igalia.com> Message-id: 20190201192935.18394-4-mreitz@redhat.com Signed-off-by: NMax Reitz <mreitz@redhat.com>
-
由 Max Reitz 提交于
bdrv_refresh_filename() should invoke itself recursively on all children, not just on file. With that change, we can remove the manual invocations in blkverify, quorum, commit, mirror, and blklogwrites. Signed-off-by: NMax Reitz <mreitz@redhat.com> Reviewed-by: NEric Blake <eblake@redhat.com> Reviewed-by: NAlberto Garcia <berto@igalia.com> Message-id: 20190201192935.18394-3-mreitz@redhat.com Signed-off-by: NMax Reitz <mreitz@redhat.com>
-
由 Max Reitz 提交于
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: NMax Reitz <mreitz@redhat.com> Message-id: 20190201192935.18394-2-mreitz@redhat.com Reviewed-by: NEric Blake <eblake@redhat.com> Signed-off-by: NMax Reitz <mreitz@redhat.com>
-
由 Thomas Huth 提交于
The QEMU_PACKED is causing a compiler warning/error with GCC 9: CC block/nvme.o block/nvme.c: In function ‘nvme_create_queue_pair’: block/nvme.c:209:22: error: taking address of packed member of ‘struct <anonymous>’ may result in an unaligned pointer value [-Werror=address-of-packed-member] 209 | q->sq.doorbell = &s->regs->doorbells[idx * 2 * s->doorbell_scale]; All members of the struct are naturally aligned, so there should not be the need for QEMU_PACKED here, and the following QEMU_BUILD_BUG_ON also ensures that there is no padding. Thus simply remove the QEMU_PACKED here. Buglink: https://bugs.launchpad.net/qemu/+bug/1817525Reported-by: NSatheesh Rajendran <sathnaga@linux.vnet.ibm.com> Signed-off-by: NThomas Huth <thuth@redhat.com> Reviewed-by: NPeter Maydell <peter.maydell@linaro.org> Signed-off-by: NKevin Wolf <kwolf@redhat.com>
-
由 Alberto Garcia 提交于
L1 table entries have a field to store the offset of an L2 table. The rest of the bits of the entry are currently reserved except from bit 63, which stores the COPIED flag. The offset is always taken from the entry using L1E_OFFSET_MASK to ensure that we only use the bits that belong to that field. While that mask is used every time we read from the L1 table, it is never used when we write to it. Due to the limits set elsewhere in the code QEMU can never produce L2 table offsets that don't fit in that field so any such offset when allocating an L2 table would indicate a bug in QEMU. Signed-off-by: NAlberto Garcia <berto@igalia.com> Reviewed-by: NEric Blake <eblake@redhat.com> Signed-off-by: NKevin Wolf <kwolf@redhat.com>
-
Add two tests of node graph modification. Signed-off-by: NVladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: NKevin Wolf <kwolf@redhat.com>
-
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: NVladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: NKevin Wolf <kwolf@redhat.com>
-
As it already said in the comment, we don't want to create loops in parent->child relations. So, when we try to append @to to @c, we should check that @c is not in @to children subtree, and we should check it recursively, not only the first level. The patch provides BFS-based search, to check the relations. This is needed for further fleecing-hook filter usage: we need to append it to source, when the hook is already a parent of target, and source may be in a backing chain of target (fleecing-scheme). So, on appending, the hook should not became a child (direct or through children subtree) of the target. Signed-off-by: NVladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: NKevin Wolf <kwolf@redhat.com>
-
由 Kevin Wolf 提交于
aio_poll() has an existing assertion that the function is only called from the AioContext's home thread if blocking is allowed. This is not enough, some handlers make assumptions about the thread they run in. Extend the assertion to non-blocking calls, too. Signed-off-by: NKevin Wolf <kwolf@redhat.com> Reviewed-by: NEric Blake <eblake@redhat.com>
-
由 Kevin Wolf 提交于
Now that bdrv_set_aio_context() works inside drained sections, it can also use the real drain function instead of open coding something similar. Signed-off-by: NKevin Wolf <kwolf@redhat.com>
-
由 Kevin Wolf 提交于
Signed-off-by: NKevin Wolf <kwolf@redhat.com> Reviewed-by: NEric Blake <eblake@redhat.com>
-
由 Kevin Wolf 提交于
When a drained node changes its AioContext, we need to move its aio_disable_external() to the new context, too. Without this fix, drain_end will try to reenable the new context, which has never been disabled, so an assertion failure is triggered. Signed-off-by: NKevin Wolf <kwolf@redhat.com> Reviewed-by: NEric Blake <eblake@redhat.com>
-
由 Kevin Wolf 提交于
The explicit aio_poll() call in bdrv_set_aio_context() was added in commit c2b6428d as a workaround for bdrv_drain() failing to achieve to actually quiesce everything (specifically the NBD client code to switch AioContext). Now that the NBD client has been fixed to complete this operation during bdrv_drain(), we don't need the workaround any more. It was wrong anyway: aio_poll() must always be run in the home thread of the AioContext. Signed-off-by: NKevin Wolf <kwolf@redhat.com> Reviewed-by: NEric Blake <eblake@redhat.com>
-
由 Kevin Wolf 提交于
bdrv_drain() must not leave connection_co scheduled, so bs->in_flight needs to be increased while the coroutine is waiting to be scheduled in the new AioContext after nbd_client_attach_aio_context(). Signed-off-by: NKevin Wolf <kwolf@redhat.com>
-
由 Kevin Wolf 提交于
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: NKevin Wolf <kwolf@redhat.com> Reviewed-by: NEric Blake <eblake@redhat.com>
-
由 Kevin Wolf 提交于
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: NKevin Wolf <kwolf@redhat.com> Reviewed-by: NEric Blake <eblake@redhat.com>
-
由 Kevin Wolf 提交于
qio_channel_yield() now updates ioc->read_write/coroutine and calls qio_channel_set_aio_fd_handlers(), so the code in the handlers has become redundant and can be removed. This does not make a difference in intermediate states because aio_co_wake() really enters the coroutine immediately here: These handlers are never run in coroutine context, and we're in the right AioContext because qio_channel_attach_aio_context() asserts that the handlers are inactive. To make these conditions more obvious, assert the right AioContext. Suggested-by: NPaolo Bonzini <pbonzini@redhat.com> Signed-off-by: NKevin Wolf <kwolf@redhat.com>
-
由 Kevin Wolf 提交于
Similar to how qemu_co_sleep_ns() allows preemption from an external coroutine entry, allow reentering qio_channel_yield() early. Signed-off-by: NKevin Wolf <kwolf@redhat.com>
-
由 Kevin Wolf 提交于
nbd_client_attach_aio_context() schedules connection_co in the new AioContext and this way reenters it in any arbitrary place that has yielded. We can restrict this a bit to the function call where the coroutine actually sits waiting when it's idle. This doesn't solve any bug yet, but it shows where in the code we need to support this random reentrance and where we don't have to care. Add FIXME comments for the existing bugs that the rest of this series will fix. Signed-off-by: NKevin Wolf <kwolf@redhat.com> Reviewed-by: NEric Blake <eblake@redhat.com>
-
由 Kevin Wolf 提交于
virtio_blk_dma_restart_bh() submits new requests, so in order to make sure that these requests are not started inside a drained section of the attached BlockBackend, we need to make sure that draining the BlockBackend waits for the BH to be executed. This BH is still questionable because its scheduled in the main thread instead of the configured iothread. Leave a FIXME comment for this. But with this fix, enabling the data plane at least waits for these requests (in bdrv_set_aio_context()) instead of changing the AioContext under their feet and making them run in the wrong thread, causing crashes and failures (e.g. due to missing locking). Signed-off-by: NKevin Wolf <kwolf@redhat.com>
-
由 Kevin Wolf 提交于
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: NKevin Wolf <kwolf@redhat.com>
-
由 Daniel P. Berrangé 提交于
Error reporting for user_creatable_add_opts_foreach was changed so that it no longer called 'error_report_err' in: commit 7e1e0c11 Author: Markus Armbruster <armbru@redhat.com> Date: Wed Oct 17 10:26:43 2018 +0200 qom: Clean up error reporting in user_creatable_add_opts_foreach() Some callers were updated to pass in "&error_fatal" but all the ones in qemu-img were left passing NULL. As a result all errors went to /dev/null instead of being reported to the user. Signed-off-by: NDaniel P. Berrangé <berrange@redhat.com> Reviewed-by: NPhilippe Mathieu-Daudé <philmd@redhat.com> Reviewed-by: NMarkus Armbruster <armbru@redhat.com> Reviewed-by: NStefano Garzarella <sgarzare@redhat.com> Signed-off-by: NKevin Wolf <kwolf@redhat.com>
-
由 Alberto Garcia 提交于
If there's an error in commit_start() then the block job must be deleted before replacing commit_top_bs, otherwise it will fail because of lack of permissions. This happens since the permission system was introduced in 8dfba279. Fortunately this bug doesn't seem to be possible to reproduce at the moment without changing the code. Signed-off-by: NAlberto Garcia <berto@igalia.com> Signed-off-by: NKevin Wolf <kwolf@redhat.com>
-
由 Denis Plotnikov 提交于
Adds a fast path on aio context setting preventing unnecessary context setting routine. Also, it prevents issues with cyclic walk of child bds-es appeared because of registering aio walking notifiers: Call stack: 0 __GI_raise 1 __GI_abort 2 __assert_fail_base 3 __GI___assert_fail 4 bdrv_detach_aio_context (bs=0x55f54d65c000) <<< 5 bdrv_detach_aio_context (bs=0x55f54fc8a800) 6 bdrv_set_aio_context (bs=0x55f54fc8a800, ...) 7 block_job_attached_aio_context 8 bdrv_attach_aio_context (bs=0x55f54d65c000, ...) <<< 9 bdrv_set_aio_context (bs=0x55f54d65c000) 10 blk_set_aio_context 11 virtio_blk_data_plane_stop 12 virtio_bus_stop_ioeventfd 13 virtio_vmstate_change 14 vm_state_notify (running=0, state=RUN_STATE_SHUTDOWN) 15 do_vm_stop (state=RUN_STATE_SHUTDOWN, send_stop=true) 16 vm_stop (state=RUN_STATE_SHUTDOWN) 17 main_loop_should_exit 18 main_loop 19 main This can happen because of "new" context attachment to VM disk bds. When attaching a new context the corresponding aio context handler is called for each of aio_notifiers registered on the VM disk bds context. Among those handlers, there is the block_job_attached_aio_context handler which sets a new aio context for the block job bds. When doing so, the old context is detached from all the block job bds children and one of them is the VM disk bds, serving as backing store for the blockjob bds, although the VM disk bds is actually the initializer of that process. Since the VM disk bds is protected with walking_aio_notifiers flag from double processing in recursive calls, the assert fires. Signed-off-by: NDenis Plotnikov <dplotnikov@virtuozzo.com> Signed-off-by: NKevin Wolf <kwolf@redhat.com>
-
由 Daniel Henrique Barboza 提交于
In qcow2_snapshot_create there is the following code block: /* Generate an ID */ find_new_snapshot_id(bs, sn_info->id_str, sizeof(sn_info->id_str)); /* Check that the ID is unique */ if (find_snapshot_by_id_and_name(bs, sn_info->id_str, NULL) >= 0) { return -EEXIST; } find_new_snapshot_id cycles through all snapshots, getting the id_str as an unsigned long int, calculating the max id_max value of all the existing id_strs and writing in the id_str pointer id_max + 1: for(i = 0; i < s->nb_snapshots; i++) { sn = s->snapshots + i; id = strtoul(sn->id_str, NULL, 10); if (id > id_max) id_max = id; } snprintf(id_str, id_str_size, "%lu", id_max + 1); Here, sn_info->id_str will have the unique value id_max + 1. Right after that, find_snapshot_by_id_and_name is called with id = sn_info->id_str and name = NULL. This will cause the function to execute the following: } else if (id) { for (i = 0; i < s->nb_snapshots; i++) { if (!strcmp(s->snapshots[i].id_str, id)) { return i; } } } In short, we're searching the existing snapshots to see if sn_info->id_str matches any existing id, right after we set in the previous line a sn_info->id_str value that is already unique. The first code block goes way back to commit 585f8587, a 2006 commit from Fabrice Bellard that simply says "new qcow2 disk image format". No more info is provided about this logic in any subsequent commits that moved this code block around. I can't say about the original design, but the current logic is redundant. bdrv_snapshot_create is called in aio_context lock, forbidding any concurrent call to accidentally create a new snapshot between the find_new_snapshot_id and find_snapshot_by_id_and_name calls. What we're ending up doing is to cycle through the snapshots two times for no viable reason. This patch eliminates the redundancy by removing the 'id is unique' check that calls find_snapshot_by_id_and_name. Signed-off-by: NDaniel Henrique Barboza <danielhb413@gmail.com> Signed-off-by: NKevin Wolf <kwolf@redhat.com>
-
由 Daniel Henrique Barboza 提交于
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: NMurilo Opsfelder Araujo <muriloo@linux.ibm.com> Signed-off-by: NDaniel Henrique Barboza <danielhb413@gmail.com> Signed-off-by: NKevin Wolf <kwolf@redhat.com>
-
由 Daniel Henrique Barboza 提交于
At this moment, QEMU attempts to create/load/delete snapshots by using either an ID (id_str) or a name. The problem is that the code isn't consistent of whether the entered argument is an ID or a name, causing unexpected behaviors. For example, when creating snapshots via savevm <arg>, what happens is that "arg" is treated as both name and id_str. In a guest without snapshots, create a single snapshot via savevm: (qemu) savevm 0 (qemu) info snapshots List of snapshots present on all disks: ID TAG VM SIZE DATE VM CLOCK -- 0 741M 2018-07-31 13:39:56 00:41:25.313 A snapshot with name "0" is created. ID is hidden from the user, but the ID is a non-zero integer that starts at "1". Thus, this snapshot has id_str=1, TAG="0". Creating a second snapshot with arg = 1, the first one is deleted: (qemu) savevm 1 (qemu) info snapshots List of snapshots present on all disks: ID TAG VM SIZE DATE VM CLOCK -- 1 741M 2018-07-31 13:42:14 00:41:55.252 What happened? - when creating the second snapshot, a verification is done inside bdrv_all_delete_snapshot to delete any existing snapshots that matches an string argument. Here, the code calls bdrv_all_delete_snapshot("1", ...); - bdrv_all_delete_snapshot calls bdrv_snapshot_find(..., "1") for each BlockDriverState of the guest. And this is where things goes tilting: bdrv_snapshot_find does a search by both id_str and name. It finds out that there is a snapshot that has id_str = 1, stores a reference to the snapshot in the sn_info pointer and then returns match found; - since a match was found, a call to bdrv_snapshot_delete_by_id_or_name() is made. This function ignores the pointer written by bdrv_snapshot_find. Instead, it deletes the snapshot using bdrv_snapshot_delete() calling it first with id_str = 1. If it fails to delete, then it calls it again with name = 1. - after all that, QEMU creates the new snapshot, that has id_str = 1 and name = 1. The user is left wondering that happened with the first snapshot created. Similar bugs can be triggered when using loadvm and delvm. Before contemplating discarding the use of ID input in these operations, I've searched the code of what would be the implications. My findings are: - the RBD and Sheepdog drivers don't care. Both uses the 'name' field as key in their logic, making id_str = name when appropriate. replay-snapshot.c does not make any special use of id_str; - qcow2 uses id_str as an unique identifier but it is automatically calculated, not being influenced by user input. Other than that, there are no distinguish operations made only with id_str; - in blockdev.c, the delete operation uses a match of both id_str AND name. Given that id_str is either a copy of 'name' or auto-generated, we're fine here. This gives motivation to not consider ID as a valid user input in HMP commands - sticking with 'name' input only is more consistent. To accomplish that, the following changes were made in this patch: - bdrv_snapshot_find() does not match for id_str anymore, only 'name'. The function is called in save_snapshot(), load_snapshot(), bdrv_all_delete_snapshot() and bdrv_all_find_snapshot(). This change makes the search function more predictable and does not change the behavior of any underlying code that uses these affected functions, which are related to HMP (which is fine) and the main loop inside vl.c (which doesn't care about it anyways); - bdrv_all_delete_snapshot() does not call bdrv_snapshot_delete_by_id_or_name anymore. Instead, it uses the pointer returned by bdrv_snapshot_find to erase the snapshot with the exact match of id_str an name. This function is called in save_snapshot and hmp_delvm, thus this change produces the intended effect; - documentation changes to reflect the new behavior. I consider this to be an API fix instead of an API change - the user was already creating snapshots using 'name', but now he/she will also enjoy a consistent behavior. Ideally we would get rid of the id_str field entirely, but this would have repercussions on existing snapshots. Another day perhaps. Signed-off-by: NDaniel Henrique Barboza <danielhb413@gmail.com> Acked-by: NDr. David Alan Gilbert <dgilbert@redhat.com> Signed-off-by: NKevin Wolf <kwolf@redhat.com>
-
由 Jeff Cody 提交于
I'll not be involved in day-to-day qemu development. Remove myself as maintainer from the remainder of the network block drivers, and revert them to the general block layer maintainership. Move 'sheepdog' to the 'Odd Fixes' support level. For VHDX, added my personal email address as a maintainer, as I can answer questions or send the occassional bug fix. Leaving it as 'Supported', instead of 'Odd Fixes', because I think the rest of the block layer maintainers and developers will upkeep it as well, if needed. Signed-off-by: NJeff Cody <jcody@redhat.com> Acked-by: NMax Reitz <mreitz@redhat.com> Message-Id: <63e205cb84c8f0a10c1bc6d5d6856d72ceb56e41.1537984851.git.jcody@redhat.com> Signed-off-by: NKevin Wolf <kwolf@redhat.com>
-
由 Jeff Cody 提交于
I'll not be involved with day-to-day qemu development, and John Snow is a block jobs wizard. Have him take over block job maintainership duties. Signed-off-by: NJeff Cody <jcody@redhat.com> Acked-by: NJohn Snow <jsnow@redhat.com> Message-Id: <d56d7c6592e7d68aa72764e9616878394bffbc14.1537984851.git.jcody@redhat.com> Signed-off-by: NKevin Wolf <kwolf@redhat.com>
-