reworked login.c

[walter harms]

Denys Vlasenko schrieb:
> 
> -#include <selinux/selinux.h>  /* for is_selinux_enabled()  */
> -#include <selinux/get_context_list.h> /* for get_default_context() */
> -#include <selinux/flask.h> /* for security class definitions  */
> +#include <selinux/selinux.h>   /* for is_selinux_enabled()  */
> +#include <selinux/get_context_list.h>  /* for get_default_context() */
> +#include <selinux/flask.h>     /* for security class definitions  */
> 
> NAK. All style cleanups which do not change code should be
> in a separate patch, so that the real changes are clearly seen.
> 
>                 if (ut->ut_pid == pid && ut->ut_line[0] && ut->ut_id[0]
> -                && (ut->ut_type == LOGIN_PROCESS || ut->ut_type == USER_PROCESS)
> -               ) {
> -                       *utptr = *ut; /* struct copy */
> -                       if (run_by_root) /* why only for root? */
> +                       && (ut->ut_type == LOGIN_PROCESS || ut->ut_type == USER_PROCESS)
> +                       ) {
> +                       *utptr = *ut;   /* struct copy */
> +                       if (run_by_root)        /* why only for root? */
>                                 memset(utptr->ut_host, 0, sizeof(utptr->ut_host));
> 
> Same.


i will try to separate then. and produce a series of patches instead of a "new" login.
but not everything will be so stead forward.


> 
> +/*
> +ask sysconf what size we need
> +alloc buffer
> +execute function
> +FIXME:
> +if we want to free the buffer and the pointer we need to return both
> +either as globals (arg) or a special struct (eeeck)
> +
> +*/
> +
> +static
> +struct passwd *my_getpwnam_r(char *username)
> +{
> ...
> +       pwdbuf = xmalloc(len);  /* it will leak for now */
> +       pwdstruct = xmalloc(sizeof(*pwdstruct));
> +       getpwnam_r(username, pwdstruct, pwdbuf, len, &pwdstruct);
> 
> Use xmalloc(sizeof(*pwdstruct) + len) to fix the FIXME.
> 
> 
> +       return -1;
> +
> +}
> +#else
> +/*
> +  FIXME: replace goto with return ?
> +*/

that is reflected in the code. for now it uses
	if blah
	  goto fusel

advantage only one return
	



> +static int check_pswd(char *username, int userlen, struct passwd **pw2)
> +{
> +       struct passwd *pw;
> 
> Comment is confusing. Which goto? I don't see any!
> Why empty line after return.
> 

> 
> +#if  ENABLE_LOGIN_SCRIPTS
> +static void run_login_script(struct passwd *pw, char *full_tty)
> +{
> +       char *t_argv[2];
> +
> +       t_argv[0] = getenv("LOGIN_PRE_SUID_SCRIPT");
> +       if (t_argv[0]) {
> +               t_argv[1] = NULL;
> +               xsetenv("LOGIN_TTY", full_tty);
> +               xsetenv("LOGIN_USER", pw->pw_name);
> +               xsetenv("LOGIN_UID", utoa(pw->pw_uid));
> +               xsetenv("LOGIN_GID", utoa(pw->pw_gid));
> +               xsetenv("LOGIN_SHELL", pw->pw_shell);
> +               spawn_and_wait(t_argv); /* NOMMU-friendly */
> +               unsetenv("LOGIN_TTY");
> +               unsetenv("LOGIN_USER");
> +               unsetenv("LOGIN_UID");
> +               unsetenv("LOGIN_GID");
> +               unsetenv("LOGIN_SHELL");
> +       }
> +
> +}
> +#else
> +static void run_login_script(struct passwd *pw, char *full_tty)
> +{
> +}
> +#endif
> 
> Can you split code movement (from main() to helpers) as a separate patch,
> which do not change any logic, just moves blocks of code?
> 
> 
> 
> -       short_tty = full_tty;
> -       username[0] = '\0';
> +       USE_SELINUX(security_context_t user_sid = NULL;
> +               )
> +               USE_FEATURE_UTMP(struct utmp utent;
> +
> +               )

yep, i did not notice.
since the USE_SELINUX macro has no ";" indent fails to see the eol and gets confused.
if it works it is nice since it removes the whole #ifdef forest


> +
> +               username[0] = '\0';
>         signal(SIGALRM, alarm_handler);
> 
> Looks ugly.
> 
> 
> -       opt = getopt32(argv, "f:h:p", &opt_user, &opt_host);
> -       if (opt & LOGIN_OPT_f) {
> +       getopt32(argv, "f:h:p", &opt_user, &opt_host);
> +       if (option_mask32 & LOGIN_OPT_f) {
> 
> NAK. Old code was shorter (in this case). Try it with make bloatcheck.
> 
> 
> --
> vda
> 
> 
>