[BusyBox] [PATCH] Add -b and -S flags to ln.c.

[Matthew S. Wood]

Vladimir,

> > +				int backup_len = strlen(src) + strlen(suffix) + 2;
> 
> 1) only +1 require

Yes, this is a legitimate off by one bug.  It's a carry over from before the suffix flag was implemented in the patch and was hard coded to "~".
 
> > +				char *backup = xmalloc(backup_len);
> > +				snprintf(backup, backup_len, "%s%s", src, suffix);
> 
> 2) this 3 lines equivalent with one line:
> 	bb_xasprintf(&backup, "%s%s", src, suffix);

Thanks, I'll make the change.

> > +				if (rename(src, backup) < 0 && errno != ENOENT) {
> > +						bb_perror_msg(src);
> > +						status = EXIT_FAILURE;
> > +						break;
> 
> 3) don`t know, but "break" may be non good idea

Break is fine in the context of the function, which does a do {} while() for each link source passed on the command line.  In fact, other loop constructs are used in the existing code (ie, continue.)  This does bring up an interesting point, however, in that GNU coreutils continues on to the next target on backup failure, though it never performs the link operation when rename() fails.  I've updated my patch to reflect this behavior.

> > +				}
> > +				/*
> > +				 * When the source and dest are both hard links, a rename
> > +				 * may succeed even though nothing happened.  Therefore,
> > +				 * always unlink().
> > +				 */
> > +				unlink(src);
> 
> 4) have memory leak, should use free(backup);

This is true, and I certainly don't make a habit of writing any sort of alloc() without a corresponding free().  The reason that there is no free() in this particular case is because all of the other basic busybox applets which I examined, such as date.c, dd.c, fold.c, and others don't free what they've allocated with xmalloc().

The rationale for the lack of a free(), I surmised, is that these applets are short lived.  Further, dealing with possibly many calls to xmalloc() and free() of approximately the same buffer size can cause serious problems for problems for dodgy malloc() implementations.

The counter argument suggests that ln could have a very long list of files which need to be backed up, and memory usage could grow to several kilobytes in such a case.  I believe this argument alone justifies a call to free().  However, in general, this brings up a more general busybox issue:

- The style-guide.txt should be updated to reflect thoughts on memory allocation and deallocation.

Regardless, an updated patch is attached.

Thanks,

Matt
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: busybox_ln-bS.diff
Url: http://lists.busybox.net/pipermail/busybox/attachments/20050418/628ab021/attachment.diff