[zeromq-dev] CZMQ: Error checking of z*_new() calls in other z*_new() functions

[Pieter Hintjens]

I hate C macros, with a very few exceptions. They break all the rules
on naming, and organization, and APIs. Please never macro, if there is
any sane alternative :-)

It's fine that the self allocation is a special case since self is a
special case anyhow (it's not maintained, every constructor always
allocates a self). The code meets the two goals of (a) catching the
failure and (b) writing code that's visually verifiable.

I'm satisfied and will update CLASS with this recommendation.

-Pieter

On Fri, Sep 5, 2014 at 4:04 PM, Goswin von Brederlow <goswin-v-b at web.de> wrote:
> On Fri, Sep 05, 2014 at 01:35:54PM +0200, Pieter Hintjens wrote:
>> What do you think of this style, for instance (slight change to
>> ziflist constructor):
>>
>>         self->names = zlist_new ();
>>         if (self->names)
>>             self->addresses = zlist_new ();
>>         if (self->addresses)
>>             self->netmasks = zlist_new ();
>>         if (self->netmasks)
>>             self->broadcasts = zlist_new ();
>>         if (self->broadcasts)
>>             ziflist_reload (self);
>>         else
>>             ziflist_destroy (&self);
>>
>> ...?
>>
>> It's clearly correct by instant visual inspection, and trivial to maintain.
>
> Neat trick. That wouldn't work for allocating self itself though.
>
> How about this:
>
> #define CHECK(x) if (!(x)) goto out
>
>         CHECK (self = zmalloc (sizeof (self_t));
>         CHECK (self->names = zlist_new ());
>         CHECK (self->addresses = zlist_new ());
>         CHECK (self->netmasks = zlist_new ());
>         CHECK (self->broadcasts = zlist_new ());
>         CHECK (ziflist_reload (self));
>         ...
>         return self;
>     out:
>         ziflist_destroy (&self);
>         return self;
>
>
>> On Thu, Sep 4, 2014 at 8:55 PM, Pieter Hintjens <ph at imatix.com> wrote:
>> > I agree, and it was meant to be safe to call the destructor on an
>> > unfinished object.
>> >
>> > I don't like the long lists of checks, they are a maintenance issue.
>> >
>> > It might be worth finding a better code style for constructors that
>> > avoids this duplication and yet gives us clean all or nothing
>> > construction. A goto might be neat. Or a series of chained if
>> > statements. I'll experiment...
>> >
>> >
>> >
>> > On Thu, Sep 4, 2014 at 4:06 PM, Goswin von Brederlow <goswin-v-b at web.de> wrote:
>> >> On Thu, Sep 04, 2014 at 11:51:27AM +0200, Pieter Hintjens wrote:
>> >>> I'd prefer assertions in most places. The problem is that adding full
>> >>> error handling on every allocation creates more complex code and
>> >>> overall increases the risk of other errors.
>> >>>
>> >>> There are a few places where it's worth catching heap exhaustion, and
>> >>> that is on message handling, ans you'll see this done in zmsg and
>> >>> zframe for instance.
>> >>>
>> >>> However for the most part I don't think application developers are
>> >>> competent to deal with out of memory situations correctly, and the
>> >>> best strategy is to fail loudly and force the machine or architecture
>> >>> to be fixed.
>> >>>
>> >>> So we can start by asserting inside constructors, and then fix those
>> >>> constructors we want to, to return NULL on failures.
>> >>
>> >> What makes it complex is having to check every allocation and then
>> >> error out if it failed. So you get tons of extra lines all doing the
>> >> same (other than the check), possibly with more and more cleanup.
>> >>
>> >> One thing that I think keeps this managable is to not check every
>> >> allocation by itself. Instead do all allocations in a block, then
>> >> check if any of them has failed and only then start configuring or
>> >> using them.
>> >>
>> >> Also the fact that destructors (and free) are save to call with a NULL
>> >> pointer helps. So if any allocation fails you simply call the classes
>> >> destructor and it will do any cleanup necessary. No need to check
>> >> which resource where allocated and which not.
>> >>
>> >> Example:
>> >>
>> >> peer_t *
>> >> peer_new (zframe_t *own_identity, zframe_t *peer_identity, proto_t *proto) {
>> >>     peer_t *self = (peer_t *) zmalloc (sizeof (peer_t));
>> >>     if (self) {
>> >>         // allocate stuff
>> >>         self->own_identity = zframe_strhex(own_identity);
>> >>         if (peer_identity) {
>> >>             self->peer_identity = zframe_strhex(peer_identity);
>> >>         } else {
>> >>             self->peer_identity = NULL;
>> >>         }
>> >>         self->proto = proto;
>> >>         self->timeout_handle =
>> >>             proto_timeout_add (self->proto, MIN_TIMEOUT, self);
>> >>         self->out_msg = zlist_new ();
>> >>         self->in_msg = zlist_new ();
>> >>         self->requests = zhash_new ();
>> >>
>> >>         // did any allocation fail?
>> >>         if (!self->own_identity || (peer_identity && !self->peer_identity) ||
>> >>             !self->timeout_handle || !self->out_msg || !self->in_msg ||
>> >>             !self->requests) {
>> >>             peer_destroy (&self);
>> >>             return NULL;
>> >>         }
>> >>
>> >>         // safe to use allocated stuff (which this example doesn't :)
>> >>         self->last_send_sequence = 0;
>> >>         self->acked_send_sequence = 0;
>> >>         self->max_recv_sequence = 0;
>> >>         self->acked_recv_sequence = 0;
>> >>         self->missed_beats = 0;
>> >>         self->current_timeout = MIN_TIMEOUT;
>> >>         self->peer_state = ACTIVE;
>> >>         self->own_state = MESSAGE;
>> >>         self->need_to_ack = false;
>> >>     }
>> >>     return self;
>> >> }
>> >>
>> >> I would prefer checks to assert and asserts to crash & burn at some
>> >> later time. I am able to deal with out-of-memory for the most part and
>> >> an assert would prevent me from doing that. And if the code doesn't
>> >> lend itself to handling out-of-memory I can always just assert().
>> >> That's my choice then.
>> >>
>> >> MfG
>> >>         Goswin
>> >> _______________________________________________
>> >> zeromq-dev mailing list
>> >> zeromq-dev at lists.zeromq.org
>> >> http://lists.zeromq.org/mailman/listinfo/zeromq-dev
>> _______________________________________________
>> zeromq-dev mailing list
>> zeromq-dev at lists.zeromq.org
>> http://lists.zeromq.org/mailman/listinfo/zeromq-dev
> _______________________________________________
> zeromq-dev mailing list
> zeromq-dev at lists.zeromq.org
> http://lists.zeromq.org/mailman/listinfo/zeromq-dev