[PATCH] libxtables: Add exit_error cb to xtables_globals

[PATCH] libxtables: Add exit_error cb to xtables_globals

[jamal]

[-- Attachment #1: Type: text/plain, Size: 78 bytes --]

Suggestion from Jan. Building up to compiling most basic app..

cheers,
jamal

[-- Attachment #2: iptv2-2 --]
[-- Type: text/plain, Size: 2073 bytes --]

commit 2960e6aa4e7d9429aa81eff9636ad3524c9282e2
Author: Jamal Hadi Salim <hadi@cyberus.ca>
Date:   Mon Feb 9 16:46:18 2009 -0500

    Introduce exit_error() as part of xtables_globals structure.
    When an application registers its xtables_globals definition
    and does not specify its exit_error() it gets assigned a
    basic version
    
    Signed-off-by: Jamal Hadi Salim <hadi@cyberus.ca>

diff --git a/include/xtables.h.in b/include/xtables.h.in
index 1d33394..3a16651 100644
--- a/include/xtables.h.in
+++ b/include/xtables.h.in
@@ -33,14 +33,6 @@
 
 struct in_addr;
 
-struct xtables_globals
-{
-	unsigned int option_offset;
-	char *program_version;
-	char *program_name;
-	struct option *opts;
-};
-
 /* Include file for additions: new matches and targets. */
 struct xtables_match
 {
@@ -191,6 +183,15 @@ enum xtables_exittype {
 	XTF_ONE_ACTION,
 };
 
+struct xtables_globals
+{
+	unsigned int option_offset;
+	char *program_version;
+	char *program_name;
+	struct option *opts;
+	void (*exit_error)(enum xtables_exittype status, const char *msg, ...);
+};
+
 extern const char *xtables_program_name;
 extern const char *xtables_modprobe_program;
 extern struct xtables_match *xtables_matches;
diff --git a/xtables.c b/xtables.c
index 95be5f8..d0fc478 100644
--- a/xtables.c
+++ b/xtables.c
@@ -46,7 +46,20 @@
 #define PROC_SYS_MODPROBE "/proc/sys/kernel/modprobe"
 #endif
 
-struct xtables_globals *xt_params;
+struct xtables_globals *xt_params = NULL;
+
+void basic_exit_error(enum xtables_exittype status, const char *msg, ...)
+{
+	va_list args;
+
+	va_start(args, msg);
+	fprintf(stderr, "%s v%s: ", xt_params->program_name, xt_params->program_version);
+	vfprintf(stderr, msg, args);
+	va_end(args);
+	fprintf(stderr, "\n");
+	exit(status);
+}
+
 /**
  * xtables_set_params - set the global parameters used by xtables
  * @xtp:	input xtables_globals structure
@@ -65,6 +78,10 @@ int xtables_set_params(struct xtables_globals *xtp)
 	}
 
 	xt_params = xtp;
+
+	if (!xt_params->exit_error)
+		xt_params->exit_error = basic_exit_error;
+
 	return 0;
 }
 

Re: [PATCH] libxtables: Add exit_error cb to xtables_globals

[Jan Engelhardt]

On Tuesday 2009-02-10 04:29, jamal wrote:

>Suggestion from Jan. Building up to compiling most basic app..
>

I looked through this and wondered - how do we want to model
the ownership of the structure?

For example, one possibility (which you did) is that
xtables_globals is defined in the program, and libxtables
takes a pointer/reference to it during xtables_set_params.

Alternatively, libxtables could collect the globals in
a struct (defined in xtables.c), and programs override
it at will, either by setting variables directly,
like it is currently done with xtables_program_name,
or by use of a function;

Any preference on which approach to choose?

Re: [PATCH] libxtables: Add exit_error cb to xtables_globals

[jamal]

On Tue, 2009-02-10 at 17:20 +0100, Jan Engelhardt wrote:

> 
> I looked through this and wondered - how do we want to model
> the ownership of the structure?
> 
> For example, one possibility (which you did) is that
> xtables_globals is defined in the program, and libxtables
> takes a pointer/reference to it during xtables_set_params.
> 
> Alternatively, libxtables could collect the globals in
> a struct (defined in xtables.c), and programs override
> it at will, either by setting variables directly,
> like it is currently done with xtables_program_name,
> or by use of a function;
> 
> Any preference on which approach to choose?

I think you could go either way.

>From a personal taste perspective (and maybe usability as
defined by someone who is lazy like myself), I would pick the 
option that makes me write the smallest amount of code - which 
is what the current patches provide.
[I dislike libcurl for example which makes me call all
sorts of set() functions].

I would even wanna go further:
unify xtables_init(), xtables_set_nfproto() and 
xtables_set_params() i.e just have the app call
xtables_init(&myxtables_params). If the app only declares
myxtables_params and sets nothing in it, then xtables_init()
provides it with the default parameters (sort of what i do with
basic_exit_error)


cheers,
jamal

Re: [PATCH] libxtables: Add exit_error cb to xtables_globals

[Patrick McHardy]

jamal wrote:
> Suggestion from Jan. Building up to compiling most basic app..

Applied, thanks.

Re: [PATCH] libxtables: Add exit_error cb to xtables_globals

[Jan Engelhardt]

On Tuesday 2009-02-10 04:29, jamal wrote:

>-struct xtables_globals
>-{
>-	unsigned int option_offset;
>-	char *program_version;
>-	char *program_name;
>-	struct option *opts;
>-};
>-
> /* Include file for additions: new matches and targets. */
> struct xtables_match
> {
>@@ -191,6 +183,15 @@ enum xtables_exittype {
> 	XTF_ONE_ACTION,
> };
> 
>+struct xtables_globals
>+{
>+	unsigned int option_offset;
>+	char *program_version;
>+	char *program_name;
>+	struct option *opts;
>+	void (*exit_error)(enum xtables_exittype status, const char *msg, ...);
>+};
>+

Why move the struct now.. makes it harder to figure out what changed.
So right, exit_error.

> #define PROC_SYS_MODPROBE "/proc/sys/kernel/modprobe"
> #endif
> 
>-struct xtables_globals *xt_params;
>+struct xtables_globals *xt_params = NULL;

Do not initialize static members - this takes up extra space
and adds no benefit. (zeroed anyway even in .bss)

>+void basic_exit_error(enum xtables_exittype status, const char *msg, ...)
>+{
>+	va_list args;
>+
>+	va_start(args, msg);
>+	fprintf(stderr, "%s v%s: ", xt_params->program_name, xt_params->program_version);
>+	vfprintf(stderr, msg, args);
>+	va_end(args);
>+	fprintf(stderr, "\n");
>+	exit(status);
>+}
>+
> /**
>  * xtables_set_params - set the global parameters used by xtables
>  * @xtp:	input xtables_globals structure
>@@ -65,6 +78,10 @@ int xtables_set_params(struct xtables_globals *xtp)
> 	}
> 
> 	xt_params = xtp;
>+
>+	if (!xt_params->exit_error)
>+		xt_params->exit_error = basic_exit_error;
>+
> 	return 0;
> }
> 

When is basic_exit_error invoked? In this patch alone, the users
are missing (or the #define).

Re: [PATCH] libxtables: Add exit_error cb to xtables_globals

[jamal]

On Wed, 2009-02-11 at 16:19 +0100, Jan Engelhardt wrote:
> On Tuesday 2009-02-10 04:29, jamal wrote:

> > 
> >+struct xtables_globals
> >+{
> >+	unsigned int option_offset;
> >+	char *program_version;
> >+	char *program_name;
> >+	struct option *opts;
> >+	void (*exit_error)(enum xtables_exittype status, const char *msg, ...);
> >+};
> >+
> 
> Why move the struct now.. makes it harder to figure out what changed.
> So right, exit_error.

Right (because of xtables_exittype)

> > #define PROC_SYS_MODPROBE "/proc/sys/kernel/modprobe"
> > #endif
> > 
> >-struct xtables_globals *xt_params;
> >+struct xtables_globals *xt_params = NULL;
> 
> Do not initialize static members - this takes up extra space
> and adds no benefit. (zeroed anyway even in .bss)

Ok. Do i need to regenerate the patch or send on top?


> When is basic_exit_error invoked? In this patch alone, the users
> are missing (or the #define).

tc/m_xt.c was at this point - but not any of the native iptables apps.
Later on patches iptables-xml uses the basic_exit_error() as well

cheers,
jamal

Re: [PATCH] libxtables: Add exit_error cb to xtables_globals

[Jan Engelhardt]

On Wednesday 2009-02-11 18:13, jamal wrote:
>> 
>> Why move the struct now.. makes it harder to figure out what changed.
>> So right, exit_error.
>
>Right (because of xtables_exittype)

Ok, makes sense. (I guess git got in a dilemma and did not figure
out the enum was moved ;-)

>> > #define PROC_SYS_MODPROBE "/proc/sys/kernel/modprobe"
>> > #endif
>> > 
>> >-struct xtables_globals *xt_params;
>> >+struct xtables_globals *xt_params = NULL;
>> 
>> Do not initialize static members - this takes up extra space
>> and adds no benefit. (zeroed anyway even in .bss)
>
>Ok. Do i need to regenerate the patch or send on top?

As long as it's not applied, regen seems best.

>> When is basic_exit_error invoked? In this patch alone, the users
>> are missing (or the #define).
>
>tc/m_xt.c was at this point - but not any of the native iptables apps.
>Later on patches iptables-xml uses the basic_exit_error() as well

Ok.