[9front] [PATCH] make exportfs give "standard" error for file does exist

[9front] [PATCH] make exportfs give "standard" error for file does exist

[Jacob Moody]

Certain 9p clients *cough* Guhnew/Linucks *cough* expect a fileserver to give back specifically "file does not exist"
for this certain error case. For linux specifically this results in create being broken. This patch catches this error
and returns back what the broken client would like.

This sucks, telling linux to go kick rocks would also be fine in my opinion.

diff 801664db7f4ed740d40f40f93f1aa4dd48e1c329 uncommitted
--- a//sys/src/cmd/exportfs/exportsrv.c
+++ b//sys/src/cmd/exportfs/exportsrv.c
@@ -198,7 +198,10 @@
 		wf = file(f->f, t->work.wname[i]);
 		if(wf == nil){
 			errstr(err, sizeof err);
-			e = err;
+			if(strstr(err, "does not exist") != nil)
+				e = "file does not exist";
+			else
+				e = err;
 			break;
 		}
     Accept:

Re: [9front] [PATCH] make exportfs give "standard" error for file does exist

[Stuart Morrow]

> Subject: make exportfs give "standard" error for FILE DOES EXIST
(my capitals)

If this goes in they need to not commit it directly from the email.

Re: [9front] [PATCH] make exportfs give "standard" error for file does exist

[Jacob Moody]

On 7/23/23 17:03, Stuart Morrow wrote:
>> Subject: make exportfs give "standard" error for FILE DOES EXIST
> (my capitals)
> 
> If this goes in they need to not commit it directly from the email.

Sorry for the typo.
Is that all you wanted to comment on?

Re: [9front] [PATCH] make exportfs give "standard" error for file does exist

[Stuart Morrow]

yep

Re: [9front] [PATCH] make exportfs give "standard" error for file does exist

[Jacob Moody]

On 7/23/23 17:24, Stuart Morrow wrote:
> yep

It is a bit frustrating to ask for input and have
someone ignore that to instead talk past you about a typo.

Re: [9front] [PATCH] make exportfs give "standard" error for file does exist

[Stuart Morrow]

> It is a bit frustrating to ask for input and have
> someone ignore that to instead talk past you about a typo.

Apologies,

 _but_

it's a typo that would have ended up in documentation, the release
notes being barely more than the git/log -s.

Only kind of typos I (anyone) ever talk about. At least I hope so.

Re: [9front] [PATCH] make exportfs give "standard" error for file does exist

[Jacob Moody]

On 7/24/23 16:20, Stuart Morrow wrote:
>> It is a bit frustrating to ask for input and have
>> someone ignore that to instead talk past you about a typo.
> 
> Apologies,
> 
>  _but_
> 
> it's a typo that would have ended up in documentation, the release
> notes being barely more than the git/log -s.

Very true. In hindsight it was a good call out and I was being more then
a bit sour. Sorry.

Re: [9front] [PATCH] make exportfs give "standard" error for file does exist

[Michael Forney]

On 2023-07-23, Jacob Moody <moody@mail.posixcafe.org> wrote:
> Certain 9p clients *cough* Guhnew/Linucks *cough* expect a fileserver to
> give back specifically "file does not exist"
> for this certain error case. For linux specifically this results in create
> being broken. This patch catches this error
> and returns back what the broken client would like.
>
> This sucks, telling linux to go kick rocks would also be fine in my
> opinion.

I think it would be better to fix linux to better accommodate these
error messages than to add a workaround in exportfs. Or at least, we
should fix linux at the same time with the intention of reverting the
patch once the linux fix is more widespread.

The two issues at hand are:
- 9front prefixes error messages with the quoted filename in syscall
error strings, exportfs just forwards them along as Rerror, and linux
only recognizes fixed strings as errno codes. This breaks v9fs with
exportfs. If linux would strip this prefix before doing error string
to errno conversion, it would also fix strings that get mapped to
other codes, such as EEXIST.
- linux does not recognize "does not exist" as ENOENT.

I've been running the following two patches for 9p in my linux kernels
to fix these two issues respectively:

https://github.com/oasislinux/linux/commit/bcf7e4dc01ad41da7ad010fa20c517ce8edb9628.patch
https://github.com/oasislinux/linux/commit/2882844e1fbbe3c56d4ec09a8d6818636cbe1cf5.patch

