Re: [PATCH 1/2] New netfilter target to trigger LED devices

[Adam Nielsen <a.nielsen@shikadi.net>]

>> Firstly, I've declared xt_led_info in both the iptables library and the
>> kernel module.  This probably isn't ideal, but I wanted to avoid having
>> to create another header file, which presumably needs to be installed on
>> the user's system before iptables can be compiled.  Please let me know
>> what the preferred solution to this might be.
> 
> One solution is to have a file <linux/netfilter/xt_LED.h>
> in the kernel tree that is copied to the iptables tree
> at iptables/include/linux/netfilter/xt_LED.h.
> 
> But you just gave me something to consider, so keep it as it is right now.

Let me know what you decide!

> I do not think the help text is needed in code (either iptables
> or kernel). It should be in the iptables manpage, i.e.
> libxt_LED.man.

That's true, it's just that as a user I often go looking in the code when
I can't find any other documentation.  Are those example commands suitable
for inclusion in the manpage?  I think they're worth keeping somewhere.

>> +		if (check_inverse(optarg, &invert, NULL, 0))
>> +			exit_error(PARAMETER_PROBLEM,
>> +				"Unexpected `!' after --led-trigger-id");
> 
> Just remove check_inverse - no more intraposition negation support.
> Inversion is indicated by the "invert" variable already.

Does this mean I can remove those whole three lines?  Or do I still need to
check the value of "invert" and complain if it's specified?

>> +		if (strlen(optarg) > 15)
> 
> Why 15, if struct xt_led_info->name is 26 in size? Oh 'tis confusing!
> Try using
> 	strlen("netfilter-") + strlen(optarg) >= sizeof(led->name) instead.

Ah ok, I didn't realise that the compiler can optimise away the strlen()
call.  In that case I don't have any problem with doing it that way.

>> +			led->delay = atoi(optarg);
> 
> strto(u)l, for great justice :)

No problem.  What's the benefit of using strto(u)l over atoi()?

> iptables-restore does not recognize single-quoted words, you will
> have to use double quotes.
> 
> Or just forbid quote characters altogether?

I was going to forbid quote characters, but then I thought that people
may want to use extended characters (dollar signs perhaps, or other
characters that could be interpreted by the shell) so I thought
quoting the output would be less restrictive.

Maybe I should only allow alphanumerics and basic punctuation?

>> +	.size		= XT_ALIGN(sizeof(struct xt_led_info)),
>> +	.userspacesize	= XT_ALIGN(sizeof(struct xt_led_info)),
> 
> Wow did you actually try this? Since you need...
> 
> 	.userspacesize = offsetof(struct xt_led_info, internal_data)

Ah ok, I found a PDF of yours that explains this properly, I
misinterpreted the purpose of the field (I thought it was comparing
existing valid structures together, rather than comparing a new
structure against an existing one.)  I think this worked for me because
I'm in the habit of removing rules by index (iptables -D INPUT 1)

>> +.BI "--led-delay " "ms"
> 
> I prefer \fB/\fI over .B/.I because they read more like inline HTML,
> but that's me.
> 
> No trailing .PP.

That's fine, I've never used whatever that markup is before (I don't
even know what it's called...!)  I just edited one of the other target
pages.

Thanks again for your feedback.  I'll wait a day or two in case
anyone else has any other comments and then resubmit.

Cheers,
Adam.