Checking web-page repeatedly

Question

I have a program that gets 5 random dice throws from random.org over and over. In order to avoid doing getaddrinfo() over and over with the exact same data, I moved some of the code to an initialization function (and an unused finalization function).

My issues are:

• is this a bad idea?
• can bad things happen to the program in some situations?
• am I doing the socket thing correctly?

Any other pointers, suggestions, criticisms are very welcome.

#include <ctype.h>
#include <netdb.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/socket.h>
#include <sys/types.h>
#include <unistd.h>
struct Data {
 char datetime[20];
 int die[5];
};
struct ConnData {
 char cmd[200];
 size_t cmdlen;
 struct addrinfo *p;
 int csock;
};
int initdata(struct ConnData *cd, const char *url);
int fetchpage(char *dst, size_t dstlen, struct ConnData *cd);
int killdata(struct ConnData *cd);
int process(struct Data *data, const char *html);
int save(const struct Data *data);
int gotenough(void);
int initdata(struct ConnData *cd, const char *url) {
 struct addrinfo hints[1];
 struct addrinfo *servinfo;
 char host[100];
 char resource[100];
 char *phost, *presource;
 const char *ptr;
 int rv;
 strcpy(host, "");
 strcpy(resource, "");
 memset(hints, 0, sizeof hints);
 servinfo = NULL;
 cd->p = NULL;
 phost = host;
 presource = resource;
 ptr = url + 7; // skip over "http://"
 while (*ptr != '/') *phost++ = *ptr++;
 *phost = 0;
 while (*ptr) *presource++ = *ptr++;
 *presource = 0;
 cd->cmdlen = (size_t)sprintf(cd->cmd, "GET %s HTTP/1.0\r\n\r\n", resource);
 hints->ai_family = AF_UNSPEC;
 hints->ai_socktype = SOCK_STREAM;
 rv = getaddrinfo(host, "80", hints, &servinfo);
 if (rv) {
 fprintf(stderr, "getaddrinfo: %s\n", gai_strerror(rv));
 return 1;
 }
 for (cd->p = servinfo; cd->p; cd->p = cd->p->ai_next) {
 cd->csock = socket(cd->p->ai_family, cd->p->ai_socktype,
 cd->p->ai_protocol);
 if (cd->csock == -1) continue; // try next address
 rv = connect(cd->csock, cd->p->ai_addr, cd->p->ai_addrlen);
 close(cd->csock);
 if (rv < 0) continue; // try next address
 break; // successful connection
 }
 if (!cd->p) {
 fprintf(stderr, "Unable to connect to HTTP server\n");
 return 2;
 }
 freeaddrinfo(servinfo);
 return 0;
}
int killdata(struct ConnData *cd) {
 /* nothing to do */
 (void)cd;
 return 0;
}
int fetchpage(char *dst, size_t dstlen, struct ConnData *cd) {
 ssize_t nbuff = 1;
 size_t used = 0;
 cd->csock = socket(cd->p->ai_family, cd->p->ai_socktype,
 cd->p->ai_protocol);
 connect(cd->csock, cd->p->ai_addr, cd->p->ai_addrlen);
 send(cd->csock, cd->cmd, cd->cmdlen, 0);
 do {
 nbuff = recv(cd->csock, dst + used, dstlen - used - 1, 0);
 if (nbuff < 0) {
 fprintf(stderr, "error obtaining data\n");
 return 3;
 }
 used += (size_t)nbuff;
 if (used + 1 == dstlen) break; /* ignore stuff that doesn't fit */
 } while (nbuff > 0);
 dst[used] = 0;
 close(cd->csock);
 return 0;
}
int process(struct Data *data, const char *html) {
 char *ptr = NULL;
 ptr = strstr(html, "<p>You rolled 5 dice:</p>");
 if (!ptr) return 1;
 for (int k = 0; k < 5; k++) {
 ptr = strstr(ptr, ".png\" alt=\"");
 if (!ptr) return k + 2;
 ptr += 11;
 data->die[k] = atoi(ptr);
 }
 ptr = strstr(ptr, "<p>Timestamp: ");
 if (!ptr) return 7;
 strncpy(data->datetime, ptr + 14, 19);
 data->datetime[19] = 0;
 return 0;
}
int save(const struct Data *data) {
 printf("%s:", data->datetime);
 for (int k = 0; k < 5; k++) printf(" %d", data->die[k]);
 printf("\n");
 return 0;
}
int gotenough(void) {
 static int n = 0;
 n += 1;
 return (n == 10);
}
int main(void) {
 unsigned afewseconds = 3;
 char html[100000];
 char url[] = "http://www.random.org/dice/?num=5";
 struct Data data[1] = {0};
 struct ConnData cd[1] = {0};
 if (initdata(cd, url)) goto LBL_INIT;
 while (1) {
 if (fetchpage(html, sizeof html, cd)) goto LBL_FETCH;
 if (process(data, html)) goto LBL_PROCESS;
 if (save(data)) goto LBL_SAVE;
 if (gotenough()) break;
 sleep(afewseconds);
 }
 if (killdata(cd)) goto LBL_KILL;
 goto LBL_FINISH;
LBL_KILL:
 fprintf(stderr, "Killing error (never happens)\n");
 goto LBL_FINISH;
LBL_SAVE:
 fprintf(stderr, "Saving error.\n");
 goto LBL_FINISH;
LBL_PROCESS:
 fprintf(stderr, "Processing error.\n");
 goto LBL_FINISH;
LBL_FETCH:
 fprintf(stderr, "Fetching page error.\n");
 goto LBL_FINISH;
LBL_INIT:
 fprintf(stderr, "Initializing data.\n");
 goto LBL_FINISH;
LBL_FINISH:
 return 0;
}

