-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 10/21/2009 09:05 AM, Martin Nagy wrote:
Hi, sorry, I accidentally sent the email too early. I'll continue
where
I left, see below.
On Wed, 2009-10-21 at 06:39 +0200, Martin Nagy wrote:
> On Tue, 2009-10-20 at 13:33 +0200, Jakub Hrozek wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> On 10/19/2009 11:32 AM, Jakub Hrozek wrote:
>>> On 10/15/2009 04:18 PM, Jakub Hrozek wrote:
>>>>> User home directories management
>>>>>
>>>>> Create and populate user directories on useradd, delete them on
userdel
>>>>>
>>>>> Fixes: #212
>>> Rebased on top of 02bf83eaf9eb9733536c6c56afcbc70e312b3cd6
>>>
>>> Jakub
>>
>> ...and now with the correct patch. Sorry for any confusion.
>
> +
<option>-M</option>,<option>--not-create-home</option>
> + { "not-create-home", 'M', POPT_ARG_NONE, NULL,
'M', _("Never
> create user's directory, overrides config"), NULL },
>
> Can we make this --no-create-home instead? It somehow sounds better for
> me and I think it's something of a custom to do it that way.
>
> + <para>
> + Do not create the user's home directory, even
> + if the system wide setting from is set to TRUE.
> + </para>
> Sounds like the "from" doesn't belong there. Anyway, maybe saying
> something like "Overrides system wide setting(s?)." instead?
>
> +
<option>-R</option>,<option>--not-remove</option>
> Same as above.
>
> + Files in the user's home directory will NOT be
> + removed along with the home directory itself and
> + the user's mail spool. Overrides the configuration.
> "along with the home directory itself and the user's mail spool" seems
> to be a little bit redundant.
>
> + and directories to be copied in the user´s
> Bad apostrophe, should be "'".
>
> In server/python/pysss.c:
> + int remove_home;
> This should be initialized to 0.
>
> In files-tests.c:
> Being a PITA: You have some symbols here (both functions and variables)
> that should be static. Also, tpl_dir should be const and tpl_filename is
> not used anywhere.
>
> +static int create_simple_file(const char *name, const char *content)
> +{
> + int fd = -1;
> The initialization is not needed. BTW, you can safely make this function
> return void and eliminate the last return. Remember that fail_if()
> terminates the process. This would save you couple of check later after
> calling create_simple_file().
>
All the above issues should be fixed.
> You could also make very simple wrappers around standard
functions you
> use a lot like mkdir(), chdir(), access(), etc. For example, something
> like this:
>
> static void xchdir(const char *path)
> {
> int ret;
> const char *err_str;
> ret = chdir(path);
> if (ret != 0) {
> err_str = strerror(errno);
> fail("Failed to chdir to %s: %s", path, err_str);
> }
> }
>
> This is just a suggestion, I won't NACK because of it, but IMHO the
> tests would look nicer and be easier to read since you don't have to put
> so much fail_if's after every function call.
>
Umm I'd like to avoid rewriting the code, but it sure is doable if required.
> + int debug;
> Should be initialized to 0.
>
done
> In sss_sync_ops.c, userdel_defaults():
> Don't forget to free conf_path.
>
done
> In useradd_defaults():
> + DEBUG(7, ("Mail dir: %s\n", data->home));
> Should be data->maildir. Jakub pointed out this one himself while
> looking over my shoulder :) Btw, this function looks a bit messy when it
> comes to cleaning up after failure, please fix this. Could you also try
> to reduce the number of variables in it? For example, you could remove
> maildir by passing &data->maildir to confdb_get_string() directly. Same
> goes for dfl_shell, dfl_create_home and umask. Other variables could
> probably be made more local. Reducing the number of variables will make
> the function a lot more easier to read (less things to keep track on).
>
the function was tidied up
> In sss_useradd.c:
> + { "create-home", 'm', POPT_ARG_NONE, NULL, 'm',
_("Crate user's
> directory if does not exist"), NULL },
>
> s/Crate/Create/
>
done
> 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.
> + if (ret != EOK) {
> + switch (ret) {
> + case EEXIST:
> + ERROR("User's home directory already exists, not
> copying"
> + "data from skeldir\n");
> + break;
> +
> + default:
> + ERROR("Cannot create user's home directory\n");
> + ret = EXIT_FAILURE;
> + goto fini;
> + }
> + }
> Suggestion: Use the more idiomatic and easier to read:
> if (ret == EEXIST) {
> ERROR("User's home directory already exists, not copying"
> "data from skeldir\n");
> } else if (ret != EOK) {
> ERROR("Cannot create user's home directory\n");
> ret = EXIT_FAILURE;
> goto fini;
> }
>
Good suggestion, thanks.
> + 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.
> In server/tools/sss_userdel.c:
> + if (ret != EOK) {
> + switch (ret) {
> + case EPERM:
> + ERROR("Not removing home dir - not owned by user
> \n");
> + break;
> +
> + default:
> + ERROR("Cannot remove homedir\n");
> + ret = EXIT_FAILURE;
> + goto fini;
> + }
> + }
> Same as above.
>
> In server/tools/tools_util.c:
> In is_owner():
> + DEBUG(1, ("Cannot stat %s: %d\n", path, ret));
> Use %s and strerror(ret) instead please.
>
> >From remove_mail_spool():
> + switch (ret) {
> + case 0:
> + break;
> + case -1:
> + DEBUG(3, ("%s not owned by %d, not removing\n",
> + spool_file, uid));
> + ret = EACCES;
> + default:
> + goto fail;
> + }
> Per [1], please add a /* FALLTHROUGH */ comment after ret = EACCESS.
>
> + DEBUG(1, ("Cannot remove() the spool file\n"));
> Please add ": %s" and strerror(ret).
>
All the above comments should be fixed.
> In remove_homedir():
> + ret = remove_mail_spool(mem_ctx, maildir, username, uid, force);
> + if (ret != EOK) {
> + DEBUG(1, ("Cannot remove user's mail spool"));
> + /* Should this be fatal? I don't think so. Maybe convert to
> ERROR? */
> + }
> The DEBUG statement is missing a \n. I think you should ignore the
> return value (so this shouldn't be fatal) and not print anything.
> Rather, I would convert the DEBUG() inside of remove_mail_spool() to an
> ERROR().
>
> + DEBUG(1, ("Cannot remove homedir based on %s: %d\n",
> + homedir, ret));
> Use strerror() and ERROR(). "based on " is probably redundant.
>
OK, here's my problem with ERROR -- the call prints directly to stderr
and these functions like remove_homedir et al. are used also from python
bindings where instead of printing the error you want to throw an
exception. I honestly don't know a very good way to solve it, I even
toyed with the idea of passing out an error string if ret != EOK.
> For the selinux related stuff.. Is selinux_file_context() really
> necessary? I don't know about selinux that much, but I'd expect that
> when you create a new file it will possess the right selinux context
> automatically.
It will posses the right *default* selinux context automatically. But
some files (for example ~/public_html) must have different contexts than
default files created in home directory, selinux_file_context() sets it.
What the routines do roughly equals to calling restorecon <fullpath>
after creating files or directories.
This can be trivially tested by configuring SSSD --without-selinux,
adding public_html to /etc/skel and then running sss_useradd.
> Also, I think it would be nice to only use HAVE_SELINUX
> when defining the functions, like this:
>
> #ifdef HAVE_FOO
> int fun1(int a, int b, int c)
> {
> /* Code here. */
> return EOK;
> }
>
> void fun2(int a)
> {
> /* Code here. */
> }
> #else /* !HAVE_FOO */
> #define fun1(a, b, c) EOK
> #define fun2(a) ((void)0)
> #endif /* HAVE_FOO */
>
> You then don't have to use HAVE_FOO every time you need fun1() or
> fun2(). But in our case, you would also have to make a simple wrapper
> (or better yet, a simple define) around setfscreatecon() to avoid using
> HAVE_FOO for some functions and not for others.
>
done, thanks
> BTW, in create_mail_spool(), you don't reset the create
context with
> setfscreatecon(NULL) as you do in create_homedir(). Is this intentional?
>
> In create_homedir() you set selinux file context but if mkdir(), chown()
> or chmod() fails, you won't reset it. If my understanding of what
> selinux_file_context() does is correct, this could be a potential
> security hazard, since the context will be used for the next created
> file.
>
both resets were fixed
> In util/files.c:
> I think that almost all DEBUG macros here should really be ERROR macros.
See above for DEBUG vs. ERROR
> Furthermore, please consider making wrappers for the common
functions
> that would do the error checking and use the ERROR() macro for errors. I
> would suggest that you do this for at least functions opendir(),
> lstat(), closedir(), rmdir(), mkdir(), chown(), chmod(), utimes(),
> symlink(), mknod(), open(), fchown(), fchmod() and close().
There might be more functions that I missed.
It would be nice to have these wrappers somewhere in a separate file in
util/. They should have a prefix like x, sss_, or something similar.
Example:
int xmkdir(const char *pathname, mode_t mode)
{
int ret;
ret = mkdir(pathname, mode);
if (ret != 0) {
ret = errno;
ERROR("Failed to create directory %s: %s", pathname,
strerror(ret));
}
return ret;
}
When we have these wrappers it is much easier to write and read code
since all the unimportant stuff is already taken care of. The resulting
binary will also won't be so big because of all those strings. And most
importantly, you don't have to duplicate the code (and potentially
bugs).
If you decide to follow this approach, I'd also recommend using the
functions like this:
Dunno, I generally prefer to just check the return value than wrappers,
you can nicely see where exactly it failed with the DEBUG. A macro would
solve this, though...is there a general consensus on what to use?
ret = xmkdir(path, mode);
if (ret != 0) goto fail;
IIRC Simo and Stephen shorten if's in this way often, it's a nice way to
not take the attention away from the calls themselves.
> In copy_dir():
> You don't check value returned from mkdir().
ret = chown(dst, (uid == - 1) ? statp->st_uid : (uid_t) uid,
(gid == - 1) ? statp->st_gid : (gid_t) gid);
Sorry for nitpicking, but please no space between '-' and '1'. Also, I
might be wrong but I think uid_t and gid_t are unsigned types. Since you
have a cast there, I'm guessing you wanted to define them as signed?
Yeah..after some thinking I decided that the "no uid set" check was not
neccessary and just removed it..
Perhaps specifying that mt needs to be array of 2 members would be
nice
(same as utimes()).
done
I just noticed that in a lot of functions in files.c you don't
give much
attention to freeing memory if an error occurs. I also noticed that this
is ok since those are static functions and the caller takes care of
freeing the whole temporary context at the end.
Both copy_tree and remove_tree are recursive, the context can be freed
only in the top level function.
However, at least a
comment for each function that doesn't care about memory because of this
should be added.
OK, added comments
Same with setting selinux_file_context(). Although here we should
maybe
reset the context anyway, just to be sure since it might be a source of
security issues.
In copy_symlink():
Same issue with uid/gid signedness as in copy_dir().
In copy_special():
Same as above.
In copy_file():
Same as above. Hm, you use this code pretty often, maybe a macro would
be appropriate?
Removed from all three
+ while ((cnt = read(ifd, buf, sizeof(buf))) > 0) {
+ if (cnt == -1) {
+ ret = errno;
+ DEBUG(1, ("Cannot read() from source file '%s': [%d][%
s].\n",
+ dst, ret, strerror(ret)));
+ goto fail;
+ }
Move the if block after the loop, otherwise it will never execute.
Good catch.
+ if (write(ofd, buf, (size_t)cnt) != cnt) {
+ ret = errno;
You have to be able to handle cases when write returns something > -1
but < cnt (write could have been interrupted by a signal handler, etc.).
Fixed
Also, I'm not sure if we shouldn't handle EAGAIN cases for
both read and
write. I don't know if a blocking I/O is the default on all platforms.
+ close(ifd);
+ ifd = -1;
Error handling here is missing.
Fixed
In copy_entry():
+ ret = copy_symlink(cctx, src, dst, &sb, mt, uid, gid);
+ if (ret != EOK) {
+ DEBUG(1, ("Cannot copy symlink '%s' to '%s':
[%d][%s]\n",
+ src, dst, ret, strerror(ret)));
+ }
+ return ret;
Suggestion: move the error message into copy_symlink() and just do
"return copy_symlink();". Same with copy_dir(), copy_special(),
copy_file().
Done. I think the error messages were extra since the copy_XXX()
functions all have their error reporting.
Martin
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora -
http://enigmail.mozdev.org/
iEYEARECAAYFAkrfU0cACgkQHsardTLnvCVQCACfb/bwZZcD4RJD6NYzK8HK6Ko7
2ywAoJfJo1T1sBl43ZX8nfMt3s6H24Uc
=63F3
-----END PGP SIGNATURE-----