[PATCH] Alignment issues reported by clang.

[Michal Židek]

https://fedorahosted.org/sssd/ticket/1359

This patch mostly silences some alignment related warnings reported by 
clang, but also fixes some real alignment issues.

But not all warnings are suppressed in this patch. These cases still 
generate warnings:

1)=====================================================
src/util/refcount.c:63:25: warning: cast from 'char *' to 'int *' 
increases required alignment from 1 to 4 [-Wcast-align]
     wrapper->refcount = (int *)((char *)wrapper->ptr + refcount_offset);
                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/util/refcount.c:82:25: warning: cast from 'char *' to 'int *' 
increases required alignment from 1 to 4 [-Wcast-align]
     wrapper->refcount = (int *)((char *)wrapper->ptr + refcount_offset);
                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Comment: This is not a problem, the data we access here is an integer 
that is part of a structure, so there should be no alignment issues (C 
compiler would not create a structure with inaccessible structure 
member). I think suppressing this warning with some additional casts 
would only confuse us later and make the code more obfuscated than it is.

2)=====================================================
src/providers/ldap/sdap_async_sudo_hostinfo.c:233:24: warning: cast from 
'struct sockaddr *' to 'struct sockaddr_in *' increases
       required alignment from 2 to 4 [-Wcast-align]
             ip4_addr = (struct sockaddr_in*)(iface->ifa_addr);
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/providers/ldap/sdap_async_sudo_hostinfo.c:267:27: warning: cast from 
'struct sockaddr *' to 'struct sockaddr_in6 *' increases
       required alignment from 2 to 4 [-Wcast-align]
             ip6_network = (struct sockaddr_in6*)(iface->ifa_netmask);
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...and other sockaddr related casts...

Comment: We only have to make sure that sockaddr is not allocated on 
stack here. But we use the getifaddrs() function for this, so it should 
be OK.

3)=====================================================
src/sss_client/nss_services.c:137:29: warning: cast from 'char *' to 
'char **' increases required alignment from 1 to 8
       [-Wcast-align]
     sr->result->s_aliases = (char **) &(sr->buffer[i+pad]);
                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Comment: Well, there is nothing we can do about this warning. We just 
make sure that the accessed data is aligned to sizeof(char **) using the 
pad variable (in the original code, we aligned it to 32 bits, but that 
was incorrect).

4)=====================================================
src/sss_client/nss_mc_group.c:67:22: warning: cast from 'char *' to 
'char **' increases required alignment from 1 to 8 [-Wcast-align]
     result->gr_mem = (char **)buffer;
                      ^~~~~~~~~~~~~~~

src/monitor/monitor.c:1588:16: warning: cast from 'char *' to 'struct 
inotify_event *' increases required alignment from 1 to 4
       [-Wcast-align]
     in_event = (struct inotify_event *)buf;
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~

src/responder/nss/nsssrv_mmap_cache.c:281:22: warning: cast from 'char 
*' to 'rel_ptr_t *' (aka 'unsigned int *') increases required
       alignment from 1 to 4 [-Wcast-align]
         name_ptr = *((rel_ptr_t *)rec->data);
                      ^~~~~~~~~~~~~~~~~~~~~~

Comment: Here we access the beginnings of allocated memory spaces. So it 
should be OK too. Adding some additional casts just to suppress the 
warnings is here IMO inappropriate (it is then not so clear what data 
type we use).

5)======================================================
src/responder/common/responder_packet.c:79:21: warning: cast from 
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
       (aka 'unsigned int *') increases required alignment from 1 to 4 
[-Wcast-align]
     packet->len = &((uint32_t *)packet->buffer)[0];
                     ^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:80:21: warning: cast from 
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
       (aka 'unsigned int *') increases required alignment from 1 to 4 
[-Wcast-align]
     packet->cmd = &((uint32_t *)packet->buffer)[1];
                     ^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:81:24: warning: cast from 
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
       (aka 'unsigned int *') increases required alignment from 1 to 4 
[-Wcast-align]
     packet->status = &((uint32_t *)packet->buffer)[2];
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:82:26: warning: cast from 
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
       (aka 'unsigned int *') increases required alignment from 1 to 4 
[-Wcast-align]
     packet->reserved = &((uint32_t *)packet->buffer)[3];
                          ^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:83:33: warning: cast from 
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
       (aka 'unsigned int *') increases required alignment from 1 to 4 
[-Wcast-align]
     packet->body = (uint8_t *)&((uint32_t *)packet->buffer)[4];
                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:128:29: warning: cast from 
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
       (aka 'unsigned int *') increases required alignment from 1 to 4 
