----- "Avi Kivity" <avi@xxxxxxxxxx> wrote:
> Michael Goldish wrote:
> > ----- "Avi Kivity" <avi@xxxxxxxxxx> wrote:
> >
> >
> >> David Huff wrote:
> >>
> >>> This patch will run pre and post scripts
> >>> defined in config file with the parameter pre_command
> >>> and post_command post_command.
> >>>
> >>> Also exports all the prameters in preprocess for passing
> >>> arguments to the script.
> >>>
> >>> + #execute any pre_commands
> >>> + pre_command = params.get("pre_command")
> >>> + if pre_command:
> >>> + # export environment vars
> >>> + for k in params.keys():
> >>> + kvm_log.info("Adding KVM_TEST_%s to Environment" %
> >>>
> >> (k))
> >>
> >>> + os.putenv("KVM_TEST_%s" % (k), str(params[k]))
> >>> + # execute command
> >>> + kvm_log.info("Executing command '%s'..." % pre_command)
> >>> + timeout = int(params.get("pre_commmand_timeout", "600"))
> >>> + (status, pid, output) = kvm_utils.run_bg("cd %s; %s" %
> >>>
> >> (test.bindir, pre_command),
> >>
> >>> + None,
> >>>
> >> kvm_log.debug, "(pre_command) ", timeout=timeout)
> >>
> >>> + if status != 0:
> >>> + kvm_utils.safe_kill(pid, signal.SIGTERM)
> >>> + raise error.TestError, "Custom processing
> pre_command
> >>>
> >> failed"
> >>
> >>>
> >>>
> >> kvm_utils.run_bg should throw an exception instead of returning
> >> status.
> >>
> >
> > - What if we're interested in the status for some reason? Its value
> > may indicate what went wrong with the child process.
> >
> Put it in the exception string.
But I want its value to be examined programmatically in the code.
It's less comfortable to retrieve it from a string.
> > - If we throw an exception we should add a parameter that controls
> > whether an exception should be thrown (something like
> "ignore_status")
> > because in some cases we don't want to throw an exception.
> >
>
> I'll complain every time I see it. If you don't care if the command
> succeeds or not, why run it in the first place?
I care, but sometimes I don't want to fail the test when a command fails.
I can put every run_bg() call in a try-except statement, but that misses the
point of raising exceptions in the first place.
> > - A run_bg call has 3 possible outcomes: success (status == 0),
> failure
> > (status != 0) and timeout (process still running). If we throw an
> exception
> > upon failure, what do we do upon timeout? Sometimes it's good (qemu
> should
> > keep running unless something went wrong) and sometimes bad
> (pre_command
> > should probably not time out). So we end up needing an
> ignore_timeout
> > parameter as well.
> >
>
> A run_bg() call should return an object with a .join() method, or
> throw
> an exception. At some point you must call the join() method, which
> throws an exception or returns None.
OK, I have queued patches to make run_bg() return such an object anyway,
except for the exceptions part.
> > - What if we want the test to fail with an informative
> test-specific
> > exception such as "something failed after migration"?
> >
>
> Chained exceptions can provide detailed information.
Wouldn't it complicate the test code?
How can I provide a detailed message such as "test command failed after migration" --
can you illustrate this in a small example?
> >> But if status != 0, will there actually be a pid to kill?
> >>
> >
> > If the timeout expires and the process is still running, status is
> None.
> >
>
> functions which can return with three possible outcomes are difficult
> to use.
I tend to see it as two possible outcomes: completion or timeout. In the former
case, status is returned. In the latter, None is returned.
> --
> Do not meddle in the internals of kernels, for they are subtle and
> quick to panic.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
