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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
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)
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
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?
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
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Matt Wilson
2012-Jun-26 00:13 UTC
Re: pygrub: avoid problems if guest files are large etc.
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=1048576All 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
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Ian Campbell
2012-Jul-02 11:22 UTC
Re: pygrub: avoid problems if guest files are large etc.
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
Matt Wilson
2012-Jul-02 19:42 UTC
Re: pygrub: avoid problems if guest files are large etc.
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 > >
Ian Campbell
2012-Jul-03 08:09 UTC
Re: pygrub: avoid problems if guest files are large etc.
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 > > > >
Matt Wilson
2012-Jul-03 16:07 UTC
Re: pygrub: avoid problems if guest files are large etc.
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
Ian Campbell
2012-Jul-04 14:48 UTC
Re: pygrub: avoid problems if guest files are large etc.
> 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.