[PATCH 08/11] xfsprogs: replace obsolete memalign with posix_memalign

[Jan Tulak]

On Wed, Aug 19, 2015 at 12:01 AM, Dave Chinner <david at fromorbit.com> wrote:

> On Tue, Aug 18, 2015 at 10:33:49AM +0200, Jan Tulak wrote:
> > On Tue, Aug 18, 2015 at 10:20 AM, Dave Chinner <david at fromorbit.com>
> wrote:
> >
> > > On Tue, Aug 18, 2015 at 09:04:24AM +0200, Jan Tulak wrote:
> > > > I thought about it. However, with memalign from malloc marked
> obsolete
> > > > (and with posix_memalign having guaranteed alignment restrictions
> [1]), I
> > > > saw it better
> > > > to use the posix variant everywhere.
> > >
> > > Putting a sane wrapper around an nasty library function is just
> > > fine. The memalign wrapper makes sense from this perspective - even
> > > gcc can't tell if variables passed to posix_memalign are correctly
> > > initialised or not, whereas no such problems exist for memalign().
> > >
> > > > I could make a wrapper simulating the old memalign behaviour, but I
> don't
> > > > think it would make sense.
> > >
> > > I think it makes more sense than using posix_memalign() everywhere
> > > and then ignoring the return variable that tells you it failed...
> > >
> > > > I searched for this, but didn't find any reasonable answer:
> > > > How long can be things in standard libraries marked obsolete before
> > > > removing?
> > >
> > > With a wrapper, we don't care.
> > >
> > > > [1] man memalign:
> > > >        On many systems there are alignment restrictions, for
> example, on
> > > buf-
> > > >        fers  used  for  direct  block  device  I/O.  POSIX specifies
> the
> > > path-
> > > >        conf(path,_PC_REC_XFER_ALIGN) call that tells what alignment
> is
> > > needed.
> > > >        Now one can use posix_memalign() to satisfy this requirement.
> > > >
> > > >        posix_memalign()  verifies  that  alignment  matches  the
> > > requirements
> > > >        detailed above.  memalign() may not check that the  alignment
> > > argument
> > > >        is correct.
> > >
> > > Yes, you can get it wrong with memalign. But we don't, because we
> > > follow the rules for DIO buffer alignment and set it correctly.
> > > Being able to directly control the alignment of the memory buffer is
> > > a reason for using memalign() over posix_memalign(), not the other
> > > way around.
> > >
> >
> > So a wrapper used on all platforms is an acceptable solution? All right,
> > this explanation makes sense. I will change it that way. The only
> question
> > I have now is whether to use posix_memalign on every platform, or
> whether to
> > make it platform_memalign and use the old memalign inside for Linux.
>
> No need for a wrapper on platforms that support memalign. We can add
> a wrapper when and if memalign ever goes away (which, FWIW, will
> break lots of code). Indeed, we alreadyhave these platform dependent
> "wrappers":
>
> include/darwin.h:#define memalign(a,sz) valloc(sz)
> include/freebsd.h:#define memalign(a,sz)        valloc(sz)
>
> The question now is - do we even need to change anything?
>
>
> https://developer.apple.com/library/ios/documentation/System/Conceptual/ManPages_iPhoneOS/man3/valloc.3.html
>
> "The valloc() function allocates size bytes of memory and returns a
>  pointer to the allocated memory.  The allocated memory is
>  aligned on a page boundary"
>
> Which means it does pretty exactly the same thing as
> posix_memalign(), and so we don't need to change anything, right?
>
>
Mmm, I'm sure I had some issue with valloc - that was why I decided to
replace it.
But now, I can't remember what exactly it was and everything seems to work
the same
with or without it. :-/
So maybe the real cause of the issue was something else, fixed in another
patch,
making this one abundant...

OK, I'm moving it out of the patchset. And I'm thinking about some private
issue
tracker where I can make notes, reference commits and such. :D

Cheers,
Jan

-- 
Jan Tulak
jtulak at redhat.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://oss.sgi.com/pipermail/xfs/attachments/20150819/41d19d9d/attachment.html>