Parse IP address from a GPRS module reply

Question

I'm writing a parser to get the IP address from a string I get from a GPRS module. Examples of the string I get from the GPRS module:

+QIACT: 1,1,1,\"10.162.143.228\"\r\n\r\nOK\r\n

+QIACT: 1,1,1,\"10.184.110.91\"\r\r\n+QIACT: 2,1,1,\"10.184.110.91\"\r\n\r\nOK\r\n

The string have the following format:

+QIACT: [ctx_id],[ctx_state],1,"[ip_address]"

• ctx_id is an integer that can go from 1 to 16.

• ctx_state is an integer that can be 0 (context deactivated) or 1 (context activated).

• ip_address which is the local ip address of the GPRS module.

I made some naive tests and the function is returning what's expected. I'm looking for some feedback/improvements I can do.

#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
#include <string.h>
#include <stdbool.h>
#define CTX_ID_MIN 1
#define CTX_ID_MAX 16
#define CTX_MAX_CNT CTX_ID_MAX + 1
#define CTX_ACTIVE '1'
#define INT_TO_ASCII(x) (x + '0')
#define IS_INVALID_CTX_ID(x) (CTX_ID_MAX < x || 0 == x)
#define GET_CTX_ID(x) (*(x + 1))
#define GET_CTX_STATE(x) (*(x + 1))
struct ctx {
 uint8_t ip[4];
 uint8_t id;
 bool is_active;
};
/**
 * The AT+QIACT? command will reply the list of the current activated context
 * and its IP address.
 *
 * Reply format: +QIACT: <ctx_id>,<ctx_state>,1,<ip_address>
 *
 * This function parses that reply (qiact_reply), look for the specified ctx_id
 * and verify if it's active, if so it will get the ip address and return true,
 * or return false otherwise.
 *
 * Examples qiact_reply data:
 * +QIACT: 1,1,1,\"10.162.143.228\"\r\n\r\nOK\r\n
 * +QIACT: 1,1,1,\"10.184.110.91\"\r\r\n+QIACT: 2,1,1,\"10.184.110.91\"\r\n\r\nOK\r\n
 */
static bool ctx_get_ip(char *qiact_reply, struct ctx *ctx)
{
 char *cursor = NULL;
 if (IS_INVALID_CTX_ID(ctx->id) || (NULL == qiact_reply)) {
 return false;
 }
 cursor = qiact_reply;
 // valid ctx_id's start at 1
 for (size_t i = 1; i < CTX_MAX_CNT; i++) {
 // Are we pointing to the ctx_id that we are looking for?
 char *_ctx_id = strchr(cursor, ' ');
 if (GET_CTX_ID(_ctx_id) == INT_TO_ASCII(ctx->id)) {
 char *ctx_state = NULL;
 ctx_state = strchr(qiact_reply, ',');
 // Point to the ctx_state field
 if (GET_CTX_STATE(ctx_state) != CTX_ACTIVE) {
 ctx->is_active = false;
 break;
 } else {
 ctx->is_active = true;
 // get the ctx ip
 char *ip_start = NULL;
 char *ip_end = NULL;
 ip_start = strchr(cursor, '"');
 ip_end = strchr(ip_start + 1, '.');
 // ip_start points to the first '"', we have to increment it by
 // 1 so it points to the beginning of the number, ip_end points
 // to the '.'.
 ctx->ip[0] = (uint8_t) strtoul(ip_start + 1, &ip_end, 10);
 // increment ip_start and ip_end
 ip_start = ip_end + 1;
 ip_end = strchr(ip_start, '.');
 ctx->ip[1] = (uint8_t) strtoul(ip_start, &ip_end, 10);
 // increment ip_start and ip_end again
 ip_start = ip_end + 1;
 ip_end = strchr(ip_start, '.');
 ctx->ip[2] = (uint8_t) strtoul(ip_start, &ip_end, 10);
 // increment ip_start and ip_end for the last time
 ip_start = ip_end + 1;
 ip_end = strchr(ip_start, '.');
 ctx->ip[3] = (uint8_t) strtoul(ip_start, &ip_end, 10);
 break;
 }
 } else {
 // Point to the next '+' symbol until we reach the last one
 cursor = strchr(cursor + 1, '+');
 if (NULL == cursor) {
 ctx->is_active = false;
 break;
 }
 }
 }
 return ctx->is_active;
}
int main(void) {
 struct ctx my_ctx = {
 .id = 1
 };
 ctx_get_ip("+QIACT: 1,1,1,\"10.184.110.91\"\r\r\n+QIACT: 3,1,1,\"10.185.110.91\"\r\n\r\nOK\r\n", &my_ctx);
 if (my_ctx.is_active) {
 printf("ctx %d is active: %s, ip: %d.%d.%d.%d\r\n", my_ctx.id,
 my_ctx.is_active ? "true" : "false",
 my_ctx.ip[0], my_ctx.ip[1], my_ctx.ip[2], my_ctx.ip[3]);
 } else {
 printf("ctx %d is active: %s\r\n", my_ctx.id,
 my_ctx.is_active ? "true" : "false");
 }
 return 0;
}

