virtio-blk: assign a default serial number if none provided

Submitted by Ryan Harper on June 2, 2010, 1:48 a.m.

Details

Message ID 20100602014854.GB16406@us.ibm.com
State New
Headers show

Commit Message

Ryan Harper June 2, 2010, 1:48 a.m.
This patch applies on-top of John's virtio-blk serial patches.

Generate default serial numbers for virtio drives based on DriveInfo.unit which is
incremented for each additional virtio-blk device.  This provides a
per-virtio-blk number to use in the default string: QM%05d that is used in
hw/ide/core.c.  The resulting serial number looks like: QM00001, etc.

Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
---
 hw/virtio-blk.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

Comments

Michael Tokarev June 2, 2010, 6:56 a.m.
02.06.2010 05:48, Ryan Harper wrote:
[]
>   hw/virtio-blk.c |    3 +++
> +    if (strlen(s->sn) == 0) {

Just out of curiocity (not that it is wrong or inefficient):
why
   strlen(s->sn)
and not, say,
   !s->sn[0]
?

/mjt
Michael S. Tsirkin June 2, 2010, 9 a.m.
On Tue, Jun 01, 2010 at 08:48:54PM -0500, Ryan Harper wrote:
> This patch applies on-top of John's virtio-blk serial patches.
> 
> Generate default serial numbers for virtio drives based on DriveInfo.unit which is
> incremented for each additional virtio-blk device.  This provides a
> per-virtio-blk number to use in the default string: QM%05d that is used in
> hw/ide/core.c.  The resulting serial number looks like: QM00001, etc.
> 
> Signed-off-by: Ryan Harper <ryanh@us.ibm.com>


I think that there's a problem with this approach in that hot plug A,
hot plug B, hot unplug A is not the same as hot plug B.
So you might get guest boot failures and no easy way to
figure out why. For guests that need S/N, I think they
really must be persistent.

> ---
>  hw/virtio-blk.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index 98c62f2..e5c6e7c 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -518,6 +518,9 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf)
>      bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
>  
>      strncpy(s->sn, drive_get_serial(s->bs), sizeof (s->sn));
> +    if (strlen(s->sn) == 0) {
> +        snprintf(s->sn, sizeof(s->sn), "QM%05d", conf->dinfo->unit);
> +    }
>  
>      s->vq = virtio_add_queue(&s->vdev, 128, virtio_blk_handle_output);
>  
> -- 
> 1.6.3.3
> 
> 
> -- 
> Ryan Harper
> Software Engineer; Linux Technology Center
> IBM Corp., Austin, Tx
> ryanh@us.ibm.com
Ryan Harper June 2, 2010, 11:42 a.m.
* Michael Tokarev <mjt@tls.msk.ru> [2010-06-02 01:57]:
> 02.06.2010 05:48, Ryan Harper wrote:
> []
> >  hw/virtio-blk.c |    3 +++
> >+    if (strlen(s->sn) == 0) {
> 
> Just out of curiocity (not that it is wrong or inefficient):
> why
>   strlen(s->sn)
> and not, say,
>   !s->sn[0]
> ?

Just matching how it's done in hw/ide/core.c:ide_init_drive()
Ryan Harper June 2, 2010, 11:45 a.m.
* Michael S. Tsirkin <mst@redhat.com> [2010-06-02 04:08]:
> On Tue, Jun 01, 2010 at 08:48:54PM -0500, Ryan Harper wrote:
> > This patch applies on-top of John's virtio-blk serial patches.
> > 
> > Generate default serial numbers for virtio drives based on DriveInfo.unit which is
> > incremented for each additional virtio-blk device.  This provides a
> > per-virtio-blk number to use in the default string: QM%05d that is used in
> > hw/ide/core.c.  The resulting serial number looks like: QM00001, etc.
> > 
> > Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
> 
> 
> I think that there's a problem with this approach in that hot plug A,
> hot plug B, hot unplug A is not the same as hot plug B.
> So you might get guest boot failures and no easy way to
> figure out why. For guests that need S/N, I think they
> really must be persistent.

That's true; though I think most boot drives boot via either LVM or UUID
which will remain persistent.  That said, if you are relying on the
by-id; then of course the user will need to specify serial versus having
one auto-generated.
Michael S. Tsirkin June 2, 2010, 11:54 a.m.
On Wed, Jun 02, 2010 at 06:45:46AM -0500, Ryan Harper wrote:
> * Michael S. Tsirkin <mst@redhat.com> [2010-06-02 04:08]:
> > On Tue, Jun 01, 2010 at 08:48:54PM -0500, Ryan Harper wrote:
> > > This patch applies on-top of John's virtio-blk serial patches.
> > > 
> > > Generate default serial numbers for virtio drives based on DriveInfo.unit which is
> > > incremented for each additional virtio-blk device.  This provides a
> > > per-virtio-blk number to use in the default string: QM%05d that is used in
> > > hw/ide/core.c.  The resulting serial number looks like: QM00001, etc.
> > > 
> > > Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
> > 
> > 
> > I think that there's a problem with this approach in that hot plug A,
> > hot plug B, hot unplug A is not the same as hot plug B.
> > So you might get guest boot failures and no easy way to
> > figure out why. For guests that need S/N, I think they
> > really must be persistent.
> 
> That's true; though I think most boot drives boot via either LVM or UUID
> which will remain persistent.  That said, if you are relying on the
> by-id; then of course the user will need to specify serial versus having
> one auto-generated.

I guess the question then would be, if you don't rely on the S/N, why
do you want to set it?

> -- 
> Ryan Harper
> Software Engineer; Linux Technology Center
> IBM Corp., Austin, Tx
> ryanh@us.ibm.com
Ryan Harper June 2, 2010, 12:26 p.m.
* Michael S. Tsirkin <mst@redhat.com> [2010-06-02 06:59]:
> On Wed, Jun 02, 2010 at 06:45:46AM -0500, Ryan Harper wrote:
> > * Michael S. Tsirkin <mst@redhat.com> [2010-06-02 04:08]:
> > > On Tue, Jun 01, 2010 at 08:48:54PM -0500, Ryan Harper wrote:
> > > > This patch applies on-top of John's virtio-blk serial patches.
> > > > 
> > > > Generate default serial numbers for virtio drives based on DriveInfo.unit which is
> > > > incremented for each additional virtio-blk device.  This provides a
> > > > per-virtio-blk number to use in the default string: QM%05d that is used in
> > > > hw/ide/core.c.  The resulting serial number looks like: QM00001, etc.
> > > > 
> > > > Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
> > > 
> > > 
> > > I think that there's a problem with this approach in that hot plug A,
> > > hot plug B, hot unplug A is not the same as hot plug B.
> > > So you might get guest boot failures and no easy way to
> > > figure out why. For guests that need S/N, I think they
> > > really must be persistent.
> > 
> > That's true; though I think most boot drives boot via either LVM or UUID
> > which will remain persistent.  That said, if you are relying on the
> > by-id; then of course the user will need to specify serial versus having
> > one auto-generated.
> 
> I guess the question then would be, if you don't rely on the S/N, why
> do you want to set it?

Functional similarity; we have default serial numbers for ide and scsi.
scsi is hotpluggable and would suffer the same issue; and of course ide
doesn't do hotplug.  I don't think most folks will encounter the above
scenario and that having the same default serial numbers being generated
like we do ide and scsi is reasonable.
Markus Armbruster June 2, 2010, 12:32 p.m.
Ryan Harper <ryanh@us.ibm.com> writes:

> * Michael Tokarev <mjt@tls.msk.ru> [2010-06-02 01:57]:
>> 02.06.2010 05:48, Ryan Harper wrote:
>> []
>> >  hw/virtio-blk.c |    3 +++
>> >+    if (strlen(s->sn) == 0) {
>> 
>> Just out of curiocity (not that it is wrong or inefficient):
>> why
>>   strlen(s->sn)
>> and not, say,
>>   !s->sn[0]
>> ?
>
> Just matching how it's done in hw/ide/core.c:ide_init_drive()

I cleaned it up there the other day.

[PATCH 11/14] ide: Turn drive serial into a qdev property ide-drive.serial
http://thread.gmane.org/gmane.comp.emulators.qemu/72441/focus=72432
http://repo.or.cz/w/qemu/kevin.git/commit/ebf5f0ff455a794faca404e62e5dd6d5bdf2fb9b

Patch hide | download patch | download mbox

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 98c62f2..e5c6e7c 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -518,6 +518,9 @@  VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf)
     bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
 
     strncpy(s->sn, drive_get_serial(s->bs), sizeof (s->sn));
+    if (strlen(s->sn) == 0) {
+        snprintf(s->sn, sizeof(s->sn), "QM%05d", conf->dinfo->unit);
+    }
 
     s->vq = virtio_add_queue(&s->vdev, 128, virtio_blk_handle_output);