Re: [Xen-devel] [PATCH] [Xend] Fix appending policy module to end of grub's config file

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Stefan Berger writes ("[Xen-devel] [PATCH] [Xend] Fix appending policy module 
to end of grub's config file"):
> Index: root/xen-unstable.hg/tools/python/xen/util/bootloader.py
> ===================================================================
> --- root.orig/xen-unstable.hg/tools/python/xen/util/bootloader.py
> +++ root/xen-unstable.hg/tools/python/xen/util/bootloader.py
> @@ -64,6 +64,8 @@ def get_kernel_val(index, att):
>  
>  def set_boot_policy(title_idx, filename):
>      boottitles = get_boot_policies()
> +    for key in boottitles.iterkeys():
> +        boottitles[key] += ".bin"
>      if boottitles.has_key(title_idx):
>          rm_policy_from_boottitle(title_idx, [ boottitles[title_idx] ])
>      rc = add_boot_policy(title_idx, filename)

Having investigated this situation I'm not really convinced that this
patch is correct.  One problem that the various Bootloader classes in
bootloader.py seem to disagree about what `title_idx' is in this
context ?  The Grub class uses line numbers but LatePolicyLoader seems
always to use DEFAULT_TITLE (ie, `any') which seems odd.  Perhaps I
have misread the code.

What your change seems to be addressing is a confusion about whether
it's a filename (with `.bin') or a stripped version.  The relevant
stripped version seems to be the one generated in
LatePolicyLoader.add_boot_policy, which just strips `.bin', but
compare this with Grub.get_boot_policies which strips `.bin' but also
various stuff from the front of what I think is the same kind of path.

I'm afraid I wasn't able to conclude which of the disagreeing parts
the code were right or wrong, or I would have suggested an alternative
way to fix the problem.

Even if the right answer is just to deal with the discrepancy over
`.bin' in this way in set_boot_policy, editing the whole array seems a
strange way to go about it.  Why not just amend the result of the
actual lookup ?  eg
  rm_policy_from_boottitle(title_idx, [ boottitles[title_idx] + '.bin' ])

> @@ -335,6 +337,8 @@ class Grub(Bootloader):
>                  os.write(tmp_fd, line)
>  
>              if module_line != "" and not found:
> +                if ord(line[-1]) not in [ 10 ]:
> +                    os.write(tmp_fd, '\n')
>                  os.write(tmp_fd, module_line)
>                  found = True

Why the circumlocutions with `ord' and `in [ ... ]' ?

As an aside, bootloader.py currently contains multiple bootfile
parsers and editors, containing a lot of very similar code.  I make it
4 parsers and 3 parser-mutators.  Surely these should be combined as
far as possible ?

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel