[libvirt] PATCH: Support setting a disk drive serial (number)

[Daniel P. Berrange]

All hard disks (IDE, SCSI, USB, VirtIO) have a concept of a serial number
which is intended to uniquely identify them. If you mean this off, QEMU
just makes up a serial on the fly. THis is not guarenteed to be stable
across guest reboots with hardware changes. It is thus desirable to be
able to specify this in libvirt XML. To that end, this patch allows
for a <serial> element inside a disk

<disk type='file' device='disk'>
<source file='/media/lacie-500-disk-2/virtual-machines/scratch5.img'/>
<target dev='vda' bus='virtio'/>
<serial>dan123virtio</serial>
</disk>

the contents is free-form. It implemenmts this for the QEMU driver for
any disk using -drive style args. Unfortunately this excludes USB disk,
even though internally QEMU can set a serial for these, the -usbdevice
syntax does not allow it

Daniel

* docs/schemas/domain.rng: Add <serial> element to disks
* src/domain_conf.h, src/domain_conf.c: XML parsing and
formatting for disk serial numbers
* src/qemu_conf.c: Set serial number when launching guests
* tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.args,
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.xml: Add
serial number to XML test
---
docs/schemas/domain.rng | 10 ++++++++++
src/domain_conf.c | 11 +++++++++++
src/domain_conf.h | 1 +
src/qemu_conf.c | 2 ++
.../qemuxml2argv-disk-drive-shared.args | 2 +-
.../qemuxml2argv-disk-drive-shared.xml | 1 +
6 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng
index f857301..16f35e4 100644
--- a/docs/schemas/domain.rng
+++ b/docs/schemas/domain.rng
@@ -336,6 +336,11 @@
<empty/>
</element>
</optional>
+ <optional>
+ <element name="serial">
+ <ref name="diskSerial"/>
+ </element>
+ </optional>
</define>
<!--
A disk description can be either of type file or block
@@ -1135,6 +1140,11 @@
<param name="pattern">[A-Za-z0-9_\.\+\-&amp;:/]+</param>
</data>
</define>
+ <define name="diskSerial">
+ <data type="string">
+ <param name="pattern">[A-Za-z0-9_\.\+\-]+</param>
+ </data>
+ </define>
<define name="genericName">
<data type="string">
<param name="pattern">[a-zA-Z0-9_\+\-]+</param>
diff --git a/src/domain_conf.c b/src/domain_conf.c
index 1d2cc7c..654f753 100644
--- a/src/domain_conf.c
+++ b/src/domain_conf.c
@@ -284,6 +284,7 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def)
if (!def)
return;

