[SSSD] [PATCH] User home directories management

Martin Nagy mnagy at redhat.com
Thu Oct 22 11:20:14 UTC 2009


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




More information about the sssd-devel mailing list