Encapsulate RPC command dispatch in an array of CRPCCommand's #1103

pull jgarzik wants to merge 1 commits into bitcoin:master from jgarzik:rpcobj2 changing 1 files +89 −84
  1. jgarzik commented at 3:57 AM on April 15, 2012: contributor

    This is an alternate version of pull request #1102, which uses function pointers ("ugly" and less extensible from C++ standpoint, but perhaps cleaner from bitcoin's perspective).

  2. Encapsulate RPC command dispatch in an array of CRPCCommand's 95413cfff0
  3. laanwj commented at 8:00 AM on April 15, 2012: member

    IMO this is less ugly instead of more ugly than #1102. All non-implementation information about RPC commands is conveniently in one place, in table vRpcCommands. You could in principle add the help message as well. And there's no long registration function that has to be kept up to date when new commands added, just a simple one.

    One remark though: can you move the RegisterRPCCommands() call to AppInit2() instead? This make sure that the RPC commands data structure is initialized even if the RPC thread is not started. #1075 makes use of this for executing local "RPC" commands without -server.

  4. sipa commented at 12:03 PM on April 15, 2012: member

    You could have an RPCDIspatcher class with a singleton object, which uses RegisterRPCCommands as constructor.

  5. laanwj commented at 12:09 PM on April 15, 2012: member

    As I understand it we currently simply take both mutexes for all RPC commands? (which is inefficient, but very safe) Did that change with this commit?

  6. sipa commented at 12:11 PM on April 15, 2012: member

    Oh right - I confused safe mode with "requires extra locking". I removed the part of the comment about that.

  7. jgarzik commented at 2:39 PM on April 15, 2012: contributor

    BTW, I am looking for suggestions on how to best encapsulate the parameter parsing in CommandLineRPC(). That is the last area where there is a long "if command==foo, do X" decision table, containing per-RPC-command logic.

    It would be nice to move that parameter parsing into the vRPCCommand table.

  8. laanwj commented at 2:53 PM on April 15, 2012: member

    I'm not entirely sure about that. IMO it would be nice to separate client and server concerns as far as possible, even into separate implementation files.

    • The server logic is a box that accepts JSON and produces JSON data structures. It knows nothing of argument parsing, formatting, just handles commands.
    • The client logic takes a command name and list of (string) arguments, and produces a string message and error code as output. The conversion from argument list to JSON internally is dependent on the command. It can either communicate with the server through the network (as JSON client) or by passing the JSON object in and out in memory (UI console).

    There's some network code and such that exists in limbo between the two.

    (I have nothing against making the "convert argument strings to JSON" data-driven as well, but I'm not convinced it belongs in the same place)

  9. jgarzik commented at 3:44 PM on April 15, 2012: contributor

    RE @laanwj "And there's no long registration function that has to be kept up to date when new commands added, just a simple one"

    Well, the long registration function is replaced with a long table. As the saying goes, "six of one, half-dozen of the other" In both this pull request and #1102, there is one (1) place that contains a master list of RPC commands. In this pull request it is vRPCCommands[], and in #1102 it is RegisterRPCCommands().

    I do lean towards this pull request too, because it is smaller and more compact. However, the continued use of 'rpcfn_type' would probably be ugly to C++ purists :)

  10. gavinandresen commented at 11:53 PM on April 16, 2012: contributor

    ACK. I like this one better.

    I like data-driven programming, so describing argument types in a std::map<string method, std::map<int argNumber, ...something... argType> > or something instead of the current big long "if/else..." appeals to me. But that's such a low priority I probably wouldn't bother.

  11. sipa commented at 8:47 PM on April 18, 2012: member

    I've extended the encapsulation a bit further (while fixing the initialization issue) in #1124.

  12. sipa referenced this in commit 457661f640 on Apr 21, 2012
  13. sipa commented at 11:50 PM on April 21, 2012: member

    Included in #1124

  14. sipa closed this on Apr 21, 2012

  15. coblee referenced this in commit a5fd7c991c on Jul 17, 2012
  16. suprnurd referenced this in commit db3a4afddb on Dec 5, 2017
  17. lateminer referenced this in commit 4e9d8e33de on Jan 22, 2019
  18. lateminer referenced this in commit 8ca5db691b on Dec 25, 2019
  19. Bushstar referenced this in commit c3fcbecea8 on Apr 21, 2020
  20. DrahtBot locked this on Sep 8, 2021

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-20 00:16 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me