[Toybox] cp.c

[enh]

On Fri, Mar 12, 2021 at 12:01 AM Rob Landley <rob at landley.net> wrote:

> On 3/11/21 10:30 AM, enh via Toybox wrote:
> > i don't understand this code in cp.c, specifically the marked line.
> because of
> > the `errno = EXDEV` on the first line of this snippet, and the absence
> of any
> > `errno = 0` in between, what was the `if (errno &&` for?
>
> Back in 2013 the code originally looked like:
>
>     errno = EXDEV;
>     if (toys.which->name[0] == 'm') rc = rename(src, TT.destname);
>
>     // Skip nonexistent sources
>     if (rc) {
>       if (errno != EXDEV ||
>         !(new = dirtree_add_node(0, src, !(toys.optflags &
> (FLAG_d|FLAG_a)))))
>           perror_msg("bad '%s'", src);
>       else dirtree_handle_callback(new, cp_node);
>     }
>
> The if (errno && stuff) was added by commit 3b9cfa70db in 2019, a commit
> which
> should definitely have set errno to 0 at some point, but did not. (Sorry, I
> should have caught that.)
>
> That patch was actually wrong for multiple reasons, because it doesn't
> check the
> --trail going off the START of the string either, and it's modifying
> environment
> data and thus screwing up what "ps" sees...
>
> A quick guess at fixing at least a couple of those issues:
>
> diff --git a/toys/posix/cp.c b/toys/posix/cp.c
> index 56fbadf9..24b9d777 100644
> --- a/toys/posix/cp.c
> +++ b/toys/posix/cp.c
> @@ -412,11 +412,11 @@ void cp_main(void)
>
>    // Loop through sources
>    for (i=0; i<toys.optc; i++) {
> -    char *src = toys.optargs[i], *trail = src;
> +    char *src = toys.optargs[i], *trail;
>      int rc = 1;
>
> -    while (*++trail);
> -    if (*--trail == '/') *trail = 0;
> +    if (!(trail = strrchr(src, '/')) || trail[1]) trail = 0;
> +    else while (trail>src && *trail=='/') *trail-- = 0;
>
>      if (destdir) {
>        char *s = FLAG(D) ? src : getbasename(src);
> @@ -453,7 +453,7 @@ void cp_main(void)
>          if (exists && no_clobber) rc = 0;
>        }
>        if (rc) rc = rename(src, TT.destname);
> -      if (errno && !*trail) *trail = '/';
> +      if (trail) trail[1] = '/';
>      }
>
>      // Copy if we didn't mv, skipping nonexistent sources
>
> But could I get a second opinion on that since I obviously flubbed
> allowing this
> patch in last time? My blog suggests I was hip-deep in doing the first
> round of
> modern shell stuff. Twitter suggests... oh hey, there's my old tweet
> explaining
> how to fix the audio output on my laptop that's been broken for weeks now:
>
>   https://twitter.com/landley/status/1142847348198039553
>
> > it seems from the history like the EXDEV assignment was for the later
> test of
> > `errno != EXDEV` and is really meant to be a "did we successfully
> move?". i
> > don't think that's still valid now the force/clobber code has been added?
>
> I think the rest of it's ok?
>

yeah, i _think_ so, but i worry that it's past the level of complexity that
humans can cope with. sure you don't just want an explicit `moved` boolean?


> The if (rc) test still gets set to 0 if rename() failed or if we answered
> "n" to
> yesno, or if -n and dest exists. So we only go into the original errno
> check
> when we need to fallback from mv to cp, and then it checks "is this an
> error
> other than "not same device", which is the only failure we should accept as
> "couldn't rename so try to copy contents".
>
> The if (MV) stanza has to exit past if (rc) rc = rename() so either rc is
> 0 or
> rename() sets errno. So that first assignment only matters to the "copy if
> we
> didn't mv" errno test if we either skipped if (MV) and are looking at the
> manually set errno = EXDEV, or if rename() failed and set errno.
>
> The problem with the other errno test that got added is he assumed that a
> successful function will set it to zero rather than leaving it unchanged.
>
> Rob
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20210312/7ee87f63/attachment-0001.htm>