Hi, Nathan -
> == main.c
>
> - port = 44323
> We'll need to register this with IANA when the time comes [...]
Right.
> Current convention is to define these in impl.h and also to
> allow override from the environment (for some reason)
This is not applicable to the web interface. Those don't use impl.h,
nor environment variables to connect.
> - -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)
> [...]
I'll switch over to the convention, such as it is.
> - "timeoutr" -> typo
Will fix.
> - 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)
strtoul accepts values like -444 and treats them as valid (large)
numbers. Not helpful to a user.
> - No need for -n option here (more like pmproxy than pmcd here)
Will zap.
> - 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!"
... nope (as you have found later).
> - 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),
I'm ambivalent about this one, but OK.
> 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.
If you're referring to half-closed sorts of situations, that should
not apply here, as when the process exits, the kernel will thoroughly
close the socket immediately.
> - MHD_USE_SSL flag - ho-hum, here's a possible can of worms:
> I notice MHD uses gnutls for its SSL services. [...]
I propose not enabling the gnutls SSL stuff in libmicrohttpd, until at
some point we undertake a port to nss directly (which we could send
upstream), or through the recently-added posix-socket-io layer in
libpcp (which we could not upstream).
> - 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.
Will fix.
> == 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.
Indeed.
> - 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
> [...]
> Step 2 seems to be amenable to being extracted into a library
> that could be shared. [...]
I must admit I don't see that much potential in this. How about we
leave refactoring till there is more than one ... product?
> 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.
(If that were desirable, a "&format=xml" type suffix on the incoming
URLs could be handled directly within the program, without requiring
much abstraction goo.)
> [...] 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>?
How about just rename to pmwebd, but not bother with libs/headers at
this time?
- FChE
|