Hi, Lukas


2013/10/11 Lukas Slebodnik <lslebodn@redhat.com>
On (11/10/13 12:53), Benjamin Franzke wrote:
>2013/10/11 Sumit Bose <sbose@redhat.com>
>
>> On Thu, Oct 10, 2013 at 02:41:54PM +0200, Benjamin Franzke wrote:
>> > Thanks for your review!
>> >
>> <snip>
>> >
>> > The patch with your concers fixed is attached.
>>
>> Thank you for the new version. I only have two comments left:
>>
>> +    if (handle == NULL || errmsg == NULL) {
>> +        *errmsg = strerror(EINVAL);
>>
>> you cannot return an error message if errmsg == NULL.
>>
>
>yes, of course not :)
>had to leave house and wanted to get the patch out before, that dumb
>mistake was the result :\
>
>>
>> +        return -EINVAL;
>> +    }
>>
>>
>> +    err = sss_nss_getsidbyname(name, &sid, &id_type);
>> +    if (err != 0)  {
>> +        /* Might be a raw string representation of SID,
>> +         * try converting that before returning an error. */
>> +        if (sid_to_cifs_sid(ctx, name, csid) == 0)
>> +            return 0;
>> +
>>
>>  ^^^^^^^^ trailing whitespace
>>
>> Oops, again.. have them highlighted in my editor now ;)
>
>> +        ctx_set_error(ctx, strerror(err));
>> +        return -err;
>> +    }
>>
>>
>> bye,
>> Sumit
>>
>
>New patch attached, with these two things fixed.

Inline.

>From c548048ad7f7db530de2194d9a71ba7a265a34c4 Mon Sep 17 00:00:00 2001
From: Benjamin Franzke <benjaminfranzke@googlemail.com>
Date: Thu, 26 Sep 2013 10:27:33 +0200
Subject: [PATCH] Add CIFS idmap plugin

---
 Makefile.am                             |  21 ++
 configure.ac                            |   2 +
 src/conf_macros.m4                      |  14 ++
 src/external/cifsidmap.m4               |  13 ++
 src/lib/cifs_idmap_sss/cifs_idmap_sss.c | 339 ++++++++++++++++++++++++++++++++
 5 files changed, 389 insertions(+)
 create mode 100644 src/external/cifsidmap.m4
 create mode 100644 src/lib/cifs_idmap_sss/cifs_idmap_sss.c

diff --git a/src/external/cifsidmap.m4 b/src/external/cifsidmap.m4
new file mode 100644
index 0000000000000000000000000000000000000000..546c373b0e66dd4136bca8d92cf220102b2f7830
--- /dev/null
+++ b/src/external/cifsidmap.m4
@@ -0,0 +1,13 @@
+AC_ARG_ENABLE([cifs-idmap-plugin],
+              [AS_HELP_STRING([--disable-cifs-idmap-plugin],
+                              [do not build CIFS idmap plugin])],
+              [build_cifs_idmap_plugin=$enableval],
+              [build_cifs_idmap_plugin=yes])
                                      ^^^^^^
This line mean that cifs_idmap_plugin will be build by default,
but sssd.spec.in file is not updated with appropriate devel package.

As Sumit said, he'll do that part.. 

Something like
  %if (0%{?fedora} >= 17)
  BuildRequires: cifs-utils-devel
  %endif

BTW: cifs-utils-devel is not available on RHEL6, do we want to build
cifs_idmap_plugin by default?

And library cifs_idmap_sss.so should be added to some (or new) subpackage
in spec file. If you are not familiar with rpm packaging, we can help you.

+
+AS_IF([test x$build_cifs_idmap_plugin = xyes],
+      [AC_CHECK_HEADER([cifsidmap.h], [],
+                       [AC_MSG_ERROR([you must have the cifsidmap header installed to build the idmap plugin])])
+      ])
+
+AM_CONDITIONAL([BUILD_CIFS_IDMAP_PLUGIN],
+               [test x$build_cifs_idmap_plugin = xyes])


diff --git a/src/lib/cifs_idmap_sss/cifs_idmap_sss.c b/src/lib/cifs_idmap_sss/cifs_idmap_sss.c
new file mode 100644
index 0000000000000000000000000000000000000000..bcea6d6d84dbd6184c2f719d0c1b4a2ac3c014ca
--- /dev/null
+++ b/src/lib/cifs_idmap_sss/cifs_idmap_sss.c

... snip ...

+static int samba_unix_sid_to_id(const char *sid, struct cifs_uxid *cuxid)
+{
+    id_t id;
+    uint8_t type;
+
+    if (sscanf(sid, "S-1-22-%hhu-%u", &type, &id) != 2)
+        return -1;
+
+    switch (type) {
+    case 1:
+        cuxid->type = CIFS_UXID_TYPE_UID;
+        cuxid->id.uid = id;
+        break;
+    case 2:
+        cuxid->type = CIFS_UXID_TYPE_GID;
+        cuxid->id.gid = id;
        ^^^^^^^
Missing brake statement.
Is it aim or mistake?

That was a mistake, thanks. 

+    default:
+        cuxid->type = CIFS_UXID_TYPE_UNKNOWN;
+        return -1;
+    }
+
+    return 0;
+}

And the last note.
You created new dynamic library. Could you add this library to dlopen test?

Done. New patch attached. 

+++ b/src/tests/dlopen-tests.c
@@ -40,6 +40,7 @@ struct so {
     { "libsss_debug.so", { LIBPFX"libsss_debug.so", NULL } },
     { "libipa_hbac.so", { LIBPFX"libipa_hbac.so", NULL } },
     { "libsss_idmap.so", { LIBPFX"libsss_idmap.so", NULL } },
+    { "cifs_idmap_sss.so", { LIBPFX"cifs_idmap_sss.so", NULL } },
     { "libsss_nss_idmap.so", { LIBPFX"libsss_nss_idmap.so", NULL } },
     { "libnss_sss.so", { LIBPFX"libnss_sss.so", NULL } },
     { "pam_sss.so", { LIBPFX"pam_sss.so", NULL } }, 

Thank you very much .

LS
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel