On Tue, Oct 22, 2013 at 03:07:31PM +0200, Lukas Slebodnik wrote:
On (22/10/13 14:09), Michal Židek wrote:
>On 10/21/2013 04:16 PM, Lukas Slebodnik wrote:
>>On (24/09/13 22:54), Michal Židek wrote:
>>>Hello,
>>>
>>>the attached patch should uses the SAFEALIGN macros to access data in
>>>the packet header (in sss_packet structure) so that it does not
>>>depend on proper memory alignment (currently the alignment is OK, but
>>>if we add something to the packet header later, it is probably safer
>>>not to depend on proper alignment). It also silences a lot of
>>
>>>alignment warnings.
>>>
>>>Thanks
>>>Michal
>>
>>There are not warnings from static analysers.
>>I tested patches with valgrind. (works fine)
>>Alignment warnings are gone.
>>
>>Few nitpicks inline
>>
>>>From f8f124d85a23983f5229ff19ff19cd0c058cd7da Mon Sep 17 00:00:00 2001
>>>From: Michal Zidek <mzidek(a)redhat.com>
>>>Date: Fri, 30 Aug 2013 18:05:35 +0200
>>>Subject: [PATCH] responder: Access packet header using SAFEALIGN macros.
>>>
>>>resolves:
>>>https://fedorahosted.org/sssd/ticket/1359
>>>---
>>>src/responder/common/responder_packet.c | 96
++++++++++++++++++++-------------
>>>1 file changed, 59 insertions(+), 37 deletions(-)
>>>
>>>diff --git a/src/responder/common/responder_packet.c
b/src/responder/common/responder_packet.c
>>>index 6476bd6..e0fbe54 100644
>>>--- a/src/responder/common/responder_packet.c
>>>+++ b/src/responder/common/responder_packet.c
>>>@@ -31,20 +31,26 @@
>>>
>>>struct sss_packet {
>>> size_t memsize;
>>>- uint8_t *buffer;
>>>-
>>>- /* header */
>>>- uint32_t *len;
>>>- uint32_t *cmd;
>>>- uint32_t *status;
>>>- uint32_t *reserved;
>>>
>>>- uint8_t *body;
>>>+ /* Structure of the buffer:
>>>+ * Bytes Content
>>>+ * ---------------------------------
>>>+ * 0-15 packet header
>>>+ * 0-3 packet length (uint32_t)
>>>+ * 4-7 command type (uint32_t)
>>>+ * 8-11 status (uint32_t)
>>>+ * 12-15 reserved
>>>+ * 16+ packet body */
>>>+ uint8_t *buffer;
>>>
>>> /* io pointer */
>>> size_t iop;
>>>};
>>>
>>>+static void packet_set_len(struct sss_packet *packet, uint32_t len);
>>>+static void packet_set_cmd(struct sss_packet *packet, enum sss_cli_command
cmd);
>>>+static uint32_t packet_get_len(struct sss_packet *packet);
>>>+
>>>/*
>>> * Allocate a new packet structure
>>> *
>>>@@ -74,14 +80,8 @@ int sss_packet_new(TALLOC_CTX *mem_ctx, size_t size,
>>> }
>>> memset(packet->buffer, 0, SSS_NSS_HEADER_SIZE);
>>>
>>>- packet->len = &((uint32_t *)packet->buffer)[0];
>>>- packet->cmd = &((uint32_t *)packet->buffer)[1];
>>>- packet->status = &((uint32_t *)packet->buffer)[2];
>>>- packet->reserved = &((uint32_t *)packet->buffer)[3];
>>>- packet->body = (uint8_t *)&((uint32_t *)packet->buffer)[4];
>>>-
>>>- *(packet->len) = size + SSS_NSS_HEADER_SIZE;
>>>- *(packet->cmd) = cmd;
>>>+ packet_set_len(packet, size + SSS_NSS_HEADER_SIZE);
>>>+ packet_set_cmd(packet, cmd);
>>>
>>> packet->iop = 0;
>>>
>>>@@ -95,13 +95,16 @@ int sss_packet_grow(struct sss_packet *packet, size_t
size)
>>>{
>>> size_t totlen, len;
>>> uint8_t *newmem;
>>>+ uint32_t packet_len;
>>>
>>> if (size == 0) {
>>> return EOK;
>>> }
>>>
>>> totlen = packet->memsize;
>>>- len = *packet->len + size;
>>>+ packet_len = packet_get_len(packet);
>>>+
>>>+ len = packet_len + size;
>>>
>>> /* make sure we do not overflow */
>>> if (totlen < len) {
>>>@@ -123,15 +126,12 @@ int sss_packet_grow(struct sss_packet *packet, size_t
size)
>>> /* re-set pointers if realloc had to move memory */
>>> if (newmem != packet->buffer) {
>>> packet->buffer = newmem;
>>>- packet->len = &((uint32_t *)packet->buffer)[0];
>>>- packet->cmd = &((uint32_t *)packet->buffer)[1];
>>>- packet->status = &((uint32_t *)packet->buffer)[2];
>>>- packet->reserved = &((uint32_t *)packet->buffer)[3];
>>>- packet->body = (uint8_t *)&((uint32_t
*)packet->buffer)[4];
>>> }
>>> }
>>>
>>>- *(packet->len) += size;
>>>+ packet_len += size;
>>>+ packet_set_len(packet, packet_len);
>>>+
>>>
>>> return 0;
>>>}
>>>@@ -141,13 +141,14 @@ int sss_packet_grow(struct sss_packet *packet, size_t
size)
>>>int sss_packet_shrink(struct sss_packet *packet, size_t size)
>>>{
>>> size_t newlen;
>>>+ size_t oldlen = packet_get_len(packet);
>>>
>>>- if (size > *(packet->len)) return EINVAL;
>>>+ if (size > oldlen) return EINVAL;
>>>
>>>- newlen = *(packet->len) - size;
>>>+ newlen = oldlen - size;
>>> if (newlen < SSS_NSS_HEADER_SIZE) return EINVAL;
>>>
>>>- *(packet->len) = newlen;
>>>+ packet_set_len(packet, newlen);
>>> return 0;
>>>}
>>>
>>>@@ -160,7 +161,7 @@ int sss_packet_set_size(struct sss_packet *packet, size_t
size)
>>> /* make sure we do not overflow */
>>> if (packet->memsize < newlen) return EINVAL;
>>>
>>>- *(packet->len) = newlen;
>>>+ packet_set_len(packet, newlen);
>>>
>>> return 0;
>>>}
>>>@@ -171,8 +172,8 @@ int sss_packet_recv(struct sss_packet *packet, int fd)
>>> size_t len;
>>> void *buf;
>>>
>>>- buf = packet->buffer + packet->iop;
>>>- if (packet->iop > 4) len = *packet->len - packet->iop;
>>>+ buf = (uint8_t *)packet->buffer + packet->iop;
>>>+ if (packet->iop > 4) len = packet_get_len(packet) -
packet->iop;
>>> else len = packet->memsize - packet->iop;
>>>
>>> /* check for wrapping */
>>>@@ -195,7 +196,7 @@ int sss_packet_recv(struct sss_packet *packet, int fd)
>>> return ENODATA;
>>> }
>>>
>>>- if (*packet->len > packet->memsize) {
>>>+ if (packet_get_len(packet) > packet->memsize) {
>>> return EINVAL;
>>> }
>>>
>>>@@ -204,7 +205,7 @@ int sss_packet_recv(struct sss_packet *packet, int fd)
>>> return EAGAIN;
>>> }
>>>
>>>- if (packet->iop < *packet->len) {
>>>+ if (packet->iop < packet_get_len(packet)) {
>>> return EAGAIN;
>>> }
>>>
>>>@@ -223,7 +224,7 @@ int sss_packet_send(struct sss_packet *packet, int fd)
>>> }
>>>
>>> buf = packet->buffer + packet->iop;
>>>- len = *packet->len - packet->iop;
>>>+ len = packet_get_len(packet) - packet->iop;
>>>
>>> errno = 0;
>>> rb = send(fd, buf, len, 0);
>>>@@ -242,7 +243,7 @@ int sss_packet_send(struct sss_packet *packet, int fd)
>>>
>>> packet->iop += rb;
>>>
>>>- if (packet->iop < *packet->len) {
>>>+ if (packet->iop < packet_get_len(packet)) {
>>> return EAGAIN;
>>> }
>>>
>>>@@ -251,16 +252,37 @@ int sss_packet_send(struct sss_packet *packet, int fd)
>>>
>>>enum sss_cli_command sss_packet_get_cmd(struct sss_packet *packet)
>>>{
>>>- return (enum sss_cli_command)(*packet->cmd);
>>>+ uint32_t cmd;
>>>+
>>>+ SAFEALIGN_COPY_UINT32(&cmd, packet->buffer + sizeof(uint32_t),
NULL);
>> ^^^^^^^^^^^^^^^^
>> I would prefer to use constant
>> something like CMD_OFFSET=1*sizeof(uint32_t)
>>>+ return (enum sss_cli_command)cmd;
>>>}
>>>
>>>void sss_packet_get_body(struct sss_packet *packet, uint8_t **body, size_t
*blen)
>>>{
>>>- *body = packet->body;
>>>- *blen = *packet->len - SSS_NSS_HEADER_SIZE;
>>>+ *body = packet->buffer + 4*sizeof(uint32_t);
>> ^^^^^^^^^^^^^^^^^^
>> the same situation
>>>+ *blen = packet_get_len(packet) - SSS_NSS_HEADER_SIZE;
>>>}
>>>
>>>void sss_packet_set_error(struct sss_packet *packet, int error)
>>>{
>>>- *(packet->status) = error;
>>>+ SAFEALIGN_SET_UINT32(packet->buffer + 2*sizeof(uint32_t), error,
NULL);
>> ^^^^^^^^^^^^^^^^^^
>> the same situation
>>>+}
>>>+
>>>+static void packet_set_len(struct sss_packet *packet, uint32_t len)
>>>+{
>>>+ SAFEALIGN_SET_UINT32(packet->buffer, len, NULL);
>>This function should be similar to sss_packet_set_error and we should use
>>constant with value 0
>>
>>>+}
>>>+
>>>+static void packet_set_cmd(struct sss_packet *packet, enum sss_cli_command
cmd)
>>>+{
>>>+ SAFEALIGN_SET_UINT32(packet->buffer + sizeof(uint32_t), cmd, NULL);
>> ^^^^^^^^^^^^^^^^
>> the same situation
>>>+}
>>
>>It was little bit confusing for me read getters and setters.
>>It is not very likely that order sss_packet will be changed.
>>All constants should be defined on one place near definition
>>of structure sss_packet.
>>
>>LS
>
>Yes, it is more readable with the constants. I also realized that I
>used old SAFEALIGN_SET macros in the setters, so I changed that as
>well (the old ones are aliases so it is just cosmetic change).
>
>New patch is attached.
>
>Thanks,
>Michal
>
ACK
LS
The code looks good to me now, but I wonder if it would be more readable
to keep using the sss_packet members, just turn them into pointers?
Something like:
struct sss_packet {
uint8_t *len;
uint8_t *cmd;
uint8_t *status;
uint8_t *reserved;
uint8_t *body;
uint8_t *buffer;
}