Re: Patch for review: "Handle InnoDB tables in copy-backup.py"

[Tobias Mueller <muelli auftrags-killer org>]

Hey :)

Owen Taylor wrote:
> Any comments on matters other than subprocess?

A minor nitpick just to have it slightly more readable and faster:
Instead of
+    first = True
+    can_hotcopy = True
+    for line in table_status.stdout:
+        if first: # skip header line
+            first = False
+            continue
+        fields = line.rstrip().split("\t")

I'd do:
can_hotcopy = True
header_line = table_status.stdout.readline()
for content_line in table_status.stdout:
    fields = content_line.rstrip().split("\t")
    [...]



Another issue:

+  outfilename = os.path.join('/var/lib/mysql-backup/', db + ".dump.gz")

That doesn't make any sense because you have already used the "/" as 
path separator :) Note, that os.path.join() doesn't escape or normalizes 
anything (like "../").

You probably want to do
outfilename = '/var/lib/mysql-backup/%s.dump.gz' % db


And in case anybody creates a database name with metacharacters for the 
filesystem (like "." or "/" or ".."), we should normalize and check 
whether we have left the directory just for security reasons. If so, 
bail out. Note, that I don't know if it's even possible to create such a 
database with MySQL. If not, the following is obsolete (as long as we 
stick to MySQL).

BACKUP_SCHEMA = '/var/lib/mysql-backup/%s.dump.gz'
outfilename = BACKUP_SCHEMA % db
outfile_norm = os.path.normpath(outfilename)
if not os.path.commonprefix((BACKUP_PREFIX, outfile_norm)) == \
                                                          BACKUP_SCHEMA:
    verbose("Tried to backup %s in %s but that would not follow the" +\
             backup scheme" % (db, outfilename, BACKUP_PREFIX))
    sys.exit(1)


Note that this doesn't really solve issues if a database is named like 
"./foundationmembers". It'll override the "foundationmembers" backup. 
But that problem exists with the current patch as well and I don't know 
a really good solution to this yet, besides prohibiting to create or 
backup weirdly named databases or padding with a hash of the database name.

Cheers,
  Tobi