Struct to web style string

Question

I've had to make several functions to turn some structures into strings. I am a still green when it comes C so I am unsure if I am doing this a very awkward way. The system I am coding for does not have snprintf, I know that would be far more elegant, however I cannot use it. Any advice?

int device_to_string(char* const asString, pDevice dev, size_t maxLength)
{
  char* ipAsString;
  size_t actualLength;
  struct in_addr addr;

  if (dev == NULL)
  {
    return NULL_ERROR;
  }

  addr.s_addr = dev->ip;
  ipAsString = inet_ntoa(addr);

  actualLength = strlen("name=")  + strlen(dev->name) +
                 strlen("&ip=")   + strlen(ipAsString) +
                 strlen("&mac=")  + strlen(dev->mac)  +
                 strlen("&type=") + strlen(dev->type) + 1;

  if (actualLength > maxLength)
  {
    return SIZE_ERROR;
  }

  strncat(asString, "name=",    strlen("name="));
  strncat(asString, dev->name,  strlen(dev->name));
  strncat(asString, "&ip=",     strlen("&ip="));
  strncat(asString, ipAsString, strlen(ipAsString));
  strncat(asString, "&mac=",    strlen("&mac="));
  strncat(asString, dev->mac,   strlen(dev->mac));
  strncat(asString, "&type=",   strlen("&type="));
  strncat(asString, dev->type,  strlen(dev->type));

  asString[actualLength] = '\0';

  return NO_ERROR;
}

Answer 1

Yeah, without snprintf and sprintf it gets a bit tedious, but I think this code is actually quite clear. You use your horizontal and vertical whitespace very well, and it's clear what you're doing with each block of code. You have also controlled for any possible issues that might come up (null pointer, insufficient buffer length, etc). Maybe there's a more concise way to do it, but in terms of clarity and maintainability I think this code will suffice.

Answer 2

One thing I would personally do would be to move the string concatenation into its own function since I see you have repeated the same 2 lines multiple times:

strncat(asString, "name=",    strlen("name="));
strncat(asString, dev->name,  strlen(dev->name));

I'd have something like:

