-----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@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----- _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel