Skip to contentSkip to conversation

Export sepol handle

PATCH In review Joshua Brindle 2 participants 3 messages Jul 25, 2006 SELinux security module

lore
Share & tools
lore

Joshua Brindle

author
#0 lore raw

PATCH 1/1 Export sepol handle

Attached is a patch to export the error reporting handle in libsepol. The setools team needs this so that we can create, destroy, set the callback, and invoke the error reporting from outside libsepol. This will allow us to better propagate the errors into our analysis libraries.

libsepol/include/sepol/handle.h +13 -2
diff -urpN -x 'Change*' -x entries -x '*.orig' -x '*.rej' -x '*.svn*' -x '*.swp' trunk/libsepol/include/sepol/handle.h branch/setools_public-policydb-components/libsepol/include/sepol/handle.h
@@ -3,11 +3,22 @@
 
 struct sepol_handle;
 typedef struct sepol_handle sepol_handle_t;
+typedef void (*sepol_handle_callback_fn_t) (void* varg, sepol_handle_t* handle, const char* fmt, ...);
 
 /* Create and return a sepol handle. */
-sepol_handle_t *sepol_handle_create(void);
+extern sepol_handle_t *sepol_handle_create(void);
+
+/* Set the callback for a sepol handle */
+extern void sepol_handle_set_callback(sepol_handle_t *h, sepol_handle_callback_fn_t fn, void *varg);
+
+/* Call the callback for a sepol handle */
+extern void sepol_handle_do_callback(sepol_handle_t* handle, const char* fmt, ...);
+
+/* Get the severity level of the last message */
+extern int sepol_handle_get_level(sepol_handle_t *h);
 
 /* Destroy a sepol handle. */
-void sepol_handle_destroy(sepol_handle_t *);
+extern void sepol_handle_destroy(sepol_handle_t *);
+
 
 #endif
libsepol/src/handle.c +38 -1
diff -urpN -x 'Change*' -x entries -x '*.orig' -x '*.rej' -x '*.svn*' -x '*.swp' trunk/libsepol/src/handle.c branch/setools_public-policydb-components/libsepol/src/handle.c
@@ -1,4 +1,6 @@
 #include <stdlib.h>
+#include <stdarg.h>
+#include <errno.h>
 #include "handle.h"
 #include "debug.h"
 

@@ -16,7 +18,42 @@ sepol_handle_t *sepol_handle_create(void
 	return sh;
 }
 
-void sepol_handle_destroy(sepol_handle_t * sh)
+void sepol_handle_set_callback(sepol_handle_t *h, sepol_handle_callback_fn_t fn, void *varg)
 {
+	if (h == NULL)
+		return;
+
+	h->msg_callback = fn;
+	h->msg_callback_arg = varg;
+}
+
+void sepol_handle_do_callback(sepol_handle_t* handle, const char* fmt, ...)
+{
+	char *tmp = NULL;
+	va_list ap;
+
+	if (handle == NULL)
+		return;
+
+	va_start(ap, fmt);
+	vasprintf(&tmp, fmt, ap);
+	va_end(ap);
+
+	ERR(handle, "%s", tmp);
+	free(tmp);
+}
+
+int sepol_handle_get_level(sepol_handle_t *h)
+{
+	if (h == NULL) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	return h->msg_level;
+}
+
+void sepol_handle_destroy(sepol_handle_t * sh) {
 	free(sh);
 }
+
libsepol/src/handle.h +1 -2
diff -urpN -x 'Change*' -x entries -x '*.orig' -x '*.rej' -x '*.svn*' -x '*.swp' trunk/libsepol/src/handle.h branch/setools_public-policydb-components/libsepol/src/handle.h
@@ -11,8 +11,7 @@ struct sepol_handle {
 #ifdef __GNUC__
 	__attribute__ ((format(printf, 3, 4)))
 #endif
-	void (*msg_callback) (void *varg,
-			      sepol_handle_t * handle, const char *fmt, ...);
+	sepol_handle_callback_fn_t msg_callback;
 	void *msg_callback_arg;
 };

--
This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.

#1 lore raw

On Tue, 2006-07-25 at 10:54 -0400, Joshua Brindle wrote:

<snip>

Quoted 4 lines

struct sepol_handle;
typedef struct sepol_handle sepol_handle_t;
+typedef void (*sepol_handle_callback_fn_t) (void* varg, sepol_handle_t* handle, const char* fmt, ...);

I'm confused about the first argument - it is just a blob of data passed to the callback, correct? Why is it called varg (which implies variable argument list to me)? And why is it passed when it is stored in the handle which is also passed? Why not just export a function to grab it from the handle?

libsepol/include/sepol/handle.h line 13

Quoted 7 lines

/* Create and return a sepol handle. */

