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

[John Snow]

On 01/30/2017 03:10 AM, Markus Armbruster wrote:
> John Snow <address@hidden> writes:
> 
>> On 01/27/2017 11:04 AM, Markus Armbruster wrote:
>>> John Snow <address@hidden> writes:
>>>
>>>> On 01/27/2017 06:51 AM, Markus Armbruster wrote:
>>>>> John Snow <address@hidden> 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...
> 

Yes, so just follow your own instincts. Just offering a non-blocking
comment.

--js