Re: [Qemu-devel] [Bug 1529449] [NEW] serial is required for -device nvme

[Markus Armbruster <armbru@redhat.com>]

Keith Busch <keith.busch@intel.com> writes:

> On Mon, Jan 11, 2016 at 05:35:50PM +0100, Markus Armbruster wrote:
>> Tom Yan <tom.ty89@gmail.com> writes:
>> > Public bug reported:
>> >
>> > I am not exactly sure if this is a bug, but I don't see why the option
>> > "serial" should be required for -device nvme like the option "drive".
>> > Truth is it seem to accept random string as its value anyway, if that's
>> > the case, couldn't qemu just generate one for it when it's not
>> > specified?
>>
>> You should've included a reproducer.  Here are mine:
>> 
>> 1. Bad error reporting on missing drive:
>> 
>>      $ upstream-qemu -nodefaults -device nvme
>>      upstream-qemu: -device nvme: Device initialization failed
>> 
>>    Expected: error reported like for other devices, e.g.
>> 
>>      $ upstream-qemu -nodefaults -device virtio-blk
>>      upstream-qemu: -device virtio-blk: drive property not set
>> 
>> 2. Bad error reporting on empty drive:
>> 
>>      $ upstream-qemu -nodefaults -drive if=none,id=foo -device nvme,drive=foo
>>      upstream-qemu: -device nvme,drive=foo: Device initialization failed
>> 
>>    Expected: error is reported like for other devices, e.g.
>> 
>>      $ upstream-qemu -nodefaults -drive if=none,id=foo -device virtio-blk,drive=foo
>>      upstream-qemu: -device virtio-blk,drive=foo: Device needs media, but drive is empty
>> 
>> 3. Bad handling of missing serial:
>> 
>>       $ upstream-qemu -nodefaults -drive if=none,id=foo,file=tmp.qcow2 -device nvme,drive=foo
>>       upstream-qemu: -device nvme,drive=foo: Device initialization failed
>> 
>>    Expected: either default the serial number, like some other devices
>>    do, or a decent error message.
>> 
>> I recommend to convert the device to realize(), and add the missing
>> error_setg().  Keith?
>
> Requiring a serial was a concious choice to push that responsibility
> on the user, but I don't see a problem having the code provide default
> serial string if the user does not over ride it.
>
> If you've multiple nvme devices in your guest, creating the same serial
> could cause problems with multipathing if they're basing end device
> uniqueness on the serial (some do). If we have the code provide the
> serial, perhaps it would be best to make each unique. That's easy enough
> to append an incrementing number to the end of the serial.

I don't have a strong opinion on whether serial can remain mandatory or
should become optional.  If we make up serial numbers, they better be
unique, of course.

I do have a strong opinion on something else: the error reporting.
Please convert the device to realize(), and add the necessary
error_setg().