[PATCH] FTP transport append()

[John Arbash Meinel]

Alexandre Saint wrote:
> Oh, sorry. I forgot one more commit. Here's the right patch.
> 
> On Sun, Apr 16, 2006 at 01:15:12PM +0200, Alexandre Saint wrote:
>> Hi,
>>
>> Here's another patch for the FTP transport:
>>
>> - Implemented a append() method. So pushing with knit format is working now.
>> - Fixed _abspath() to return the given path when it is given a absolute path.
> - Simplified listdir().
> 
> In fact, the NLST command of the FTP protocol will only return *named* files,
> so no "." or "..". Moreover, the files are returned relative.
> 
>> Cheers,
>> Alex.
>> -- 
>> alex
> 
> 

I'll try to give it a decent review:
> 
> === modified file 'a/bzrlib/transport/ftp.py'
> --- a/bzrlib/transport/ftp.py	
> +++ b/bzrlib/transport/ftp.py	
> @@ -134,8 +134,8 @@
>              relpath_parts = relpath[:]
>          if len(relpath_parts) > 1:
>              if relpath_parts[0] == '':
> -                raise ValueError("path %r within branch %r seems to be absolute"
> -                                 % (relpath, self._path))
> +                # the path seems to be absolute
> +                return relpath
>          basepath = self._path.split('/')
>          if len(basepath) > 0 and basepath[-1] == '':
>              basepath = basepath[:-1]


I don't think we should allow arbitrary absolute paths at this point.
Because that would allow people to access files outside of this branch,
which seems like a security risk.
We probably need to check that the path starts with our expected base path.

I also don't think the interface for _abspath() allows absolute paths to
be passed in. But I wasn't able to find _abspath in other transports, so
I don't have much to go off of. (In other words, passing in a
non-relative path really is an error)

> @@ -303,11 +303,39 @@
>                  raise TransportError(msg="Cannot remove directory at %s" % \
>                          self._abspath(rel_path), extra=str(e))
>  
> -    def append(self, relpath, f):
> +    def append(self, relpath, fp, retries=0):
>          """Append the text in the file-like object into the final
>          location.
>          """
> -        raise TransportNotPossible('ftp does not support append()')
> +        if isinstance(fp, basestring):
> +            fp = StringIO(fp)
> +        fp.seek(0)

I'm pretty sure that we decided to get rid of magic interfaces which
have us check isinstance(fp, basestring)

Also, 'fp' is not guaranteed not to be in the middle of some other file.
So doing "fp.seek(0)" is not safe. LocalTransport uses fp.seek(0,2) to
make sure that we have a location (because fp.tell() fails on win32).

Also, changing the parameter name from 'f' to 'fp' is an interface
change, so any code that does transport.append(relpath='xxx', f=myfile)
will fail.

I would leave the parameter named 'f'.


> +        try:
> +            abspath = self._abspath(relpath)
> +            mutter("FTP appe to %s" % abspath)
> +            ftp = self._get_FTP()
> +            if self.has(abspath):
> +                length = ftp.size(abspath)
> +            else:
> +                length = 0
> +            ftp.voidcmd("TYPE I")
> +            cmd = "APPE %s" % abspath
> +            conn = ftp.transfercmd(cmd)
> +            conn.sendall(fp.read())
> +            conn.close()
> +            ftp.getresp()
> +            return length
> +        except ftplib.error_perm, e:
> +            FtpTransportError("Error appending data to %s" % abspath,
> +                    orig_error=e)
> +        except ftplib.error_temp, e:
> +            if retries > _number_of_retries:
> +                raise TransportError("FTP temporary error during APPEND %s." \
> +                        "Aborting." % self.abspath(relpath), orig_error=e)
> +            else:
> +                warning("FTP temporary error: %s. Retrying." % str(e))
> +                self._FTP_instance = None
> +                self.append(relpath, fp, retries+1)

Otherwise this function looks good. (As long as APPE really is a
standard ftp command)

>  
>      def copy(self, rel_from, rel_to):
>          """Copy the item at rel_from to the location at rel_to"""
> @@ -347,11 +375,7 @@
>              mutter("FTP nlst: %s" % self._abspath(relpath))
>              f = self._get_FTP()
>              basepath = self._abspath(relpath)
> -            # FTP.nlst returns paths prefixed by relpath, strip 'em
> -            the_list = f.nlst(basepath)
> -            stripped = [path[len(basepath)+1:] for path in the_list]
> -            # Remove . and .. if present, and return
> -            return [path for path in stripped if path not in (".", "..")]
> +            return f.nlst(basepath)
>          except ftplib.error_perm, e:
>              raise TransportError(orig_error=e)

Are you sure this is the case for all ftp servers? I have the feeling
that some of them return one form, and some return another. So we need
to have some sort of detection of what type of files we are getting
back, and properly handle it. I'm guessing someone was testing the code,
and that is what they found.

That said, I can say that with the only ftp server I have access to, it
does indeed only return the base name, with no prefix on it. Because
doing "t.listdir('.')" returns files with their first letter chopped off.

John
=:->


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060421/64d39536/attachment.pgp