ShuriZma
/
xemu
mirror of https://github.com/xemu-project/xemu.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

block: Allow changing bs->file on reopen 

When the x-blockdev-reopen was added it allowed reconfiguring the
graph by replacing backing files, but changing the 'file' option was
forbidden. Because of this restriction some operations are not
possible, notably inserting and removing block filters.

This patch adds support for replacing the 'file' option. This is
similar to replacing the backing file and the user is likewise
responsible for the correctness of the resulting graph, otherwise this
can lead to data corruption.

Signed-off-by: Alberto Garcia <berto@igalia.com>
 [vsementsov: bdrv_reopen_parse_file_or_backing() is modified a lot]
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20210610120537.196183-9-vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
This commit is contained in:
Alberto Garcia 2021-06-10 15:05:36 +03:00 committed by Kevin Wolf
parent 3d0e8743f0
commit ecd30d2d97
3 changed files with 67 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

78
block.c
View File

@ -92,7 +92,7 @@ static void bdrv_remove_filter_or_cow_child(BlockDriverState *bs,
static int bdrv_reopen_prepare(BDRVReopenState *reopen_state, static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue, BlockReopenQueue *queue,
Transaction *set_backings_tran, Error **errp); Transaction *change_child_tran, Error **errp);
static void bdrv_reopen_commit(BDRVReopenState *reopen_state); static void bdrv_reopen_commit(BDRVReopenState *reopen_state);
static void bdrv_reopen_abort(BDRVReopenState *reopen_state); static void bdrv_reopen_abort(BDRVReopenState *reopen_state);
@ -4148,6 +4148,10 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
refresh_list = bdrv_topological_dfs(refresh_list, found, refresh_list = bdrv_topological_dfs(refresh_list, found,
state->old_backing_bs); state->old_backing_bs);
} }
if (state->old_file_bs) {
refresh_list = bdrv_topological_dfs(refresh_list, found,
state->old_file_bs);
}
} }
/* /*
@ -4240,64 +4244,81 @@ int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
* *
* Return 0 on success, otherwise return < 0 and set @errp. * Return 0 on success, otherwise return < 0 and set @errp.
*/ */
static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, static int bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
Transaction *set_backings_tran, bool is_backing, Transaction *tran,
Error **errp) Error **errp)
{ {
BlockDriverState *bs = reopen_state->bs; BlockDriverState *bs = reopen_state->bs;
BlockDriverState *new_backing_bs; BlockDriverState *new_child_bs;
BlockDriverState *old_child_bs = is_backing ? child_bs(bs->backing) :
child_bs(bs->file);
const char *child_name = is_backing ? "backing" : "file";
QObject *value; QObject *value;
const char *str; const char *str;
value = qdict_get(reopen_state->options, "backing"); value = qdict_get(reopen_state->options, child_name);
if (value == NULL) { if (value == NULL) {
return 0; return 0;
} }
switch (qobject_type(value)) { switch (qobject_type(value)) {
case QTYPE_QNULL: case QTYPE_QNULL:
new_backing_bs = NULL; assert(is_backing); /* The 'file' option does not allow a null value */
new_child_bs = NULL;
break; break;
case QTYPE_QSTRING: case QTYPE_QSTRING:
str = qstring_get_str(qobject_to(QString, value)); str = qstring_get_str(qobject_to(QString, value));
new_backing_bs = bdrv_lookup_bs(NULL, str, errp); new_child_bs = bdrv_lookup_bs(NULL, str, errp);
if (new_backing_bs == NULL) { if (new_child_bs == NULL) {
return -EINVAL; return -EINVAL;
} else if (bdrv_recurse_has_child(new_backing_bs, bs)) { } else if (bdrv_recurse_has_child(new_child_bs, bs)) {
error_setg(errp, "Making '%s' a backing file of '%s' " error_setg(errp, "Making '%s' a %s child of '%s' would create a "
"would create a cycle", str, bs->node_name); "cycle", str, child_name, bs->node_name);
return -EINVAL; return -EINVAL;
} }
break; break;
default: default:
/* 'backing' does not allow any other data type */ /*
* The options QDict has been flattened, so 'backing' and 'file'
* do not allow any other data type here.
*/
g_assert_not_reached(); g_assert_not_reached();
} }
if (bs->backing) { if (old_child_bs == new_child_bs) {
if (bdrv_skip_implicit_filters(bs->backing->bs) == new_backing_bs) { return 0;
}
if (old_child_bs) {
if (bdrv_skip_implicit_filters(old_child_bs) == new_child_bs) {
return 0; return 0;
} }
if (bs->backing->bs->implicit) { if (old_child_bs->implicit) {
error_setg(errp, "Cannot change backing link if '%s' has " error_setg(errp, "Cannot replace implicit %s child of %s",
"an implicit backing file", bs->node_name); child_name, bs->node_name);
return -EPERM; return -EPERM;
} }
} }
if (bs->drv->is_filter && !bs->backing) { if (bs->drv->is_filter && !old_child_bs) {
/* /*
* Filters always have a file or a backing child, so we are trying to * Filters always have a file or a backing child, so we are trying to
* change wrong child * change wrong child
*/ */
error_setg(errp, "'%s' is a %s filter node that does not support a " error_setg(errp, "'%s' is a %s filter node that does not support a "
"backing child", bs->node_name, bs->drv->format_name); "%s child", bs->node_name, bs->drv->format_name, child_name);
return -EINVAL; return -EINVAL;
} }
reopen_state->old_backing_bs = bs->backing ? bs->backing->bs : NULL; if (is_backing) {
return bdrv_set_backing_noperm(bs, new_backing_bs, set_backings_tran, errp); reopen_state->old_backing_bs = old_child_bs;
} else {
reopen_state->old_file_bs = old_child_bs;
}
return bdrv_set_file_or_backing_noperm(bs, new_child_bs, is_backing,
tran, errp);
} }
/* /*
@ -4319,7 +4340,7 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state,
*/ */
static int bdrv_reopen_prepare(BDRVReopenState *reopen_state, static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue, BlockReopenQueue *queue,
Transaction *set_backings_tran, Error **errp) Transaction *change_child_tran, Error **errp)
{ {
int ret = -1; int ret = -1;
int old_flags; int old_flags;
@ -4439,12 +4460,21 @@ static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
* either a reference to an existing node (using its node name) * either a reference to an existing node (using its node name)
* or NULL to simply detach the current backing file. * or NULL to simply detach the current backing file.
*/ */
ret = bdrv_reopen_parse_backing(reopen_state, set_backings_tran, errp); ret = bdrv_reopen_parse_file_or_backing(reopen_state, true,
change_child_tran, errp);
if (ret < 0) { if (ret < 0) {
goto error; goto error;
} }
qdict_del(reopen_state->options, "backing"); qdict_del(reopen_state->options, "backing");
/* Allow changing the 'file' option. In this case NULL is not allowed */
ret = bdrv_reopen_parse_file_or_backing(reopen_state, false,
change_child_tran, errp);
if (ret < 0) {
goto error;
}
qdict_del(reopen_state->options, "file");
/* Options that are not handled are only okay if they are unchanged /* Options that are not handled are only okay if they are unchanged
* compared to the old state. It is expected that some options are only * compared to the old state. It is expected that some options are only
* used for the initial open, but not reopen (e.g. filename) */ * used for the initial open, but not reopen (e.g. filename) */

1
include/block/block.h
View File

@ -209,6 +209,7 @@ typedef struct BDRVReopenState {
BlockdevDetectZeroesOptions detect_zeroes; BlockdevDetectZeroesOptions detect_zeroes;
bool backing_missing; bool backing_missing;
BlockDriverState *old_backing_bs; /* keep pointer for permissions update */ BlockDriverState *old_backing_bs; /* keep pointer for permissions update */
BlockDriverState *old_file_bs; /* keep pointer for permissions update */
QDict *options; QDict *options;
QDict *explicit_options; QDict *explicit_options;
void *opaque; void *opaque;

23
tests/qemu-iotests/245
View File

@ -146,8 +146,8 @@ class TestBlockdevReopen(iotests.QMPTestCase):
self.reopen(opts, {'driver': 'raw'}, "Cannot change the option 'driver'") self.reopen(opts, {'driver': 'raw'}, "Cannot change the option 'driver'")
self.reopen(opts, {'driver': ''}, "Invalid parameter ''") self.reopen(opts, {'driver': ''}, "Invalid parameter ''")
self.reopen(opts, {'driver': None}, "Invalid parameter type for 'driver', expected: string") self.reopen(opts, {'driver': None}, "Invalid parameter type for 'driver', expected: string")
self.reopen(opts, {'file': 'not-found'}, "Cannot change the option 'file'") self.reopen(opts, {'file': 'not-found'}, "Cannot find device='' nor node-name='not-found'")
self.reopen(opts, {'file': ''}, "Cannot change the option 'file'") self.reopen(opts, {'file': ''}, "Cannot find device='' nor node-name=''")
self.reopen(opts, {'file': None}, "Invalid parameter type for 'file', expected: BlockdevRef") self.reopen(opts, {'file': None}, "Invalid parameter type for 'file', expected: BlockdevRef")
self.reopen(opts, {'file.node-name': 'newname'}, "Cannot change the option 'node-name'") self.reopen(opts, {'file.node-name': 'newname'}, "Cannot change the option 'node-name'")
self.reopen(opts, {'file.driver': 'host_device'}, "Cannot change the option 'driver'") self.reopen(opts, {'file.driver': 'host_device'}, "Cannot change the option 'driver'")
@ -443,7 +443,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
# Illegal operation: hd2 is a child of hd1 # Illegal operation: hd2 is a child of hd1
self.reopen(opts[2], {'backing': 'hd1'}, self.reopen(opts[2], {'backing': 'hd1'},
"Making 'hd1' a backing file of 'hd2' would create a cycle") "Making 'hd1' a backing child of 'hd2' would create a cycle")
# hd2 <- hd0, hd2 <- hd1 # hd2 <- hd0, hd2 <- hd1
self.reopen(opts[0], {'backing': 'hd2'}) self.reopen(opts[0], {'backing': 'hd2'})
@ -454,8 +454,9 @@ class TestBlockdevReopen(iotests.QMPTestCase):
# More illegal operations # More illegal operations
self.reopen(opts[2], {'backing': 'hd1'}, self.reopen(opts[2], {'backing': 'hd1'},
"Making 'hd1' a backing file of 'hd2' would create a cycle") "Making 'hd1' a backing child of 'hd2' would create a cycle")
self.reopen(opts[2], {'file': 'hd0-file'}, "Cannot change the option 'file'") self.reopen(opts[2], {'file': 'hd0-file'},
"Permission conflict on node 'hd0-file': permissions 'write, resize' are both required by node 'hd0' (uses node 'hd0-file' as 'file' child) and unshared by node 'hd2' (uses node 'hd0-file' as 'file' child).")
result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'hd2') result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'hd2')
self.assert_qmp(result, 'error/class', 'GenericError') self.assert_qmp(result, 'error/class', 'GenericError')
@ -497,18 +498,18 @@ class TestBlockdevReopen(iotests.QMPTestCase):
# Illegal: hd2 is backed by hd1 # Illegal: hd2 is backed by hd1
self.reopen(opts[1], {'backing': 'hd2'}, self.reopen(opts[1], {'backing': 'hd2'},
"Making 'hd2' a backing file of 'hd1' would create a cycle") "Making 'hd2' a backing child of 'hd1' would create a cycle")
# hd1 <- hd0 <- hd2 # hd1 <- hd0 <- hd2
self.reopen(opts[2], {'backing': 'hd0'}) self.reopen(opts[2], {'backing': 'hd0'})
# Illegal: hd2 is backed by hd0, which is backed by hd1 # Illegal: hd2 is backed by hd0, which is backed by hd1
self.reopen(opts[1], {'backing': 'hd2'}, self.reopen(opts[1], {'backing': 'hd2'},
"Making 'hd2' a backing file of 'hd1' would create a cycle") "Making 'hd2' a backing child of 'hd1' would create a cycle")
# Illegal: hd1 cannot point to itself # Illegal: hd1 cannot point to itself
self.reopen(opts[1], {'backing': 'hd1'}, self.reopen(opts[1], {'backing': 'hd1'},
"Making 'hd1' a backing file of 'hd1' would create a cycle") "Making 'hd1' a backing child of 'hd1' would create a cycle")
# Remove all backing files # Remove all backing files
self.reopen(opts[0]) self.reopen(opts[0])
@ -530,7 +531,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
# Illegal: hd0 is a child of the blkverify node # Illegal: hd0 is a child of the blkverify node
self.reopen(opts[0], {'backing': 'bv'}, self.reopen(opts[0], {'backing': 'bv'},
"Making 'bv' a backing file of 'hd0' would create a cycle") "Making 'bv' a backing child of 'hd0' would create a cycle")
# Delete the blkverify node # Delete the blkverify node
result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'bv') result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'bv')
@ -563,7 +564,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
# You can't make quorum0 a backing file of hd0: # You can't make quorum0 a backing file of hd0:
# hd0 is already a child of quorum0. # hd0 is already a child of quorum0.
self.reopen(hd_opts(0), {'backing': 'quorum0'}, self.reopen(hd_opts(0), {'backing': 'quorum0'},
"Making 'quorum0' a backing file of 'hd0' would create a cycle") "Making 'quorum0' a backing child of 'hd0' would create a cycle")
# Delete quorum0 # Delete quorum0
result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'quorum0') result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'quorum0')
@ -969,7 +970,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
# We can't remove hd1 while the commit job is ongoing # We can't remove hd1 while the commit job is ongoing
opts['backing'] = None opts['backing'] = None
self.reopen(opts, {}, "Cannot change backing link if 'hd0' has an implicit backing file") self.reopen(opts, {}, "Cannot replace implicit backing child of hd0")
# hd2 <- hd0 # hd2 <- hd0
self.vm.run_job('commit0', auto_finalize = False, auto_dismiss = True) self.vm.run_job('commit0', auto_finalize = False, auto_dismiss = True)