<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Mar 12, 2021 at 12:01 AM Rob Landley <<a href="mailto:rob@landley.net">rob@landley.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 3/11/21 10:30 AM, enh via Toybox wrote:<br>
> i don't understand this code in cp.c, specifically the marked line. because of<br>
> the `errno = EXDEV` on the first line of this snippet, and the absence of any<br>
> `errno = 0` in between, what was the `if (errno &&` for?<br>
<br>
Back in 2013 the code originally looked like:<br>
<br>
    errno = EXDEV;<br>
    if (toys.which->name[0] == 'm') rc = rename(src, TT.destname);<br>
<br>
    // Skip nonexistent sources<br>
    if (rc) {<br>
      if (errno != EXDEV ||<br>
        !(new = dirtree_add_node(0, src, !(toys.optflags & (FLAG_d|FLAG_a)))))<br>
          perror_msg("bad '%s'", src);<br>
      else dirtree_handle_callback(new, cp_node);<br>
    }<br>
<br>
The if (errno && stuff) was added by commit 3b9cfa70db in 2019, a commit which<br>
should definitely have set errno to 0 at some point, but did not. (Sorry, I<br>
should have caught that.)<br>
<br>
That patch was actually wrong for multiple reasons, because it doesn't check the<br>
--trail going off the START of the string either, and it's modifying environment<br>
data and thus screwing up what "ps" sees...<br>
<br>
A quick guess at fixing at least a couple of those issues:<br>
<br>
diff --git a/toys/posix/cp.c b/toys/posix/cp.c<br>
index 56fbadf9..24b9d777 100644<br>
--- a/toys/posix/cp.c<br>
+++ b/toys/posix/cp.c<br>
@@ -412,11 +412,11 @@ void cp_main(void)<br>
<br>
   // Loop through sources<br>
   for (i=0; i<toys.optc; i++) {<br>
-    char *src = toys.optargs[i], *trail = src;<br>
+    char *src = toys.optargs[i], *trail;<br>
     int rc = 1;<br>
<br>
-    while (*++trail);<br>
-    if (*--trail == '/') *trail = 0;<br>
+    if (!(trail = strrchr(src, '/')) || trail[1]) trail = 0;<br>
+    else while (trail>src && *trail=='/') *trail-- = 0;<br>
<br>
     if (destdir) {<br>
       char *s = FLAG(D) ? src : getbasename(src);<br>
@@ -453,7 +453,7 @@ void cp_main(void)<br>
         if (exists && no_clobber) rc = 0;<br>
       }<br>
       if (rc) rc = rename(src, TT.destname);<br>
-      if (errno && !*trail) *trail = '/';<br>
+      if (trail) trail[1] = '/';<br>
     }<br>
<br>
     // Copy if we didn't mv, skipping nonexistent sources<br>
<br>
But could I get a second opinion on that since I obviously flubbed allowing this<br>
patch in last time? My blog suggests I was hip-deep in doing the first round of<br>
modern shell stuff. Twitter suggests... oh hey, there's my old tweet explaining<br>
how to fix the audio output on my laptop that's been broken for weeks now:<br>
<br>
  <a href="https://twitter.com/landley/status/1142847348198039553" rel="noreferrer" target="_blank">https://twitter.com/landley/status/1142847348198039553</a><br>
<br>
> it seems from the history like the EXDEV assignment was for the later test of<br>
> `errno != EXDEV` and is really meant to be a "did we successfully move?". i<br>
> don't think that's still valid now the force/clobber code has been added?<br>
<br>
I think the rest of it's ok?<br></blockquote><div><br></div><div>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?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
The if (rc) test still gets set to 0 if rename() failed or if we answered "n" to<br>
yesno, or if -n and dest exists. So we only go into the original errno check<br>
when we need to fallback from mv to cp, and then it checks "is this an error<br>
other than "not same device", which is the only failure we should accept as<br>
"couldn't rename so try to copy contents".<br>
<br>
The if (MV) stanza has to exit past if (rc) rc = rename() so either rc is 0 or<br>
rename() sets errno. So that first assignment only matters to the "copy if we<br>
didn't mv" errno test if we either skipped if (MV) and are looking at the<br>
manually set errno = EXDEV, or if rename() failed and set errno.<br>
<br>
The problem with the other errno test that got added is he assumed that a<br>
successful function will set it to zero rather than leaving it unchanged.<br>
<br>
Rob<br>
</blockquote></div></div>