[-Wcast-align]
             packet->len = &((uint32_t *)packet->buffer)[0];
                             ^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:129:29: warning: cast from 
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
       (aka 'unsigned int *') increases required alignment from 1 to 4 
[-Wcast-align]
             packet->cmd = &((uint32_t *)packet->buffer)[1];
                             ^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:130:32: warning: cast from 
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
       (aka 'unsigned int *') increases required alignment from 1 to 4 
[-Wcast-align]
             packet->status = &((uint32_t *)packet->buffer)[2];
                                ^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:131:34: warning: cast from 
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
       (aka 'unsigned int *') increases required alignment from 1 to 4 
[-Wcast-align]
             packet->reserved = &((uint32_t *)packet->buffer)[3];
                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~
src/responder/common/responder_packet.c:132:41: warning: cast from 
'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
       (aka 'unsigned int *') increases required alignment from 1 to 4 
[-Wcast-align]
             packet->body = (uint8_t *)&((uint32_t *)packet->buffer)[4];
                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~

Comment: Same as number 4, but we also use addresses that are multiples 
of the sizeof(data_type_used) (relative to the beginning of the buffer). 
Which is OK.


================================================================
The rest of the warnings are suppressed by this patch.

I also have one question. Does talloc guarantee that the allocated block 
of memory (without the talloc related metadata) is aligned to the 
sizeof(biggest_primitive_C_data_type)? I know malloc does this, but I am 
not sure about talloc. I saw some places in the code, where we rely on 
this feature, so my guess is that talloc does it. Can someone confirm 
it? The above mentioned 3, 4 and 5 also rely on proper aligned memory block.

The patch is in attachment.

Thanks
Michal

[Simo Sorce]

