On 11/22/2010 05:52, Alan Bateman wrote:
Xueming Shen wrote:
Alan,
After staring those simple, 11 lines of change for minutes, I believe
we should simply
go back with the original approach at
http://cr.openjdk.java.net/~sherman/6858865/webrev.00
The change in
http://cr.openjdk.java.net/~sherman/6858865/webrev.00
obviously is problematic, especially in case like in_len == 0 but
out_len != 0 (no more,
or no more new input, but with a valid output buffer), it's
definitely possible the
inflater/deflater might have more bites to (and should) output in
this scenario, it's a
bug to return 0 here.
Sure it's possible to go further like
if (in_len == 0)
return 0;
....
if (len == 0)
...
But given the purpose of this fix is to solve that particular
"regression" (which actually
does not cause any regression for "mainstream" platforms), I prefer
not take the risk
of causing another real regression/behavior change here, especially
we got burned
couple times here in the past when tried to do better.
Agree?
-Sherman
I understand the concern but I don't think that patch-1 is risky. That
is, for Inflater, I can't see any side effects to simply returning 0
if either length is 0. Same thing for Deflater when not in the code
path that calls deflateParams. For the deflateParams cases then the
changes in the latest patch is fine with me.
We can not simply return 0 if the "out" buffer length is NOT 0 even when
"in" buffer length is 0. The de/inflater might
have something to push/flush out even there is no more input. Consider
the buffer in last invocation of de/inflating is
not big enough, de/infalte() gets invoked the second time without any
new data (and the data for the last invocation
has been consumed by the zlib de/inflate). The patch-1 (the second one)
is simply wrong.
It is probably OK to return 0 if the output buffer is 0, I said
"probably" is because I would need to read all those
zlib code again to make sure there is no any internal status change (for
example in case like the "in" buffer is NOT
zero but the "out" buffer is 0, consider the de/inflater might try to
consume more data even the output buffer is
zero) or even if there is an internal status change it would not have
any negative impact. This is why I'm saying
it is probably not worth the effect.
-Sherman
