Closed spbooth closed 9 months ago
@spbooth : I have updated the PR after reviewing it.
There are two changes that are not only white space corrections. a) I moved the close(fd) statement up into the if(this is a file) conditional, since it is opened inside it. b) Since the mdctx now lives much longer and there are goto errors while it exists I added its destruction to the error branch and adjusted some of the goto labels.
Let me know if I have misunderstood the intentions of the PR.
@msalle: Are you OK with the changes made?
Can we merge this?
Can we merge this?
@ellert What are your thoughts on my comment https://github.com/gridcf/gct/pull/216/files#r1290396767
Can we merge this?
@ellert What are your thoughts on my comment https://github.com/gridcf/gct/pull/216/files#r1290396767
I already gave a long reply to this in the discussion above. As far as I can see the code is correct as it is.
Can we merge this?
@ellert What are your thoughts on my comment https://github.com/gridcf/gct/pull/216/files#r1290396767
I already gave a long reply to this in the discussion above. As far as I can see the code is correct as it is.
Not sure where you mean?
Just to clarify, my question is specifically about the lines 2165-2169 in globus_gass_copy_glob.c in Stephen's fork
Note that read_left
is set to length
in line 2128.
In the 5 lines in the while loop, n
keeps being subtracted from read_left
but the check is whether length
is bigger equal 0 which doesn't seem to make sense unless I completely miss the reasoning? I would expect a check such as if (read_left >= n)
The lines are there in the original code too, see lines 2152-2156 there but since we are reordering and in any case fixing a bug, I think we should check this part is correct.
Can we merge this?
@ellert What are your thoughts on my comment https://github.com/gridcf/gct/pull/216/files#r1290396767
I already gave a long reply to this in the discussion above. As far as I can see the code is correct as it is.
Not sure where you mean? Just to clarify, my question is specifically about the lines 2165-2169 in globus_gass_copy_glob.c in Stephen's fork Note that
read_left
is set tolength
in line 2128. In the 5 lines in the while loop,n
keeps being subtracted fromread_left
but the check is whetherlength
is bigger equal 0 which doesn't seem to make sense unless I completely miss the reasoning? I would expect a check such asif (read_left >= n)
The lines are there in the original code too, see lines 2152-2156 there but since we are reordering and in any case fixing a bug, I think we should check this part is correct.
Here I quote my reply again. The check is supposed to check if the original value of length
was negative since this is the condition that determines if the bytes_left
counter is used or not.
Either length
is negative, and the routine reads all data until the end of the file, in this case the bytes_left
counter is not used, and count
stays constant. Or length
is non-negative and the routine keeps track of the number of bytes read, i.e. the bytes_left
counter is used, and count
is adjusted to never exceed bytes_left
.
The check input >= 0
determines whether bytes_left
and count
should be adjusted after each call to read()
or not.
When the bytes_left
counter is used, count
is never greater than bytes_left
. The n
returned from read()
is never greater than count
, so bytes_left
will never become negative when decreased by n
, so count
is never negative.
When the bytes_left
counter is not used, count
is never changed, so count
is never negative.
Can we merge this?
@ellert What are your thoughts on my comment https://github.com/gridcf/gct/pull/216/files#r1290396767
I already gave a long reply to this in the discussion above. As far as I can see the code is correct as it is.
Not sure where you mean? Just to clarify, my question is specifically about the lines 2165-2169 in globus_gass_copy_glob.c in Stephen's fork Note that
read_left
is set tolength
in line 2128. In the 5 lines in the while loop,n
keeps being subtracted fromread_left
but the check is whetherlength
is bigger equal 0 which doesn't seem to make sense unless I completely miss the reasoning? I would expect a check such asif (read_left >= n)
The lines are there in the original code too, see lines 2152-2156 there but since we are reordering and in any case fixing a bug, I think we should check this part is correct.Here I quote my reply again. The check is supposed to check if the original value of
length
was negative since this is the condition that determines if thebytes_left
counter is used or not.Either
length
is negative, and the routine reads all data until the end of the file, in this case thebytes_left
counter is not used, andcount
stays constant. Orlength
is non-negative and the routine keeps track of the number of bytes read, i.e. thebytes_left
counter is used, andcount
is adjusted to never exceedbytes_left
.The check
input >= 0
determines whetherbytes_left
andcount
should be adjusted after each call toread()
or not.When the
bytes_left
counter is used,count
is never greater thanbytes_left
. Then
returned fromread()
is never greater thancount
, sobytes_left
will never become negative when decreased byn
, socount
is never negative.When the
bytes_left
counter is not used,count
is never changed, socount
is never negative.
Ah, I understand, thanks!
I must say it's not the most clear code (I would personally have moved the block initialising read_left
and count
down to just before the while
loop, and moved the if (length>=0)
outside the while loop and combine with the other if (length>=0
), but better leave it as is for now.
Merging this now. Luckily no CI tests timed out on Travis this time.
suggested fix for issue #215