[Toybox] Regarding usage of cp --preserve

[Rob Landley]

On 08/31/2015 09:40 PM, Hyejin Kim wrote:
> Hi.
>  
> When I adding --preserve=context option into cp --preserve, found some
> abnormal behavior.
> So, try to share with you and want to find right way or modification.

Thanks for the heads up.

I'm halfway through implementing --preserve, I need to add the xattr
parts to it next. But you're right, the comma_scan() thing seems like an
obvious bug.

> Internally, --preserve uses comma_scan() function on parsing attributes.
> But I guess it can't cover the string without last comma.
> like, "cp --preserve=timestamps,mode" or "cp --preserve=timestamps"
>  
>  
> This will display "bad --preserve=mode" or "bad --preserve=timestamps"
>  
> Furthur, I can't understand well the logic parsing first letter of
> attributes.
> if I execute "cp --preserve=mota", it also goes error.

I need to add test suite entries for all of that...

> 340   if (CFG_CP_PRESERVE && TT.c.preserve) {
> 341     char *pre = xstrdup(TT.c.preserve), *s;

The xtrdup() is because modifying environment space is impolite. (It can
make ps give the wrong display info.) Since comma_scan() with the 1
removes seen arguments from the string...

> 342
> 343     if (comma_scan(pre, "all", 1)) TT.pflags = ~0;
> 344     else
> {                                                                                     
> // <-- I just added this for test. and if all string is catched, I think
> we do not need to dig more.

If somebody says --preserve=all,walrus I want an error message. If
somebody says --preserve=all,links that's redundant but not an error.
Parsing removes the recognized elements from the list...

> 345       for (i=0; i<ARRAY_LEN(preserve); i++)
> 346         if (comma_scan(pre, preserve[i], 1)) TT.pflags |= 1<<i;
> 347       if (*pre)
> {                                                                          
> 348
> 349         // Try to interpret as letters, commas won't set anything
> this doesn't.
> 350         for (s = TT.c.preserve; *s; s++)
> {                                        // <-- if have *pre ( in short
> this means remained string without seperating comma), try to compare
> character on by on from original input (TT.c.preserve)

The line below this one:

>                                                                                                          

Has an awful lot of trailing whitespace. Just FYI...

> // I guess, this code allow the combination of first letter format (like
> mota) and full name format(like mode,timestamps), right ? 

Well, not really intentionally.

My reasoning is that no valid long name can be interpreted as a
combination of the short (first letter) versions, each has at least one
letter that nothing starts with. (Although if we add something starting
with r xattr won't.) Since we only remove exact matches, we might as
well scan for long names first, then short names. That way we don't ever
have to _undo_ a previous match. (Ala treating xattr as "xattr, all,
timestamps" and then failing and having to return to some sort of
previous state checkpoint.)

> // For example, "cp --preserve=timestamps,m". this uses both of full
> name format and first letter format. I just wonder Rob's intention.

See above. I just didn't bother to exclude that. I can have the
individual letter check there only if the flags are otherwise 0, but
that's tricky because -a or -p can set pflags.

I the really tricky case is "cp --preserve=links --preserve=mx" which
could happen if there's a cp alias lying around where any call to cp
becomes "cp --preserve=links" and then you add --preserve=mx on your
own. Gotta parse both styles, which means I need to change the
(preserve): type to (preserve)* except I don't think ; works with * yet.
(As I said, this is unfinished. I need to not only implement all these
corner cases, but add test suite entries for them.)

> // Which usage is correct and can be supported?
> 351           printf("*s :%s\n", s);
> 352           for (i=0; i<ARRAY_LEN(preserve); i++)
> 353             if (*s == *preserve[i]) TT.pflags |= 1<<i;
> 354           if (i == ARRAY_LEN(preserve)) {
> 355             if (*s == 'a') TT.pflags = ~0;
> 356             else
> break;                                                                     
> // this compare one character with each "m/o/t/a". *s it just only the
> first letter of user input. if failed to comparing with 'a', breaked. Is
> it right?

No, the enclosing for(;;) loop on line 349 advances s. We compare each s
against the first letter of each preserve[] entry, or against 'a' for all.

The break is if the for loop quoted above hit the end of the preserve[]
array without maching any of them, _then_ it tests a, and if _that_ test
fails it breaks. (Because it couldn't match the letter and needs an
error message.)

Otherwise it should advance through and consume all recognized single
letter input, setting the appropriate flags. (It's possible that's not
what it _is_ doing, but that was the intent...)

>                                                                                                           
> // How can arrive next character if have a string example "mot". this
> has no 'a'. just will be compared by m only then breaked.

Ah, I see the problem. The TT.pflags |= 1<<i; on line 351 should also break;

Yup, bug.

Thanks,

Rob

 1441079168.0