[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