Re: [PATCH] Provides validation for the image file argument.

[Michael DeHaan]

Darryl L. Pierce wrote:
>  On Mon, Dec 22, 2008 at 04:27:13PM -0500, Darryl L. Pierce wrote:
>    
> > A file must have minimally a path and filename.
> >
> > If a file contains authentication information then it must also have a
> > hostname.
> >
> > Paths must be absolute.
> >
> > If the file includes a protocol then that is dropped.
> >
> > Signed-off-by: Darryl L. Pierce <dpierce(a)redhat.com&gt;
> > ---
> >  cobbler/item_image.py |   29 +++++++++++++++++++++++++----
> >  1 files changed, 25 insertions(+), 4 deletions(-)
> >
> > diff --git a/cobbler/item_image.py b/cobbler/item_image.py
> > index 98e0255..481951d 100644
> > --- a/cobbler/item_image.py
> > +++ b/cobbler/item_image.py
> > @@ -113,11 +113,32 @@ class Image(item.Item):
> >      def set_file(self,filename):
> >          """
> >          Stores the image location.  This should be accessible on all nodes
> > -        that need to access it.  Format: either /mnt/commonpath/foo.iso or 
> > -        nfs://host/path/foo.iso
> > +        that need to access it.  Format: can be one of the following:
> > +        * username:password@hostname:/path/to/the/filename.ext
> > +        * username@hostname:/path/to/the/filename.ext
> > +        * hostname:/path/to/the/filename.ext
> > +        * /path/to/the/filename.ext
> >          """
> > -        # FIXME: this should accept NFS paths or filesystem paths
> > -        self.file = filename
> > +        uri = ""
> > +        auth = hostname = path = ""
> > +        # we'll discard the protocol if it's supplied, for legacy support
> > +        if filename.find("://") != -1:
> > +            ignored, uri = filename.split("://")
> > +            filename = uri
> > +        else:
> > +            uri = filename
> > +
> > +        if filename.find("@") != -1: auth, filename =
> filename.split("@")
> > +        if filename.find(":") != -1: hostname, filename =
> filename.split(":")
> > +        # raise an exception if we don't have a valid path
> > +        if filename[0] != '/': raise CX(_("file contains an invalid
> path"))
> > +        if filename.find("/") != -1: path, filename =
> filename.rsplit("/", 1)
> > +
> > +        if len(filename) == 0:  raise CX(_("missing filename"))
> > +        if len(auth) > 0 and len(hostname) == 0:
> > +            raise CX(_("a hostname must be specified with authentication
> details"))
> > +
> > +        self.file = uri
> >          return True
> >  
> >      def set_os_version(self,os_version):
> > -- 
> > 1.6.0.6
> >     
>
>  Haven't heard any feedback on this patch in a month. I'm going to step
>  up and push it if nobody objects.
>
>    
>  ------------------------------------------------------------------------
>
>  _______________________________________________
>  cobbler mailing list
>  cobbler(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/cobbler
>     
It got lost in the patch queue.

A couple of things you need to do if you push it:

(A) remove the one line if's
(B) add unit tests to test_basic.py
(C) verify that it accepts "/path/to/foo.iso" and 
"nfs://user@path:/path/to/foo.iso" and "nfs://path:/path/to/foo.iso"
in 
terms of input.
(D) verify it does not accept relative paths

Writing something in test_basic.py for this would probably be a good idea.

--Michael