Re: [PATCH v2] MI: Replace sequence number with ID

[Mohammad-Reza Nabipoor]

Hi, Jose

On Tue, Nov 09, 2021 at 06:32:37PM +0100, Jose E. Marchesi wrote:
> 
> Hi Mohammad.
> 
> >> 2021-08-19  Mohammad-Reza Nabipoor  <mnabipoor@gnu.org>
> >> 
> >>    * poke/pk-mi-msg.h (pk_mi_seqnum): Replaced by `pk_mi_id`.
> >>    (pk_mi_id): New typedef.
> >>    (pk_mi_make_req): Add new arg (`id`).
> >>    (pk_mi_make_event): Likewise.
> >>    (pk_mi_make_resp): Likewise.
> >>    (pk_mi_msg_number): Replaced by `pk_mi_msg_id`.
> >>    (pk_mi_set_msg_number): Replaced by `pk_mi_set_msg_id`.
> >>    (pk_mi_msg_resp_req_number): Replaced by `pk_mi_msg_resp_req_id`.
> >>    (pk_mi_msg_id): New function declaration.
> >>    (pk_mi_make_resp_auto): Likewise.
> >>    (pk_mi_make_event_auto): Likewise.
> >>    (pk_mi_set_msg_id): Likewise.
> >>    (pk_mi_msg_resp_req_id): Likewise.
> 
> Please excuse my stubborness, ...


"stubbornness" is a good feature for a maintainer; I understand :)
I'm sorry that I didn't communicate my intent clearly.


> ... but I still fail to see why we need to use
> different ID namespaces for responses and events.
> 


The answer for both questions:
  "to make testing easier by having perdictable IDs for messages"


> In the MI protocol we have requests, responses and events.
> 
> Requests are inititiated by clients.  It is the client who has to
> maintain the ID namespace for requests, with just one requirement: these
> IDs must be significant enough for the client to clearly handle a
> reference to a given request.  poke itself must be oblivious of how
> these IDs are created/organized.
> 
> Responses and events are created by poke.  The same requirements for
> these IDs apply: they must be meaningful enough for poke to distinguish
> between them, and also the client must be able to be oblivious on how
> they are generated/organized.
> 
> So, why do we need a different namespace for responses and event IDs?
> 


You're 100% right. But I thought introducing a separate ID for events can
make the prediction of next ID straightforward in the test harness (esp. when
we have events interleaved with chunked responses).

The way currently test harness works is this like: it synthesizes the expected
response in JSON, and compare it with the incoming message. So if ID behaves
predictably, it's just a simple ==-like comparison.
But if you think this is overkill, I can live it by ignoring the ID completely
in tests (by assigning zero to ID of both messages).

This is a minor improvement, but I thought it is justifiable to consider events
and responses as distinct channels of information.


> Why do we need to expand pk_mi_make_WHAT functions to get an explicit
> ID?
> 

The problem with implicit ID was that to create a new `pk_msg` from JSON,
first we should call `pk_mi_make_WHAT`, and then setting the ID to our desired
value. Calling `pk_mi_make_WHAT` has a side-effect: increase of global variable
`next_id`. Which makes IDs totaly unpredictable from client point of view.

So I introduced the following functions:

  pk_mi_msg pk_mi_make_req (enum pk_mi_req_type type, pk_mi_id id);
  pk_mi_msg pk_mi_make_event (enum pk_mi_event_type type, pk_mi_id id);
  pk_mi_msg pk_mi_make_event_auto (enum pk_mi_event_type type);
  pk_mi_msg pk_mi_make_resp (enum pk_mi_resp_type type, pk_mi_id id,
                             pk_mi_id req_id, int success_p, const char 
*errmsg);
  pk_mi_msg pk_mi_make_resp_auto (enum pk_mi_resp_type type, pk_mi_id req_id,
                                  int success_p, const char *errmsg);

The `*_auto` family has side-effect.


Regards,
Mohammad-Reza