Re: git: 851dc7f859c2 - main - jail: add jail descriptors

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