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 09:45:43 +0300 Message-ID: <4A261C17.7080900@redhat.com> References: <511807252.1061881244009668219.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]:44611 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751553AbZFCGpo (ORCPT ); Wed, 3 Jun 2009 02:45:44 -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 n536jkkt021627 for ; Wed, 3 Jun 2009 02:45:46 -0400 In-Reply-To: <511807252.1061881244009668219.JavaMail.root@zmail05.collab.prod.int.phx2.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: Michael Goldish wrote: > ----- "Avi Kivity" 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. > - 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? > - 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. > - 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. >> 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. -- Do not meddle in the internals of kernels, for they are subtle and quick to panic.