Output:

ctx 1 is active: true, ip: 10.184.110.91

• Summer: In order to be truly review ready, in my opinion you should implement the improvements you see before asking for review. I don't think that makes your question off-topic, but making the code as good as you can make the question better
• chux: static bool ctx_get_ip() returns bool not "Return the list of the current activated context and its IP address.". What does the return value of ctx_get_ip() imply? Looks more like success/failure.
• Carlos_47: @bruglesco I updated the question.
• Carlos_47: @chux "The Return the list of the current ..." text was some help from the GPRS module documentation, i updated the code so it doesn't get confused with the actual function behavior.

Answer 1

Use const

As code does not modify the string at qiact_reply, uses const for greater functionality (function can then accept const char *) and open to more optimizations. It also conveys to the caller that qiact_repl data is not changed.

Use integer

I'd expect CTX_ACTIVE to be 1 and not '1' and scan accordingly. As defined, it works in OP's code, but is unusual.

size_t

Good use of size_t for array indexing.

Why CTX_MAX_CNT?

CTX_MAX_CNT appears secondary. I'd limit parsing to the length of the string qiact_reply.

IP: No range protection¹

Pedantic code would insure range is [0...255]. That is easier to do with strtol() than strtoul(). Not a big issue though.

Parsing unclear

Subjective: I found the code difficult to assess how well it catches all corner cases of invalid input detection. To that end, I offer an alternative below. It has weakness¹ too, but I can see them easier.

I've allowed for liberal acceptance of white-space.

I also like the clear idea that *ctx_ip is only changed when the function returns true. Alternative code may want to clear *ctx_ip on false.

#define IP_FMT " \"%3" SCNu8 " .%3" SCNu8 " .%3" SCNu8 " .%3" SCNu8 " \""
static bool ctx_get_ip2(const char *qiact_reply, uint8_t ctx_id, struct ip_addr *ctx_ip) {
 if (IS_INVALID_CTX_ID(ctx_id) || (NULL == qiact_reply)) {
 return false; // bad ID or pointer
 }
 const char *s = qiact_reply;
 while (*s) {
 struct ip_addr my_ctx_ip;
 int my_ctx_id;
 char my_ctx_state;
 // Use %n to record scanning offset
 int n = 0;
 sscanf(s, " +QIACT: %2d , %c , 1 ," IP_FMT " %n", &my_ctx_id, &my_ctx_state, //
 &my_ctx_ip.ip[0], &my_ctx_ip.ip[1], &my_ctx_ip.ip[2], &my_ctx_ip.ip[3], //
 &n);
 if (n == 0) {
 return false; // Parsing incomplete, Invalid syntax
 }
 if (my_ctx_id == ctx_id) {
 if (my_ctx_state != CTX_ACTIVE) {
 return false; // Not active
 }
 *ctx_ip = my_ctx_ip;
 return true; // Success!
 }
 s += n; // advance to next
 }
 return false; // record not found
}

• Carlos_47: I think and correct me if i'm wrong, casting the found ip octect to uint8_t will make sure it's not bigger than 255. I tried to take into account all other suggestions, check the updated question for the new function. i will study your approach and see which one is "faster" and "smaller".
• chux: @Carlos_47 "casting the found ip octect to uint8_t will make sure it's not bigger than 255" is true for the ctx->ip[ip_octect], but is not true for ip_start. If ip_start was "256", that out-of-range text would not be saved correctly nor detected as an error. It comes down to how robust you want your parser and to avoid being a hacker victim.
• chux: @Carlos_47 Concerning the "updated question. See is it okay to edit my own question to include revised code? and its "should not append your revised code to the question.". Suggest rolling back your edit and post your own answer.

Answer 2

Based on the comments i updated the function, i feel more comfortable with it, changes:

