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

[Stefan Berger <stefanb@xxxxxxxxxx>]

xen-devel-bounces@xxxxxxxxxxxxxxxxxxx wrote on 11/12/2007
06:41:38 AM:

> 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.

The Grub class uses the title_idx as the index of
the title entry, not the line number. You may have 3 'title' entries in
your grub config file, then you have possible indices 0,1,2. The LatePolicyLoader
class does not work on grub's configuration file, but a simple text file
that contains only one entry indicating which policy to load once xend
has started.

> 
> What your change seems to be addressing is a confusion about whether
> it's a filename (with `.bin') or a stripped version.  The relevant

That's correct. The '.bin' appended to a policy's
name results in the filename.

> 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' ])

Yes, you are correct, this would have probably been
easier.

> 
> > @@ -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 [ ... ]' ?

line[-1] = '\n' would have done the same.

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

The problem with the parsers is that they all work
slightly different and have different code branches inside of them. I would
not want to add additional parameters to a unified parser function that
requires construction of  if - then - else clauses so that it behaves
differently depending on its intended purpose.

Thanks.

   Stefan

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