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