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

[Owen Taylor <otaylor redhat com>]

On Sun, 2009-08-09 at 21:12 +0200, Olav Vitters wrote:
> On Sun, Aug 09, 2009 at 02:59:11PM -0400, Owen Taylor wrote:
> >  # Backup!
> > +def shell_quote(s):
> > +    return "'" + s.replace("'", "'\\''") + "'"
> 
> Insure! Not acceptable for a script run by root, even if impact is
> minimal.

I'm not actually aware of any cases that that fails. It's a standard
technique for escaping arbitrary strings in shell.

http://www.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html

"Enclosing characters in single-quotes ( '' ) shall preserve the literal
value of each character within the single-quotes. A single-quote cannot
occur within single-quotes."

No exceptions other than ' itself. As far as I know, my code should be
pretty robust against weirdly named databases (assuming that we allow
people to create databases with weird names, which we typically
wouldn't. I'm already being paranoid)

But I agree that not using the shell is more straightforward, if not 
more convenient. I couldn't remember if subprocess was a Python 2.4 or
2.5 addition, but in a check it seems to be a 2.4 addition and thus OK 
on RHEL5.

> >  for db in dbs:
> > -    os.spawnlp(os.P_WAIT, 'mysqlhotcopy', 'mysqlhotcopy', '--quiet', '--allowold', db, '/var/lib/mysql-backup')
> 
> Better to use subprocess.
> 
> > +    table_status = os.popen("mysql --batch -e 'show table status' %s" % shell_quote(db), 'r')
> 
> Better to use subprocess
> 
> > +        os.spawnlp(os.P_WAIT, 'mysqlhotcopy', 'mysqlhotcopy', '--quiet', '--allowold', db, '/var/lib/mysql-backup')
> 
> Better to use subprocess
> 
> > +        os.spawnlp(os.P_WAIT, 'sh', 'sh', '-c',
> > +                  "mysqldump --single-transaction %s | gzip -c > %s" % (shell_quote(db), shell_quote(outfile)))
> 
> Use subprocess. You can pipe things together

Hmm, that one is going to be painful. I'll give it a go.

Any comments on matters other than subprocess?

- Owen