-----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(a)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?
-----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-----