[Libburn] patch enabling arbitrary fd within burn_source

[Derek Foreman]

On Sun, 11 Dec 2005, Thomas Schmitt wrote:

> Hi,
>
> Find attached my patch against the CVS version of December 8, 2005.
> (I would appreciate any hints about submitting patches properly.)

This looks fine to me.

> cdrskin runs with the patched CVS version (after a little
> #ifdef because of the change in struct burn_progress).
> So the changes seem not to be overly poisonous or incomplete.
>
> As soon as the fate of these changes is decided i will
> adapt cdrskin to the new CVS version and develop a
> proposal how it could be included in libburn.
> Currently its uncompressed weight is 39670 bytes.
>
>
> Some comments :
> ---------------------------------------------------------------
> I am still undecided wether i should put emphasis on the
> new constructor
>  struct burn_source *burn_file_source_new3(const char *path,
>                                          const char *subpath,
>                                          int fixed_size)
>
> It could be obsoleted by
> - refering interested users of existing fd to /dev/fd/#
>  resp /proc/self/fd/#
> - manipulating  source->fixed_size  directly in the application
>  (currently i do this anyway for my padding workaround)

I'd allow for a setter function, but I don't want the app poking around 
in the struct directly.

I think I prefer a new constructor with a "size hint", and if it's 
non-zero it'll prevent any checks of the file size.

Also, it might be best to force the application to open the files, and 
just always expect file descriptors.

Would this work for you?

> On the other hand, both above gestures are not overly
> clean in respect to system abstraction and data encapsulation.
> So i still propose this new function.

Agree.  New constructor is required.

> ---------------------------------------------------------------
> I introduced  burn_source.delivered_bytes  and
> burn_source.recent_bytes which are not used by any code outside
> file_read()  yet.
>
> burn_source.recent_bytes tackles the problem that on error read(2)
> is free to either
>  return -1 immediately
> or to
>  return a partial buffer and report the error on the next call.
>
> My file_read() implements the former behavior because it is too
> risky to hope for a repetition of the error on the next read
> call. I understand that libburn is mainly interested in reading
> whole sectors.
> But by help of burn_source.recent_bytes one can retrieve an
> eventual valid partial buffer after  file_read()  returned -1.
>
> So if libburn wants partial buffers on error (note: this is not
> about EOF) then it should handle in  sector.c:get_bytes()
> an eventual non-zero src->recent_bytes as valid data before
> it starts its padding.
>
>
> burn_source.delivered_bytes is just an idle counter yet.
> I believe that any good data source object should have such a
> counter of total bytes. Applications for that will emerge.

We've already got the burn_progress struct and burn_drive_get_status...

Is this just two ways of viewing nearly identical information?

My current thinking is that we just shouldn't return short from the source 
function, unless it's EOF.  The source can block (and the drive can 
underrun...)

> Is there any specification for the contract between burn_source
> and burn_source.read() ?
> In the API docs i only find this description:
>  int read(struct burn_source *, unsigned char *buffer, int size)
>  Read data from the source.
> If i design my own Yoyodyne-read() what would it have to do
> so it doesn't break libburn ?

Never try to put more than "size" bytes into "buffer".
Other than that, you can do most anything you want in there.  But try to 
do it before the drive's buffer depletes. :)

I think a return of -1 should mean there are no more bytes to be found 
here, ever.

> ---------------------------------------------------------------
> I revoke all my proposals and observations concerning padding
> at the end of payload data. Currently i work around that problem
> by
>  src->fixed_size = burn_source_get_size(src) + skin->padding;
>
> It would be nice if someone could have a look into
> the possibility to do anything with the parameter "tail" in
>  burn_track_define_data()
> and to have libburn track padding enabled. (I want to add
> 300 kB of zeros to appease certain cdrom drivers.)

Working on that now.

> ---------------------------------------------------------------
> I still believe there should be a change of the data type for
> source size from  int  to  off_t  or  double .
> But i refrain from making such an intrusive change in libburn
> on my own.
>
> My cdrskin application uses type double and will be easily
> adapted to hand over to libburn any other type than int.
> (50 bit accuracy should be enough for a few years.)

I'm not a fan of double for this application.
How about uint64_t from stdint.h? (I think it's c99, is libburn ever 
going to run on a non-c99 platform?)