| Message ID | 167658439591.3590000.1501103007888420501.stgit@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfsprogs: random fixes for 6.2 | expand |
On Thu, Feb 16, 2023 at 01:53:15PM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > A user requested the ability to specify directory entry names in a > protofile that have spaces in them. The protofile format itself does > not allow spaces (yay 1973-era protofiles!) but it does allow slashes. > Slashes aren't allowed in directory entry names, so we'll permit this > one gross hack. > > / > 0 0 > d--775 1000 1000 > : Descending path /code/t/fstests > get/isk.sh ---775 1000 1000 /code/t/fstests/getdisk.sh > $ > > Will produce "get isk.h" in the root directory. > While I don't really mind this patch, it seems strange to me to simply replace a slash with a space in lieu of failing the prototype with an 'invalid character' error message, or something like that. With this patch, an user could be mistakenly assuming the get/isk.sh path will be created, and instead, what's gonna be created is a file with a space. I don't really mind it, but I think we could be misleading users. > Requested-by: Daan De Meyer <daan.j.demeyer@gmail.com> > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > mkfs/proto.c | 23 ++++++++++++++++++++++- > 1 file changed, 22 insertions(+), 1 deletion(-) > > > diff --git a/mkfs/proto.c b/mkfs/proto.c > index 68ecdbf3632..bf8de0189db 100644 > --- a/mkfs/proto.c > +++ b/mkfs/proto.c > @@ -171,6 +171,27 @@ getstr( > return NULL; > } > > +/* Extract directory entry name from a protofile. */ > +static char * > +getdirentname( > + char **pp) > +{ > + char *p = getstr(pp); > + char *c = p; > + > + if (!p) > + return NULL; > + > + /* Replace slash with space because slashes aren't allowed. */ > + while (*c) { > + if (*c == '/') > + *c = ' '; > + c++; > + } > + > + return p; > +} > + > static void > rsvfile( > xfs_mount_t *mp, > @@ -580,7 +601,7 @@ parseproto( > rtinit(mp); > tp = NULL; > for (;;) { > - name = getstr(pp); > + name = getdirentname(pp); > if (!name) > break; > if (strcmp(name, "$") == 0) >
On Wed, Feb 22, 2023 at 10:03:03AM +0100, Carlos Maiolino wrote: > On Thu, Feb 16, 2023 at 01:53:15PM -0800, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > A user requested the ability to specify directory entry names in a > > protofile that have spaces in them. The protofile format itself does > > not allow spaces (yay 1973-era protofiles!) but it does allow slashes. > > Slashes aren't allowed in directory entry names, so we'll permit this > > one gross hack. > > > > / > > 0 0 > > d--775 1000 1000 > > : Descending path /code/t/fstests > > get/isk.sh ---775 1000 1000 /code/t/fstests/getdisk.sh > > $ > > > > Will produce "get isk.h" in the root directory. > > > > While I don't really mind this patch, it seems strange to me to simply replace a > slash with a space in lieu of failing the prototype with an 'invalid character' > error message, or something like that. > With this patch, an user could be mistakenly assuming the get/isk.sh path will > be created, and instead, what's gonna be created is a file with a space. > I don't really mind it, but I think we could be misleading users. Hmm. I agree that it /does/ look weird. I guess we could invent suboptions for mkfs -p: mkfs.xfs -p slashes_are_spaces=1,/tmp/protofile Which would turn on this odd looking functionalty? How does that sound? Sidebar: As it is now, the slash gets passed to _addname, with the result that mkfs will error out. Another thing I (just) remembered is that the proto.c will also pick up nulls and pass them to addname, so: get\000isk.sh ---775 1000 1000 /code/t/fstests/getdisk.sh will also cause mkfs to fail the format. I suppose that will also require patching... --D > > > Requested-by: Daan De Meyer <daan.j.demeyer@gmail.com> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > --- > > mkfs/proto.c | 23 ++++++++++++++++++++++- > > 1 file changed, 22 insertions(+), 1 deletion(-) > > > > > > diff --git a/mkfs/proto.c b/mkfs/proto.c > > index 68ecdbf3632..bf8de0189db 100644 > > --- a/mkfs/proto.c > > +++ b/mkfs/proto.c > > @@ -171,6 +171,27 @@ getstr( > > return NULL; > > } > > > > +/* Extract directory entry name from a protofile. */ > > +static char * > > +getdirentname( > > + char **pp) > > +{ > > + char *p = getstr(pp); > > + char *c = p; > > + > > + if (!p) > > + return NULL; > > + > > + /* Replace slash with space because slashes aren't allowed. */ > > + while (*c) { > > + if (*c == '/') > > + *c = ' '; > > + c++; > > + } > > + > > + return p; > > +} > > + > > static void > > rsvfile( > > xfs_mount_t *mp, > > @@ -580,7 +601,7 @@ parseproto( > > rtinit(mp); > > tp = NULL; > > for (;;) { > > - name = getstr(pp); > > + name = getdirentname(pp); > > if (!name) > > break; > > if (strcmp(name, "$") == 0) > > > > -- > Carlos Maiolino
On Wed, Feb 22, 2023 at 08:37:03AM -0800, Darrick J. Wong wrote: > On Wed, Feb 22, 2023 at 10:03:03AM +0100, Carlos Maiolino wrote: > > On Thu, Feb 16, 2023 at 01:53:15PM -0800, Darrick J. Wong wrote: > > > From: Darrick J. Wong <djwong@kernel.org> > > > > > > A user requested the ability to specify directory entry names in a > > > protofile that have spaces in them. The protofile format itself does > > > not allow spaces (yay 1973-era protofiles!) but it does allow slashes. > > > Slashes aren't allowed in directory entry names, so we'll permit this > > > one gross hack. > > > > > > / > > > 0 0 > > > d--775 1000 1000 > > > : Descending path /code/t/fstests > > > get/isk.sh ---775 1000 1000 /code/t/fstests/getdisk.sh > > > $ > > > > > > Will produce "get isk.h" in the root directory. > > > > > > > While I don't really mind this patch, it seems strange to me to simply replace a > > slash with a space in lieu of failing the prototype with an 'invalid character' > > error message, or something like that. > > With this patch, an user could be mistakenly assuming the get/isk.sh path will > > be created, and instead, what's gonna be created is a file with a space. > > I don't really mind it, but I think we could be misleading users. > > Hmm. I agree that it /does/ look weird. I guess we could invent > suboptions for mkfs -p: Sorry, I don't mean to give you more trouble, but silently replacing a character by another one does indeed looks weird to me. > > mkfs.xfs -p slashes_are_spaces=1,/tmp/protofile > > Which would turn on this odd looking functionalty? How does that sound? this sounds better to me :) > > Sidebar: > > As it is now, the slash gets passed to _addname, with the result that > mkfs will error out. > > Another thing I (just) remembered is that the proto.c will also pick up > nulls and pass them to addname, so: > > get\000isk.sh ---775 1000 1000 /code/t/fstests/getdisk.sh > > will also cause mkfs to fail the format. I suppose that will also > require patching... Interesting. /me never played much with protofiles I'll pickup the first 4 patches of this series tomorrow and skip this one for now. I plan to release a 6.2 soon, as we already have linux 6.2 release, but I want to do a libxfs sync first, hopefully I'll have the bandwidth tomorrow and next week release 6.2. will see :) > > > > > > Requested-by: Daan De Meyer <daan.j.demeyer@gmail.com> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > > --- > > > mkfs/proto.c | 23 ++++++++++++++++++++++- > > > 1 file changed, 22 insertions(+), 1 deletion(-) > > > > > > > > > diff --git a/mkfs/proto.c b/mkfs/proto.c > > > index 68ecdbf3632..bf8de0189db 100644 > > > --- a/mkfs/proto.c > > > +++ b/mkfs/proto.c > > > @@ -171,6 +171,27 @@ getstr( > > > return NULL; > > > } > > > > > > +/* Extract directory entry name from a protofile. */ > > > +static char * > > > +getdirentname( > > > + char **pp) > > > +{ > > > + char *p = getstr(pp); > > > + char *c = p; > > > + > > > + if (!p) > > > + return NULL; > > > + > > > + /* Replace slash with space because slashes aren't allowed. */ > > > + while (*c) { > > > + if (*c == '/') > > > + *c = ' '; > > > + c++; > > > + } > > > + > > > + return p; > > > +} > > > + > > > static void > > > rsvfile( > > > xfs_mount_t *mp, > > > @@ -580,7 +601,7 @@ parseproto( > > > rtinit(mp); > > > tp = NULL; > > > for (;;) { > > > - name = getstr(pp); > > > + name = getdirentname(pp); > > > if (!name) > > > break; > > > if (strcmp(name, "$") == 0) > > > > > > > -- > > Carlos Maiolino
On Thu, Feb 23, 2023 at 09:48:39AM +0100, Carlos Maiolino wrote: > On Wed, Feb 22, 2023 at 08:37:03AM -0800, Darrick J. Wong wrote: > > On Wed, Feb 22, 2023 at 10:03:03AM +0100, Carlos Maiolino wrote: > > > On Thu, Feb 16, 2023 at 01:53:15PM -0800, Darrick J. Wong wrote: > > > > From: Darrick J. Wong <djwong@kernel.org> > > > > > > > > A user requested the ability to specify directory entry names in a > > > > protofile that have spaces in them. The protofile format itself does > > > > not allow spaces (yay 1973-era protofiles!) but it does allow slashes. > > > > Slashes aren't allowed in directory entry names, so we'll permit this > > > > one gross hack. > > > > > > > > / > > > > 0 0 > > > > d--775 1000 1000 > > > > : Descending path /code/t/fstests > > > > get/isk.sh ---775 1000 1000 /code/t/fstests/getdisk.sh > > > > $ > > > > > > > > Will produce "get isk.h" in the root directory. > > > > > > > > > > While I don't really mind this patch, it seems strange to me to simply replace a > > > slash with a space in lieu of failing the prototype with an 'invalid character' > > > error message, or something like that. > > > With this patch, an user could be mistakenly assuming the get/isk.sh path will > > > be created, and instead, what's gonna be created is a file with a space. > > > I don't really mind it, but I think we could be misleading users. > > > > Hmm. I agree that it /does/ look weird. I guess we could invent > > suboptions for mkfs -p: > > Sorry, I don't mean to give you more trouble, but silently replacing a character > by another one does indeed looks weird to me. Agreed. > > > > mkfs.xfs -p slashes_are_spaces=1,/tmp/protofile > > > > > > Which would turn on this odd looking functionalty? How does that sound? > > this sounds better to me :) Ok, will do. > > > > Sidebar: > > > > As it is now, the slash gets passed to _addname, with the result that > > mkfs will error out. > > > > Another thing I (just) remembered is that the proto.c will also pick up > > nulls and pass them to addname, so: > > > > get\000isk.sh ---775 1000 1000 /code/t/fstests/getdisk.sh > > > > will also cause mkfs to fail the format. I suppose that will also > > require patching... > > Interesting. > /me never played much with protofiles > > I'll pickup the first 4 patches of this series tomorrow and skip this one for > now. I plan to release a 6.2 soon, as we already have linux 6.2 release, but I > want to do a libxfs sync first, hopefully I'll have the bandwidth tomorrow and > next week release 6.2. will see :) Ok. The slashes_are_spaces=1 patches are pretty trivial, so I'll send them in Tuesday's batch. --D > > > > > > > > > > Requested-by: Daan De Meyer <daan.j.demeyer@gmail.com> > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > > > --- > > > > mkfs/proto.c | 23 ++++++++++++++++++++++- > > > > 1 file changed, 22 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/mkfs/proto.c b/mkfs/proto.c > > > > index 68ecdbf3632..bf8de0189db 100644 > > > > --- a/mkfs/proto.c > > > > +++ b/mkfs/proto.c > > > > @@ -171,6 +171,27 @@ getstr( > > > > return NULL; > > > > } > > > > > > > > +/* Extract directory entry name from a protofile. */ > > > > +static char * > > > > +getdirentname( > > > > + char **pp) > > > > +{ > > > > + char *p = getstr(pp); > > > > + char *c = p; > > > > + > > > > + if (!p) > > > > + return NULL; > > > > + > > > > + /* Replace slash with space because slashes aren't allowed. */ > > > > + while (*c) { > > > > + if (*c == '/') > > > > + *c = ' '; > > > > + c++; > > > > + } > > > > + > > > > + return p; > > > > +} > > > > + > > > > static void > > > > rsvfile( > > > > xfs_mount_t *mp, > > > > @@ -580,7 +601,7 @@ parseproto( > > > > rtinit(mp); > > > > tp = NULL; > > > > for (;;) { > > > > - name = getstr(pp); > > > > + name = getdirentname(pp); > > > > if (!name) > > > > break; > > > > if (strcmp(name, "$") == 0) > > > > > > > > > > -- > > > Carlos Maiolino > > -- > Carlos Maiolino
diff --git a/mkfs/proto.c b/mkfs/proto.c index 68ecdbf3632..bf8de0189db 100644 --- a/mkfs/proto.c +++ b/mkfs/proto.c @@ -171,6 +171,27 @@ getstr( return NULL; } +/* Extract directory entry name from a protofile. */ +static char * +getdirentname( + char **pp) +{ + char *p = getstr(pp); + char *c = p; + + if (!p) + return NULL; + + /* Replace slash with space because slashes aren't allowed. */ + while (*c) { + if (*c == '/') + *c = ' '; + c++; + } + + return p; +} + static void rsvfile( xfs_mount_t *mp, @@ -580,7 +601,7 @@ parseproto( rtinit(mp); tp = NULL; for (;;) { - name = getstr(pp); + name = getdirentname(pp); if (!name) break; if (strcmp(name, "$") == 0)