Re: [PATCH 2/2] MI: Add error reporting functionality for invalid msgs

[Jose E. Marchesi]

Hi Mohammad.

>  static void
>  pk_mi_dispatch_msg (pk_mi_msg msg)
>  {
> -  if (pk_mi_msg_kind (msg) == PK_MI_MSG_REQUEST)
> +  const char* errmsg = "";
> +
> +  if (pk_mi_msg_kind (msg) != PK_MI_MSG_REQUEST) {
> +    errmsg = "expects request message";
> +    goto error;
> +  }

GNU coding style please.

> +  switch (pk_mi_msg_req_type (msg))
>      {
> -      switch (pk_mi_msg_req_type (msg))
> -        {
> -        case PK_MI_REQ_EXIT:
> -          {
> -            pk_mi_msg resp
> -              = pk_mi_make_resp (PK_MI_RESP_EXIT,
> -                                 pk_mi_msg_number (msg),
> -                                 1 /* success_p */,
> -                                 NULL /* errmsg */);
> -            pk_mi_send (resp);
> -            pk_mi_msg_free (resp);
> -            pk_mi_exit_p = 1;
> -            break;
> -          }
> -        case PK_MI_REQ_PRINTV:
> -          {
> -            int ok;
> -            pk_mi_msg resp;
> -            pk_val arg, val;
> -
> -            arg = pk_mi_get_arg (msg, "value");
> -
> -            ok = pk_defvar (poke_compiler, "__pkl_mi_value", arg);
> -            assert (ok == PK_OK);
> -
> -            ok = pk_compile_expression (poke_compiler,
> -                                        "format (\"%v\", __pkl_mi_value)",
> -                                        NULL, &val);
> -            assert (ok == PK_OK);
> -
> -            resp = pk_mi_make_resp (PK_MI_RESP_PRINTV, pk_mi_msg_number 
> (msg),
> -                                    1 /* success_p */, NULL /* errmsg */);
> -            pk_mi_set_arg (resp, "string", val);
> -            pk_mi_send (resp);
> -            pk_mi_msg_free (resp);
> -            break;
> -          }
> -        default:
> -          assert (0);
> -        }
> +    case PK_MI_REQ_EXIT:
> +      {
> +        pk_mi_msg resp
> +          = pk_mi_make_resp (PK_MI_RESP_EXIT,
> +                             pk_mi_msg_number (msg),
> +                             1 /* success_p */,
> +                             NULL /* errmsg */);
> +        pk_mi_send (resp);
> +        pk_mi_msg_free (resp);
> +        pk_mi_exit_p = 1;
> +        break;
> +      }
> +    case PK_MI_REQ_PRINTV:
> +      {
> +        int ok;
> +        pk_mi_msg resp;
> +        pk_val arg, val;
> +
> +        arg = pk_mi_get_arg (msg, "value");
> +
> +        ok = pk_defvar (poke_compiler, "__pkl_mi_value", arg);
> +        assert (ok == PK_OK);
> +
> +        ok = pk_compile_expression (poke_compiler,
> +                                    "format (\"%v\", __pkl_mi_value)",
> +                                    NULL, &val);
> +        assert (ok == PK_OK);
> +
> +        resp = pk_mi_make_resp (PK_MI_RESP_PRINTV, pk_mi_msg_number (msg),
> +                                1 /* success_p */, NULL /* errmsg */);
> +        pk_mi_set_arg (resp, "string", val);
> +        pk_mi_send (resp);
> +        pk_mi_msg_free (resp);
> +        break;
> +      }
> +    default:
> +      errmsg = "invalid request type";
> +      goto error;
>      }
> -  else
> -    {
> -      pk_mi_msg invreq_msg = pk_mi_make_event (PK_MI_EVENT_INVREQ);
>  
> -      pk_mi_set_arg (invreq_msg, "reqnum",
> -                     pk_make_uint (pk_mi_msg_number (msg), 64));
> -      pk_mi_send (invreq_msg);
> -      pk_mi_msg_free (invreq_msg);
> -    }
> +  goto done;
> +
> +error:
> +  pk_mi_invalid_msg (msg, errmsg);

I would call that function with a more explicit pk_mi_send_invalid_msg.

Other than that, this series is OK for both master and maint/poke-1.
Thanks!