• SylvainD: If what you want is to get random numbers, there should be an easiest way ;-)
• pmg: LOL -- that is just a working example of the real thing.

Answer 1

Things you could improve:

Dependencies:

• You are way overdoing this problem. By a lot. You can generate pseudo-random on your computer just fine. There is no reason to create a dependency on the internet for that. What if you don't have access to the internet? Right now your program fails to function, because of that huge and unnecessary dependency.

• If you absolutely must use the internet, use libcurl instead of your own DIY way of connecting to the internet. And make sure to support "offline-mode".

  libcurl is a free and easy-to-use client-side URL transfer library, supporting DICT, FILE, FTP, FTPS, Gopher, HTTP, HTTPS, IMAP, IMAPS, LDAP, LDAPS, POP3, POP3S, RTMP, RTSP, SCP, SFTP, SMTP, SMTPS, Telnet and TFTP. libcurl supports SSL certificates, HTTP POST, HTTP PUT, FTP uploading, HTTP form based upload, proxies, cookies, user+password authentication (Basic, Digest, NTLM, Negotiate, Kerberos), file transfer resume, http proxy tunneling and more!

  libcurl is highly portable, it builds and works identically on numerous platforms, including Solaris, NetBSD, FreeBSD, OpenBSD, Darwin, HPUX, IRIX, AIX, Tru64, Linux, UnixWare, HURD, Windows, Amiga, OS/2, BeOs, Mac OS X, Ultrix, QNX, OpenVMS, RISC OS, Novell NetWare, DOS and more...

Syntax/Styling:

• Never use goto.

  Neal Stephenson thinks it's cute to name his labels 'dengo'

  Yes, there are some rare situations where you may find it necessary to use it. This is not one of them.

   if (initdata(cd, url)) goto LBL_INIT;
   while (1) {
   if (fetchpage(html, sizeof html, cd)) goto LBL_FETCH;
   if (process(data, html)) goto LBL_PROCESS;
   if (save(data)) goto LBL_SAVE;
   if (gotenough()) break;
   sleep(afewseconds);
   }
   if (killdata(cd)) goto LBL_KILL;
   goto LBL_FINISH;
  LBL_KILL:
   fprintf(stderr, "Killing error (never happens)\n");
   goto LBL_FINISH;
  LBL_SAVE:
   fprintf(stderr, "Saving error.\n");
   goto LBL_FINISH;
  LBL_PROCESS:
   fprintf(stderr, "Processing error.\n");
   goto LBL_FINISH;
  LBL_FETCH:
   fprintf(stderr, "Fetching page error.\n");
   goto LBL_FINISH;
  LBL_INIT:
   fprintf(stderr, "Initializing data.\n");
   goto LBL_FINISH;
  LBL_FINISH:
   return 0;
  

  All you are doing is printing a simple unique error message for every goto label. And then you go to LBL_FINISH, where you indicate a successful return whether you have an error or not. Get rid of the gotos completely, and return unique error indicators.

• typedef your structs.

  struct Data {
   char datetime[20];
   int die[5];
  };
  

  The typedef means you no longer have to write struct all over the place. That not only saves some space, it also can make the code cleaner since it provides a bit more abstraction.

  typedef struct
  {
   char datetime[20];
   int die[5];
  } Data;
  

• Use parenthesis with sizeof

  sizeof hints
  

  sizeof(hints)
  

• This isn't a standard way to do a counter.

  int gotenough(void) {
   static int n = 0;
   n += 1;
   return (n == 10);
  }
  

Memory/Data Handling:

• Is your method killdata() trying to erase your struct?

  int killdata(struct ConnData *cd) {
   /* nothing to do */
   (void)cd;
   return 0;
  }
  

  Throwing your data into a void container doesn't "kill" it. You need to go through each of the struct members and set them to NULL.

Final Code:

I've re-written your code completely into a more portable program, that accomplishes the basic functionality that your program does. As you can see from the length of my program to your program, this isn't a very hard task to accomplish without using the internet.

#include <stdio.h>
#include <stdlib.h>
#include <time.h>
int main(void)
{
 srand((unsigned int) time(NULL));
 for (int i = 0; i < 100; i++)
 {
 printf("%d\n", (rand() % 6) + 1);
 }
 return 0;
}

• pmg: hmmm .. my issue is not with generation of (pseudo)random numbers, thank you anyway. My issue is with connecting to the internet repeatedly over and over again without wasting resources (the random dice is an example program that connects to the internet repeatedly over and over again).
• syb0rg: @pmg That is why I included the little thing about libcurl in there. It will reduce down your code a lot, and make things a bit more simple and easy to understand.
• pmg: "never use gotos": ok; "typedef your structs": no thanks, writing an extra struct is no pain and I fail to see what you gain for including abstraction in a program in this way; "use parenthesis with sizeof": no thanks, I'd use parenthesis if I applied sizeof to a type (and even then the parenthesis would belong to the type, not to the operator). Thank you for your review. I appreciate it.