[haiku-commits] Re: haiku: hrev46860 - in src/kits: shared package/hpkg package/hpkg/v1

  • From: Ingo Weinhold <ingo_weinhold@xxxxxx>
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Mon, 17 Feb 2014 23:05:00 +0100

On 02/17/2014 05:20 PM, Adrien Destugues wrote:
Sorry, I thought I had replied to this already.
I'm unsure how the wrapper can be done in a safe way, as you can't (by design) 
assume it's possible
to use a BDataIO for atomic writes. What would the wrapper do if it can only 
write part of the data
to the underlying BDataIO? should it block until the remaining part can be 
written? Should it
return an error, and risk getting the following Write calls out of sync?

I don't see why the writes have to be atomic. What I'm saying is that
the writes should be complete and fail otherwise.

Ok, I think I understand the difference now.

According to what you write below, unfortunately you don't.

Huh? ATM ZlibDecompressor does not provide the number of bytes written
either *and* you are masquerading incomplete writes as a success!

ZlibDecompressor doesn't have to provide this. as you said, all the code was 
designed for atomic writes (either everything is written, or nothing is). I 
don't see how this can be implemented safely over the BDataIO interface, 
because once you have called BDataIO::Write, and it returns you a number of 
bytes smaller than you expected, you can't get your bytes back. So, yes, the 
wrapper could try to write more bytes in a loop, but if that fails, all it can 
do is return an error code. The caller has no idea if nothing was written at 
all, or maybe some of the data was.

As I wrote in my previous mail (you removed that part in your reply for some reason), the wrapper class can easily provide that information.

I don't understand why you insist on the atomic write thing. BDataOutput::WriteData() method does *not* require atomic writes. It requires to write all data or otherwise fail (regardless of whether it has written any data). That's what keeps the ZlibDecompressor implementation simple. Atomic writes are not required in addition and wouldn't affect ZlibDecompressor at all.

Is it always safe to ignore this? I don't know, and I guess that depends on 
both the caller and the DataIO being used. In some cases, this may be 
recoverable by trying again later (that's the case with the BDynamicBuffer the 
HttpRequest code is currently using). If we don't use this, the whole download 
will have to be restarted.

I don't know what you're referring to with the "this"'s in the paragraph. The situation is the following: BDataIO::Write() may only write a part of the data given. That is common behavior for certain FD types (FIFOs, TTYs, sockets). It is not an error and usually only means that some internal buffer has run full and the next write may block. Simply writing in a loop ensures that everything will be written, unless some other error occurs.

Currently ZlibDecompressor does not write data in such a loop, which means that in case of a partial write the output data will be corrupt and the ZlibDecompressor user won't even be notified by an error code, nor is there any other way the issue could be detected.

So, the write-loop could be factored to the wrapper. But, the ZlibDecompressor 
would still have to ckeck if the loop managed to write all the data, and try 
again later (maybe at the next decompression chunk) if it didn't.

Why on earth should it do such a thing? If a data output can possibly recover from failed write, the respective BDataOutput::WriteData() implementation should handle that, or rather the BDataIO::Write() implementation for that matter.

I don't think BDataOuptput would be easy to use in such a way, with a "write as much 
as you can" policy. Yes, we could have a method return the number of bytes actually 
written, keep track of that in the caller, and get this working. But the code wouldn't be 
that much simpler for the caller. Or, we could try to do this all inside the wrapper, but 
it would probably mean accepting all incoming writes, storing them to an internal buffer, 
and trying to flush them later on. This makes it difficult to handle non-recoverable 
errors in time.

What we need is some kind of "warning" state for the stream: not a complete 
success, but not an irrecoverable error either. BDataIO provides this by allowing partial 
writes. Maybe the EAGAIN error code could be used with BDataOutput, but this would still 
need special handling from the caller. Maybe it would still manage to move some of the 
code to the wrapper.

I really can't follow your train of thought. The only differences between using BDataOutput vs. BDataIO in ZlibDecompressor are 1. whether the more appropriate interface is used and 2. if you want to use a BDataIO whether the loop is implemented in the adapter class or in ZlibDecompressor. Recoverable write errors and other esoteric issues should not be handled in ZlibDecompressor in either case, but rather in the entities that actually know about them.

CU, Ingo


Other related posts: