Re: git: a4e4132fa3bf - main - swapoff(2): replace special device name argument with a structure

On Mon, Dec 06, 2021 at 05:21:24PM +0000, Brooks Davis wrote:
> On Sat, Dec 04, 2021 at 10:21:07PM +0000, Konstantin Belousov wrote:
> > The branch main has been updated by kib:
> > 
> > URL: https://cgit.FreeBSD.org/src/commit/?id=a4e4132fa3bfadb6047fc0fa5f399f4640460300
> > 
> > commit a4e4132fa3bfadb6047fc0fa5f399f4640460300
> > Author:     Konstantin Belousov <kib@FreeBSD.org>
> > AuthorDate: 2021-11-29 16:26:31 +0000
> > Commit:     Konstantin Belousov <kib@FreeBSD.org>
> > CommitDate: 2021-12-04 22:20:58 +0000
> > 
> >     swapoff(2): replace special device name argument with a structure
> >     
> >     For compatibility, add a placeholder pointer to the start of the
> >     added struct swapoff_new_args, and use it to distinguish old vs. new
> >     style of syscall invocation.
> 
> I agree with Jess that this should be a new syscall.

> The entry in
> sycalls.master now fails to describe the memory footprint of the name
> argument.  No system call should be created or altered to have a memory
> footprint not describable with SAL annotations unless an applicable
> standard such as POSIX requires it.
Why?  Such requirement is not enforced in any way by the syscall
processing infrastructure, and more, that annotations are not utilized
in any way by the system.

Also, I do not remember a discussion anywhere which would indicate that
community agreed to this requirement.  Since arguably I am the person
that added enough new syscalls in recent times (I do not claim that I
added the majority but probably quite close to it), I should have been
added to the discussion for it to be fair.

The only reference I can find that defines what SAL is, is
https://docs.microsoft.com/en-us/cpp/c-runtime-library/sal-annotations?view=msvc-170

I can add some annotations there, but I am really surprised to see 'must'
statements about it.
> 
> > diff --git a/sys/vm/swap_pager.h b/sys/vm/swap_pager.h
> > index 395fbc9957c4..469de3e8eaf4 100644
> > --- a/sys/vm/swap_pager.h
> > +++ b/sys/vm/swap_pager.h
> > @@ -69,6 +69,14 @@ struct swdevt {
> >  #define	SW_UNMAPPED	0x01
> >  #define	SW_CLOSING	0x04
> >  
> > +struct swapoff_new_args {
> > +	const char *name_old_syscall;
> > +	const char *name;
> > +	u_int flags;
> > +	u_int pad0;
> > +	uintptr_t pad1[8];
> > +};
> 
> If you're going to attempt to add future-proofing, please pad with the
> assumption that pointers are 128-bit sized and aligned.  In this
> case, that would mean an uint64_t pad before pad1.  If there were done
> in place, adding the pad and dropping pad1 to 6 elements would be safe.
Again, this is something new and relatively arbitrary.  I will do this,
I do not see much harm from changing this on HEAD still.