[SSSD] [PATCH] User home directories management

Stephen Gallagher sgallagh at redhat.com
Thu Oct 22 11:32:56 UTC 2009


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/22/2009 07:20 AM, Martin Nagy wrote:
> On Wed, 2009-10-21 at 20:30 +0200, Jakub Hrozek wrote:
>>>> In main():
>>>> +        switch (ret) {
>>>> +            case 'G':
>>>> +                groups = poptGetOptArg(pc);
>>>> +                if (!groups) {
>>>> +                    ret = -1;
>>>> +                    break;
>>>> +                }
>>>> +
>>>> +            case 'm':
>>>> +                pc_create_home = DO_CREATE_HOME;
>>>> +                break;
>>>> The first break shouldn't be inside of the if, but it wouldn't be able
>>>> to break out of the while loop anyway, so you should add a goto instead.
>>>>
>>
>> It would (that's what ret=-1 was for) but a goto is much more convenient
>> here - fixed.
> 
> It wouldn't, ret will be changed by assignment from poptGetNextOpt().
> 
>>>> +            ERROR("Cannot create user's mail spool\n");
>>>> +            DEBUG(1, ("Cannot create user's mail spool: [%d][%s].\n",
>>>> +                        ret, strerror(ret)));
>>>> Why don't you use only ERROR and include the error string there? I think
>>>> that strerror() will translate the message according to the locale, so
>>>> that shouldn't be an issue.
>>>>
>>
>> Because I generally think that the user should not be bothered with
>> strerror messages, they can be cryptic (think ENOENT returned from a
>> database search - strerror is "No such file or directory") and also I
>> think there should be a debug message with most (if not all) ERROR
>> messages - to give developers a string that is guaranteed to be NOT
>> translated.
>>
>> But it's true that in case of file operations, strerror() messages are
>> OK. Added.
> 
> Fair enough.
> 
> All the important points were addressed, thank you. I noticed couple of
> other things however.
> 
> One more typo I noticed in server/tools/sss_useradd.c:
> +        { "create-home", 'm', POPT_ARG_NONE, NULL, 'm', _("Create
> user's directory if does not exist"), NULL },
> 
> Insert 'it' between 'if' and 'does'.
> 
> A more serious bug I see in files.c:
> +    while ((cnt = read(ifd, buf, sizeof(buf))) > 0) {
> +        while (cnt > 0) {
> +            written = write(ofd, buf, (size_t)cnt);
> +            if (written == -1) {
> +                ret = errno;
> +                DEBUG(1, ("Cannot write() to source file '%s': [%d][%
> s].\n",
> +                            dst, ret, strerror(ret)));
> +                goto fail;
> +            }
> +            cnt -= written;
> +        }
> +    }
> 
> You should write like this:
> written = write(ofd, buf + written, cnt);
> Plus, this will require you to set written to 0 before the second while.
> 
> Additionally, I would like to ask someone (Stephen?) to review the
> autoconf/automake stuff in this patch once more. They look reasonable to
> me, but I'm not exactly an expert in this field. Thanks.
> 
> Martin
> 
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel

The autotools stuff looks fine to me, though I wonder if there's a way
to detect whether SELinux is available for the platform at all. I think
the default should be "True on systems where SELinux is available in the
kernel" and "False on systems where SELinux is not available in the kernel".

Unfortunately, I can't think of a way to accomplish this, so I guess
right now it's perfectly fine to force them to set --without-selinux.

- -- 
Stephen Gallagher
RHCE 804006346421761

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkrgQuAACgkQeiVVYja6o6NkegCfbH52KaORaXTlspyyWBfScYIk
bo0AniZGLfUQBdsTFniYKkTf9SYxvIGd
=GBXC
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list