Looking into this more closely, I realize that "does not exist" isn't
coming from cwfs, so the commit message in the second patch is wrong.
Do you know where "does not exist" is coming from, primarily? I don't
remember the situation I was seeing this message in. There is an
instance in /sys/src/9/port/chan.c:/^walk, which seems likely, but I'm
not familiar enough with the channel code to know when this occurs.

I can send those patches to the linux 9p maintainers. I don't think
they are too controversial, so I am optimistic that they would be
accepted, but who knows.

Re: [9front] [PATCH] make exportfs give "standard" error for file does exist

[Jacob Moody]

On 7/23/23 19:07, Michael Forney wrote:
> On 2023-07-23, Jacob Moody <moody@mail.posixcafe.org> wrote:
>> Certain 9p clients *cough* Guhnew/Linucks *cough* expect a fileserver to
>> give back specifically "file does not exist"
>> for this certain error case. For linux specifically this results in create
>> being broken. This patch catches this error
>> and returns back what the broken client would like.
>>
>> This sucks, telling linux to go kick rocks would also be fine in my
>> opinion.
> 
> I think it would be better to fix linux to better accommodate these
> error messages than to add a workaround in exportfs. Or at least, we
> should fix linux at the same time with the intention of reverting the
> patch once the linux fix is more widespread.
> 
> The two issues at hand are:
> - 9front prefixes error messages with the quoted filename in syscall
> error strings, exportfs just forwards them along as Rerror, and linux
> only recognizes fixed strings as errno codes. This breaks v9fs with
> exportfs. If linux would strip this prefix before doing error string
> to errno conversion, it would also fix strings that get mapped to
> other codes, such as EEXIST.
> - linux does not recognize "does not exist" as ENOENT.
> 
> I've been running the following two patches for 9p in my linux kernels
> to fix these two issues respectively:
> 
> https://github.com/oasislinux/linux/commit/bcf7e4dc01ad41da7ad010fa20c517ce8edb9628.patch
> https://github.com/oasislinux/linux/commit/2882844e1fbbe3c56d4ec09a8d6818636cbe1cf5.patch
> 
> Looking into this more closely, I realize that "does not exist" isn't
> coming from cwfs, so the commit message in the second patch is wrong.
> Do you know where "does not exist" is coming from, primarily? I don't
> remember the situation I was seeing this message in. There is an
> instance in /sys/src/9/port/chan.c:/^walk, which seems likely, but I'm
> not familiar enough with the channel code to know when this occurs.
> 
> I can send those patches to the linux 9p maintainers. I don't think
> they are too controversial, so I am optimistic that they would be
> accepted, but who knows.

Thank you!

I agree with your conclusion of those two core issues here. I noticed that with
this error mismatch v9fs would not be able to create files. It seems to walk to the
file first to ensure that it doesn't exist before actually sending the create call.
I would argue that perhaps this is a logic error as well, and it would be better to just
send a create call. But I do know enough about posix semantics to know if there is a good
reason for this.

If you would be willing to send those patches to the linux 9p folks, I agree that would
be the best solution here. I counted that out because I have not the slightest on how
to go about that. Let me know if you need any help with that.

Thanks,
moody

Re: [9front] [PATCH] make exportfs give "standard" error for file does exist

[ori]

