[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Bug 1529449] [NEW] serial is required for -device nvme
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [Bug 1529449] [NEW] serial is required for -device nvme |
Date: |
Tue, 12 Jan 2016 11:12:09 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Keith Busch <address@hidden> writes:
> On Mon, Jan 11, 2016 at 05:35:50PM +0100, Markus Armbruster wrote:
>> Tom Yan <address@hidden> 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().