On Thu, 2012-10-11 at 13:40 +0200, Michal Židek wrote:
>  https://fedorahosted.org/sssd/ticket/1359
>  
>  This patch mostly silences some alignment related warnings reported by 
>  clang, but also fixes some real alignment issues.
>  
>  But not all warnings are suppressed in this patch. These cases still 
>  generate warnings:
>  
>  1)=====================================================
>  src/util/refcount.c:63:25: warning: cast from 'char *' to 'int *' 
>  increases required alignment from 1 to 4 [-Wcast-align]
>       wrapper->refcount = (int *)((char *)wrapper->ptr + refcount_offset);
>                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  src/util/refcount.c:82:25: warning: cast from 'char *' to 'int *' 
>  increases required alignment from 1 to 4 [-Wcast-align]
>       wrapper->refcount = (int *)((char *)wrapper->ptr + refcount_offset);
>                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  
>  Comment: This is not a problem, the data we access here is an integer 
>  that is part of a structure, so there should be no alignment issues (C 
>  compiler would not create a structure with inaccessible structure 
>  member). I think suppressing this warning with some additional casts 
>  would only confuse us later and make the code more obfuscated than it is.
>  
>  2)=====================================================
>  src/providers/ldap/sdap_async_sudo_hostinfo.c:233:24: warning: cast from 
>  'struct sockaddr *' to 'struct sockaddr_in *' increases
>         required alignment from 2 to 4 [-Wcast-align]
>               ip4_addr = (struct sockaddr_in*)(iface->ifa_addr);
>                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  src/providers/ldap/sdap_async_sudo_hostinfo.c:267:27: warning: cast from 
>  'struct sockaddr *' to 'struct sockaddr_in6 *' increases
>         required alignment from 2 to 4 [-Wcast-align]
>               ip6_network = (struct sockaddr_in6*)(iface->ifa_netmask);
>                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  ...and other sockaddr related casts...
>  
>  Comment: We only have to make sure that sockaddr is not allocated on 
>  stack here. But we use the getifaddrs() function for this, so it should 
>  be OK.
>  
>  3)=====================================================
>  src/sss_client/nss_services.c:137:29: warning: cast from 'char *' to 
>  'char **' increases required alignment from 1 to 8
>         [-Wcast-align]
>       sr->result->s_aliases = (char **) &(sr->buffer[i+pad]);
>                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  Comment: Well, there is nothing we can do about this warning. We just 
>  make sure that the accessed data is aligned to sizeof(char **) using the 
>  pad variable (in the original code, we aligned it to 32 bits, but that 
>  was incorrect).
>  
>  4)=====================================================
>  src/sss_client/nss_mc_group.c:67:22: warning: cast from 'char *' to 
>  'char **' increases required alignment from 1 to 8 [-Wcast-align]
>       result->gr_mem = (char **)buffer;
>                        ^~~~~~~~~~~~~~~
>  
>  src/monitor/monitor.c:1588:16: warning: cast from 'char *' to 'struct 
>  inotify_event *' increases required alignment from 1 to 4
>         [-Wcast-align]
>       in_event = (struct inotify_event *)buf;
>                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>  
>  src/responder/nss/nsssrv_mmap_cache.c:281:22: warning: cast from 'char 
>  *' to 'rel_ptr_t *' (aka 'unsigned int *') increases required
>         alignment from 1 to 4 [-Wcast-align]
>           name_ptr = *((rel_ptr_t *)rec->data);
>                        ^~~~~~~~~~~~~~~~~~~~~~
>  
>  Comment: Here we access the beginnings of allocated memory spaces. So it 
>  should be OK too. Adding some additional casts just to suppress the 
>  warnings is here IMO inappropriate (it is then not so clear what data 
>  type we use).
>  
>  5)======================================================
>  src/responder/common/responder_packet.c:79:21: warning: cast from 
>  'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
>         (aka 'unsigned int *') increases required alignment from 1 to 4 
>  [-Wcast-align]
>       packet->len = &((uint32_t *)packet->buffer)[0];
>                       ^~~~~~~~~~~~~~~~~~~~~~~~~~
>  src/responder/common/responder_packet.c:80:21: warning: cast from 
>  'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
>         (aka 'unsigned int *') increases required alignment from 1 to 4 
>  [-Wcast-align]
>       packet->cmd = &((uint32_t *)packet->buffer)[1];
>                       ^~~~~~~~~~~~~~~~~~~~~~~~~~
>  src/responder/common/responder_packet.c:81:24: warning: cast from 
>  'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
>         (aka 'unsigned int *') increases required alignment from 1 to 4 
>  [-Wcast-align]
>       packet->status = &((uint32_t *)packet->buffer)[2];
>                          ^~~~~~~~~~~~~~~~~~~~~~~~~~
>  src/responder/common/responder_packet.c:82:26: warning: cast from 
>  'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
>         (aka 'unsigned int *') increases required alignment from 1 to 4 
>  [-Wcast-align]
>       packet->reserved = &((uint32_t *)packet->buffer)[3];
>                            ^~~~~~~~~~~~~~~~~~~~~~~~~~
>  src/responder/common/responder_packet.c:83:33: warning: cast from 
>  'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
>         (aka 'unsigned int *') increases required alignment from 1 to 4 
>  [-Wcast-align]
>       packet->body = (uint8_t *)&((uint32_t *)packet->buffer)[4];
>                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~
>  src/responder/common/responder_packet.c:128:29: warning: cast from 
>  'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
>         (aka 'unsigned int *') increases required alignment from 1 to 4 
>  [-Wcast-align]
>               packet->len = &((uint32_t *)packet->buffer)[0];
>                               ^~~~~~~~~~~~~~~~~~~~~~~~~~
>  src/responder/common/responder_packet.c:129:29: warning: cast from 
>  'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
>         (aka 'unsigned int *') increases required alignment from 1 to 4 
>  [-Wcast-align]
>               packet->cmd = &((uint32_t *)packet->buffer)[1];
>                               ^~~~~~~~~~~~~~~~~~~~~~~~~~
>  src/responder/common/responder_packet.c:130:32: warning: cast from 
>  'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
>         (aka 'unsigned int *') increases required alignment from 1 to 4 
>  [-Wcast-align]
>               packet->status = &((uint32_t *)packet->buffer)[2];
>                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~
>  src/responder/common/responder_packet.c:131:34: warning: cast from 
>  'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
>         (aka 'unsigned int *') increases required alignment from 1 to 4 
>  [-Wcast-align]
>               packet->reserved = &((uint32_t *)packet->buffer)[3];
>                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~
>  src/responder/common/responder_packet.c:132:41: warning: cast from 
>  'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
>         (aka 'unsigned int *') increases required alignment from 1 to 4 
>  [-Wcast-align]
>               packet->body = (uint8_t *)&((uint32_t *)packet->buffer)[4];
>                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~
>  
>  Comment: Same as number 4, but we also use addresses that are multiples 
>  of the sizeof(data_type_used) (relative to the beginning of the buffer). 
>  Which is OK.
>  
>  
>  ================================================================
>  The rest of the warnings are suppressed by this patch.
>  
>  I also have one question. Does talloc guarantee that the allocated block 
>  of memory (without the talloc related metadata) is aligned to the 
>  sizeof(biggest_primitive_C_data_type)? I know malloc does this, but I am 
>  not sure about talloc. I saw some places in the code, where we rely on 
>  this feature, so my guess is that talloc does it. Can someone confirm 
>  it? The above mentioned 3, 4 and 5 also rely on proper aligned memory block.
>  
>  The patch is in attachment.
>  
>  Thanks
>  Michal
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel 

