From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ipmail06.adl2.internode.on.net ([150.101.137.129]:20526 "EHLO ipmail06.adl2.internode.on.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754408AbcGUWlU (ORCPT ); Thu, 21 Jul 2016 18:41:20 -0400 Date: Fri, 22 Jul 2016 08:40:57 +1000 From: Dave Chinner Subject: Re: [PATCH 6/6] xfstests: Add mkfs input validation tests Message-ID: <20160721224057.GD12670@dastard> References: <1468500214-6237-1-git-send-email-jtulak@redhat.com> <1468500214-6237-7-git-send-email-jtulak@redhat.com> <20160716093358.GL2432@eguan.usersys.redhat.com> <20160717233003.GX1922@dastard> <20160718114723.GG27776@eguan.usersys.redhat.com> <20160720235433.GA12670@dastard> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Sender: fstests-owner@vger.kernel.org Content-Transfer-Encoding: quoted-printable To: Jan Tulak Cc: Eryu Guan , fstests@vger.kernel.org, xfs-oss List-ID: On Thu, Jul 21, 2016 at 04:24:52PM +0200, Jan Tulak wrote: > On Thu, Jul 21, 2016 at 1:54 AM, Dave Chinner wro= te: > >> >> I think you need to test on a 4k sector size disk. I use scsi_deb= ug to > >> >> simulate physical 4k sector disk to reproduce this: > >> >> > >> >> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe -r scsi_debug > >> >> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe scsi_debug dev_size_m= b=3D128 physblk_exp=3D3 > >> >> [root@dhcp-66-86-11 xfsprogs-dev]# blockdev --getbsz --getpbsz --= getss /dev/sdc > >> >> 4096 > >> >> 4096 > >> >> 512 > >> >> [root@dhcp-66-86-11 xfsprogs-dev]# mkfs -t xfs -l version=3D1 -m = crc=3D1 /dev/sdc > > > > So this is an invalid filesystem configuration. It should be > > detected as such during command line parsing and rejected before we > > get anywhere near checking topology constraints. In mkfs > > terms, it's a conflicting option set. > > > >> And the culprit is in mkfs, some forty lines before the crc & log ve= rsion check: > >> > >> 2026 =E2=87=A5 } else if (lsectorsize > XFS_MIN_SECTORSIZE && = !lsu && !lsunit) { > >> 2027 =E2=87=A5 =E2=87=A5 lsu =3D blocksize; > >> 2028 =E2=87=A5 =E2=87=A5 sb_feat.log_version =3D 2; > >> 2029 =E2=87=A5 } > >> > >> The possible solutions I can think of are: > > > > None of which really appeal because, IMO, they are trying to solve > > the wrong problem. > > > > The whole point of moving to table based command line option parsing > > is that we can encode these sorts of conflicts into the option > > table. The conflict resolution in the option table is currently not > > complete - it can only encode and detect conflicts within a > > suboption type, but not across suboption types (e.g. within -d > > suboptions, but not between -d and -l suboptions). > > > > This is simply because I never got as far as implementing this level > > of conflict encoding/resolution. In essence, the conflict array > > needs to define the sub option type, the suboption that is > > in conflict and the value that it conflicts against. Hence the > > conflicts table can then encode such things as "version 1 logs are > > invalid for CRC enabled filesystems" and vice versa. > > >=20 > Ok, in long term, the correct way is to extend the conflicts table. Not long term. It's fairly simple to do. 1. Convert all the individual subopt parameter tables to an array of tabl= es with a defined index for each set of subopts, 2. add a value field to the parameter, and store the CLI value in it when it is set 3. make the conflicts array in each subopts a structure like: struct conflicts { int subopt; int index; int invalid_value; }; and convert all the existing conflicts to this format 4. Define cross-subopt conflicts like this: .subopt_params =3D { { .index =3D M_CRC, - .conflicts =3D { LAST_CONFLICT }, + .conflicts =3D { { LOG, L_VERSION, 1 }, + { LAST_CONFLICT, 0, 0 }, }, .minval =3D 0, .maxval =3D 1, .defaultval =3D 1, }, And the L_VERSION subopt parameter will have a conflict like + .conflicts =3D { { META, M_CRC, 1 }, + { LAST_CONFLICT, 0, 0 }, }, 5. update the conflict lookup to do cross option lookups via checking the relevant option conflict table. e.g by checking the conflict[i].value against subopt[LOG].subopt_params[L_VERSION].value... > But what in the meantime? Are we going to let it be now until it is > fixed by the enhanced table? IMO: fix it once, fix it properly. > And regarding my question at the end of the mail, I interpret your > answer as "if the arguments are wrong, fail ASAP and don't try to fix > it." The first step in any program shoul dbe to validate user supplied inputs. Once they are validated and known good, you don't have to add random code to handle invalid combinations - you can just assume the inputs are valid to begin with and those corner cases don't need to be handled. Cheers, Dave. --=20 Dave Chinner david@fromorbit.com From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay.sgi.com (relay2.corp.sgi.com [137.38.102.29]) by oss.sgi.com (Postfix) with ESMTP id 857F97CA0 for ; Thu, 21 Jul 2016 17:41:24 -0500 (CDT) Received: from cuda.sgi.com (cuda3.sgi.com [192.48.176.15]) by relay2.corp.sgi.com (Postfix) with ESMTP id 583FF30404E for ; Thu, 21 Jul 2016 15:41:21 -0700 (PDT) Received: from ipmail06.adl2.internode.on.net (ipmail06.adl2.internode.on.net [150.101.137.129]) by cuda.sgi.com with ESMTP id 0Ch6FqtH1oeW9wHK for ; Thu, 21 Jul 2016 15:41:18 -0700 (PDT) Date: Fri, 22 Jul 2016 08:40:57 +1000 From: Dave Chinner Subject: Re: [PATCH 6/6] xfstests: Add mkfs input validation tests Message-ID: <20160721224057.GD12670@dastard> References: <1468500214-6237-1-git-send-email-jtulak@redhat.com> <1468500214-6237-7-git-send-email-jtulak@redhat.com> <20160716093358.GL2432@eguan.usersys.redhat.com> <20160717233003.GX1922@dastard> <20160718114723.GG27776@eguan.usersys.redhat.com> <20160720235433.GA12670@dastard> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: List-Id: XFS Filesystem from SGI List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: xfs-bounces@oss.sgi.com Sender: xfs-bounces@oss.sgi.com To: Jan Tulak Cc: Eryu Guan , fstests@vger.kernel.org, xfs-oss T24gVGh1LCBKdWwgMjEsIDIwMTYgYXQgMDQ6MjQ6NTJQTSArMDIwMCwgSmFuIFR1bGFrIHdyb3Rl Ogo+IE9uIFRodSwgSnVsIDIxLCAyMDE2IGF0IDE6NTQgQU0sIERhdmUgQ2hpbm5lciA8ZGF2aWRA ZnJvbW9yYml0LmNvbT4gd3JvdGU6Cj4gPj4gPj4gSSB0aGluayB5b3UgbmVlZCB0byB0ZXN0IG9u IGEgNGsgc2VjdG9yIHNpemUgZGlzay4gSSB1c2Ugc2NzaV9kZWJ1ZyB0bwo+ID4+ID4+IHNpbXVs YXRlIHBoeXNpY2FsIDRrIHNlY3RvciBkaXNrIHRvIHJlcHJvZHVjZSB0aGlzOgo+ID4+ID4+Cj4g Pj4gPj4gW3Jvb3RAZGhjcC02Ni04Ni0xMSB4ZnNwcm9ncy1kZXZdIyBtb2Rwcm9iZSAtciBzY3Np X2RlYnVnCj4gPj4gPj4gW3Jvb3RAZGhjcC02Ni04Ni0xMSB4ZnNwcm9ncy1kZXZdIyBtb2Rwcm9i ZSBzY3NpX2RlYnVnIGRldl9zaXplX21iPTEyOCBwaHlzYmxrX2V4cD0zCj4gPj4gPj4gW3Jvb3RA ZGhjcC02Ni04Ni0xMSB4ZnNwcm9ncy1kZXZdIyBibG9ja2RldiAtLWdldGJzeiAtLWdldHBic3og LS1nZXRzcyAvZGV2L3NkYwo+ID4+ID4+IDQwOTYKPiA+PiA+PiA0MDk2Cj4gPj4gPj4gNTEyCj4g Pj4gPj4gW3Jvb3RAZGhjcC02Ni04Ni0xMSB4ZnNwcm9ncy1kZXZdIyBta2ZzIC10IHhmcyAtbCB2 ZXJzaW9uPTEgLW0gY3JjPTEgL2Rldi9zZGMKPiA+Cj4gPiBTbyB0aGlzIGlzIGFuIGludmFsaWQg ZmlsZXN5c3RlbSBjb25maWd1cmF0aW9uLiBJdCBzaG91bGQgYmUKPiA+IGRldGVjdGVkIGFzIHN1 Y2ggZHVyaW5nIGNvbW1hbmQgbGluZSBwYXJzaW5nIGFuZCByZWplY3RlZCBiZWZvcmUgd2UKPiA+ IGdldCBhbnl3aGVyZSBuZWFyIGNoZWNraW5nIHRvcG9sb2d5IGNvbnN0cmFpbnRzLiBJbiBta2Zz Cj4gPiB0ZXJtcywgaXQncyBhIGNvbmZsaWN0aW5nIG9wdGlvbiBzZXQuCj4gPgo+ID4+IEFuZCB0 aGUgY3VscHJpdCBpcyBpbiBta2ZzLCBzb21lIGZvcnR5IGxpbmVzIGJlZm9yZSB0aGUgY3JjICYg bG9nIHZlcnNpb24gY2hlY2s6Cj4gPj4KPiA+PiAyMDI2IOKHpSAgICAgICB9IGVsc2UgaWYgKGxz ZWN0b3JzaXplID4gWEZTX01JTl9TRUNUT1JTSVpFICYmICFsc3UgJiYgIWxzdW5pdCkgewo+ID4+ IDIwMjcg4oelICAgICAgIOKHpSAgICAgICBsc3UgPSBibG9ja3NpemU7Cj4gPj4gMjAyOCDih6Ug ICAgICAg4oelICAgICAgIHNiX2ZlYXQubG9nX3ZlcnNpb24gPSAyOwo+ID4+IDIwMjkg4oelICAg ICAgIH0KPiA+Pgo+ID4+IFRoZSBwb3NzaWJsZSBzb2x1dGlvbnMgSSBjYW4gdGhpbmsgb2YgYXJl Ogo+ID4KPiA+IE5vbmUgb2Ygd2hpY2ggcmVhbGx5IGFwcGVhbCBiZWNhdXNlLCBJTU8sIHRoZXkg YXJlIHRyeWluZyB0byBzb2x2ZQo+ID4gdGhlIHdyb25nIHByb2JsZW0uCj4gPgo+ID4gVGhlIHdo b2xlIHBvaW50IG9mIG1vdmluZyB0byB0YWJsZSBiYXNlZCBjb21tYW5kIGxpbmUgb3B0aW9uIHBh cnNpbmcKPiA+IGlzIHRoYXQgd2UgY2FuIGVuY29kZSB0aGVzZSBzb3J0cyBvZiBjb25mbGljdHMg aW50byB0aGUgb3B0aW9uCj4gPiB0YWJsZS4gVGhlIGNvbmZsaWN0IHJlc29sdXRpb24gaW4gdGhl IG9wdGlvbiB0YWJsZSBpcyBjdXJyZW50bHkgbm90Cj4gPiBjb21wbGV0ZSAtIGl0IGNhbiBvbmx5 IGVuY29kZSBhbmQgZGV0ZWN0IGNvbmZsaWN0cyB3aXRoaW4gYQo+ID4gc3Vib3B0aW9uIHR5cGUs IGJ1dCBub3QgYWNyb3NzIHN1Ym9wdGlvbiB0eXBlcyAoZS5nLiB3aXRoaW4gLWQKPiA+IHN1Ym9w dGlvbnMsIGJ1dCBub3QgYmV0d2VlbiAtZCBhbmQgLWwgc3Vib3B0aW9ucykuCj4gPgo+ID4gVGhp cyBpcyBzaW1wbHkgYmVjYXVzZSBJIG5ldmVyIGdvdCBhcyBmYXIgYXMgaW1wbGVtZW50aW5nIHRo aXMgbGV2ZWwKPiA+IG9mIGNvbmZsaWN0IGVuY29kaW5nL3Jlc29sdXRpb24uIEluIGVzc2VuY2Us IHRoZSBjb25mbGljdCBhcnJheQo+ID4gbmVlZHMgdG8gZGVmaW5lIHRoZSBzdWIgb3B0aW9uIHR5 cGUsIHRoZSBzdWJvcHRpb24gdGhhdCBpcwo+ID4gaW4gY29uZmxpY3QgYW5kIHRoZSB2YWx1ZSB0 aGF0IGl0IGNvbmZsaWN0cyBhZ2FpbnN0LiBIZW5jZSB0aGUKPiA+IGNvbmZsaWN0cyB0YWJsZSBj YW4gdGhlbiBlbmNvZGUgc3VjaCB0aGluZ3MgYXMgInZlcnNpb24gMSBsb2dzIGFyZQo+ID4gaW52 YWxpZCBmb3IgQ1JDIGVuYWJsZWQgZmlsZXN5c3RlbXMiIGFuZCB2aWNlIHZlcnNhLgo+ID4KPiAK PiBPaywgaW4gbG9uZyB0ZXJtLCB0aGUgY29ycmVjdCB3YXkgaXMgdG8gZXh0ZW5kIHRoZSBjb25m bGljdHMgdGFibGUuCgpOb3QgbG9uZyB0ZXJtLiBJdCdzIGZhaXJseSBzaW1wbGUgdG8gZG8uCgox LiBDb252ZXJ0IGFsbCB0aGUgaW5kaXZpZHVhbCBzdWJvcHQgcGFyYW1ldGVyIHRhYmxlcyB0byBh biBhcnJheSBvZiB0YWJsZXMKICAgd2l0aCBhIGRlZmluZWQgaW5kZXggZm9yIGVhY2ggc2V0IG9m IHN1Ym9wdHMsCjIuIGFkZCBhIHZhbHVlIGZpZWxkIHRvIHRoZSBwYXJhbWV0ZXIsIGFuZCBzdG9y ZSB0aGUgQ0xJIHZhbHVlIGluCiAgIGl0IHdoZW4gaXQgaXMgc2V0CjMuIG1ha2UgdGhlIGNvbmZs aWN0cyBhcnJheSBpbiBlYWNoIHN1Ym9wdHMgYSBzdHJ1Y3R1cmUgbGlrZToKCnN0cnVjdCBjb25m bGljdHMgewogICAgICAgIGludCAgICAgc3Vib3B0OwogICAgICAgIGludCAgICAgaW5kZXg7CiAg ICAgICAgaW50ICAgICBpbnZhbGlkX3ZhbHVlOwp9OwoKYW5kIGNvbnZlcnQgYWxsIHRoZSBleGlz dGluZyBjb25mbGljdHMgdG8gdGhpcyBmb3JtYXQKCjQuIERlZmluZSBjcm9zcy1zdWJvcHQgY29u ZmxpY3RzIGxpa2UgdGhpczoKCiAgICAgICAuc3Vib3B0X3BhcmFtcyA9IHsKICAgICAgICAgICAg ICAgIHsgLmluZGV4ID0gTV9DUkMsCi0gICAgICAgICAgICAgICAgIC5jb25mbGljdHMgPSB7IExB U1RfQ09ORkxJQ1QgfSwKKwkJICAuY29uZmxpY3RzID0geyB7IExPRywgTF9WRVJTSU9OLCAxIH0s CisJCQkJIHsgTEFTVF9DT05GTElDVCwgMCwgMCB9LCB9LAogICAgICAgICAgICAgICAgICAubWlu dmFsID0gMCwKICAgICAgICAgICAgICAgICAgLm1heHZhbCA9IDEsCiAgICAgICAgICAgICAgICAg IC5kZWZhdWx0dmFsID0gMSwKICAgICAgICAgICAgICAgIH0sCgpBbmQgdGhlIExfVkVSU0lPTiBz dWJvcHQgcGFyYW1ldGVyIHdpbGwgaGF2ZSBhIGNvbmZsaWN0IGxpa2UKCisJCSAgLmNvbmZsaWN0 cyA9IHsgeyBNRVRBLCBNX0NSQywgMSB9LAorCQkJCSB7IExBU1RfQ09ORkxJQ1QsIDAsIDAgfSwg fSwKCjUuIHVwZGF0ZSB0aGUgY29uZmxpY3QgbG9va3VwIHRvIGRvIGNyb3NzIG9wdGlvbiBsb29r dXBzIHZpYQpjaGVja2luZyB0aGUgcmVsZXZhbnQgb3B0aW9uIGNvbmZsaWN0IHRhYmxlLiBlLmcg YnkgY2hlY2tpbmcgdGhlCmNvbmZsaWN0W2ldLnZhbHVlIGFnYWluc3Qgc3Vib3B0W0xPR10uc3Vi b3B0X3BhcmFtc1tMX1ZFUlNJT05dLnZhbHVlLi4uCgo+IEJ1dCB3aGF0IGluIHRoZSBtZWFudGlt ZT8gQXJlIHdlIGdvaW5nIHRvIGxldCBpdCBiZSBub3cgdW50aWwgaXQgaXMKPiBmaXhlZCBieSB0 aGUgZW5oYW5jZWQgdGFibGU/CgpJTU86IGZpeCBpdCBvbmNlLCBmaXggaXQgcHJvcGVybHkuCgo+ IEFuZCByZWdhcmRpbmcgbXkgcXVlc3Rpb24gYXQgdGhlIGVuZCBvZiB0aGUgbWFpbCwgSSBpbnRl cnByZXQgeW91cgo+IGFuc3dlciBhcyAiaWYgdGhlIGFyZ3VtZW50cyBhcmUgd3JvbmcsIGZhaWwg QVNBUCBhbmQgZG9uJ3QgdHJ5IHRvIGZpeAo+IGl0LiIKClRoZSBmaXJzdCBzdGVwIGluIGFueSBw cm9ncmFtIHNob3VsIGRiZSB0byB2YWxpZGF0ZSB1c2VyIHN1cHBsaWVkCmlucHV0cy4gT25jZSB0 aGV5IGFyZSB2YWxpZGF0ZWQgYW5kIGtub3duIGdvb2QsIHlvdSBkb24ndCBoYXZlIHRvCmFkZCBy YW5kb20gY29kZSB0byBoYW5kbGUgaW52YWxpZCBjb21iaW5hdGlvbnMgLSB5b3UgY2FuIGp1c3Qg YXNzdW1lCnRoZSBpbnB1dHMgYXJlIHZhbGlkIHRvIGJlZ2luIHdpdGggYW5kIHRob3NlIGNvcm5l ciBjYXNlcyBkb24ndCBuZWVkCnRvIGJlIGhhbmRsZWQuCgpDaGVlcnMsCgpEYXZlLgotLSAKRGF2 ZSBDaGlubmVyCmRhdmlkQGZyb21vcmJpdC5jb20KCl9fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fCnhmcyBtYWlsaW5nIGxpc3QKeGZzQG9zcy5zZ2kuY29tCmh0 dHA6Ly9vc3Muc2dpLmNvbS9tYWlsbWFuL2xpc3RpbmZvL3hmcwo=