[SSSD] [PATCH] CIFS idmap Plugin using SSSD
Lukas Slebodnik
lslebodn at redhat.com
Fri Oct 11 14:38:37 UTC 2013
On (11/10/13 16:23), Benjamin Franzke wrote:
>Hi, Lukas
>
>
>2013/10/11 Lukas Slebodnik <lslebodn at redhat.com>
>
>> On (11/10/13 12:53), Benjamin Franzke wrote:
>> >2013/10/11 Sumit Bose <sbose at 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 at 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..
>
No problem.
I read this thread very quickly :-)
I've just wanted to point out, that it can break nightly built on RHEL6.
So it would be better to do changes at one, but I don't have a problem with two
patches.
>>
>> 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.
>
Thank you.
>>
>> + 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.
>
Thank you for updating test.
>>
>> +++ 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
It looks good to me.
LS
More information about the sssd-devel
mailing list