4:04 a.m.
Thanks for your remarks, they were really valuable. Comments below.
----- "Will Woods" <wwoods(a)redhat.com> wrote:
On Mon, 2010-07-12 at 09:18 -0400, Kamil Paral wrote:
I like what this patch is doing overall; a couple comments below.
> +def get_aq_vars(controlfile, extradata):
> + '''Extract AutoQA specific test variables from control file
and
> + return them in a dictionary. All these variables start with
prefix 'aq_'.
> + Arguments:
> + * controlfile - control file
> + * extradata - dictionary with extra variables received from
hook options
> + Returns: dictionary with AutoQA specific test variables
> + '''
> + lines = [line for line in open(controlfile).readlines() if
line.startswith('aq_')]
> + vars = extradata.copy()
> + for line in lines:
> + try:
> + exec line in vars
Interesting - rather than actually interpreting the entire control
file,
you're executing *only* those lines that start with 'aq_'.
Which means:
1) that variable needs to be *literal* - this wouldn't work:
release_ver = figure_out_release_ver()
aq_labels = [release_ver]
Nor this:
if rawhide:
aq_labels=['rawhide']
else:
aq_labels=['f13']
Ah, you got me, I didn't imagine that. I wanted to evaluate the whole
control file but that would mean I also run "job.run_test(...)" line,
which I don't want to. So I just parsed out aq_* variables.
Alright, possible solutions:
1. Make separate file for AutoQA variables, like control.autoqa. We
can then easily evaluate the whole file. But it's another file for
every test... I don't like many files :)
2. We can find "job.runtest(...)" line in control file, cut it out/
comment it out, and evaluate the rest of the file. That's easy to do,
on the other hand maybe some more complex stuff may be present in
the control file in the future and some problems may arise.
Which approach is better? Personally I would do the second one. We
can always redesign it in the future. Or do you have some better idea?
2) Any test can execute any python code inside the 'autoqa' script -
which usually runs as root. This makes me a little nervous, although
I'm
not sure what user the control file normally runs as, and we're
making
no guarantees about security anyway.
I understand it. Currently only autoqa script and watchers script run
on autotest server machine. Now also parts of tests will run there.
But there are these two points:
1. I think we will always review any test that would be accepted and
deployed to our production servers. It's very easy to see any mischief
in the control file. I don't think it is probable some malicious code
could be put in there and we haven't seen it.
2. I have the feeling we can't do without it. We need to use autotest
labels, right? They must be defined per test, not per hook. Also we
need a dynamic definition ("because test XX runs on YY.fc13 package,
it needs 'fc13' autotest label" - you can hardly define this statically).
Dynamic definition means some scripting language to provide the logic,
you can't get by with just a plain text file or smth. Well and that
means some Python script in the end.
To be correct, in the aforementioned example we could get by with
a static mapping of ENVR suffixes -> autotest labels. But I expect
some more complex needs will arise in the future.
So, it is my impression we can't really do much about it, we need
it. And I don't really see it as a big security problem.
So anyway, I'm not sure how I feel about this. At the very least it
seems confusing to have lines in the control file that get
*partially*
evaluated by autoqa, although not as you might expect.
How does autotest evaluate the control files as a whole?
Yes, I believe autotest executes the whole control file.
> + except:
> + print "Evaluating of AutoQA variables from control file
failed:"
> + print " Line: %s" % line.strip()
> + print " Control file: %s" % controlfile
> + return {}
> + # let's leave only aq_* keys present
> + for key in vars.copy():
Why vars.copy() instead of just 'for key in vars:'?
Because you can't delete keys from a collection that you're traversing at
the same time. There might be some... side effects :)
> + if not key.startswith('aq_'):
> + del vars[key]
> + return vars
> +
The rest seems fine - I pushed a commit to clean up some trailing
whitespace.
I should finally define some vim shortcuts to fix this up for me...
Anyway, if we can figure out how to make the partial evaluation of
the
control files a bit saner (or you can help me understand why this is
actually sane and I'm just confused) I'll be happy to merge this into
master.
-w
_______________________________________________
autoqa-devel mailing list
autoqa-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/autoqa-devel