Re: [PATCH 4/5] 9pfs: fix wrong errno being sent to Linux client on macOS host

[Christian Schoenebeck]

On Donnerstag, 21. April 2022 12:48:35 CEST Greg Kurz wrote:
> On Tue, 19 Apr 2022 13:41:59 +0200
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > Linux and macOS only share some errno definitions with equal macro
> > name and value. In fact most mappings for errno are completely
> > different on the two systems.
> > 
> > This patch converts some important errno values from macOS host to
> > corresponding Linux errno values before eventually sending such error
> > codes along with Tlerror replies (if 9p2000.L is used that is), which
> > fixes a bunch of misbehaviours when running a Linux client on macOS
> > host.
> 
> This even fixes an actual protocol violation :
> 
> lerror -- return error code
> 
> size[4] Rlerror tag[2] ecode[4]
> 
> lerror replaces the reply message used in a successful call. ecode is a
> numerical Linux errno.
> ^^^^^^^^^^^^^^^^^^^^^
> 
> Taken from
> https://github.com/chaos/diod/wiki/protocol#lerror----return-error-code

Again, something to add to the commit log?

> > For instance this patch fixes:
> >   mount -t 9p -o posixacl ...
> > 
> > on Linux guest if security_mode=mapped was used for 9p server, which
> > refused to mount successfully, because macOS returned ENOATTR==93
> > when client tried to retrieve POSIX ACL xattrs, because errno 93
> > is defined as EPROTONOSUPPORT==93 on Linux, so Linux client believed
> > that xattrs were not supported by filesystem on host in general.
> > 
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> > 
> >  hw/9pfs/9p.c | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index d953035e1c..becc41cbfd 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -57,6 +57,31 @@ enum {
> > 
> >  P9ARRAY_DEFINE_TYPE(V9fsPath, v9fs_path_free);
> > 
> > +/* Translates errno from host -> Linux if needed */
> > +static int errno_to_dotl(int err) {
> > +#if defined(CONFIG_LINUX)
> > +    /* nothing to translate (Linux -> Linux) */
> > +#elif defined(CONFIG_DARWIN)
> > +    /* translation mandatory for macOS hosts */
> > +    if (err == ENAMETOOLONG) {
> > +        err = 36; /* ==ENAMETOOLONG on Linux */
> > +    } else if (err == ENOTEMPTY) {
> > +        err = 39; /* ==ENOTEMPTY on Linux */
> > +    } else if (err == ELOOP) {
> > +        err = 40; /* ==ELOOP on Linux */
> > +    } else if (err == ENOATTR) {
> > +        err = 61; /* ==ENODATA on Linux */
> > +    } else if (err == ENOTSUP) {
> > +        err = 95; /* ==EOPNOTSUPP on Linux */
> > +    } else if (err == EOPNOTSUPP) {
> > +        err = 95; /* ==EOPNOTSUPP on Linux */
> > +    }
> 
> I'm assuming you have audited all errnos, right ? Just to be sure
> that this won't bite anymore.

Depends on what you mean with "all". Like I wrote in the commit log, for now I 
translated in this patch those errnos that I identified as important, and 
those are audited by me of course (checked the man pages for what errors are 
expected as result from calls on Linux vs. macOS side and looked up numeric 
values from header files on both sides, tests).

However if you rather mean really all errnos that were ever defined on Linux 
and macOS, then the answer is no. That would probably quite some work, and I'm 
not sure if you could just try to translate them dry, i.e. by just looking at 
the headers or something.

So yes, your concern is justified. The question is, should this all be 
translated right now already, or would it be OK to address this minimum set of 
errno translation for now (especially for qemu-stable) and then later address 
the rest?

On the long term you would probably import the Linux errno header file into 
the code base, then prefix the individual macros with something like 
DOTL_ENODATA, etc. and then use those macros for translating the errnos 
instead of using literal numerics, maybe using GCC's designated array 
initializers then.

> > +#else
> > +#error Missing errno translation to Linux for this host system
> > +#endif
> > +    return err;
> > +}
> > +
> 
> As with the other patch, I'd rather move this magic to 9p-util.h .

Makes sense. There is indeed already too much utility code piled up in 9p.c.

> >  static ssize_t pdu_marshal(V9fsPDU *pdu, size_t offset, const char *fmt,
> >  ...) {
> >  
> >      ssize_t ret;
> > 
> > @@ -1054,6 +1079,8 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu,
> > ssize_t len)> 
> >              }
> >              len += ret;
> >              id = P9_RERROR;
> > 
> > +        } else {
> > +            err = errno_to_dotl(err);
> > 
> >          }
> >          
> >          ret = pdu_marshal(pdu, len, "d", err);