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

[Jose E. Marchesi]

> 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"

But you see, that is exactly my point: message IDs must not be
predictable: they don't absolutely need to, and also it is probably a
good idea for them to not be.

The client implementations should not rely on predictable IDs, and the
same can be said about the server implementation (poke itself.)

If we are going to test the MI as a black-box, I don't think we need to
check for the validity of IDs at all.

I have nothing in principle against using different ID "namespaces" for
responses and events, but they complicate the implementation and IMO it
is unnecessary.

>> 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