Sorry Michael,
haven't really checked as carefully as I want but I think with this
patch you are changing the client protocol by changing one alignemnt.

You can't do that, changes to the client protocol are not allowed.

Can you confirm/deny ?

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

[Michal Židek]

On 10/11/2012 03:53 PM, Simo Sorce wrote:
>  On Thu, 2012-10-11 at 13:40 +0200, Michal Židek wrote:
> > https://fedorahosted.org/sssd/ticket/1359
> >
> > This patch mostly silences some alignment related warnings reported by
> > clang, but also fixes some real alignment issues.
> >
> > But not all warnings are suppressed in this patch. These cases still
> > generate warnings:
> >
> > 1)=====================================================
> > src/util/refcount.c:63:25: warning: cast from 'char *' to 'int *'
> > increases required alignment from 1 to 4 [-Wcast-align]
> >       wrapper->refcount = (int *)((char *)wrapper->ptr + refcount_offset);
> >                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > src/util/refcount.c:82:25: warning: cast from 'char *' to 'int *'
> > increases required alignment from 1 to 4 [-Wcast-align]
> >       wrapper->refcount = (int *)((char *)wrapper->ptr + refcount_offset);
> >                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Comment: This is not a problem, the data we access here is an integer
> > that is part of a structure, so there should be no alignment issues (C
> > compiler would not create a structure with inaccessible structure
> > member). I think suppressing this warning with some additional casts
> > would only confuse us later and make the code more obfuscated than it is.
> >
> > 2)=====================================================
> > src/providers/ldap/sdap_async_sudo_hostinfo.c:233:24: warning: cast from
> > 'struct sockaddr *' to 'struct sockaddr_in *' increases
> >         required alignment from 2 to 4 [-Wcast-align]
> >               ip4_addr = (struct sockaddr_in*)(iface->ifa_addr);
> >                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > src/providers/ldap/sdap_async_sudo_hostinfo.c:267:27: warning: cast from
> > 'struct sockaddr *' to 'struct sockaddr_in6 *' increases
> >         required alignment from 2 to 4 [-Wcast-align]
> >               ip6_network = (struct sockaddr_in6*)(iface->ifa_netmask);
> >                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > ...and other sockaddr related casts...
> >
> > Comment: We only have to make sure that sockaddr is not allocated on
> > stack here. But we use the getifaddrs() function for this, so it should
> > be OK.
> >
> > 3)=====================================================
> > src/sss_client/nss_services.c:137:29: warning: cast from 'char *' to
> > 'char **' increases required alignment from 1 to 8
> >         [-Wcast-align]
> >       sr->result->s_aliases = (char **) &(sr->buffer[i+pad]);
> >                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > Comment: Well, there is nothing we can do about this warning. We just
> > make sure that the accessed data is aligned to sizeof(char **) using the
> > pad variable (in the original code, we aligned it to 32 bits, but that
> > was incorrect).
> >
> > 4)=====================================================
> > src/sss_client/nss_mc_group.c:67:22: warning: cast from 'char *' to
> > 'char **' increases required alignment from 1 to 8 [-Wcast-align]
> >       result->gr_mem = (char **)buffer;
> >                        ^~~~~~~~~~~~~~~
> >
> > src/monitor/monitor.c:1588:16: warning: cast from 'char *' to 'struct
> > inotify_event *' increases required alignment from 1 to 4
> >         [-Wcast-align]
> >       in_event = (struct inotify_event *)buf;
> >                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > src/responder/nss/nsssrv_mmap_cache.c:281:22: warning: cast from 'char
> > *' to 'rel_ptr_t *' (aka 'unsigned int *') increases required
> >         alignment from 1 to 4 [-Wcast-align]
> >           name_ptr = *((rel_ptr_t *)rec->data);
> >                        ^~~~~~~~~~~~~~~~~~~~~~
> >
> > Comment: Here we access the beginnings of allocated memory spaces. So it
> > should be OK too. Adding some additional casts just to suppress the
> > warnings is here IMO inappropriate (it is then not so clear what data
> > type we use).
> >
> > 5)======================================================
> > src/responder/common/responder_packet.c:79:21: warning: cast from
> > 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
> >         (aka 'unsigned int *') increases required alignment from 1 to 4
> > [-Wcast-align]
> >       packet->len = &((uint32_t *)packet->buffer)[0];
> >                       ^~~~~~~~~~~~~~~~~~~~~~~~~~
> > src/responder/common/responder_packet.c:80:21: warning: cast from
> > 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
> >         (aka 'unsigned int *') increases required alignment from 1 to 4
> > [-Wcast-align]
> >       packet->cmd = &((uint32_t *)packet->buffer)[1];
> >                       ^~~~~~~~~~~~~~~~~~~~~~~~~~
> > src/responder/common/responder_packet.c:81:24: warning: cast from
> > 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
> >         (aka 'unsigned int *') increases required alignment from 1 to 4
> > [-Wcast-align]
> >       packet->status = &((uint32_t *)packet->buffer)[2];
> >                          ^~~~~~~~~~~~~~~~~~~~~~~~~~
> > src/responder/common/responder_packet.c:82:26: warning: cast from
> > 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
> >         (aka 'unsigned int *') increases required alignment from 1 to 4
> > [-Wcast-align]
> >       packet->reserved = &((uint32_t *)packet->buffer)[3];
> >                            ^~~~~~~~~~~~~~~~~~~~~~~~~~
> > src/responder/common/responder_packet.c:83:33: warning: cast from
> > 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
> >         (aka 'unsigned int *') increases required alignment from 1 to 4
> > [-Wcast-align]
> >       packet->body = (uint8_t *)&((uint32_t *)packet->buffer)[4];
> >                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~
> > src/responder/common/responder_packet.c:128:29: warning: cast from
> > 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
> >         (aka 'unsigned int *') increases required alignment from 1 to 4
> > [-Wcast-align]
> >               packet->len = &((uint32_t *)packet->buffer)[0];
> >                               ^~~~~~~~~~~~~~~~~~~~~~~~~~
> > src/responder/common/responder_packet.c:129:29: warning: cast from
> > 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
> >         (aka 'unsigned int *') increases required alignment from 1 to 4
> > [-Wcast-align]
> >               packet->cmd = &((uint32_t *)packet->buffer)[1];
> >                               ^~~~~~~~~~~~~~~~~~~~~~~~~~
> > src/responder/common/responder_packet.c:130:32: warning: cast from
> > 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
> >         (aka 'unsigned int *') increases required alignment from 1 to 4
> > [-Wcast-align]
> >               packet->status = &((uint32_t *)packet->buffer)[2];
> >                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~
> > src/responder/common/responder_packet.c:131:34: warning: cast from
> > 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
> >         (aka 'unsigned int *') increases required alignment from 1 to 4
> > [-Wcast-align]
> >               packet->reserved = &((uint32_t *)packet->buffer)[3];
> >                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~
> > src/responder/common/responder_packet.c:132:41: warning: cast from
> > 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
> >         (aka 'unsigned int *') increases required alignment from 1 to 4
> > [-Wcast-align]
> >               packet->body = (uint8_t *)&((uint32_t *)packet->buffer)[4];
> >                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Comment: Same as number 4, but we also use addresses that are multiples
> > of the sizeof(data_type_used) (relative to the beginning of the buffer).
> > Which is OK.
> >
> >
> > ================================================================
> > The rest of the warnings are suppressed by this patch.
> >
> > I also have one question. Does talloc guarantee that the allocated block
> > of memory (without the talloc related metadata) is aligned to the
> > sizeof(biggest_primitive_C_data_type)? I know malloc does this, but I am
> > not sure about talloc. I saw some places in the code, where we rely on
> > this feature, so my guess is that talloc does it. Can someone confirm
> > it? The above mentioned 3, 4 and 5 also rely on proper aligned memory block.
> >
> > The patch is in attachment.
> >
> > Thanks
> > Michal
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel(a)lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
>
>  Sorry Michael,
>  haven't really checked as carefully as I want but I think with this
>  patch you are changing the client protocol by changing one alignemnt.
>
>  You can't do that, changes to the client protocol are not allowed.
>
>  Can you confirm/deny ?
>
>  Simo.
>  
Hello Simo,

