Some bro.bif issues

Development
1

This is the function set_buf:

    function set_buf%(f: file, buffered: bool%): any
        %{
        f->SetBuf(buffered);
        return new Val(0, TYPE_VOID);
        %}

It's return value is any although void is returned. Shouldn't the return value
in the function signature be void as well?

Moreover, here is the function exit:

    function exit%(%): int
        %{
        exit(0);
        return 0;
        %}

Shouldn't it return 'void' at the scripting layer since it shuts down
the Bro process immediately? Why not let users control the return code
like this:

    function exit%(code: int): void
        %{
        exit(code);
        %}

There seems to be another redundancy:

    function active_connection%(id: conn_id%): bool
        %{
        Connection* c = sessions->FindConnection(id);
        return new Val(c ? 1 : 0, TYPE_BOOL);
        %}

and

    function connection_exists%(c: conn_id%): bool
        %{
        if ( sessions->FindConnection(c) )
            return new Val(1, TYPE_BOOL);
        else
            return new Val(0, TYPE_BOOL);
        %}

are essentially the same. Only the latter is used in the current set of
scripts. Should we get rid of the former?

The same sort of redundancy exists for:

    function connection_record%(cid: conn_id%): connection
        %{
        Connection* c = sessions->FindConnection(cid);
        if ( c )
            return c->BuildConnVal();
        else
            {
            // Hard to recover from this until we have union types ...
            builtin_error("connection ID not a known connection (fatal)", cid);
            exit(0);
            return 0;
            }
        %}

and

    function lookup_connection%(cid: conn_id%): connection
        %{
        Connection* conn = sessions->FindConnection(cid);
        if ( conn )
            return conn->BuildConnVal();

        builtin_error("connection ID not a known connection", cid);

        // Return a dummy connection record.
        RecordVal* c = new RecordVal(connection_type);

        RecordVal* id_val = new RecordVal(conn_id);
        id_val->Assign(0, new AddrVal((unsigned int) 0));
        id_val->Assign(1, new PortVal(ntohs(0), TRANSPORT_UDP));
        id_val->Assign(2, new AddrVal((unsigned int) 0));
        id_val->Assign(3, new PortVal(ntohs(0), TRANSPORT_UDP));
        c->Assign(0, id_val);

        RecordVal* orig_endp = new RecordVal(endpoint);
        orig_endp->Assign(0, new Val(0, TYPE_COUNT));
        orig_endp->Assign(1, new Val(int(0), TYPE_COUNT));

        RecordVal* resp_endp = new RecordVal(endpoint);
        resp_endp->Assign(0, new Val(0, TYPE_COUNT));
        resp_endp->Assign(1, new Val(int(0), TYPE_COUNT));

        c->Assign(1, orig_endp);
        c->Assign(2, resp_endp);

        c->Assign(3, new Val(network_time, TYPE_TIME));
        c->Assign(4, new Val(0.0, TYPE_INTERVAL));
        c->Assign(5, new TableVal(string_set)); // service
        c->Assign(6, new StringVal("")); // addl
        c->Assign(7, new Val(0, TYPE_COUNT)); // hot
        c->Assign(8, new StringVal("")); // history

        return c;
        %}

except that the latter returns a dummy connection value. Neither function is
currently used at script land. My vote would be to remove the former one, as
the latter has more graceful semantics.

    Matthias

2

This is the function set_buf:

     function set_buf%(f: file, buffered: bool%): any
         %{
         f->SetBuf(buffered);
         return new Val(0, TYPE_VOID);
         %}

It's return value is any although void is returned. Shouldn't the return value
in the function signature be void as well?

Except for this one, all BiF's that don't have a return value are declared as returning "any" and will unconditionally do a "return 0"...

AFAIK BiF's currently can't return void. Although I think that's just (an easily fixed) limitation of the BiF-parser.
So I guess that either this BiF should be changed to return 0 to be consistent with other BiFs. Or the BiF-parser should be changed to allow void as return value and the BiF's updated accordingly. Might want to schedule this for 2.1 or 2.2....

Moreover, here is the function exit:

     function exit%(%): int
         %{
         exit(0);
         return 0;
         %}

Per the above. Currently it should return an "any" to be consistent with other code.

(Search for "return 0" in any BiF file and you'll find plenty of functions that should return void but do in fact return "any")

     function active_connection%(id: conn_id%): bool

VS.

     function connection_exists%(c: conn_id%): bool

>

are essentially the same. Only the latter is used in the current set of
scripts. Should we get rid of the former?

sounds good. The latter is also the more intuitive name....

The same sort of redundancy exists for:

     function connection_record%(cid: conn_id%): connection
     function lookup_connection%(cid: conn_id%): connection

except that the latter returns a dummy connection value. Neither function is
currently used at script land. My vote would be to remove the former one, as
the latter has more graceful semantics.

ACK.
We for sure shouldn't call exit() when the connection doesn't exist...

cu
Gregor

3

Do others have any objections? Otherwise I would go ahead an remove
them.

    Matthias

4

Cleaning these things up makes sense. Just commit these all to the
cleanup branch you created, and we can merge that in when 2.1 devel
starts.

Robin

5

All of this is welcome change! The duplication and inconsistency with some of these functions has driven me crazy for a long time.

Thanks,
  .Seth