shall libreplace become a public library (Was: Re: [SCM] Samba Shared Repository - branch master updated)

Fri Nov 29 15:16:18 MST 2013

On Fri, Nov 29, 2013 at 10:56:07PM +0100, Christian Ambach wrote:
> Am 26.11.13 23:57, schrieb Andrew Bartlett:
> >>lib/replace/system/filesys.h also unconditionally includes <sys/stat.h>,
> >>so I did not see the need to use the replace header.
> >Yes, we should avoid those, but we should also always use the libreplace
> >headers.
> 
> Unfortunately, it is not possible to do this in all spots.
> In the attached patchset, I have changed a lot of files to eliminate
> inconsistencies.
> 
> However, I failed for the file that the thread was started for.
> In order to remove the include in lib/util/samba_util.h, which is part
> of the public library samba-util, I would have to add a dependency of
> this library to replace, which is not a public library yet.
> Or is there another way to make this work (that I missed)?
Hmm, that's a good point. That's a good reason for still including
<sys/stat.h> directly here. It's a pity we can't just forward
declare "struct stat;". Sorry for the noise. :-(

> So what shall we do? Is it worth the efforts (and do we really want to
> expose replace as a public library) to fix this inconsistency?
> Or shall we leave the affected two header files of two of our public
> libraries (see list below) in their current state?
I think we should leave it as is - just "#include <sys/stat.h>".
libreplace is really meant as glue, it doesn't make
sense to provide it as a shared library.

libreplace isn't the best place for the system/*.h headers -
libreplace should focus on just providing APIs we would expect from libc and
friends - but that's how it is at the moment.

Thanks for doing this janitorial work!

> From f6195d2cc4318a08a7ac74e7acc41a7f7c1f5b8f Mon Sep 17 00:00:00 2001
> From: Christian Ambach <ambi at samba.org>
> Date: Fri, 22 Nov 2013 05:09:08 +0100
> Subject: [PATCH 01/10] s3:utils remove orphaned code
> 
> this does not even compile at all.. looks like a real orphan

Reviewed-By: Jelmer Vernooij <jelmer at samba.org>

> From 55a26402981fd2b13bffa1ac978bdacd5303a09b Mon Sep 17 00:00:00 2001
> From: Christian Ambach <ambi at samba.org>
> Date: Fri, 22 Nov 2013 05:11:01 +0100
> Subject: [PATCH 02/10] lib/replace remove orphaned code
> 
> this is not compiled and used anymore

Reviewed-By: Jelmer Vernooij <jelmer at samba.org>

I guess if we ever care about replacing getdents again in the future,
we can always bring back this code.

> From 076c5c06615014ba9ab1f9eb633751bfe52af1c7 Mon Sep 17 00:00:00 2001
> From: Christian Ambach <ambi at samba.org>
> Date: Fri, 22 Nov 2013 05:13:56 +0100
> Subject: [PATCH 05/10] s3:pam_smbpass change includes
> 
> use the ones from libreplace instead of system ones
> ---
>  source3/pam_smbpass/general.h | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/source3/pam_smbpass/general.h b/source3/pam_smbpass/general.h
> index 6e13f8d..1d022ec 100644
> --- a/source3/pam_smbpass/general.h
> +++ b/source3/pam_smbpass/general.h
> @@ -21,13 +21,10 @@
>  #define PAM_AUTHTOK_RECOVER_ERR PAM_AUTHTOK_RECOVERY_ERR
>  #endif
>  
> -#include <stdio.h>
> -#include <stdlib.h>
> -#include <syslog.h>
> -#include <unistd.h>
> -#include <sys/types.h>
> -#include <sys/stat.h>
> -#include <sys/wait.h>
> +#include "../../lib/replace/replace.h"
> +#include "../../lib/replace/system/filesys.h"
> +#include "../../lib/replace/system/wait.h"
> +#include "../../lib/replace/system/syslog.h"

Please just include "system/foo.h", rather than specifying the full
path - -Ilib/replace should be in the CFLAGS.

> From aa957b139e0245e46702e3854d8b4bb1b99124a0 Mon Sep 17 00:00:00 2001
> From: Christian Ambach <ambi at samba.org>
> Date: Fri, 22 Nov 2013 05:15:13 +0100
> Subject: [PATCH 06/10] lib/socket_wrapper fix compilation when libreplace is
>  not around
> 
> ---
>  lib/socket_wrapper/socket_wrapper.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/socket_wrapper/socket_wrapper.c b/lib/socket_wrapper/socket_wrapper.c
> index 3c9d0f1..8f12592 100644
> --- a/lib/socket_wrapper/socket_wrapper.c
> +++ b/lib/socket_wrapper/socket_wrapper.c
> @@ -56,7 +56,6 @@
>  #include <sys/stat.h>
>  #include <sys/socket.h>
>  #include <sys/ioctl.h>
> -#include <sys/filio.h>
I don't think we can just remove this include - it's required for ioctl() on some platforms.

The rest of the changes in this commit look okay.

> From 8956a7f433bae414ef4244a302aff90ea65dacd5 Mon Sep 17 00:00:00 2001
> From: Christian Ambach <ambi at samba.org>
> Date: Fri, 22 Nov 2013 05:15:59 +0100
> Subject: [PATCH 07/10] lib/ntdb: fix compilation when libreplace is not around

Reviewed-By: Jelmer Vernooij <jelmer at samba.org>

> From baaca3e8c59fc9160eae10c86a6683ce4d685895 Mon Sep 17 00:00:00 2001
> From: Christian Ambach <ambi at samba.org>
> Date: Fri, 22 Nov 2013 05:17:38 +0100
> Subject: [PATCH 08/10] lib/ntdb: correct includes in private header
> 
> include all necessary headers when libreplace is not around

Reviewed-By: Jelmer Vernooij <jelmer at samba.org>

> From ab15379effa255925d85fa7c4cb0bec95e4868f9 Mon Sep 17 00:00:00 2001
> From: Christian Ambach <ambi at samba.org>
> Date: Fri, 22 Nov 2013 05:18:36 +0100
> Subject: [PATCH 09/10] lib/ntdb optimize includes in ntdb tests
> 
> use the private header (which will use libreplace or system headers) instead of direct includes of system includes

It might be intentional that the testsuite only uses the external
API, and that could be a reason why these are not including private.h
at the moment. Perhaps Rusty can comment on that.

> From 832a335f79bb20b15eb263e516d341846b5348b7 Mon Sep 17 00:00:00 2001
> From: Christian Ambach <ambi at samba.org>
> Date: Fri, 22 Nov 2013 05:19:16 +0100
> Subject: [PATCH 10/10] lib/ntdb optimize includes in ntdb tools
> 
> use the private header (which will use libreplace or system headers) instead of direct includes of system includes

Reviewed-By: Jelmer Vernooij <jelmer at samba.org>

... but it'd be great if Rusty could have a look too.

Cheers,

Jelmer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20131129/0ddd90a4/attachment.pgp>