[Ovirt-devel] [PATCH ovirt-node] Allow the appliance to manage the hardware it is running on - part 1

[Jim Meyering]

Alan Pevec <apevec at redhat.com> wrote:
> From: Chris Lalancette <clalance at redhat.com>
>
>     Add ovirt-listen-awake daemon.  This is a very small daemon that oVirt
>     appliance can communicate with to send simple messages to the host it is
>     running on. It's used so that the appliance can "manage" the hardware
>     it is running on, removing the need for the fake oVirt Nodes.

Hi Chris,

I haven't built or tested, but did go through and write some suggestions:

> diff --git a/ovirt-listen-awake/ovirt-listen-awake.c b/ovirt-listen-awake/ovirt-listen-awake.c
> +// We only ever expect to receive the strings "AWAKE" or "IDENTIFY", so
> +// 20 bytes is more than enough
> +#define BUFLEN 20
> +#define LOGFILE "/var/log/ovirt-host.log"
> +
> +static int streq(char *first, char *second)
> +{
> +  int first_len, second_len;
> +
> +  first_len = strlen(first);
> +  second_len = strlen(second);
> +
> +  if (first_len == second_len && strncmp(first, second, first_len) == 0)
> +    return 1;
> +  return 0;
> +}

Just looking at the function name, I wondered if this was stripping
trailing or leading spaces.  In fact, it does neither.
If you add a comment something like this, others won't wonder ;-)

/* Truncate the NUL-terminated string, BUFFER, at the first
   white space byte.  */

