Derek: Patch proposal after your recent decisions

On Wed, 15 Feb 2006, scdbackupgmx.net wrote:

> Hi,
>
> attached is my implementation proposal concerning
> your recent decisions about off_t, burn_source_file
> and burn_source_fd.
> It has been tested via  test/burniso  for regular file
> input as well as for piped stdin.
>
>
> Notes:
>
> -  #include <sys/types.h>  is back in libburn.h
>  I removed <stdint.h> because i believe you
introduced it only
>  for the sake of the now dismissed int64_t.
>  If this is an error, i apologize. 

Not an error.  Thanks.

> - The new class burn_source_fd is implemented in the
source files
>  files.[ch]. That is because i feel unable to bring new
files
>  into the binary build system of libburn.
>  Please split  file.[ch]  according to your style
preferences.

Due to their similarity I'll probably leave them in the
same file.

> - Both read methods of burn_source_file and of
burn_source_fd
>  are now based on a generic wrapper function around
read(2):
>   read_full_buffer()
>  Since this is neither a method of burn_source_file nor
of
>  burn_source_fd it should probably be in a more neutral
>  source file.
>  Please move it as appropriate.

I think I'll make it static and keep it there, I don't
forsee it being 
more useful in a general sense.  It can be moved later if it
is.

> - off_t is currently rather 32 bit on conventional
systems.
>  I feel unable to introduce external macro settings
into the
>  binary build system of libburn. There seems to be no
libburn
>  include file which is really included by all libburn
sources.
>
>  It is not urgent to augment off_t to 64 bit unless you
want
>  to introduce it quickly for clarity reasons. The code
>  proposal will work for CD sizes even with 32 bit
off_t.
>  (My statement that one would need 64 bit to recognize
files
>   >= 4GB was wrong. 32 bit fstat() returns -1 in
such a case.
>   This suffices for now.)

Libburn can't currently handle more than 99 minutes in a
track, since 
that's the CD limit.  That makes big files rather
uninteresting at this 
point.  This issue will lie dormant as a comment in a file
somewhere until 
the real need arises.

>  Please remember to eventually announce the need for
64-bit
>  macros in the API documentation at an exposed place.

<nod>

> - The get_size() methods for now restrict their return
value to
>  2 GB minus 800 MB.
>  This shall ensure that no return value is issued which
leads
>  to an integer rollover after any reasonable CD size
computation
>  is applied to it. (2GB-1 might roll over if you add
150 sectors)
>  I assume that 1.2 GB is larger than any valid CD image
and that
>  no CD size computation adds more than 800 MB.
>  As soon as libburn is checked for off_t safety, this
restriction
>  might get lifted. For now it seems better that way.
>
>  I considered to set an assert() on the file size. But
if libburn
>  uses abort() as a regular means of communication with
the calling
>  application then it would need to offer appropriate
signal handling.
>  (I could contribute some.)

Don't you dare trap SIGABRT in the front end! 

If an assert is thrown, it's a mission critical bug and
someone needs to 
fix some code.  (Or the hardware acted in a really flakey
way and we need 
to know how to work around it.  heh.)

It might be nice to have some kind of softer assert() that
still reports 
line number and failed expression, but also makes sure the
drive returns 
to a sane state.

>  Whatever, the new get_size() implementation is in any
case better
>  than the old one. I advise you to accept it for now,
even if you
>  think that it is not optimal. (I think so myself, but
will first
>  have to scan for the real int-off_t problems.)

All this code is in CVS now, but I hope to see a simpler
get_size as soon 
as we add the right #defines to make off_t play 64 bit all
the time.

Thanks


Is the whitelist your only out of tree patch now?
_______________________________________________
libburn mailing list
libburnlists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libburn

Hi,

> >  It is not urgent to augment off_t to 64 bit
unless you want
> >  to introduce it quickly for clarity reasons.
> 
> Libburn can't currently handle more than 99 minutes in
a track, since 
> that's the CD limit.  That makes big files rather
uninteresting at this 
> point.  This issue will lie dormant as a comment in a
file somewhere until 
> the real need arises.

I am nearly finished with the overview.
We got not very many problem spots with bytecount int.

The number of spots with sectorcount int is much larger
but those will only make trouble with 4 TB media.
(To be expected in about 15 to 20 years.)

Some functions are too obscure for my little brain.
You will get a list of them and have to check yourself
wether there are fat bytecounts involved.


> Don't you dare trap SIGABRT in the front end! 

Currently i have to. Temporarily, of course, and only
until the drive gets into state BURN_DRIVE_IDLE.
Afterwards cdrskin delivers a nice non-zero exit.


> If an assert is thrown, it's a mission critical bug
and someone needs to 
> fix some code.  (Or the hardware acted in a really
flakey way and we need 
> to know how to work around it.  heh.)

No assert event will pass unnoticed by the user of cdrskin.
The abort procedure is loud. Dozens of messages saying
essentially
  cdrskin: ABORT : <It is done when it is done. Be
patient.>
and finally
  cdrskin: ABORT : Program done. Even if you do not see a
shell prompt.


> It might be nice to have some kind of softer assert()
that still reports 
> line number and failed expression, but also makes sure
the drive returns 
> to a sane state.

Yes. It would be nice if libburn could take care of
not leaving the drive endlessly waiting.
Proper signal handling with threads is still a riddle
to me. For the purpose of cdrskin i found solutions
but there are still scenarios for which i can hardly
imagine to get happy with threads.


The problem with ill sized files will hopefully
be eased by the int-to-off_t transition. But as long as we
use 32 bit off_t there will always be files which return
-1 on fstat() due to >4 GB of size.
These files are unusable with libburn and deserve some kind
of abort. I will probably make cdrskin test input files
for successful lstat() before i allow them to hit libburn.

All in all, i believe a fake 0-size is better than an
abort in the  get_size()  function.


> All this code is in CVS now, but I hope to see a
simpler get_size as soon 
> as we add the right #defines to make off_t play 64 bit
all the time.

With 32 bit we would be able to raise the size ban toi
4 GB minus 800 MB. You might also decide that your code will
not need a computation reserve of 800 MB and raise it even
a bit more.
But essentially this waits for the 64 bit move.


> Is the whitelist your only out of tree patch now?

Among the submitted ideas, it is the last one remaining.

The main obstacle currently seems to be the decision
about the future meaning of 
  burn_drive_info.location
and wether we need a
  burn_drive_info.drive_id
in order to achieve a unique and sufficiently persistent
drive address.
(This does not only affect the wish to isolate from
 unfriendly drives but also cdrskin's method of addressing
 a drive via a long time persistent preconfigured address.)

The actual way of implementation is a less important
issue and might be easily changed after a while
if the first choice turns out to be suboptimal.

My favorite implementation would still be a whitelist
directly settable by the application and no new
burn_drive_scan_single() function. None of my other
ideas on that topic is that straight and versatile.
I just love it. {


As soon as cdrskin can make use of the CVS version i will
check about half a dozen workarounds wether they are still
needed. I will report to you.
Next i will annoy you with a list of minor flaws and 
enhancement wishes which i hold back until the important
changes are done. )

Then there are two big wishes which i cannot fulfill myself.
Not even as a provisory patch:

- a write mode that demands no size prediction.
  (Like cdrecord -data -tao )

- a function which is equivalent to cdrecord -abort and
  makes drives usable again which fell victim to an
  incompletely handled libburn abort. cdrskin does its
  best to prevent this, but it still can happen.


All other of my wishes are currently assigned to Santa
Claus.


Have a nice day 

Thomas

_______________________________________________
libburn mailing list
libburnlists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libburn