• Removed unused macros.
• Macro to init the ctx struct (INIT_CTX).
• ctx_get_ip returns false on invalid data input, true otherwise.
• Replaced the for loop that iterated all the possible ctx id with a while loop that iterate the qiact_reply input until is invalid.
• Validation of the ctx_id found on the qiact_reply input.

What do you think?

#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
#include <string.h>
#include <stdbool.h>
#define IS_CTX_ACTIVE(x) ((*(x + 1)) == '1')
#define IS_VALID_CTX_ID(x) ((CTX_ID_RANGE_MIN <= x) && (CTX_ID_RANGE_MAX >= x))
#define INT_TO_ASCII(x) (x + '0')
#define GET_CTX_STATE(x) (*(x + 1))
struct ctx {
 uint8_t ip[4];
 uint8_t id;
 bool is_active;
};
/**
 * The AT+QIACT? command will reply the list of the current activated context
 * and its IP address.
 *
 * Reply format: +QIACT: <ctx_id>,<ctx_state>,1,<ip_address>
 *
 * This function parses that reply (qiact_reply), look for the specified ctx_id
 * and verify if it's active, if so it will get the ip address and set the
 * is_active member to true.
 *
 * @return false is case of any invalid data, true otherwise.
 *
 * Examples qiact_reply data:
 * +QIACT: 1,1,1,\"10.162.143.228\"\r\n\r\nOK\r\n
 * +QIACT: 1,1,1,\"10.184.110.91\"\r\r\n+QIACT: 2,1,1,\"10.184.110.91\"\r\n\r\nOK\r\n
 */
static bool ctx_get_ip(const char *qiact_reply, struct ctx *ctx)
{
 bool result = false;
 if (!IS_VALID_CTX_ID(ctx->id) || (NULL == qiact_reply)) {
 return result;
 }
 const char *cursor = strchr(qiact_reply, '+');
 // iterate @c qiact_reply while @c cursor points to a '+' sign (it's not NULL)
 while (NULL != cursor) {
 // Find the current <ctx_id> field
 char *ctx_id_s = strchr(cursor, ' ');
 char *ctx_id_e = strchr(ctx_id_s, ',');
 ctx_id_s++;
 uint8_t ctx_id = (uint8_t) strtoul(ctx_id_s, &ctx_id_e, 10);
 // Did we found the ctx id we are looking for?
 if ((ctx_id == ctx->id) && IS_VALID_CTX_ID(ctx_id)) {
 result = true;
 ctx->is_active = false;
 // If so, get the <ctx_state>
 char *ctx_state = strchr(cursor, ',');
 if (IS_CTX_ACTIVE(ctx_state)) {
 ctx->is_active = true;
 // The ctx state is active, let's get the <ip_address>
 char *ip_start = strchr(cursor, '"');
 char *ip_end = strchr(ip_start, '.');
 ip_start++; // point to the digit after '"'
 for (size_t ip_octect = 0; ip_octect < 4; ip_octect++) {
 ctx->ip[ip_octect] = (uint8_t) strtoul(ip_start, &ip_end, 10);
 // Move to the next octect, we are here on the second octect
 ip_start = ip_end + 1;
 ip_end = strchr(ip_start, '.');
 }
 }
 break;
 } else {
 // Move to the next '+' symbol
 cursor = strchr(cursor + 1, '+');
 if (NULL == cursor) {
 // We reached the end of @c qiact_reply without finding the
 // ctx id we were looking for, let's exit
 ctx->is_active = false;
 result = false;
 break;
 }
 }
 }
 return result;
}
#define INIT_CTX(ctx_id) \
{ \
 .id = ctx_id, \
 .ip[0] = 0, \
 .ip[1] = 0, \
 .ip[2] = 0, \
 .ip[3] = 0, \
 .is_active = false, \
}
int main(void) {
 struct ctx my_ctx = INIT_CTX(1);
 bool result = ctx_get_ip("+QIACT: 1,1,1,\"10.184.110.91\"\r\r\n"
 "+QIACT: 16,1,1,\"10.185.110.91\"\r\n\r\nOK\r\n",
 &my_ctx);
 if (result) {
 if (my_ctx.is_active) {
 printf("ctx %d is activated, ip: %d.%d.%d.%d\r\n", my_ctx.id,
 my_ctx.ip[0], my_ctx.ip[1], my_ctx.ip[2], my_ctx.ip[3]);
 } else {
 printf("ctx %d is deactivated\r\n", my_ctx.id);
 }
 } else {
 printf("Invalid input\r\n");
 }
 return 0;
}