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

[Fam Zheng]

On Mon, 01/30 09:10, 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...

I guess the painpoint is "okay, what the heck does the machine support then?" -
that "3 > 2" is the good part of the old message.

Do we have a user documentation for this? Or, can we give a hint how to figure
that out?

Fam