[PATCH] Initialize dvd_reader_t struct in DVDOpenImageFile
Hi, This is a patch I found in MythTV's copy of libdvdread that seems like it could/should be upstream too. It ensures that the dvd_reader_t struct is initialized (e.g. the css_state field is currently undefined if 'have_css' is false). The patch is part of a bigger MythTV-specific change by Chris Pinkham: https://github.com/MythTV/mythtv/commit/3af3489 Richard Hulme
Hello Richard, On Sunday, 26 January 2014 at 17:13, Richard Hulme wrote:
Hi,
This is a patch I found in MythTV's copy of libdvdread that seems like it could/should be upstream too. It ensures that the dvd_reader_t struct is initialized (e.g. the css_state field is currently undefined if 'have_css' is false).
Thanks for forwarding the patch. If I understand correctly, with that memset in place, some of the subsequent assignments dvd->path_root = NULL; [...] dvd->udfcache = NULL; [...] dvd->css_title = 0; are not necessary anymore. Actually, that case of leaving css_state uninitialized is a genuine bug, but I'm not sure if using memset is the right solution. Does anyone else have thoughts on this case? Regards, Dominik -- Fedora http://fedoraproject.org/wiki/User:Rathann RPMFusion http://rpmfusion.org | MPlayer http://mplayerhq.hu "Faith manages." -- Delenn to Lennier in Babylon 5:"Confessions and Lamentations"
On 01/02/14 15:21, Dominik 'Rathann' Mierzejewski wrote:
Hello Richard,
On Sunday, 26 January 2014 at 17:13, Richard Hulme wrote:
Hi,
This is a patch I found in MythTV's copy of libdvdread that seems like it could/should be upstream too. It ensures that the dvd_reader_t struct is initialized (e.g. the css_state field is currently undefined if 'have_css' is false).
Thanks for forwarding the patch.
If I understand correctly, with that memset in place, some of the subsequent assignments dvd->path_root = NULL; [...] dvd->udfcache = NULL; [...] dvd->css_title = 0;
are not necessary anymore. Actually, that case of leaving css_state uninitialized is a genuine bug, but I'm not sure if using memset is the right solution. Does anyone else have thoughts on this case?
Hi Dominik, If I had written that patch myself, I think I might have just gone with explicitly initialising have_css to zero, just to fit in with the rest of the existing code and to keep the changes to a minimum. The two other possibilities I see are either staying with memset or using calloc instead of malloc/memset. In either case the explicit initialisations to zero/NULL are not necessary. To my eyes, calloc or memset have the advantage of ensuring the entire structure is initialised, but there is only have_css which is not currently being initialised and the structure is unlikely to grow, so that argument in favour of calloc or memset is not as strong as it could be. Also, not all fields should be zero (but only 3 from 7). In the end, I think it comes down to personal preference, but I'm happy to adapt the patch as necessary. Richard.
Le 02/02/2014 12:00, Richard Hulme a écrit :
In the end, I think it comes down to personal preference, but I'm happy to adapt the patch as necessary.
Not that it matters much, but I prefer the calloc approach. It is less confusing for newcomers on the code. -- Jean-Baptiste Kempf http://www.jbkempf.com/ - +33 672 704 734 Sent from my Electronic Device
Le 26/01/2014 17:13, Richard Hulme a écrit :
The patch is part of a bigger MythTV-specific change by Chris Pinkham:
Any way of getting part of this upstream? -- Jean-Baptiste Kempf http://www.jbkempf.com/ - +33 672 704 734 Sent from my Electronic Device
On 18/02/14 16:42, Jean-Baptiste Kempf wrote:
Le 26/01/2014 17:13, Richard Hulme a écrit :
The patch is part of a bigger MythTV-specific change by Chris Pinkham:
Any way of getting part of this upstream?
Which bit in particular? That particular patch primarily adds support for using MythTV's remote file-handling. The one generic bit was the patch I originally attached to the first email of this thread (the call to memset). Richard.
On 18 Feb, Richard Hulme wrote :
On 18/02/14 16:42, Jean-Baptiste Kempf wrote:
Le 26/01/2014 17:13, Richard Hulme a écrit :
The patch is part of a bigger MythTV-specific change by Chris Pinkham:
Any way of getting part of this upstream?
Which bit in particular? That particular patch primarily adds support for using MythTV's remote file-handling. The one generic bit was the patch I originally attached to the first email of this thread (the call to memset).
With more and more things on the "cloud", it would make sense to have a way to pass a pointer to a open() and read() function to dvdread, instead of using the filesystem ones, so that people could host their isos over http. With my kindest regards, -- Jean-Baptiste Kempf http://www.jbkempf.com/ - +33 672 704 734 Sent from my Electronic Device
On Tuesday, 18 February 2014 at 20:18, Jean-Baptiste Kempf wrote:
On 18 Feb, Richard Hulme wrote :
On 18/02/14 16:42, Jean-Baptiste Kempf wrote:
Le 26/01/2014 17:13, Richard Hulme a écrit :
The patch is part of a bigger MythTV-specific change by Chris Pinkham:
Any way of getting part of this upstream?
Which bit in particular? That particular patch primarily adds support for using MythTV's remote file-handling. The one generic bit was the patch I originally attached to the first email of this thread (the call to memset).
With more and more things on the "cloud", it would make sense to have a way to pass a pointer to a open() and read() function to dvdread, instead of using the filesystem ones, so that people could host their isos over http.
I remember a patch that added transparent rar decompression support, but it was never accepted. Maybe it's time to revisit that in a more general way. I'll bump some old threads with unreviewed/unmerged patches in the next few days, so that you or someone else can look at them with a fresh set of eyes. Regards, Dominik -- Fedora http://fedoraproject.org/wiki/User:Rathann RPMFusion http://rpmfusion.org | MPlayer http://mplayerhq.hu "Faith manages." -- Delenn to Lennier in Babylon 5:"Confessions and Lamentations"
Le 22/02/2014 01:05, Dominik 'Rathann' Mierzejewski a écrit :
On Tuesday, 18 February 2014 at 20:18, Jean-Baptiste Kempf wrote:
On 18 Feb, Richard Hulme wrote :
On 18/02/14 16:42, Jean-Baptiste Kempf wrote:
Le 26/01/2014 17:13, Richard Hulme a écrit :
The patch is part of a bigger MythTV-specific change by Chris Pinkham:
Any way of getting part of this upstream?
Which bit in particular? That particular patch primarily adds support for using MythTV's remote file-handling. The one generic bit was the patch I originally attached to the first email of this thread (the call to memset).
With more and more things on the "cloud", it would make sense to have a way to pass a pointer to a open() and read() function to dvdread, instead of using the filesystem ones, so that people could host their isos over http.
I remember a patch that added transparent rar decompression support, but it was never accepted. Maybe it's time to revisit that in a more general way.
I'll bump some old threads with unreviewed/unmerged patches in the next few days, so that you or someone else can look at them with a fresh set of eyes.
That would be a good idea :) -- Jean-Baptiste Kempf http://www.jbkempf.com/ - +33 672 704 734 Sent from my Electronic Device
Le 26/01/2014 17:13, Richard Hulme a écrit :
Hi,
This is a patch I found in MythTV's copy of libdvdread that seems like it could/should be upstream too. It ensures that the dvd_reader_t struct is initialized (e.g. the css_state field is currently undefined if 'have_css' is false).
Applied. -- Jean-Baptiste Kempf http://www.jbkempf.com/ - +33 672 704 734 Sent from my Electronic Device
participants (3)
-
Dominik 'Rathann' Mierzejewski -
Jean-Baptiste Kempf -
Richard Hulme