On 09/20/2013 05:10 PM, Dmitri Pal wrote:
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.
Thanks for the responses.
I decided to reduce the patches for this thread to only similar or
simple changes. I think it is still quite enough for a review. I will
post patches for the rest of the issues (like the one with sockaddr) in
separate threads. I hope this will make review easier. So after applying
these patches there will still be a lot of warnings.
New patches attached.
PS: Sorry for delayed answer.