[PATCH 2/2] s4-openldap: Optionally compile libcli/security as a public library to be used by openldap

[Jelmer Vernooij]

Hi Nadezhda,

On Thu, Dec 12, 2013 at 06:57:44PM +0200, Nadezhda Ivanova wrote:
> A continuation of the previous patch, this one adds an option to the
> configure script to compile libcli/security as a library that can be used
> by OpenLdap. This is not the final version of the interface, more functions
> will be added as the access checks are posted to OpenLDAP.

> From 5f789901941f29a1ddfa444bfcdc5f824e21860f Mon Sep 17 00:00:00 2001
> From: Nadezhda Ivanova <nivanova at symas.com>
> Date: Thu, 12 Dec 2013 18:40:41 +0200
> Subject: [PATCH 2/2] s4-openldap: Optionally compile libcli/security as a
>  public library to be used by openldap
Mostly looks good to me, some minor comments below:

> diff --git a/libcli/security/samba_security.h b/libcli/security/samba_security.h
> new file mode 100644
> index 0000000..880cc32
> --- /dev/null
> +++ b/libcli/security/samba_security.h

samba_security.h is a very generic name; it looks like this is create_descriptor.h?

Perhaps add a separate header file that includes
<security/create_descriptor.h> and possibly other future public header files from libsamba-security.

> @@ -0,0 +1,32 @@
> +#ifndef _LIBCLI_SAMBA_SECURITY_SECURITY_H_
> +#define _LIBCLI_SAMBA_SECURITY_SECURITY_H_
> +#include <stdbool.h>
> +#include <stdint.h>
> +#include <talloc.h>
> +#include "lib/util/data_blob.h"
> +
> +typedef enum {
> +	CR_DESCR_INVALID = 0,
> +	CR_DESCR_SCHEMA,
> +	CR_DESCR_CONFIG,
> +	CR_DESCR_DEFAULT,
> +	CR_DESCR_OTHER
> +} CR_DESCR_PARTITION;
> +
> +DATA_BLOB *cr_descr_get_descriptor_to_show(TALLOC_CTX *mem_ctx,
> +					   const DATA_BLOB *sd,
> +					   uint32_t sd_flags);
> +
> +DATA_BLOB *cr_descr_get_new_descriptor_as_blob(TALLOC_CTX *mem_ctx,
> +					       const DATA_BLOB *blob_token,
> +					       const DATA_BLOB *blob_domain_sid,
> +					       const char *defaultSecurityDescriptor,
> +					       const DATA_BLOB *schemaIDGUID,
> +					       const DATA_BLOB *parent,
> +					       const DATA_BLOB *object,
> +					       const DATA_BLOB *old_sd,
> +					       CR_DESCR_PARTITION partition,
> +					       uint32_t sd_flags);
> +
> +
> +#endif

> diff --git a/libcli/security/security.pc.in b/libcli/security/security.pc.in
> new file mode 100644
> index 0000000..2c33d67
> --- /dev/null
> +++ b/libcli/security/security.pc.in
^ This should be samba-security.pc.in, as it's going to be installed in the
system pkg-config path on distributions, where the name 'security' is fairly
ambiguous.

> @@ -0,0 +1,10 @@
> +prefix=@prefix@
> +exec_prefix=@exec_prefix@
> +libdir=@libdir@
> +includedir=@includedir@
> +
> +Name: samba-security
> +Description: A collection of samba access check functions
This description doesn't seem to cover everything that is in libsamba-security.so,
only what is in the current header file.

> +Version: @PACKAGE_VERSION@
> +Libs: @LIB_RPATH@ -L${libdir} -lsamba-security
> +Cflags: -I${includedir}
> \ No newline at end of file
> diff --git a/libcli/security/wscript_build b/libcli/security/wscript_build
> index b529ec8..7f20902 100644
> --- a/libcli/security/wscript_build
> +++ b/libcli/security/wscript_build
> @@ -1,11 +1,28 @@
>  #!/usr/bin/env python
>  
> +VERSION = '0.0.1'
> +if bld.CONFIG_SET('AD_OPENLDAP_DEPS_ENABLED'):
> +    vnum = VERSION
> +    private_library=False
style nit: please add spaces around = in assignments.

> +    pc_files='security.pc'
> +    includes='include'
> +    public_headers='samba_security.h'
> +else:
> +    vnum = None
> +    private_library=True
> +    pc_files=None
> +    includes=''
> +    public_headers=None
Building samba-security as a public library optionally is going to increase the number of
configurations in which Samba can be found. Is there a particular reason for not wanting this
as a public library in some cases?

Cheers,

Jelmer