scsi-disk includes in the Device Identification VPD page, depending on configuration amongst others, a vendor specific designator that consists either of the serial number if given or the BlockBackend name (which is a host detail that better shouldn't have been leaked to the guest, but now we have to maintain it for compatibility). With anonymous BlockBackends, i.e. scsi-disk devices constructed with drive=<node-name>, and no serial number explicitly specified, this ends up as an empty string. If this happens to more than one disk, we have accidentally signalled to the OS that this is a multipath setup, which is obviously not what was intended. Instead of using an empty string for the vendor specific designator, simply leave out that designator, which makes Linux detect such setups as separate disks again. Signed-off-by: Kevin Wolf <kwolf(a)redhat.com&gt; --- hw/scsi/scsi-disk.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index 0e9027c8f3..93eef40b87 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -652,12 +652,14 @@ static int scsi_disk_emulate_vpd_page(SCSIRequest *req, uint8_t *outbuf) DPRINTF("Inquiry EVPD[Device identification] " "buffer size %zd\n", req->cmd.xfer); - outbuf[buflen++] = 0x2; /* ASCII */ - outbuf[buflen++] = 0; /* not officially assigned */ - outbuf[buflen++] = 0; /* reserved */ - outbuf[buflen++] = id_len; /* length of data following */ - memcpy(outbuf + buflen, str, id_len); - buflen += id_len; + if (id_len) { + outbuf[buflen++] = 0x2; /* ASCII */ + outbuf[buflen++] = 0; /* not officially assigned */ + outbuf[buflen++] = 0; /* reserved */ + outbuf[buflen++] = id_len; /* length of data following */ + memcpy(outbuf + buflen, str, id_len); + buflen += id_len; + } if (s->qdev.wwn) { outbuf[buflen++] = 0x1; /* Binary */ -- 2.20.1

Kevin Wolf <kwolf(a)redhat.com&gt; writes: Before the patch, we always add this descriptor, but as you explain in your commit message, its contents can be wrong. After the patch, we add this descriptor only when we have a suitable name (we use serial number, else falling back to BlockBackend name). It's possible we add *no* descriptors. I wonder whether that's okay. I consulted section SPC-4 section 7.8.5 Device Identification VPD page, but failed to penetrate the dense prose there.

The new device_id property specifies which value to use for the vendor specific designator in the Device Identification VPD page. In particular, this is necessary for libvirt to maintain guest ABI compatibility when no serial number is given and a VM is switched from -drive (where the BlockBackend name is used) to -blockdev (where the vendor specific designator is left out by default). Signed-off-by: Kevin Wolf <kwolf(a)redhat.com&gt; --- hw/scsi/scsi-disk.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index 93eef40b87..e74e1e7c48 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -104,6 +104,7 @@ typedef struct SCSIDiskState char *serial; char *vendor; char *product; + char *device_id; bool tray_open; bool tray_locked; /* @@ -642,13 +643,8 @@ static int scsi_disk_emulate_vpd_page(SCSIRequest *req, uint8_t *outbuf) case 0x83: /* Device identification page, mandatory */ { - const char *str = s->serial ?: blk_name(s->qdev.conf.blk); - int max_len = s->serial ? 20 : 255 - 8; - int id_len = strlen(str); + int id_len = s->device_id ? MIN(strlen(s->device_id), 255 - 8) : 0; - if (id_len > max_len) { - id_len = max_len; - } DPRINTF("Inquiry EVPD[Device identification] " "buffer size %zd\n", req->cmd.xfer); @@ -657,7 +653,7 @@ static int scsi_disk_emulate_vpd_page(SCSIRequest *req, uint8_t *outbuf) outbuf[buflen++] = 0; /* not officially assigned */ outbuf[buflen++] = 0; /* reserved */ outbuf[buflen++] = id_len; /* length of data following */ - memcpy(outbuf + buflen, str, id_len); + memcpy(outbuf + buflen, s->device_id, id_len); buflen += id_len; } @@ -2363,6 +2359,16 @@ static void scsi_realize(SCSIDevice *dev, Error **errp) if (!s->vendor) { s->vendor = g_strdup("QEMU"); } + if (!s->device_id) { + if (s->serial) { + s->device_id = g_strdup_printf("%.20s", s->serial); + } else { + const char *str = blk_name(s->qdev.conf.blk); + if (str && *str) { + s->device_id = g_strdup(str); + } + } + } if (blk_is_sg(s->qdev.conf.blk)) { error_setg(errp, "unwanted /dev/sg*"); @@ -2904,7 +2910,9 @@ static const TypeInfo scsi_disk_base_info = { DEFINE_PROP_STRING("ver", SCSIDiskState, version), \ DEFINE_PROP_STRING("serial", SCSIDiskState, serial), \ DEFINE_PROP_STRING("vendor", SCSIDiskState, vendor), \ - DEFINE_PROP_STRING("product", SCSIDiskState, product) + DEFINE_PROP_STRING("product", SCSIDiskState, product), \ + DEFINE_PROP_STRING("device_id", SCSIDiskState, device_id) + static Property scsi_hd_properties[] = { DEFINE_SCSI_DISK_PROPERTIES(), -- 2.20.1

On Mon, Jan 28, 2019 at 09:50:41 +0100, Peter Krempa wrote: I've found: commit b443ae67130d32ad06b06fc9aa6d04d05ccd93ce Author: Markus Armbruster <armbru(a)redhat.com&gt; Date: Mon May 16 15:04:53 2011 +0200 scsi: Split qdev "scsi-disk" into "scsi-hd" and "scsi-cd" Which seems to put scsi-hd and scsi-cd as the thing to use. Libvirt's code obviously did not adapt after that and we still keep to probe scsi-disk instead of the split up devices, whereas we will never use scsi-disk. Also some of our tests seem to look as if scsi-disk was used which confused me. Since the new property is present also for scsi-hd and scsi-cd we really need to adapt our code to that. Btw, this also means that qemu can deprecate scsi-disk. Sorry for the noise.

On Fri, Jan 25, 2019 at 06:46:53PM +0100, Kevin Wolf wrote: I don't think we've deprecated things in the past which would cause a guest ABI change when finally deleted. Shouldn't we be instead setting using machine type versioning here, so that existing machine types keep using the blk_name() fallback forever, while QEMU 4.0+ machine types avoid blk_name() fallback immediately. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|