Quoth Michael Forney <mforney@mforney.org>:
> On 2023-07-23, Jacob Moody <moody@mail.posixcafe.org> wrote:
> > Certain 9p clients *cough* Guhnew/Linucks *cough* expect a fileserver to
> > give back specifically "file does not exist"
> > for this certain error case. For linux specifically this results in create
> > being broken. This patch catches this error
> > and returns back what the broken client would like.
> >
> > This sucks, telling linux to go kick rocks would also be fine in my
> > opinion.
> 
> I think it would be better to fix linux to better accommodate these
> error messages than to add a workaround in exportfs. Or at least, we
> should fix linux at the same time with the intention of reverting the
> patch once the linux fix is more widespread.
> 
> The two issues at hand are:
> - 9front prefixes error messages with the quoted filename in syscall
> error strings, exportfs just forwards them along as Rerror, and linux
> only recognizes fixed strings as errno codes. This breaks v9fs with
> exportfs. If linux would strip this prefix before doing error string
> to errno conversion, it would also fix strings that get mapped to
> other codes, such as EEXIST.
> - linux does not recognize "does not exist" as ENOENT.
> 
> I've been running the following two patches for 9p in my linux kernels
> to fix these two issues respectively:
> 
> https://github.com/oasislinux/linux/commit/bcf7e4dc01ad41da7ad010fa20c517ce8edb9628.patch
> https://github.com/oasislinux/linux/commit/2882844e1fbbe3c56d4ec09a8d6818636cbe1cf5.patch
> 
> Looking into this more closely, I realize that "does not exist" isn't
> coming from cwfs, so the commit message in the second patch is wrong.
> Do you know where "does not exist" is coming from, primarily? I don't
> remember the situation I was seeing this message in. There is an
> instance in /sys/src/9/port/chan.c:/^walk, which seems likely, but I'm
> not familiar enough with the channel code to know when this occurs.
> 
> I can send those patches to the linux 9p maintainers. I don't think
> they are too controversial, so I am optimistic that they would be
> accepted, but who knows.

I would be tempted to make 2 changes, and adopt a convention:

1. Replace full string comparisons with prefix comparisons
2. change 9front to add useful information as a suffix, rather than as a prefix:

	file does not exist: '/path/to/file'

this will make strncmp do the right thing, and make it possible to
both have our cake and eat it too; use well known error strings and
compare them, and still give the user additional useful information.

Re: [9front] [PATCH] make exportfs give "standard" error for file does exist

[hiro]

> 2. change 9front to add useful information as a suffix, rather than as a
> prefix:

ori: so simple. nice.

makes me wonder why they didn't do it this way in the first place.
anti-unix sabotage i hope!

Re: [9front] [PATCH] make exportfs give "standard" error for file does exist

[Jacob Moody]

On 7/24/23 03:13, hiro wrote:
>> 2. change 9front to add useful information as a suffix, rather than as a
>> prefix:
> 
> ori: so simple. nice.
> 
> makes me wonder why they didn't do it this way in the first place.
> anti-unix sabotage i hope!

I could be wrong but I think it's just reversing the arguments in
/sys/src/9/port/chan.c:1250. There is probably more strings to fix,
but this covers the specific 'path' <error> stuff

Re: [9front] [PATCH] make exportfs give "standard" error for file does exist

[ori]

Quoth Jacob Moody <moody@posixcafe.org>:
> On 7/24/23 03:13, hiro wrote:
> >> 2. change 9front to add useful information as a suffix, rather than as a
> >> prefix:
> > 
> > ori: so simple. nice.
> > 
> > makes me wonder why they didn't do it this way in the first place.
> > anti-unix sabotage i hope!
> 
> I could be wrong but I think it's just reversing the arguments in
> /sys/src/9/port/chan.c:1250. There is probably more strings to fix,
> but this covers the specific 'path' <error> stuff
> 

yeah, I'm sure there will be other things to change, but
this is a good start; we can document that this is our
new convention, maybe put a list of "well known" prefixes
in a manpage, and start fixing the fmts that were newly
defined as bugs.

[9front] Re: [PATCH] make exportfs give "standard" error for file does exist

[Anthony Martin]

The lady doth protest too much, methinks.

Here's a list of all file servers that do not use Enonexist,
aka "file does not exist" for this purpose:

	git/fs
	nusb/disk
	nusb/serial
	nusb/usbd
	skelfs

There's a commonality here. These are 9front snowflakes.
Just make them use the common error.

There is one wrinkle to my theory, however. When the kernel
attempts to walk a path but only partially succeeds (i.e. when
devwalk, mntwalk, or shrwalk returns with wq->clone set to
nil), it makes up an error since it has no way of knowing the
real underlying cause of failure. The error walk chooses is
"does not exist" or Enotdir even though it could be completely
wrong. That error could be renamed. I propose

	extern char Ecrippled[];	/* cannot walk */

but not really.

Alternatively, you could send a patch for net/9p/error.c to
lkml. Have you seen that monstrosity? What's one more
addition?

