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). Simo. -- Simo Sorce * Red Hat, Inc * New York _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
