A few patches for minor defects
I put the libdvdnav code in MythTV through a static analysis tool and it found some defects. I ported the patches I submitted there to the fork on mplayerhq. I offer them here to you in hopes you find them useful. E -- Erik Hovland mail: erik@hovland.org web: http://hovland.org/ PGP/GPG public key available on request
Il Saturday 19 April 2008 00:03:39 Erik Hovland ha scritto:
I put the libdvdnav code in MythTV through a static analysis tool and it found some defects. I ported the patches I submitted there to the fork on mplayerhq. I offer them here to you in hopes you find them useful.
E
almost all of them perfect and applied, with one exception: the dlclose() after a successful dlopen() seems to be the wrong thing to do, if my interpretation of the man page is correct. I'd like to have a clarification on this behalf. There's only one patch left to review; maybe today or tomorrow. In the meantime thanks a lot !
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Nico Sabbi wrote:
Il Saturday 19 April 2008 00:03:39 Erik Hovland ha scritto:
I put the libdvdnav code in MythTV through a static analysis tool and it found some defects. I ported the patches I submitted there to the fork on mplayerhq. I offer them here to you in hopes you find them useful.
E
almost all of them perfect and applied, with one exception: the dlclose() after a successful dlopen() seems to be the wrong thing to do, if my interpretation of the man page is correct. I'd like to have a clarification on this behalf. There's only one patch left to review; maybe today or tomorrow.
In the meantime thanks a lot !
Although I'm not a DVDnav programmer (only a very intensive user), I agree on dlclose: dlclose should only be called if the functions loaded from it are no more needed. It is not that opening a dynamic library with dlopen loads the function somewhere in the memory and a handle is kept to the file (well, this is done, too). calling dlclose tells the OS that functions from this module will no longer be used, and also calls the exit functions defined for the library (_fini), sometimes also the module-specifc atexit functions. This stuff is my daily bread-and-butter, so I don't think I'm off here a lot. - -- Cheers, Alex -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.4-svn0 (GNU/Linux) Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org iD8DBQFICk4r1J0MF2u394wRAg9EAKCAzb+VXLb+vsGu9ywgW7UgTNRKpACfUzth 6+65utG/t2RWiNBbmewumiQ= =486J -----END PGP SIGNATURE-----
On Sat, Apr 19, 2008 at 09:42:02PM +0200, Nico Sabbi wrote:
Il Saturday 19 April 2008 00:03:39 Erik Hovland ha scritto:
I put the libdvdnav code in MythTV through a static analysis tool and it found some defects. I ported the patches I submitted there to the fork on mplayerhq. I offer them here to you in hopes you find them useful.
E
almost all of them perfect and applied, with one exception: the dlclose() after a successful dlopen() seems to be the wrong thing to do, if my interpretation
Right. This is something that I don't think I have resolved for myself. I can see how the man page could be interpreted at least two different ways.
of the man page is correct. I'd like to have a clarification on this behalf.
Yes, Agreed. And since it is just a little file leak at best I don't see any problem with not accepting that patch.
There's only one patch left to review; maybe today or tomorrow.
In the meantime thanks a lot !
Your welcome. And really it is my way of saying thanks for stewarding dvdnav. E -- Erik Hovland mail: erik@hovland.org web: http://hovland.org/ PGP/GPG public key available on request
Il Saturday 19 April 2008 22:09:06 Erik Hovland ha scritto:
Yes, Agreed. And since it is just a little file leak at best I don't see any problem with not accepting that patch.
There's only one patch left to review; maybe today or tomorrow.
In the meantime thanks a lot !
Your welcome. And really it is my way of saying thanks for stewarding dvdnav.
E
even the last patch was applied, but in a less obfuscated version and with the necessary fix in dvdnav_get_current_menu(). If you want a write account send an email to root@mplayerhq.hu specifying that it's for the dvdnav repository alone. Rathann, go on with the release if you want
Il Sunday 20 April 2008 10:27:17 Nico Sabbi ha scritto:
If you want a write account send an email to root@mplayerhq.hu specifying that it's for the dvdnav repository alone.
same speech for Diego Pettenò and Andreas Oman (if he doesn't already have it)
Nico Sabbi wrote:
Il Sunday 20 April 2008 10:27:17 Nico Sabbi ha scritto:
If you want a write account send an email to root@mplayerhq.hu specifying that it's for the dvdnav repository alone.
same speech for Diego Pettenò and Andreas Oman (if he doesn't already have it)
So, I've made a split of dvdnav/dvdread for my mediaplayer project. Main reason for not contributing back was that i really don't want to mess around with the auto-stuff. http://svn.lonelycoder.com/viewvc/trunk/libs/ I guess the same could be done here. Should it also include libdvdcss? (or is that code still too 'shady'?) May i also suggest that we lose some weight from the top directory? I.e, remove these directories: dvd-web/ kio-dvdnav/ libdvdnav2/ www/ xine-dvdnav/ xine-howto/ What about the auto-stuff? Can it not be dropped? Maintaining two build infrastructures is too much work IMHO.
On Monday 21 April 2008 09:24:44 Andreas Öman wrote:
Nico Sabbi wrote:
Il Sunday 20 April 2008 10:27:17 Nico Sabbi ha scritto:
If you want a write account send an email to root@mplayerhq.hu specifying that it's for the dvdnav repository alone.
same speech for Diego Pettenò and Andreas Oman (if he doesn't already have it)
So, I've made a split of dvdnav/dvdread for my mediaplayer project. Main reason for not contributing back was that i really don't want to mess around with the auto-stuff.
how to disagree? :)
good, I'll give it a look
I guess the same could be done here. Should it also include libdvdcss? (or is that code still too 'shady'?)
better leave it alone
May i also suggest that we lose some weight from the top directory? I.e, remove these directories: dvd-web/
I have to look at it
kio-dvdnav/ libdvdnav2/
yes and yes
www/
no, there are the descriptions of the IFO structures
xine-dvdnav/ xine-howto/
definitely yes
What about the auto-stuff? Can it not be dropped? Maintaining two build infrastructures is too much work IMHO.
there are still too many platforms where configure2 doesn't work well (e.g. Solaris) . Better keep the auto* stuff until configure2 is perfect
Nico Sabbi wrote:
On Monday 21 April 2008 09:24:44 Andreas Öman wrote:
What about the auto-stuff? Can it not be dropped? Maintaining two build infrastructures is too much work IMHO.
there are still too many platforms where configure2 doesn't work well (e.g. Solaris) . Better keep the auto* stuff until configure2 is perfect
Hm, anything in particular on solaris that does not work? I tried to configure and build on these platforms and it seems to work fine for me: SunOS ridley 5.10 Generic_118822-11 sun4u sparc SUNW,Sun-Fire-V210 SunOS tott 5.8 Generic_108528-15 sun4u sparc SUNW,Ultra-250 The latter is ancient, i know :-) But i just wanted to test it...
On Monday 21 April 2008 09:57:49 Andreas Öman wrote:
Nico Sabbi wrote:
On Monday 21 April 2008 09:24:44 Andreas Öman wrote:
What about the auto-stuff? Can it not be dropped? Maintaining two build infrastructures is too much work IMHO.
there are still too many platforms where configure2 doesn't work well (e.g. Solaris) . Better keep the auto* stuff until configure2 is perfect
Hm, anything in particular on solaris that does not work?
I don't remember exactly what because I gave it just a very quick look. I seem to remember that at least install didn't work like ginstall is used to work, but there was something else too
I tried to configure and build on these platforms and it seems to work fine for me:
SunOS ridley 5.10 Generic_118822-11 sun4u sparc SUNW,Sun-Fire-V210 SunOS tott 5.8 Generic_108528-15 sun4u sparc SUNW,Ultra-250
The latter is ancient, i know :-) But i just wanted to test it...
are you using a Nevada build? In Nexenta all is fine (due to the massive replacement of Sun's tools with GNU counterparts). I saw that you slimmed down a bit too much your dvdnav import: no dvdnav-config, no configure2 and many more... :)
participants (4)
-
Alexander Roalter -
Andreas Öman -
Erik Hovland -
Nico Sabbi