<div dir="ltr">Thanks for your review!<div><br><div class="gmail_extra"><div class="gmail_quote">2013/10/10 Sumit Bose <span dir="ltr">&lt;<a href="mailto:sbose@redhat.com" target="_blank">sbose@redhat.com</a>&gt;</span><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>On Wed, Oct 09, 2013 at 03:50:11PM +0200, Benjamin Franzke wrote:<br>
&gt; Hi Sumit,<br>
&gt;<br>
&gt; Sorry for the delay, got sidetracked by trying to find an appropriate way<br>
&gt; to handle the well known sids etc..<br>
&gt; But thanks to your reminder I got over to do the integration into sssd&#39;s<br>
&gt; autoconf/make now :)<br>
&gt; So here is the patch as an attachment.<br>
&gt; (as recommended by the contrib page - but if you like it better for review<br>
&gt; l&#39;ll git send-email it as well..)<br>
<br>
</div>Thanks a lot for the patch. It works well in my test but I have some<br>
comments, see below.<br>
<div>&gt;<br>
&gt; This patch does not include an update for the rpm specs.<br>
&gt; Could you do that integration? For me as a gentoo user that is currently an<br>
&gt; unknown-area :)<br>
<br>
</div>Sure no problem.<br>
<div><div><br>
&gt;<br>
&gt; Regards, Ben<br>
&gt;<br>
&gt; 2013/10/9 Sumit Bose &lt;<a href="mailto:sbose@redhat.com" target="_blank">sbose@redhat.com</a>&gt;<br>
&gt;<br>
&gt; &gt; On Mon, Oct 07, 2013 at 12:22:13PM +0200, Benjamin Franzke wrote:<br>
&gt; &gt; &gt; 2013/10/7 Sumit Bose &lt;<a href="mailto:sbose@redhat.com" target="_blank">sbose@redhat.com</a>&gt;<br>
&gt; &gt; &gt;<br>
&gt; &gt; &gt; &gt; On Thu, Oct 03, 2013 at 11:57:07PM +0200, Benjamin Franzke wrote:<br>
&gt; &gt; &gt; &gt; &gt; &gt;<br>
&gt; &gt; &gt; &gt; &gt; &gt; Hi Ben,<br>
&gt; &gt; &gt; &gt; &gt; &gt;<br>
&gt; &gt; &gt; &gt; &gt; &gt; this is really great, thank you very much for the contribution! As<br>
&gt; &gt; a<br>
&gt; &gt; &gt; &gt; &gt; &gt; matter of fact, we&#39;ve been planning on delivering the CIFS plugin<br>
&gt; &gt; as<br>
&gt; &gt; &gt; &gt; &gt; &gt; part of the next release:<br>
&gt; &gt; &gt; &gt; &gt; &gt; <a href="https://fedorahosted.org/sssd/ticket/1534" target="_blank">https://fedorahosted.org/sssd/ticket/1534</a><br>
&gt; &gt; &gt; &gt; &gt; &gt;<br>
&gt; &gt; &gt; &gt; &gt; &gt; Have you found the design page we created?<br>
&gt; &gt; &gt; &gt; &gt; &gt;<br>
&gt; &gt; &gt; &gt;<br>
&gt; &gt; <a href="https://fedorahosted.org/sssd/wiki/DesignDocs/IntegrateSSSDWithCIFSClient" target="_blank">https://fedorahosted.org/sssd/wiki/DesignDocs/IntegrateSSSDWithCIFSClient</a><br>
&gt; &gt; &gt; &gt; &gt;<br>
&gt; &gt; &gt; &gt; &gt;<br>
&gt; &gt; &gt; &gt; &gt; Yeah, I&#39;ve read those pages before writing the plugin..<br>
&gt; &gt; &gt; &gt; &gt;<br>
&gt; &gt; &gt; &gt; &gt;<br>
&gt; &gt; &gt; &gt; &gt; &gt;<br>
&gt; &gt; &gt; &gt; &gt; &gt; Mostly the work on our side was done by Sumit Bose (I&#39;ve CC-ed<br>
&gt; &gt; him). I<br>
&gt; &gt; &gt; &gt; &gt; &gt; would propose to get in touch with him to avoid duplicating some<br>
&gt; &gt; &gt; &gt; effort.<br>
&gt; &gt; &gt; &gt; &gt; &gt;<br>
&gt; &gt; &gt; &gt; &gt; Unfortunately Sumit is in Germany and there is a holiday so he won&#39;t<br>
&gt; &gt; be<br>
&gt; &gt; &gt; &gt; &gt; &gt; available until Monday.<br>
&gt; &gt; &gt; &gt; &gt; &gt;<br>
&gt; &gt; &gt; &gt; &gt;<br>
&gt; &gt; &gt; &gt; &gt; &gt; In general, I would suggest to merge this code into the SSSD tree<br>
&gt; &gt; if<br>
&gt; &gt; &gt; &gt; you<br>
&gt; &gt; &gt; &gt; &gt; &gt; wouldn&#39;t mind. I think it would makes sense to have the code in the<br>
&gt; &gt; &gt; &gt; same<br>
&gt; &gt; &gt; &gt; &gt; &gt; repo as the ID mapping library.<br>
&gt; &gt; &gt; &gt; &gt; &gt;<br>
&gt; &gt; &gt; &gt; &gt;<br>
&gt; &gt; &gt; &gt; &gt; I&#39;d really appreciate having it integrated in sssd tree,<br>
&gt; &gt; &gt; &gt; &gt; to have centralized maintainance and adoption by distributions..<br>
&gt; &gt; &gt; &gt;<br>
&gt; &gt; &gt; &gt; Thanks a lot for the patch. I would suggest to put your code into<br>
&gt; &gt; &gt; &gt; src/lib/cifs_idmap_sss/ and modify SSSD&#39;s Makefile.am to build the<br>
&gt; &gt; &gt; &gt; plugin. Would you like to do the integration and send the result for<br>
&gt; &gt; &gt; &gt; review to this list again?<br>
&gt; &gt; &gt; &gt;<br>
&gt; &gt; &gt;<br>
&gt; &gt; &gt; I&#39;d like to do that integration, will send a patch..<br>
&gt; &gt;<br>
&gt; &gt; I&#39;d like to add the plugin to the next Fedora release which<br>
&gt; &gt; unfortunately has its deadline next week. I don&#39;t want to rush you, but<br>
&gt; &gt; do you have a chance to send the patch today or tomorrow? Otherwise<br>
&gt; &gt; would you mind if I prepare the patch to integrate your code into the<br>
&gt; &gt; SSSD tree?<br>
&gt; &gt;<br>
&gt; &gt; Thanks for your help.<br>
&gt; &gt;<br>
&gt; &gt; bye,<br>
&gt; &gt; Sumit<br>
&gt; &gt; &gt;<br>
&gt; &gt; &gt;<br>
&gt; &gt; &gt; &gt; About the well-know SIDs. They should not be handled inside the plugin<br>
&gt; &gt; &gt; &gt; but by SSSD itself. If you are interested in adding them I&#39;d be happy<br>
&gt; &gt; to<br>
&gt; &gt; &gt; &gt; help you how to start (if my help is needed at all :-).<br>
&gt; &gt; &gt; &gt;<br>
&gt; &gt; &gt;<br>
&gt; &gt; &gt; Yup, sounds better to handle the SIDs by SSSD core. I&#39;d like to try..<br>
&gt; &gt; &gt; I&#39;ll see whether time permits me to do that soon.. (doing this as a part<br>
&gt; &gt; of<br>
&gt; &gt; &gt; my thesis)<br>
&gt; &gt; &gt;<br>
&gt; &gt; &gt;<br>
&gt; &gt; &gt;<br>
&gt; &gt; &gt; &gt;<br>
&gt; &gt; &gt; &gt; bye,<br>
&gt; &gt; &gt; &gt; Sumit<br>
&gt; &gt; &gt; &gt;<br>
&gt; &gt; &gt; &gt; &gt;<br>
&gt; &gt; &gt; &gt; &gt;<br>
&gt; &gt; &gt; &gt; &gt; &gt; Thanks again for the contribution, much appreciated!<br>
&gt; &gt; &gt; &gt; &gt; &gt; _______________________________________________<br>
&gt; &gt; &gt; &gt; &gt; &gt; sssd-devel mailing list<br>
&gt; &gt; &gt; &gt; &gt; &gt; <a href="mailto:sssd-devel@lists.fedorahosted.org" target="_blank">sssd-devel@lists.fedorahosted.org</a><br>
&gt; &gt; &gt; &gt; &gt; &gt; <a href="https://lists.fedorahosted.org/mailman/listinfo/sssd-devel" target="_blank">https://lists.fedorahosted.org/mailman/listinfo/sssd-devel</a><br>
&gt; &gt; &gt; &gt; &gt; &gt;<br>
&gt; &gt; &gt; &gt;<br>
&gt; &gt; &gt; &gt; &gt; _______________________________________________<br>
&gt; &gt; &gt; &gt; &gt; sssd-devel mailing list<br>
&gt; &gt; &gt; &gt; &gt; <a href="mailto:sssd-devel@lists.fedorahosted.org" target="_blank">sssd-devel@lists.fedorahosted.org</a><br>
&gt; &gt; &gt; &gt; &gt; <a href="https://lists.fedorahosted.org/mailman/listinfo/sssd-devel" target="_blank">https://lists.fedorahosted.org/mailman/listinfo/sssd-devel</a><br>
&gt; &gt; &gt; &gt;<br>
&gt; &gt; &gt; &gt; _______________________________________________<br>
&gt; &gt; &gt; &gt; sssd-devel mailing list<br>
&gt; &gt; &gt; &gt; <a href="mailto:sssd-devel@lists.fedorahosted.org" target="_blank">sssd-devel@lists.fedorahosted.org</a><br>
&gt; &gt; &gt; &gt; <a href="https://lists.fedorahosted.org/mailman/listinfo/sssd-devel" target="_blank">https://lists.fedorahosted.org/mailman/listinfo/sssd-devel</a><br>
&gt; &gt; &gt; &gt;<br>
&gt; &gt;<br>
&gt; &gt; &gt; _______________________________________________<br>
&gt; &gt; &gt; sssd-devel mailing list<br>
&gt; &gt; &gt; <a href="mailto:sssd-devel@lists.fedorahosted.org" target="_blank">sssd-devel@lists.fedorahosted.org</a><br>
&gt; &gt; &gt; <a href="https://lists.fedorahosted.org/mailman/listinfo/sssd-devel" target="_blank">https://lists.fedorahosted.org/mailman/listinfo/sssd-devel</a><br>
&gt; &gt;<br>
&gt; &gt;<br>
<br>
</div></div>...<br>
<br>
&gt; diff --git a/src/lib/cifs_idmap_sss/cifs_idmap_sss.c b/src/lib/cifs_idmap_sss/cifs_idmap_sss.c<br>
&gt; new file mode 100644<br>
&gt; index 0000000000000000000000000000000000000000..f0599cb6b943145a685dbfd57bf5f8f48bfbfde6<br>
&gt; --- /dev/null<br>
&gt; +++ b/src/lib/cifs_idmap_sss/cifs_idmap_sss.c<br>
&gt; @@ -0,0 +1,338 @@<br>
&gt; +/*<br>
&gt; +    Authors:<br>
&gt; +        Benjamin Franzke &lt;<a href="mailto:benjaminfranzke@googlemail.com" target="_blank">benjaminfranzke@googlemail.com</a>&gt;<br>
&gt; +<br>
&gt; +    Copyright (C) 2013 Benjamin Franzke<br>
&gt; +<br>
&gt; +    This program is free software; you can redistribute it and/or modify<br>
&gt; +    it under the terms of the GNU General Public License as published by<br>
&gt; +    the Free Software Foundation; either version 3 of the License, or<br>
&gt; +    (at your option) any later version.<br>
&gt; +<br>
&gt; +    This program is distributed in the hope that it will be useful,<br>
&gt; +    but WITHOUT ANY WARRANTY; without even the implied warranty of<br>
&gt; +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the<br>
&gt; +    GNU General Public License for more details.<br>
&gt; +<br>
&gt; +    You should have received a copy of the GNU General Public License<br>
&gt; +    along with this program.  If not, see &lt;<a href="http://www.gnu.org/licenses/" target="_blank">http://www.gnu.org/licenses/</a>&gt;.<br>
&gt; +*/<br>
&gt; +<br>
&gt; +/* TODO: Support well known SIDs as in samba&#39;s<br>
&gt; + *        - librpc/idl/security.idl or<br>
&gt; + *        - source4/rpc_server/lsa/lsa_lookup.c?<br>
&gt; + */<br>
&gt; +<br>
&gt; +/* TODO: Support of [all] samba&#39;s Unix SIDs:<br>
&gt; + *         Users:  S-1-22-1-%UID<br>
&gt; + *         Groups: S-1-22-2-%GID<br>
&gt; + */<br>
&gt; +<br>
&gt; +#include &lt;stdio.h&gt;<br>
&gt; +#include &lt;stdlib.h&gt;<br>
&gt; +#include &lt;errno.h&gt;<br>
&gt; +#include &lt;string.h&gt;<br>
&gt; +#include &lt;limits.h&gt;<br>
&gt; +#include &lt;stdarg.h&gt;<br>
&gt; +<br>
&gt; +#include &lt;cifsidmap.h&gt;<br>
&gt; +<br>
&gt; +#include &quot;lib/idmap/sss_idmap.h&quot;<br>
&gt; +#include &quot;sss_client/idmap/sss_nss_idmap.h&quot;<br>
&gt; +<br>
&gt; +#define WORLD_SID &quot;S-1-1-0&quot;<br>
&gt; +<br>
&gt; +#ifdef DEBUG<br>
&gt; +#include &lt;syslog.h&gt;<br>
&gt; +#define debug(str, ...) \<br>
&gt; +    syslog(0, &quot;%s: &quot; str &quot;\n&quot;, \<br>
&gt; +           __FUNCTION__, ##__VA_ARGS__)<br>
&gt; +#else<br>
&gt; +#define debug(...) do { } while(0)<br>
&gt; +#endif<br>
&gt; +<br>
&gt; +struct sssd_ctx {<br>
&gt; +    struct sss_idmap_ctx *idmap;<br>
&gt; +    const char **errmsg;<br>
&gt; +};<br>
&gt; +<br>
&gt; +#define ctx_set_error(ctx, error) \<br>
&gt; +    do { \<br>
&gt; +        *ctx-&gt;errmsg = error; \<br>
&gt; +        debug(&quot;%s&quot;, error ? error : &quot;&quot;); \<br>
&gt; +    } while (0);<br>
&gt; +<br>
&gt; +int cifs_idmap_init_plugin(void **handle, const char **errmsg)<br>
&gt; +{<br>
&gt; +    struct sssd_ctx *ctx;<br>
&gt; +    enum idmap_error_code err;<br>
<br>
it should be checked if handle or errmsg are NULL.</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
&gt; +<br>
&gt; +    ctx = malloc(sizeof *ctx);<br>
&gt; +    if (!ctx) {<br>
&gt; +        *errmsg = &quot;Failed to allocate context&quot;;<br>
&gt; +        return -1;<br>
&gt; +    }<br>
&gt; +    ctx-&gt;errmsg = errmsg;<br>
&gt; +    ctx_set_error(ctx, NULL);<br>
&gt; +<br>
&gt; +    err = sss_idmap_init(NULL, NULL, NULL, &amp;ctx-&gt;idmap);<br>
&gt; +    if (err != IDMAP_SUCCESS) {<br>
&gt; +        ctx_set_error(ctx, idmap_error_string(err));<br>
<br>
ctx should be freed to not leak memory.<br>
<br>
&gt; +        return -1;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    *handle = ctx;<br>
&gt; +    return 0;<br>
&gt; +}<br>
&gt; +<br>
&gt; +void cifs_idmap_exit_plugin(void *handle)<br>
&gt; +{<br>
&gt; +    struct sssd_ctx *ctx = handle;<br>
&gt; +<br>
&gt; +    debug(&quot;exit&quot;);<br>
&gt; +<br>
<br>
it should be check if ctx is NULL before using it.<br>
<br>
&gt; +    sss_idmap_free(ctx-&gt;idmap);<br>
&gt; +<br>
&gt; +    free(ctx);<br>
&gt; +}<br>
&gt; +<br>
&gt; +<br>
&gt; +/* Test with `getcifsacl file` on client. */<br>
&gt; +int cifs_idmap_sid_to_str(void *handle, const struct cifs_sid *csid,<br>
&gt; +                          char **name)<br>
&gt; +{<br>
&gt; +    struct sssd_ctx *ctx = handle;<br>
&gt; +    enum idmap_error_code iderr;<br>
&gt; +    char *sid;<br>
&gt; +    enum sss_id_type id_type;<br>
&gt; +    int err;<br>
&gt; +<br>
&gt; +    iderr = sss_idmap_bin_sid_to_sid(ctx-&gt;idmap,<br>
&gt; +                                     (uint8_t *) csid, sizeof(*csid), &amp;sid);<br>
<br>
please use const uint8_t. Using sss_idmap_bin_sid_to_sid() works as expected<br>
because struct cifs_sid is defined in the right order and is packed.<br>
Nevertheless I think in the long run we should add something like<br>
sss_idmap_cifs_sid_to_sid() to make the code more robust. But this does not<br>
need to happen now. </blockquote><div>Ah, that missing const annoyes me..</div><div>Left that out while trying different formats (smb_sid etc..), and then I forgot to add it back :\</div><div><br></div><div>I checked the bin_sid format in the sssd tests and found they match</div>
<div>the alignment and packing specifications of cifs_sid, so its the same format..<br></div><div>What is bin_sid for, if not for comaptible binary formats?</div><div>Yeah, it could be replaced with a cifs_sid wrapper,</div>
<div>but maybe just remove the bin_sid stuff then as well?</div><div>(BTW:  removed that const when trying other formats, like smb_sid, which were</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<br>
&gt; +    if (iderr != IDMAP_SUCCESS) {<br>
&gt; +        ctx_set_error(ctx, idmap_error_string(iderr));<br>
&gt; +        *name = NULL;<br>
&gt; +        return -1;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    debug(&quot;sid: %s&quot;, sid);<br>
&gt; +<br>
&gt; +    if (strcmp(sid, WORLD_SID) == 0) {<br>
&gt; +        *name = strdup(&quot;\\Everyone&quot;);<br>
&gt; +        if (!*name) {<br>
&gt; +            ctx_set_error(ctx, strerror(ENOMEM));<br>
&gt; +            return -ENOMEM;<br>
&gt; +        }<br>
&gt; +        return 0;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    err = sss_nss_getnamebysid(sid, name, &amp;id_type);<br>
&gt; +    if (err != 0)  {<br>
&gt; +        ctx_set_error(ctx, strerror(err));<br>
&gt; +        *name = NULL;<br>
&gt; +        return -err;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    /* FIXME: Map Samba Unix SIDs? (sid-&gt;id and use getpwuid)? */<br>
&gt; +<br>
&gt; +    debug(&quot;name: %s&quot;, *name);<br>
&gt; +<br>
&gt; +    return 0;<br>
&gt; +}<br>
&gt; +<br>
&gt; +static int sid_to_cifs_sid(struct sssd_ctx *ctx, const char *sid,<br>
&gt; +                           struct cifs_sid *csid)<br>
&gt; +{<br>
&gt; +    uint8_t *bsid = NULL;<br>
&gt; +    enum idmap_error_code err;<br>
&gt; +    size_t length;<br>
&gt; +<br>
&gt; +    err = sss_idmap_sid_to_bin_sid(ctx-&gt;idmap,<br>
&gt; +                                   sid, &amp;bsid, &amp;length);<br>
&gt; +    if (err != IDMAP_SUCCESS) {<br>
&gt; +        ctx_set_error(ctx, idmap_error_string(err));<br>
&gt; +        return -1;<br>
&gt; +    }<br>
&gt; +    if (length &gt; sizeof(struct cifs_sid)) {<br>
&gt; +        ctx_set_error(ctx, &quot;too large sid length&quot;);<br>
&gt; +        free(bsid);<br>
&gt; +        return -1;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    memcpy(csid, bsid, length);<br>
&gt; +    free(bsid);<br>
&gt; +<br>
&gt; +    return 0;<br>
&gt; +}<br>
&gt; +<br>
&gt; +/* Test with setcifsacl -a */<br>
&gt; +int cifs_idmap_str_to_sid(void *handle, const char *name,<br>
&gt; +                          struct cifs_sid *csid)<br>
&gt; +{<br>
&gt; +    struct sssd_ctx *ctx = handle;<br>
&gt; +    int err;<br>
&gt; +    enum sss_id_type id_type;<br>
&gt; +    const char *sid;<br>
&gt; +    char *sss_sid = NULL;<br>
&gt; +    int success = 0;<br>
&gt; +<br>
&gt; +    debug(&quot;%s&quot;, name);<br>
&gt; +<br>
&gt; +    if (strncmp(&quot;S-&quot;, name, 2) == 0) {<br>
<br>
I think this check is not needed and puts an unnecessary limitation to the user<br>
names.<br></blockquote><div><br></div><div>With that implementation it was a limitation to the user names.</div><div>I changed the code to use string-sid to cifs-sid conversion only if name lookup fails.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<br>
&gt; +        debug(&quot;%s: name is sid string representation&quot;, __FUNCTION__);<br>
&gt; +        sid = name;<br>
&gt; +        if (!sid) {<br>
&gt; +        }<br>
&gt; +    } else {<br>
&gt; +        err = sss_nss_getsidbyname(name, &amp;sss_sid, &amp;id_type);<br>
&gt; +        if (err != 0)  {<br>
&gt; +            ctx_set_error(ctx, strerror(err));<br>
&gt; +            /* TODO: Map name==Everyone to WOLD_SID? */<br>
&gt; +            return -err;<br>
&gt; +        }<br>
&gt; +        sid = sss_sid;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    if (sid_to_cifs_sid(ctx, sid, csid) != 0)<br>
&gt; +        success = -1;<br>
&gt; +<br>
&gt; +    if (sss_sid)<br>
<br>
the if here is not needed, free() can handle NULL.<br>
<br>
&gt; +        free(sss_sid);<br>
&gt; +<br>
&gt; +    return success;<br>
&gt; +}<br>
&gt; +<br>
&gt; +static int samba_unix_sid_to_id(const char *sid, struct cifs_uxid *cuxid)<br>
&gt; +{<br>
&gt; +    id_t id;<br>
&gt; +    uint8_t type;<br>
&gt; +<br>
&gt; +    if (sscanf(sid, &quot;S-1-22-%hhu-%u&quot;, &amp;type, &amp;id) != 2)<br>
&gt; +        return -1;<br>
&gt; +<br>
&gt; +    switch (type) {<br>
&gt; +    case 1:<br>
&gt; +        cuxid-&gt;type = CIFS_UXID_TYPE_UID;<br>
&gt; +        cuxid-&gt;id.uid = id;<br>
&gt; +        break;<br>
&gt; +    case 2:<br>
&gt; +        cuxid-&gt;type = CIFS_UXID_TYPE_GID;<br>
&gt; +        cuxid-&gt;id.gid = id;<br>
&gt; +    default:<br>
&gt; +        cuxid-&gt;type = CIFS_UXID_TYPE_UNKNOWN;<br>
&gt; +        return -1;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    return 0;<br>
&gt; +}<br>
&gt; +<br>
&gt; +static int sss_sid_to_id(struct sssd_ctx *ctx, const char *sid,<br>
&gt; +                         struct cifs_uxid *cuxid)<br>
&gt; +{<br>
&gt; +    int err;<br>
&gt; +    enum sss_id_type id_type;<br>
&gt; +<br>
&gt; +    err = sss_nss_getidbysid(sid, (uint32_t *)&amp;cuxid-&gt;id.uid, &amp;id_type);<br>
&gt; +    if (err != 0)  {<br>
&gt; +        ctx_set_error(ctx, strerror(err));<br>
&gt; +        return -1;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    switch (id_type) {<br>
&gt; +    case SSS_ID_TYPE_UID:<br>
&gt; +        cuxid-&gt;type = CIFS_UXID_TYPE_UID;<br>
&gt; +        break;<br>
&gt; +    case SSS_ID_TYPE_GID:<br>
&gt; +        cuxid-&gt;type = CIFS_UXID_TYPE_GID;<br>
&gt; +        break;<br>
&gt; +    case SSS_ID_TYPE_BOTH:<br>
&gt; +        cuxid-&gt;type = CIFS_UXID_TYPE_BOTH;<br>
&gt; +        break;<br>
&gt; +    case SSS_ID_TYPE_NOT_SPECIFIED:<br>
&gt; +    default:<br>
&gt; +        return -1;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    return 0;<br>
&gt; +}<br>
&gt; +<br>
&gt; +/**<br>
&gt; + * cifs_idmap_sids_to_ids - convert struct cifs_sids to struct cifs_uxids<br>
&gt; + * usecase: mount.cifs -o sec=krb5,multiuser,cifsacl,nounix<br>
<br>
there is a trailing whitespace                                ^<br>
<br>
&gt; + * test: ls -n on mounted share<br>
&gt; + */<br>
&gt; +int cifs_idmap_sids_to_ids(void *handle, const struct cifs_sid *csid,<br>
&gt; +                           const size_t num, struct cifs_uxid *cuxid)<br>
&gt; +{<br>
&gt; +    struct sssd_ctx *ctx = handle;<br>
&gt; +    enum idmap_error_code err;<br>
&gt; +    int success = -1;<br>
&gt; +    size_t i;<br>
&gt; +    char *sid;<br>
&gt; +<br>
&gt; +    debug(&quot;num: %zd&quot;, num);<br>
&gt; +<br>
&gt; +    if (num &gt; UINT_MAX) {<br>
&gt; +        ctx_set_error(ctx, &quot;num is too large.&quot;);<br>
&gt; +        return -EINVAL;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    for (i = 0; i &lt; num; ++i) {<br>
&gt; +        err = sss_idmap_bin_sid_to_sid(ctx-&gt;idmap, (uint8_t *) &amp;csid[i],<br>
&gt; +                                       sizeof(csid[i]), &amp;sid);<br>
<br>
please use const uint8_t here as well.<br>
<br>
&gt; +        if (err != IDMAP_SUCCESS) {<br>
&gt; +            ctx_set_error(ctx, idmap_error_string(err));<br>
&gt; +            continue;<br>
&gt; +        }<br>
&gt; +<br>
&gt; +        cuxid[i].type = CIFS_UXID_TYPE_UNKNOWN;<br>
&gt; +<br>
&gt; +        if (sss_sid_to_id(ctx, sid, &amp;cuxid[i]) == 0 ||<br>
<br>
there is a trailing whitespace                           ^<br>
<br>
&gt; +            samba_unix_sid_to_id(sid, &amp;cuxid[i]) == 0) {<br>
&gt; +<br>
&gt; +            debug(&quot;setting uid of %s to %d&quot;, sid, cuxid[i].id.uid);<br>
&gt; +            success = 0;<br>
&gt; +        }<br>
&gt; +<br>
&gt; +        free(sid);<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    return success;<br>
&gt; +}<br>
&gt; +<br>
&gt; +<br>
&gt; +int cifs_idmap_ids_to_sids(void *handle, const struct cifs_uxid *cuxid,<br>
&gt; +                           const size_t num, struct cifs_sid *csid)<br>
&gt; +{<br>
&gt; +    struct sssd_ctx *ctx = handle;<br>
&gt; +    int err, success = -1;<br>
&gt; +    char *sid;<br>
&gt; +    enum sss_id_type id_type;<br>
&gt; +    size_t i;<br>
&gt; +<br>
&gt; +    debug(&quot;num ids: %zd&quot;, num);<br>
&gt; +<br>
&gt; +    if (num &gt; UINT_MAX) {<br>
&gt; +        ctx_set_error(ctx, &quot;num is too large.&quot;);<br>
&gt; +        return -EINVAL;<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    for (i = 0; i &lt; num; ++i) {<br>
&gt; +        err = sss_nss_getsidbyid((uint32_t)cuxid[i].id.uid, &amp;sid, &amp;id_type);<br>
&gt; +        if (err != 0)  {<br>
&gt; +            ctx_set_error(ctx, strerror(err));<br>
&gt; +            csid[i].revision = 0;<br>
&gt; +            /* FIXME: would it be safe to map *any* uid/gids unknown by sssd to<br>
&gt; +             * SAMBA&#39;s UNIX SIDs? */<br>
&gt; +            continue;<br>
&gt; +        }<br>
&gt; +<br>
&gt; +        if (sid_to_cifs_sid(ctx, sid, csid) == 0)<br>
&gt; +            success = 0;<br>
&gt; +        else<br>
&gt; +            csid[i].revision = 0;<br>
&gt; +        free(sid);<br>
&gt; +    }<br>
&gt; +<br>
&gt; +    return success;<br>
&gt; +}<br>
<span><font color="#888888">&gt; --<br>
&gt; 1.8.1.5<br>
&gt;<br></font></span></blockquote><div><br></div><div>The patch with your concers fixed is attached.</div></div><br></div></div></div>