Hacker News Reader

Comment by midas007 | Hacker News Reader

    TRUE * 1e6

It's horrible code that seems to indicate a lack of production coding standards.

Errors, warnings and assertions need to be handled semantically sensibly, obviously and consistently.

Most functions, errors should be handled individually and returned as they happen and the only condition at the very bottom should be success.

My personal fav is anomaly(...) which checks for a nonfatal but suspicious condition. It always throws a warning on STDERR if it fails, even when compiled with -DNDEBUG.

Example: https://gist.github.com/steakknife/9271284

If anyone wants to see decent examples of real C/C++ done right:

nginx [0], doom3 [1], postgres [2], varnish [3] and openssh [4]

References:

[0] http://hg.nginx.org/nginx/file/0251f2f1dc93

[1] https://github.com/TTimo/doom3.gpl

[2] https://github.com/postgres/postgres

[3] https://www.varnish-cache.org/trac/browser

[4] http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/

View on HN

Do you have any specific examples? Just picking one randomly and the first error checking code I saw looked like this:

  if (ctx->main_conf == NULL) {
     return NGX_CONF_ERROR;
  }

One of my personal "favorites" is this gem from OpenSSH:

    /* No MIN_SIZEOF here - we absolutely *must not* truncate the
     * username (XXX - so check for trunc!) */
    strlcpy(li->username, pw->pw_name, sizeof(li->username));

I don't think that is anything out of the ordinary, besides if pw->pw_name is longer that sizeof(li->username) it will get truncated. I was more thinking of error checking macros and error handling.

Basically the remark was that the comment went from discussing error handling, to the (much) broader scope of 'C and C++ done "right"', and as an example 5 complete, large open source projects were dumped for us to find the examples ourselves.

Just because the code doesn't misuse memory doesn't mean the code is correct. In this case, the return value of strlcpy should be checked to verify that the username is not truncated as it is added to this data structure. The comment even states that the truncation must be avoided, but rather than adding a check there is simply a XXX ;P.

I see, thanks! That's pretty funny. :D

If you want to get good at something, you're going to have to work at it. No one else can give you a shortcut to common sense that comes with mastery. Only pointers. That's just the way life works.

I can figure out how to look up these projects myself. The point is, if you are going to give examples on error handling be more specific. I don't need any general "life lessons" here, the point is it looked like you were trying to make a point, without having one. I.e you did not actually have any examples, you only made it appear so.

Assuming loginrec.c, it's been refactored to this in 6.5p1:

    if (strlcpy(li->username, pw->pw_name, sizeof(li->username)) >=
        sizeof(li->username)) {
      error("%s: username too long (%lu > max %lu)", __func__,
          (unsigned long)strlen(pw->pw_name),
          (unsigned long)sizeof(li->username) - 1);
      return NULL;
    }

Yay! \o/ :D

Alternatively (assuming dst isn't a pointer):

    rv = strlcpy(dst, src, sizeof(dst));
    check(rv == strlen(dst), "Error, src string truncated");

Or perhaps more clear:

    strlcpy(dst, src, sizeof(dst));
    check(strlen(src) == strlen(dst), "Error, src string truncated");

Reply on news.ycombinator.com