On (11/10/13 16:23), Benjamin Franzke wrote:
Hi, Lukas
2013/10/11 Lukas Slebodnik <lslebodn(a)redhat.com>
> On (11/10/13 12:53), Benjamin Franzke wrote:
> >2013/10/11 Sumit Bose <sbose(a)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(a)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