+ VIR_FREE(def->serial);
VIR_FREE(def->src);
VIR_FREE(def->dst);
VIR_FREE(def->driverName);
@@ -661,6 +662,7 @@ virDomainDiskDefParseXML(virConnectPtr conn,
char *bus = NULL;
char *cachetag = NULL;
char *devaddr = NULL;
+ char *serial = NULL;

if (VIR_ALLOC(def) < 0) {
virReportOOMError(conn);
@@ -718,6 +720,9 @@ virDomainDiskDefParseXML(virConnectPtr conn,
} else if ((flags & VIR_DOMAIN_XML_INTERNAL_STATUS) &&
xmlStrEqual(cur->name, BAD_CAST "state")) {
devaddr = virXMLPropString(cur, "devaddr");
+ } else if ((serial == NULL) &&
+ (xmlStrEqual(cur->name, BAD_CAST "serial"))) {
+ serial = (char *)xmlNodeGetContent(cur);
}
}
cur = cur->next;
@@ -836,6 +841,8 @@ virDomainDiskDefParseXML(virConnectPtr conn,
driverName = NULL;
def->driverType = driverType;
driverType = NULL;
+ def->serial = serial;
+ serial = NULL;

cleanup:
VIR_FREE(bus);
@@ -847,6 +854,7 @@ cleanup:
VIR_FREE(driverName);
VIR_FREE(cachetag);
VIR_FREE(devaddr);
+ VIR_FREE(serial);

return def;

@@ -3519,6 +3527,9 @@ virDomainDiskDefFormat(virConnectPtr conn,
virBufferAddLit(buf, " <readonly/>\n");
if (def->shared)
virBufferAddLit(buf, " <shareable/>\n");
+ if (def->serial)
+ virBufferEscapeString(buf, " <serial>%s</serial>\n",
+ def->serial);

if (flags & VIR_DOMAIN_XML_INTERNAL_STATUS) {
virBufferAddLit(buf, " <state");
diff --git a/src/domain_conf.h b/src/domain_conf.h
index 44302be..7bad5e7 100644
--- a/src/domain_conf.h
+++ b/src/domain_conf.h
@@ -109,6 +109,7 @@ struct _virDomainDiskDef {
char *dst;
char *driverName;
char *driverType;
+ char *serial;
int cachemode;
unsigned int readonly : 1;
unsigned int shared : 1;
diff --git a/src/qemu_conf.c b/src/qemu_conf.c
index 082f107..95d41e6 100644
--- a/src/qemu_conf.c
+++ b/src/qemu_conf.c
@@ -1748,6 +1748,8 @@ int qemudBuildCommandLine(virConnectPtr conn,
if (disk->driverType &&
qemuCmdFlags & QEMUD_CMD_FLAG_DRIVE_FORMAT)
virBufferVSprintf(&opt, ",format=%s", disk->driverType);
+ if (disk->serial)
+ virBufferVSprintf(&opt, ",serial=%s", disk->serial);

if (disk->cachemode) {
const char *mode =
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.args
index 611cd33..07cbe0e 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.args
@@ -1 +1 @@
-LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -drive file=/dev/HostVG/QEMUGuest1,if=ide,index=0,format=qcow2,cache=off -drive file=/dev/HostVG/QEMUGuest2,if=ide,media=cdrom,index=2,format=raw -net none -serial none -parallel none -usb
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -drive file=/dev/HostVG/QEMUGuest1,if=ide,index=0,format=qcow2,serial=XYZXYZXYZYXXYZYZYXYZY,cache=off -drive file=/dev/HostVG/QEMUGuest2,if=ide,media=cdrom,index=2,format=raw -net none -serial none -parallel none -usb
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.xml
index b386315..c4e4734 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.xml
@@ -19,6 +19,7 @@
<source dev='/dev/HostVG/QEMUGuest1'/>
<target dev='hda' bus='ide'/>
<shareable/>
+ <serial>XYZXYZXYZYXXYZYZYXYZY</serial>
</disk>
<disk type='block' device='cdrom'>
<driver name='qemu' type='raw'/>
--
1.6.2.5

[Dave Allan]

Post by Daniel P. Berrange
All hard disks (IDE, SCSI, USB, VirtIO) have a concept of a serial number
which is intended to uniquely identify them. If you mean this off, QEMU
just makes up a serial on the fly. THis is not guarenteed to be stable
across guest reboots with hardware changes. It is thus desirable to be
able to specify this in libvirt XML. To that end, this patch allows
for a <serial> element inside a disk
<disk type='file' device='disk'>
<source file='/media/lacie-500-disk-2/virtual-machines/scratch5.img'/>
<target dev='vda' bus='virtio'/>
<serial>dan123virtio</serial>
</disk>
the contents is free-form. It implemenmts this for the QEMU driver for
any disk using -drive style args. Unfortunately this excludes USB disk,
even though internally QEMU can set a serial for these, the -usbdevice
syntax does not allow it
Daniel
* docs/schemas/domain.rng: Add <serial> element to disks
* src/domain_conf.h, src/domain_conf.c: XML parsing and
formatting for disk serial numbers
* src/qemu_conf.c: Set serial number when launching guests
* tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.args,
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.xml: Add
serial number to XML test
---
docs/schemas/domain.rng | 10 ++++++++++
src/domain_conf.c | 11 +++++++++++
src/domain_conf.h | 1 +
src/qemu_conf.c | 2 ++
.../qemuxml2argv-disk-drive-shared.args | 2 +-
.../qemuxml2argv-disk-drive-shared.xml | 1 +
6 files changed, 26 insertions(+), 1 deletions(-)
diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng
index f857301..16f35e4 100644
--- a/docs/schemas/domain.rng
+++ b/docs/schemas/domain.rng
@@ -336,6 +336,11 @@
<empty/>
</element>
</optional>
+ <optional>
+ <element name="serial">
+ <ref name="diskSerial"/>
+ </element>
+ </optional>
</define>
<!--
A disk description can be either of type file or block
@@ -1135,6 +1140,11 @@
<param name="pattern">[A-Za-z0-9_\.\+\-&amp;:/]+</param>
</data>
</define>
+ <define name="diskSerial">
+ <data type="string">
+ <param name="pattern">[A-Za-z0-9_\.\+\-]+</param>
+ </data>
+ </define>
<define name="genericName">
<data type="string">
<param name="pattern">[a-zA-Z0-9_\+\-]+</param>
diff --git a/src/domain_conf.c b/src/domain_conf.c
index 1d2cc7c..654f753 100644
--- a/src/domain_conf.c
+++ b/src/domain_conf.c
@@ -284,6 +284,7 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def)
if (!def)
return;
+ VIR_FREE(def->serial);
VIR_FREE(def->src);
VIR_FREE(def->dst);
VIR_FREE(def->driverName);
@@ -661,6 +662,7 @@ virDomainDiskDefParseXML(virConnectPtr conn,
char *bus = NULL;
char *cachetag = NULL;
char *devaddr = NULL;
+ char *serial = NULL;
if (VIR_ALLOC(def) < 0) {
virReportOOMError(conn);
@@ -718,6 +720,9 @@ virDomainDiskDefParseXML(virConnectPtr conn,
} else if ((flags & VIR_DOMAIN_XML_INTERNAL_STATUS) &&
xmlStrEqual(cur->name, BAD_CAST "state")) {
devaddr = virXMLPropString(cur, "devaddr");
+ } else if ((serial == NULL) &&
+ (xmlStrEqual(cur->name, BAD_CAST "serial"))) {
+ serial = (char *)xmlNodeGetContent(cur);
}
}
cur = cur->next;
@@ -836,6 +841,8 @@ virDomainDiskDefParseXML(virConnectPtr conn,
driverName = NULL;
def->driverType = driverType;
driverType = NULL;
+ def->serial = serial;
+ serial = NULL;
VIR_FREE(bus);
VIR_FREE(driverName);
VIR_FREE(cachetag);
VIR_FREE(devaddr);
+ VIR_FREE(serial);
return def;
@@ -3519,6 +3527,9 @@ virDomainDiskDefFormat(virConnectPtr conn,
virBufferAddLit(buf, " <readonly/>\n");
if (def->shared)
virBufferAddLit(buf, " <shareable/>\n");
+ if (def->serial)
+ virBufferEscapeString(buf, " <serial>%s</serial>\n",
+ def->serial);
if (flags & VIR_DOMAIN_XML_INTERNAL_STATUS) {
virBufferAddLit(buf, " <state");
diff --git a/src/domain_conf.h b/src/domain_conf.h
index 44302be..7bad5e7 100644
--- a/src/domain_conf.h
+++ b/src/domain_conf.h
@@ -109,6 +109,7 @@ struct _virDomainDiskDef {
char *dst;
char *driverName;
char *driverType;
+ char *serial;
int cachemode;
unsigned int readonly : 1;
unsigned int shared : 1;
diff --git a/src/qemu_conf.c b/src/qemu_conf.c
index 082f107..95d41e6 100644
--- a/src/qemu_conf.c
+++ b/src/qemu_conf.c
@@ -1748,6 +1748,8 @@ int qemudBuildCommandLine(virConnectPtr conn,
if (disk->driverType &&
qemuCmdFlags & QEMUD_CMD_FLAG_DRIVE_FORMAT)
virBufferVSprintf(&opt, ",format=%s", disk->driverType);
+ if (disk->serial)
+ virBufferVSprintf(&opt, ",serial=%s", disk->serial);
if (disk->cachemode) {
const char *mode =
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.args
index 611cd33..07cbe0e 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.args
@@ -1 +1 @@
-LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -drive file=/dev/HostVG/QEMUGuest1,if=ide,index=0,format=qcow2,cache=off -drive file=/dev/HostVG/QEMUGuest2,if=ide,media=cdrom,index=2,format=raw -net none -serial none -parallel none -usb
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -drive file=/dev/HostVG/QEMUGuest1,if=ide,index=0,format=qcow2,serial=XYZXYZXYZYXXYZYZYXYZY,cache=off -drive file=/dev/HostVG/QEMUGuest2,if=ide,media=cdrom,index=2,format=raw -net none -serial none -parallel none -usb
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.xml
index b386315..c4e4734 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.xml
@@ -19,6 +19,7 @@
<source dev='/dev/HostVG/QEMUGuest1'/>
<target dev='hda' bus='ide'/>
<shareable/>
+ <serial>XYZXYZXYZYXXYZYZYXYZY</serial>
</disk>
<disk type='block' device='cdrom'>
<driver name='qemu' type='raw'/>

This patch seems like a very good idea to me. I'm surprised it doesn't
cause more grief if the serial # keeps changing. I didn't review the
code too closely, but a definite ACK to the functionality, and I saw no
problems with the implementation in the quick look I took.

As a possible additional feature, in the case of an entire device being
assigned to the guest, have you considered passing through the serial of
the underlying device? The serial is available in the nodedev XML.

Dave

[Daniel P. Berrange]

Post by Dave Allan

Post by Daniel P. Berrange
All hard disks (IDE, SCSI, USB, VirtIO) have a concept of a serial number
which is intended to uniquely identify them. If you mean this off, QEMU
just makes up a serial on the fly. THis is not guarenteed to be stable
across guest reboots with hardware changes. It is thus desirable to be
able to specify this in libvirt XML. To that end, this patch allows
for a <serial> element inside a disk
<disk type='file' device='disk'>
<source
file='/media/lacie-500-disk-2/virtual-machines/scratch5.img'/>
<target dev='vda' bus='virtio'/>
<serial>dan123virtio</serial>
</disk>
the contents is free-form. It implemenmts this for the QEMU driver for
any disk using -drive style args. Unfortunately this excludes USB disk,
even though internally QEMU can set a serial for these, the -usbdevice
syntax does not allow it
Daniel
* docs/schemas/domain.rng: Add <serial> element to disks
* src/domain_conf.h, src/domain_conf.c: XML parsing and
formatting for disk serial numbers
* src/qemu_conf.c: Set serial number when launching guests
* tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.args,
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.xml: Add
serial number to XML test

[snip]

Post by Dave Allan
This patch seems like a very good idea to me. I'm surprised it doesn't
cause more grief if the serial # keeps changing. I didn't review the
code too closely, but a definite ACK to the functionality, and I saw no
problems with the implementation in the quick look I took.

Well it causes grief for Windows guests with WHQL'd virtio drivers I
believe. We've probably just been fortunate with Linux guests that not
much functional stuff is based off the serial number. The serial number
is used for /dev/disk/by-id symlinks, but most distros use /dev/disk/by-uuid
symlinks for mounting volume instead and that's based off a filesystem
level UUID, instead of block device level serial.

Post by Dave Allan
As a possible additional feature, in the case of an entire device being
assigned to the guest, have you considered passing through the serial of
the underlying device? The serial is available in the nodedev XML.

That's a policy decision, and so not within scope of libvirt. Of course
the application provisioning a new guest can see the host device's
serial, and copy that across to the guest XML it uses if desired.

Daniel

[Dave Allan]

Post by Daniel P. Berrange

Post by Dave Allan

Post by Daniel P. Berrange
All hard disks (IDE, SCSI, USB, VirtIO) have a concept of a serial number
which is intended to uniquely identify them. If you mean this off, QEMU
just makes up a serial on the fly. THis is not guarenteed to be stable
across guest reboots with hardware changes. It is thus desirable to be
able to specify this in libvirt XML. To that end, this patch allows
for a <serial> element inside a disk
<disk type='file' device='disk'>
<source
file='/media/lacie-500-disk-2/virtual-machines/scratch5.img'/>
<target dev='vda' bus='virtio'/>
<serial>dan123virtio</serial>
</disk>
the contents is free-form. It implemenmts this for the QEMU driver for
any disk using -drive style args. Unfortunately this excludes USB disk,
even though internally QEMU can set a serial for these, the -usbdevice
syntax does not allow it
Daniel
* docs/schemas/domain.rng: Add <serial> element to disks
* src/domain_conf.h, src/domain_conf.c: XML parsing and
formatting for disk serial numbers
* src/qemu_conf.c: Set serial number when launching guests
* tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.args,
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.xml: Add
serial number to XML test

[snip]

Post by Dave Allan
This patch seems like a very good idea to me. I'm surprised it doesn't
cause more grief if the serial # keeps changing. I didn't review the
code too closely, but a definite ACK to the functionality, and I saw no
problems with the implementation in the quick look I took.

Well it causes grief for Windows guests with WHQL'd virtio drivers I
believe. We've probably just been fortunate with Linux guests that not
much functional stuff is based off the serial number. The serial number
is used for /dev/disk/by-id symlinks, but most distros use /dev/disk/by-uuid
symlinks for mounting volume instead and that's based off a filesystem
level UUID, instead of block device level serial.

Yes, I was briefly puzzled as to how any modern Linux could work, as I
mistook uuid for id in my fstab. :)

Post by Daniel P. Berrange

Post by Dave Allan
As a possible additional feature, in the case of an entire device being
assigned to the guest, have you considered passing through the serial of
the underlying device? The serial is available in the nodedev XML.

That's a policy decision, and so not within scope of libvirt. Of course
the application provisioning a new guest can see the host device's
serial, and copy that across to the guest XML it uses if desired.

Ok, that's fair.

Dave

[Mark McLoughlin]

Post by Daniel P. Berrange
+ if (disk->serial)
+ virBufferVSprintf(&opt, ",serial=%s", disk->serial);

qemu-0.9.1 didn't support serial=, qemu-0.10.0 and kvm-83 were the first
versions to add it

Looks fine apart from that

Cheers,
Mark.

[Daniel Veillard]

On Mon, Aug 17, 2009 at 06:12:55PM +0100, Daniel P. Berrange wrote:
[...]

Post by Daniel P. Berrange
+ if (disk->serial)
+ virBufferVSprintf(&opt, ",serial=%s", disk->serial);

Sounds fine, just that if the content is free form maybe one need
to quote that argument

",serial='%s'"

to avoid potential problems. Also wondering what happens if qemu/kvm
doesn't support the option, maybe this should be part of the detected
features as Mark raised,

Good idea, ACK,

Daniel

[Daniel P. Berrange]

Post by Daniel Veillard
[...]

Post by Daniel P. Berrange
+ if (disk->serial)
+ virBufferVSprintf(&opt, ",serial=%s", disk->serial);

Sounds fine, just that if the content is free form maybe one need
to quote that argument
",serial='%s'"
to avoid potential problems.

That would be a nice idea if QEMU actually supported quoting :-)

It treats the value as any character following the '=' up until the
next ',' (or end of string).

Daniel

[Daniel Veillard]

Post by Daniel P. Berrange

Post by Daniel Veillard
[...]

Post by Daniel P. Berrange
+ if (disk->serial)
+ virBufferVSprintf(&opt, ",serial=%s", disk->serial);

Sounds fine, just that if the content is free form maybe one need
to quote that argument
",serial='%s'"
to avoid potential problems.

That would be a nice idea if QEMU actually supported quoting :-)
It treats the value as any character following the '=' up until the
next ',' (or end of string).

OMG !

Any further comment removed before sending ...

Daniel