pygrub: verify chosen kernel really exists

pygrub: verify chosen kernel really exists

[Andrew Cooper]

[-- Attachment #1: Type: text/plain, Size: 244 bytes --]

This patch has been sitting in the XenServer patch queue for an
embarrassingly long time.  I have formatted it suitably for upstreaming.

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com


[-- Attachment #2: pygrub-bug-test-kernel-exists.patch --]
[-- Type: text/x-patch, Size: 969 bytes --]

pygrub: verify chosen kernel really exists

Verify that the chosen kernel really exists, and fail with an informative error
if it does not.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

diff -r 32034d1914a6 tools/pygrub/src/pygrub
--- a/tools/pygrub/src/pygrub
+++ b/tools/pygrub/src/pygrub
@@ -821,10 +821,15 @@ if __name__ == "__main__":
     if not fs:
         raise RuntimeError, "Unable to find partition containing kernel"
 
+    # Does the chosen kernel really exist ?
+    try:
+        data = fs.open_file(chosencfg["kernel"]).read()
+    except:
+        raise RuntimeError, "The chosen kernel does not exist"
+
     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)

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: pygrub: verify chosen kernel really exists

[Ian Campbell]

On Fri, 2012-06-22 at 19:37 +0100, Andrew Cooper wrote:
> 
> diff -r 32034d1914a6 tools/pygrub/src/pygrub
> --- a/tools/pygrub/src/pygrub
> +++ b/tools/pygrub/src/pygrub
> @@ -821,10 +821,15 @@ if __name__ == "__main__":
>      if not fs:
>          raise RuntimeError, "Unable to find partition containing
> kernel"
>  
> +    # Does the chosen kernel really exist ?
> +    try:
> +        data = fs.open_file(chosencfg["kernel"]).read()

If you make this use .exists() and left the open/read where it was that
would be more in keeping with the intended meaning of the not_really
flag.


> +    except:
> +        raise RuntimeError, "The chosen kernel does not exist"
> +
>      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) 

Re: pygrub: verify chosen kernel really exists

[M A Young]

On Mon, 25 Jun 2012, Ian Campbell wrote:

> On Fri, 2012-06-22 at 19:37 +0100, Andrew Cooper wrote:
>>
>> diff -r 32034d1914a6 tools/pygrub/src/pygrub
>> --- a/tools/pygrub/src/pygrub
>> +++ b/tools/pygrub/src/pygrub
>> @@ -821,10 +821,15 @@ if __name__ == "__main__":
>>      if not fs:
>>          raise RuntimeError, "Unable to find partition containing
>> kernel"
>>
>> +    # Does the chosen kernel really exist ?
>> +    try:
>> +        data = fs.open_file(chosencfg["kernel"]).read()
>
> If you make this use .exists() and left the open/read where it was that
> would be more in keeping with the intended meaning of the not_really
> flag.

Also if you are checking for the kernel, shouldn't you also check for the 
ramdisk if one is requested for consistency.

It also hits the same area of code as the proposed protection against 
problems caused by huge kernels in the guests (eg. see
http://lists.xen.org/archives/html/xen-devel/2012-05/msg01183.html
or http://lists.xen.org/archives/html/xen-devel/2012-05/msg01499.html
and it was suggested that it might be better to use a function to do this 
bit of the code rather than duplicating it for both kernel and ramdisk.

 	Michael Young

Re: pygrub: verify chosen kernel really exists

[Ian Campbell]

On Mon, 2012-06-25 at 11:17 +0100, M A Young wrote:
> It also hits the same area of code as the proposed protection against 
> problems caused by huge kernels in the guests (eg. see
> http://lists.xen.org/archives/html/xen-devel/2012-05/msg01183.html
> or http://lists.xen.org/archives/html/xen-devel/2012-05/msg01499.html
> and it was suggested that it might be better to use a function to do this 
> bit of the code rather than duplicating it for both kernel and ramdisk.

BTW, what is the status of that patch?

Re: pygrub: verify chosen kernel really exists

[M A Young]

On Mon, 25 Jun 2012, Ian Campbell wrote:

> On Mon, 2012-06-25 at 11:17 +0100, M A Young wrote:
>> It also hits the same area of code as the proposed protection against
>> problems caused by huge kernels in the guests (eg. see
>> http://lists.xen.org/archives/html/xen-devel/2012-05/msg01183.html
>> or http://lists.xen.org/archives/html/xen-devel/2012-05/msg01499.html
>> and it was suggested that it might be better to use a function to do this
>> bit of the code rather than duplicating it for both kernel and ramdisk.
>
> BTW, what is the status of that patch?

I left it drift a bit, but I was starting to look again at it when I saw 
this patch, particularly as it suggested to me a different way to cause 
problems from the guest - have a huge kernel file and a missing ramdisk so 
you might get pygrub to backtrace and is a good chance the temporary 
kernel file won't get deleted.

 	Michael Young

pygrub: avoid problems if guest files are large etc.

[M A Young]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 737 bytes --]

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

[-- Attachment #2: Type: TEXT/PLAIN, Size: 4212 bytes --]

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
 
 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):
+        if not_really:
+            if fs.file_exists(file_to_read):
+                return "<%s:%s>" % (file_type,file_to_read)
+            else:
+                sys.exit("The requested %s file does not exist" % file_type)
+        try:
+            datafile = fs.open_file(file_to_read)
+        except:
+            if clean_extra_file:
+                os.unlink(clean_extra_file)
+            sys.exit("Error opening %s" % file_to_read)
+        (tfd, ret) = tempfile.mkstemp(prefix="boot_"+file_type+".",
+                                                       dir=output_directory)
+        dataoff=0
+        while True:
+            data=datafile.read(fs_read_max,dataoff)
+            if len(data) == 0:
+                os.close(tfd)
+                del datafile
+                return ret
+            try:
+                os.write(tfd, data)
+            except:
+                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)
+
     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, "")
 
     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"])
     else:
         initrd = None
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

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