void addParam( char *paramString, const char *paramName, const char *paramValue )
{
    strncat( paramString, paramName, strlen( paramName );
    strncat( paramString, paramValue, strlen( paramValue );
}

Reduces the length of your function and makes it slightly cleaner, imho.

Answer 3

Given the specification of strncat(), you are misusing it; if you must use a concatenation operation, simply use strcat().

For reference, the C standard says:

§7.21.3.2 The strncat function

Synopsis

#include <string.h>
char *strncat(char * restrict s1, const char * restrict s2, size_t n);

Description

The strncat function appends not more than n characters (a null character and characters that follow it are not appended) from the array pointed to by s2 to the end of the string pointed to by s1. The initial character of s2 overwrites the null character at the end of s1. A terminating null character is always appended to the result.²⁶¹⁾

²⁶¹⁾ Thus, the maximum number of characters that can end up in the array pointed to by s1 is strlen(s1)+n+1.

Now, don't get me wrong - what you are doing is safe, because you've previously checked the length, but making strncat() do the donkey work of checking the length is really pointless. You are also not telling the whole truth to strncat(); there is more space in the target buffer than you are admitting. That is safe, but it is a bit wasteful.

---

I suggest using a simple function such as:

char *copy_string(char *target, const char *source)
{
    strcpy(target, source);
    return(target + strlen(source));
}

target = asString;
target = copy_string(target, "name=");
target = copy_string(target, dev->name);
target = copy_string(target, "&ip=");
target = copy_string(target, ipAsString);
target = copy_string(target, "&mac=");
target = copy_string(target, dev->mac);
target = copy_string(target, "&type=");
target = copy_string(target, dev->type);

One advantage of this is that it avoids quadratic behaviour. As your output string grows longer, the strncat() operation gradually has to skip over more and more characters each time it is called. With long enough strings, this can become a measurable overhead. It is a nuisance that strcpy() et al return the start address of the string instead of the address of the null at end of the string; it means you end up scanning it twice - once to copy, once to determine the length.

---

That notation is still a bit tedious to use. I have previously created variable-length argument list functions like this:

#include <stdarg.h>

char *vstrcpy(char *buffer, size_t buflen, ...)
{
    const char *arg;
    char *bufend = buffer + buflen;
    char *target = buffer;
    va_list args;

    va_start(args, buflen);
    while ((arg = va_arg(args, const char *)) != 0)
    {
         size_t arglen = strlen(arg);
         if (target + arglen >= bufend)
             return(0);
         strcpy(target, arg);
         target += arglen;
    }
    return(target);
}

target = vstrcpy(asString, maxLength, "name=",  dev->name,
                                      "&ip=",   ipAsString,
                                      "&mac=",  dev->mac,
                                      "&type=", dev->type, (const char *)0);
if (target == 0)
    ...something went wrong despite your pre-computation...

Note that you must provide an explicit cast to that final end marker; you would be invoking undefined behaviour on Windows 64 (as a specific example) if you omitted the cast. This is as notationally convenient as the equivalent snprintf() (it requires you to write a null pointer at the end instead of a format string consisting of repeated %s operations near the beginning), and it is as safe. You can decide on the best return value - I've chosen NULL to indicate that there wasn't enough space and a pointer to the final null if there is enough room. You could mimic snprintf() more closely if you calculated the full length that would be required, just making sure to skip the copying when you've reached the end of the string. You probably then need to use offsets rather than pointers because although you are guaranteed to be able to evaluate buffer + buflen, you are not guaranteed to be able to evaluate buffer + buflen + 1. That design leads to:

#include <stdarg.h>

size_t vstrcpy(char *buffer, size_t buflen, ...)
{
    const char *arg;
    char *target = buffer;
    size_t offset = 0;
    va_list args;

    va_start(args, buflen);
    while ((arg = va_arg(args, const char *)) != 0)
    {
         size_t arglen = strlen(arg);
         if (offset + arglen < buflen)
             strcpy(target + offset, arg);
         offset += arglen;
    }
    return(offset);
}

len = vstrcpy(asString, maxLength, "name=",  dev->name,
                                   "&ip=",   ipAsString,
                                   "&mac=",  dev->mac,
                                   "&type=", dev->type, (const char *)0);

if (len >= maxLength)
    ...something went wrong despite your pre-computation...

This only copies whole arguments that fit. If there are 20 characters left in the buffer but the next string is 30 characters long, then the 20 characters are left unused. You still get told the actual length of the space required, though, and are guaranteed no overflow. If you have different requirements (such as copying as much as possible, even if it means a partial argument copy), then modify the code to do as you require. In my book, if the strings don't fit where I'm trying to place them, I've screwed up.

---

I'm not sure if modern optimizing compilers pre-compute strlen("string constant"). If they don't, you can get a small benefit from using sizeof("string constant")-1 instead, where the -1 accounts for the terminal null that sizeof() includes in the size it returns.

---

This example is not big enough to benefit from a table-driven approach to assembling the string. Occasionally, if the structures have enough elements in them, you can use a table-driven approach with the offsetof() macro identifying the start locations of the string members in the data structure. You might need to encode the types for mixed types (such as the IP address, or numbers that have to be converted to a string before printing). It is those complications that mean you need a considerable number of elements in the structures before you use the technique.

In outline, for a structure that only contains null terminated strings, you can do:

struct description
{
    const char *tag;
    size_t      offset;
};

static const struct description dev_desc[] =
{
    { "name=",  offsetof(pDevice, name) },
    { "&ip=",   offsetof(pDevice, ip)   },  // Taking liberties here!
    { "&mac=",  offsetof(pDevice, mac)  },
    { "&type=", offsetof(pDevice, type) },
};
enum { DEV_DESC_SIZE = sizeof(dev_desc) / sizeof(*dev_desc) };

size_t offset = 0;
for (size_t i = 0; i < DEV_DESC_SIZE; i++)
{
     offset = vstrcpy(&asString[offset], maxLength - offset,
                      dev_desc[i].tag, ((char *)&dev + dev_desc[i].offset),
                      (const char *)0);
     if (offset > maxLength)
         break;
}

This iterates through the fields in the structure dev using the control information from the descriptor array dev_desc. The expressions get more complex if you have to deal with other types than just character strings. If you ever have the misfortune to deal with structures with hundreds of elements, though, this can be a life-saver since you reduce the code to a simple loop and a simple table which encapsulates the complexity of the structure. (I see Jerry Coffin gave an answer using some of this idea, but I think my version has some merits over his.)

---

Your final assignment buys you nothing:

  asString[actualLength] = '\0';

The null was inserted by the final strncat().

Answer 4

There's a bigger-picture issue here; this kind of manual string manipulation is a Bad Idea. It's guaranteed to be buggy, often with security consequences.

You ought to use (preferred) or at least implement a string utility class. Here are a couple examples:

• http://git.gnome.org/browse/glib/tree/glib/gstring.h
• http://cgit.freedesktop.org/dbus/dbus/tree/dbus/dbus-string.h
• ftp://vsftpd.beasts.org/users/cevans/untar/vsftpd-2.3.2/str.h

This way all your string code will be much more concise, and entire classes of bugs eliminated.