libsepol/include/sepol/handle.h +5 -1
-sepol_handle_t *sepol_handle_create(void);
+extern sepol_handle_t *sepol_handle_create(void);
+
+/* Set the callback for a sepol handle */
+extern void sepol_handle_set_callback(sepol_handle_t *h, sepol_handle_callback_fn_t fn, void *varg);
+

See a above.

libsepol/include/sepol/handle.h line 16

Quoted 3 lines
libsepol/include/sepol/handle.h +3 -0
+/* Call the callback for a sepol handle */
+extern void sepol_handle_do_callback(sepol_handle_t* handle, const char* fmt, ...);
+

This is meant to replace ERR, INFO, etc. for external users of the library? Calling it do_callback is odd - it is forcing the user to think about and know about how the error system works.

Why not:

sepol_report_err(sepol_handle_t *handle, const char* fmt, ...);

With one for info and warn, or a single function that takes a level and some defines.

Also, the pointer goes with the variable name and not the type (this is C not C++) - e.g., sepol_handle_t *handle not sepol_handle_t* handle.

libsepol/include/sepol/handle.h line 18

Quoted 3 lines
libsepol/include/sepol/handle.h +2 -0
+/* Get the severity level of the last message */
+extern int sepol_handle_get_level(sepol_handle_t *h);

The severity should be passed with the message to the callback - this interface cannot be used reliably with a threaded app. Even if the error reporting system is not re-entrant (which it probably isn't) this interface is harder to use and prevents future threading changes.

This means that the level should probably be removed from the handle as well.

libsepol/include/sepol/handle.h line 22

Quoted 5 lines

/* Destroy a sepol handle. */

libsepol/include/sepol/handle.h +2 -1
-void sepol_handle_destroy(sepol_handle_t *);
+extern void sepol_handle_destroy(sepol_handle_t *);
+

Variable names?

Karl

--
This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.

author
#2 lore raw
Quoted 6 lines

Sent: Tuesday, July 25, 2006 11:55 AM

On Tue, 2006-07-25 at 10:54 -0400, Joshua Brindle wrote:

<snip>

Quoted 12 lines

Review

Quoted 5 lines

struct sepol_handle;
typedef struct sepol_handle sepol_handle_t;

(quoted diff) +2 -0
+typedef void (*sepol_handle_callback_fn_t) (void* varg, 
+sepol_handle_t* handle, const char* fmt, ...);

I'm confused about the first argument - it is just a blob of data passed to the callback, correct? Why is it called varg (which implies variable argument list to me)? And why is it passed when it is stored in the handle which is also passed? Why not just export a function to grab it from the handle?

This was the previous callback in the handle moved to a typedef, no changes made. The default callback also uses a varg.

Quoted 20 lines

libsepol/include/sepol/handle.h line 13

Quoted 8 lines

/* Create and return a sepol handle. */ -sepol_handle_t
*sepol_handle_create(void);

libsepol/include/sepol/handle.h +6 -0
+extern sepol_handle_t *sepol_handle_create(void);
+
+/* Set the callback for a sepol handle */ extern void 
+sepol_handle_set_callback(sepol_handle_t *h, 
+sepol_handle_callback_fn_t fn, void *varg);
+

See a above.

Review

Quoted 4 lines
(quoted diff) +4 -0
+/* Call the callback for a sepol handle */ extern void 
+sepol_handle_do_callback(sepol_handle_t* handle, const char* fmt, 
+...);
+

This is meant to replace ERR, INFO, etc. for external users of the library? Calling it do_callback is odd - it is forcing the user to think about and know about how the error system works.

Quoted 4 lines

Why not:

sepol_report_err(sepol_handle_t *handle, const char* fmt, ...);

Eh? How does this get the error from the library into the app calling it. The purpose of this patch is to let errors in libsepol propagate out to setools.

Quoted 17 lines

With one for info and warn, or a single function that takes a level and some defines.

Also, the pointer goes with the variable name and not the type (this is C not C++) - e.g., sepol_handle_t *handle not sepol_handle_t* handle.

Review

Quoted 3 lines
(quoted diff) +2 -0
+/* Get the severity level of the last message */ extern int 
+sepol_handle_get_level(sepol_handle_t *h);

The severity should be passed with the message to the callback - this interface cannot be used reliably with a threaded app. Even if the error reporting system is not re-entrant (which it probably isn't) this interface is harder to use and prevents future threading changes.

Quoted 2 lines

This means that the level should probably be removed from the handle as well.

This is the previous implementation, AFAIK there are other thread unsafe parts of the handle. This, however, is only being exported to static users which must have knowledge of the library and can implement locking on the handle if they are threaded.

Quoted 9 lines

libsepol/include/sepol/handle.h line 22

Quoted 6 lines

/* Destroy a sepol handle. */

libsepol/include/sepol/handle.h +2 -1
-void sepol_handle_destroy(sepol_handle_t *);
+extern void sepol_handle_destroy(sepol_handle_t *);
+

Variable names?

Matched format.

--
This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.