Re: [librsync-devel] Re: [rfc] hide rs_buffers_t from public interface

On 16 Feb 2004, David Helgason <[email protected]> wrote:
> On 16. feb 2004, at 05:36, Martin Pool wrote:
> 
> >I was thinking on the weekend about what could be done to make the
> >librsync interface a bit simpler.
> >
> >It might be better to have the library do all input and output through
> >user callbacks, rather than exposing its buffering at all.  This is to
> >say that something like rs_job_drive would be the main interface, and
> >rs_buffers_t would be removed, or at least not public.  When the
> >library needs some input data, it calls the input callback.  Similarly
> >for output and for reading blocks from the basis file.
> 
> I don't mind this interface at all, but I need the buffer interface to 
> do my work (run the algorithms on data inside postgresql) as I've got 
> no file descriptors  in there. So please don't remove or hide it.
> 
> I also avoided the callback stuff as there wouldn't have been any win 
> there (and a net loss in more complex code and having the code spread 
> out: something I dislike when doing C). The buffer interface may take a 
> few minutes to wrap ones head around, but it's really golden in its 
> straight-forwardness. I'd almost say its a doc problem if anything.
> 
> ...
> 
> Now, looking at my code again, I see that the postgresql read and write 
> functions do have a signature exactly like the normal read/write 
> functions. So your suggestion might just plug'n'play. Hey, cool. Do 
> your worst :)

OK, thanks for your feedback.  Maybe it is worth keeping.

> Ps. I've got:
> 
> 	do {
> 		// Read in more data when emptying the in-buffer
> 		if (bufs.avail_in == 0 && ! bufs.eof_in) {
> 			total_read_delta += bufs.avail_in = 
> 			lo_read(fd_delta, inBuffer, bufSize);
> 			bufs.next_in = inBuffer;
> 			if (total_read_delta == length_delta)
> 				bufs.eof_in = 1;
> 
> 			elog(DEBUG, "Reading %d bytes (eof %d)", 
> 			bufs.avail_in, bufs.eof_in);
> 		}
> 		
> 		// Iterate the job
> 		ok = rs_job_iter (job, &bufs);
> 
> 		// Write out data when filling the out-buffer
> 		if (bufs.avail_out == 0 || bufs.eof_in) {
> 			int bytes = lo_write(fd_out, outBuffer, 
> 			bufs.next_out - outBuffer);
> 			bufs.next_out = outBuffer;
> 			bufs.avail_out = bufSize;
> 			elog(DEBUG, "Writing %d bytes (eof %d)", bytes, 
> 			bufs.eof_in);
> 		}
> 
> 		if (ok == RS_DONE)
> 			break;
> 		if (ok != RS_BLOCKED)
> 			elog(ERROR, "An error occured creating the patch 
> 			(%s)", rs_strerror(ok));
> 	} while (ok == RS_BLOCKED);

So in what I was suggesting, you would basically write little shims
like this:

rs_result lo_write_shim(void *opaque, const char *buf, 
          size_t len, size_t *written)
{
        int bytes;

        bytes = lo_write(*(int *) opaque, buf, len);
        /* handle errors? */
        *written = bytes;
        return RS_DONE;       
}

rs_result lo_read_shim(void *opaque, char *buf, size_t len, size_t
*read_bytes)
{
        int bytes = lo_read(*(int*) opaque, buf, len);
        *read_bytes = bytes;
        /* does lo_read tell you about EOF? */
        return RS_DONE;
}

.....

        result = rs_delta_begin(&job, sigs, lo_read_shim, &fd_in,
                                lo_write_shim, &fd_out);
        if (result != RS_OK)
                return result;

        while ((result = rs_job_run(job)) == RS_BLOCKED)
                ;


I think that will not be noticeably slower, and it is considerably
easier to get right.

-- 
Martin

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQFAMVUWPGPKP6Cz6IsRAl07AJ0cd4Eq58A4NdU2hhWuwNRgtL+nZQCZAeeW
aOcrOgGiXmdNJ3NHZZpsEYg=
=CQyJ
-----END PGP SIGNATURE-----