Cheers,
  Anthony

Re: [9front] Re: [PATCH] make exportfs give "standard" error for file does exist

[Jacob Moody]

On 7/23/23 19:51, Anthony Martin wrote:
> The lady doth protest too much, methinks.
> 
> Here's a list of all file servers that do not use Enonexist,
> aka "file does not exist" for this purpose:
> 
> 	git/fs
> 	nusb/disk
> 	nusb/serial
> 	nusb/usbd
> 	skelfs
> 
> There's a commonality here. These are 9front snowflakes.
> Just make them use the common error.

Sure, those can be fixed but that is not the problem here.
The problem is that exportfs sends the errstr back from what
it gets from open(2). The kernel gives you back the path attempted
to be accessed in quotes prefixed to 'does not exist'.
Every fileserver using Enotexist would not fix exportfs causing
this issue with v9fs.

Re: [9front] Re: [PATCH] make exportfs give "standard" error for file does exist

[Anthony Martin]

Jacob Moody <moody@mail.posixcafe.org> once said:
> The problem is that exportfs sends the errstr back from what
> it gets from open(2). The kernel gives you back the path attempted
> to be accessed in quotes prefixed to 'does not exist'.
> Every fileserver using Enotexist would not fix exportfs causing
> this issue with v9fs.

I understand. That's partly why I suggested sending a patch
to lkml. They use memcmp on the errstr but they should be
doing the equivalent of strstr (like Go's syscall package).

Cheers,
  Anthony

Re: [9front] Re: [PATCH] make exportfs give "standard" error for file does exist

[ori]

Quoth Anthony Martin <ality@pbrane.org>:
> Jacob Moody <moody@mail.posixcafe.org> once said:
> > The problem is that exportfs sends the errstr back from what
> > it gets from open(2). The kernel gives you back the path attempted
> > to be accessed in quotes prefixed to 'does not exist'.
> > Every fileserver using Enotexist would not fix exportfs causing
> > this issue with v9fs.
> 
> I understand. That's partly why I suggested sending a patch
> to lkml. They use memcmp on the errstr but they should be
> doing the equivalent of strstr (like Go's syscall package).
> 
> Cheers,
>   Anthony

note, the problem with strstr is that walking to a file named
'file does not exist' may confuse the caller; this is why I
would like to move to a convention where the "well known" error
is at a well known location.

Re: [9front] Re: [PATCH] make exportfs give "standard" error for file does exist

[Anthony Martin]

ori@eigenstate.org once said:
> note, the problem with strstr is that walking to a file named
> 'file does not exist' may confuse the caller; this is why I
> would like to move to a convention where the "well known" error
> is at a well known location.

1. Spaces in file names are suspect to begin with.

2. If you name a file "file does not exist", you deserve
the consequences.

3. Almost two decades ago, ericvh wanted to standardize
error strings to appease Linux v9fs. Nothing happened.

4. Over a decade ago, rminnich proposed using "%d:%s"
with a prefixed errno for all errors, mostly to appease
Linux v9fs. Nothing happened.

5. All of this only matters because the POSIX tolerators
are trying to parse error strings to make up an "error
code" and do something special for each one so v9fs will
work with vfs_create. How can that ever not be brittle?

6. 9p2000.L, .u, etc. Enough said.

7. This is a lost cause. Outside of Plan 9, friends only
let friends use emu(1).

8. Your idea isn't bad but is it worth it?

Thanks for coming to my TED talk.

Cheers,
  Anthony

Re: [9front] Re: [PATCH] make exportfs give "standard" error for file does exist

[hiro]

> 8. Your idea isn't bad but is it worth it?
it is

did you find any downsides?

Re: [9front] Re: [PATCH] make exportfs give "standard" error for file does exist

[ori]

Quoth Anthony Martin <ality@pbrane.org>:
> ori@eigenstate.org once said:
> > note, the problem with strstr is that walking to a file named
> > 'file does not exist' may confuse the caller; this is why I
> > would like to move to a convention where the "well known" error
> > is at a well known location.
> 
> 1. Spaces in file names are suspect to begin with.
> 
> 2. If you name a file "file does not exist", you deserve
> the consequences.
> 

I don't need to name a file that; I just need to try walking to it.