Date: Fri, 05 Sep 2025 10:24:16 -0700 From: James Gritton <jamie@freebsd.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: 851dc7f859c2 - main - jail: add jail descriptors Message-ID: <282327ee69d5c26f379961c12e19dfbe@freebsd.org> In-Reply-To: <aLpxozYUfi_S-U7b@kib.kiev.ua> References: <202509042031.584KVpxY000408@gitrepo.freebsd.org> <aLokHDP-EMa1LR0D@kib.kiev.ua> <da6b56365c188ce55bb4e878636bc911@freebsd.org> <aLpxozYUfi_S-U7b@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On 2025-09-04 22:14, Konstantin Belousov wrote: > On Thu, Sep 04, 2025 at 09:43:13PM -0700, James Gritton wrote: >> On 2025-09-04 16:43, Konstantin Belousov wrote: >> > jd = malloc(sizeof(*jd), M_JAILDESC, M_WAITOK | M_ZERO); >> > error = falloc_caps(td, &fp, fdp, 0, NULL); >> > finit(fp, priv_check_cred(fp->f_cred, PRIV_JAIL_SET) == 0 >> > ? FREAD | FWRITE : FREAD, DTYPE_JAILDESC, jd, &jaildesc_ops); >> > ^^^^^^^^^^^ '?' should be placed on the previous line >> >> I wasn't aware of this requirement; style(9) is silent on it. > In fact style(9) contains the explicit requirement: > If you have to wrap a long statement, put the operator at the end > of the > line. > There are a lot more of this pattern repeated in the commit. Ah, yes it says that. I was scanning examples looking for the "?", and missed the general statement. I don't think it's the best move for the ?: operator in particular, but I appreciate consistency in style. I can change that in kern_jaildesc.c, but it's a little trickier in kern_jail.c since it's already replete with me having done it wrong over the years. >> > Generated files should have been committed as a follow-up, not in the >> > same commit as written code. >> >> The FreeBSD Wiki explicitly allows it in the same commit. > I always objected against this practice. For instance, the commit > message > for this commit is even less useful because most of the limit was > filled > with the auto-generated stuff, instead of the code. Same for reading > the > commits with log. > > Could you please point me to the wiki page? https://wiki.freebsd.org/AddingSyscalls#Committing >> > jaildesc_find() returns EBADF when passed file type is not DTYPE_JAIL. >> > Normally EBADF means that the object underlying the file is invalidated, >> > like vnode is reclaimed, tty is revoked, etc. For the wrong type, EINVAL >> > should be returned. >> >> That's part of the code that I lifted from process descriptors, nearly >> identical to procdesc_find. A check of other c_type checks shows >> EBADF isn't uncommon. > So procdesc is wrong as well, I think. The existing code base is quite inconsistent. I some EINVAL, some EBADF (procdesc, kqueue, fcntl), also EPERM, ENODEV, ENOTSUPP, EPIPE, and ENOTSOCK. EINVAL is the most common, but there are enough EBADF that I feel I can keep it. - Jamie
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?282327ee69d5c26f379961c12e19dfbe>