From patchwork Tue Nov 18 16:16:28 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Omer Zilberberg X-Patchwork-Id: 5331441 Return-Path: X-Original-To: patchwork-fstests@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 48C069F2ED for ; Tue, 18 Nov 2014 16:16:37 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 4268D2012B for ; Tue, 18 Nov 2014 16:16:36 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 0567820127 for ; Tue, 18 Nov 2014 16:16:35 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754607AbaKRQQe (ORCPT ); Tue, 18 Nov 2014 11:16:34 -0500 Received: from mail-wg0-f51.google.com ([74.125.82.51]:61218 "EHLO mail-wg0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754591AbaKRQQd (ORCPT ); Tue, 18 Nov 2014 11:16:33 -0500 Received: by mail-wg0-f51.google.com with SMTP id k14so8819618wgh.24 for ; Tue, 18 Nov 2014 08:16:32 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :cc:subject:references:in-reply-to:content-type :content-transfer-encoding; bh=lP75qSgsu2kH/012CCEVQWropzD/1iUIYwATn4bvDVI=; b=eAMAj9jGRbTgcgKpz7aJWNiocJ3VO3jtJ6atR4MaC4UP1nu9qMrpf1xHrIf9qbFUN/ BF/Ee5k+MLEiz7IE0XeiqMIteQ2hvm+IIIM45+XUeKdoNN7eT4NTmidDWz1FMBMLLsJU ov5Hg9hPUE0SoL0QKhi4YdPURaku+okerIR75dQNuZK7aTWKcRvqTLZP4759H6pyAdGf t7DrTPuzKzAmENHvE5mIGCstSNDJwjQ2rmylBXC+fqdbxQNhHWpdZQ+OhDWgfnBex0lg qBa8cZlIjJD12t3mVmU21h7EKmmgN0HIChLOjSx6QtGIgqX4uxcCNc9s1YvhWJq+egNa hbbw== X-Gm-Message-State: ALoCoQkdCRYBRXar9BIwIYxBmJP7ZccvgakPjljpiT1Q4A+8Mc+4VOFH5Ucwhh/XNSuPaIVXv5tE X-Received: by 10.180.9.196 with SMTP id c4mr41248771wib.66.1416327392046; Tue, 18 Nov 2014 08:16:32 -0800 (PST) Received: from [10.0.0.12] ([207.232.55.62]) by mx.google.com with ESMTPSA id pf4sm45324122wjb.36.2014.11.18.08.16.30 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 18 Nov 2014 08:16:31 -0800 (PST) Message-ID: <546B70DC.2060502@plexistor.com> Date: Tue, 18 Nov 2014 18:16:28 +0200 From: Omer Zilberberg User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Dave Chinner CC: Eryu Guan , Steve French , fstests@vger.kernel.org, "linux-nfs@vger.kernel.org" , "linux-cifs@vger.kernel.org" , Weston Andros Adamson Subject: Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS References: <1414775040-4051-1-git-send-email-eguan@redhat.com> <1414775040-4051-2-git-send-email-eguan@redhat.com> <20141113033317.GC28565@dastard> <20141115053533.GK2863@dhcp-13-216.nay.redhat.com> <20141117054141.GK23575@dastard> <20141117060603.GO2863@dhcp-13-216.nay.redhat.com> <20141117065443.GL23575@dastard> <546A0BFE.6080608@plexistor.com> <20141117202207.GN23575@dastard> In-Reply-To: <20141117202207.GN23575@dastard> Sender: fstests-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: fstests@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 11/17/2014 10:22 PM, Dave Chinner wrote: > On Mon, Nov 17, 2014 at 04:53:50PM +0200, Omer Zilberberg wrote: >>> Another FYI: we can actually test any filesystem without a scratch >>> device configured: >>> >>> $ sudo TEST_DEV=/dev/vda TEST_DIR=/mnt/test ./check generic/120 >>> FSTYP -- xfs (non-debug) >>> PLATFORM -- Linux/x86_64 test2 3.18.0-rc2-dgc+ >>> >>> generic/120 16s ... [not run] this test requires a valid $SCRATCH_DEV >>> Not run: generic/120 >>> Passed all 0 tests >>> $ >> >> Please note that since commit 83ef157d, that is no longer true, because _require_test calls _is_block_dev with empty parameter, which prints usage: >> $ sudo TEST_DEV=/dev/sda TEST_DIR=/mnt/dev0 ./check generic/001 >> FSTYP -- ext4 >> PLATFORM -- Linux/x86_64 testvm 3.17.0 >> >> generic/001 7s ... - output mismatch (see /opt/xfstests/results//generic/001.out.bad) >> --- tests/generic/001.out 2014-09-10 11:04:44.249185592 +0300 >> +++ /opt/xfstests/results//generic/001.out.bad 2014-11-17 15:14:12.380061760 +0200 >> @@ -1,4 +1,5 @@ >> QA output created by 001 >> +Usage: _is_block_dev dev >> cleanup >> setup .................................... >> iter 1 chain ... check .................................... >> ... >> (Run 'diff -u tests/generic/001.out /opt/xfstests/results//generic/001.out.bad' to see the entire diff) >> Ran: generic/001 >> Failures: generic/001 >> Failed 1 of 1 tests >> >> Here's a patch to fix that: >> ---- >> Subject: [PATCH] _required_test: removed unneeded test for scratch_dev >> >> testing for scratch_dev in _required_test is unnecessary, since it is >> not required for these tests. Furthermore, if a scratch_dev is not given, >> all tests which are supposed to pass on test_dev fail, because >> _is_block_dev is given an empty string as scratch_dev, and prints usage. > > I'm beginning to sound like a broken record. Why isn't this *invalid > config check* being done in the config parsing rather than on every > test that requires a scratch or test device? > > i.e. removing the check is not the correct fix - checking it during > config parsing means none of these checks need to be done in > _require_test, nor in _require_scratch_nocheck. > > i.e. in get_next_config() we already know the FSTYP, so we should > be checking that: > > a) TEST_DEV is valid for fstyp > b) SCRATCH_DEV (if configured) is valid for fstyp > c) TEST_DEV != SCRATCH_DEV if they both exist and fstyp > indicates they should be block devices > > And then we can pretty much assume that they are valid everywhere > else, and _require_scratch_nocheck() simply needs to check for a > non-empty string... > >> Signed-off-by: Omer Zilberberg >> --- >> common/rc | 6 +----- >> 1 file changed, 1 insertion(+), 5 deletions(-) >> >> diff --git a/common/rc b/common/rc >> index d5e3aff..b863808 100644 >> --- a/common/rc >> +++ b/common/rc >> @@ -1104,7 +1104,7 @@ _require_scratch() >> } >> >> >> -# this test needs a test partition - check we're ok & unmount it >> +# this test needs a test partition - check we're ok & mount if necessary >> # >> _require_test() >> { >> @@ -1138,10 +1138,6 @@ _require_test() >> then >> _notrun "this test requires a valid \$TEST_DEV" >> fi >> - if [ "`_is_block_dev $SCRATCH_DEV`" = "`_is_block_dev $TEST_DEV`" ] >> - then >> - _notrun "this test requires a valid \$TEST_DEV" >> - fi >> if [ ! -d "$TEST_DIR" ] >> then >> _notrun "this test requires a valid \$TEST_DIR" > > The patch is whitespace damaged. Please read: > > https://www.kernel.org/doc/Documentation/email-clients.txt Here's the patch resent with whitespace fixed, at least as a temporary solution... ---- Subject: [PATCH] _required_test: removed unneeded test for scratch_dev testing for scratch_dev in _required_test is unnecessary, since it is not required for these tests. Furthermore, if a scratch_dev is not given, all tests which are supposed to pass on test_dev fail, because _is_block_dev is given an empty string as scratch_dev, and prints usage. Signed-off-by: Omer Zilberberg --- common/rc | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/common/rc b/common/rc index d5e3aff..b863808 100644 --- a/common/rc +++ b/common/rc @@ -1104,7 +1104,7 @@ _require_scratch() } -# this test needs a test partition - check we're ok & unmount it +# this test needs a test partition - check we're ok & mount if necessary # _require_test() { @@ -1138,10 +1138,6 @@ _require_test() then _notrun "this test requires a valid \$TEST_DEV" fi - if [ "`_is_block_dev $SCRATCH_DEV`" = "`_is_block_dev $TEST_DEV`" ] - then - _notrun "this test requires a valid \$TEST_DEV" - fi if [ ! -d "$TEST_DIR" ] then _notrun "this test requires a valid \$TEST_DIR"