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 <mzi...@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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
