[Toybox] [HACK] hackfixing cp for compilation

[Rob Landley]

On 11/01/2012 08:16:39 PM, Rob Landley wrote:
> On 10/28/2012 10:42:32 PM, Roy Tam wrote:
>> It is broken for months, so I hackfixed it.
> 
> Sigh. Good point.
> 
> I was fixing cp properly before work and email and life went nuts.
> 
> Let's see...

TT.destname isn't getting initialized. Last commit that worked (before  
the dirtree changes) was 528, so comparing with that. What was I  
thinking?

   http://landley.net/hg/toybox/file/538/toys/cp.c

First line of cp_main() used to be TT.destname =  
toys.optargs[--toys.optc]; and the reason I removed that was:

A) I was thinking about someday adding NOFORK capability (the ability  
for toysh or similar to just call cp_main() as a function, probably  
through some variant of toy_exec(), and clean up after it in the same  
process context without fork/exec overhead). This meant the ability to  
free the argument memory, so permuting the arguments so it lost track  
of one and potentially leaked memory would be a downside. But that's  
bogus because toys.argc still has them all.

B) Adding the ability to tell the optstring to parse non-option  
arguments into the global block, so the <2 fixed arguments would wing  
up in TT without needing to do it by hand. (The idea was getting it to  
automatically do the type conversions.) I poked at this a bit, but  
couldn't work out a good way to express it that actually reduced  
complexity.

So, putting that back in...

Next up, TT.destisnew was only ever set, not read, so I yanked that  
case, tested for the other one, and unified the else cases to avoid the  
goto that was combining the two error_exit() cases.

That gets us to the TODO, which is about this problem:

   readlink -f ./nonexistent : prints absolute path of where it _would_  
go.
   realpath("./nonexistent", NULL): returns NULL

This is actually why toybox's readlink defaults to "n" for now, I need  
to make this case work.

However, what I'm trying to do here is duplicate detection, as  
mentioned in the previous reply...

Rob

>> diff -U8 toys/posix/cp.c toys/posix/cp.c_hf
>> --- toys/posix/cp.c     2012-10-27 10:15:31.000000000 +0800
>> +++ toys/posix/cp.c_hf  2012-10-29 11:41:01.000000000 +0800
>> @@ -171,37 +171,37 @@
>> 
>>         // Loop through sources
>> 
>>         for (i=0; i<toys.optc; i++) {
>>                 char *dst, *src = toys.optargs[i];
>> 
>>                 // Skip src==dest (TODO check inodes to catch "cp  
>> blah ./blah").
>> 
>> -               if (!strncmp(src, TT.destname)) continue;
>> +               if (!strcmp(src, TT.destname)) continue;
> 
> Sigh.  Looks like I checked in half-finished work at some point.  
> (Possibly when I categorized commands into subdirectories.)
> 
> What I was going for here is trying to catch destinations that are  
> _under_ source, ala:
> 
>   cp -r dir dir/sub
> 
> If I'm using cannonicalized absolute paths (with all the symlinks  
> resolved), then a
> matching prefix tells me I've got an endless loop coming up. But I  
> also need to catch:
> 
>   cp * .
> 
> Last I poked at this, I'm was trying to work out what all the cases  
> were and a
> graceful way to catch them without a forest of special cases...



>>                 // Skip nonexistent sources.
>> 
>>                 TT.keep_symlinks = toys.optflags & (FLAG_d|FLAG_a);
>>                 if (TT.keep_symlinks ? lstat(src, &st) : stat(src,  
>> &st)
>> -                       || (st.st_dev = dst.st_dev && st.st_ino ==  
>> dst.dst_ino))
>> +                       /*|| (st.st_dev = dst.st_dev && st.st_ino ==
>> dst.dst_ino)*/)
> 
> Another variant of the same thing. I was stepping back and trying to  
> collate it all into one test.
> 
> Unfortunately, "endless recursion with -r" and "copy over itself"  
> seem like the need to be two separate tests.
> 
>>                 {
>>  objection:
>>                         perror_msg("bad '%s'", src);
>>                         toys.exitval = 1;
>>                         continue;
>>                 }
>> 
>>                 // Copy directory or file.
>> 
>>                 if (TT.destisdir) {
>>                         char *s;
>> 
>>                         // Catch "cp -R .. ." and friends that would
>> go on forever
>> -                       if (dpath && (s = realpath(src, NULL)) {
>> +                       if (dpath && (s = realpath(src, NULL))) {
> 
> Ok, that's just embarassing. But it also means "I never actually  
> tested this code, and it needs a largeish test suite entry because  
> it's a pile of corner cases with a bunch of command line options".
> 
> I'm poking at it.
> 
> Thanks,
> 
> Rob

 1353368080.0