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 361BA7CA0 for ; Thu, 7 Apr 2016 08:27:29 -0500 (CDT) Received: from cuda.sgi.com (cuda1.sgi.com [192.48.157.11]) by relay2.corp.sgi.com (Postfix) with ESMTP id CE8A530404E for ; Thu, 7 Apr 2016 06:27:28 -0700 (PDT) Received: from mail-ig0-f180.google.com (mail-ig0-f180.google.com [209.85.213.180]) by cuda.sgi.com with ESMTP id slEfjRQn2qAdjG8L (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO) for ; Thu, 07 Apr 2016 06:27:26 -0700 (PDT) Received: by mail-ig0-f180.google.com with SMTP id gy3so13284819igb.1 for ; Thu, 07 Apr 2016 06:27:26 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <57065E19.9070401@sandeen.net> References: <1458818136-56043-1-git-send-email-jtulak@redhat.com> <1458818136-56043-4-git-send-email-jtulak@redhat.com> <5705BB39.5010003@sandeen.net> <57065E19.9070401@sandeen.net> From: Jan Tulak Date: Thu, 7 Apr 2016 15:27:06 +0200 Message-ID: Subject: Re: [PATCH 03/19] mkfs: Sanitise the superblock feature macros List-Id: XFS Filesystem from SGI List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: multipart/mixed; boundary="===============0287178948099930389==" Errors-To: xfs-bounces@oss.sgi.com Sender: xfs-bounces@oss.sgi.com To: Eric Sandeen Cc: xfs-oss --===============0287178948099930389== Content-Type: multipart/alternative; boundary=089e01538da0048bf5052fe50a4d --089e01538da0048bf5052fe50a4d Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Thu, Apr 7, 2016 at 3:18 PM, Eric Sandeen wrote: > On 4/7/16 8:09 AM, Jan Tulak wrote: > > > ... > > > On Thu, Apr 7, 2016 at 3:43 AM, Eric Sandeen > wrote: > > > > > @@ -981,11 +1077,21 @@ main( > > > int worst_freelist; > > > libxfs_init_t xi; > > > struct fs_topology ft; > > > - int lazy_sb_counters; > > > - int crcs_enabled; > > > - int finobt; > > > - bool finobtflag; > > > - int spinodes; > > > + struct sb_feat_args sb_feat =3D { > > > + .finobt =3D 1, > > > + .finobtflag =3D false, > > > > > > should we really have "finobtflag" in this structure? > > This structure should only carry feature selections, not feature > > specification flags I think. Why is this the only such flag > > in the structure? > > > > Pretty sure finobtflag should stay a variable for now > > just like lvflag (which goes with log_version). > > > > > > =E2=80=8BIt might be right to move it out=E2=80=8B, but the flag is rem= oved few > > patches later entirely. Is it worth of the work? I would say nah, let > > it die where it is. :-) > > Given that it doesn't seem to be a bug, I guess that might be ok, > but in general introducing incorrect things and fixing them later > in the series is strongly discouraged... > > > > > > > ... > > > > > @@ -1517,7 +1617,14 @@ main( > > > c =3D atoi(value); > > > if (c < 0 || c > 1) > > > illegal(value, "m > crc"); > > > - crcs_enabled =3D c; > > > + if (c && nftype) { > > > + fprintf(stderr, > > > +_("cannot specify both crc and ftype\n")); > > > + usage(); > > > > hm, why is conflict checking added? It's not what the commit says > > the patch does. > > > > It also regresses the bug I fixed in > > > > commit b990de8ba4e2df2bc76a140799d3ddb4a0eac4ce > > Author: Eric Sandeen sandeen@sandeen.net>> > > Date: Tue Aug 18 17:53:17 2015 +1000 > > > > mkfs.xfs: fix ftype-vs-crc option combination testing > > > > with this patch, it is broken again: > > > > # mkfs/mkfs.xfs -m crc=3D0 -n ftype=3D1 -dfile,name=3Dfsfile,size= =3D16g > > > > # mkfs/mkfs.xfs -n ftype=3D1 -m crc=3D0 -dfile,name=3Dfsfile,size= =3D16g > > cannot specify both crc and ftype > > Usage: mkfs.xfs > > ... > > > > =E2=80=8BBecause the patch is much older than your fix, and at the time= it > > was created, it is possible that there wasn't any such check... I > > would call it the risk of necromancy. :-)=E2=80=8B > > Most likely a forward-port or merge error I think. > > > Anyway, I already fixed this issue in this cycle, and added the the > > ftype, crc order into a test checking for options sanity. Just I > > didn't submitted the change yet. > > Ok, so it is fixed in your new version of this patch? > =E2=80=8BYes. =E2=80=8B > > > > > ... > > > > > @@ -1879,23 +1988,25 @@ _("32 bit Project IDs always enabled on > CRC enabled filesytems\n")); > > > } else { > > > /* > > > * The kernel doesn't currently support > crc=3D0,finobt=3D1 > > > - * filesystems. If crcs are not enabled and the use= r > has > > > - * explicitly turned them off then silently turn > them off > > > - * to avoid an unnecessary warning. If the user > explicitly > > > - * tried to use crc=3D0,finobt=3D1, then issue a wa= rning > before > > > - * turning them off. > > > + * filesystems. If crcs are not enabled and the use= r > has not > > > + * explicitly turned finobt on, then silently turn > it off to > > > + * avoid an unnecessary warning. If the user > explicitly tried > > > + * to use crc=3D0,finobt=3D1, then issue a warning > before turning > > > + * them off. > > > */ > > > - if (finobt && finobtflag) { > > > - fprintf(stderr, > > > -_("warning: finobt not supported without CRC support, > disabled.\n")); > > > + if (sb_feat.finobt){ > > > + if (sb_feat.finobtflag) { > > > + fprintf(stderr, > > > + _("warning: finobt not supported without CRC support, > disabled.\n")); > > > + } > > > + sb_feat.finobt =3D 0; > > > > like I mentioned, just this, I think (assuming we like the silent > turning > > off, but that would be a different patch): > > > > > > =E2=80=8BMerging the conditions is indeed cleaner. > > > > And I will change it to failure, if the conflicting options are given > > explicitly. Just a small patch adding "usage();" and removing > > "warning"...=E2=80=8B > > Ok, so for this patch, nothing but the mechanical change matching all the > others, > right? If there is any change in behavior to be done, that should be a > different patch. > =E2=80=8BExactly. =E2=80=8B > > -Eric > > _______________________________________________ > xfs mailing list > xfs@oss.sgi.com > http://oss.sgi.com/mailman/listinfo/xfs > --=20 Jan Tulak jtulak@redhat.com / jan@tulak.me --089e01538da0048bf5052fe50a4d Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
On Thu, Ap= r 7, 2016 at 3:18 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
On 4/7/16 8:09 AM, Jan Tu= lak wrote:
>
...

> On Thu, Apr 7, 2016 at 3:43 AM, Eric Sandeen <sandeen@sandeen.net <mailto:sandeen@sandeen.net>> wrote:
>
>=C2=A0 =C2=A0 =C2=A0> @@ -981,11 +1077,21 @@ main(
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 =C2=A0 =C2=A0int=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0worst_freelist;<= br> >=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 =C2=A0 =C2=A0libxfs_init_t=C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0xi;
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 =C2=A0 =C2=A0struct fs_topology= =C2=A0 =C2=A0 =C2=A0 ft;
>=C2=A0 =C2=A0 =C2=A0> -=C2=A0 =C2=A0 =C2=A0int=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0lazy_sb_counters; >=C2=A0 =C2=A0 =C2=A0> -=C2=A0 =C2=A0 =C2=A0int=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0crcs_enabled;
>=C2=A0 =C2=A0 =C2=A0> -=C2=A0 =C2=A0 =C2=A0int=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0finobt;
>=C2=A0 =C2=A0 =C2=A0> -=C2=A0 =C2=A0 =C2=A0bool=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 finobtflag;
>=C2=A0 =C2=A0 =C2=A0> -=C2=A0 =C2=A0 =C2=A0int=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0spinodes;
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0struct sb_feat_args=C2=A0= =C2=A0 =C2=A0sb_feat =3D {
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0.finobt =3D 1,
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0.finobtflag =3D false,
>
>
>=C2=A0 =C2=A0 =C2=A0should we really have "finobtflag" in thi= s structure?
>=C2=A0 =C2=A0 =C2=A0This structure should only carry feature selections= , not feature
>=C2=A0 =C2=A0 =C2=A0specification flags I think.=C2=A0 Why is this the = only such flag
>=C2=A0 =C2=A0 =C2=A0in the structure?
>
>=C2=A0 =C2=A0 =C2=A0Pretty sure finobtflag should stay a variable for n= ow
>=C2=A0 =C2=A0 =C2=A0just like lvflag (which goes with log_version).
>
>
> =E2=80=8BIt might be right to move it out=E2=80=8B, but the flag is re= moved few
> patches later entirely. Is it worth of the work? I would say nah, let<= br> > it die where it is. :-)

Given that it doesn't seem to be a bug, I guess that might be ok= ,
but in general introducing incorrect things and fixing them later
in the series is strongly discouraged...

>
>
>=C2=A0 =C2=A0 =C2=A0...
>
>=C2=A0 =C2=A0 =C2=A0> @@ -1517,7 +1617,14 @@ main(
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0c =3D atoi(value);
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0if (c < 0 || c > 1)
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0illegal(value, "m crc&= quot;);
>=C2=A0 =C2=A0 =C2=A0> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0crcs_enabled =3D c;
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0if (c && nftype) {
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0fprintf(stderr,
>=C2=A0 =C2=A0 =C2=A0> +_("cannot specify both crc and ftype\n&q= uot;));
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0usage();
>
>=C2=A0 =C2=A0 =C2=A0hm, why is conflict checking added?=C2=A0 It's = not what the commit says
>=C2=A0 =C2=A0 =C2=A0the patch does.
>
>=C2=A0 =C2=A0 =C2=A0It also regresses the bug I fixed in
>
>=C2=A0 =C2=A0 =C2=A0commit b990de8ba4e2df2bc76a140799d3ddb4a0eac4ce
>=C2=A0 =C2=A0 =C2=A0Author: Eric Sandeen <sandeen@sandeen.net <mailto:sandeen@sandeen.net>>
>=C2=A0 =C2=A0 =C2=A0Date:=C2=A0 =C2=A0Tue Aug 18 17:53= :17 2015 +1000
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0mkfs.xfs: fix ftype-vs-crc option com= bination testing
>
>=C2=A0 =C2=A0 =C2=A0with this patch, it is broken again:
>
>=C2=A0 =C2=A0 =C2=A0# mkfs/mkfs.xfs -m crc=3D0 -n ftype=3D1 -dfile,name= =3Dfsfile,size=3D16g
>=C2=A0 =C2=A0 =C2=A0<success>
>=C2=A0 =C2=A0 =C2=A0 # mkfs/mkfs.xfs -n ftype=3D1 -m crc=3D0 -dfile,nam= e=3Dfsfile,size=3D16g
>=C2=A0 =C2=A0 =C2=A0cannot specify both crc and ftype
>=C2=A0 =C2=A0 =C2=A0Usage: mkfs.xfs
>=C2=A0 =C2=A0 =C2=A0...
>
> =E2=80=8BBecause the patch is much older than your fix, and at the tim= e it
> was created, it is possible that there wasn't any such check... I<= br> > would call it the risk of necromancy. :-)=E2=80=8B

Most likely a forward-port or merge error I think.

> Anyway, I already fixed this issue in this cycle, and added the the > ftype, crc order into a test checking for options sanity. Just I
> didn't submitted the change yet.

Ok, so it is fixed in your new version of this patch?

=E2=80=8BYes.
=E2=80=8B
=C2=A0

>
>=C2=A0 =C2=A0 =C2=A0...
>
>=C2=A0 =C2=A0 =C2=A0> @@ -1879,23 +1988,25 @@ _("32 bit Project= IDs always enabled on CRC enabled filesytems\n"));
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 =C2=A0 =C2=A0} else {
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0/*
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 * The kernel doesn't currently support crc=3D0,finobt=3D1 >=C2=A0 =C2=A0 =C2=A0> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 * filesystems. If crcs are not enabled and the user has
>=C2=A0 =C2=A0 =C2=A0> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 * explicitly turned them off then silently turn them off
>=C2=A0 =C2=A0 =C2=A0> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 * to avoid an unnecessary warning. If the user explicitly
>=C2=A0 =C2=A0 =C2=A0> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 * tried to use crc=3D0,finobt=3D1, then issue a warning before
>=C2=A0 =C2=A0 =C2=A0> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 * turning them off.
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 * filesystems. If crcs are not enabled and the user has not
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 * explicitly turned finobt on, then silently turn it off to
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 * avoid an unnecessary warning. If the user explicitly tried
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 * to use crc=3D0,finobt=3D1, then issue a warning before turning
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 * them off.
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 */
>=C2=A0 =C2=A0 =C2=A0> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0if (finobt && finobtflag) {
>=C2=A0 =C2=A0 =C2=A0> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0fprintf(stderr,
>=C2=A0 =C2=A0 =C2=A0> -_("warning: finobt not supported without= CRC support, disabled.\n"));
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0if (sb_feat.finobt){
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (sb_feat.finobtflag) {
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0fprintf(stderr,<= br> >=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0_("warning: finobt n= ot supported without CRC support, disabled.\n"));
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0sb_feat.finobt =3D 0;
>
>=C2=A0 =C2=A0 =C2=A0like I mentioned, just this, I think (assuming we l= ike the silent turning
>=C2=A0 =C2=A0 =C2=A0off, but that would be a different patch):
>
>
> =E2=80=8BMerging the conditions is indeed cleaner.
>
> And I will change it to failure, if the conflicting options are given<= br> > explicitly. Just a small patch adding "usage();" and removin= g
> "warning"...=E2=80=8B

Ok, so for this patch, nothing but the mechanical change matchi= ng all the others,
right?=C2=A0 If there is any change in behavior to be done, that should be = a different patch.

= =E2=80=8BExactly.
=E2=80=8B
=C2=A0

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs



--
=
--089e01538da0048bf5052fe50a4d-- --===============0287178948099930389== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs --===============0287178948099930389==--