From 3ee21808e5590d4343ddc37b25ba86c825612c8c Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 28 Jul 2011 18:37:29 +0100 Subject: [PATCH] Huge changes to virt-server and shell quoting - replace print-execute-command with print-shscript-command and print-auxverb-command - sort out shell quoting - bugfixes - other cleanups --- TODO | 6 +-- doc/README.virtualisation-server | 34 +++++++------- runner/adt-run | 80 ++++++++++++++++--------------- virt-subproc/VirtSubproc.py | 81 ++++++++++++++++++++++++-------- virt-subproc/adt-virt-chroot | 32 +++++++++---- virt-subproc/adt-virt-null | 13 ++--- virt-subproc/adt-virt-schroot | 13 +++-- virt-subproc/adt-virt-xenlvm | 10 ++-- 8 files changed, 168 insertions(+), 101 deletions(-) diff --git a/TODO b/TODO index 82b9781..c4b2f77 100644 --- a/TODO +++ b/TODO @@ -8,9 +8,7 @@ not needed for demo: - remove dependency on python in testbed -- Nightmare shell quoting situation: - * satdep-auxverb and the sh -c hack - * testbeds should provide /only/ print-execute-command auxverb - - schroot's snapshots should use dmsetup directly, not lvm ? at least there should be non-persistent snapshots + +- remove debugging from auxverb script diff --git a/doc/README.virtualisation-server b/doc/README.virtualisation-server index d391d65..7d40ef5 100644 --- a/doc/README.virtualisation-server +++ b/doc/README.virtualisation-server @@ -63,11 +63,6 @@ Protocol (with distinct s) may be advertised in which case more than one such user is available. - + print-execute-command - The 'print-execute-command' command is available, so that the - caller can execute multiple concurrent commands on the testbed - with asynchronous input and output, if desired. - * Command open @@ -77,11 +72,14 @@ Protocol Checks that the testbed is present and reserves it (waiting for other uses of the testbed to finish first, if necessary). State: Closed to Open. is a pathname on the - testbed which can be used freely by the test scripts. + testbed which can be used freely by the test scripts and which + is valid until the next `close', `revert' or `quit'. * Command revert + response + ok Restores the testbed, undoing all of the changes made so far. State: Open, remains Open. Only available if the `revert' @@ -96,25 +94,27 @@ Protocol advertised). State: Open to Closed. -* Command - print-execute-command +* Commands + print-auxverb-command + print-shstring-command response - ok ,,... auxverb|shstring [ ...] + ok ,,... [ ...] Prints a command that can be executed by the caller to run a command - on the testbed. Only available if the `print-execute-command' - capability is advertised. + on the testbed. The command has the following properties (which are, for example, satisfiable when the virt server uses `env' `ssh' or `dchroot'): - The caller is expected to url-decode and each , append the command to be run on the testbed, and call exec on the results. - - If auxverb is advertised, the supplied additional arguments to - command will be interpreted as the command and arguments to be - run on the testbed (as env and nice interpret their arguments) - - If shstring is advertised, there should be one additional - argument which will be fed to sh -c on the testbed (this is the - way ssh interprets its arguments). + - For the command provided by `print-auxverb-command', the supplied + additional arguments to command will be interpreted as the + command and arguments to be run on the testbed (as env and nice + interpret their arguments) + - The command provided by `print-shstring-command', should be + invoked with exactly one additional argument which should be a + legal shell script. It will be fed to sh -c on the testbed (this + is the way ssh interprets its arguments). - The testbed program's stdin, stdout and stderr will be plumbed through to the stdin, stdout and stderr passed to ; this may involve fd passing, or indirection via pipes or sockets. The diff --git a/runner/adt-run b/runner/adt-run index 4d2ae5b..4b64ce8 100755 --- a/runner/adt-run +++ b/runner/adt-run @@ -48,6 +48,7 @@ errorcode = 0 # exit status that we are going to use timeouts = { 'short':100, 'install':3000, 'test':10000, 'build':100000 } binaries = None # Binaries (.debs we have registered) build_essential = ["build-essential"] +paths = [] #---------- output handling # @@ -217,6 +218,7 @@ class AutoFile: # p.what # p.path[tb] None or path not None => path known # p.file[tb] None or path not None => file exists + # p.ephem[tb] boolean True => destroyed by tb reset # p.spec string or None # p.spec_tbp True or False, or not set if spec is None # p.dir '' or '/' @@ -225,7 +227,9 @@ class AutoFile: p.what = what p.path = [None,None] p.file = [None,None] + p.ephem = [False,False] p.spec = None + p.spec_tbp = None p.dir = '' def __repr__(p): @@ -261,6 +265,7 @@ class AutoFile: if not tbp: p.path[tbp] = tmpdir+'/'+p.what else: + p.ephem[tbp] = True p.path[tbp] = testbed.scratch.path[True]+'/'+p.what def write(p, tbp=False): @@ -299,8 +304,15 @@ class AutoFile: return p.file[tbp] + p.dir def invalidate(p, tbp=False): + if p.path[tbp] is not None: + p._debug('invalidating %s' % 'HT'[tbp]) p.file[tbp] = None - p._debug('invalidated %s' % 'HT'[tbp]) + if p.spec_tbp != tbp or p.spec is None: + p.path[tbp] = None + + def invalidate_ephem(p, tbp=False): + if p.ephem[tbp]: + p.invalidate(tbp) def _debug(p, m): debug('/ %s#%x: %s' % (p.what, id(p), m), 3) @@ -308,6 +320,7 @@ class AutoFile: def _constructed(p): p._debug('constructed: '+str(p)) p._check() + paths.append(p) def _check(p): for tbp in [False,True]: @@ -333,6 +346,7 @@ class AutoFile: if sibling: trim = os.path.dirname else: trim = lambda x: x for tbp in [False,True]: + p.ephem[tbp] = parent.ephem[tbp] if parent.path[tbp] is None: continue trimmed = trim(parent.path[tbp]) if trimmed: trimmed += '/' @@ -373,6 +387,12 @@ class OutputDir(OutputFile): p.dir = '/' p._constructed() +class RelativeInputDir(AutoFile): + def __init__(p, what, parent, leaf, onlyon_tbp=None, sibling=False): + p._relative_init(what, parent, leaf, onlyon_tbp, True, sibling) + p.dir = '/' + p._constructed() + class RelativeInputFile(AutoFile): def __init__(p, what, parent, leaf, onlyon_tbp=None, sibling=False): p._relative_init(what, parent, leaf, onlyon_tbp, True, sibling) @@ -721,7 +741,6 @@ class Testbed: tb.scratch = None tb.modified = False tb.blamed = [] - tb._ephemeral = [] tb._debug('init') tb._need_reset_apt = False def _debug(tb, m, minlevel=0): @@ -735,8 +754,6 @@ class Testbed: stdin=p, stdout=p, stderr=tb._errplumb.stream) tb.expect('ok') tb.caps = tb.commandr('capabilities') - if not 'print-execute-command' in tb.caps: - tb.bomb('testbed does not support print-execute-command') def stop(tb): tb._debug('stop') tb.close() @@ -754,22 +771,22 @@ class Testbed: tb._debug('open, scratch=%s' % tb.scratch) if tb.scratch is not None: return pl = tb.commandr('open') + tb._opened(pl) + def _opened(tb, pl): tb.scratch = InputDir('tb-scratch', pl[0], True) tb.deps_processed = [] + for af in paths: af.invalidate_ephem(True) tb._auxverbscript_make() - def _auxverbscript_make(tb): - pec = tb.commandr('print-execute-command') - if len(pec) < 2: tb.bomb('too few results from print-execute-command') - tb.ec_cmdl = map(urllib.unquote, pec[0].split(',')) - tb.ec_mode = urllib.unquote(pec[1]) - tb.ec_infos = map(urllib.unquote,pec[2:]) - - shellquote_re = regexp.compile(r'([\\"$`])') - def shellquote_arg(s): return '"' + shellquote_re.sub(r'\\\1', s) + '"' + pec = tb.commandr('print-auxverb-command') + if len(pec) < 1: tb.bomb('too few results from print-execute-command') + cmdl = map(urllib.unquote, pec[0].split(',')) + + shellquote_re = regexp.compile('"') + def shellquote_arg(s): return "'" + shellquote_re.sub(r"'\''", s) + "'" def shellquote_cmdl(l): return ' '.join(map(shellquote_arg,l)) - tb._debug('tb.ec_cmdl = %s' % (`tb.ec_cmdl`)) + tb._debug('cmdl = %s' % (`cmdl`)) tb.ec_auxverbscript = TemporaryFile('satdep-auxverb') print >>open(tb.ec_auxverbscript.write(),'w'), ( @@ -777,13 +794,11 @@ class Testbed: set -ex echo >&2 ": $*" if [ $# = 2 ] && [ "x$1" = xdpkg-architecture ] && [ "x$2" = x-qDEB_HOST_ARCH ]; then + # This is a pretty nasty hack. Hopefully it can go away + # eventually. See #635763. set -- dpkg --print-architecture fi -if [ "x$1" = xsh ] && [ "x$2" = x-c ]; then - shift; shift - # what a horrible hack! -fi -exec '''+shellquote_cmdl(tb.ec_cmdl)+' "$*"'+"\n" +exec '''+shellquote_cmdl(cmdl)+' "$@"'+"\n" ) os.chmod(tb.ec_auxverbscript.write(), 0755) @@ -810,12 +825,10 @@ exec '''+shellquote_cmdl(tb.ec_cmdl)+' "$*"'+"\n" (tb.modified, tb.deps_processed, deps_new), 1) if 'revert' in tb.caps and (tb.modified or [d for d in tb.deps_processed if d not in deps_new]): - for af in tb._ephemeral: af.read(False) + for af in paths: af.read(False) tb._debug('reset **') - tb.command('revert') - tb.blamed = [] - for af in tb._ephemeral: af.invalidate(True) - tb._auxverbscript_make() + pl = tb.commandr('revert') + tb._opened(pl) tb.modified = False def prepare2(tb, deps_new): tb._debug('prepare2, deps_new=%s' % deps_new, 1) @@ -824,8 +837,6 @@ exec '''+shellquote_cmdl(tb.ec_cmdl)+' "$*"'+"\n" def prepare(tb, deps_new): tb.prepare1(deps_new) tb.prepare2(deps_new) - def register_ephemeral(tb, af): - tb._ephemeral.append(af) def _install_deps(tb, deps_new): tb._debug(' installing dependencies '+`deps_new`, 1) tb.deps_processed = deps_new @@ -877,7 +888,7 @@ exec '''+shellquote_cmdl(tb.ec_cmdl)+' "$*"'+"\n" if nresults is not None and len(ll) != nresults: tb.bomb("sent `%s', got `%s' (%d result parameters)," " expected %d result parameters" % - (string, l, len(ll), nresults)) + (tb.lastsend, l, len(ll), nresults)) return ll def commandr(tb, cmd, args=(), nresults=None, unquote=True): # pass args=[None,...] or =(None,...) to avoid more url quoting @@ -1312,7 +1323,7 @@ def determine_package(act): if not act.pkg: badpkg('no good Package: line in control file') class Binaries: - def __init__(b): + def __init__(b, tb): b.dir = TemporaryDir('binaries') b.dir.write() ok = False @@ -1685,12 +1696,9 @@ def build_source(act, control_override): built = True - act.tests_tree = InputDir(what+'-tests-tree', - work.read(True)+os.path.basename(result_pwd), + act.tests_tree = RelativeInputDir(what+'-tests-tree', + work, os.path.basename(result_pwd), True) - if act.ah['dsc_tests']: - testbed.register_ephemeral(act.work) - testbed.register_ephemeral(act.tests_tree) if not built: act.blamed = [] @@ -1744,11 +1752,7 @@ def process_actions(): debug_a1('starting') testbed.open() - binaries = Binaries() - - for act in opts.actions: - if act.af is not None and not act.af.spec_tbp: - testbed.register_ephemeral(act.af) + binaries = Binaries(testbed) binaries.reset() control_override = None diff --git a/virt-subproc/VirtSubproc.py b/virt-subproc/VirtSubproc.py index 8839fd0..f1f735d 100644 --- a/virt-subproc/VirtSubproc.py +++ b/virt-subproc/VirtSubproc.py @@ -29,6 +29,7 @@ import urllib import signal import subprocess import traceback +import errno import re as regexp debuglevel = None @@ -65,8 +66,7 @@ def cmdnumargs(c, ce, nargs=0, noptargs=0): def cmd_capabilities(c, ce): cmdnumargs(c, ce) - return caller.hook_capabilities() + ['execute-debug', - 'print-execute-command'] + return caller.hook_capabilities() + ['execute-debug'] def cmd_quit(c, ce): cmdnumargs(c, ce) @@ -77,15 +77,17 @@ def cmd_close(c, ce): if not downtmp: bomb("`close' when not open") cleanup() -def cmd_print_execute_command(c, ce): +def cmd_print_auxverb_command(c, ce): return print_command('auxverb', c, ce) +def cmd_print_shstring_command(c, ce): return print_command('shstring', c, ce) + +def print_command(which, c, ce): + global downs cmdnumargs(c, ce) - if not downtmp: bomb("`print-execute-command' when not open") - if hasattr(caller,'hook_callerexeccmd'): - (cl,kvl) = caller.hook_callerexeccmd() - else: - cl = down - kvl = ['shstring'] - return [','.join(map(urllib.quote, cl))] + kvl + if not downtmp: bomb("`print-%s-command' when not open" % which) + cl = downs[which] + if not len(cl): + cl = ['sh','-c','exec "$@"','x'] + cl + return [','.join(map(urllib.quote, cl))] def preexecfn(): caller.hook_forked_inchild() @@ -105,14 +107,14 @@ def execute_raw(what, instr, timeout, *popenargs, **popenargsk): def execute(cmd_string, cmd_list=[], downp=False, outp=False, timeout=0): cmdl = cmd_string.split() - if downp: perhaps_down = down + if downp: perhaps_down = downs['auxverb'] else: perhaps_down = [] if outp: stdout = subprocess.PIPE else: stdout = None cmd = cmdl + cmd_list - if len(perhaps_down): cmd = perhaps_down + [' '.join(cmd)] + if len(perhaps_down): cmd = perhaps_down + cmd (status, out) = execute_raw(cmdl[0], None, timeout, cmd, stdout=stdout) @@ -127,18 +129,57 @@ def cmd_open(c, ce): global downtmp cmdnumargs(c, ce) if downtmp: bomb("`open' when already open") - downtmp = caller.hook_open() - if downtmp is None: - downtmp = execute('mktemp -t -d', downp=True, outp=True) - debug("down = %s, downtmp = %s" % (string.join(down), downtmp)) + caller.hook_open() + opened1() + downtmp = caller.hook_downtmp() + return opened2() + +def downtmp_mktemp(): + global downtmp + return execute('mktemp -t -d', downp=True, outp=True) + +def downtmp_remove(): + global downtmp + execute('rm -rf --', [downtmp], downp=True) + +perl_quote_re = regexp.compile('[^-+=_.,;:() 0-9a-zA-Z]') +def perl_quote_1chargroup(m): return '\\x%02x' % ord(m.group(0)) +def perl_quote(s): return '"'+perl_quote_re.sub(perl_quote_1chargroup, s)+'"' + +def opened1(): + global down, downkind, downs + debug("downkind = %s, down = %s" % (downkind, `down`)) + if downkind == 'auxverb': + downs = { 'auxverb': down, + 'shstring': down + ['sh','-c'] } + elif downkind == 'shstring': + downs = { 'shstring': down, + 'auxverb': ['perl','-e',''' + @cmd=('''+(','.join(map(perl_quote,down)))+'''); + my $shstring = pop @ARGV; + s/'/'\\\\''/g foreach @ARGV; + push @cmd, "'$_'" foreach @ARGV; + my $argv0=$cmd[0]; + exec $argv0 @cmd; + die "$argv0: $!"; + '''] } + debug("downs = %s" % `downs`) + +def opened2(): + global downtmp, downs + debug("downtmp = %s" % (downtmp)) return [downtmp] def cmd_revert(c, ce): + global downtmp cmdnumargs(c, ce) if not downtmp: bomb("`revert' when not open") if not 'revert' in caller.hook_capabilities(): bomb("`revert' when `revert' not advertised") caller.hook_revert() + opened1() + downtmp = caller.hook_downtmp() + return opened2() def down_python_script(gobody, functions=''): # Many things are made much harder by the inability of @@ -148,6 +189,7 @@ def down_python_script(gobody, functions=''): script = ( "import urllib\n" "import os\n" + "import errno\n" "def setfd(fd,fnamee,write,mode=0666):\n" " fname = urllib.unquote(fnamee)\n" " if write: rw = os.O_WRONLY|os.O_CREAT|os.O_TRUNC\n" @@ -165,8 +207,9 @@ def down_python_script(gobody, functions=''): debug("+P ...\n"+script) scripte = urllib.quote(script) - cmdl = down + ["exec python -c 'import urllib; s = urllib.unquote(%s);" - " exec s'" % ('"%s"' % scripte)] + cmdl = (downs['shstring'] + + ["exec python -c 'import urllib; s = urllib.unquote(%s);" + " exec s'" % ('"%s"' % scripte)]) return cmdl def cmd_execute(c, ce): @@ -352,8 +395,6 @@ def cleanup(): sethandlers(signal.SIG_DFL) cleaning = True if downtmp: - if not 'revert' in caller.hook_capabilities(): - execute('rm -rf --', [downtmp], downp=True) caller.hook_cleanup() cleaning = False downtmp = False diff --git a/virt-subproc/adt-virt-chroot b/virt-subproc/adt-virt-chroot index 5d5f56f..b722153 100755 --- a/virt-subproc/adt-virt-chroot +++ b/virt-subproc/adt-virt-chroot @@ -52,11 +52,24 @@ def parse_args(): chroot_arg = args[0] if not chroot_arg: pe("chroot specification may not be empty") - if chroot_arg == '=': down = ['dchroot','-q'] - elif chroot_arg == '=': down = ['dchroot','-q'] - elif chroot_arg[0] == '=': down = ['dchroot','-q','-c',chroot_arg[1:]] - elif chroot_arg[0] == '/': down = ['chroot',chroot_arg,'--'] - else: pe("chroot spec must be =[DCHROOT] or /PATH/TO/CHROOT") + if chroot_arg == '=': + down = ['dchroot','-q','--'] + elif chroot_arg == '=': + down = ['dchroot','-q','--'] + elif chroot_arg[0] == '=': + down = ['dchroot','-q','-c',chroot_arg[1:],'--'] + elif chroot_arg[0] == '/': + down = ['chroot',chroot_arg] + else: + pe("chroot spec must be =[DCHROOT] or /PATH/TO/CHROOT") + + if down[0] == 'chroot': + vsp.downkind = 'auxverb' + elif down[0] == 'dchroot': + vsp.downkind = 'shstring' + else: + print >>sys.stderr, 'down[0]=%s' % `down[0]` + assert(False) if opts.gain_root != None: down = opts.gain_root.split() + down @@ -67,12 +80,13 @@ def parse_args(): vsp.down = down def hook_open(): - global downtmp - vsp.execute('true', downp=True) - return None + pass + +def hook_downtmp(): + return vsp.downtmp_mktemp() def hook_cleanup(): - pass + vsp.downtmp_remove() def hook_forked_inchild(): pass diff --git a/virt-subproc/adt-virt-null b/virt-subproc/adt-virt-null index f515286..10b6b86 100755 --- a/virt-subproc/adt-virt-null +++ b/virt-subproc/adt-virt-null @@ -52,16 +52,17 @@ def parse_args(): if os.getuid()==0: capabilities.append('root-on-testbed') - vsp.down = ['sh','-ec'] + vsp.down = [] + vsp.downkind = 'auxverb' def hook_open(): - global downtmp - vsp.execute('true', downp=True) - downtmp = vsp.execute('mktemp -t -d', downp=True, outp=True) - return downtmp + pass + +def hook_downtmp(): + return vsp.downtmp_mktemp() def hook_cleanup(): - vsp.execute('rm -rf --', [downtmp], downp=True) + vsp.downtmp_remove() def hook_forked_inchild(): pass diff --git a/virt-subproc/adt-virt-schroot b/virt-subproc/adt-virt-schroot index afcd68a..cb7477d 100755 --- a/virt-subproc/adt-virt-schroot +++ b/virt-subproc/adt-virt-schroot @@ -67,11 +67,11 @@ def parse_args(): capabilities.append('revert') if [True - for exp_name in cfg['root-users'].split() + for exp_name in cfg['root-users'].split(',') for got_uid in [os.getuid()] if got_uid == pwd.getpwnam(exp_name).pw_uid ] or [True - for exp_name in cfg['root-groups'].split() + for exp_name in cfg['root-groups'].split(',') for got_gid in [os.getgid()] + os.getgroups() if got_gid == grp.getgrnam(exp_name).gr_gid ]: @@ -82,15 +82,20 @@ def hook_open(): sessid = vsp.execute('schroot -b -c',[schroot], downp=False, outp=True) vsp.down = ['schroot','-r','-c',sessid] if 'root-on-testbed' in capabilities: vsp.down += ['-u','root'] - vsp.down += ['--','sh','-c'] - return None + vsp.down += ['--'] + vsp.downkind = 'auxverb' + +def hook_downtmp(): + return vsp.downtmp_mktemp() def hook_revert(): + vsp.downtmp_remove() hook_cleanup() hook_open() def hook_cleanup(): global schroot, sessid, downtmp + vsp.downtmp_remove() vsp.execute('schroot -e -c',[sessid], downp=False) def hook_forked_inchild(): diff --git a/virt-subproc/adt-virt-xenlvm b/virt-subproc/adt-virt-xenlvm index 08608da..e82a35b 100755 --- a/virt-subproc/adt-virt-xenlvm +++ b/virt-subproc/adt-virt-xenlvm @@ -90,6 +90,7 @@ def parse_args(): ['--','sh','-ec','echo y; exec cat']) vsp.down = (gain_root + ['adt-xenlvm-on-testbed'] + xargs_direct + xlargs + ['--']) + vsp.downkind = 'shstring' else: if opts.gain_root: pe('--userv and --gain-root are not compatible') @@ -105,6 +106,7 @@ def parse_args(): ' and subcommand details (code=%d)' % get_down.returncode) vsp.down = pon0.split('\0') + vsp.downkind = 'auxverb' pauses = opts.pause.split(',') def do_open(): @@ -123,7 +125,6 @@ def do_open(): if l != "y\n": vsp.bomb("with-testbed sh gave wrong output `%s', not `l'" % l.rstrip("\n")) - vsp.execute('mkdir %s' % downtmp, downp=True) def do_close(): global withholder @@ -142,13 +143,16 @@ def hook_forked_inchild(): def hook_open(): hook_cleanup() - do_open() + return do_open() + +def hook_downtmp(): + vsp.execute('mkdir %s' % downtmp, downp=True) return downtmp def hook_revert(): check_pause('revert') do_close() - do_open() + return do_open() def hook_cleanup(): check_pause('cleanup') -- 2.30.2