Re: [bug-myserver] [PATCH] Exceptions

[Giuseppe Scrivano]

It is a big amount of work!  Thank you!

Here are my comments, you can apply them to your current code and use
this comment: "git commit --amend", your last commit will be modified.


Lisa Vitolo <syn.shai...@gmail.com> writes:

> From 80489b75c6147c78fec2d3f67d8422cd2486be90 Mon Sep 17 00:00:00 2001
> From: Nihal Delver <ni...@nihal.delvers>

can you please use your real name? :-)



> Date: Tue, 6 Apr 2010 21:09:36 +0200
> Subject: [PATCH] The exception support was created.

I would use "Map POSIX errors to C++ exceptions"



> The exception classes were created, representing errno values, and divided in 
> 3 categories.
> An abstract superclass allows the developer to get the error message, the 
> process backtrace,
> and a numeric value representing the priority of the error from the exception 
> object.
> The namespace of checked::function is formed by common used functions that
> in case of an error throw the right exception.

Please wrap these lines to 76 chars too.



>  myserver/include/base/exceptions/Makefile        | 1338 
> ++++++++++++++++++++++
>  myserver/include/base/exceptions/Makefile.in     | 1338 
> ++++++++++++++++++++++
>  myserver/src/base/exceptions/.deps/checked.Po    |  325 ++++++
>  myserver/src/base/exceptions/.deps/exceptions.Po |  212 ++++

These files are not for the patch, they are generated automatically by
the magic gnu tools.



> +#define CHECK(x) if((x) < 0) checked::raiseException();
> +#define CHECK_NULL(x) if((x) == NULL) checked::raiseException();

Please safeguard these macros, the "if" is dangerous, also, more
important, we need the return value, if it is not an error it can be
useful to the caller.

The macro should be like:

#define CHECK(x) {int _r = (x); if (_r < 0) nop (); else return _r;}

but I really hate a return statement in a macro.  I think we need a
function here:

int
checkError (int x)
{
  if (x < 0)
    checked::raiseException();
  else
    return x;
}

const void *
checkErrorNull (const void *x)
{
  if (x == NULL)
    checked::raiseException();
  else
    return x;
}


so you can do something like (notice the explicit return):

namespace checked
{
  int read (int fd, void *buf, size_t count)
  {
    return gnulib::read (fd, buf, count);
  }
...
}

I see `read(2)', that I have used in my example, is not specified in the
namespace, please add it too.



> +  int accept (int fd, struct sockaddr *addr, socklen_t *addrlen)
> +  {
> +    CHECK (gnulib::accept (fd, addr, addrlen));
> +  }
> +
>
>                  [ ... ]
> +
> +      case EXDEV:
> +        throw LinkException ();
> +        break;
> +
> +      case EXFULL:
> +        throw InvalidExchangeException ();
> +        break;
> +
> +      default:
> +        throw UnknownException ();
> +        break;
> +    }
> +  }
> +  
> +}

Please move all the previous declarations (and the missing one for
`read(2)') to the checked.cpp file.



> diff --git a/myserver/include/base/exceptions/exceptions.h 
> b/myserver/include/base/exceptions/exceptions.h

.h files must be include-guarded in a global:

#ifndef EXCEPTIONS_H
# define EXCEPTIONS_H

[... everything you have now in the file ...]

#endif

> +/*
> + * Priorities
> + */
> +#define LOW_P    0
> +#define MEDIUM_P 1
> +#define HIGH_P   2

Please use an enum for this, or even better, the already defined
LoggingLevel enum present in include/log/stream/log_stream.h, so we can
have a 1-to-1 mapping with the logging classes.



> +class AbstractServerException: public exception
> +{
> +  void **buffer;
> +  int size;
> +  int priority;
> +     
> +  protected:
> +    int localErrno;
> +    char **btString;
> +    
> +    void setPriority (int p)
> +    {
> +             priority = p;
> +     }

Please move the private and the protected section after the public
section, it makes the code slightly more readable as the reader
immediately see the public interface exposed by the class.

It also means that any macro definition in the file must be reindented.

I know doxygen comments are boring :-)  But please add them at least for
the most important classes.  Child classes can use a "\see" to the
parent class method, we can do this after the patch, it is not important
now.



> +  public:
> +     AbstractServerException ()
> +     {
> +       localErrno = errno;
> +       buffer = NULL;
> +       btString = NULL;
> +     
> +       #ifdef HAVE_BACKTRACE
> +         size = 20;
> +             buffer = (void **) gnulib::malloc (sizeof(void *) * size);
> +             int w = backtrace (buffer, size);
> +      #endif

We don't really want this, alloc'ing memory while handling an exception
is not a good idea.


Everything after the "= NULL" initializations, must be moved to the
getBacktrace function as:

if (! buffer)
  {
# ifdef HAVE_BACKTRACE
[...]
# endif

# ifdef HAVE_BACKTRACE_SYMBOLS
[...]
  #endif
     return btString;
  }


I would play safe and do at the beginning of the function:

  if (errno == ENOMEM)
    return NULL;



> +     virtual ~AbstractServerException () throw ()
> +     {
> +       free (buffer);
> +      free (btString);
> +     }

if (buffer)
   free (buffer);

if (btString)
   free (btString);


and again, great work!  MyServer code will look much better with C++
exceptions!

Cheers,
Giuseppe