Merge lp:~vila/bzr/443041-ftp-append-bytes into lp:bzr

[Vincent Ladeuil]

[Vincent Ladeuil (vila)]

The bug occurred when I installed pyftpdlib on the jaunty slave.

Before that, no ftp server was available so we didn't have the same
coverage (hopefully medusa is installed on pqm... anybody can confirm ?)

Anyway, I also fixed a spurious warning from the server thread that rarely occurs
(but was present in the babune failure).

The root cause was that apparently we tried to issue a SIZE command while being in ASCII mode
instead of BINARY mode, separating the transfer command (and using ftplib facility for that
instead of redoing that ourself) from the size command avoided the issue.

I also avoid reading the whole text to be appended, our ftp transport is already pretty bad
are buffering the whole file when reading, no need to make it worse than necessary here.

[Vincent Ladeuil (vila)]

I also cleaned up some useless imports.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/443041-ftp-append-bytes into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> The bug occurred when I installed pyftpdlib on the jaunty slave.
>
> Before that, no ftp server was available so we didn't have the same
> coverage (hopefully medusa is installed on pqm... anybody can confirm ?)
>
> Anyway, I also fixed a spurious warning from the server thread that rarely occurs
> (but was present in the babune failure).
>
> The root cause was that apparently we tried to issue a SIZE command while being in ASCII mode
> instead of BINARY mode, separating the transfer command (and using ftplib facility for that
> instead of redoing that ourself) from the size command avoided the issue.
>
> I also avoid reading the whole text to be appended, our ftp transport is already pretty bad
> are buffering the whole file when reading, no need to make it worse than necessary here.

I disagree with changing to use a file pointer rather than the raw text.
Mostly because we never actually call append with a file pointer, but we
call it with bytes to be appended. I think the FTP layering has it
wrong, and the 'file' form should be calling the 'bytes' form, rather
than the other way around...

I was originally concerned that you require '.tell' and '.seek' or the
file-like objects, as there are a lot of file-like objects that don't
implement it. (urllib's socket handler, reading from stdin, etc.)

Ultimately, we won't be buffering any less with your patch, it just
means we'll be reading from the StringIO wrapper more...

I'm also having some difficulty sorting out what your actual fix was,
because of all the other noise. Something about not actually returning
the start bytes?

- - def _fallback_append(self, relpath, text, mode = None):
+ def _fallback_append(self, relpath, fp, mode = None):
         remote = self.get(relpath)
- - remote.seek(0, 2)
- - remote.write(text)
- - remote.seek(0, 0)
+ remote.seek(0, os.SEEK_END)
+ osutils.pumpfile(fp, remote)
+ remote.seek(0)
         return self.put_file(relpath, remote, mode)

^- I guess I didn't realize this was going on, but writing to the
file-like object that you get back from "get()" is rather ugly.... I
suppose if we know that \.get() returns a StringIO() we can do it.
Though note that doing .pumpfile() introduces a memory thrashing
condition, where we append bytes to an existing string N/32kB times, and
thus suffer that many reallocations. (Pushing up 100MB is thus 3200
reallocations of 100MB of data...)

Though I guess if we are worried about FTP performance, this is the
least of our worries. When we push up a 100MB pack, we are likely going
to do it 1MB at a time, and since upper level code assumes that .append
is available, we are going to:
  create the file
  read an empty file
  add 1MB
  put the 1MB file back up
  read the 1MB file
  add 1MB
  put the 2MB file back up
  ...

Which I think means we read (1-100MB) 5GB and write 5GB... Regardless of
the reallocations we do in memory to append 1MB to the big buffer, 32k...

review: Needs Information

[Vincent Ladeuil (vila)]

>>>>> "jam" == John A Meinel <email address hidden> writes:

<snip/>

    jam> I disagree with changing to use a file pointer rather than the raw text.

You should have read the code more closely, all the modified
calls happen under append_file, which receives a file-like object
(that the defined *API*).

    jam> Mostly because we never actually call append with a file
    jam> pointer, but we call it with bytes to be appended.

No. append_file() is called with a file.

    jam> I think the FTP layering has it wrong, and the 'file'
    jam> form should be calling the 'bytes' form, rather than the
    jam> other way around...

ESCOPE.

    jam> I was originally concerned that you require '.tell' and
    jam> '.seek' or the file-like objects, as there are a lot of
    jam> file-like objects that don't implement it. (urllib's
    jam> socket handler, reading from stdin, etc.)

