aboutsummaryrefslogtreecommitdiff
path: root/block.c
AgeCommit message (Collapse)AuthorFilesLines
2018-12-14block: Assert that flags are up-to-date in bdrv_reopen_prepare()Alberto Garcia1-0/+6
Towards the end of bdrv_reopen_queue_child(), before starting to process the children, the update_flags_from_options() function is called in order to have BDRVReopenState.flags in sync with the options from the QDict. This is necessary because during the reopen process flags must be updated for all nodes in the queue so bdrv_is_writable_after_reopen() and the permission checks work correctly. Because of that, calling update_flags_from_options() again in bdrv_reopen_prepare() doesn't really change the flags (they are already up-to-date). But we need to call it in order to remove those options from QemuOpts and that way indicate that they have been processed. Signed-off-by: Alberto Garcia <berto@igalia.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-12-14block: Remove assertions from update_flags_from_options()Alberto Garcia1-4/+0
This function takes four options (cache.direct, cache.no-flush, read-only and auto-read-only) from a QemuOpts object and updates the flags accordingly. If any of those options is not set (because it was missing from the original QDict or because it had an invalid value) then the function aborts with a failed assertion: $ qemu-io -c 'reopen -o read-only=foo' hd.qcow2 block.c:1126: update_flags_from_options: Assertion `qemu_opt_find(opts, BDRV_OPT_CACHE_DIRECT)' failed. Aborted This assertion is unnecessary, and it forces any caller of bdrv_reopen() to pass all the aforementioned four options. This may have made sense in order to remove ambiguity when bdrv_reopen() was taking both flags and options, but that's not the case anymore. It's also unnecessary if we want to validate the option values, because bdrv_reopen_prepare() already takes care of that, as we can see if we remove the assertions: $ qemu-io -c 'reopen -o read-only=foo' hd.qcow2 Parameter 'read-only' expects 'on' or 'off' Signed-off-by: Alberto Garcia <berto@igalia.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-12-14block: Stop passing flags to bdrv_reopen_queue_child()Alberto Garcia1-29/+19
Now that all callers are passing the new options using the QDict we no longer need the 'flags' parameter. This patch makes the following changes: 1) The update_options_from_flags() call is no longer necessary so it can be removed. 2) The update_flags_from_options() call is now used in all cases, and is moved down a few lines so it happens after the options QDict contains the final set of values. 3) The flags parameter is removed. Now the flags are initialized using the current value (for the top-level node) or the parent flags (after inherit_options()). In both cases the initial values are updated to reflect the new options in the QDict. This happens in bdrv_reopen_queue_child() (as explained above) and in bdrv_reopen_prepare(). Signed-off-by: Alberto Garcia <berto@igalia.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-12-14block: Remove flags parameter from bdrv_reopen_queue()Alberto Garcia1-2/+3
Now that all callers are passing all flag changes as QDict options, the flags parameter is no longer necessary, so we can get rid of it. Signed-off-by: Alberto Garcia <berto@igalia.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-12-14block: Drop bdrv_reopen()Alberto Garcia1-21/+0
No one is using this function anymore, so we can safely remove it. Signed-off-by: Alberto Garcia <berto@igalia.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-12-14block: Use bdrv_reopen_set_read_only() in bdrv_backing_update_filename()Alberto Garcia1-5/+5
This patch replaces the bdrv_reopen() calls that set and remove the BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function. Signed-off-by: Alberto Garcia <berto@igalia.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-12-14block: Add bdrv_reopen_set_read_only()Alberto Garcia1-0/+17
Most callers of bdrv_reopen() only use it to switch a BlockDriverState between read-only and read-write, so this patch adds a new function that does just that. We also want to get rid of the flags parameter in the bdrv_reopen() API, so this function sets the "read-only" option and passes the original flags (which will then be updated in bdrv_reopen_prepare()). Signed-off-by: Alberto Garcia <berto@igalia.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-11-27block: Don't inactivate children before parentsKevin Wolf1-31/+53
bdrv_child_cb_inactivate() asserts that parents are already inactive when children get inactivated. This precondition is necessary because parents could still issue requests in their inactivation code. When block nodes are created individually with -blockdev, all of them are monitor owned and will be returned by bdrv_next() in an undefined order (in practice, in the order of their creation, which is usually children before parents), which obviously fails the assertion: qemu: block.c:899: bdrv_child_cb_inactivate: Assertion `bs->open_flags & BDRV_O_INACTIVE' failed. This patch fixes the ordering by skipping nodes with still active parents in bdrv_inactivate_recurse() because we know that they will be covered by recursion when the last active parent becomes inactive. With the correct parents-before-children ordering, we also got rid of the reason why commit aad0b7a0bfb introduced two passes, so we can go back to a single-pass recursion. This is necessary so we can rely on the BDRV_O_INACTIVE flag to skip nodes with active parents (the flag used to be set only in pass 2, so we would always skip non-root nodes in pass 1 because all parents would still be considered active; setting the flag in pass 1 would mean, that we never skip anything in pass 2 because all parents are already considered inactive). Because of the change to single pass, this patch is best reviewed with whitespace changes ignored. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com>
2018-11-22block: Update BlockDriverState.inherits_from on bdrv_drop_intermediate()Alberto Garcia1-0/+16
The previous patch fixed the inherits_from pointer after block-stream, and this one does the same for block-commit. When block-commit finishes and the 'top' node is not the topmost one from the backing chain then all nodes above 'base' up to and including 'top' are removed from the chain. The bdrv_drop_intermediate() call converts a chain like this one: base <- intermediate <- top <- active into this one: base <- active In a simple scenario each backing file from the first chain has the inherits_from attribute pointing to its parent. This means that reopening 'active' will recursively reopen all its children, whose options can be changed in the process. However after the 'block-commit' call base.inherits_from is NULL and the chain is broken, so 'base' does not inherit from 'active' and will not be reopened automatically: $ qemu-img create -f qcow2 hd0.qcow2 1M $ qemu-img create -f qcow2 -b hd0.qcow2 hd1.qcow2 $ qemu-img create -f qcow2 -b hd1.qcow2 hd2.qcow2 $ $QEMU -drive if=none,file=hd2.qcow2 { 'execute': 'block-commit', 'arguments': { 'device': 'none0', 'top': 'hd1.qcow2' } } { 'execute': 'human-monitor-command', 'arguments': { 'command-line': 'qemu-io none0 "reopen -o backing.l2-cache-size=2M"' } } { "return": "Cannot change the option 'backing.l2-cache-size'\r\n"} This patch updates base.inherits_from in this scenario, and adds a test case. Signed-off-by: Alberto Garcia <berto@igalia.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-11-22block: Update BlockDriverState.inherits_from on bdrv_set_backing_hd()Alberto Garcia1-0/+21
When a BlockDriverState's child is opened (be it a backing file, the protocol layer, or any other) inherits_from is set to point to the parent node. Children opened separately and then attached to a parent don't have this pointer set. bdrv_reopen_queue_child() uses this to determine whether a node's children must also be reopened inheriting the options from the parent or not. If inherits_from points to the parent then the child is reopened and its options can be changed, like in this example: $ qemu-img create -f qcow2 hd0.qcow2 1M $ qemu-img create -f qcow2 hd1.qcow2 1M $ $QEMU -drive if=none,node-name=hd0,file=hd0.qcow2,\ backing.driver=qcow2,backing.file.filename=hd1.qcow2 (qemu) qemu-io hd0 "reopen -o backing.l2-cache-size=2M" If the child does not inherit from the parent then it does not get reopened and its options cannot be changed: $ $QEMU -drive if=none,node-name=hd1,file=hd1.qcow2 -drive if=none,node-name=hd0,file=hd0.qcow2,backing=hd1 (qemu) qemu-io hd0 "reopen -o backing.l2-cache-size=2M" Cannot change the option 'backing.l2-cache-size' If a disk image has a chain of backing files then all of them are also connected through their inherits_from pointers (i.e. it's possible to walk the chain in reverse order from base to top). However this is broken if the intermediate nodes are removed using e.g. block-stream because the inherits_from pointer from the base node becomes NULL: $ qemu-img create -f qcow2 hd0.qcow2 1M $ qemu-img create -f qcow2 -b hd0.qcow2 hd1.qcow2 $ qemu-img create -f qcow2 -b hd1.qcow2 hd2.qcow2 $ $QEMU -drive if=none,file=hd2.qcow2 (qemu) qemu-io none0 "reopen -o backing.l2-cache-size=2M" (qemu) block_stream none0 0 hd0.qcow2 (qemu) qemu-io none0 "reopen -o backing.l2-cache-size=2M" Cannot change the option 'backing.l2-cache-size' This patch updates the inherits_from pointer if the intermediate nodes of a backing chain are removed using bdrv_set_backing_hd(), and adds a test case for this scenario. Signed-off-by: Alberto Garcia <berto@igalia.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-11-22block: Fix update of BDRV_O_AUTO_RDONLY in update_flags_from_options()Alberto Garcia1-3/+1
Commit e35bdc123a4ace9f4d3fcca added the auto-read-only option and the code to update its corresponding flag in update_flags_from_options(), but forgot to clear the flag if auto-read-only is false. Signed-off-by: Alberto Garcia <berto@igalia.com> Reported-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-11-19block: Always abort reopen after prepare succeededMax Reitz1-0/+12
bdrv_reopen_multiple() does not invoke bdrv_reopen_abort() for the element of the reopen queue for which bdrv_reopen_prepare() failed, because it assumes that the prepare function will have rolled back all changes already. However, bdrv_reopen_prepare() does not do this in every case: It may notice an error after BlockDriver.bdrv_reopen_prepare() succeeded, and it will not invoke BlockDriver.bdrv_reopen_abort() then; and neither will bdrv_reopen_multiple(), as explained above. This is wrong because we must always call .bdrv_reopen_commit() or .bdrv_reopen_abort() after .bdrv_reopen_prepare() has succeeded. Otherwise, the block driver has no chance to undo what it has done in its implementation of .bdrv_reopen_prepare(). To fix this, bdrv_reopen_prepare() has to call .bdrv_reopen_abort() if it wants to return an error after .bdrv_reopen_prepare() has succeeded. Signed-off-by: Max Reitz <mreitz@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-11-05block: Require auto-read-only for existing fallbacksKevin Wolf1-15/+27
Some block drivers have traditionally changed their node to read-only mode without asking the user. This behaviour has been marked deprecated since 2.11, expecting users to provide an explicit read-only=on option. Now that we have auto-read-only=on, enable these drivers to make use of the option. This is the only use of bdrv_set_read_only(), so we can make it a bit more specific and turn it into a bdrv_apply_auto_read_only() that is more convenient for drivers to use. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com>
2018-11-05block: Add auto-read-only optionKevin Wolf1-0/+17
If a management application builds the block graph node by node, the protocol layer doesn't inherit its read-only option from the format layer any more, so it must be set explicitly. Backing files should work on read-only storage, but at the same time, a block job like commit should be able to reopen them read-write if they are on read-write storage. However, without option inheritance, reopen only changes the read-only option for the root node (typically the format layer), but not the protocol layer, so reopening fails (the format layer wants to get write permissions, but the protocol layer is still read-only). A simple workaround for the problem in the management tool would be to open the protocol layer always read-write and to make only the format layer read-only for backing files. However, sometimes the file is actually stored on read-only storage and we don't know whether the image can be opened read-write (for example, for NBD it depends on the server we're trying to connect to). This adds an option that makes QEMU try to open the image read-write, but allows it to degrade to a read-only mode without returning an error. The documentation for this option is consciously phrased in a way that allows QEMU to switch to a better model eventually: Instead of trying when the image is first opened, making the read-only flag dynamic and changing it automatically whenever the first BLK_PERM_WRITE user is attached or the last one is detached would be much more useful behaviour. Unfortunately, this more useful behaviour is also a lot harder to implement, and libvirt needs a solution now before it can switch to -blockdev, so let's start with this easier approach for now. Instead of adding a new auto-read-only option, turning the existing read-only into an enum (with a bool alternate for compatibility) was considered, but it complicated the implementation to the point that it didn't seem to be worth it. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com>
2018-11-05block: Update flags in bdrv_set_read_only()Kevin Wolf1-0/+7
To fully change the read-only state of a node, we must not only change bs->read_only, but also update bs->open_flags. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com>
2018-11-05block: replace "discard" literal with BDRV_OPT_DISCARD macroAlberto Garcia1-3/+3
Signed-off-by: Alberto Garcia <berto@igalia.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-10-29dirty-bitmaps: clean-up bitmaps loading and migration logicVladimir Sementsov-Ogievskiy1-4/+7
This patch aims to bring the following behavior: 1. We don't load bitmaps, when started in inactive mode. It's the case of incoming migration. In this case we wait for bitmaps migration through migration channel (if 'dirty-bitmaps' capability is enabled) or for invalidation (to load bitmaps from the image). 2. We don't remove persistent bitmaps on inactivation. Instead, we only remove bitmaps after storing. This is the only way to restore bitmaps, if we decided to resume source after [failed] migration with 'dirty-bitmaps' capability enabled (which means, that bitmaps were not stored). 3. We load bitmaps on open and any invalidation, it's ok for all cases: - normal open - migration target invalidation with dirty-bitmaps capability (bitmaps are migrating through migration channel, the are not stored, so they should have IN_USE flag set and will be skipped when loading. However, it would fail if bitmaps are read-only[1]) - migration target invalidation without dirty-bitmaps capability (normal load of the bitmaps, if migrated with shared storage) - source invalidation with dirty-bitmaps capability (skip because IN_USE) - source invalidation without dirty-bitmaps capability (bitmaps were dropped, reload them) [1]: to accurately handle this, migration of read-only bitmaps is explicitly forbidden in this patch. New mechanism for not storing bitmaps when migrate with dirty-bitmaps capability is introduced: migration filed in BdrvDirtyBitmap. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: John Snow <jsnow@redhat.com>
2018-10-19block: Clean up bdrv_img_create()'s error reportingMarkus Armbruster1-3/+0
bdrv_img_create() takes an Error ** argument and uses it in the conventional way, except for one place: when qemu_opts_do_parse() fails, it first reports its error to stderr or the HMP monitor with error_report_err(), then error_setg()'s a generic error. When the caller reports that second error similarly, this produces two consecutive error messages on stderr or the HMP monitor. When the caller does something else with it, such as send it via QMP, the first error still goes to stderr or the HMP monitor. Fortunately, no such caller exists. Simply use the first error as is. Update expected output of qemu-iotest 049 accordingly. Cc: Kevin Wolf <kwolf@redhat.com> Cc: Max Reitz <mreitz@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <20181017082702.5581-37-armbru@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
2018-10-19error: Fix use of error_prepend() with &error_fatal, &error_abortMarkus Armbruster1-3/+3
From include/qapi/error.h: * Pass an existing error to the caller with the message modified: * error_propagate(errp, err); * error_prepend(errp, "Could not frobnicate '%s': ", name); Fei Li pointed out that doing error_propagate() first doesn't work well when @errp is &error_fatal or &error_abort: the error_prepend() is never reached. Since I doubt fixing the documentation will stop people from getting it wrong, introduce error_propagate_prepend(), in the hope that it lures people away from using its constituents in the wrong order. Update the instructions in error.h accordingly. Convert existing error_prepend() next to error_propagate to error_propagate_prepend(). If any of these get reached with &error_fatal or &error_abort, the error messages improve. I didn't check whether that's the case anywhere. Cc: Fei Li <fli@suse.com> Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <20181017082702.5581-2-armbru@redhat.com>
2018-10-01block: Allow changing 'detect-zeroes' on reopenAlberto Garcia1-24/+40
'detect-zeroes' is one of the basic BlockdevOptions available for all drivers, but it's not handled by bdrv_reopen_prepare(), so any attempt to change it results in an error: (qemu) qemu-io virtio0 "reopen -o detect-zeroes=on" Cannot change the option 'detect-zeroes' Since there's no reason why we shouldn't allow changing it and the implementation is simple let's just do it. Signed-off-by: Alberto Garcia <berto@igalia.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-10-01block: Allow changing 'discard' on reopenAlberto Garcia1-0/+11
'discard' is one of the basic BlockdevOptions available for all drivers, but it's not handled by bdrv_reopen_prepare() so any attempt to change it results in an error: (qemu) qemu-io virtio0 "reopen -o discard=on" Cannot change the option 'discard' Since there's no reason why we shouldn't allow changing it and the implementation is simple let's just do it. Signed-off-by: Alberto Garcia <berto@igalia.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-10-01block: Forbid trying to change unsupported options during reopenAlberto Garcia1-15/+7
The bdrv_reopen_prepare() function checks all options passed to each BlockDriverState (in the reopen_state->options QDict) and makes all necessary preparations to apply the option changes requested by the user. Options are removed from the QDict as they are processed, so at the end of bdrv_reopen_prepare() only the options that can't be changed are left. Then a loop goes over all remaining options and verifies that the old and new values are identical, returning an error if they're not. The problem is that at the moment there are options that are removed from the QDict although they can't be changed. The consequence of this is any modification to any of those options is silently ignored: (qemu) qemu-io virtio0 "reopen -o discard=on" This happens when all options from bdrv_runtime_opts are removed from the QDict but then only a few of them are processed. Since it's especially important that "node-name" and "driver" are not changed, the code puts them back into the QDict so they are checked at the end of the function. Instead of putting only those two options back into the QDict, this patch puts all unprocessed options using qemu_opts_to_qdict(). update_flags_from_options() also needs to be modified to prevent BDRV_OPT_CACHE_NO_FLUSH, BDRV_OPT_CACHE_DIRECT and BDRV_OPT_READ_ONLY from going back to the QDict. Signed-off-by: Alberto Garcia <berto@igalia.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-10-01block: Allow child references on reopenAlberto Garcia1-0/+18
In the previous patches we removed all child references from bs->{options,explicit_options} because keeping them is useless and wrong. Because of this, any attempt to reopen a BlockDriverState using a child reference as one of its options would result in a failure, because bdrv_reopen_prepare() would detect that there's a new option (the child reference) that wasn't present in bs->options. But passing child references on reopen can be useful. It's a way to specify a BDS's child without having to pass recursively all of the child's options, and if the reference points to a different BDS then this can allow us to replace the child. However, replacing the child is something that needs to be implemented case by case and only when it makes sense. For now, this patch allows passing a child reference as long as it points to the current child of the BlockDriverState. It's also important to remember that, as a consequence of the previous patches, this child reference will be removed from bs->{options,explicit_options} after the reopening has been completed. Signed-off-by: Alberto Garcia <berto@igalia.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-10-01block: Don't look for child references in append_open_options()Alberto Garcia1-12/+1
In the previous patch we removed child references from bs->options, so there's no need to look for them here anymore. Signed-off-by: Alberto Garcia <berto@igalia.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-10-01block: Remove child references from bs->{options,explicit_options}Alberto Garcia1-1/+12
Block drivers allow opening their children using a reference to an existing BlockDriverState. These references remain stored in the 'options' and 'explicit_options' QDicts, but we don't need to keep them once everything is open. What is more important, these values can become wrong if the children change: $ qemu-img create -f qcow2 hd0.qcow2 10M $ qemu-img create -f qcow2 hd1.qcow2 10M $ qemu-img create -f qcow2 hd2.qcow2 10M $ $QEMU -drive if=none,file=hd0.qcow2,node-name=hd0 \ -drive if=none,file=hd1.qcow2,node-name=hd1,backing=hd0 \ -drive file=hd2.qcow2,node-name=hd2,backing=hd1 After this hd2 has hd1 as its backing file. Now let's remove it using block_stream: (qemu) block_stream hd2 0 hd0.qcow2 Now hd0 is the backing file of hd2, but hd2's options QDicts still contain backing=hd1. Signed-off-by: Alberto Garcia <berto@igalia.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-09-25block: Use a single global AioWaitKevin Wolf1-5/+0
When draining a block node, we recurse to its parent and for subtree drains also to its children. A single AIO_WAIT_WHILE() is then used to wait for bdrv_drain_poll() to become true, which depends on all of the nodes we recursed to. However, if the respective child or parent becomes quiescent and calls bdrv_wakeup(), only the AioWait of the child/parent is checked, while AIO_WAIT_WHILE() depends on the AioWait of the original node. Fix this by using a single AioWait for all callers of AIO_WAIT_WHILE(). This may mean that the draining thread gets a few more unnecessary wakeups because an unrelated operation got completed, but we already wake it up when something _could_ have changed rather than only if it has certainly changed. Apart from that, drain is a slow path anyway. In theory it would be possible to use wakeups more selectively and still correctly, but the gains are likely not worth the additional complexity. In fact, this patch is a nice simplification for some places in the code. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com>
2018-09-25block: Fix use after free error in bdrv_open_inherit()Alberto Garcia1-0/+1
When a block device is opened with BDRV_O_SNAPSHOT and the bdrv_append_temp_snapshot() call fails then the error code path tries to unref the already destroyed 'options' QDict. This can be reproduced easily by setting TMPDIR to a location where the QEMU process can't write: $ TMPDIR=/nonexistent $QEMU -drive driver=null-co,snapshot=on Signed-off-by: Alberto Garcia <berto@igalia.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-08-24qjson: Have qobject_from_json() & friends reject empty and blankMarkus Armbruster1-5/+0
The last case where qobject_from_json() & friends return null without setting an error is empty or blank input. Callers: * block.c's parse_json_protocol() reports "Could not parse the JSON options". It's marked as a work-around, because it also covered actual bugs, but they got fixed in the previous few commits. * qobject_input_visitor_new_str() reports "JSON parse error". Also marked as work-around. The recent fixes have made this unreachable, because it currently gets called only for input starting with '{'. * check-qjson.c's empty_input() and blank_input() demonstrate the behavior. * The other callers are not affected since they only pass input with exactly one JSON value or, in the case of negative tests, one error. Fail with "Expecting a JSON value" instead of returning null, and simplify callers. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <20180823164025.12553-48-armbru@redhat.com>
2018-08-15block: Simplify append_open_options()Alberto Garcia1-5/+2
This function returns a BDS's driver-specific options, excluding also those from its children. Since we have just removed all children options from bs->options there's no need to do this last step. We allow references to children, though ("backing": "node0"), so those we still have to remove. Signed-off-by: Alberto Garcia <berto@igalia.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-08-15block: Update bs->options if bdrv_reopen() succeedsAlberto Garcia1-1/+14
If bdrv_reopen() succeeds then bs->explicit_options is updated with the new values, but bs->options never changes. Here's an example: { "execute": "blockdev-add", "arguments": { "driver": "qcow2", "node-name": "hd0", "overlap-check": "all", "file": { "driver": "file", "filename": "hd0.qcow2" } } } After this, both bs->options and bs->explicit_options contain "overlap-check": "all". Now let's change that using qemu-io's reopen command: (qemu) qemu-io hd0 "reopen -o overlap-check=none" After this, bs->explicit_options contains the new value but bs->options still keeps the old one. This patch updates bs->options after a BDS has been successfully reopened. Signed-off-by: Alberto Garcia <berto@igalia.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-08-15block: Simplify bdrv_reopen_abort()Alberto Garcia1-5/+4
If a bdrv_reopen_multiple() call fails, then the explicit_options QDict has to be deleted for every entry in the reopen queue. This must happen regardless of whether that entry's bdrv_reopen_prepare() call succeeded or not. This patch simplifies the cleanup code a bit. Signed-off-by: Alberto Garcia <berto@igalia.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-08-15block: Remove children options from bs->{options,explicit_options}Alberto Garcia1-0/+11
When bdrv_open_inherit() opens a BlockDriverState the options QDict can contain options for some of its children, passed in the form of child-name.option=value So while each child is opened with that subset of options, those same options remain stored in the parent BDS, leaving (at least) two copies of each one of them ("child-name.option=value" in the parent and "option=value" in the child). Having the children options stored in the parent is unnecessary and it can easily lead to an inconsistent state: $ qemu-img create -f qcow2 hd0.qcow2 10M $ qemu-img create -f qcow2 -b hd0.qcow2 hd1.qcow2 $ qemu-img create -f qcow2 -b hd1.qcow2 hd2.qcow2 $ $QEMU -drive file=hd2.qcow2,node-name=hd2,backing.node-name=hd1 This opens a chain of images hd0 <- hd1 <- hd2. Now let's remove hd1 using block_stream: (qemu) block_stream hd2 0 hd0.qcow2 After this hd2 contains backing.node-name=hd1, which is no longer correct because hd1 doesn't exist anymore. This patch removes all children options from the parent dictionaries at the end of bdrv_open_inherit() and bdrv_reopen_queue_child(). Signed-off-by: Alberto Garcia <berto@igalia.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-08-15block: make .bdrv_close optionalVladimir Sementsov-Ogievskiy1-1/+3
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-07-23block: Fix typos in comments (found by codespell)Stefan Weil1-1/+1
Signed-off-by: Stefan Weil <sw@weilnetz.de> Reviewed-by: John Snow <jsnow@redhat.com> Reviewed-by: Jeff Cody <jcody@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-07-10block: Poll after drain on attaching a nodeKevin Wolf1-1/+1
Commit dcf94a23b1 ('block: Don't poll in parent drain callbacks') removed polling in bdrv_child_cb_drained_begin() on the grounds that the original bdrv_drain() already will poll and BdrvChildRole.drained_begin calls must not cause graph changes (and therefore must not call aio_poll() or the recursion through the graph will break. This reasoning is correct for calls through bdrv_do_drained_begin(). However, BdrvChildRole.drained_begin is also called when a node that is already in a drained section (i.e. bdrv_do_drained_begin() has already returned and therefore can't poll any more) is attached to a new parent. In this case, we must explicitly poll to have all requests completed before the drained new child can be attached to the parent. In bdrv_replace_child_noperm(), we know that we're not inside the recursion of bdrv_do_drained_begin() because graph changes are not allowed there, and bdrv_replace_child_noperm() is a graph change. The call of BdrvChildRole.drained_begin() must therefore be followed by a BDRV_POLL_WHILE() that waits for the completion of requests. Reported-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-07-05block: Don't silently truncate node namesKevin Wolf1-0/+6
If the user passes a too long node name string, we silently truncate it to fit into BlockDriverState.node_name, i.e. to 31 characters. Apart from surprising the user when the node has a different name than requested, this also bypasses the check for duplicate names, so that the same name can be assigned to multiple nodes. Fix this by just making too long node names an error. Reported-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-07-05block: Move two block permission constants to the relevant enumAri Sundholm1-6/+0
This allows using the two constants outside of block.c, which will happen in a subsequent patch. Signed-off-by: Ari Sundholm <ari@tuxera.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-06-29block: Move bdrv_truncate() implementation to io.cKevin Wolf1-110/+1
This moves the bdrv_truncate() implementation from block.c to block/io.c so it can have access to the tracked requests infrastructure. This involves making refresh_total_sectors() public (in block_int.h). Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
2018-06-29block: Convert .bdrv_truncate callback to coroutine_fnKevin Wolf1-8/+55
bdrv_truncate() is an operation that can block (even for a quite long time, depending on the PreallocMode) in I/O paths that shouldn't block. Convert it to a coroutine_fn so that we have the infrastructure for drivers to make their .bdrv_co_truncate implementation asynchronous. This change could potentially introduce new race conditions because bdrv_truncate() isn't necessarily executed atomically any more. Whether this is a problem needs to be evaluated for each block driver that supports truncate: * file-posix/win32, gluster, iscsi, nfs, rbd, ssh, sheepdog: The protocol drivers are trivially safe because they don't actually yield yet, so there is no change in behaviour. * copy-on-read, crypto, raw-format: Essentially just filter drivers that pass the request to a child node, no problem. * qcow2: The implementation modifies metadata, so it needs to hold s->lock to be safe with concurrent I/O requests. In order to avoid double locking, this requires pulling the locking out into preallocate_co() and using qcow2_write_caches() instead of bdrv_flush(). * qed: Does a single header update, this is fine without locking. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
2018-06-18block: Generalize should_update_child() ruleMax Reitz1-10/+34
Currently, bdrv_replace_node() refuses to create loops from one BDS to itself if the BDS to be replaced is the backing node of the BDS to replace it: Say there is a node A and a node B. Replacing B by A means making all references to B point to A. If B is a child of A (i.e. A has a reference to B), that would mean we would have to make this reference point to A itself -- so we'd create a loop. bdrv_replace_node() (through should_update_child()) refuses to do so if B is the backing node of A. There is no reason why we should create loops if B is not the backing node of A, though. The BDS graph should never contain loops, so we should always refuse to create them. If B is a child of A and B is to be replaced by A, we should simply leave B in place there because it is the most sensible choice. A more specific argument would be: Putting filter drivers into the BDS graph is basically the same as appending an overlay to a backing chain. But the main child BDS of a filter driver is not "backing" but "file", so restricting the no-loop rule to backing nodes would fail here. Signed-off-by: Max Reitz <mreitz@redhat.com> Reviewed-by: Fam Zheng <famz@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Message-id: 20180613181823.13618-7-mreitz@redhat.com Signed-off-by: Max Reitz <mreitz@redhat.com>
2018-06-18block: Allow graph changes in bdrv_drain_all_begin/end sectionsKevin Wolf1-3/+31
bdrv_drain_all_*() used bdrv_next() to iterate over all root nodes and did a subtree drain for each of them. This works fine as long as the graph is static, but sadly, reality looks different. If the graph changes so that root nodes are added or removed, we would have to compensate for this. bdrv_next() returns each root node only once even if it's the root node for multiple BlockBackends or for a monitor-owned block driver tree, which would only complicate things. The much easier and more obviously correct way is to fundamentally change the way the functions work: Iterate over all BlockDriverStates, no matter who owns them, and drain them individually. Compensation is only necessary when a new BDS is created inside a drain_all section. Removal of a BDS doesn't require any action because it's gone afterwards anyway. Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-06-18block: ignore_bds_parents parameter for drain functionsKevin Wolf1-4/+7
In the future, bdrv_drained_all_begin/end() will drain all invidiual nodes separately rather than whole subtrees. This means that we don't want to propagate the drain to all parents any more: If the parent is a BDS, it will already be drained separately. Recursing to all parents is unnecessary work and would make it an O(n²) operation. Prepare the drain function for the changed drain_all by adding an ignore_bds_parents parameter to the internal implementation that prevents the propagation of the drain to BDS parents. We still (have to) propagate it to non-BDS parents like BlockBackends or Jobs because those are not drained separately. Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-06-18block: Don't poll in parent drain callbacksKevin Wolf1-1/+1
bdrv_do_drained_begin() is only safe if we have a single BDRV_POLL_WHILE() after quiescing all affected nodes. We cannot allow that parent callbacks introduce a nested polling loop that could cause graph changes while we're traversing the graph. Split off bdrv_do_drained_begin_quiesce(), which only quiesces a single node without waiting for its requests to complete. These requests will be waited for in the BDRV_POLL_WHILE() call down the call chain. Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-06-18block: Drain recursively with a single BDRV_POLL_WHILE()Kevin Wolf1-1/+1
Anything can happen inside BDRV_POLL_WHILE(), including graph changes that may interfere with its callers (e.g. child list iteration in recursive callers of bdrv_do_drained_begin). Switch to a single BDRV_POLL_WHILE() call for the whole subtree at the end of bdrv_do_drained_begin() to avoid such effects. The recursion happens now inside the loop condition. As the graph can only change between bdrv_drain_poll() calls, but not inside of it, doing the recursion here is safe. Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-06-18block: Really pause block jobs on drainKevin Wolf1-0/+9
We already requested that block jobs be paused in .bdrv_drained_begin, but no guarantee was made that the job was actually inactive at the point where bdrv_drained_begin() returned. This introduces a new callback BdrvChildRole.bdrv_drained_poll() and uses it to make bdrv_drain_poll() consider block jobs using the node to be drained. For the test case to work as expected, we have to switch from block_job_sleep_ns() to qemu_co_sleep_ns() so that the test job is even considered active and must be waited for when draining the node. Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-06-15block: Add block-specific QDict headerMax Reitz1-0/+1
There are numerous QDict functions that have been introduced for and are used only by the block layer. Move their declarations into an own header file to reflect that. While qdict_extract_subqdict() is in fact used outside of the block layer (in util/qemu-config.c), it is still a function related very closely to how the block layer works with nested QDicts, namely by sometimes flattening them. Therefore, its declaration is put into this header as well and util/qemu-config.c includes it with a comment stating exactly which function it needs. Suggested-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Max Reitz <mreitz@redhat.com> Message-Id: <20180509165530.29561-7-mreitz@redhat.com> [Copyright note tweaked, superfluous includes dropped] Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-06-11block: Make bdrv_is_writable() publicMax Reitz1-3/+14
This is a useful function for the whole block layer, so make it public. At the same time, users outside of block.c probably do not need to make use of the reopen functionality, so rename the current function to bdrv_is_writable_after_reopen() create a new bdrv_is_writable() function that just passes NULL to it for the reopen queue. Cc: qemu-stable@nongnu.org Signed-off-by: Max Reitz <mreitz@redhat.com> Message-id: 20180606193702.7113-2-mreitz@redhat.com Reviewed-by: John Snow <jsnow@redhat.com> Reviewed-by: Jeff Cody <jcody@redhat.com> Signed-off-by: Max Reitz <mreitz@redhat.com>
2018-06-11block: Add Error parameter to bdrv_amend_optionsMax Reitz1-2/+6
Looking at the qcow2 code that is riddled with error_report() calls, this is really how it should have been from the start. Along the way, turn the target_version/current_version comparisons at the beginning of qcow2_downgrade() into assertions (the caller has to make sure these conditions are met), and rephrase the error message on using compat=1.1 to get refcount widths other than 16 bits. Signed-off-by: Max Reitz <mreitz@redhat.com> Message-id: 20180509210023.20283-3-mreitz@redhat.com Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: John Snow <jsnow@redhat.com> Signed-off-by: Max Reitz <mreitz@redhat.com>
2018-05-23block: Cancel job in bdrv_close_all() callersKevin Wolf1-3/+1
Now that we cancel all jobs and not only block jobs on shutdown, doing that in bdrv_close_all() isn't really appropriate any more. Move the job_cancel_sync_all() call to the callers, and only assert that there are no job running in bdrv_close_all(). Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2018-05-23job: Move completion and cancellation to JobKevin Wolf1-1/+3
This moves the top-level job completion and cancellation functions from BlockJob to Job. Signed-off-by: Kevin Wolf <kwolf@redhat.com>