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().
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.
+ int debug;
Should be initialized to 0.
In sss_sync_ops.c, userdel_defaults():
Don't forget to free conf_path.
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).
In sss_useradd.c:
+ { "create-home", 'm', POPT_ARG_NONE, NULL, 'm',
_("Crate user's
directory if does not exist"), NULL },
s/Crate/Create/
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.
+ 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;
}
+ 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.
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).
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.
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. 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.
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.
In util/files.c:
I think that almost all DEBUG macros here should really be ERROR macros.
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().
In copy_dir():
You don't check value returned from mkdir().
[1]
http://freeipa.org/page/Coding_Style#Switch