[collectd] [PATCH 2/5] sigrok: bare bones plugin

[Bert Vermeulen]

I've reworked the patch to take comments into account, and resubmitted it as
a pull request on github. I'll reply to the comments raised below:

On 07/22/2013 09:35 PM, Dan Fandrich wrote:
> On Mon, Jul 22, 2013 at 06:21:18PM +0200, Bert Vermeulen wrote:
>> +static int plugin_config(const char *key, const char *value)
>> +{
>> +
>> +	if (!strcmp(key, "loglevel"))
>> +		loglevel = atoi(value);
>> +	else if (!strcmp(key, "conn"))
>> +		conn = strdup(value);
> 
> This will leak memory if the config file defines these elements more than once.

Replaced with cf_util_get_string() and _int().

> Personally, I don't see the need to split this patch up into several pieces. A
> skeleton by itself isn't very useful to review.

It's a habit I have which has come in useful in the sigrok project. If
people ask how to do some particular thing, I like to point them at a commit
which does exactly that for another driver/module/thing. It didn't work out
well here, judging from the comments on code that was reverted in a later
patch :-)

On 07/22/2013 09:50 PM, Florian Forster wrote:
> Unfortunately, this is going to cause licensing issues: Many parts of
> collectd are licenses under the GPL version 2, which is not compatible
> to GPL version 3.

I've relicensed the patch as GPLv2+.

>> +static int cd_logger(void *cb_data, int loglevel, const char *format, va_list args)
> 
> Macros are better suited for this kind of prefix implementation, because
> you don't need a separate buffer / call to vsnprintf(). E.g.:
> 
>   #define SR_INFO(...) INFO ("sigrok plugin: " __VA_ARGS__)
> 
>> +	if (!strcmp(key, "loglevel"))
>> +		loglevel = atoi(value);
> 
> Please use parse_log_severity() from src/plugin.h to parse a string into
> a log severity.

That's not going to work, because this refers to the libsigrok loglevel, not
collectd's loglevel. They're not the same, of course.

That also means the function can't be replaced by a macro: it needs to check
the configured libsigrok loglevel, and log to collectd or not accordingly.

> Please use cf_util_get_string() from src/configfile.h for string 
> arguments.
> 
> Please use cf_util_get_cdtime() from src/configfile.h for this.
> 
> Please use cdtime_t from src/utils_time.h; you can simply do a
> 
> cdtime_t elapsed = now - previous;
> 
> with that time representation.

Done.

>> +	if (packet->type == SR_DF_END) {
>> +		/* TODO: try to restart acquisition after a delay? */
>> +		INFO("oops! ended");
>
> There's a specialized logginf function in the code. Why not use it here?

Are you referring to cd_logger()? That's a libsigrok log catcher, i.e. a
callback registered with libsigrok. If that's not what you meant, please
clarify.

> The "plugin_" prefix is reserved for functions defined in src/plugin.h.
> Maybe you can use "csr_" or something like that instead?

Done.


-- 
Bert Vermeulen        bert at biot.com          email/xmpp