seek() is already needed, AFAIK tell() is always implemented when seek() is.

    jam> Ultimately, we won't be buffering any less with your patch, it just
    jam> means we'll be reading from the StringIO wrapper more...

Yes. Eliminating an *additional* buffering layer.

    jam> I'm also having some difficulty sorting out what your
    jam> actual fix was, because of all the other
    jam> noise. Something about not actually returning the start
    jam> bytes?

Ha, right, my bug report was a bit sparse, sorry.

What happened was that we couldn't get the size of the file (that
we want to return after appending) so we returned 0 instead and
that made the test fail.

We couldn't get the size because the mode was ASCII and you can't
issue a SIZE command then.

The mode was ASCII because the APPE command was not implemented
using ftp.transfercmd().

ftp.transfercmd() wants a file-like object.

The file-like object has been transformed into its content by the
caller for no apparent good reasons.

I reversed that transformation to be able to still use the
file-like object provided by the caller of my caller.

And then you vetoed that :-D

    jam> - def _fallback_append(self, relpath, text, mode = None):
    jam> + def _fallback_append(self, relpath, fp, mode = None):
    jam> remote = self.get(relpath)
    jam> - remote.seek(0, 2)
    jam> - remote.write(text)
    jam> - remote.seek(0, 0)
    jam> + remote.seek(0, os.SEEK_END)
    jam> + osutils.pumpfile(fp, remote)
    jam> + remote.seek(0)
    jam> return self.put_file(relpath, remote, mode)

    jam> ^- I guess I didn't realize this was going on, but writing to the
    jam> file-like object that you get back from "get()" is rather ugly....

to say the least, but ESCOPE.

<snip/>

    jam> So... "we already buffer to much", arguably we don't buffer enough yet...

I reduced the buffering and stands by that sentence :D

    jam> What we need is a custom FTPStreamWriter that when it
    jam> realizes the remote site doesn't support APPE then
    jam> buffers to a temp file on disk, and pushes up the whole
    jam> content once we have finished.

Different bug....

    jam> Anyway, I'm not sure I can approve this as a bug fix,
    jam> because I can't really s...

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

...

>
> jam> Mostly because we never actually call append with a file
> jam> pointer, but we call it with bytes to be appended.
>
> No. append_file() is called with a file.

FTPTransport.append_bytes is not directly implemented, instead it uses
the compatibility thunk defined in Transport:
    def append_bytes(self, relpath, bytes, mode=None):
...
        if not isinstance(bytes, str):
            raise TypeError(
                'bytes must be a plain string, not %s' % type(bytes))
        return self.append_file(relpath, StringIO(bytes), mode=mode)

>
> jam> I think the FTP layering has it wrong, and the 'file'
> jam> form should be calling the 'bytes' form, rather than the
> jam> other way around...
>
> ESCOPE.

My very specific point is that our code *never* calls append_file
directly, and *always* calls append_bytes. The fact that you have a
'file-like' object is bogus, because we really passed in a string, that
we then faked. (Originally, all of Transport was file-like object based,
until we realized that a lot of things were done with raw bytes and we
were wasting a lot of time translating.)

>
> jam> I was originally concerned that you require '.tell' and
> jam> '.seek' or the file-like objects, as there are a lot of
> jam> file-like objects that don't implement it. (urllib's
> jam> socket handler, reading from stdin, etc.)
>
> seek() is already needed, AFAIK tell() is always implemented when seek() is.
>
> jam> Ultimately, we won't be buffering any less with your patch, it just
> jam> means we'll be reading from the StringIO wrapper more...
>
> Yes. Eliminating an *additional* buffering layer.

