[PATCH] to add Mark Lord's symlinks to busybox

[Claus Klein]

Hi Denys,

I was a little busy, but now I hope it is right.

I have tried to add a testsuit for symlinks too.
Perhaps you should have a look at the option handling, I am not sure if 
it is realy better exept that the order is no more flexible.

Cheers
Claus

Denys Vlasenko schrieb:
> On Sunday 30 March 2008 10:45, Claus Klein wrote:
>   
>> In my opinion it is recommended for use by anyone developing and/or 
>> maintaining a Linux distribution or CD-ROM.
>> Too I added a chroot option  to be able to check the symlinks in a 
>> target root file system on a development host
>> to prevent dead links on the embedded system.
>>     
>
> Sounds ok. Let's see what we have there.
>
> +static char *progname;
>
> No need for this, char *applet_name is provided already.
>
> +static int substr(char *s, const char *old, const char *new)
> +{
> +    char *tmp = NULL;
> +    int oldlen = strlen(old), newlen = 0;
>
> Entire file uses spaces for indentation. Needs to be ran thru unexpand
> and convert leading 4-space blocks into tabs.
>
> +        if ((tmp = malloc(strlen(s))) == NULL) {
>
> Assignments inside if() hurt readability. Please do them
> immediately before if() instead. (Not applicable
> to assignments in while()).
>
> In this particular case, however, just use xmalloc().
> It never returns NULL.
>
> +            bb_simple_perror_msg_and_die("no memory\n");
>
> bb_XXXXerror_msg[_and_die]() doesn't need trailing \n.
>
> +        while ((*s++ = *p++));
>
> I prefer
> 	while ((*s++ = *p++) != '\0')
>               continue;
>
> for readability reasons. However, in this particular case
> you can just use strcpy() instead.
>
> +    if (tmp)
> +        free(tmp);
>
> Just free(tmp). free(NULL) is safe.
>
> +        for (s = p + 3; p != path && *--p != '/';);
>
> Not very readable.
>
> +static int shorten_path(char *path, char *abspath)
> +{
> +    static char dir[PATH_MAX];
>
> Such big static objects are no-no in busybox (NOMMU systems will be hurt).
> xmalloc them.
>
> +    int shortened = 0;
> +    char *p;
> +
> +    /* get rid of unnecessary "../dir" sequences */
> +    while (abspath && strlen(abspath) > 1 && (p = strstr(path, "../"))) {
>
> strlen() > 1 can be open-coded - code will be smaller.
>
> +static void dirwalk(char *path, int pathlen, dev_t dev)
> +{
> +    char *name;
> +    DIR *dfd;
> +    static struct stat st;
> +    static struct dirent *dp;
>
> Seems like they do not need to be static.
>
> +int symlinks_main(int argc, char **argv)
> +{
> +    while (--argc) {
> +        p = *++argv;
> +        if (*p == '-') {
> +            if (*++p == '\0')
>
> replace by call to getopt32 (look in other applets for examples).
>
> +                    if (chdir(path) == -1)
> +                        bb_perror_msg_and_die("chdir(%s) failed\n", path);
>
> xchdir().
>
> +                    if (NULL == getcwd(cwd, PATH_MAX)) {
> +                        bb_simple_perror_msg_and_die("getcwd() failed\n");
> +                    }
>
> boox has xrealloc_getcwd_or_warn(). Wrap it into
> a helper which exits on failure, and use that.
> (because you have at least two callsites).
>
> --
> vda
>   

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: symlinks.diff
Url: http://busybox.net/lists/busybox/attachments/20080405/4d9d8ff7/attachment-0001.txt