Re: Updated [test]: coreutils-8.24-2

Re: Updated [test]: coreutils-8.24-2

[Fergus]

Yesterday's update from coreutils-8.23-4 to 8.24-1 has done something
horrible to cp. Previously
cp -r {sourcepath}/subdirectory {targetpath}
is allowed even when {targetpath}/subdirectory already exists - which, very
often or even usually, it does.
(A very common context is where a Cygwin user maintains their own
installation resource under /release/ and
seeks to grow the resource after an update! But instances are multiple.)
However under 8.24-1 the instruction is halted with an error message if
{targetpath}/subdirectory already exists.
Surely this is an accident and not an intended property of the update??
I have not tried [test]: coreutils-8.24-2 and I do not know whether this and
maybe other peculiarities of 8.24-1 
have been identified and corrected, but hope that this will eventually
happen?
Thank you!
Fergus



--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: Updated [test]: coreutils-8.24-2

[Fergus]

> .. No, you're the first to report it (so 8.24-2 has the same issue), but
> now that I know about it, it will get fixed soon.

Fantastic, thank you.
Fergus


--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: Updated [test]: coreutils-8.24-2

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 932 bytes --]

On 08/26/2015 04:50 PM, Fergus wrote:
>> .. No, you're the first to report it (so 8.24-2 has the same issue), but
>> now that I know about it, it will get fixed soon.

Spot the bugs:

int
cygwin_spelling (char const *path)
{
...
  int len;
...
  if (! path || ! *path || len > PATH_MAX)
    /* PATH will cause EINVAL or ENAMETOOLONG, treat it as non-existing.  */
    return -1;
  len = strlen (path);


D'oh. But this same flub of mine was also present in at least 8.23-4; so
it was the upstream churn in src/cp.c that caused the stack to be
different to the point that it now matters.

[For those keeping score, I should use size_t and not int to store
strlen() values, since it matters on 64-bit when encountering the
unlikely >2G string; and it helps to never branch on uninitialized memory]

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: Updated [test]: coreutils-8.24-2

[Sam Edge]

On 27/08/2015 00:28, Eric Blake wrote:
> On 08/26/2015 04:50 PM, Fergus wrote:
>>> .. No, you're the first to report it (so 8.24-2 has the same issue), but
>>> now that I know about it, it will get fixed soon.
> Spot the bugs:
>
> int
> cygwin_spelling (char const *path)
> {
> ...
>   int len;
> ...
>   if (! path || ! *path || len > PATH_MAX)
>     /* PATH will cause EINVAL or ENAMETOOLONG, treat it as non-existing.  */
>     return -1;
>   len = strlen (path);
>
>
> D'oh. But this same flub of mine was also present in at least 8.23-4; so
> it was the upstream churn in src/cp.c that caused the stack to be
> different to the point that it now matters.
>
> [For those keeping score, I should use size_t and not int to store
> strlen() values, since it matters on 64-bit when encountering the
> unlikely >2G string; and it helps to never branch on uninitialized memory]
>

One might add, "Always, always initialize automatic variables. This
ensures deterministic behaviour. The compiler will optimise out the
redundant ones."

Rapid diagnosis once reported so I'll let you off this time, Eric. ;-)

-- 
Sam Edge


--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: Updated [test]: coreutils-8.24-2

[Peter Rosin]

On 2015-08-27 21:47, Sam Edge wrote:
> One might add, "Always, always initialize automatic variables. This
> ensures deterministic behaviour. The compiler will optimise out the
> redundant ones."

Seems like a fast way to not get a compiler warning if you do happen
to use a variable that would be used uninitialized if it were not for
an ugly pointless initializer.

Sorry if I added the wrong number of negations in there, but you get my
point anyway, hopefully...

Cheers,
Peter


--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: Updated [test]: coreutils-8.24-2

[Helmut Karlowski]

Am 27.08.2015, 21:26 Uhr, schrieb Peter Rosin:

> Seems like a fast way to not get a compiler warning if you do happen
> to use a variable that would be used uninitialized if it were not for
> an ugly pointless initializer.

> Sorry if I added the wrong number of negations in there, but you get my
> point anyway, hopefully...

Did you mean: "The compiler-warning would inform you that the value is not  
set in a code-path"?

That's why I usually try to avoid initializing auto-variables (and maybe  
save some bytes).

-Helmut


--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: Updated [test]: coreutils-8.24-2

[Sam Edge]

On 27/08/2015 21:33, Helmut Karlowski wrote:
> Did you mean: "The compiler-warning would inform you that the value is
> not set in a code-path"?

I'd agree if I was always in the position of starting projects from
scratch. All compiler warnings should be errors and disabling a compiler
warning has to have a valid justification during review, yes.

But try it with an existing code base. That's when you need
deterministic behaviour from your modifications with minimal disturbance
to anything else.

> That's why I usually try to avoid initializing auto-variables (and
> maybe save some bytes).

If you can afford the tech debt recovery time to change your compiler
warnings to errors or sift through the build output - and prove to your
QA/RA that you don't have to re-certify the product.

But you're unlikely to save any bytes using a modern compiler.

Anyway, we're off-topic so adieu.

-- Sam Edge

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: Updated [test]: coreutils-8.24-2

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1209 bytes --]

On 08/26/2015 04:16 PM, Fergus wrote:
> Yesterday's update from coreutils-8.23-4 to 8.24-1 has done something
> horrible to cp. Previously
> cp -r {sourcepath}/subdirectory {targetpath}
> is allowed even when {targetpath}/subdirectory already exists - which, very
> often or even usually, it does.

> Surely this is an accident and not an intended property of the update??

Not documented in NEWS, so it is an accident. I'm investigating now.  It
does not reproduce on Linux, so it appears to be a bug in my
Cygwin-specific patches; in particular, I had problems where the file
src/cp.c changed upstream between 8.23 and 8.24, and where I may have
misported part of the cygwin-specific patches to that file when carrying
them forward between versions.

> I have not tried [test]: coreutils-8.24-2 and I do not know whether this and
> maybe other peculiarities of 8.24-1 
> have been identified and corrected, but hope that this will eventually
> happen?

No, you're the first to report it (so 8.24-2 has the same issue), but
now that I know about it, it will get fixed soon.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]