[FFmpeg-devel] [PATCH] [RFC] libavutil/mem.c: Check return value of posix_memalign

[Patrik Kullman]

On Fri, 2009-02-13 at 21:35 +0100, Michael Niedermayer wrote:
> On Fri, Feb 13, 2009 at 07:37:16PM +0000, M?ns Rullg?rd wrote:
> > Patrik Kullman <patrik at yes.nu> writes:
> > 
> > > On Fri, 2009-02-13 at 18:11 +0000, M?ns Rullg?rd wrote:
> > >> Patrik Kullman <patrik at yes.nu> writes:
> > >> 
> > >> > On Fri, 2009-02-13 at 16:26 +0000, M?ns Rullg?rd wrote:
> > >> >> Reimar D?ffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de> writes:
> > >> >> 
> > >> >> > On Fri, Feb 13, 2009 at 03:47:23PM +0000, M?ns Rullg?rd wrote:
> > >> >> >> > libavutil/mem.c: In function ?av_malloc?:
> > >> >> >> > libavutil/mem.c:66: warning: ignoring return value of ?posix_memalign?,
> > >> >> >> > declared with attribute warn_unused_result
> > >> >> >> 
> > >> >> >> That warning is bogus.  If posix_memalign() fails, memalign() will too
> > >> >> >> since they are likely to be the same function.  We set the pointer to
> > >> >> >> NULL before the call, so if it fails, we return NULL as we're supposed
> > >> >> >> to.
> > >> >> >
> > >> >> > Well, if you are pedantic you here assume that posix_memalign will not
> > >> >> > modify ptr if it fails (or at least not set it != NULL), but that
> > >> >> > actually does not seem to be guaranteed by POSIX (it doesn't say
> > >> >> > anything about the value of ptr when an error is returned...).
> > >> >> 
> > >> >> That is indeed the case, and thus your suggestion of explicitly
> > >> >> setting it to NULL makes sense.
> > >> >
> > >> > So, this is OK?
> > >> >
> > >> > Index: libavutil/mem.c
> > >> > ===================================================================
> > >> > --- libavutil/mem.c	(revision 17211)
> > >> > +++ libavutil/mem.c	(working copy)
> > >> > @@ -63,7 +63,8 @@
> > >> >      ptr = (char*)ptr + diff;
> > >> >      ((char*)ptr)[-1]= diff;
> > >> >  #elif HAVE_POSIX_MEMALIGN
> > >> > -    posix_memalign(&ptr,16,size);
> > >> > +    if (posix_memalign(&ptr,16,size) != 0)
> > >> 
> > >> Drop the != 0.
> > >> 
> > >> > +        ptr = NULL;
> > >> >  #elif HAVE_MEMALIGN
> > >> >      ptr = memalign(16,size);
> > >> >      /* Why 64?
> > >> 
> > >> Otherwise no objections from me.  Maybe we should drop the initial
> > >> NULL assignment since with this change, it is not necessary.
> > >
> > > Ok, dropped the != 0.
> > >
> > >
> > > Index: libavutil/mem.c
> > > ===================================================================
> > > --- libavutil/mem.c	(revision 17211)
> > > +++ libavutil/mem.c	(working copy)
> > > @@ -63,7 +63,8 @@
> > >      ptr = (char*)ptr + diff;
> > >      ((char*)ptr)[-1]= diff;
> > >  #elif HAVE_POSIX_MEMALIGN
> > > -    posix_memalign(&ptr,16,size);
> > > +    if (posix_memalign(&ptr,16,size))
> > > +        ptr = NULL;
> > >  #elif HAVE_MEMALIGN
> > >      ptr = memalign(16,size);
> > >      /* Why 64?
> > 
> > Now we're just waiting for the maintainer to give his approval (or not).
> 
> iam fine with it though i must say ive never been a friend of posix_memalign()
> memalign() itself works fine and doesnt need such obscure checks
> 

Is this an OK from the maintainer?