> +static void strip_space(char *buffer)
> +{
> +  int i;
> +
> +  i = 0;
> +  while (buffer[i] != '\0') {
> +    if (isspace(buffer[i])) {
> +      // once we see the first whitespace character, we are done
> +      buffer[i] ='\0';
> +      break;
> +    }
> +    i++;
> +  }
> +}
> +
> +static int listenSocket(int listen_port)
> +{
> +  struct sockaddr_in a;
> +  int s,yes;
> +
> +  if (listen_port < 0) {
> +    fprintf(stderr, "Invalid listen_port %d\n", listen_port);

Please include "argv[0]: " at the beginning of each diagnostic.  That is
recommended practice, because it easier to figure out which program
produces that error message buried in thousands of lines from many
different programs.

Easiest might be to do this (note no trailing "\n"):
[first #include <error.h>, of course]

  error(0, 0, "Invalid listen_port %d", listen_port);

Don't worry about the portability of "error".
If it becomes an issue, we can use gnulib.

> +    return -1;
> +  }
> +
> +  s = socket(AF_INET, SOCK_STREAM, 0);
> +  if (s < 0) {
> +    fprintf(stderr, "Failed creating socket: %s\n", strerror(errno));

  error(0, errno, "Failed creating socket");

> +    return -1;
> +  }
> +  yes = 1;
> +  if (setsockopt(s, SOL_SOCKET, SO_REUSEADDR,(char *)&yes, sizeof (yes)) < 0) {
> +    fprintf(stderr, "Failed setsockopt: %s\n", strerror(errno));

  error(0, errno, "Failed setsockopt");

> +    close(s);
> +    return -1;
> +  }
> +  memset (&a, 0, sizeof (a));
> +  a.sin_port = htons (listen_port);
> +  a.sin_family = AF_INET;
> +  if (bind(s, (struct sockaddr *) &a, sizeof (a)) < 0) {
> +    fprintf(stderr, "Error binding to port %d: %s\n", listen_port,
> +	    strerror(errno));

Likewise.
You can probably replace all fprintf(stderr,... uses.

> +    close(s);
> +    return -1;
> +  }
> +  listen(s, 10);

Don't you want to diagnose listen failure?
It looks like at least EADDRINUSE can occur.

> +  return s;
> +}
> +
> +static void usage(void)
> +{
> +  printf("Usage: host-startup [OPTIONS]\n");

host-startup?  must be an old name.

I like to set a global "program_name = argv[0];" in main,
then use that from the usage function.

> +  printf("OPTIONS:\n");
> +  printf(" -h\tShow this help message\n");
> +  printf(" -n\tDo not daemonize (useful for debugging\n");

Sometimes it's important not to exit(1), e.g. for --help,
so either move the "exit" out to each caller or make the exit
status a parameter.

> +  exit(1);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +  int listen_socket, conn;
> +  struct sockaddr_in client_address;
> +  unsigned int addrlen;
> +  FILE *conn_stream;
> +  FILE *logfile;
> +  char buffer[BUFLEN];
> +  int c;
> +  int do_daemon;
> +
> +  do_daemon = 1;
> +
> +  while ((c=getopt(argc, argv, ":hn")) != -1) {
> +    switch(c) {
> +    case 'h':
> +      usage();
> +      break;
> +    case 'n':
> +      do_daemon = 0;
> +      break;
> +    default:
> +      usage();
> +    }
> +  }
> +
> +  if ((argc-optind) != 0)
> +    usage();

I know this is just a tiny helper program,
but it's nice to diagnose this with e.g.,

    ovirt-listen-awake: extra operand "FOO"

rather than just printing help output.  E.g.,

    $ who am i j
    who: extra operand `j'
    Try `who --help' for more information.
    [Exit 1]

> +  listen_socket = listenSocket(7777);
> +  if (listen_socket < 0)
> +    return 2;
> +
> +  if (do_daemon) {
> +    logfile = fopen(LOGFILE,"a+");
> +    if (logfile == NULL) {
> +      fprintf(stderr, "Error opening logfile %s: %s\n",
> +	      LOGFILE, strerror(errno));
> +      return 3;
> +    }
> +
> +    // NOTE: this closes stdout and stderr
> +    daemon(0, 0);

Upon failure (i.e., to fork), diagnose and exit:

       if (daemon(0, 0) < 0)
         error(EXIT_FAILURE, errno, "Failed do daemonize");

> +    // so re-open them to the logfile here
> +    dup2(fileno(logfile), STDOUT_FILENO);
> +    dup2(fileno(logfile), STDERR_FILENO);
> +  }
> +
> +  while (1) {
> +    addrlen = sizeof(client_address);
> +    memset(&client_address, 0, addrlen);
> +    memset(buffer, 0, BUFLEN);

This memset looks unnecessary.

> +    conn = accept(listen_socket, (struct sockaddr *)&client_address, &addrlen);
> +    if (conn < 0) {
> +      fprintf(stderr, "Error accepting socket: %s\n", strerror(errno));
> +      continue;
> +    }
> +
> +    conn_stream = fdopen(conn, "r");
> +    if (conn_stream == NULL) {
> +      fprintf(stderr, "Error converting fd to stream: %s\n", strerror(errno));
> +      close(conn);
> +      continue;
> +    }
> +
> +    if (fgets(buffer, BUFLEN, conn_stream) == NULL) {

You may want a separate test to detect EOF.
Otherwise, errno is not defined in that case.

> +      fprintf(stderr, "Error receiving data: %s\n", strerror(errno));
> +      fclose(conn_stream);
> +      continue;
> +    }
> +
> +    strip_space(buffer);
> +
> +    if (streq(buffer, "IDENTIFY")) {
> +      // run ovirt-identify-node against 192.168.50.2 (the WUI node)
> +      fprintf(stderr, "Doing identify\n");
> +      // FIXME: it would be best to call the "find_srv" shell script here to
> +      // find out where we should contact.  However, we would still have to
> +      // hardcode which DNS server to use (192.168.50.2), and which domain
> +      // name to use "priv.ovirt.org" to get this to work.  I don't have
> +      // a good idea how to solve this at the moment.
> +      system("ovirt-identify-node -s 192.168.50.2 -p 12120");
> +    }
> +    else if (streq(buffer, "AWAKE")) {
> +      // run ovirt-awake against 192.168.50.2
> +      fprintf(stderr, "Doing awake\n");
> +      // FIXME: I hate to duplicate this stuff here, but I can't use the
> +      // ovirt init scripts as-is; they depend too much on the environment
> +      // (in particular, which DNS server to use to resolve, and which
> +      // domainname).  Until I come up with a good solution for that, I'll
> +      // have to leave this as-is.
> +      system("wget -q http://192.168.50.2:80/ipa/config/krb5.ini -O /etc/krb5.conf");
> +      system("ovirt-awake start 192.168.50.2 12120 /etc/libvirt/krb5.tab");
> +    }
> +    else {
> +      fprintf(stderr, "Unknown command %s\n", buffer);
> +    }
> +
> +    fclose(conn_stream);
> +  }
> +
> +  close(listen_socket);
> +  fclose(logfile);

Might be nice to exit nonzero upon fclose failure (i.e., write failure).

> +  return 0;
> +}