Re: [Xen-devel] [OSSTEST Nested PATCH v11 2/7] Parsing grub which has 'submenu' primitive

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

longtao.pang writes ("[OSSTEST Nested PATCH v11 2/7] Parsing grub which has 
'submenu' primitive"):
> Now auto-gen kernel grub2 config file's boot menu entries can have
> 2-level hierarchy, containing 'submenu' primitive, which is comprised by
> several sub-menuentries. Xen boot entries are grouped into such kind of
> 'submenu' block. This patch adds setupboot_grub2() ability to handle
> such new grub.cfg format

Parsing submenus is good, but I have some style quibbles with your
patch I'm afraid.

>          while (<$f>) {
>              next if m/^\s*\#/ || !m/\S/;
>              if (m/^\s*\}\s*$/) {
> -                die unless $entry;
> +                die unless $entry || $submenu;
> +                if(!defined $entry && defined $submenu){

$entry and $submenu are either some ref, or undef, so you don't need
`defined'.  You can just use them as if they were booleans in the if,
just as you did above.  (I'm not sure that `!defined $entry' adds much
given the previous line, but that's maybe a matter or taste.)

Missing spaces after if and inside `){'.

> +                    logm("Met end of a submenu starting from ".
> +                        "$submenu->{StartLine}. ".
> +                        "Our want kern is $want_kernver");
> +                    $submenu=undef;

Missing space after or around `='.  (Other places too.)

> +                    pop @offsets;
> +                    $offsets[$#offsets]++;
> +                    next;
> +                }
>                  my (@missing) =
>                      grep { !defined $entry->{$_} } 
>                       (defined $xenhopt
> @@ -438,8 +448,12 @@ sub setupboot_grub2 ($$$$) {
>              }
>              if (m/^menuentry\s+[\'\"](.*)[\'\"].*\{\s*$/) {
>                  die $entry->{StartLine} if $entry;
> -                $entry= { Title => $1, StartLine => $., Number => $count };
> -                $count++;
> +                $entry= { Title => $1, StartLine => $., MenuEntryPath => 
> join ">", @offsets };

Needs rewrapping.  Probably best to expand the { } into a multi-line
structure.

> +                $offsets[$#offsets]++;
> +            }
> +            if (m/^submenu\s+[\'\"](.*)[\'\"].*\{\s*$/) {
> +                $submenu={ StartLine =>$., MenuEntryPath => join ">", 
> @offsets };

Unless I'm mistaken, the MenuEntryPath of a $submenu is never used ?
Not setting it would avoid (a) a need to rewrap and (b) me complaining
that you have open-coded the join twice.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel