From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH][KVM_AUTOTEST] Added functionality to the preprocessor to run scripts Date: Wed, 03 Jun 2009 21:17:09 +0300 Message-ID: <4A26BE25.1060509@redhat.com> References: <597666885.1130601244052338415.JavaMail.root@zmail05.collab.prod.int.phx2.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, David Huff To: Michael Goldish Return-path: Received: from mx2.redhat.com ([66.187.237.31]:39582 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751536AbZFCSRG (ORCPT ); Wed, 3 Jun 2009 14:17:06 -0400 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n53IH3wB003384 for ; Wed, 3 Jun 2009 14:17:05 -0400 In-Reply-To: <597666885.1130601244052338415.JavaMail.root@zmail05.collab.prod.int.phx2.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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. 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. 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'. You can use rsync to implement 'copy unless exists'. >> >> 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. >> 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! 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. -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.