----- "Avi Kivity" <avi@xxxxxxxxxx> wrote:
> Michael Goldish wrote:
> >> 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.
> >
>
> I think you don't want to run the test at all in that case.
I do -- the conversion is just intended to save some disk space after the
test completes.
> But if you do, you shouldn't invoke ImageMagick if you don't have it installed.
>
> Otherwise you can't distinguish between failure due to a corrupted
> source image, out of disk space, or ImageMagick not installed.
You can, if you look at the output and/or status that run_bg() returns (e.g.
"command not found" and 127). But you don't need to because you shouldn't
do anything special if the command fails, except inform the user that it
failed. This command is really not critical (we're currently not using it
anyway -- it's just planned for the near future).
> Ignoring errors is usually a bug.
>
> > 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.
> >
>
> Again it doesn't distinguish between real errors and 'file does not
> exist'. Use something like 'if test -f file; then ls -l file; else
> echo
> missing; fi'.
I don't see what's wrong with 'ls filename'. It does exactly what the line
you suggested does -- if the file exists it displays some info, and if it
doesn't it says "No such file ...".
In the test we explicitly check for the "No such file" message, so it's
being handled properly.
> You can use rsync to implement 'copy unless exists'.
The guest may not like rsync. We may have to work with FTP in some cases
(Windows).
> >>
> >> 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?
> >
>
> You can have a 'stage' variable and assign it your current stage, and
> use it when reporting errors.
>
> Anything is better than a maze of ifs.
>
> > 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).
> >
> >
>
> The message should explain what happened, no more, no less.
"What happened" depends on the point of view.
It doesn't really help the user to print something like
"read_until_output_matches: Timeout elapsed"
instead of
"Could not log into guest after migration".
It would also not be useful to see
"get_command_status_output: command 'tar xfj autotest.tar.bz2' failed with status 1"
without knowing if it's a remote or a local command (the function itself doesn't know).
> >> 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.
> >
>
> But if it's nonzero it has exited already!
That's why we use safe_kill(), which kills only if the process is still running.
> However, I feel I'm not making any progress, so I'll stop. I just
> wish I could use exceptions in the code I write.
We are making progress: I'll try to think of a simple way to start working
with exceptions without breaking too many things. In the worst case, we can
just keep two sets of functions: low-level ones that don't raise anything,
and high level ones that raise an exception whenever anything seems to have
gone wrong.
I'm sorry for being a bit too stubborn. I prefer to understand things
before going ahead and implementing them, and I still feel that some of my
questions remain unanswered.
>
> --
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.
--
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
