qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v1 2/2] ide: support reporting of rotation rate


From: Daniel P. Berrange
Subject: Re: [Qemu-devel] [PATCH v1 2/2] ide: support reporting of rotation rate
Date: Fri, 20 Oct 2017 10:02:19 +0100
User-agent: Mutt/1.9.1 (2017-09-22)

On Fri, Oct 20, 2017 at 10:42:21AM +0200, Kevin Wolf wrote:
> [ Cc: qemu-block ]
> 
> Am 04.10.2017 um 13:40 hat Daniel P. Berrange geschrieben:
> > The Linux kernel will query the ATA IDENTITY DEVICE data, word 217
> > to determine the rotations per minute of the disk. If this has
> > the value 1, it is taken to be an SSD and so Linux sets the
> > 'rotational' flag to 0 for the I/O queue and will stop using that
> > disk as a source of random entropy. Other operating systems may
> > also take into account rotation rate when setting up default
> > behaviour.
> > 
> > Mgmt apps should be able to set the rotation rate for virtualized
> > block devices, based on characteristics of the host storage in use,
> > so that the guest OS gets sensible behaviour out of the box. This
> > patch thus adds a 'rotation-rate' parameter for 'ide-hd' device
> > types.
> > 
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> >  hw/ide/core.c             | 1 +
> >  hw/ide/qdev.c             | 1 +
> >  include/hw/ide/internal.h | 8 ++++++++
> >  3 files changed, 10 insertions(+)
> > 
> > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > index 5f1cd3b91f..a04766aee7 100644
> > --- a/hw/ide/core.c
> > +++ b/hw/ide/core.c
> > @@ -208,6 +208,7 @@ static void ide_identify(IDEState *s)
> >      if (dev && dev->conf.discard_granularity) {
> >          put_le16(p + 169, 1); /* TRIM support */
> >      }
> > +    put_le16(p + 217, dev->rotation_rate); /* Nominal media rotation rate 
> > */
> 
> Coverity points out that all other dereferences of dev have a NULL check
> first. Are we sure that it is always non-NULL?
> 
> A follow-up patch is necessary either way. Either fix the missing NULL
> check here or remove useless NULL checks in the other places.

'dev' comes from:

    IDEDevice *dev = s->unit ? s->bus->slave : s->bus->master;

IIUC, this is choosing either the first or the second unit on the IDE
bus. Presumably this can be lead to dev==NULL, when the guest OS calls
identify on a unit that doesn't have a drive attached. Soo the NULL
checks looks like its required to me.

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 :|



reply via email to

[Prev in Thread] Current Thread [Next in Thread]