Bug#715461: Re: Bug#715461: libsdl-mixer1.2: no sf2 sound fonts loaded by default

2013-08-08 Thread Dominique Dumont
On Wednesday 07 August 2013 22:13:49 you wrote:
 For example, one fix that comes to mind is to change the line in the
 first patch:
 
 char* soundfont_paths =
 /usr/share/sounds/sf2/TimGM6mb.sf2:/usr/share/sounds/sf2/FluidR3_GM.sf2;
 
 to this:
 
 char* soundfont_paths =
 SDL_strdup(/usr/share/sounds/sf2/TimGM6mb.sf2:/usr/share/sounds/sf2/FluidR3
 _GM.sf2);
 
 What do you think?  Feels less intrusive than having a second patch.

ok to reduce the number of patches.

But the SDL_strdup solution is needlessly complicated and will probably have 
some eyebrows raised very high in the future. 

I'd rather see bug-718129-rm-bad-free.patch merged into bug-715461-
soundfont_paths.patch so as to have one simple, correct patch.

All the best



signature.asc
Description: This is a digitally signed message part.


Bug#715461: Re: Bug#715461: libsdl-mixer1.2: no sf2 sound fonts loaded by default

2013-08-08 Thread Manuel A. Fernandez Montecelo
2013/8/8 Dominique Dumont d...@debian.org:
 On Wednesday 07 August 2013 22:13:49 you wrote:
 For example, one fix that comes to mind is to change the line in the
 first patch:

 char* soundfont_paths =
 /usr/share/sounds/sf2/TimGM6mb.sf2:/usr/share/sounds/sf2/FluidR3_GM.sf2;

 to this:

 char* soundfont_paths =
 SDL_strdup(/usr/share/sounds/sf2/TimGM6mb.sf2:/usr/share/sounds/sf2/FluidR3
 _GM.sf2);

 What do you think?  Feels less intrusive than having a second patch.

 ok to reduce the number of patches.

 But the SDL_strdup solution is needlessly complicated and will probably have
 some eyebrows raised very high in the future.

 I'd rather see bug-718129-rm-bad-free.patch merged into bug-715461-
 soundfont_paths.patch so as to have one simple, correct patch.

I don't know if my intentions were clear.

I meant to modify the first patch bug-715461-soundfont_paths.patch so
when that variable soundfont_paths is assigned, it's done with
SDL_strdup() (it's done in several places in the code --that's where I
got the idea from--, so it fits), and remove the second patch
altogether, bug-718129-rm-bad-free.patch.

I think that this fits the simple, correct patch idea that you
mention, and I don't see anything complicated about it -- it's just to
assign the variable with dynamic memory, which is the way the rest of
the code thinks that it should be (there are more instances trying to
free memory from this varaible).

The variable can be set by users of the library to use dynamic memory
[1], so removing that SDL_free() is theoretically incorrect -- if it
gets assigned other content in runtime, it would not free it where the
SDL_free() is removed (which is the end of the program, so actually it
shoudn't be that important, bug e.g. valgrind would report it as a
leak).

Still, if anybody thinks that other solutions are preferrable, it's OK
by me -- I have no special interest in pushing this solution over
others.  I volunteer to fix this, no matter the solution chosen, if
nobody else wants.

And Dominique, sorry that I didn't catch this when you asked me, I was
busy at work and couldn't pay full attention to the issue.


Cheers.

[1] music.c, int Mix_SetSoundFonts(const char *paths)

-- 
Manuel A. Fernandez Montecelo manuel.montez...@gmail.com


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org