Sort of. In that "cStringIO.StringIO(s).read() is not s"

But completely not in that we didn't just implement "append_bytes()" in
the first place.

...

>
> ftp.transfercmd() wants a file-like object.
>
> The file-like object has been transformed into its content by the
> caller for no apparent good reasons.
>
> I reversed that transformation to be able to still use the
> file-like object provided by the caller of my caller.
>
> And then you vetoed that :-D
>

So there isn't a form that takes a plain string? Certainly we *don't*
want to be appending in ASCII mode, given that we are usually appending
compressed bytes.

>
> jam> Anyway, I'm not sure I can approve this as a bug fix,
> jam> because I can't really see the bug fix. Changing from
> jam> taking 'text' to taking a file-like object I think is
> jam> the wrong fix,
>
> You don't look at the right part. I don't think using a file-like
> object that produced the string is not wrong.

Well, only that I was thinking up at the higher level.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkrKOpcACgkQJdeBCYSNAANQqwCfQHN+iUG4GZ4Jg7OpSLud2mfv
3uwAn1TIFXn7EYMuLwaUg+t4A3c7mJ8q
=TbDg
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

John, I'm sorry if my last email wasn't clear.

Many points in your analysis are correct and I agreed with
them. You outlined at least 2 bugs in our current ftp
implementation.

The bug I am fixing is a regression. Our test suite is not
passing anymore on jaunty with pyftpdlib installed.

I can uninstall it or fix that bug.

<snip/>

    jam> My very specific point is that our code *never* calls
    jam> append_file directly, and *always* calls
    jam> append_bytes. The fact that you have a 'file-like'
    jam> object is bogus,

That's bug #1.

<snip/>

    jam> Sort of. In that "cStringIO.StringIO(s).read() is not s"

Yes. Not even a fraction of the other bugs you're pointing.

    jam> But completely not in that we didn't just implement
    jam> "append_bytes()" in the first place.

True.

<snip/>

    jam> So there isn't a form that takes a plain string?

At that layer. No. append_bytes() is implemented with
append_file().

That's still bug #1.

    jam> Certainly we *don't* want to be appending in ASCII mode,
    jam> given that we are usually appending compressed bytes.