do you mean this change?

-    /* Make sure pr->buffer[i+pad] is 32 bit aligned */
+    /* Make sure pr->buffer[i+pad] is aligned to sizeof (char **) */
      pad = 0;
-    while((i + pad) % 4) {
+    while((i + pad) % sizeof(char **)) {
          pad++;
      }

I have a mistake here, it should have been sizeof(char *) not 
sizeof(char**), because we store pointers to char on that location. I 
know that this will probably be the same number, but it is still a 
mistake (I fixed it in the new patch).

But to your question. I think this does not change the client protocol, 
the function only reads the reply (the reply is in the client protocol 
format), but stores data into another buffer. As far as I understand the 
code, this new buffer has nothing to do with the client protocol (it is 
connected with the reply via those mentioned char pointers, but nothing 
more), but maybe I am wrong, it is not easy to read piece of code.

Or do you mean a different part of patch affects the client protocol?

Thanks
Michal

[Simo Sorce]

On Thu, 2012-10-11 at 20:44 +0200, Michal Židek wrote:
>  > Sorry Michael,
>  > haven't really checked as carefully as I want but I think with this
>  > patch you are changing the client protocol by changing one
>  alignemnt.
>  >
>  > You can't do that, changes to the client protocol are not allowed.
>  >
>  > Can you confirm/deny ?
>  >
>  > Simo.
>  >
>  
>  Hello Simo,
>  
>  do you mean this change?
>  
>  -    /* Make sure pr->buffer[i+pad] is 32 bit aligned */
>  +    /* Make sure pr->buffer[i+pad] is aligned to sizeof (char **) */
>        pad = 0;
>  -    while((i + pad) % 4) {
>  +    while((i + pad) % sizeof(char **)) {
>            pad++;
>        }
>  
>  I have a mistake here, it should have been sizeof(char *) not 
>  sizeof(char**), because we store pointers to char on that location. I 
>  know that this will probably be the same number, but it is still a 
>  mistake (I fixed it in the new patch).
>  
>  But to your question. I think this does not change the client
>  protocol, 
>  the function only reads the reply (the reply is in the client
>  protocol 
>  format), but stores data into another buffer. As far as I understand
>  the 
>  code, this new buffer has nothing to do with the client protocol (it
>  is 
>  connected with the reply via those mentioned char pointers, but
>  nothing 
>  more), but maybe I am wrong, it is not easy to read piece of code.
>  
>  Or do you mean a different part of patch affects the client protocol?
>   
This is the change I meant, and you are right this affects the internal
buffer returned to glibc not the actual protocol.

I guess maybe we should rename some of those variable names to make it
clearer what buffer are we referring to.

Thanks for double checking with me, this code is sometimes tricky and
alignment issues make it trickier.

All the other alignment fixes look good to me, but the patch is quite
bit, I wouldn't mind if a second pair of eyes could check and give a
second ack.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

[Jakub Hrozek]

On Thu, Oct 11, 2012 at 03:34:03PM -0400, Simo Sorce wrote:
>  On Thu, 2012-10-11 at 20:44 +0200, Michal Židek wrote:
>  > > Sorry Michael,
>  > > haven't really checked as carefully as I want but I think with this
>  > > patch you are changing the client protocol by changing one
>  > alignemnt.
>  > >
>  > > You can't do that, changes to the client protocol are not allowed.
>  > >
>  > > Can you confirm/deny ?
>  > >
>  > > Simo.
>  > >
>  > 
>  > Hello Simo,
>  > 
>  > do you mean this change?
>  > 
>  > -    /* Make sure pr->buffer[i+pad] is 32 bit aligned */
>  > +    /* Make sure pr->buffer[i+pad] is aligned to sizeof (char **) */
>  >       pad = 0;
>  > -    while((i + pad) % 4) {
>  > +    while((i + pad) % sizeof(char **)) {
>  >           pad++;
>  >       }
>  > 
>  > I have a mistake here, it should have been sizeof(char *) not 
>  > sizeof(char**), because we store pointers to char on that location. I 
>  > know that this will probably be the same number, but it is still a 
>  > mistake (I fixed it in the new patch).
>  > 
>  > But to your question. I think this does not change the client
>  > protocol, 
>  > the function only reads the reply (the reply is in the client
>  > protocol 
>  > format), but stores data into another buffer. As far as I understand
>  > the 
>  > code, this new buffer has nothing to do with the client protocol (it
>  > is 
>  > connected with the reply via those mentioned char pointers, but
>  > nothing 
>  > more), but maybe I am wrong, it is not easy to read piece of code.
>  > 
>  > Or do you mean a different part of patch affects the client protocol?
>  > 
>  This is the change I meant, and you are right this affects the internal
>  buffer returned to glibc not the actual protocol.
>  
>  I guess maybe we should rename some of those variable names to make it
>  clearer what buffer are we referring to.
>  
>  Thanks for double checking with me, this code is sometimes tricky and
>  alignment issues make it trickier.
>  
>  All the other alignment fixes look good to me, but the patch is quite
>  bit, I wouldn't mind if a second pair of eyes could check and give a
>  second ack.
>  
>  Simo. 
Thanks for checking the client part Simo.

I will review the code tomorrow or Monday. I moved the ticket to a later
release because I think it touches quite a few parts of the SSSD so it's
easy to overlook something. Also the patch is mostly useful to secondary
architectures.

[Jakub Hrozek]

On Thu, Oct 11, 2012 at 11:07:55PM +0200, Jakub Hrozek wrote:
>  On Thu, Oct 11, 2012 at 03:34:03PM -0400, Simo Sorce wrote:
>  > On Thu, 2012-10-11 at 20:44 +0200, Michal Židek wrote:
>  > > > Sorry Michael,
>  > > > haven't really checked as carefully as I want but I think with this
>  > > > patch you are changing the client protocol by changing one
>  > > alignemnt.
>  > > >
>  > > > You can't do that, changes to the client protocol are not allowed.
>  > > >
>  > > > Can you confirm/deny ?
>  > > >
>  > > > Simo.
>  > > >
>  > > 
>  > > Hello Simo,
>  > > 
>  > > do you mean this change?
>  > > 
>  > > -    /* Make sure pr->buffer[i+pad] is 32 bit aligned */
>  > > +    /* Make sure pr->buffer[i+pad] is aligned to sizeof (char **) */
>  > >       pad = 0;
>  > > -    while((i + pad) % 4) {
>  > > +    while((i + pad) % sizeof(char **)) {
>  > >           pad++;
>  > >       }
>  > > 
>  > > I have a mistake here, it should have been sizeof(char *) not 
>  > > sizeof(char**), because we store pointers to char on that location. I 
>  > > know that this will probably be the same number, but it is still a 
>  > > mistake (I fixed it in the new patch).
>  > > 
>  > > But to your question. I think this does not change the client
>  > > protocol, 
>  > > the function only reads the reply (the reply is in the client
>  > > protocol 
>  > > format), but stores data into another buffer. As far as I understand
>  > > the 
>  > > code, this new buffer has nothing to do with the client protocol (it
>  > > is 
>  > > connected with the reply via those mentioned char pointers, but
>  > > nothing 
>  > > more), but maybe I am wrong, it is not easy to read piece of code.
>  > > 
>  > > Or do you mean a different part of patch affects the client protocol?
>  > > 
>  > This is the change I meant, and you are right this affects the internal
>  > buffer returned to glibc not the actual protocol.
>  > 
>  > I guess maybe we should rename some of those variable names to make it
>  > clearer what buffer are we referring to.
>  > 
>  > Thanks for double checking with me, this code is sometimes tricky and
>  > alignment issues make it trickier.
>  > 
>  > All the other alignment fixes look good to me, but the patch is quite
>  > bit, I wouldn't mind if a second pair of eyes could check and give a
>  > second ack.
>  > 
>  > Simo.
>  
>  Thanks for checking the client part Simo.
>  
>  I will review the code tomorrow or Monday. I moved the ticket to a later
>  release because I think it touches quite a few parts of the SSSD so it's
>  easy to overlook something. Also the patch is mostly useful to secondary
>  architectures. 
I'm sorry this patch got forgotten. Can you rebase it to the current
master?

[Michal Židek]

On 03/20/2013 01:53 PM, Jakub Hrozek wrote:
>  On Thu, Oct 11, 2012 at 11:07:55PM +0200, Jakub Hrozek wrote:
> > On Thu, Oct 11, 2012 at 03:34:03PM -0400, Simo Sorce wrote:
> >> On Thu, 2012-10-11 at 20:44 +0200, Michal Židek wrote:
> >>>> Sorry Michael,
> >>>> haven't really checked as carefully as I want but I think with this
> >>>> patch you are changing the client protocol by changing one
> >>> alignemnt.
> >>>>
> >>>> You can't do that, changes to the client protocol are not allowed.
> >>>>
> >>>> Can you confirm/deny ?
> >>>>
> >>>> Simo.
> >>>>
> >>>
> >>> Hello Simo,
> >>>
> >>> do you mean this change?
> >>>
> >>> -    /* Make sure pr->buffer[i+pad] is 32 bit aligned */
> >>> +    /* Make sure pr->buffer[i+pad] is aligned to sizeof (char **) */
> >>>        pad = 0;
> >>> -    while((i + pad) % 4) {
> >>> +    while((i + pad) % sizeof(char **)) {
> >>>            pad++;
> >>>        }
> >>>
> >>> I have a mistake here, it should have been sizeof(char *) not
> >>> sizeof(char**), because we store pointers to char on that location. I
> >>> know that this will probably be the same number, but it is still a
> >>> mistake (I fixed it in the new patch).
> >>>
> >>> But to your question. I think this does not change the client
> >>> protocol,
> >>> the function only reads the reply (the reply is in the client
> >>> protocol
> >>> format), but stores data into another buffer. As far as I understand
> >>> the
> >>> code, this new buffer has nothing to do with the client protocol (it
> >>> is
> >>> connected with the reply via those mentioned char pointers, but
> >>> nothing
> >>> more), but maybe I am wrong, it is not easy to read piece of code.
> >>>
> >>> Or do you mean a different part of patch affects the client protocol?
> >>>
> >> This is the change I meant, and you are right this affects the internal
> >> buffer returned to glibc not the actual protocol.
> >>
> >> I guess maybe we should rename some of those variable names to make it
> >> clearer what buffer are we referring to.
> >>
> >> Thanks for double checking with me, this code is sometimes tricky and
> >> alignment issues make it trickier.
> >>
> >> All the other alignment fixes look good to me, but the patch is quite
> >> bit, I wouldn't mind if a second pair of eyes could check and give a
> >> second ack.
> >>
> >> Simo.
> >
> > Thanks for checking the client part Simo.
> >
> > I will review the code tomorrow or Monday. I moved the ticket to a later
> > release because I think it touches quite a few parts of the SSSD so it's
> > easy to overlook something. Also the patch is mostly useful to secondary
> > architectures.
>
>  I'm sorry this patch got forgotten. Can you rebase it to the current
>  master? 
I think it is better to do the https://fedorahosted.org
/sssd/ticket/1772 first, and then rebase this patch with the
new macros used. I'll do it after I fix ticket #1741
(sss_cache doesn't support subdomains).

Michal

[Jakub Hrozek]

On Wed, Mar 20, 2013 at 02:08:29PM +0100, Michal Židek wrote:
>  On 03/20/2013 01:53 PM, Jakub Hrozek wrote:
>  >On Thu, Oct 11, 2012 at 11:07:55PM +0200, Jakub Hrozek wrote:
>  >>On Thu, Oct 11, 2012 at 03:34:03PM -0400, Simo Sorce wrote:
>  >>>On Thu, 2012-10-11 at 20:44 +0200, Michal Židek wrote:
>  >>>>>Sorry Michael,
>  >>>>>haven't really checked as carefully as I want but I think with
> this
>  >>>>>patch you are changing the client protocol by changing one
>  >>>>alignemnt.
>  >>>>>
>  >>>>>You can't do that, changes to the client protocol are not
> allowed.
>  >>>>>
>  >>>>>Can you confirm/deny ?
>  >>>>>
>  >>>>>Simo.
>  >>>>>
>  >>>>
>  >>>>Hello Simo,
>  >>>>
>  >>>>do you mean this change?
>  >>>>
>  >>>>-    /* Make sure pr->buffer[i+pad] is 32 bit aligned */
>  >>>>+    /* Make sure pr->buffer[i+pad] is aligned to sizeof (char **) */
>  >>>>       pad = 0;
>  >>>>-    while((i + pad) % 4) {
>  >>>>+    while((i + pad) % sizeof(char **)) {
>  >>>>           pad++;
>  >>>>       }
>  >>>>
>  >>>>I have a mistake here, it should have been sizeof(char *) not
>  >>>>sizeof(char**), because we store pointers to char on that location. I
>  >>>>know that this will probably be the same number, but it is still a
>  >>>>mistake (I fixed it in the new patch).
>  >>>>
>  >>>>But to your question. I think this does not change the client
>  >>>>protocol,
>  >>>>the function only reads the reply (the reply is in the client
>  >>>>protocol
>  >>>>format), but stores data into another buffer. As far as I understand
>  >>>>the
>  >>>>code, this new buffer has nothing to do with the client protocol (it
>  >>>>is
>  >>>>connected with the reply via those mentioned char pointers, but
>  >>>>nothing
>  >>>>more), but maybe I am wrong, it is not easy to read piece of code.
>  >>>>
>  >>>>Or do you mean a different part of patch affects the client protocol?
>  >>>>
>  >>>This is the change I meant, and you are right this affects the internal
>  >>>buffer returned to glibc not the actual protocol.
>  >>>
>  >>>I guess maybe we should rename some of those variable names to make it
>  >>>clearer what buffer are we referring to.
>  >>>
>  >>>Thanks for double checking with me, this code is sometimes tricky and
>  >>>alignment issues make it trickier.
>  >>>
>  >>>All the other alignment fixes look good to me, but the patch is quite
>  >>>bit, I wouldn't mind if a second pair of eyes could check and give a
>  >>>second ack.
>  >>>
>  >>>Simo.
>  >>
>  >>Thanks for checking the client part Simo.
>  >>
>  >>I will review the code tomorrow or Monday. I moved the ticket to a later
>  >>release because I think it touches quite a few parts of the SSSD so it's
>  >>easy to overlook something. Also the patch is mostly useful to secondary
>  >>architectures.
>  >
>  >I'm sorry this patch got forgotten. Can you rebase it to the current
>  >master?
>  
>  I think it is better to do the https://fedorahosted.org
>  /sssd/ticket/1772 first, and then rebase this patch with the
>  new macros used. 
Good idea.

>  I'll do it after I fix ticket #1741
>  (sss_cache doesn't support subdomains). 
Yes, that one definitely takes precedence, sure. I'm just trying to
clean up the list :)