I have the following HTTP reply saved in a local file called source.txt:

HTTP/1.1 301 Moved
Connection: close
Content-length: 111
Location: https://11.12.13.14:81/
Content-type: text/html; charset="utf-8"

<html><head><META HTTP-EQUIV="refresh" CONTENT="0;URL=https://11.12.13.14:81/"></head><body></body></html>

Source code:

#include <stdio.h>
#include <stdlib.h>
#define MAXBUFLEN 1024

char* getLocation(char* source)
{
    const char *p1 = strstr(source, "Location:")+10;
    const char *p2 = strstr(p1, "\n");
    size_t len = p2-p1;
    char *res = (char*)malloc(sizeof(char)*(len+1));
    strncpy(res, p1, len);
    res[len] = '\0';
    return res;
}

char* getData(char* source)
{
    const char *p1 = strstr(source, "://")+3;
    const char *p2 = strstr(p1, "\n");
    size_t len = p2-p1;
    char *res = (char*)malloc(sizeof(char)*(len+1));
    strncpy(res, p1, len);
    res[len] = '\0';
    return res;
}

int main()
{
    char source[MAXBUFLEN];
    char host[100];
    int port;
    FILE *fp = fopen("source.txt", "r");
    if (fp != NULL) {
        size_t newLen = fread(source, sizeof(char), MAXBUFLEN, fp);
        if (newLen == 0) {
            fputs("Error reading file", stderr);
        } else {
            source[++newLen] = '\0';

//extraction code
            char* res = getLocation(source);
            printf("getLocation result: %s\n", res);
            if (strstr(res, "://"))
            {
                res = getData(source);
                printf("getData result: %s\n", res);
                if (strstr(res, ":"))
                {
                    sscanf(res, "%[^:]:%d[^/]", host, &port);
                    printf("host: %s | port: %d\n", host, port);
                }
                else
                    printf("delimiter not found\n");
            }
            else
                printf("no link\n");
//
        }
    }
    fclose(fp);
}

The program is working well, but it's very ugly. Is there any way to improve the code to avoid doing so many operations? I mean somehow merging the two functions, getLocation() and getData().

  • What about at least a minimal description on what this functions should do? – alk Sep 7 '13 at 16:15
  • you really need a description for that short code? – Sam Reina Sep 8 '13 at 12:35
  • Please excuse, I do not need nothing, not even to read your posting. I just tried to kindly mention that an introduction to an issue, raises the chances of keeping a possible reader attracted. – alk Sep 8 '13 at 12:37

Your getLocation can be improved a little:

For a start, the call parameter should be const (although if you were happy modifying the string you could avoid the memory allocation).

Moving on:

const char *p1 = strstr(source, "Location:")+10;

This is ok, except that p1 is cryptic and it is better to avoid embedded constants such as 10. Since this is the length of "Location:", we can extract that:

#define LOC "Location:"
const char *loc = strstr(source, LOC) - 1 + sizeof LOC;

Note the -1 for the \0 at the end of LOC. But it is now uglier still. And of course it might fail, returning NULL. So we need to protect the next line:

const char *p2 = strstr(p1, "\n");

in which you are looking for just one character; strchr is more appropriate and eol is a more descriptive name:

if (loc) {
    const char *eol = strchr(loc, '\n');
}

So next you get the length

size_t len = p2-p1;

which needs a cast to avoid a sign-conversion warning:

size_t len = (size_t) (eol - loc);

You allocate memory next:

char *res = (char*)malloc(sizeof(char)*(len+1));

but you should omit the cast and note that sizeof(char) is 1 by definition, so:

char *res = malloc(len + 1);

and of course allocation could also fail, so you must check before copying the address into it. You copy the address thus:

strncpy(res, p1, len);
res[len] = '\0';

but as you already know the length of the string, memcpy would be better, as strncpy must check each char in the input string for \0

if (res) {
    memcpy(res, loc, len);
    res[len] = '\0';
}

Putting it all together, we get:

static char* getLocation(const char* source)
{
#define LOC "Location:"
    size_t len = 0;
    char *eol = NULL;
    const char *loc = strstr(source, LOC) - 1 + sizeof LOC;
    if (loc) {
        eol = strchr(loc, '\n');
    }
    if (eol && loc) {
        len = (size_t) (eol - loc);
    }
    if (len) {
        char *res = malloc(len + 1);
        if (res) {
            memcpy(res, loc, len);
            res[len] = '\0';
        }
        return res;
    }
    return NULL;
}

Yes, string handling in C is ugly.

My conclusion from this ugliness is that another approach is needed. Why not modify the input string by putting a \0 at eol and returning loc? Then you could extract the address and port directly from that:

static char* getLocation(char* source)
{
#define LOC "Location:"
    char *loc = strstr(source, LOC) - 1 + sizeof LOC;
    if (loc) {
        char *eol = strchr(loc, '\n');
        if (eol) {
            *eol = '\0';
        }
    }
    return loc;
}

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.