* Re: [Qemu-devel] [PATCH v2] blockdev: Print a warning for legacy drive options that belong to -device
2017-05-12 10:33 [Qemu-devel] [PATCH v2] blockdev: Print a warning for legacy drive options that belong to -device Thomas Huth
@ 2017-05-29 16:09 ` Thomas Huth
2017-05-30 5:20 ` Markus Armbruster
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Thomas Huth @ 2017-05-29 16:09 UTC (permalink / raw)
To: qemu-devel, Kevin Wolf, Max Reitz; +Cc: qemu-block
On 12.05.2017 12:33, Thomas Huth wrote:
> We likely do not want to carry these legacy -drive options along forever.
> Let's emit a deprecation warning for the -drive options that have a
> replacement with the -device option, so that the (hopefully few) remaining
> users are aware of this and can adapt their scripts / behaviour accordingly.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> v2:
> - Check for !qtest_enabled() since tests/hd-geo-test still uses these
> - Added "addr" to the list, too
> - Also mark the options as deprecated in the documentation
>
> blockdev.c | 14 ++++++++++++++
> qemu-options.hx | 5 ++++-
> 2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 0b38c3d..aef38f0 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -50,6 +50,7 @@
> #include "qmp-commands.h"
> #include "block/trace.h"
> #include "sysemu/arch_init.h"
> +#include "sysemu/qtest.h"
> #include "qemu/cutils.h"
> #include "qemu/help_option.h"
> #include "qemu/throttle-options.h"
> @@ -797,6 +798,9 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> const char *filename;
> Error *local_err = NULL;
> int i;
> + const char *deprecated[] = {
> + "serial", "trans", "secs", "heads", "cyls", "addr"
> + };
>
> /* Change legacy command line options into QMP ones */
> static const struct {
> @@ -880,6 +884,16 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> "update your scripts.\n");
> }
>
> + /* Other deprecated options */
> + if (!qtest_enabled()) {
> + for (i = 0; i < ARRAY_SIZE(deprecated); i++) {
> + if (qemu_opt_get(legacy_opts, deprecated[i]) != NULL) {
> + error_report("'%s' is deprecated, please use the corresponding "
> + "option of '-device' instead", deprecated[i]);
> + }
> + }
> + }
> +
> /* Media type */
> value = qemu_opt_get(legacy_opts, "media");
> if (value) {
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 9d7964d..2f66f1a 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -615,6 +615,8 @@ of available connectors of a given interface type.
> This option defines the type of the media: disk or cdrom.
> @item cyls=@var{c},heads=@var{h},secs=@var{s}[,trans=@var{t}]
> These options have the same definition as they have in @option{-hdachs}.
> +These parameters are deprecated, use the corresponding parameters
> +of @code{-device} instead.
> @item snapshot=@var{snapshot}
> @var{snapshot} is "on" or "off" and controls snapshot mode for the given drive
> (see @option{-snapshot}).
> @@ -631,7 +633,8 @@ an untrusted format header.
> @item serial=@var{serial}
> This option specifies the serial number to assign to the device.
> @item addr=@var{addr}
> -Specify the controller's PCI address (if=virtio only).
> +Specify the controller's PCI address (if=virtio only). This parameter is
> +deprecated, use the corresponding parameter of @code{-device} instead.
> @item werror=@var{action},rerror=@var{action}
> Specify which @var{action} to take on write and read errors. Valid actions are:
> "ignore" (ignore the error and try to continue), "stop" (pause QEMU),
>
Ping ... any comments on this version of my patch?
Thomas
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v2] blockdev: Print a warning for legacy drive options that belong to -device
2017-05-12 10:33 [Qemu-devel] [PATCH v2] blockdev: Print a warning for legacy drive options that belong to -device Thomas Huth
2017-05-29 16:09 ` Thomas Huth
@ 2017-05-30 5:20 ` Markus Armbruster
2017-05-30 7:45 ` Thomas Huth
2017-06-23 16:41 ` Thomas Huth
2017-07-04 14:40 ` Kevin Wolf
3 siblings, 1 reply; 6+ messages in thread
From: Markus Armbruster @ 2017-05-30 5:20 UTC (permalink / raw)
To: Thomas Huth; +Cc: qemu-devel, Kevin Wolf, Max Reitz, qemu-block
Thomas Huth <thuth@redhat.com> writes:
> We likely do not want to carry these legacy -drive options along forever.
> Let's emit a deprecation warning for the -drive options that have a
> replacement with the -device option, so that the (hopefully few) remaining
> users are aware of this and can adapt their scripts / behaviour accordingly.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> v2:
> - Check for !qtest_enabled() since tests/hd-geo-test still uses these
> - Added "addr" to the list, too
> - Also mark the options as deprecated in the documentation
>
> blockdev.c | 14 ++++++++++++++
> qemu-options.hx | 5 ++++-
> 2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 0b38c3d..aef38f0 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -50,6 +50,7 @@
> #include "qmp-commands.h"
> #include "block/trace.h"
> #include "sysemu/arch_init.h"
> +#include "sysemu/qtest.h"
> #include "qemu/cutils.h"
> #include "qemu/help_option.h"
> #include "qemu/throttle-options.h"
> @@ -797,6 +798,9 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> const char *filename;
> Error *local_err = NULL;
> int i;
> + const char *deprecated[] = {
> + "serial", "trans", "secs", "heads", "cyls", "addr"
> + };
I know I worked on turning (some of?) these into qdev properties, but
I've since forgotten the details, so let me review their current state
real quick:
* "serial": Silently ignored unless the device model chooses to pick it
up. Device models with a serial number should pick it up as a
compatibility fallback for their qdev property, with blkconf_serial().
Goes back to 2010-2011:
a8686a9 virtio-blk: Turn drive serial into a qdev property
c3a90cb usb-storage: Turn drive serial into a qdev property usb-storage.serial
a0fef65 scsi: Turn drive serial into a qdev property scsi-disk.serial
6ced55a ide: Turn drive serial into a qdev property ide-drive.serial
* "trans", "secs", "heads", "cyls": Similar, with blkconf_geometry().
Goes back to 2012.
6e6f61a ide: qdev property for BIOS CHS translation
ba80196 ide: qdev properties for disk geometry
e63e7fd virtio-blk: qdev properties for disk geometry
d252df4 scsi-hd: qdev properties for disk geometry
* "addr": Only accepted with if=virtio. drive_new() desugars it into
the qdev property.
Okay.
>
> /* Change legacy command line options into QMP ones */
> static const struct {
> @@ -880,6 +884,16 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
if (qemu_opt_get(legacy_opts, "boot") != NULL) {
fprintf(stderr, "qemu-kvm: boot=on|off is deprecated and will be "
"ignored. Future versions will reject this parameter. Please "
> "update your scripts.\n");
Unrelated to this patch: this is ugly. It's also almost three years
old. Can we bury the corpse already?
> }
>
> + /* Other deprecated options */
> + if (!qtest_enabled()) {
> + for (i = 0; i < ARRAY_SIZE(deprecated); i++) {
> + if (qemu_opt_get(legacy_opts, deprecated[i]) != NULL) {
> + error_report("'%s' is deprecated, please use the corresponding "
> + "option of '-device' instead", deprecated[i]);
> + }
> + }
> + }
> +
> /* Media type */
> value = qemu_opt_get(legacy_opts, "media");
> if (value) {
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 9d7964d..2f66f1a 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -615,6 +615,8 @@ of available connectors of a given interface type.
> This option defines the type of the media: disk or cdrom.
> @item cyls=@var{c},heads=@var{h},secs=@var{s}[,trans=@var{t}]
> These options have the same definition as they have in @option{-hdachs}.
> +These parameters are deprecated, use the corresponding parameters
> +of @code{-device} instead.
> @item snapshot=@var{snapshot}
> @var{snapshot} is "on" or "off" and controls snapshot mode for the given drive
> (see @option{-snapshot}).
> @@ -631,7 +633,8 @@ an untrusted format header.
> @item serial=@var{serial}
> This option specifies the serial number to assign to the device.
> @item addr=@var{addr}
> -Specify the controller's PCI address (if=virtio only).
> +Specify the controller's PCI address (if=virtio only). This parameter is
> +deprecated, use the corresponding parameter of @code{-device} instead.
> @item werror=@var{action},rerror=@var{action}
> Specify which @var{action} to take on write and read errors. Valid actions are:
> "ignore" (ignore the error and try to continue), "stop" (pause QEMU),
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v2] blockdev: Print a warning for legacy drive options that belong to -device
2017-05-30 5:20 ` Markus Armbruster
@ 2017-05-30 7:45 ` Thomas Huth
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Huth @ 2017-05-30 7:45 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Kevin Wolf, Max Reitz, qemu-block
On 30.05.2017 07:20, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
>
>> We likely do not want to carry these legacy -drive options along forever.
>> Let's emit a deprecation warning for the -drive options that have a
>> replacement with the -device option, so that the (hopefully few) remaining
>> users are aware of this and can adapt their scripts / behaviour accordingly.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
[...]
>> /* Change legacy command line options into QMP ones */
>> static const struct {
>> @@ -880,6 +884,16 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> if (qemu_opt_get(legacy_opts, "boot") != NULL) {
> fprintf(stderr, "qemu-kvm: boot=on|off is deprecated and will be "
> "ignored. Future versions will reject this parameter. Please "
>> "update your scripts.\n");
>
> Unrelated to this patch: this is ugly. It's also almost three years
> old. Can we bury the corpse already?
If you like to get rid of this now, feel free to send a patch ...
otherwise I'll make sure that it'll go away with QEMU v3.0 (it's on the
to-be-removed list on http://wiki.qemu.org/Features/LegacyRemoval already)
[...]
>> @@ -631,7 +633,8 @@ an untrusted format header.
>> @item serial=@var{serial}
>> This option specifies the serial number to assign to the device.
>> @item addr=@var{addr}
>> -Specify the controller's PCI address (if=virtio only).
>> +Specify the controller's PCI address (if=virtio only). This parameter is
>> +deprecated, use the corresponding parameter of @code{-device} instead.
>> @item werror=@var{action},rerror=@var{action}
>> Specify which @var{action} to take on write and read errors. Valid actions are:
>> "ignore" (ignore the error and try to continue), "stop" (pause QEMU),
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
Thanks!
Thomas
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] blockdev: Print a warning for legacy drive options that belong to -device
2017-05-12 10:33 [Qemu-devel] [PATCH v2] blockdev: Print a warning for legacy drive options that belong to -device Thomas Huth
2017-05-29 16:09 ` Thomas Huth
2017-05-30 5:20 ` Markus Armbruster
@ 2017-06-23 16:41 ` Thomas Huth
2017-07-04 14:40 ` Kevin Wolf
3 siblings, 0 replies; 6+ messages in thread
From: Thomas Huth @ 2017-06-23 16:41 UTC (permalink / raw)
To: qemu-devel, Kevin Wolf, Max Reitz; +Cc: Markus Armbruster, qemu-block
On 12.05.2017 12:33, Thomas Huth wrote:
> We likely do not want to carry these legacy -drive options along forever.
> Let's emit a deprecation warning for the -drive options that have a
> replacement with the -device option, so that the (hopefully few) remaining
> users are aware of this and can adapt their scripts / behaviour accordingly.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> v2:
> - Check for !qtest_enabled() since tests/hd-geo-test still uses these
> - Added "addr" to the list, too
> - Also mark the options as deprecated in the documentation
>
> blockdev.c | 14 ++++++++++++++
> qemu-options.hx | 5 ++++-
> 2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 0b38c3d..aef38f0 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -50,6 +50,7 @@
> #include "qmp-commands.h"
> #include "block/trace.h"
> #include "sysemu/arch_init.h"
> +#include "sysemu/qtest.h"
> #include "qemu/cutils.h"
> #include "qemu/help_option.h"
> #include "qemu/throttle-options.h"
> @@ -797,6 +798,9 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> const char *filename;
> Error *local_err = NULL;
> int i;
> + const char *deprecated[] = {
> + "serial", "trans", "secs", "heads", "cyls", "addr"
> + };
>
> /* Change legacy command line options into QMP ones */
> static const struct {
> @@ -880,6 +884,16 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> "update your scripts.\n");
> }
>
> + /* Other deprecated options */
> + if (!qtest_enabled()) {
> + for (i = 0; i < ARRAY_SIZE(deprecated); i++) {
> + if (qemu_opt_get(legacy_opts, deprecated[i]) != NULL) {
> + error_report("'%s' is deprecated, please use the corresponding "
> + "option of '-device' instead", deprecated[i]);
> + }
> + }
> + }
> +
> /* Media type */
> value = qemu_opt_get(legacy_opts, "media");
> if (value) {
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 9d7964d..2f66f1a 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -615,6 +615,8 @@ of available connectors of a given interface type.
> This option defines the type of the media: disk or cdrom.
> @item cyls=@var{c},heads=@var{h},secs=@var{s}[,trans=@var{t}]
> These options have the same definition as they have in @option{-hdachs}.
> +These parameters are deprecated, use the corresponding parameters
> +of @code{-device} instead.
> @item snapshot=@var{snapshot}
> @var{snapshot} is "on" or "off" and controls snapshot mode for the given drive
> (see @option{-snapshot}).
> @@ -631,7 +633,8 @@ an untrusted format header.
> @item serial=@var{serial}
> This option specifies the serial number to assign to the device.
> @item addr=@var{addr}
> -Specify the controller's PCI address (if=virtio only).
> +Specify the controller's PCI address (if=virtio only). This parameter is
> +deprecated, use the corresponding parameter of @code{-device} instead.
> @item werror=@var{action},rerror=@var{action}
> Specify which @var{action} to take on write and read errors. Valid actions are:
> "ignore" (ignore the error and try to continue), "stop" (pause QEMU),
>
ping²
Any takers?
Thomas
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v2] blockdev: Print a warning for legacy drive options that belong to -device
2017-05-12 10:33 [Qemu-devel] [PATCH v2] blockdev: Print a warning for legacy drive options that belong to -device Thomas Huth
` (2 preceding siblings ...)
2017-06-23 16:41 ` Thomas Huth
@ 2017-07-04 14:40 ` Kevin Wolf
3 siblings, 0 replies; 6+ messages in thread
From: Kevin Wolf @ 2017-07-04 14:40 UTC (permalink / raw)
To: Thomas Huth; +Cc: qemu-devel, Max Reitz, qemu-block, Markus Armbruster
Am 12.05.2017 um 12:33 hat Thomas Huth geschrieben:
> We likely do not want to carry these legacy -drive options along forever.
> Let's emit a deprecation warning for the -drive options that have a
> replacement with the -device option, so that the (hopefully few) remaining
> users are aware of this and can adapt their scripts / behaviour accordingly.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> v2:
> - Check for !qtest_enabled() since tests/hd-geo-test still uses these
> - Added "addr" to the list, too
> - Also mark the options as deprecated in the documentation
Thanks, added the missing deprecation note for 'serial' in the
documentation and applied to the block branch.
Kevin
^ permalink raw reply [flat|nested] 6+ messages in thread