Re: [PATCH] Detect GNU/kFreeBSD in user-visible kernel headers (v2)

2011-11-27 Thread Robert Millan
Hi Bruce,

2011/11/27 Bruce Evans :
> % Index: sys/cam/scsi/scsi_low.h
> % ===
> % --- sys/cam/scsi/scsi_low.h   (revision 227956)
> % +++ sys/cam/scsi/scsi_low.h   (working copy)
> % @@ -53,10 +53,10 @@
> %  #define      SCSI_LOW_INTERFACE_XS
> %  #endif       /* __NetBSD__ */
> % % -#ifdef     __FreeBSD__
> % +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
> %  #define      SCSI_LOW_INTERFACE_CAM
> %  #define      CAM
> % -#endif       /* __FreeBSD__ */
> % +#endif /* __FreeBSD__ || __FreeBSD_kernel__ */
>
> It still has the whitespace-after tab style change for cam.

My initial patch didn't have it.  Precisely I thought that you
requested this in your first mail.  Did I missunderstand?

> % Index: sys/dev/firewire/firewirereg.h
> % ===
> % --- sys/dev/firewire/firewirereg.h    (revision 227956)
> % +++ sys/dev/firewire/firewirereg.h    (working copy)
> % @@ -75,7 +75,8 @@
> %  };
> % %  struct firewire_softc {
> % -#if defined(__FreeBSD__) && __FreeBSD_version >= 50
> % +#if (defined(__FreeBSD__) || defined(__FreeBSD_kernel__)) && \
> % +    __FreeBSD_version >= 50
> %       struct cdev *dev;
> %  #endif
> %       struct firewire_comm *fc;
>
> Here is a pre-existing problem that you didn't fix on a line that you
> changed.

Yes.  I didn't say I would fix all pre-existing problems in lines that
need to be modified.  In some cases I did, and in some cases I opted
to preserve the status quo.

> __FreeBSD_version it is impossible to determine if the data structure
> has new fields like the cdev in it.   is a prerequisite
> for almost all kernel .c files, so this prerequisite should be satisfied
> automatically for them,

Earlier you asked not to include .  If  is a
prerequisite, why not just include it then?
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"


Re: [PATCH] Detect GNU/kFreeBSD in user-visible kernel headers (v2)

2011-11-26 Thread Bruce Evans

On Sat, 26 Nov 2011, Robert Millan wrote:


On Fri, Nov 25, 2011 at 11:16:15AM -0700, Warner Losh wrote:

Hey Bruce,

These sound like good suggestions, but I'd hoped to actually go through all 
these files with a fine-toothed comb to see which ones were still relevant.  
You've found a bunch of good areas to clean up, but I'd like to humbly suggest 
they be done in a follow-on commit.


Hi,

I'm sending a new patch.  Thanks Bruce for your input.  TTBOMK this corrects
all the problems you spotted that were introduced by my patch.  It doesn't
fix pre-existing problems in the files however, except in cases where I had
to modify that line anyway.

I think it's a good compromise between my initial patch and an exhaustive
cleanup of those headers (which I'm probably not the most indicate for).


It fixes most style bugs, but not some-pre-existing problems, even in cases
where you had to modify the line anyway.

% Index: sys/cam/scsi/scsi_low.h
% ===
% --- sys/cam/scsi/scsi_low.h   (revision 227956)
% +++ sys/cam/scsi/scsi_low.h   (working copy)
% @@ -53,10 +53,10 @@
%  #define  SCSI_LOW_INTERFACE_XS
%  #endif   /* __NetBSD__ */
% 
% -#ifdef	__FreeBSD__

% +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
%  #define  SCSI_LOW_INTERFACE_CAM
%  #define  CAM
% -#endif   /* __FreeBSD__ */
% +#endif /* __FreeBSD__ || __FreeBSD_kernel__ */

It still has the whitespace-after tab style change for cam.

% Index: sys/dev/firewire/firewirereg.h
% ===
% --- sys/dev/firewire/firewirereg.h(revision 227956)
% +++ sys/dev/firewire/firewirereg.h(working copy)
% @@ -75,7 +75,8 @@
%  };
% 
%  struct firewire_softc {

% -#if defined(__FreeBSD__) && __FreeBSD_version >= 50
% +#if (defined(__FreeBSD__) || defined(__FreeBSD_kernel__)) && \
% +__FreeBSD_version >= 50
%   struct cdev *dev;
%  #endif
%   struct firewire_comm *fc;

Here is a pre-existing problem that you didn't fix on a line that you
changed.  The __FreeBSD__ ifdef is nonsense here, since __FreeBSD__
being defined has nothing to do with either whether __FreeBSD_version
is defined or whether there is a struct cdev * in the data structure.

Previously:
- defined(__FreeBSD__) means that the compiler is for FreeBSD
- __FreeBSD_version >= 50 means that FreeBSD  has
  been included and has defined __FreeBSD_version to a value that
  satisifes this.  It would be a bug for anything else to define
  __FreeBSD_version.  Unfortunately, there is a bogus #undef of
  __FreeBSD_version that breaks detection of other things defining
  it.
- the __FreeBSD__ part of the test has no effect except to break
  compiling this file with a non-gcc compiler.  In particular,
  it doesn't prevent errors for -Wundef -Werror.  But other ifdefs
  in this file use an unguarded __FreeBSD_version.  Thus this file
  never worked with -Wundef -Werror, and the __FreeBSD__ part has
  no effect except the breakage.

Now: as above, except:
- defined(__FreeBSD_kernel__) means that FreeBSD 
  been included and that this header is new enough to define
  __FreeBSD_kernel__.  This has the same bug with the #undef,
  which I pointed out before (I noticed it for this but not
  for __FreeBSD_version).  And it has a style bug in its name
  which I pointed out before -- 2 underscores in its name.
  __FreeBSD_version doesn't have this style bug.  The definition
  of __FreeBSD_kernel__ has already been committed.  Is it too
  late to fix its name?
- when  is new enough to define __FreeBSD_kernel__,
  it must be new enough to define __FreeBSD_version >= 50.
  Thus there is now no -Wundef error.
- the __FreeBSD__ ifdef remains nonsense.  If you just removed it,
  then you wouldn't need the __FreeBSD_kernel__ ifdef (modulo the
  -Wundef error).  You didn't add the __FreeBSD_kernel__ ifdef to
  any of the other lines with the __FreeBSD_kernel__ ifdef in this
  file, apparently because the others don't have the nonsensical
  __FreeBSD__ ifdef.

The nonsense and changes to work around it make the logic for this
ifdef even more convoluted and broken than might first appear.  In
a previous patchset, you included  to ensure that
__FreeBSD_kernel__ is defined for newer kernel sources (instead of
testing if it is defined).  Ifdefs like the above make 
a prerequsite for this file anyway, since without knowing
__FreeBSD_version it is impossible to determine if the data structure
has new fields like the cdev in it.   is a prerequisite
for almost all kernel .c files, so this prerequisite should be satisfied
automatically for them, but it isn't clear what happens for user .c files.
I think the ifdef should be something like the following to enforce the
prerequisite:

#ifndef _SYS_PARAM_H_
/*
 * Here I don't support __FreeBSD_version__ to be set outside of
 *  to hack around a missing include of .
 * The case where the kernel is so old that __FreeBSD_

Re: [PATCH] Detect GNU/kFreeBSD in user-visible kernel headers (v2)

2011-11-26 Thread Warner Losh
This looks fine to me.

Warner

On Nov 26, 2011, at 1:07 PM, Robert Millan wrote:

> 

___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"


Re: [PATCH] Detect GNU/kFreeBSD in user-visible kernel headers (v2)

2011-11-26 Thread Robert Millan
On Fri, Nov 25, 2011 at 11:16:15AM -0700, Warner Losh wrote:
> Hey Bruce,
> 
> These sound like good suggestions, but I'd hoped to actually go through all 
> these files with a fine-toothed comb to see which ones were still relevant.  
> You've found a bunch of good areas to clean up, but I'd like to humbly 
> suggest they be done in a follow-on commit.

Hi,

I'm sending a new patch.  Thanks Bruce for your input.  TTBOMK this corrects
all the problems you spotted that were introduced by my patch.  It doesn't
fix pre-existing problems in the files however, except in cases where I had
to modify that line anyway.

I think it's a good compromise between my initial patch and an exhaustive
cleanup of those headers (which I'm probably not the most indicate for).

-- 
Robert Millan
Index: sys/cam/scsi/scsi_low.h
===
--- sys/cam/scsi/scsi_low.h	(revision 227956)
+++ sys/cam/scsi/scsi_low.h	(working copy)
@@ -53,10 +53,10 @@
 #define	SCSI_LOW_INTERFACE_XS
 #endif	/* __NetBSD__ */
 
-#ifdef	__FreeBSD__
+#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 #define	SCSI_LOW_INTERFACE_CAM
 #define	CAM
-#endif	/* __FreeBSD__ */
+#endif /* __FreeBSD__ || __FreeBSD_kernel__ */
 
 / includes ***/
 #ifdef	__NetBSD__
@@ -64,7 +64,7 @@
 #include 
 #endif	/* __NetBSD__ */
 
-#ifdef	__FreeBSD__
+#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 #include 
 #include 
 #include 
@@ -75,7 +75,7 @@
 
 #include 
 #include 
-#endif	/* __FreeBSD__ */
+#endif /* __FreeBSD__ || __FreeBSD_kernel__ */
 
 / functions macro /
 #ifdef	__NetBSD__
@@ -85,13 +85,13 @@
 #define	SCSI_LOW_BZERO(pt, size)	memset((pt), 0, (size))
 #endif	/* __NetBSD__ */
 
-#ifdef	__FreeBSD__
+#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 #undef	MSG_IDENTIFY
 #define	SCSI_LOW_DEBUGGER(dev)	kdb_enter(KDB_WHY_CAM, dev)
 #define	SCSI_LOW_DELAY(mu)	DELAY((mu))
 #define	SCSI_LOW_SPLSCSI	splcam
 #define	SCSI_LOW_BZERO(pt, size)	bzero((pt), (size))
-#endif	/* __FreeBSD__ */
+#endif /* __FreeBSD__ || __FreeBSD_kernel__ */
 
 / os depend interface structures **/
 #ifdef	__NetBSD__
@@ -111,7 +111,7 @@
 };
 #endif	/* __NetBSD__ */
 
