Re: [Qemu-devel] [PATCH v2 8/8] hw: Drop superfluous special checks for orphaned -drive

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> On 01/27/2017 11:04 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> On 01/27/2017 06:51 AM, Markus Armbruster wrote:
>>>> John Snow <jsnow@redhat.com> writes:
>>>>
>>>>> On 01/26/2017 10:09 AM, Markus Armbruster wrote:
>>>>>> We've traditionally rejected orphans here and there, but not
>>>>>> systematically.  For instance, the sun4m machines have an onboard SCSI
>>>>>> HBA (bus=0), and have always rejected bus>0.  Other machines with an
>>>>>> onboard SCSI HBA don't.
>>>>>>
>>>>>> Commit a66c9dc made all orphans trigger a warning, and the previous
>>>>>> commit turned this into an error.  The checks "here and there" are now
>>>>>> redundant.  Drop them.
>>>>>>
>>>>>> Note that the one in mips_jazz.c was wrong: it rejected bus > MAX_FD,
>>>>>> but MAX_FD is the number of floppy drives per bus.
>>>>>>
>>>>>> Error messages change from
>>>>>>
>>>>>>     $ qemu-system-x86_64 -drive if=ide,bus=2
>>>>>>     qemu-system-x86_64: Too many IDE buses defined (3 > 2)
>>>>>>     $ qemu-system-mips64 -M magnum,accel=qtest -drive if=floppy,bus=2,id=fd1
>>>>>>     qemu: too many floppy drives
>>>>>>     $ qemu-system-sparc -M LX -drive if=scsi,bus=1
>>>>>>     qemu: too many SCSI bus
>>>>>>
>>>>>> to
>>>>>>
>>>>>>     $ qemu-system-x86_64 -drive if=ide,bus=2
>>>>>>     qemu-system-x86_64: -drive if=ide,bus=2: machine type does not support this drive
>>>>>>     $ qemu-system-mips64 -M magnum,accel=qtest -drive if=floppy,bus=2,id=fd1
>>>>>>     qemu-system-mips64: -drive if=floppy,bus=2,id=fd1: machine type does not support this drive
>>>>>>     $ qemu-system-sparc -M LX -drive if=scsi,bus=1
>>>>>>     qemu-system-sparc: -drive if=scsi,bus=1: machine type does not support this drive
>>>>>>
>>>>>
>>>>> Hm, that's a lot less helpful, isn't it? Can we augment with hints?
>>>>
>>>> The message itself may be less specific, but it now comes with a precise
>>>> location.  Personally, I'd even find
>>>>
>>>>     qemu-system-sparc: -drive if=scsi,bus=1: *mumble* *mumble*
>>>>
>>>> more helpful than
>>>>
>>>>     qemu: too many SCSI bus
>>>>
>>>> because the former tells me *which* of the options is bad.  We tend to
>>>> have lots and lots of them.
>>>>
>>>> The deleted special case errors cover only a minority of "orphan"
>>>> -drive.  If these cases need improvement, then so will the general case.
>>>> If you can come up with a hint that makes the general case message more
>>>> useful, I'm more than happy to squash it into PATCH 6.
>>>>
>>>
>>> The old error had "why" and the new error has "where" but neither has
>>> both. I would suggest that from the "why" you can divine the "where,"
>>> but the opposite is not as easily true.
>> 
>> Some users will be able to divine more easily than others.  Consider my
>> "too many floppy drives" example.  There's just one, and the machine
>> actually supports two.  The user has to make the connection to "bus=2"
>> somehow.  Now, anybody crazy enough to mess with bus= can probably be
>> expected to figure this out, but still, the deleted error messages
>> aren't exactly wonderful.
>> 
>>> The new error even suggests information I think is wrong and misleading:
>>> We do support SCSI! (Just not this many of them.)
>> 
>> Well, the error doesn't say "machine doesn't support SCSI", only
>> "doesn't support this particular -drive".  Perhaps it could be worded
>> more clearly.  Ideas?
>> 
>
> Ah, I see what you mean now. I interpreted "this drive" to mean SCSI,
> not this SCSI *instance*.

Uh, I would've written "does not support if=scsi" then.  But I see where
you come from.

>                           If it can be made clearer that QEMU is simply
> unable to instantiate this particular instance, that'd be fine.
>
> Instead of "Machine type does not support this drive,"
>
> how about
>
> "Machine type cannot instantiate this drive instance"

Hmm.

> Or ... follow your own best judgement. This is really YOUR wheelhouse.
> My example is a little wordy.

All I can come up with is even wordier: "Machine type doesn't support
this combination of if, bus, unit", or "if=scsi,bus=%d,unit=%d not
supported with this machine type".  Could also be confusing when the
user specified index instead of bus, unit.

Good error messages are hard...