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

This patch fixes the case where a module line is supposed to be added to
the very end of the file but the file does not end in with a new line.
Also fixes a problem that in some cases the module line would not be
properly be removed.

Signed-off-by: Stefan Berger <stefanb@us.ibm.com>



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

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@lists.xensource.com
http://lists.xensource.com/xen-devel

xen-devel-bounces@lists.xensource.com 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@lists.xensource.com
> http://lists.xensource.com/xen-devel

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