Re: [PATCH 04/16 v2] Determine supported extended-remote features

[Pedro Alves <palves at redhat dot com>]

On 10/22/2014 12:23 AM, Breazeal, Don wrote:

> Hi Pedro,
> Thanks for looking at this.  I should have made the rationale for
> the new qExtendedFeatures packet more clear in my original
> submission.  I'll try to answer all of your questions in the
> explanation below.
> 
> I had started out trying to report the features as
> supported/unsupported using qSupported, following the
> implementation of PACKET_multiprocess_feature as a model.
> I abandoned that approach when I saw these comments in
> remote.c_initialize_remote:
> 
>         /* Ideally all configs would have a command associated.  Some
>            still don't though.  */
>           [---snip---]
>           case PACKET_multiprocess_feature:
>           [---snip---]
>             /* Additions to this list need to be well justified:
>                pre-existing packets are OK; new packets are not.  */
> 
> I interpreted this to mean that the qSupported query was intended
> to be used only to enable/disable packets that mapped to commands,
> and that defining new packets that represented something else was
> discouraged.

Oh, you're reading this backwards --- this block is making sure that
we don't forget to call add_packet_config_cmd whenever we add a
new feature/packet.  The commands that is talking about are the
"set remote foo-packet" commands that add_packet_config_cmd registers.
Those are commands that allow force- enable/disabling a given
RSP packet/feature.  That loop is allowing some exceptions for
some packets/features that already didn't have the corresponding
"set remote foo-packet" commands when that assertion was
first added:

  /* Assert that we've registered commands for all packet configs.  */
  {
    int i;

    for (i = 0; i < PACKET_MAX; i++)
      {
	/* Ideally all configs would have a command associated.  Some
	   still don't though.  */
	int excepted;


See the log of ca4f7f8be, the commit that added this.

We can certainly add new qSupported features that don't map
to regular user commands.

Guess this would make it clearer:

-  /* Assert that we've registered commands for all packet configs.  */
+  /* Assert that we've registered "set remote foo-packet" commands for all packet configs.  */

> In refreshing my memory about this I wrote up some detailed notes on
> it.  Let me know if you want to see the gruesome details.
> 
> Do you think the qSupported approach is preferable despite the
> comment?  Did I misinterpret it?

Yes, qSupported is preferable.  I'm sorry about not foreseeing
this potential confusion when I wrote that comment.

Thanks,
Pedro Alves