-#ifdef	__FreeBSD__
+#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 typedef	struct scsi_sense_data scsi_low_osdep_sense_data_t;
 
 struct scsi_low_osdep_interface {
@@ -134,7 +134,7 @@
 
 struct scsi_low_osdep_lun_interface {
 };
-#endif	/* __FreeBSD__ */
+#endif /* __FreeBSD__ || __FreeBSD_kernel__ */
 
 / os depend interface functions */
 struct slccb;
Index: sys/cam/scsi/scsi_low_pisa.h
===
--- sys/cam/scsi/scsi_low_pisa.h	(revision 227956)
+++ sys/cam/scsi/scsi_low_pisa.h	(working copy)
@@ -40,8 +40,8 @@
 int scsi_low_notify_pisa(pisa_device_handle_t, pisa_event_t);
 #endif	/* __NetBSD__ */
 
-#ifdef	__FreeBSD__
+#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 int scsi_low_activate_pisa(struct scsi_low_softc *, int);
 int scsi_low_deactivate_pisa(struct scsi_low_softc *);
-#endif	/* __FreeBSD__ */
+#endif /* __FreeBSD__ || __FreeBSD_kernel__ */
 #endif	/* !_SCSI_LOW_PISA_H_ */
Index: sys/contrib/altq/altq/altq_var.h
===
--- sys/contrib/altq/altq/altq_var.h	(revision 227956)
+++ sys/contrib/altq/altq/altq_var.h	(working copy)
@@ -201,7 +201,7 @@
 #define	CALLOUT_STOP(c)		untimeout((c)->c_func,(c)->c_arg)
 #define	CALLOUT_INITIALIZER	{ NULL, NULL }
 #endif
-#if !defined(__FreeBSD__)
+#if !defined(__FreeBSD__) && !defined(__FreeBSD_kernel__)
 typedef void (timeout_t)(void *);
 #endif
 
Index: sys/contrib/altq/altq/if_altq.h
===
--- sys/contrib/altq/altq/if_altq.h	(revision 227956)
+++ sys/contrib/altq/altq/if_altq.h	(working copy)
@@ -29,7 +29,7 @@
 #ifndef _ALTQ_IF_ALTQ_H_
 #define	_ALTQ_IF_ALTQ_H_
 
-#ifdef __FreeBSD__
+#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 #include 		/* XXX */
 #include 		/* XXX */
 #include 		/* XXX */
@@ -51,7 +51,7 @@
 	int	ifq_len;
 	int	ifq_maxlen;
 	int	ifq_drops;
-#ifdef __FreeBSD__
+#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 	struct	mtx ifq_mtx;
 #endif
 
Index: sys/contrib/pf/net/if_pflog.h
===
--- sys/contrib/pf/net/if_pflog.h	(revision 227956)
+++ sys/contrib/pf/net/if_pflog.h	(working copy)
@@ -30,7 +30,7 @@
 #define	PFLOGIFS_MAX	16
 
 struct pflog_softc {
-#ifdef __FreeBSD__
+#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 	struct ifnet		*sc_ifp;	/* the interface pointer */
 #else
 	struct ifnet		sc_if;		/* the interface */
@@ -74,7 +74,7 @@
 #define	OLD_PFLOG_HDRLEN	sizeof(struct old_pfloghdr)
 
 #ifdef _KERNEL
-#ifdef __FreeBSD__
+#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 struct pf_rule;
 struct pf_rule

Re: [PATCH] Detect GNU/kFreeBSD in user-visible kernel headers (v2)

2011-11-25 Thread Warner Losh
Hey Bruce,

These sound like good suggestions, but I'd hoped to actually go through all 
these files with a fine-toothed comb to see which ones were still relevant.  
You've found a bunch of good areas to clean up, but I'd like to humbly suggest 
they be done in a follow-on commit.

Warner

On Nov 24, 2011, at 10:54 PM, Bruce Evans wrote:

> On Thu, 24 Nov 2011, Robert Millan wrote:
> 
>> 2011/11/24 Bruce Evans :
>>> Now it adds lots of namespace pollution (all of , including
>>> all of its namespace pollution), just to get 1 new symbol defined.
>> 
>> Well, my initial patch (see mail with same subject modulo "v2") didn't
>> have this problem.  Now that __FreeBSD_kernel__ is defined, many
>> #ifdefs can be simplified, but maybe it's not desireable for all of
>> them.  At least not until we can rely on the compiler to define this
>> macro.
>> 
>> So in this particular case maybe it's better to use the other approach?
>> 
>> See attachment.
> 
> That is clean enough, except for some style bugs.  (I thought of worse
> ways like duplicating the logic of , or directing
>  to only declare version macros, or putting version macros
> in a little separate param header and including that.  The latter would
> be cleanest, but gives even more includes, and not worth it for this,
> but it would have been better for __FreeBSD_version.  I don't like
> having to recompile half the universe according to dependencies on
>  because only __FreeBSD_version__ in it changed.  Basic
> headers rarely change apart from that.  BTW, a recent discussion in
> the POSIX mailing list says that standardized generation of depenedencies
> should not generate dependencies on system headers.  This would break
> the effect of putting mistakes like __FreeBSD_version__ in any system
> header :-).)
> 
> % diff -ur sys.old/cam/scsi/scsi_low.h sys/cam/scsi/scsi_low.h
> % --- sys.old/cam/scsi/scsi_low.h 2007-12-25 18:52:02.0 +0100
> % +++ sys/cam/scsi/scsi_low.h 2011-11-13 14:12:41.121908380 +0100
> % @@ -53,7 +53,7 @@
> %  #defineSCSI_LOW_INTERFACE_XS
> %  #endif /* __NetBSD__ */
> % % -#ifdef   __FreeBSD__
> % +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
> %  #defineSCSI_LOW_INTERFACE_CAM
> %  #defineCAM
> %  #endif /* __FreeBSD__ */
> 
> This also fixes some style bugs (tab instead of space after `#ifdef').
> But it doesn't fix others (tab instead of space after `#ifdef', and
> comment on a short ifdef).  And it introduces a new one (the comment
> on the ifdef now doesn't even match the code).
> 
> cam has a highly non-KNF style, so it may require all of these style
> bugs except the comment not matching the code.  This makes it hard
> for non-cam programmers to maintain.  According to grep, it prefers
> a tab to a space after `#ifdef' by a ratio of 89:38 in a version
> checked out a year or two ago.  But in 9.0-BETA1, the counts have
> blown out and the ratio has reduced to 254:221.  The counts are
> more than doubled because the first version is a cvs checkout and
> the second version is a svn checkout, and it is too hard to filter
> out the svn duplicates.  I guess the ratio changed because the new
> ata subsystem is not bug for bug compatible with cam style.  Anywyay,
> there never was a consistent cam style to match.
> 
> % @@ -64,7 +64,7 @@
> %  #include 
> %  #endif /* __NetBSD__ */
> % % -#ifdef   __FreeBSD__
> % +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
> %  #include 
> %  #include 
> %  #include 
> 
> Same problems, but now the ifdef is larger (but not large enough to
> need a comment on its endif), so the inconsistent comment is not
> visible in the patch.
> 
> % [... similarly throught cam]
> 
> % diff -ur sys.old/contrib/altq/altq/if_altq.h sys/contrib/altq/altq/if_altq.h
> % --- sys.old/contrib/altq/altq/if_altq.h 2011-03-10 19:49:15.0 
> +0100
> % +++ sys/contrib/altq/altq/if_altq.h 2011-11-13 14:12:41.119907128 +0100
> % @@ -29,7 +29,7 @@
> %  #ifndef _ALTQ_IF_ALTQ_H_
> %  #define_ALTQ_IF_ALTQ_H_
> % % -#ifdef __FreeBSD__
> % +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
> %  #include   /* XXX */
> %  #include  /* XXX */
> %  #include  /* XXX */
> % @@ -51,7 +51,7 @@
> % int ifq_len;
> % int ifq_maxlen;
> % int ifq_drops;
> % -#ifdef __FreeBSD__
> % +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
> % struct  mtx ifq_mtx;
> %  #endif
> %
> 
> No new problems, but I wonder how this even compiles when the ifdefs
> are not satisfed.  Here we are exporting mounds of kernel data structures
> to userland.  There is a similar mess in .  There it has
> no ifdefs at all for the lock, mutex and event headers there, and you
> didn't touch it.   is unfortunately actually needed in
> userland.  The mutexes in its data structures cannot simply be left
> out, since then the data structures become incompatible with the actual
> ones.  I don't see how the above can work with the mutex lef

Re: [PATCH] Detect GNU/kFreeBSD in user-visible kernel headers (v2)

2011-11-25 Thread Bruce Evans

On Thu, 24 Nov 2011, Robert Millan wrote:


2011/11/24 Bruce Evans :

Now it adds lots of namespace pollution (all of , including
all of its namespace pollution), just to get 1 new symbol defined.


Well, my initial patch (see mail with same subject modulo "v2") didn't
have this problem.  Now that __FreeBSD_kernel__ is defined, many
#ifdefs can be simplified, but maybe it's not desireable for all of
them.  At least not until we can rely on the compiler to define this
macro.

So in this particular case maybe it's better to use the other approach?

See attachment.


That is clean enough, except for some style bugs.  (I thought of worse
ways like duplicating the logic of , or directing
 to only declare version macros, or putting version macros
in a little separate param header and including that.  The latter would
be cleanest, but gives even more includes, and not worth it for this,
but it would have been better for __FreeBSD_version.  I don't like
having to recompile half the universe according to dependencies on
 because only __FreeBSD_version__ in it changed.  Basic
headers rarely change apart from that.  BTW, a recent discussion in
the POSIX mailing list says that standardized generation of depenedencies
should not generate dependencies on system headers.  This would break
the effect of putting mistakes like __FreeBSD_version__ in any system
header :-).)

% diff -ur sys.old/cam/scsi/scsi_low.h sys/cam/scsi/scsi_low.h
% --- sys.old/cam/scsi/scsi_low.h   2007-12-25 18:52:02.0 +0100
% +++ sys/cam/scsi/scsi_low.h   2011-11-13 14:12:41.121908380 +0100
% @@ -53,7 +53,7 @@
%  #define  SCSI_LOW_INTERFACE_XS
%  #endif   /* __NetBSD__ */
% 
% -#ifdef	__FreeBSD__

% +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
%  #define  SCSI_LOW_INTERFACE_CAM
%  #define  CAM
%  #endif   /* __FreeBSD__ */

This also fixes some style bugs (tab instead of space after `#ifdef').
But it doesn't fix others (tab instead of space after `#ifdef', and
comment on a short ifdef).  And it introduces a new one (the comment
on the ifdef now doesn't even match the code).

cam has a highly non-KNF style, so it may require all of these style
bugs except the comment not matching the code.  This makes it hard
for non-cam programmers to maintain.  According to grep, it prefers
a tab to a space after `#ifdef' by a ratio of 89:38 in a version
checked out a year or two ago.  But in 9.0-BETA1, the counts have
blown out and the ratio has reduced to 254:221.  The counts are
more than doubled because the first version is a cvs checkout and
the second version is a svn checkout, and it is too hard to filter
out the svn duplicates.  I guess the ratio changed because the new
ata subsystem is not bug for bug compatible with cam style.  Anywyay,
there never was a consistent cam style to match.

% @@ -64,7 +64,7 @@
%  #include 
%  #endif   /* __NetBSD__ */
% 
% -#ifdef	__FreeBSD__

% +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
%  #include 
%  #include 
%  #include 

Same problems, but now the ifdef is larger (but not large enough to
need a comment on its endif), so the inconsistent comment is not
visible in the patch.

% [... similarly throught cam]

% diff -ur sys.old/contrib/altq/altq/if_altq.h sys/contrib/altq/altq/if_altq.h
% --- sys.old/contrib/altq/altq/if_altq.h   2011-03-10 19:49:15.0 
+0100
% +++ sys/contrib/altq/altq/if_altq.h   2011-11-13 14:12:41.119907128 +0100
% @@ -29,7 +29,7 @@
%  #ifndef _ALTQ_IF_ALTQ_H_
%  #define  _ALTQ_IF_ALTQ_H_
% 
% -#ifdef __FreeBSD__

% +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
%  #include   /* XXX */
%  #include  /* XXX */
%  #include  /* XXX */
% @@ -51,7 +51,7 @@
%   int ifq_len;
%   int ifq_maxlen;
%   int ifq_drops;
% -#ifdef __FreeBSD__
% +#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
%   struct  mtx ifq_mtx;
%  #endif
%

No new problems, but I wonder how this even compiles when the ifdefs
are not satisfed.  Here we are exporting mounds of kernel data structures
to userland.  There is a similar mess in .  There it has
no ifdefs at all for the lock, mutex and event headers there, and you
didn't touch it.   is unfortunately actually needed in
userland.  The mutexes in its data structures cannot simply be left
out, since then the data structures become incompatible with the actual
ones.  I don't see how the above can work with the mutex left out.

By "not even compiles", I meant the header itself, but there should be
no problems there because the second ifdef should kill the only use of
all the headers.  And userland should compile since it shouldn't use
the ifdefed out (kernel) parts of the data struct.  But leaving out
the data substructures changes the ABI, so how could any application that
actually uses the full structure work?  And if nothing uses it, it
shouldn't be exported.

Exporting the full pollution of sys/lock.h, sys/mutex.h and sys/event.h
to userland is p

Re: [PATCH] Detect GNU/kFreeBSD in user-visible kernel headers (v2)

2011-11-24 Thread Robert Millan
Hi Bruce,

2011/11/24 Bruce Evans :
> Now it adds lots of namespace pollution (all of , including
> all of its namespace pollution), just to get 1 new symbol defined.

Well, my initial patch (see mail with same subject modulo "v2") didn't
have this problem.  Now that __FreeBSD_kernel__ is defined, many
#ifdefs can be simplified, but maybe it's not desireable for all of
them.  At least not until we can rely on the compiler to define this
macro.

So in this particular case maybe it's better to use the other approach?

See attachment.
diff -ur sys.old/cam/scsi/scsi_low.h sys/cam/scsi/scsi_low.h
--- sys.old/cam/scsi/scsi_low.h	2007-12-25 18:52:02.0 +0100
+++ sys/cam/scsi/scsi_low.h	2011-11-13 14:12:41.121908380 +0100
@@ -53,7 +53,7 @@
 #define	SCSI_LOW_INTERFACE_XS
 #endif	/* __NetBSD__ */
 
-#ifdef	__FreeBSD__
+#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 #define	SCSI_LOW_INTERFACE_CAM
 #define	CAM
 #endif	/* __FreeBSD__ */
@@ -64,7 +64,7 @@
 #include 
 #endif	/* __NetBSD__ */
 
-#ifdef	__FreeBSD__
+#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 #include 
 #include 
 #include 
@@ -85,7 +85,7 @@
 #define	SCSI_LOW_BZERO(pt, size)	memset((pt), 0, (size))
 #endif	/* __NetBSD__ */
 
-#ifdef	__FreeBSD__
+#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 #undef	MSG_IDENTIFY
 #define	SCSI_LOW_DEBUGGER(dev)	kdb_enter(KDB_WHY_CAM, dev)
 #define	SCSI_LOW_DELAY(mu)	DELAY((mu))
@@ -111,7 +111,7 @@
 };
 #endif	/* __NetBSD__ */
 
-#ifdef	__FreeBSD__
+#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 typedef	struct scsi_sense_data scsi_low_osdep_sense_data_t;
 
 struct scsi_low_osdep_interface {
diff -ur sys.old/cam/scsi/scsi_low_pisa.h sys/cam/scsi/scsi_low_pisa.h
--- sys.old/cam/scsi/scsi_low_pisa.h	2005-01-05 23:34:37.0 +0100
+++ sys/cam/scsi/scsi_low_pisa.h	2011-11-13 14:12:41.121908380 +0100
@@ -40,7 +40,7 @@
 int scsi_low_notify_pisa(pisa_device_handle_t, pisa_event_t);
 #endif	/* __NetBSD__ */
 
-#ifdef	__FreeBSD__
+#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 int scsi_low_activate_pisa(struct scsi_low_softc *, int);
 int scsi_low_deactivate_pisa(struct scsi_low_softc *);
 #endif	/* __FreeBSD__ */
diff -ur sys.old/contrib/altq/altq/altq_var.h sys/contrib/altq/altq/altq_var.h
--- sys.old/contrib/altq/altq/altq_var.h	2011-03-10 19:49:15.0 +0100
+++ sys/contrib/altq/altq/altq_var.h	2011-11-13 14:12:41.118907341 +0100
@@ -201,7 +201,7 @@
 #define	CALLOUT_STOP(c)		untimeout((c)->c_func,(c)->c_arg)
 #define	CALLOUT_INITIALIZER	{ NULL, NULL }
 #endif
-#if !defined(__FreeBSD__)
+#if !defined(__FreeBSD__) && !defined(__FreeBSD_kernel__)
 typedef void (timeout_t)(void *);
 #endif
 
diff -ur sys.old/contrib/altq/altq/if_altq.h sys/contrib/altq/altq/if_altq.h
--- sys.old/contrib/altq/altq/if_altq.h	2011-03-10 19:49:15.0 +0100
+++ sys/contrib/altq/altq/if_altq.h	2011-11-13 14:12:41.119907128 +0100
@@ -29,7 +29,7 @@
 #ifndef _ALTQ_IF_ALTQ_H_
 #define	_ALTQ_IF_ALTQ_H_
 
-#ifdef __FreeBSD__
+#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 #include 		/* XXX */
 #include 		/* XXX */
 #include 		/* XXX */
@@ -51,7 +51,7 @@
 	int	ifq_len;
 	int	ifq_maxlen;
 	int	ifq_drops;
-#ifdef __FreeBSD__
+#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 	struct	mtx ifq_mtx;
 #endif
 
diff -ur sys.old/contrib/pf/net/if_pflog.h sys/contrib/pf/net/if_pflog.h
--- sys.old/contrib/pf/net/if_pflog.h	2011-06-28 13:57:25.0 +0200
+++ sys/contrib/pf/net/if_pflog.h	2011-11-13 14:12:41.130906469 +0100
@@ -30,7 +30,7 @@
 #define	PFLOGIFS_MAX	16
 
 struct pflog_softc {
-#ifdef __FreeBSD__
+#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 	struct ifnet		*sc_ifp;	/* the interface pointer */
 #else
 	struct ifnet		sc_if;		/* the interface */
@@ -74,7 +74,7 @@
 #define	OLD_PFLOG_HDRLEN	sizeof(struct old_pfloghdr)
 
 #ifdef _KERNEL
-#ifdef __FreeBSD__
+#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 struct pf_rule;
 struct pf_ruleset;
 struct pfi_kif;
diff -ur sys.old/contrib/pf/net/if_pflow.h sys/contrib/pf/net/if_pflow.h
--- sys.old/contrib/pf/net/if_pflow.h	2011-06-28 13:57:25.0 +0200
+++ sys/contrib/pf/net/if_pflow.h	2011-11-13 14:12:41.130906469 +0100
@@ -66,7 +66,7 @@
 	unsigned int		 sc_maxcount;
 	u_int64_t		 sc_gcounter;
 	struct ip_moptions	 sc_imo;
-#ifdef __FreeBSD__
+#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 	struct callout		 sc_tmo;
 #else
 	struct timeout		 sc_tmo;
diff -ur sys.old/contrib/pf/net/if_pfsync.h sys/contrib/pf/net/if_pfsync.h
--- sys.old/contrib/pf/net/if_pfsync.h	2011-06-28 13:57:25.0 +0200
+++ sys/contrib/pf/net/if_pfsync.h	2011-11-13 14:12:41.131906257 +0100
@@ -268,7 +268,7 @@
 	int		 pfsyncr_authlevel;
 };
 
-#ifdef __FreeBSD__
+#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 #define	SIOCSETPFSYNC   _IOW('i', 247, struct ifreq)
 #define	SIOCGETPFSYNC   _IOWR('i', 248, struct ifreq)
 #endif
@@ -288,7 +288,7 @@
 #define	PFSYNC_S_DEFER	0xfe
 #define	PFSYNC_S_NONE	0xff
 
-#ifdef __

Re: [PATCH] Detect GNU/kFreeBSD in user-visible kernel headers (v2)

2011-11-23 Thread Bruce Evans

On Wed, 23 Nov 2011, Robert Millan wrote:


Here we go again :-)

Out of the kernel headers that are installed in /usr/include/ hierracy, there
are some which include support multiple operating systems (usually FreeBSD and
other *BSD flavours).

This patch adds support to detect GNU/kFreeBSD as well.  In all cases, we
match the same declarations as FreeBSD does (which is to be expected in kernel
headers, since both systems share the same kernel).


Now it adds lots of namespace pollution (all of , including
all of its namespace pollution), just to get 1 new symbol defined.

% Index: sys/cam/scsi/scsi_low.h
% ===
% --- sys/cam/scsi/scsi_low.h   (revision 227831)
% +++ sys/cam/scsi/scsi_low.h   (working copy)
% @@ -44,6 +44,8 @@
%  #ifndef  _SCSI_LOW_H_
%  #define  _SCSI_LOW_H_
% 
% +#include 

% +
%  /*
%   * Scsi low OSDEP 
%   * (All os depend structures should be here!)
% 
% [... 22 more headers polluted]


All the affected headers are poorly implemented ones.  Mostly kernel
headers which escaped to userland.

Bruce
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"


[PATCH] Detect GNU/kFreeBSD in user-visible kernel headers (v2)

2011-11-22 Thread Robert Millan

Here we go again :-)

Out of the kernel headers that are installed in /usr/include/ hierracy, there
are some which include support multiple operating systems (usually FreeBSD and
other *BSD flavours).

This patch adds support to detect GNU/kFreeBSD as well.  In all cases, we
match the same declarations as FreeBSD does (which is to be expected in kernel
headers, since both systems share the same kernel).

-- 
Robert Millan
Index: sys/cam/scsi/scsi_low.h
===
--- sys/cam/scsi/scsi_low.h	(revision 227831)
+++ sys/cam/scsi/scsi_low.h	(working copy)
@@ -44,6 +44,8 @@
 #ifndef	_SCSI_LOW_H_
 #define	_SCSI_LOW_H_
 
+#include 
+
 /*
  * Scsi low OSDEP 
  * (All os depend structures should be here!)
@@ -53,10 +55,10 @@
 #define	SCSI_LOW_INTERFACE_XS
 #endif	/* __NetBSD__ */
 
-#ifdef	__FreeBSD__
+#ifdef	__FreeBSD_kernel__
 #define	SCSI_LOW_INTERFACE_CAM
 #define	CAM
-#endif	/* __FreeBSD__ */
+#endif	/* __FreeBSD_kernel__ */
 
 / includes ***/
 #ifdef	__NetBSD__
@@ -64,7 +66,7 @@
 #include 
 #endif	/* __NetBSD__ */
 
-#ifdef	__FreeBSD__
+#ifdef	__FreeBSD_kernel__
 #include 
 #include 
 #include 
@@ -75,7 +77,7 @@
 
 #include 
 #include 
-#endif	/* __FreeBSD__ */
+#endif	/* __FreeBSD_kernel__ */
 
 / functions macro /
 #ifdef	__NetBSD__
@@ -85,13 +87,13 @@
 #define	SCSI_LOW_BZERO(pt, size)	memset((pt), 0, (size))
 #endif	/* __NetBSD__ */
 
-#ifdef	__FreeBSD__
+#ifdef	__FreeBSD_kernel__
 #undef	MSG_IDENTIFY
 #define	SCSI_LOW_DEBUGGER(dev)	kdb_enter(KDB_WHY_CAM, dev)
 #define	SCSI_LOW_DELAY(mu)	DELAY((mu))
 #define	SCSI_LOW_SPLSCSI	splcam
 #define	SCSI_LOW_BZERO(pt, size)	bzero((pt), (size))
-#endif	/* __FreeBSD__ */
+#endif	/* __FreeBSD_kernel__ */
 
 / os depend interface structures **/
 #ifdef	__NetBSD__
@@ -111,7 +113,7 @@
 };
 #endif	/* __NetBSD__ */
 
-#ifdef	__FreeBSD__
+#ifdef	__FreeBSD_kernel__
 typedef	struct scsi_sense_data scsi_low_osdep_sense_data_t;
 
 struct scsi_low_osdep_interface {
@@ -134,7 +136,7 @@
 
 struct scsi_low_osdep_lun_interface {
 };
-#endif	/* __FreeBSD__ */
+#endif	/* __FreeBSD_kernel__ */
 
 / os depend interface functions */
 struct slccb;
Index: sys/cam/scsi/scsi_low_pisa.h
===
--- sys/cam/scsi/scsi_low_pisa.h	(revision 227831)
+++ sys/cam/scsi/scsi_low_pisa.h	(working copy)
@@ -34,14 +34,16 @@
 #ifndef	_SCSI_LOW_PISA_H_
 #define	_SCSI_LOW_PISA_H_
 
+#include 
+
 #ifdef	__NetBSD__
 int scsi_low_activate_pisa(pisa_device_handle_t);
 int scsi_low_deactivate_pisa(pisa_device_handle_t);
 int scsi_low_notify_pisa(pisa_device_handle_t, pisa_event_t);
 #endif	/* __NetBSD__ */
 
-#ifdef	__FreeBSD__
+#ifdef	__FreeBSD_kernel__
 int scsi_low_activate_pisa(struct scsi_low_softc *, int);
 int scsi_low_deactivate_pisa(struct scsi_low_softc *);
-#endif	/* __FreeBSD__ */
+#endif	/* __FreeBSD_kernel__ */
 #endif	/* !_SCSI_LOW_PISA_H_ */
Index: sys/contrib/altq/altq/altq_var.h
===
--- sys/contrib/altq/altq/altq_var.h	(revision 227831)
+++ sys/contrib/altq/altq/altq_var.h	(working copy)
@@ -201,7 +201,7 @@
 #define	CALLOUT_STOP(c)		untimeout((c)->c_func,(c)->c_arg)
 #define	CALLOUT_INITIALIZER	{ NULL, NULL }
 #endif
-#if !defined(__FreeBSD__)
+#if !defined(__FreeBSD_kernel__)
 typedef void (timeout_t)(void *);
 #endif
 
Index: sys/contrib/altq/altq/if_altq.h
===
--- sys/contrib/altq/altq/if_altq.h	(revision 227831)
+++ sys/contrib/altq/altq/if_altq.h	(working copy)
@@ -29,7 +29,9 @@
 #ifndef _ALTQ_IF_ALTQ_H_
 #define	_ALTQ_IF_ALTQ_H_
 
-#ifdef __FreeBSD__
+#include 
+
+#ifdef __FreeBSD_kernel__
 #include 		/* XXX */
 #include 		/* XXX */
 #include 		/* XXX */
@@ -51,7 +53,7 @@
 	int	ifq_len;
 	int	ifq_maxlen;
 	int	ifq_drops;
-#ifdef __FreeBSD__
+#ifdef __FreeBSD_kernel__
 	struct	mtx ifq_mtx;
 #endif
 
Index: sys/contrib/pf/net/if_pflog.h
===
--- sys/contrib/pf/net/if_pflog.h	(revision 227831)
+++ sys/contrib/pf/net/if_pflog.h	(working copy)
@@ -27,10 +27,12 @@
 #ifndef _NET_IF_PFLOG_H_
 #define	_NET_IF_PFLOG_H_
 
+#include 
+
 #define	PFLOGIFS_MAX	16
 
 struct pflog_softc {
-#ifdef __FreeBSD__
+#ifdef __FreeBSD_kernel__
 	struct ifnet		*sc_ifp;	/* the interface pointer */
 #else
 	struct ifnet		sc_if;		/* the interface */
@@ -74,7 +76,7 @@
 #define	OLD_PFLOG_HDRLEN	sizeof(struct old_pfloghdr)
 
 #ifdef _KERNEL
-#ifdef __FreeBSD__
+#ifdef __FreeBSD_kernel__
 struct pf_rule;
 struct pf_ruleset;
 struct pfi_kif;
@@ -90,7 +92,7 @@
 	if (pflog_packet_ptr != NULL)			\
 		pflog_packet_ptr(i,a,b,c,d,e,f,g,h);\
 } while (0)
-#else /* ! __FreeBSD__ */
+#else /* ! __FreeBSD_kernel__ */
 #if NPFLOG > 0
 #define	PFLOG_PACKET(i,x,a,b,c,d,e,f,g,h) pf