On Fri, Dec 06, 2013 at 11:58:38AM -0500, Simo Sorce wrote:
On Fri, 2013-12-06 at 17:55 +0100, Michal Židek wrote:
> On 12/05/2013 12:22 PM, Michal Židek wrote:
> > On 12/03/2013 05:04 PM, Simo Sorce wrote:
> >> On Tue, 2013-12-03 at 16:03 +0100, Michal Židek wrote:
> >>> On 12/03/2013 02:34 PM, Simo Sorce wrote:
> >>>> On Tue, 2013-12-03 at 13:28 +0100, Michal Židek wrote:
> >>>>> On 12/03/2013 10:57 AM, Lukas Slebodnik wrote:
> >>>>
> >>>>>> I thik we can change while loop with som expresion.
> >>>>>> pad = i % sizeof(char *)
> >>>>>> pad = (pad != 0) ? sizeof(char *) - pad : 0;
> >>>>>>
> >>>>>
> >>>>> Ok, I added this change to the patch. I did not want to change
the
> >>>>> code
> >>>>> much with these patches, the purpose was to just fix the
alignment
> >>>>> issues. But this is very small change and additional patch
would just
> >>>>> change the same lines. So I think it is ok to include.
> >>>>>
> >>>>>> or something nicer :-) (maybe macro?)
> >>>>>
> >>>>> I do not think that obfuscation with a macro is appropriate
here.
> >>>>
> >>>> Actually the code is pretty hard to read and a macro would avoid
> >>>> duplication and make it clear what the purpose is:
> >>>>
> >>>> Something like:
> >>>>
> >>>> #define PTR_SIZE sizeof(char *)
> >>>> #define ALIGN_PTR_PAD(base, padding) \
> >>>> padding = (PTR_SIZE - (base % PTR_SIZE)) % PTR_SIZE)
> >>>>
> >>>> Then in the code just call:
> >>>>
> >>>> ALIGN_PTR_PAD(i, pad)
> >>>
> >>> I tend to dislike macros, but maybe we will need similar pattern on
> >>> other places in the future as well, so putting this into macro may not
> >>> be as bad idea as I originally though.
> >>>
> >>> However I would propose a different macro, that would be used like
this:
> >>> pad = CALCULATE_PAD(i, char *);
> >>
> >> Maybe PADDING_SIZE() then.
> >>
> >> Simo.
> >>
> >>
> >
> > Patch with changed macro name attached.
> >
> > Michal
> >
>
> I am sending new patch that will also suppress the alignment warning in
> sss_nss_mc_parse_result.
>
> It introduces two simple macros IS_ALIGNED to check if address is
> aligned to sizeof(some_type) and DISCARD_ALIGN that will internally
> cast the pointer to void* to suppress the warning.
>
> If there is no bug, the buffer in this function should be properly
> aligned, however if the alignment is incorect, the function will fail
> with EFAULT. I originally wanted to do this check only for architectures
> that have problems with accessing unaligned memory, but then decided to
> do it for all platforms. The reason is, that we mostly test on x86 and
> we want to identify these issues as soon as possible if they occur.
>
> New patch is attached.
Thank you Michal,
it looks good!
Ack from me (but I haven't actually tested the code).
I ran a couple of sanity tests and haven't seen anything broken. I'm
going to push.