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

[Markus Armbruster]

Fam Zheng <address@hidden> writes:

> 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?" 
> -

At the place where we can reliably detect orphaned drives regardless of
what the machine initialization code does, we have no idea.

> that "3 > 2" is the good part of the old message.

At the places that actually adopt drives, we know.  But there are many
of them.  Just three check for orphans.  One of them gets it right
(ide/core.c), one of them gets it wrong (mips_jazz.c), and one of them
sets a problematic example (sun4m.c): if copied to a machine that lets
users configure additional SCSI HBAs, it would break if=scsi for those.
Dampens my enthusiasm for improving the error message by adding similar
checks to all the places that adopt drives.

We could make machines declare what they support.  Better, I think.  So
if you have a burning desire to leave your mark in git-blame for every
machine...

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

Try it out:

    for i in ide scsi floppy pflash mtd sd virtio xen
    do
        for ((b=0; b<4; b++))
        do
            for ((u=0; u<10; u++))
            do
                echo q | qemu-system-x86_64 -nodefaults -S -display none 
-monitor stdio -drive if=$i,bus=$b,unit=$u,file=tmp.qcow2 >/dev/null
            done
        done
    done

My upper bounds for bus and unit are arbitrary.