Bug 8298

Created attachment 7003 [details] [review]
dbus-0.93-fbsd.patch

Created attachment 7283 [details] [review]
dbus-0.93-fbsd.patch

New patch, the only difference is the check for dirfd (now also if it`s a macro
it will be defined in config.h)

- the bus.c change I don't understand - it's definitely wrong (there should 
not be any platform ifdefs outside of the sysdeps.c files) but I also don't 
know the motivation so can't suggest a better approach
- adding -lpthread to selinux_libs seems odd - what's the reasoning here?
- the #undef LOCAL_CREDS at least needs a comment explaining it (I have no 
guess, but even if  did, the code should have docs on this)

I'm committing the dirfp portion of the patch.  

Things that need fixing up:

* Local creds needs a new patch with a comment on why it is undefined.  
* The bus.c platform macro needs to be put in the correct watch file
* -lpthreads on SELinux libs needs explaining.  Actually I think that is just a
bug in the patch since FreeBSD don't have an SELinux implementation.

If you can get me a patch by Friday that would be great because I am planning on
doing an RC2 release.

(In reply to comment #4)
> I'm committing the dirfp portion of the patch.  
> 
> Things that need fixing up:
> 
> * Local creds needs a new patch with a comment on why it is undefined.

FreeBSD works better with cmsgcred (with local creds i cannot make it works)
 
> * The bus.c platform macro needs to be put in the correct watch file

Can you explain how can i do if context is not passed to bus_watch_directory?
In teory you can only pass context to _dbus_list_foreach replacing NULL 
(without ifdef, dnotify will ignore it)

> * -lpthreads on SELinux libs needs explaining.  Actually I think that is just 
a
> bug in the patch since FreeBSD don't have an SELinux implementation.

/bin/bash ../libtool --tag=CC --mode=link 
gcc  -g -O2 -Wall -Wchar-subscripts -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wcast-align -Wsign-compare -Wdeclaration-after-statement -fno-common    -o 
dbus-test -export-dynamic dbus-test-main.o libdbus-convenience.la
gcc -g -O2 -Wall -Wchar-subscripts -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wcast-align -Wsign-compare -Wdeclaration-after-statement -fno-common -o 
dbus-test dbus-test-main.o -Wl,--export-dynamic  ./.libs/libdbus-convenience.a
./.libs/libdbus-convenience.a(dbus-threads.o): In function 
`_dbus_internal_condvar_wait_timeout':
/home/drizzt/dbus/dbus/dbus-threads.c:927: undefined reference to 
`pthread_cond_timedwait'
collect2: ld returned 1 exit status

pthread is used (dbus-threads.c) and should be linked ever, not only in selinux

> If you can get me a patch by Friday that would be great because I am planning 
on
> doing an RC2 release.

If you reply soon i'll try

> (In reply to comment #4) 
> > * Local creds needs a new patch with a comment on why it is undefined.
> 
> FreeBSD works better with cmsgcred (with local creds i cannot make it works)

Just need a patch with that comment above the #undef

> > * The bus.c platform macro needs to be put in the correct watch file
> 
> Can you explain how can i do if context is not passed to bus_watch_directory?
> In teory you can only pass context to _dbus_list_foreach replacing NULL 
> (without ifdef, dnotify will ignore it)

If bus_context_get_loop is on all platforms then just send it in (assuming you
don't have to later free it).  Actually I would send in the context and let the
platform code do what it wants with it.

> > * -lpthreads on SELinux libs needs explaining.  Actually I think that is just 
> a
> > bug in the patch since FreeBSD don't have an SELinux implementation.
> 
> /bin/bash ../libtool --tag=CC --mode=link 
> gcc  -g -O2 -Wall -Wchar-subscripts -Wmissing-declarations
-Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wcast-align
-Wsign-compare -Wdeclaration-after-statement -fno-common    -o 
> dbus-test -export-dynamic dbus-test-main.o libdbus-convenience.la
> gcc -g -O2 -Wall -Wchar-subscripts -Wmissing-declarations -Wmissing-prototypes
-Wnested-externs -Wpointer-arith -Wcast-align -Wsign-compare
-Wdeclaration-after-statement -fno-common -o 
> dbus-test dbus-test-main.o -Wl,--export-dynamic  ./.libs/libdbus-convenience.a
> ./.libs/libdbus-convenience.a(dbus-threads.o): In function 
> `_dbus_internal_condvar_wait_timeout':
> /home/drizzt/dbus/dbus/dbus-threads.c:927: undefined reference to 
> `pthread_cond_timedwait'
> collect2: ld returned 1 exit status
> 
> pthread is used (dbus-threads.c) and should be linked ever, not only in selinux

I added -lpthreads already to a couple of places.  It should be linked in the
correct places and not randomly.  So it should only be linked to libdbus and
executables which use libdbus (assuming the linker is not smart enough to see
that libdbus is linked to pthreads so the executable should be too). In fact it
should be added to the pkg-config Libs: section if the platform's linker can not
automaticly link pthreads.

If you can can you make these three seperate patches.  Thanks.

I think a nicer fix may be in order for LOCAL_CREDS, the current code seems 
potentially screwed up and definitely hard to maintain. There are lots of
things like:
 #if defined(HAVE_CMSGCRED) && !defined(LOCAL_CREDS)
but it looks to me like on a system with both, we may well use the code for 
CMSGCRED part of the time and the code for LOCAL_CREDS part of the time. Which 
could be why LOCAL_CREDS isn't working for you on freebsd.

So my suggestion would be that a) rather than looking at defined(LOCAL_CREDS) 
we add a HAVE_LOCAL_CREDS to config.h b) we have configure.in ensure that only 
one of HAVE_CMSGCRED / HAVE_LOCAL_CREDS is ever defined c) we write the code 
consistently as:
 #ifdef HAVE_CMSGCRED
 #else
 #  ifdef HAVE_LOCAL_CREDS
 #  endif
 #endif

(or when appropriate, as just #ifdef one or the other). The point is be sure 
they are mutually exclusive.

I don't know which one should have priority. I think the LOCAL_CREDS API is a 
little nicer, no? So if after cleaning this up, LOCAL_CREDS starts working on 
BSD, I'd probably give it priority. The priority should be chosen in 
configure.in, not at each individual point in the code. i.e. configure.in 
should guarantee only one of the two is defined.

Something like this might be good too:
 #if defined(HAVE_LOCAL_CREDS) && defined(HAVE_CMSGCRED)
 #error "should not have defined both local creds and cmsgcred"
 #endif

I took care of passing in the context so there are two more patches here that
are holding up 1.0.  The -lpthread patch and rearanging the CMSGCRED and
LOCAL_CREDS macros.  Both of which I can not test.

I took care of the rest of teh bugs but the creds stuff still needs to be
cleaned up (look at doc/TODO).

Created attachment 7659 [details] [review]
dbus-0.95-fbsd.patch

You forgot (i think) to pass context to bus_watch_directory.
Appling this patch it will works fine :)

I must have missed that file when committing.  It is committed now.  Thanks.