[Libburn] Looking at the libburn code

[Derek Foreman]

On Wed, 15 Feb 2006, Philip Martin wrote:

> I've just had a look at some of the libburn code for the first time
> and there are a few issues that I see:
>
> - The pthread_mutex_xxx functions all have return values but these are
>  ignored by the libburn code.  These functions are unlikely to fail,
>  but assuming they won't fail is a bit of a gamble.

I think pthread_mutex_lock/unlock will only fail if used incorrectly, 
therefor their returns are only interesting for assert()ions

I also assumed that pthread_mutex_init can't fail, but I guess on some 
platforms it does.  I haven't touched this yet.

> - Calls to write(2) and close(2) don't have their return value
>  checked, these calls could fail.

Yep, bugs.  (might be a while before I get to these)

> - The pthread_create return value is checked but the libburn code
>  doesn't do anything sensible -- the calling function returns no
>  error but the linked list refers to free'd memory.  Again, it's
>  unlikely to happen, but it's not impossible.

Yep, bug, I think it's fixed in CVS.

> - A pthread_mutex_t is copied when enumerate_common calls
>  burn_drive_register in sg.c.  POSIX classifies that as undefined
>  behaviour, although it works on lots of systems.

I'd not realized this was undefined, but it makes sense that it would be.
I think this one's fixed in CVS as well.

> - There is no call to pthread_mutex_destroy, that could be a problem
>  on some platforms.
>
> - There is a realloc call in structure.c that doesn't make use the
>  return value.  I assume it's reducing the memory allocation and so
>  the memory is unlikely to be moved, but it's still not portable.

That one's even stupider than you think.  The & is totally bogus.

I think I've fixed it in cvs.

> - Some of the files have unnecessary includes, e.g. sbc.c, others
>  don't have includes for things they use, e.g. transport.h doesn't
>  include pthread.h and relies on sys/types.h pulling in something
>  suitable.

I've added pthread.h to transport.h, and thrown a few more forward 
declarations in various include files, and removed some extraneous 
includes from sbc.c.

I'd not turn away a patch to fix these things, but I'm too lazy to go 
looking if gcc on linux isn't whining at me.

> - Is burn_wait_all supposed to sleep?  The current implementation
>  would appear to skip the sleep.

Well, the situation that was intended to cause a sleep fires an assert 
anyway.  The whole function needs a re-think.

> I regard all of the above as bugs, although some of them will rarely
> if ever affect Linux.  Is the project interested in fixing them?

All bug fixes welcome.

Thanks