On 09/24/2013 10:38 PM, Michal Židek wrote:
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.
Sending patches rebased on top of current master.
Michal