On 09/19/2013 08:28 PM, Simo Sorce wrote:
On Thu, 2013-09-19 at 18:22 +0200, Lukas Slebodnik wrote:
> On (19/09/13 09:00), Simo Sorce wrote:
>> On Thu, 2013-09-19 at 09:17 +0200, Sumit Bose wrote:
>>> On Wed, Sep 18, 2013 at 07:07:46PM +0200, Lukas Slebodnik wrote:
>>>> On (12/09/13 16:55), Michal Židek wrote:
>>>>> On 09/12/2013 02:03 PM, Lukas Slebodnik wrote:
>>>>>> On (11/09/13 17:05), Michal Židek wrote:
>>>>>>> Patches 1-4 and 9:
>>>>>>> These patches use the SAFEALIGN macros where it is
appropriate. I
>>>>>>> split them into several patches for easier review, so that
client
>>>>>>> code changes are not mixed with the rest of the code and
changes that
>>>>>>> are a little different than the rest have their own patch.
>>>>>>>
>>>>>>> Patches 5-6:
>>>>>>> Here I think it is not needed to use uint8_t* or char*. We
cast the
>>>>>>> pointer anyway and there are no real alignment issues (the
memory is
>>>>>>> aligned properly). I was thinking about suppressing the
warnings with
>>>>>>> #pragma here, but in this particular situation it is IMO
better to
>>>>>>> have just void*.
>>>>>>>
>>>>>>> Patches 7-8:
>>>>>>> These were all false positive warnings. It is possible to
suppress
>>>>>>> them with additional casting but I found it less readable so
I used
>>>>>>> #pragma.
>>>>>>>
>>>>>>> Patch 10:
>>>>>>> We had improperly aligned part of a buffer used in client
code. Also
>>>>>>> some code blocks here must rely on fact that the buffer they
get is
>>>>>>> aligned properly (they can do nothing about it if it
isn't). In these
>>>>>>> cases it is better to silence the warnings, so that it does
not make
>>>>>>> permanent noise during compilation.
>>>>>>>
>>>>>>> Thanks
>>>>>>> Michal
>>>>>> You forgot to fix two warnings in tests.
>>>>>>
>>>>>> CC src/tests/cmocka/dyndns_tests-test_dyndns.o
>>>>>> src/tests/cmocka/test_dyndns.c:130:10: warning: cast from
'struct sockaddr *' to
>>>>>> 'struct sockaddr_in *' increases required alignment
from 2 to 4 [-Wcast-align]
>>>>>> ((struct sockaddr_in *) ifap->ifa_addr)->sin_family
= AF_INET;
>>>>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>> src/tests/cmocka/test_dyndns.c:134:26: warning: cast from
'struct sockaddr *' to
>>>>>> 'struct sockaddr_in *' increases required alignment
from 2 to 4 [-Wcast-align]
>>>>>> &(((struct sockaddr_in *)
ifap->ifa_addr)->sin_addr)) != 1) {
>>>>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>> 2 warnings generated.
>>>>>>
>>>>>> LS
>>>>> Thanks, new patches are attached.
>>>>>
>>>>> Michal
>>>>>
>>>> I tested some compilers and patches work fine with clang and gcc.
>>>> But there are also another compilers in the world :-)
>>>> ant they needn't understand "pragma GCC". This pragma is
not standardized.
>>>>
>>>>
>>>> src/providers/dp_dyndns.c(145): warning #161: unrecognized #pragma
>>>> #pragma GCC diagnostic push
>>>> ^
>>>>
>>>> src/providers/dp_dyndns.c(146): warning #161: unrecognized #pragma
>>>> #pragma GCC diagnostic ignored "-Wcast-align"
>>>> ^
>>>>
>>>> src/providers/dp_dyndns.c(206): warning #161: unrecognized #pragma
>>>> #pragma GCC diagnostic pop
>>>> ^
>>>>
>>>> If you are 100% sure about alignment, you can define own macro.
>>>>
>>>> Something like this:
>>>> #define ignore_align(var, dest_type) \
>>>> (dest_type *)((void*)(var))
>>>>
>>>> And usage:
>>>> wrapper->refcount = ignore_align(wrapper->ptr +
refcount_offset, int);
>>>>
>>>>
>>>> Does anybody have any better ideas?
>>> Compiler specific #pragma should not be used. And I think in general
>>> #pragma should be used with great care. Currently there is only one use
>>> case in sssd in the memcache code to align some structs which make
>>> sense.
>> Indeed, agree 100%
>>
>>> Especially I think ingnoring a warning is not a good idea. If currently
>>> no one has a good idea how to handle the sockaddr and in_addr structs in
>>> a way which does not produce a warning I would prefer to keep the
>>> warning around until someone has a good idea. Imo a comment in the code
>>> that the warning can be ignored would be sufficient for the time being.
>> casting sockaddr is a bug, unless you know for sure it was created as
>> sockaddr_un and then simply casted to sockaddr, but in that case I would
>> consider the casting to pass it around just a different bug.
>>
>> Please use sockaddr_un anywhere you need to perform casts. Pass around
>> sockaddr_un and cast it only at the time of use when needed.
> It is not possible to use sockaddr_un(or sockaddr_storage).
>
> We use function getifaddrs to retrieve addresses of network interfaces.
>
> Manual page says (man 3 getifaddrs)
> --------------------------------------------------------------------------
> int getifaddrs(struct ifaddrs **ifap);
>
> The getifaddrs() function creates a linked list of structures
> describing the network interfaces of the local system, and stores
> the address of the first item of the list in *ifap.
> The list consists of ifaddrs structures, defined as follows:
>
> struct ifaddrs {
> struct ifaddrs *ifa_next; /* Next item in list */
> char *ifa_name; /* Name of interface */
> unsigned int ifa_flags; /* Flags from SIOCGIFFLAGS */
> struct sockaddr *ifa_addr; /* Address of interface */
> ^^^^^^^^^^^^^^^
> We cannot change this type
> struct sockaddr *ifa_netmask; /* Netmask of interface */
> union {
> struct sockaddr *ifu_broadaddr;
> /* Broadcast address of interface */
> struct sockaddr *ifu_dstaddr;
> /* Point-to-point destination address */
> } ifa_ifu;
> #define ifa_broadaddr ifa_ifu.ifu_broadaddr
> #define ifa_dstaddr ifa_ifu.ifu_dstaddr
> void *ifa_data; /* Address-specific data */
> };
>
> --------------------------------------------------------------------------
In this case you should copy the contents of sockaddr to a new variable
of the right type, and not cast.
Simo.
Yes, this is the only way you can deal with these issues.
I remembered this from my experiments with ELAPI 3 years ago.
I ran into similar problems with this function and had to copy data.
--
Thank you,
Dmitri Pal
Sr. Engineering Manager for IdM portfolio
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/