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

[John Snow]

On 10/20/2017 05:02 AM, Daniel P. Berrange wrote:
> 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
> 

I'm sorry I didn't notice. I had an unexpected LOA, has this been addressed?

CC me and I will take care of it.

--John