[Libburn] Looking at the libburn code
manmower at signalmarketing.com
Tue Feb 14 20:15:12 PST 2006
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
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.
More information about the libburn