Hi guys,
I was trying out some of Serhei's demos and made a few notes
as I went through some of the src/pmwebapi code ... (just the
C code - hopefully others with strong javascript-fu will take
a look over the demos too).
== main.c
- port = 44323
We'll need to register this with IANA when the time comes,
unless this is another of brolley's evil plots to confuse me ;)
Current convention is to define these in impl.h and also to
allow override from the environment (for some reason)
- -H vs -h for host
I guess this is because -h is very commonly used for "help"
nowadays, it is really unfortunate that we went with -h for
remote host. You're beating a new path here ... every other
PCP tool uses -h for host, and -? for help.
- -A vs -a for archive (and also -a for uriprefix)
Same deal - except -A is already taken in pcp convention and
is used by the client tools to mean alignment from start-of-
an-archive (see PCPIntro(1) "interval & alignment" section)
[ these things make me wonder if it is finally time to go for
long options (and keep existing conventions for back-compat,
which we could discourage and gently phase out)? ]
- "timeoutr" -> typo
- The -t option parsing would seem simpler with strtoul? but
seem to have gone for a signed local on purpose, not sure I
follow the rationale there (*shrug*, no big deal of course)
- No need for -n option here (more like pmproxy than pmcd here)
- Could/should enable threading now? Noticed this...
/* Start microhttpd daemon. Use the application-driven threading
model, so we don't complicate things with threads. In the
future, if PMAPI becomes safe to invoke from a multithreaded
application, consider MHD_USE_THREAD_PER_CONNECTION, with
ample locking over the contexts etc. */
... the future has arrived! (where's my hoverboard!?!)
- also: /* | MHD_USE_IPv6 */ - should that be enabled too now?
microhttpd.h header file suggests "yes!"
- shutting down comments:
/* Shut down cleanly, out of a misplaced sense of propriety. */
...
/* We could pmDestroyContext() all the active contexts. */
I understand the malloc->free is a waste of time when shutting
down (although valgrind reporting will be helped so I personally
prefer it for daemons always), but I'm also led to believe that
at a TCP protocol level its nicer to shutdown your connections
cleanly (which DestroyContext will do) so the remote host(s) all
explicitly know they won't have to hang around waiting.
- MHD_USE_SSL flag - ho-hum, here's a possible can of worms:
I notice MHD uses gnutls for its SSL services. This means we'll
need different / extra steps for configuring this daemon to the
NSS-based setup we have for pmcd and pmproxy, I think. gnutls
also isn't FIPS certified, might be problematic for some. Really
have no idea how best to tackle this ... we're going to need to
support https though, for sure.
- naturally, none of Daves funky new internal-networking-API stuff
is in use here. what to do? - leave libmicrohttpd needing its own
certificate configuration, etc to other pcp daemons (inconsistent,
more docs, no fips, more work for users, etc) or somehow try to
tackle this (downside - much more work for us devs, probably we
would have to fork libmicrohttpd?)
- this leads into "should we both embed this server code and have
it separately linked" - why did we do that again? (can't recall
but I may have suggested it way back? am now thinking perhaps
we should just go with the configure-found one, then wont be so
tempted to worry about NSS vs gnutls issues :) nor the #ifdef's
riddled throughout the code). Really, I don't know how best to
tackle this whole area, hoping someone else has some good ideas!
- mhd_respond() has a grammatical typo in the preceding comment
("operation an existing context"). Can be a static function,
as can handle_signals() below it.
== pmwebapi.c
Just general comments here, above was probably too detailed:
- /* if-threaded: pthread_mutex_t context_lock; */
ah yeah, so not just as simple as asserting libmicrophttpd and
libpcp do threading - we need to do our own too in some spots.
- pmwebapi_respond_*
These tend to be of the form:
1. extract details of the web request & its parameters
2. do the pcp api calls for request/parameters and walk the
results building up a char* buffer (JSON)
3. direct the char* buffer back through the webserver code
That's a bit of an oversimplification, there's also session
management stuff in there of course, but not much else I can
see. (?)
Step 2 seems to be amenable to being extracted into a library
that could be shared. I think this would encapsulate the sort
of thing that would be needed by all webapps/containers that
people might want to directly export PCP data (Apache modules,
and java/python/... web server wrappers like flask.pocoo.org,
cherrypy.org, etc would need). These tend to have their own
session management capabilities too sometimes, so perhaps if
the session code in pmwebapi already could be abstracted too
(probably within the same library for optional use) we should
be in a happy place.
Further, if someone desperately wants an XML output instead of
JSON, this abstraction could allow the library to provide an
alternate set of interfaces for XML that the same daemon could
serve up instead.
These APIs/library might be another angle for us to be able to
tackle the SSL/certificates/NSS issue from earlier - there is a
mod_nss Apache module, perhaps a mod_pmwebapi might be able to
interoperate with that (/me waves hands wildly) somehow to tackle
that.
I have other thoughts about how some of the requests seem to make
quite a few pmapi calls (& synchronous roundtrips to pmcd - which
might be remote), but I think that could be deferred to a later
time. Also some calls looked like they could do with caching of
the pmDesc structures, again defer that I think. Streaming of
results back to the client (web sockets/HTML5?) is something else
that might be needed later - defer until its a problem IMO; we
have a good starting point here & those things can come later.
HTH. A great effort all round guys!
Oh wait - one final thought - for the daemon, the name "pmwebapi"
doesn't seem to roll off the tongue - how would you feel about a
"pmwebd" and perhaps a libpcp_webapi.so / <pcp/webapi.h>?
cheers.
--
Nathan
|