[Matt Wilson]

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

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

[M A Young]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1061 bytes --]

On Tue, 26 Jun 2012, Matt Wilson wrote:

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

Here is version 5 of my patch, modified following Matt Wilson's 
suggestions, standardizing the indentations, moving some of the clean up 
out of the loop, and reporting the actual error.

 	Michael Young

[-- Attachment #2: Type: TEXT/PLAIN, Size: 4319 bytes --]

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-07-01 21:15:07.820574802 +0100
@@ -28,6 +28,7 @@
 import grub.ExtLinuxConf
 
 PYGRUB_VER = 0.6
+FS_READ_MAX = 1024 * 1024
 
 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,37 @@
     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):
+        if not_really:
+            if fs.file_exists(file_to_read):
+                return "<%s:%s>" % (file_type, file_to_read)
+            else:
+                sys.exit("The requested %s file does not exist" % file_type)
+        try:
+            datafile = fs.open_file(file_to_read)
+        except Exception, e:
+            print >>sys.stderr, e
+            sys.exit("Error opening %s in guest" % file_to_read)
+        (tfd, ret) = tempfile.mkstemp(prefix="boot_"+file_type+".",
+                                      dir=output_directory)
+        dataoff = 0
+        while True:
+            data = datafile.read(FS_READ_MAX, dataoff)
+            if len(data) == 0:
+                os.close(tfd)
+                del datafile
+                return ret
+            try:
+                os.write(tfd, data)
+            except Exception, e:
+                print >>sys.stderr, e
+                os.close(tfd)
+                os.unlink(ret)
+                del datafile
+                sys.exit("Error writing temporary copy of "+file_type)
+            dataoff += len(data)
+
     try:
         opts, args = getopt.gnu_getopt(sys.argv[1:], 'qinh::',
                                    ["quiet", "interactive", "not-really", "help", 
@@ -821,24 +854,18 @@
     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)
 
     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)
+        try:
+            bootcfg["ramdisk"] = copy_from_image(fs, chosencfg["ramdisk"],
+                                                 "ramdisk", output_directory,
+                                                 not_really)
+        except:
+            if not not_really:
+                os.unlink(bootcfg["kernel"])
+            raise
     else:
         initrd = None
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

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

[Ian Campbell]

On Mon, 2012-07-02 at 00:47 +0100, M A Young wrote:
> On Tue, 26 Jun 2012, Matt Wilson wrote:
> 
> > 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.
> 
> Here is version 5 of my patch, modified following Matt Wilson's 
> suggestions, standardizing the indentations, moving some of the clean up 
> out of the loop, and reporting the actual error.

Looks good to me. 

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> 
>  	Michael Young

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

[Matt Wilson]

On Mon, Jul 02, 2012 at 04:22:34AM -0700, Ian Campbell wrote:
> > 
> > Here is version 5 of my patch, modified following Matt Wilson's 
> > suggestions, standardizing the indentations, moving some of the clean up 
> > out of the loop, and reporting the actual error.
> 
> Looks good to me. 

Me too.

Matt

> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> > 
> >  	Michael Young
> 
> 

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

[Ian Campbell]

On Mon, 2012-07-02 at 20:42 +0100, Matt Wilson wrote:
> On Mon, Jul 02, 2012 at 04:22:34AM -0700, Ian Campbell wrote:
> > > 
> > > Here is version 5 of my patch, modified following Matt Wilson's 
> > > suggestions, standardizing the indentations, moving some of the clean up 
> > > out of the loop, and reporting the actual error.
> > 
> > Looks good to me. 
> 
> Me too.

Can we take that as an
Acked-by: Matt Wilson <msw@amazon.com>
?

I'm holding off committing anything at the moment until we get a test
pass and a staging push.

Ian.

> 
> Matt
> 
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > > 
> > >  	Michael Young
> > 
> > 

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

[Matt Wilson]

On Tue, Jul 03, 2012 at 01:09:04AM -0700, Ian Campbell wrote:
> On Mon, 2012-07-02 at 20:42 +0100, Matt Wilson wrote:
> > On Mon, Jul 02, 2012 at 04:22:34AM -0700, Ian Campbell wrote:
> > > > 
> > > > Here is version 5 of my patch, modified following Matt Wilson's 
> > > > suggestions, standardizing the indentations, moving some of the clean up 
> > > > out of the loop, and reporting the actual error.
> > > 
> > > Looks good to me. 
> > 
> > Me too.
> 
> Can we take that as an
> Acked-by: Matt Wilson <msw@amazon.com>
> ?

Sure,

Acked-by: Matt Wilson <msw@amazon.com>

> I'm holding off committing anything at the moment until we get a test
> pass and a staging push.

Probably a good idea. :-)

Thanks!

Matt

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

[Ian Campbell]

> Here is version 5 of my patch, modified following Matt Wilson's 
> suggestions, standardizing the indentations, moving some of the clean up 
> out of the loop, and reporting the actual error.

I have applied this, thanks.

Ian.