On Wed, Jun 13, 2018 at 04:18:17PM +0200, Markus Armbruster wrote:
> Kevin Wolf <kw...@redhat.com> writes:
>
> > The -drive option serial was deprecated in QEMU 2.10. It's time to
> > remove it.
> >
> > Tests need to be updated to set the serial number with -global instead
> > of using the -drive option.
> >
> > Signed-off-by: Kevin Wolf <kw...@redhat.com>
Reviewed-by: Jeff Cody <jc...@redhat.com>
> > ---
> > include/hw/block/block.h | 1 -
> > include/sysemu/blockdev.h | 1 -
> > block/block-backend.c | 1 -
> > blockdev.c | 22 ----------------------
> > hw/block/block.c | 13 -------------
> > hw/block/nvme.c | 1 -
> > hw/block/virtio-blk.c | 1 -
> > hw/ide/qdev.c | 1 -
> > hw/scsi/scsi-disk.c | 1 -
> > hw/usb/dev-storage.c | 1 -
> > tests/ahci-test.c | 6 +++---
> > tests/ide-test.c | 8 ++++----
> > qemu-doc.texi | 5 -----
> > qemu-options.hx | 6 +-----
> > 14 files changed, 8 insertions(+), 60 deletions(-)
> >
> > diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> > index d4f4dfffab..e9f9e2223f 100644
> > --- a/include/hw/block/block.h
> > +++ b/include/hw/block/block.h
> > @@ -72,7 +72,6 @@ static inline unsigned int
> > get_physical_block_exp(BlockConf *conf)
> >
> > /* Configuration helpers */
> >
> > -void blkconf_serial(BlockConf *conf, char **serial);
> > bool blkconf_geometry(BlockConf *conf, int *trans,
> > unsigned cyls_max, unsigned heads_max, unsigned
> > secs_max,
> > Error **errp);
> > diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> > index c0ae3700ec..24954b94e0 100644
> > --- a/include/sysemu/blockdev.h
> > +++ b/include/sysemu/blockdev.h
> > @@ -35,7 +35,6 @@ struct DriveInfo {
> > bool is_default; /* Added by default_drive() ? */
> > int media_cd;
> > QemuOpts *opts;
> > - char *serial;
> > QTAILQ_ENTRY(DriveInfo) next;
> > };
> >
> > diff --git a/block/block-backend.c b/block/block-backend.c
> > index d55c328736..2d1a3463e8 100644
> > --- a/block/block-backend.c
> > +++ b/block/block-backend.c
> > @@ -419,7 +419,6 @@ static void drive_info_del(DriveInfo *dinfo)
> > return;
> > }
> > qemu_opts_del(dinfo->opts);
> > - g_free(dinfo->serial);
> > g_free(dinfo);
> > }
> >
> > diff --git a/blockdev.c b/blockdev.c
> > index 83b3cc12e9..4ab3d6ec0b 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -730,10 +730,6 @@ QemuOptsList qemu_legacy_drive_opts = {
> > .type = QEMU_OPT_STRING,
> > .help = "interface (ide, scsi, sd, mtd, floppy, pflash,
> > virtio)",
> > },{
> > - .name = "serial",
> > - .type = QEMU_OPT_STRING,
> > - .help = "disk serial number",
> > - },{
> > .name = "file",
> > .type = QEMU_OPT_STRING,
> > .help = "file name",
> > @@ -775,13 +771,9 @@ DriveInfo *drive_new(QemuOpts *all_opts,
> > BlockInterfaceType block_default_type)
> > const char *werror, *rerror;
> > bool read_only = false;
> > bool copy_on_read;
> > - const char *serial;
> > const char *filename;
> > Error *local_err = NULL;
> > int i;
> > - const char *deprecated[] = {
> > - "serial"
> > - };
> >
> > /* Change legacy command line options into QMP ones */
> > static const struct {
> > @@ -858,16 +850,6 @@ DriveInfo *drive_new(QemuOpts *all_opts,
> > BlockInterfaceType block_default_type)
> > goto fail;
> > }
> >
> > - /* 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) {
>
> I guess I'd remove this in a separate patch, to make bringing it back
> easier in case we need it again.
>
> > @@ -948,9 +930,6 @@ DriveInfo *drive_new(QemuOpts *all_opts,
> > BlockInterfaceType block_default_type)
> > goto fail;
> > }
> >
> > - /* Serial number */
> > - serial = qemu_opt_get(legacy_opts, "serial");
> > -
> > /* no id supplied -> create one */
> > if (qemu_opts_id(all_opts) == NULL) {
> > char *new_id;
> > @@ -1025,7 +1004,6 @@ DriveInfo *drive_new(QemuOpts *all_opts,
> > BlockInterfaceType block_default_type)
> > dinfo->type = type;
> > dinfo->bus = bus_id;
> > dinfo->unit = unit_id;
> > - dinfo->serial = g_strdup(serial);
> >
> > blk_set_legacy_dinfo(blk, dinfo);
> >
> > diff --git a/hw/block/block.c b/hw/block/block.c
> > index b6c80ab0b7..cf0eb826f1 100644
> > --- a/hw/block/block.c
> > +++ b/hw/block/block.c
> > @@ -15,19 +15,6 @@
> > #include "qapi/qapi-types-block.h"
> > #include "qemu/error-report.h"
> >
> > -void blkconf_serial(BlockConf *conf, char **serial)
> > -{
> > - DriveInfo *dinfo;
> > -
> > - if (!*serial) {
> > - /* try to fall back to value set with legacy -drive serial=... */
> > - dinfo = blk_legacy_dinfo(conf->blk);
> > - if (dinfo) {
> > - *serial = g_strdup(dinfo->serial);
> > - }
> > - }
> > -}
> > -
> > void blkconf_blocksizes(BlockConf *conf)
> > {
> > BlockBackend *blk = conf->blk;
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 811084b6a7..d5bf95b79b 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -1215,7 +1215,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error
> > **errp)
> > return;
> > }
> >
> > - blkconf_serial(&n->conf, &n->serial);
> > if (!n->serial) {
> > error_setg(errp, "serial property not set");
> > return;
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index 50b5c869e3..225fe44b7a 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -935,7 +935,6 @@ static void virtio_blk_device_realize(DeviceState *dev,
> > Error **errp)
> > return;
> > }
> >
> > - blkconf_serial(&conf->conf, &conf->serial);
> > if (!blkconf_apply_backend_options(&conf->conf,
> > blk_is_read_only(conf->conf.blk),
> > true,
> > errp)) {
> > diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> > index f395d24592..573b022e1e 100644
> > --- a/hw/ide/qdev.c
> > +++ b/hw/ide/qdev.c
> > @@ -188,7 +188,6 @@ static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind
> > kind, Error **errp)
> > return;
> > }
> >
> > - blkconf_serial(&dev->conf, &dev->serial);
> > if (kind != IDE_CD) {
> > if (!blkconf_geometry(&dev->conf, &dev->chs_trans, 65535, 16, 255,
> > errp)) {
> > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> > index 9f43583ea0..920850a1c1 100644
> > --- a/hw/scsi/scsi-disk.c
> > +++ b/hw/scsi/scsi-disk.c
> > @@ -2372,7 +2372,6 @@ static void scsi_realize(SCSIDevice *dev, Error
> > **errp)
> > return;
> > }
> >
> > - blkconf_serial(&s->qdev.conf, &s->serial);
> > blkconf_blocksizes(&s->qdev.conf);
> >
> > if (s->qdev.conf.logical_block_size >
> > diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
> > index 481694a473..47b992f403 100644
> > --- a/hw/usb/dev-storage.c
> > +++ b/hw/usb/dev-storage.c
> > @@ -606,7 +606,6 @@ static void usb_msd_storage_realize(USBDevice *dev,
> > Error **errp)
> > return;
> > }
> >
> > - blkconf_serial(&s->conf, &dev->serial);
> > blkconf_blocksizes(&s->conf);
> > if (!blkconf_apply_backend_options(&s->conf, blk_is_read_only(blk),
> > true,
> > errp)) {
> > diff --git a/tests/ahci-test.c b/tests/ahci-test.c
> > index 1a7b761304..937ed2f910 100644
> > --- a/tests/ahci-test.c
> > +++ b/tests/ahci-test.c
> > @@ -180,12 +180,12 @@ static AHCIQState *ahci_boot(const char *cli, ...)
> > s = ahci_vboot(cli, ap);
> > va_end(ap);
> > } else {
> > - cli = "-drive if=none,id=drive0,file=%s,cache=writeback,serial=%s"
> > - ",format=%s"
> > + cli = "-drive if=none,id=drive0,file=%s,cache=writeback,format=%s"
> > " -M q35 "
> > "-device ide-hd,drive=drive0 "
> > + "-global ide-hd.serial=%s "
> > "-global ide-hd.ver=%s";
> > - s = ahci_boot(cli, tmp_path, "testdisk", imgfmt, "version");
> > + s = ahci_boot(cli, tmp_path, imgfmt, "testdisk", "version");
>
> Not this patch's problem: ahci_boot() and ahci_boot_and_enable() lack
> GCC_FMT_ATTR().
>
> This patch's problem, sort of: once they have it, we'll have to
> eliminate @cli here.
>
> I figure adding GCC_FMT_ATTR() will require us to clean up the
> ahci_boot(NULL) silliness:
>
> static AHCIQState *ahci_boot(const char *cli, ...)
> {
> AHCIQState *s;
> va_list ap;
>
> va_start(ap, cli);
> s = ahci_vboot(cli, ap);
> va_end(ap);
>
> return s;
> }
>
> static AHCIQState *ahci_boot_defaults(void)
> {
> return ahci_boot("-drive if=none,id=drive0,file=%s,"
> "cache=writeback,serial=%s,format=%s"
> " -M q35 "
> "-device ide-hd,drive=drive0 "
> "-global ide-hd.ver=%s",
> tmp_path, "testdisk", imgfmt, "version");
> }
>
> Anyway, you decide whether you want to do anything here now.
>
> > }
> >
> > return s;
> > diff --git a/tests/ide-test.c b/tests/ide-test.c
> > index 2384c2c3e2..f39431b1a9 100644
> > --- a/tests/ide-test.c
> > +++ b/tests/ide-test.c
> > @@ -529,8 +529,8 @@ static void test_bmdma_no_busmaster(void)
> > static void test_bmdma_setup(void)
> > {
> > ide_test_start(
> > - "-drive file=%s,if=ide,serial=%s,cache=writeback,format=raw "
> > - "-global ide-hd.ver=%s",
> > + "-drive file=%s,if=ide,cache=writeback,format=raw "
> > + "-global ide-hd.serial=%s -global ide-hd.ver=%s",
> > tmp_path, "testdisk", "version");
> > qtest_irq_intercept_in(global_qtest, "ioapic");
> > }
> > @@ -561,8 +561,8 @@ static void test_identify(void)
> > int ret;
> >
> > ide_test_start(
> > - "-drive file=%s,if=ide,serial=%s,cache=writeback,format=raw "
> > - "-global ide-hd.ver=%s",
> > + "-drive file=%s,if=ide,cache=writeback,format=raw "
> > + "-global ide-hd.serial=%s -global ide-hd.ver=%s",
> > tmp_path, "testdisk", "version");
> >
> > dev = get_pci_device(&bmdma_bar, &ide_bar);
> > diff --git a/qemu-doc.texi b/qemu-doc.texi
> > index 338477725f..282bc3dc35 100644
> > --- a/qemu-doc.texi
> > +++ b/qemu-doc.texi
> > @@ -2850,11 +2850,6 @@ with ``-device ...,netdev=x''), or ``-nic
> > user,smb=/some/dir''
> > (for embedded NICs). The new syntax allows different settings to be
> > provided per NIC.
> >
> > -@subsection -drive serial=... (since 2.10.0)
> > -
> > -The drive serial argument is replaced by the the serial argument
> > -that can be specified with the ``-device'' parameter.
> > -
> > @subsection -usbdevice (since 2.10.0)
> >
> > The ``-usbdevice DEV'' argument is now a synonym for setting
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index c2531e2f3c..d5b0c26e8e 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -805,7 +805,7 @@ ETEXI
> > DEF("drive", HAS_ARG, QEMU_OPTION_drive,
> > "-drive [file=file][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n"
> > "
> > [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
> > - " [,snapshot=on|off][,serial=s][,rerror=ignore|stop|report]\n"
> > + " [,snapshot=on|off][,rerror=ignore|stop|report]\n"
> > "
> > [,werror=ignore|stop|report|enospc][,id=name][,aio=threads|native]\n"
> > " [,readonly=on|off][,copy-on-read=on|off]\n"
> > " [,discard=ignore|unmap][,detect-zeroes=on|off|unmap]\n"
> > @@ -879,10 +879,6 @@ The default mode is @option{cache=writeback}.
> > Specify which disk @var{format} will be used rather than detecting
> > the format. Can be used to specify format=raw to avoid interpreting
> > an untrusted format header.
> > -@item serial=@var{serial}
> > -This option specifies the serial number to assign to the device. 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 <arm...@redhat.com>
>
