On Mon, Sep 24, 2007 at 12:49:54PM -0400, Mathieu Desnoyers wrote:
> +struct __mark_marker {
> + const char *name; /* Marker name */
> + const char *format; /* Marker format string, describing the
> + * variable argument list.
> + */
> + char state; /* Marker state. */
> + marker_probe_func *call;/* Probe handler function pointer */
> + void *pdata; /* Private probe data */
This is normally called private in the kernel, and keeping this
consistant would be nice.
> +} __attribute__((aligned(8)));
Why do we care about the alignment here?
> +/* To be used for string format validity checking with gcc */
> +static inline void __attribute__ ((format (printf, 1, 2)))
> + __mark_check_format(const char *fmt, ...) { }
Please put each of the curly braces on a line of it's own, so it's
clear this is an empty inline from the 1000 feet few, as it first
looks like a prototype. Also aren't __attributes__ normally afer
the function identifier, ala:
static inline void __mark_check_format(const char *fmt, ...)
__attribute__ ((format (printf, 1, 2)))
{
}
or is this after notation only for prototypes but not actual implementations?
(yeah, gnu C extensions sometimes have syntax that odd)
> +#ifdef CONFIG_MARKERS
> +void module_update_markers(struct module *probe_module, int *refcount)
> +{
> + struct module *mod;
> +
> + mutex_lock(&module_mutex);
> + list_for_each_entry(mod, &modules, list)
> + if (!mod->taints)
> + marker_update_probe_range(mod->markers,
> + mod->markers + mod->num_markers,
> + probe_module, refcount);
> + mutex_unlock(&module_mutex);
> +}
> +EXPORT_SYMBOL_GPL(module_update_markers);
Why is this exported? The markers code is always built into the kernel,
isn't it?
> +EXPORT_SYMBOL_GPL(module_get_iter_markers);
Same here.
> +/*
> + * Add the marker to the marker hash table. Must be called with markers_mutex
> + * held.
> + */
> +static int add_marker(const char *name,
> + const char *format, marker_probe_func *probe, void *pdata)
static int add_marker(const char *name, const char *format,
marker_probe_func *probe, void *private)
> +void marker_update_probe_range(
> + struct __mark_marker *begin,
> + struct __mark_marker *end,
> + struct module *probe_module,
> + int *refcount)
void marker_update_probe_range(struct __mark_marker *begin,
struct __mark_marker *end, struct module *probe_module,
int *refcount)
> +EXPORT_SYMBOL_GPL(marker_update_probe_range);
What is this one exported for?
> +/*
> + * Update probes, removing the faulty probes.
> + * Issues a synchronize_sched() when no reference to the module passed
> + * as parameter is found in the probes so the probe module can be
> + * safely unloaded from now on.
> + */
> +static inline void marker_update_probes(struct module *probe_module)
no need to mark this inline, the compiler takes care of that for you
if nessecary.
> +int marker_get_iter_range(struct __mark_marker **marker,
> + struct __mark_marker *begin,
> + struct __mark_marker *end)
int marker_get_iter_range(struct __mark_marker **marker,
struct __mark_marker *begin, struct __mark_marker *end)
> + int found = 0;
> +
> + if (!*marker && begin != end) {
> + found = 1;
> + *marker = begin;
> + } else if (*marker >= begin && *marker < end) {
> + found = 1;
> + /*
> + * *marker is known to be a valid marker from now on.
> + */
> + }
> + return found;
if (!*marker && begin != end) {
*marker = begin;
return 1;
}
if (*marker >= begin && *marker < end)
return 1;
return 0;
?
There seem to be a lot of exports and some functions that don't seem
to be used by the obvious marker use-cases like your example, blktrace
or sputrace. Care to explain why we'd really want them or better cut
them out for this first submission?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
