Re: pygrub: avoid problems if guest files are large etc.

[Matt Wilson <msw@amazon.com>]

On Mon, Jun 25, 2012 at 02:34:02PM -0700, M A Young wrote:
> On Mon, 25 Jun 2012, Ian Campbell wrote:
> 
> > BTW, what is the status of that patch?
> 
> I am attaching version 4 of my patch. It moves handling of the kernel and 
> ramdisk into a function along the lines of Miroslav Rezanina's patch
> http://lists.xen.org/archives/html/xen-devel/2012-05/msg01499.html
> It adds checking for problems opening the kernel or ramdisk files and 
> checks they exist in the not_really mode as discussed in this thread.
> 
> I still think it is a good idea to remove the temporary copies of the 
> kernel and ramdisk if there are problems copying them because I am not 
> convinced the calling process always removes them and it might be 
> presented with truncated files if copying the files filled the filespace.
> 
>  	Michael Young
>
> Make pygrub cope better with big files in the guest.
> Only read the first megabyte of a configuration file (grub etc.) and read
> the kernel and ramdisk files from the guest in one megabyte pieces
> so pygrub doesn't use a lot of memory if the files are large.
> With --not-really option check that the chosen kernel and ramdisk files exist.
> If there are problems writing the copy of the kernel or ramdisk, delete the
> copied files and exit in case they have filled the filesystem.
> 
> Signed-off-by: Michael Young <m.a.young@durham.ac.uk>
> 
> --- xen-4.2.0/tools/pygrub/src/pygrub.orig	2012-05-12 16:40:48.000000000 +0100
> +++ xen-4.2.0/tools/pygrub/src/pygrub	2012-06-25 21:53:49.556446369 +0100
> @@ -28,6 +28,7 @@
>  import grub.ExtLinuxConf
>  
>  PYGRUB_VER = 0.6
> +fs_read_max=1048576

All other constants in the global scope are in all caps. "1024 * 1024"
is a bit more readable.

>  def enable_cursor(ison):
>      if ison:
> @@ -448,7 +449,8 @@
>          if self.__dict__.get('cf', None) is None:
>              raise RuntimeError, "couldn't find bootloader config file in the image provided."
>          f = fs.open_file(self.cf.filename)
> -        buf = f.read()
> +        # limit read size to avoid pathological cases
> +        buf = f.read(fs_read_max)
>          del f
>          self.cf.parse(buf)
>  
> @@ -697,6 +699,39 @@
>      def usage():
>          print >> sys.stderr, "Usage: %s [-q|--quiet] [-i|--interactive] [-n|--not-really] [--output=] [--kernel=] [--ramdisk=] [--args=] [--entry=] [--output-directory=] [--output-format=sxp|simple|simple0] <image>" %(sys.argv[0],)
>  
> +    def copy_from_image(fs, file_to_read, file_type, output_directory,
> +                                              not_really, clean_extra_file):

Could the indention here follow PEP-8 [1] guidelines?

> +        if not_really:
> +            if fs.file_exists(file_to_read):
> +                return "<%s:%s>" % (file_type,file_to_read)

missing space after ,

> +            else:
> +                sys.exit("The requested %s file does not exist" % file_type)
> +        try:
> +            datafile = fs.open_file(file_to_read)
> +        except:

I personally don't like the pattern of:
    try:
        ...
    except:
        ...
There's a lot of opportunity to hide exceptions other than those that
you'd expect. At minimum, I'd capture the exception and try to add it
to the error message.

> +            if clean_extra_file:
> +                os.unlink(clean_extra_file)

It would seem that you're pushing a cleanup task to the innermost
function. Shouldn't it be the responsibility of the caller to clean up
on an exception?

> +            sys.exit("Error opening %s" % file_to_read)
> +        (tfd, ret) = tempfile.mkstemp(prefix="boot_"+file_type+".",
> +                                                       dir=output_directory)

Fix indention to be PEP-8.

> +        dataoff=0
> +        while True:
> +            data=datafile.read(fs_read_max,dataoff)

Missing space after ,
Missing spaces before and after =

> +            if len(data) == 0:
> +                os.close(tfd)
> +                del datafile
> +                return ret
> +            try:
> +                os.write(tfd, data)
> +            except:

try: ... except: again can make us blind to unexpected errors. At
minimum capture the error and include it in the exit string.

> +                os.close(tfd)
> +                os.unlink(ret)
> +                del datafile
> +                if clean_extra_file:
> +                    os.unlink(clean_extra_file)
> +                sys.exit("error writing temporary copy of "+file_type)
> +            dataoff+=len(data)

Spaces before and after +=

> +
>      try:
>          opts, args = getopt.gnu_getopt(sys.argv[1:], 'qinh::',
>                                     ["quiet", "interactive", "not-really", "help", 
> @@ -821,24 +856,12 @@
>      if not fs:
>          raise RuntimeError, "Unable to find partition containing kernel"
>  
> -    if not_really:
> -        bootcfg["kernel"] = "<kernel:%s>" % chosencfg["kernel"]
> -    else:
> -        data = fs.open_file(chosencfg["kernel"]).read()
> -        (tfd, bootcfg["kernel"]) = tempfile.mkstemp(prefix="boot_kernel.",
> -                                                    dir=output_directory)
> -        os.write(tfd, data)
> -        os.close(tfd)
> +    bootcfg["kernel"] = copy_from_image(fs, chosencfg["kernel"], 
> +                              "kernel", output_directory, not_really, "")

PEP-8 indention

>  
>      if chosencfg["ramdisk"]:
> -        if not_really:
> -            bootcfg["ramdisk"] = "<ramdisk:%s>" % chosencfg["ramdisk"]
> -        else:
> -            data = fs.open_file(chosencfg["ramdisk"],).read()
> -            (tfd, bootcfg["ramdisk"]) = tempfile.mkstemp(
> -                prefix="boot_ramdisk.", dir=output_directory)
> -            os.write(tfd, data)
> -            os.close(tfd)
> +        bootcfg["ramdisk"] = copy_from_image(fs, chosencfg["ramdisk"],
> +              "ramdisk", output_directory, not_really, bootcfg["kernel"])

PEP-8 indention. This seems to be the place that should be cleaning up
images on error. E.g.:

    try:
        bootcfg["ramdisk"] = copy_from_image(fs, chosencfg["ramdisk"],
                                             "ramdisk", output_directory,
                                             not_really)
    except Exception, e:
        exc_info = sys.exc_info()
        try:
	    os.unlink(bootcfg["kernel"])
        except Exception, e:
	    # log an error
        # re-raise original exception
        raise exc_info[0], exc_info[1], exc_into[2]


Re-raising the exception is the fancy thing to do. If we don't care
if os.unlink() raises an exception, you could leave off the inner
try: and just use "raise" to re-raise.

>      else:
>          initrd = None
>  

Matt

[1] http://www.python.org/dev/peps/pep-0008/#indentation