| Message ID | 1314211389-28915-13-git-send-email-aliguori@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
Am 24.08.2011 20:43, schrieb Anthony Liguori: > A new QMP only command to change the blockdev associated with a block device. > The semantics of change right now are just plain scary. This command introduces > sane semantics. For more details, see the documentation in the schema file. Changes to semantics should be mentioned in the commit message, and in more detail than just that they are sane (the documentation in the schema file doesn't describe differences to the old command). Just assuming that you are right about sanity, I wonder how sanity and compatibility could possibly be achieved at once... I suspect the semantics is clearer now in that it doesn't overload one command with block and VNC subcommands, but I doubt that the semantics of the block version is much better than before. So please call the command drive_change, it's similar to drive_add and drive_del, not to the future blockdev_add/remove. > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > --- > blockdev.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++--- > qapi-schema.json | 30 +++++++++++++++ > qmp-commands.hx | 8 ++++ > 3 files changed, 140 insertions(+), 6 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index 37b2f29..c00c69d 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -697,12 +697,101 @@ void qmp_block_passwd(const char *device, const char *password, Error **err) > qmp_set_blockdev_password(device, password, err); > } > > +static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename, > + int bdrv_flags, BlockDriver *drv, > + const char *password, Error **errp) > +{ > + if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) { > + error_set(errp, QERR_OPEN_FILE_FAILED, filename); > + return; > + } > + > + if (bdrv_key_required(bs)) { > + if (password) { > + if (bdrv_set_key(bs, password) < 0) { > + error_set(errp, QERR_INVALID_PASSWORD); > + } > + } else { > + error_set(errp, QERR_DEVICE_ENCRYPTED, bdrv_get_device_name(bs), > + bdrv_get_encrypted_filename(bs)); > + } > + } else if (password) { > + error_set(errp, QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs)); > + } > +} > + > +void qmp_change_blockdev(const char *device, const char *filename, > + bool has_format, const char *format, > + bool has_password, const char *password, > + Error **errp) > +{ > + BlockDriverState *bs, *bs1; > + BlockDriver *drv = NULL; > + int bdrv_flags; > + Error *err = NULL; > + bool probed_raw = false; > + > + bs = bdrv_find(device); > + if (!bs) { > + error_set(errp, QERR_DEVICE_NOT_FOUND, device); > + return; > + } > + > + if (has_format) { > + drv = bdrv_find_whitelisted_format(format); > + if (!drv) { > + error_set(errp, QERR_INVALID_BLOCK_FORMAT, format); > + return; > + } > + } > + > + bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR; > + bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0; > + > + if (!has_password) { > + password = NULL; > + } > + > + /* Try to open once with a temporary block device to make sure that > + * the disk isn't encrypted and we lack a key. This also helps keep > + * this operation as a transaction. That is, if we fail, we try very > + * hard to make sure that the state is the same as before the operation > + * was started. > + */ > + bs1 = bdrv_new(""); > + qmp_bdrv_open_encrypted(bs1, filename, bdrv_flags, drv, password, &err); > + if (!has_format && bs1->drv->unsafe_probe) { > + probed_raw = true; > + } > + bdrv_delete(bs1); > + > + if (err) { > + error_propagate(errp, err); > + return; > + } > + > + if (probed_raw) { > + /* We will not auto probe raw files. */ > + error_set(errp, QERR_MISSING_PARAMETER, "format"); > + return; > + } > + > + eject_device(bs, 0, &err); > + if (err) { > + error_propagate(errp, err); > + return; > + } > + > + qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, password, errp); > +} Can't this share code with the old command? I don't like this duplication. Kevin
On 08/25/2011 07:46 AM, Kevin Wolf wrote: > Am 24.08.2011 20:43, schrieb Anthony Liguori: >> A new QMP only command to change the blockdev associated with a block device. >> The semantics of change right now are just plain scary. This command introduces >> sane semantics. For more details, see the documentation in the schema file. > > Changes to semantics should be mentioned in the commit message, and in > more detail than just that they are sane (the documentation in the > schema file doesn't describe differences to the old command). Right, but the schema describes the old command. There are a couple major differences with the new command vs old: 1) If you had a block device named 'vnc', the old command is broken. 2) For an encrypted device, the old change command returns an error but has semantically completed the operation. The usage is: (qemu) change ide0 foo.img Error: device ide0 is encrypted (qemu) block_passwd ide0 secret Because even though an error is returned, the change operation has actually succeeded. The new command has transactional semantics. If an error is returned, nothing has happened. To set passwords, an additional option is supported to specify the password. So you would do: (qemu) change-blockdev ide0 foo.img Error: device ide0 is encrypted (qemu) change-blockdev ide0 foo.img secret (qemu) In addition, change-blockdev will not allow you to perform unsafe probing. You can only change to a raw device if you specify a raw format. > > Just assuming that you are right about sanity, I wonder how sanity and > compatibility could possibly be achieved at once... I suspect the > semantics is clearer now in that it doesn't overload one command with > block and VNC subcommands, but I doubt that the semantics of the block > version is much better than before. I don't see how we can change the behavior of the change command. > So please call the command drive_change, it's similar to drive_add and > drive_del, not to the future blockdev_add/remove. Ack. >> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> >> --- >> blockdev.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++--- >> qapi-schema.json | 30 +++++++++++++++ >> qmp-commands.hx | 8 ++++ >> 3 files changed, 140 insertions(+), 6 deletions(-) >> >> diff --git a/blockdev.c b/blockdev.c >> index 37b2f29..c00c69d 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -697,12 +697,101 @@ void qmp_block_passwd(const char *device, const char *password, Error **err) >> qmp_set_blockdev_password(device, password, err); >> } >> >> +static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename, >> + int bdrv_flags, BlockDriver *drv, >> + const char *password, Error **errp) >> +{ >> + if (bdrv_open(bs, filename, bdrv_flags, drv)< 0) { >> + error_set(errp, QERR_OPEN_FILE_FAILED, filename); >> + return; >> + } >> + >> + if (bdrv_key_required(bs)) { >> + if (password) { >> + if (bdrv_set_key(bs, password)< 0) { >> + error_set(errp, QERR_INVALID_PASSWORD); >> + } >> + } else { >> + error_set(errp, QERR_DEVICE_ENCRYPTED, bdrv_get_device_name(bs), >> + bdrv_get_encrypted_filename(bs)); >> + } >> + } else if (password) { >> + error_set(errp, QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs)); >> + } >> +} >> + >> +void qmp_change_blockdev(const char *device, const char *filename, >> + bool has_format, const char *format, >> + bool has_password, const char *password, >> + Error **errp) >> +{ >> + BlockDriverState *bs, *bs1; >> + BlockDriver *drv = NULL; >> + int bdrv_flags; >> + Error *err = NULL; >> + bool probed_raw = false; >> + >> + bs = bdrv_find(device); >> + if (!bs) { >> + error_set(errp, QERR_DEVICE_NOT_FOUND, device); >> + return; >> + } >> + >> + if (has_format) { >> + drv = bdrv_find_whitelisted_format(format); >> + if (!drv) { >> + error_set(errp, QERR_INVALID_BLOCK_FORMAT, format); >> + return; >> + } >> + } >> + >> + bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR; >> + bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0; >> + >> + if (!has_password) { >> + password = NULL; >> + } >> + >> + /* Try to open once with a temporary block device to make sure that >> + * the disk isn't encrypted and we lack a key. This also helps keep >> + * this operation as a transaction. That is, if we fail, we try very >> + * hard to make sure that the state is the same as before the operation >> + * was started. >> + */ >> + bs1 = bdrv_new(""); >> + qmp_bdrv_open_encrypted(bs1, filename, bdrv_flags, drv, password,&err); >> + if (!has_format&& bs1->drv->unsafe_probe) { >> + probed_raw = true; >> + } >> + bdrv_delete(bs1); >> + >> + if (err) { >> + error_propagate(errp, err); >> + return; >> + } >> + >> + if (probed_raw) { >> + /* We will not auto probe raw files. */ >> + error_set(errp, QERR_MISSING_PARAMETER, "format"); >> + return; >> + } >> + >> + eject_device(bs, 0,&err); >> + if (err) { >> + error_propagate(errp, err); >> + return; >> + } >> + >> + qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, password, errp); >> +} > > Can't this share code with the old command? I don't like this duplication. qmp_bdrv_open_encrypted() is the code that can be shared. The next patch I think changes the old command to use this function. Regards, Anthony Liguori > Kevin >
Am 25.08.2011 14:56, schrieb Anthony Liguori: > On 08/25/2011 07:46 AM, Kevin Wolf wrote: >> Am 24.08.2011 20:43, schrieb Anthony Liguori: >>> A new QMP only command to change the blockdev associated with a block device. >>> The semantics of change right now are just plain scary. This command introduces >>> sane semantics. For more details, see the documentation in the schema file. >> >> Changes to semantics should be mentioned in the commit message, and in >> more detail than just that they are sane (the documentation in the >> schema file doesn't describe differences to the old command). > > Right, but the schema describes the old command. > > There are a couple major differences with the new command vs old: > > 1) If you had a block device named 'vnc', the old command is broken. > > 2) For an encrypted device, the old change command returns an error but > has semantically completed the operation. The usage is: > > (qemu) change ide0 foo.img > Error: device ide0 is encrypted > (qemu) block_passwd ide0 secret > > Because even though an error is returned, the change operation has > actually succeeded. The new command has transactional semantics. If an > error is returned, nothing has happened. To set passwords, an > additional option is supported to specify the password. So you would do: > > (qemu) change-blockdev ide0 foo.img > Error: device ide0 is encrypted > (qemu) change-blockdev ide0 foo.img secret > (qemu) Does it really work like this? We need to improve the HMP implementation to ask for the password when needed instead of requiring users to put it as plain text in their command. > In addition, change-blockdev will not allow you to perform unsafe > probing. You can only change to a raw device if you specify a raw format. Makes sense for QMP, but I would consider it a bug for HMP. So HMP code should deal with it and allow using raw without specifying the format. qemu is getting worse and worse to use without a management tool. :-( Kevin
On 08/25/2011 08:47 AM, Kevin Wolf wrote: > Am 25.08.2011 14:56, schrieb Anthony Liguori: >> On 08/25/2011 07:46 AM, Kevin Wolf wrote: >>> Am 24.08.2011 20:43, schrieb Anthony Liguori: >>>> A new QMP only command to change the blockdev associated with a block device. >>>> The semantics of change right now are just plain scary. This command introduces >>>> sane semantics. For more details, see the documentation in the schema file. >>> >>> Changes to semantics should be mentioned in the commit message, and in >>> more detail than just that they are sane (the documentation in the >>> schema file doesn't describe differences to the old command). >> >> Right, but the schema describes the old command. >> >> There are a couple major differences with the new command vs old: >> >> 1) If you had a block device named 'vnc', the old command is broken. >> >> 2) For an encrypted device, the old change command returns an error but >> has semantically completed the operation. The usage is: >> >> (qemu) change ide0 foo.img >> Error: device ide0 is encrypted >> (qemu) block_passwd ide0 secret >> >> Because even though an error is returned, the change operation has >> actually succeeded. The new command has transactional semantics. If an >> error is returned, nothing has happened. To set passwords, an >> additional option is supported to specify the password. So you would do: >> >> (qemu) change-blockdev ide0 foo.img >> Error: device ide0 is encrypted >> (qemu) change-blockdev ide0 foo.img secret >> (qemu) > > Does it really work like this? We need to improve the HMP implementation > to ask for the password when needed instead of requiring users to put it > as plain text in their command. I didn't add an HMP version of change-blockdev. change is fine for HMP. And change does prompt for password both for VNC and block. The main reason for introducing change-blockdev because the only way I could make the HMP change command work and be implemented in terms of QMP was to use the change-blockdev command. > >> In addition, change-blockdev will not allow you to perform unsafe >> probing. You can only change to a raw device if you specify a raw format. > > Makes sense for QMP, but I would consider it a bug for HMP. So HMP code > should deal with it and allow using raw without specifying the format. Yup, that's how change behaves. > qemu is getting worse and worse to use without a management tool. :-( One thing that's different now is that HMP and QMP are fundamentally different interfaces. I hope this gives us the flexibility to implement better HMP commands that we would never expose in QMP and vice versa. As long as HMP commands are implemented in terms of QMP commands, we should be very open and aggressive about improving HMP usablity. Regards, Anthony Liguori > > Kevin >
On Wed, 24 Aug 2011 13:43:07 -0500 Anthony Liguori <aliguori@us.ibm.com> wrote: > A new QMP only command to change the blockdev associated with a block device. > The semantics of change right now are just plain scary. This command introduces > sane semantics. For more details, see the documentation in the schema file. IMO, this has to be split into two commits. First you introduce the new command and then you fix do_change_block(). Also, there's a small problem with the generated code: it has to return -1 on errors. The monitor expects a -1 return _and_ a qerror object on errors. The generated code in middle mode is returning 0 even on errors, which makes QMP to emit a UndefinedError error by default. (it's probably not worth it to discuss the merits of having two ways of signaling errors in the monitor, as this is all going to be dropped pretty soon). > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > --- > blockdev.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++--- > qapi-schema.json | 30 +++++++++++++++ > qmp-commands.hx | 8 ++++ > 3 files changed, 140 insertions(+), 6 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index 37b2f29..c00c69d 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -697,12 +697,101 @@ void qmp_block_passwd(const char *device, const char *password, Error **err) > qmp_set_blockdev_password(device, password, err); > } > > +static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename, > + int bdrv_flags, BlockDriver *drv, > + const char *password, Error **errp) > +{ > + if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) { > + error_set(errp, QERR_OPEN_FILE_FAILED, filename); > + return; > + } > + > + if (bdrv_key_required(bs)) { > + if (password) { > + if (bdrv_set_key(bs, password) < 0) { > + error_set(errp, QERR_INVALID_PASSWORD); > + } > + } else { > + error_set(errp, QERR_DEVICE_ENCRYPTED, bdrv_get_device_name(bs), > + bdrv_get_encrypted_filename(bs)); > + } > + } else if (password) { > + error_set(errp, QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs)); > + } > +} > + > +void qmp_change_blockdev(const char *device, const char *filename, > + bool has_format, const char *format, > + bool has_password, const char *password, > + Error **errp) > +{ > + BlockDriverState *bs, *bs1; > + BlockDriver *drv = NULL; > + int bdrv_flags; > + Error *err = NULL; > + bool probed_raw = false; > + > + bs = bdrv_find(device); > + if (!bs) { > + error_set(errp, QERR_DEVICE_NOT_FOUND, device); > + return; > + } > + > + if (has_format) { > + drv = bdrv_find_whitelisted_format(format); > + if (!drv) { > + error_set(errp, QERR_INVALID_BLOCK_FORMAT, format); > + return; > + } > + } > + > + bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR; > + bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0; > + > + if (!has_password) { > + password = NULL; > + } > + > + /* Try to open once with a temporary block device to make sure that > + * the disk isn't encrypted and we lack a key. This also helps keep > + * this operation as a transaction. That is, if we fail, we try very > + * hard to make sure that the state is the same as before the operation > + * was started. > + */ > + bs1 = bdrv_new(""); > + qmp_bdrv_open_encrypted(bs1, filename, bdrv_flags, drv, password, &err); > + if (!has_format && bs1->drv->unsafe_probe) { > + probed_raw = true; > + } > + bdrv_delete(bs1); > + > + if (err) { > + error_propagate(errp, err); > + return; > + } > + > + if (probed_raw) { > + /* We will not auto probe raw files. */ > + error_set(errp, QERR_MISSING_PARAMETER, "format"); > + return; > + } > + > + eject_device(bs, 0, &err); > + if (err) { > + error_propagate(errp, err); > + return; > + } > + > + qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, password, errp); > +} > + > int do_change_block(Monitor *mon, const char *device, > const char *filename, const char *fmt) > { > BlockDriverState *bs; > BlockDriver *drv = NULL; > int bdrv_flags; > + Error *err = NULL; > > bs = bdrv_find(device); > if (!bs) { > @@ -716,16 +805,23 @@ int do_change_block(Monitor *mon, const char *device, > return -1; > } > } > - if (eject_device(bs, 0, NULL) < 0) { > - return -1; > - } > + > bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR; > bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0; > - if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) { > - qerror_report(QERR_OPEN_FILE_FAILED, filename); > + > + eject_device(bs, 0, &err); > + if (err) { > + qerror_report_err(err); > + return -1; > + } > + > + qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, &err); > + if (err) { > + qerror_report_err(err); > return -1; > } > - return monitor_read_bdrv_key_start(mon, bs, NULL, NULL); > + > + return 0; > } > > int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data) > diff --git a/qapi-schema.json b/qapi-schema.json > index 211200a..139c6e3 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -123,3 +123,33 @@ > # when this command is executed is undefined. > ## > { 'command': 'change-vnc-listen', 'data': {'target': 'str'} } > + > +## > +# @change-blockdev: > +# > +# This is the preferred interface for changing a block device. > +# > +# @device: The block device name. > +# > +# @filename: The new filename for the block device. This may contain options > +# encoded in a format specified by @format. > +# > +# @format: #optional The format to use open the block device We need a list (or a pointers) of valid formats. > +# > +# @password: #optional The password to use if the block device is encrypted > +# > +# Returns: Nothing on success. > +# If @device is not a valid block device, DeviceNotFound > +# If @format is not a valid block format, InvalidBlockFormat > +# If @filename is encrypted and @password isn't supplied, > +# DeviceEncrypted. The call should be repeated with @password > +# supplied. > +# If @format is not specified and @filename is a format that cannot > +# be safely probed, MissingParameter. > +# If @filename cannot be opened, OpenFileFailed > +# > +# Since: 1.0 > +## > +{ 'command': 'change-blockdev', > + 'data': {'device': 'str', 'filename': 'str', '*format': 'str', > + '*password': 'str'} } > diff --git a/qmp-commands.hx b/qmp-commands.hx > index c0d0ca3..cec7135 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -121,6 +121,14 @@ EQMP > .mhandler.cmd_new = do_change, > }, > > + { > + .name = "change-blockdev", > + .args_type = "device:B,target:F,arg:s?pass:s?", The arguments names don't match the schema. > + .params = "device filename [format] [password]", > + .help = "change a removable medium, optional format", > + .mhandler.cmd_new = qmp_marshal_input_change_blockdev, > + }, > + > SQMP > change > ------
On 08/25/2011 09:09 AM, Luiz Capitulino wrote: > On Wed, 24 Aug 2011 13:43:07 -0500 > Anthony Liguori<aliguori@us.ibm.com> wrote: > >> A new QMP only command to change the blockdev associated with a block device. >> The semantics of change right now are just plain scary. This command introduces >> sane semantics. For more details, see the documentation in the schema file. > > IMO, this has to be split into two commits. First you introduce the new command > and then you fix do_change_block(). > > Also, there's a small problem with the generated code: it has to return -1 on > errors. The monitor expects a -1 return _and_ a qerror object on errors. The > generated code in middle mode is returning 0 even on errors, which makes QMP to > emit a UndefinedError error by default. You mean: if (local_err) { qerror_report_err(local_err); error_free(local_err); return -1; } return 0; That should behave exactly the right way as qerror_report_err() propagates sends the Error as a QError. >> + >> +## >> +# @change-blockdev: >> +# >> +# This is the preferred interface for changing a block device. >> +# >> +# @device: The block device name. >> +# >> +# @filename: The new filename for the block device. This may contain options >> +# encoded in a format specified by @format. >> +# >> +# @format: #optional The format to use open the block device > > We need a list (or a pointers) of valid formats. We do, but that needs to be a command because it depends on the build options. >> +# >> +# @password: #optional The password to use if the block device is encrypted >> +# >> +# Returns: Nothing on success. >> +# If @device is not a valid block device, DeviceNotFound >> +# If @format is not a valid block format, InvalidBlockFormat >> +# If @filename is encrypted and @password isn't supplied, >> +# DeviceEncrypted. The call should be repeated with @password >> +# supplied. >> +# If @format is not specified and @filename is a format that cannot >> +# be safely probed, MissingParameter. >> +# If @filename cannot be opened, OpenFileFailed >> +# >> +# Since: 1.0 >> +## >> +{ 'command': 'change-blockdev', >> + 'data': {'device': 'str', 'filename': 'str', '*format': 'str', >> + '*password': 'str'} } >> diff --git a/qmp-commands.hx b/qmp-commands.hx >> index c0d0ca3..cec7135 100644 >> --- a/qmp-commands.hx >> +++ b/qmp-commands.hx >> @@ -121,6 +121,14 @@ EQMP >> .mhandler.cmd_new = do_change, >> }, >> >> + { >> + .name = "change-blockdev", >> + .args_type = "device:B,target:F,arg:s?pass:s?", > > The arguments names don't match the schema. Do we need to set args_type for QMP? Regards, Anthony Liguori >> + .params = "device filename [format] [password]", >> + .help = "change a removable medium, optional format", >> + .mhandler.cmd_new = qmp_marshal_input_change_blockdev, >> + }, >> + >> SQMP >> change >> ------ >
On Thu, 25 Aug 2011 09:21:02 -0500 Anthony Liguori <aliguori@us.ibm.com> wrote: > On 08/25/2011 09:09 AM, Luiz Capitulino wrote: > > On Wed, 24 Aug 2011 13:43:07 -0500 > > Anthony Liguori<aliguori@us.ibm.com> wrote: > > > >> A new QMP only command to change the blockdev associated with a block device. > >> The semantics of change right now are just plain scary. This command introduces > >> sane semantics. For more details, see the documentation in the schema file. > > > > IMO, this has to be split into two commits. First you introduce the new command > > and then you fix do_change_block(). > > > > Also, there's a small problem with the generated code: it has to return -1 on > > errors. The monitor expects a -1 return _and_ a qerror object on errors. The > > generated code in middle mode is returning 0 even on errors, which makes QMP to > > emit a UndefinedError error by default. > > You mean: > > if (local_err) { > qerror_report_err(local_err); > error_free(local_err); > return -1; > } > return 0; > > That should behave exactly the right way as qerror_report_err() > propagates sends the Error as a QError. Oh that's right, what's happening is that qerror_report_err() is returning before setting qerror in the Monitor object... I reported that in my review of patch 01/14. > > >> + > >> +## > >> +# @change-blockdev: > >> +# > >> +# This is the preferred interface for changing a block device. > >> +# > >> +# @device: The block device name. > >> +# > >> +# @filename: The new filename for the block device. This may contain options > >> +# encoded in a format specified by @format. > >> +# > >> +# @format: #optional The format to use open the block device > > > > We need a list (or a pointers) of valid formats. > > We do, but that needs to be a command because it depends on the build > options. > > >> +# > >> +# @password: #optional The password to use if the block device is encrypted > >> +# > >> +# Returns: Nothing on success. > >> +# If @device is not a valid block device, DeviceNotFound > >> +# If @format is not a valid block format, InvalidBlockFormat > >> +# If @filename is encrypted and @password isn't supplied, > >> +# DeviceEncrypted. The call should be repeated with @password > >> +# supplied. > >> +# If @format is not specified and @filename is a format that cannot > >> +# be safely probed, MissingParameter. > >> +# If @filename cannot be opened, OpenFileFailed > >> +# > >> +# Since: 1.0 > >> +## > >> +{ 'command': 'change-blockdev', > >> + 'data': {'device': 'str', 'filename': 'str', '*format': 'str', > >> + '*password': 'str'} } > >> diff --git a/qmp-commands.hx b/qmp-commands.hx > >> index c0d0ca3..cec7135 100644 > >> --- a/qmp-commands.hx > >> +++ b/qmp-commands.hx > >> @@ -121,6 +121,14 @@ EQMP > >> .mhandler.cmd_new = do_change, > >> }, > >> > >> + { > >> + .name = "change-blockdev", > >> + .args_type = "device:B,target:F,arg:s?pass:s?", > > > > The arguments names don't match the schema. > > Do we need to set args_type for QMP? Yes. > > Regards, > > Anthony Liguori > > > >> + .params = "device filename [format] [password]", > >> + .help = "change a removable medium, optional format", > >> + .mhandler.cmd_new = qmp_marshal_input_change_blockdev, > >> + }, > >> + > >> SQMP > >> change > >> ------ > > >
diff --git a/blockdev.c b/blockdev.c index 37b2f29..c00c69d 100644 --- a/blockdev.c +++ b/blockdev.c @@ -697,12 +697,101 @@ void qmp_block_passwd(const char *device, const char *password, Error **err) qmp_set_blockdev_password(device, password, err); } +static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename, + int bdrv_flags, BlockDriver *drv, + const char *password, Error **errp) +{ + if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) { + error_set(errp, QERR_OPEN_FILE_FAILED, filename); + return; + } + + if (bdrv_key_required(bs)) { + if (password) { + if (bdrv_set_key(bs, password) < 0) { + error_set(errp, QERR_INVALID_PASSWORD); + } + } else { + error_set(errp, QERR_DEVICE_ENCRYPTED, bdrv_get_device_name(bs), + bdrv_get_encrypted_filename(bs)); + } + } else if (password) { + error_set(errp, QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs)); + } +} + +void qmp_change_blockdev(const char *device, const char *filename, + bool has_format, const char *format, + bool has_password, const char *password, + Error **errp) +{ + BlockDriverState *bs, *bs1; + BlockDriver *drv = NULL; + int bdrv_flags; + Error *err = NULL; + bool probed_raw = false; + + bs = bdrv_find(device); + if (!bs) { + error_set(errp, QERR_DEVICE_NOT_FOUND, device); + return; + } + + if (has_format) { + drv = bdrv_find_whitelisted_format(format); + if (!drv) { + error_set(errp, QERR_INVALID_BLOCK_FORMAT, format); + return; + } + } + + bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR; + bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0; + + if (!has_password) { + password = NULL; + } + + /* Try to open once with a temporary block device to make sure that + * the disk isn't encrypted and we lack a key. This also helps keep + * this operation as a transaction. That is, if we fail, we try very + * hard to make sure that the state is the same as before the operation + * was started. + */ + bs1 = bdrv_new(""); + qmp_bdrv_open_encrypted(bs1, filename, bdrv_flags, drv, password, &err); + if (!has_format && bs1->drv->unsafe_probe) { + probed_raw = true; + } + bdrv_delete(bs1); + + if (err) { + error_propagate(errp, err); + return; + } + + if (probed_raw) { + /* We will not auto probe raw files. */ + error_set(errp, QERR_MISSING_PARAMETER, "format"); + return; + } + + eject_device(bs, 0, &err); + if (err) { + error_propagate(errp, err); + return; + } + + qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, password, errp); +} + int do_change_block(Monitor *mon, const char *device, const char *filename, const char *fmt) { BlockDriverState *bs; BlockDriver *drv = NULL; int bdrv_flags; + Error *err = NULL; bs = bdrv_find(device); if (!bs) { @@ -716,16 +805,23 @@ int do_change_block(Monitor *mon, const char *device, return -1; } } - if (eject_device(bs, 0, NULL) < 0) { - return -1; - } + bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR; bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0; - if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) { - qerror_report(QERR_OPEN_FILE_FAILED, filename); + + eject_device(bs, 0, &err); + if (err) { + qerror_report_err(err); + return -1; + } + + qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, &err); + if (err) { + qerror_report_err(err); return -1; } - return monitor_read_bdrv_key_start(mon, bs, NULL, NULL); + + return 0; } int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data) diff --git a/qapi-schema.json b/qapi-schema.json index 211200a..139c6e3 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -123,3 +123,33 @@ # when this command is executed is undefined. ## { 'command': 'change-vnc-listen', 'data': {'target': 'str'} } + +## +# @change-blockdev: +# +# This is the preferred interface for changing a block device. +# +# @device: The block device name. +# +# @filename: The new filename for the block device. This may contain options +# encoded in a format specified by @format. +# +# @format: #optional The format to use open the block device +# +# @password: #optional The password to use if the block device is encrypted +# +# Returns: Nothing on success. +# If @device is not a valid block device, DeviceNotFound +# If @format is not a valid block format, InvalidBlockFormat +# If @filename is encrypted and @password isn't supplied, +# DeviceEncrypted. The call should be repeated with @password +# supplied. +# If @format is not specified and @filename is a format that cannot +# be safely probed, MissingParameter. +# If @filename cannot be opened, OpenFileFailed +# +# Since: 1.0 +## +{ 'command': 'change-blockdev', + 'data': {'device': 'str', 'filename': 'str', '*format': 'str', + '*password': 'str'} } diff --git a/qmp-commands.hx b/qmp-commands.hx index c0d0ca3..cec7135 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -121,6 +121,14 @@ EQMP .mhandler.cmd_new = do_change, }, + { + .name = "change-blockdev", + .args_type = "device:B,target:F,arg:s?pass:s?", + .params = "device filename [format] [password]", + .help = "change a removable medium, optional format", + .mhandler.cmd_new = qmp_marshal_input_change_blockdev, + }, + SQMP change ------
A new QMP only command to change the blockdev associated with a block device. The semantics of change right now are just plain scary. This command introduces sane semantics. For more details, see the documentation in the schema file. Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> --- blockdev.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++--- qapi-schema.json | 30 +++++++++++++++ qmp-commands.hx | 8 ++++ 3 files changed, 140 insertions(+), 6 deletions(-)