Obviously. We *are* transferring bytes. But somehow the mode ends
up being ASCII *anyway* (the log reveals that the server errors
on 'Can't use SIZE in ASCII mode (or something like that, I don't
have a copy at hand anymore). That's the only thing I want to fix
here so the core of the fix is:

- cmd = "APPE %s" % abspath
- conn = ftp.transfercmd(cmd)
- conn.sendall(text)
- conn.close()
+ starting_at = fp.tell()
+ ftp.storbinary("APPE %s" % abspath, fp)
  self._setmode(relpath, mode)
- ftp.getresp()

The _setmode() and the transfercmd() were mixed, the getresp()
call is part of the ftplib.storbinary() implementation.

Just swapping the two lines can be enough. But we are at the
other hand of the consequences of bug #1, ftplib really want a
file-like object.... and that's from where I went up back to the
previous transformation of the file-like object required by the
implementation.

That's why "changing from taking 'text' to taking a file-like
object" is the right fix, because it eliminates a file ->
text -> file transformation.

Did you say ironic ?

:-D

<snip/>

    jam> Well, only that I was thinking up at the higher level.

No doubt and that was nice catches.

bug #2 is that we shouldn't use the result of get() to call
_put_file(). At a minimum the file should be downloaded once and
once only. We may use a StringIO() up to some threshold (64K, 1M
?) and degrade to a temp file.

But those two bugs were there before this bug, let's address it
first.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

...

>
> jam> So there isn't a form that takes a plain string?
>
> At that layer. No. append_bytes() is implemented with
> append_file().
>
> That's still bug #1.
>
> jam> Certainly we *don't* want to be appending in ASCII mode,
> jam> given that we are usually appending compressed bytes.
>
> Obviously. We *are* transferring bytes. But somehow the mode ends
> up being ASCII *anyway* (the log reveals that the server errors
> on 'Can't use SIZE in ASCII mode (or something like that, I don't
> have a copy at hand anymore). That's the only thing I want to fix
> here so the core of the fix is:
>
> - cmd = "APPE %s" % abspath
> - conn = ftp.transfercmd(cmd)
> - conn.sendall(text)
> - conn.close()
> + starting_at = fp.tell()
> + ftp.storbinary("APPE %s" % abspath, fp)
> self._setmode(relpath, mode)
> - ftp.getresp()
>
> The _setmode() and the transfercmd() were mixed, the getresp()
> call is part of the ftplib.storbinary() implementation.
>
> Just swapping the two lines can be enough. But we are at the
> other hand of the consequences of bug #1, ftplib really want a
> file-like object.... and that's from where I went up back to the
> previous transformation of the file-like object required by the
> implementation.

If you look at the details of "storbinary" you can see:

    def storbinary(self, cmd, fp, blocksize=8192, callback=None):
...
        self.voidcmd('TYPE I')
        conn = self.transfercmd(cmd)
        while 1:
            buf = fp.read(blocksize)
            if not buf: break
            conn.sendall(buf)
            if callback: callback(buf)
        conn.close()
        return self.voidresp()

^- So I would say the appropriate fix is not to change to calling
storbinary, but instead to just add:

 self.voidcmd('TYPE I')

just before the existing
  conn = ftp.transfercmd('APPE %s' % (filename,))

My actual guess is that if the transmission is being done in ASCII mode,
then we have a different bug, possibly just in the test suite. (Meaning
this bug doesn't actually cause problem is the wild, but is exposed when
running the test suite.)

Most likely, we do something like "os.list_dir()" in the test just
before the "append" test, and that the "list_dir()" is setting the
socket into ascii mode, and doesn't set the mode back into binary for
the file transfer.

I'm not sure if we always want to set binary even when we don't need to,
but as long as it doesn't actually incur a full round trip, it is
probably fine. But no..

    def voidcmd(self, cmd):
        """Send a command and expect a response beginning with '2'."""
        self.putcmd(cmd)
        return self.voidresp()

^- So it means that if we don't track the binary/ascii ourselves, then
using storbinary() actually triggers an *extra* round-trip per APPE call.

...

> But those two bugs were there before this bug, let's address it
> first.

There are a lot of bugs/gremlins in this code... *I* have never taken it
all that seriously versus sftp/ssh support.

What I also don't understand is this code by Vincent Ladeuil on line 557:

        finally:
            # Restore binary mode as nlst switch to ascii mode to
r...

[Vincent Ladeuil (vila)]

>>>>> "jam" == John A Meinel <email address hidden> writes:

<snip/>

    >> The _setmode() and the transfercmd() were mixed, the getresp()
    >> call is part of the ftplib.storbinary() implementation.

While it's a bit ugly that wasn't the real root cause.

<snip/>

    jam> If you look at the details of "storbinary" you can see:

    jam> def storbinary(self, cmd, fp, blocksize=8192, callback=None):
    jam> ...
    jam> self.voidcmd('TYPE I')
    jam> conn = self.transfercmd(cmd)
    jam> while 1:
    jam> buf = fp.read(blocksize)
    jam> if not buf: break
    jam> conn.sendall(buf)
    jam> if callback: callback(buf)
    jam> conn.close()
    jam> return self.voidresp()

    jam> ^- So I would say the appropriate fix is not to change to calling
    jam> storbinary,

No comment about the fact that storbinary() implements *some*
buffering where we don't ?

No fear about sendall() blowing up with too big strings ?

Ok, let's called that bug #3 since I reverted that part.

    jam> but instead to just add:

    jam> self.voidcmd('TYPE I')

    jam> just before the existing
    jam> conn = ftp.transfercmd('APPE %s' % (filename,))

I deleted that line you want to add to avoid the incurred full
round-trip when I better tracked the binary mode (you refer to
that later, that's revno 4063 in bzr.dev).

That's where I (but all of us with me) fall into the trap of
untested code.

Namely, the GSSAPI ftp implementation is not actually tested
because we don't have a kerberos test server.

Yet, the way it's implemented is that we create a GSSAPI ftp
client if the python kerberos module is present (it is on my
jaunty host), try to authenticate with that but fallback to the
regular authentication if we can't.

And that's all that is tested, the fallback, but *only* if the
kerberos module is present !

That module wasn't installed when I landed revno 4063 (on neither
my test host nor pqm, nor anybody running the full test suite
with an ftp test server, any takers ?), so I didn't test the
combination kerberos+[medusa|pyftpdlib], I only tested medusa and
then pyftpdlib (since only one is used I had to setup the three
test envs: no ftp server, medusa, pyftpdlib).

    jam> My actual guess is that if the transmission is being
    jam> done in ASCII mode, then we have a different bug,
    jam> possibly just in the test suite. (Meaning this bug
    jam> doesn't actually cause problem is the wild, but is
    jam> exposed when running the test suite.)

    jam> Most likely, we do something like "os.list_dir()" in the
    jam> test just before the "append" test, and that the
    jam> "list_dir()" is setting the socket into ascii mode, and
    jam> doesn't set the mode back into binary for the file
    jam> transfer.

Quite the contrary, invoking list_dir() force the mode back to
binary because that's the only command that requires the ASCII
mode. So any call to list_dir() would have *avoided* the bug not
revealed it.

<snip/>

    jam> ^- So it means that if we don't track the binary/ascii
    jam> ourselves, then using storbinary() actually triggers an
    jam> *ext...

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/443041-ftp-append-bytes into lp:bzr.
>
> Requested reviews:
> John A Meinel (jameinel)
>
>

So as I understand it, the main change is that _create_connection for
the regular ftp server was calling:
             # binary mode is the default
             connection.voidcmd('TYPE I')

And the implementation of the GSSAPI ftp server was not. So you
refactored the code such that only "_login()" is overridden, rather than
all of "_create_connection".

Less code duplication is very nice.

 review: approve
 merge: approve

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkrLX5IACgkQJdeBCYSNAANGBgCgyRAkVr0jmG861id48+LGlRqe
IyUAn0xCfFiFsT6JT0Cuv+Hy9u50TaEd
=X5qe
-----END PGP SIGNATURE-----

review: Approve

[Vincent Ladeuil (vila)]

>>>>> "jam" == John A Meinel <email address hidden> writes:

    jam> Review: Approve
    jam> Vincent Ladeuil wrote:
    >> Vincent Ladeuil has proposed merging lp:~vila/bzr/443041-ftp-append-bytes into lp:bzr.
    >>
    >> Requested reviews:
    >> John A Meinel (jameinel)
    >>
    >>

    jam> So as I understand it, the main change is that _create_connection for
    jam> the regular ftp server was calling:
    jam> # binary mode is the default
    jam> connection.voidcmd('TYPE I')

    jam> And the implementation of the GSSAPI ftp server was not. So you
    jam> refactored the code such that only "_login()" is overridden, rather than
    jam> all of "_create_connection".

Yes. _login and connection_class.

    jam> Less code duplication is very nice.

And means less bugs an less discussions :-D

    jam> review: approve
    jam> merge: approve

Thanks for your patience

=== modified file 'NEWS'
--- NEWS	2009-10-06 02:38:44 +0000
+++ NEWS	2009-10-06 08:55:22 +0000
@@ -104,6 +104,10 @@
   with some combinations of remote and local formats.  This was causing
   "unknown object type identifier 60" errors.  (Andrew Bennetts, #427736)
 
+* ftp transports were built differently when the kerberos python module was
+  present leading to obscure failures related to ASCII/BINARY modes.
+  (Vincent Ladeuil, #443041)
+
 * Network streams now decode adjacent records of the same type into a
   single stream, reducing layering churn. (Robert Collins)
 

=== modified file 'bzrlib/tests/ftp_server/pyftpdlib_based.py'
--- bzrlib/tests/ftp_server/pyftpdlib_based.py	2009-03-23 14:59:43 +0000
+++ bzrlib/tests/ftp_server/pyftpdlib_based.py	2009-10-06 08:55:22 +0000
@@ -84,7 +84,7 @@
         line = self.fs.fs2ftp(path)
         if self.fs.isfile(self.fs.realpath(path)):
             why = "Not a directory: %s" % line
-            self.log('FAIL SIZE "%s". %s.' % (line, why))
+            self.log('FAIL NLST "%s". %s.' % (line, why))
             self.respond("550 %s."  %why)
         else:
             ftpserver.FTPHandler.ftp_NLST(self, path)
@@ -180,8 +180,7 @@
         self._port = self._ftp_server.socket.getsockname()[1]
         self._ftpd_starting = threading.Lock()
         self._ftpd_starting.acquire() # So it can be released by the server
-        self._ftpd_thread = threading.Thread(
-                target=self._run_server,)
+        self._ftpd_thread = threading.Thread(target=self._run_server,)
         self._ftpd_thread.start()
         # Wait for the server thread to start (i.e release the lock)
         self._ftpd_starting.acquire()
@@ -202,7 +201,11 @@
         self._ftpd_running = True
         self._ftpd_starting.release()
         while self._ftpd_running:
-            self._ftp_server.serve_forever(timeout=0.1, count=1)
+            try:
+                self._ftp_server.serve_forever(timeout=0.1, count=1)
+            except select.error, e:
+                if e.args[0] != errno.EBADF:
+                    raise
         self._ftp_server.close_all(ignore_all=True)
 
     def add_user(self, user, password):

=== modified file 'bzrlib/transport/ftp/__init__.py'
--- bzrlib/transport/ftp/__init__.py	2009-07-22 06:51:13 +0000
+++ bzrlib/transport/ftp/__init__.py	2009-10-06 08:55:22 +0000
@@ -25,17 +25,13 @@
 """
 
 from cStringIO import StringIO
-import errno
 import ftplib
 import getpass
 import os
-import os.path
-import urlparse
 import random
 import socket
 import stat
 import time
-from warnings import warn
 
 from bzrlib import (
     config,
@@ -51,8 +47,6 @@
     register_urlparse_netloc_protocol,
     Server,
     )
-from bzrlib.transport.local import LocalURLServer
-import bzrlib.ui
 
 
 register_urlparse_netloc_protocol('aftp')
@@ -99,8 +93,9 @@
             self.is_active = True
         else:
             self.is_active = False
-        
-        # Most modern FTP servers support the APPE command. If ours doesn't, we (re)set this flag accordingly later.
+
+        # Most modern FTP servers support the APPE command. If ours doesn't, we
+        # (re)set this flag accordingly later.
         self._has_append = True
 
     def _get_FTP(self):
@@ -113,6 +108,8 @@
             self._set_connection(connection, credentials)
         return connection
 
+    connection_class = ftplib.FTP
+
     def _create_connection(self, credentials=None):
         """Create a new connection with the provided credentials.
 
@@ -138,13 +135,9 @@
                ((self._host, self._port, user, '********',
                 self.is_active),))
         try:
-            connection = ftplib.FTP()
+            connection = self.connection_class()
             connection.connect(host=self._host, port=self._port)
-            if user and user != 'anonymous' and \
-                    password is None: # '' is a valid password
-                password = auth.get_password('ftp', self._host, user,
-                                             port=self._port)
-            connection.login(user=user, passwd=password)
+            self._login(connection, auth, user, password)
             connection.set_pasv(not self.is_active)
             # binary mode is the default
             connection.voidcmd('TYPE I')
@@ -157,6 +150,13 @@
                                         " %s" % str(e), orig_error=e)
         return connection, (user, password)
 
+    def _login(self, connection, auth, user, password):
+        # '' is a valid password
+        if user and user != 'anonymous' and password is None:
+            password = auth.get_password('ftp', self._host,
+                                         user, port=self._port)
+        connection.login(user=user, passwd=password)
+
     def _reconnect(self):
         """Create a new connection with the previously used credentials"""
         credentials = self._get_credentials()
@@ -391,7 +391,6 @@
         location.
         """
         text = f.read()
-        
         abspath = self._remote_path(relpath)
         if self.has(relpath):
             ftp = self._get_FTP()
@@ -426,18 +425,18 @@
         except ftplib.error_perm, e:
             # Check whether the command is not supported (reply code 502)
             if str(e).startswith('502 '):
-                warning("FTP server does not support file appending natively. " \
-                    "Performance may be severely degraded! (%s)", e)
+                warning("FTP server does not support file appending natively. "
+                        "Performance may be severely degraded! (%s)", e)
                 self._has_append = False
                 self._fallback_append(relpath, text, mode)
             else:
                 self._translate_perm_error(e, abspath, extra='error appending',
                     unknown_exc=errors.NoSuchFile)
-            
         except ftplib.error_temp, e:
             if retries > _number_of_retries:
-                raise errors.TransportError("FTP temporary error during APPEND %s." \
-                        "Aborting." % abspath, orig_error=e)
+                raise errors.TransportError(
+                    "FTP temporary error during APPEND %s. Aborting."
+                    % abspath, orig_error=e)
             else:
                 warning("FTP temporary error: %s. Retrying.", str(e))
                 self._reconnect()
@@ -445,9 +444,9 @@
 
     def _fallback_append(self, relpath, text, mode = None):
         remote = self.get(relpath)
-        remote.seek(0, 2)
+        remote.seek(0, os.SEEK_END)
         remote.write(text)
-        remote.seek(0, 0)
+        remote.seek(0)
         return self.put_file(relpath, remote, mode)
 
     def _setmode(self, relpath, mode):

=== modified file 'bzrlib/transport/ftp/_gssapi.py'
--- bzrlib/transport/ftp/_gssapi.py	2009-09-18 01:25:08 +0000
+++ bzrlib/transport/ftp/_gssapi.py	2009-10-06 08:55:22 +0000
@@ -97,52 +97,22 @@
 
     """
 
-    def _create_connection(self, credentials=None):
-        """Create a new connection with the provided credentials.
-
-        :param credentials: The credentials needed to establish the connection.
-
-        :return: The created connection and its associated credentials.
-
-        The credentials are a tuple with the username and password. The
-        password is used if GSSAPI Authentication is not available.
+    connection_class = GSSAPIFtp
+
+    def _login(self, connection, auth, user, password):
+        """Login with GSSAPI Authentication.
+
+        The password is used if GSSAPI Authentication is not available.
 
         The username and password can both be None, in which case the
         credentials specified in the URL or provided by the
         AuthenticationConfig() are used.
         """
-        if credentials is None:
-            user, password = self._user, self._password
-        else:
-            user, password = credentials
-
-        auth = config.AuthenticationConfig()
-        if user is None:
-            user = auth.get_user('ftp', self._host, port=self._port,
-                                 default=getpass.getuser())
-        mutter("Constructing FTP instance against %r" %
-               ((self._host, self._port, user, '********',
-                self.is_active),))
         try:
-            connection = GSSAPIFtp()
-            connection.connect(host=self._host, port=self._port)
-            try:
-                connection.gssapi_login(user=user)
-            except ftplib.error_perm, e:
-                if user and user != 'anonymous' and \
-                        password is None: # '' is a valid password
-                    password = auth.get_password('ftp', self._host, user,
-                                                 port=self._port)
-                connection.login(user=user, passwd=password)
-            connection.set_pasv(not self.is_active)
-        except socket.error, e:
-            raise errors.SocketConnectionError(self._host, self._port,
-                                               msg='Unable to connect to',
-                                               orig_error= e)
+            connection.gssapi_login(user=user)
         except ftplib.error_perm, e:
-            raise errors.TransportError(msg="Error setting up connection:"
-                                        " %s" % str(e), orig_error=e)
-        return connection, (user, password)
+            super(GSSAPIFtpTransport, self)._login(connection, auth,
+                                                   user, password)
 
 
 def get_test_permutations():