On 11/17/20, John Baldwin <j...@freebsd.org> wrote:
> On 11/14/20 11:22 AM, Mateusz Guzik wrote:
>> Author: mjg
>> Date: Sat Nov 14 19:22:02 2020
>> New Revision: 367695
>> URL: https://svnweb.freebsd.org/changeset/base/367695
>>
>> Log:
>> thread: batch credential freeing
>>
>> Modified:
>> head/sys/kern/kern_prot.c
>> head/sys/kern/kern_thread.c
>> head/sys/sys/ucred.h
>>
>> Modified: head/sys/kern/kern_prot.c
>> ==============================================================================
>> --- head/sys/kern/kern_prot.c Sat Nov 14 19:21:46 2020
>> (r367694)
>> +++ head/sys/kern/kern_prot.c Sat Nov 14 19:22:02 2020
>> (r367695)
>> @@ -2007,6 +2071,17 @@ crfree(struct ucred *cr)
>> mtx_unlock(&cr->cr_mtx);
>> return;
>> }
>> + crfree_final(cr);
>> +}
>> +
>> +static void
>> +crfree_final(struct ucred *cr)
>> +{
>> +
>> + KASSERT(cr->cr_users == 0, ("%s: users %d not == 0 on cred %p",
>> + __func__, cr->cr_users, cr));
>> + KASSERT(cr->cr_ref == 0, ("%s: ref %d not == 0 on cred %p",
>> + __func__, cr->cr_ref, cr));
>> /*
>
> Please add blank lines before comments. It's in style(9) and I've noticed
> a pattern in your changes of not including them.
>
Ok.
>> Modified: head/sys/sys/ucred.h
>> ==============================================================================
>> --- head/sys/sys/ucred.h Sat Nov 14 19:21:46 2020 (r367694)
>> +++ head/sys/sys/ucred.h Sat Nov 14 19:22:02 2020 (r367695)
>> @@ -114,6 +114,28 @@ struct xucred {
>> struct proc;
>> struct thread;
>>
>> +struct credbatch {
>> + struct ucred *cred;
>> + int users;
>> + int ref;
>> +};
>> +
>> +static inline void
>> +credbatch_prep(struct credbatch *crb)
>> +{
>> + crb->cred = NULL;
>> + crb->users = 0;
>> + crb->ref = 0;
>> +}
>> +void credbatch_add(struct credbatch *crb, struct thread *td);
>> +static inline void
>> +credbatch_process(struct credbatch *crb)
>> +{
>> +
>> +}
>> +void credbatch_add(struct credbatch *crb, struct thread *td);
>> +void credbatch_final(struct credbatch *crb);
>> +
>
> Do not mix prototypes and inlines, especially without spaces
> around the prototype in the middle. Also, the kernel uses __inline
> rather than inline (for better or for worse).
The kernel has a huge mix of inline and __inline, to the point where
my impression was that __inline is obsolete.
> Better would be:
>
> static __inline void
> credbatch_prep()
> {
> ...
> }
>
> static __inline void
> credbatch_process()
> {
> ...
> }
>
> void credbatch_add();
> void credbatch_final();
>
I disagree. The current header order follows how these routines are used.
> It seems you just have a duplicate credbatch_add() in fact.
>
Indeed, I'll whack it.
> Also, why have an empty inline function?
>
Interested parties can check the consumer (also seen in the diff) to
see this is for consistency. I don't think any comments are warranted
in the header.
> These changes would benefit from review.
>
I don't think it's feasible to ask for review for everything lest it
degardes to rubber stamping and I don't think this change warranted
it, regardless of the cosmetic issues which can always show up.
> --
> John Baldwin
>
--
Mateusz Guzik <mjguzik gmail.com>
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
