----- "Avi Kivity" <avi@xxxxxxxxxx> wrote:
> Michael Goldish wrote:
> >>>>
> >>>>
> >>> - 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.
> >
>
> Then create a custom exception object and put it in a field.
>
> >
> >>> - 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.
> >
>
> I think it's very rare to want to let the test continue even if some
> command fails.
>
> Can you give examples?
Some commands are not critical, like one that converts the screendumps
from PPM to PNG format. We don't want to fail the test if you don't have
ImageMagick installed.
A similar function used for remote commands is sometimes used for checking
if a file exists on the guest and getting its size at the same time,
using ls. If the file doesn't exist we should SCP it into the guest, not
fail the test.
> >>> 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?
> >
>
> try:
> code
> code
> code
> except e:
> raise ChainedException('exception while running migration test',
> e)
>
> Instead of checking each code line, you provide a wrapper for the
> entire test.
It's not informative to add it for the entire test, because we already know
what test failed -- we don't need the exception string for that.
Instead, we'd have to wrap small sections of the test separately and provide
a different exception string for each. The migration test may be split to
four sections: setup, pre-migration, the migration itself and post-migration.
This may not be so bad, but do you still think it's a good solution?
There's also another issue: the utility functions that should raise exceptions
don't know what they're being used for, so there can be exception string
collisions. For example, both run_bg(), used for local processes, and
kvm_spawn.get_command_status_output(), used for SSH commands, would say
something like "command %s failed". It would be incorrect to hardcode a more
specific message into any of them (in my opinion).
> >>>> 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.
> >
>
> The caller has to test for three possible outcomes. Success, failure,
> and 'command is running in the background'. If your callsites don't
> check for all three, something's wrong.
When we want the command to succeed on time, we just test for status == 0.
If it's a nonzero integer or None, something went wrong, and we should kill
the process just in case it's still running.
> --
> 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
