[PATCH] add-shell, remove-shell: new applets

Tito farmatito at tiscali.it
Wed Nov 3 07:04:15 UTC 2010


On Wednesday 03 November 2010 02:48:57 Denys Vlasenko wrote:
> On Wednesday 20 October 2010 17:30, Tito wrote:
> > Hi,
> > was not sure if it could be removed and was to lazy to test.
> > Fixed.
> > Thanks for your hint.
> > Attached v2 of thr patch.
> > Ciao,
> > Tito
> 
> Tito, you are turning update_passwd() into a Franken-function.
> 
> It sings. It dances. It has so many uses it's hard to understand
> both the meaning of its parameters in all its many (nine!) 
> possible ways to be called, and more importantly,
> it's hard to understand its source. Or rather, now it's even
> hardER to understand its source.
> 
> Yes, this way add-shell and remove shell is smaller.
> Is it worth the reduced simplicity?

Hi,
I think yes it is worth it as update_passwd has all the machinery in place for
updating files in /etc that is duplicated in add/remove shell
and was duplicated (or missing) in most of the loginutils
at time it was cobbled together.
So if it does  the safe/right thing for /etc/passwd /etc/group /etc/shadow
/etc/gshadow why not for /etc/shells. Wasn't a goal to reuse existing
code (proven to work). This is difficult without adding a minimum of 
complexity to the code. In this case I just have added 12 lines
to update_passwd to do the work of about 70 lines of code in
add/remove shell.
My personal point of view about simplicity is that if I as a self-taught
programmer understand it, it must be simple code  :-) ,
compared for example to the stuff in sha1 that I never
will fully understand lacking (or maybe having forgot about )
the needed mathematical background.
I agree with you that currently the name of the function in libbb
is misleading and maybe should be renamed in 
bb_safe_update_files_in_etc_and_whatnot.  :-)
But you are the boss....

Ciao,
Tito 

> 
> +
> +#define REMOVE_SHELL (ENABLE_REMOVE_SHELL && (!ENABLE_ADD_SHELL || applet_name[6] == '-'))
> +#define ADD_SHELL    (ENABLE_ADD_SHELL && (!ENABLE_REMOVE_SHELL || applet_name[3] == '-'))
> +
>  int FAST_FUNC update_passwd(const char *filename,
>                 const char *name,
>                 const char *new_passwd,
> @@ -167,7 +177,16 @@
>                 line = xmalloc_fgetline(old_fp);
>                 if (!line) /* EOF/error */
>                         break;
> -               if (strncmp(name_colon, line, user_len) != 0) {
> +               if (ADD_SHELL || REMOVE_SHELL) {
> +                       /* remove colon */      
> +                       name_colon[user_len - 1] = '\0';
> +                       if (strcmp(name_colon, line) != 0) {
> +                               fprintf(new_fp, "%s\n", line);
> +                               goto next;
> +                       } else if (REMOVE_SHELL) {
> +                               changed_lines++;
> +                       }
> +               } else if (strncmp(name_colon, line, user_len) != 0) {
>                         fprintf(new_fp, "%s\n", line);
>                         goto next;
>                 }
> @@ -247,7 +266,7 @@
>                                 bb_error_msg("can't find %s in %s", member, filename);
>                 }
>  #endif
> -               if ((ENABLE_ADDUSER || ENABLE_ADDGROUP)
> +               if ((ENABLE_ADDUSER || ENABLE_ADDGROUP || ENABLE_ADD_SHELL)
>                  && applet_name[0] == 'a' && !member
>                 ) {
>                         /* add user or group */
> 
> 




More information about the busybox mailing list