Re: [Qemu-devel] [PATCH v4 0/6] Clean up bogus default boot order

[Markus Armbruster]

"Michael S. Tsirkin" <address@hidden> writes:

> On Fri, Aug 16, 2013 at 01:13:44PM +0200, address@hidden wrote:
>> From: Markus Armbruster <address@hidden>
>> 
>> The first five patches are admittedly related to the stated purpose of
>> this series pretty much only by "I can't stand perpetuating this
>> stupid crap".  Max Filippov and Peter Maydell already cleaned up
>> Xtensa and ARM the same way.
>
> I picked up patches 3,4 and 5 on my tree.
> 1 and 2 were rebased by Eduardo, I'm taking them
> from his patchset.
> 6 needs to be rebased and comments addressed.

Applies fine with "git-am -3".  Pushed to
http://repo.or.cz/w/qemu/armbru.git/shortlog/refs/heads/boot-order
for your convenience.

We discussed the patch at some length, but it's not 100% clear to me
what exactly you'd like me to address and how.  So let's recap briefly.

I think your main point was that PC machine type declarations are a bit
repetitive.  They all share two lines:

    .max_cpus = 255,
    DEFAULT_MACHINE_OPTIONS,

where DEFAULT_MACHINE_OPTIONS is defined as

    #define DEFAULT_MACHINE_OPTIONS \
        .boot_order = "cad"

Many of them also share one of these lines:

    .desc = "Standard PC (Q35 + ICH9, 2009)",
    .desc = "Standard PC (i440FX + PIIX, 1996)",
    .desc = "Standard PC",

My patch touches only the shared DEFAULT_MACHINE_OPTIONS line.  It
becomes

   .boot_order = "cad"

Commit message explains why:

    We set default boot order "cad" in every single machine definition
    except "pseries" and "moxiesim", even though very few boards actually
    care for boot order, and "cad" makes sense for even fewer.

    Machines that care:
    
    * pc and its variants
    
      Accept up to three letters 'a', 'b' (undocumented alias for 'a'),
      'c', 'd' and 'n'.  Reject all others (fatal with -boot).

    [...]

    Strip characters these machines ignore from their default boot order.
    
    For all other machines, remove the unused default boot order
    alltogether.

The change is systematic: if the machine uses .boot_order, strip the
characters it ignores from its initial value, else drop the initializer,
so .boot_order remains null.

I don't want to squash further cleanup into this one, because it's hard
enough to review as it is (and it already got competent review).  I
could be persuaded to do further cleanup on top, but you need to tell me
what cleanup you want.  Probably faster if you do it yourself :)