Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 28 Feb 2002 08:14:03 -0500
From:      "Brian F. Feldman" <green@FreeBSD.org>
To:        Bruce Evans <bde@zeta.org.au>
Cc:        "Brian F. Feldman" <green@FreeBSD.org>, Alfred Perlstein <bright@mu.org>, bde@FreeBSD.org, arch@FreeBSD.org
Subject:   Re: Do we want the _SYS_SYSPROTO_H_ junk? 
Message-ID:  <200202281314.g1SDE3t50441@green.bikeshed.org>
In-Reply-To: Your message of "Thu, 28 Feb 2002 20:52:35 %2B1100." <20020228202851.X52134-100000@gamplex.bde.org> 

next in thread | previous in thread | raw e-mail | index | archive | help
Bruce Evans <bde@zeta.org.au> wrote:
> On Wed, 27 Feb 2002, Brian F. Feldman wrote:
> 
> > I want to know if, on new code, we should put them.  E.g.:
> >
> > #ifndef _SYS_SYSPROTO_H_
> > struct open_args {
> >         char    *path;
> >         int     flags;
> >         int     mode;
> > };
> > #endif
> > int
> > open(td, uap)
> >         struct thread *td;
> >         register struct open_args /* {
> >                 syscallarg(char *) path;
> >                 syscallarg(int) flags;
> >                 syscallarg(int) mode;
> >         } */ *uap;
> > {
> >
> > The first part, if ever actually called into existence by sysproto.h not
> > being included, would be bogus.  Do we want to keep introducing those?
> 
> The ifdef'ed version is better, if there is to be one at all, because it
> can at least in theory be checked automatically (e.g., by not including
> <sys/sysproto.h> so that the struct is not declared, but somehow declare
> the function anyway; then compile and check if the compile worked and
> gave the same result).

However, it doesn't _really_ match what's in sys/sysproto.h because it 
doesn't have explicit padding (so could potentially assemble to a different 
format for that object, rather than the one expected by the syscall handler).

> The second pseudo-declaration of the struct in the comment is bogus.  I
> removed the comment globally when I implemented <sys/sysproto.h>, but it
> came back in a few files in the Lite2 merge.

So for documentation's sake, since it really is useful to generally see the 
real "arguments" to a syscall (or vop, or ...) instead of the opaque one, we 
should not have the comment in the declaration itself but keep the one 
before it?  I'd definitely prefer to have one.

> Both versions are really just comments.  It can be hard to remember what
> is in *uap without them.  Automatic checking for the ifdefed version would
> just check the consistency of the comments.  Wrong comments for simple
> things are worse than no comments.
> 
> The same few files that have syscallarg() in comments also have SCARG()
> in code.  We don't really use either syscallarg() or SCARG().  We just
> require the MD code to arrange the struct so that ordinary struct member
> references work right.  I would prefer the MD code to push the struct
> members onto the stack so that no args structs or pseudo-declarations
> of them are required.

Wouldn't this be not-too-hard to do by declaring an inline function with 
some assembly which would push the argument space onto the stack before 
struct proc * and then calling the sy_call?  The only trouble seems to be 
C's general lack of wanting to let you dynamically choose an arbitrary 
amount of data onto the stack, unless there are endianness/object layout
concerns.

> I would keep introducing the ifdefed version of the struct while there
> is still an args struct.

Why, though, do we declare it with the explicit padding in sys/sysproto.h if 
that's just what the machine's compiler will pad it to in the first place?

-- 
Brian Fundakowski Feldman                           \'[ FreeBSD ]''''''''''\
  <> green@FreeBSD.org  <> bfeldman@tislabs.com      \  The Power to Serve! \
 Opinions expressed are my own.                       \,,,,,,,,,,,,,,,,,,,,,,\



To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-arch" in the body of the message




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200202281314.g1SDE3t50441>