On Mon, 2007-08-20 at 16:27 -0400, Mathieu Desnoyers wrote:
> The marker activation functions sits in kernel/marker.c. A hash table is used
> to keep track of the registered probes and armed markers, so the markers
> within
> a newly loaded module that should be active can be activated at module load
> time.
Hi Mathieu!
Just reading through this patch, a couple of comments:
> +/* To be used for string format validity checking with gcc */
> +static inline void __mark_check_format(const char *fmt, ...)
> + __attribute__ ((format (printf, 1, 2)));
> +static inline void __mark_check_format(const char *fmt, ...) { }
If you place the __attribute__() before the function name, you can do
this in the definition.
> ===================================================================
> --- linux-2.6-lttng.orig/kernel/module.c 2007-08-10 19:44:18.000000000
> -0400
> +++ linux-2.6-lttng/kernel/module.c 2007-08-10 23:54:38.000000000 -0400
> @@ -1980,6 +1986,10 @@ static struct module *load_module(void _
> sechdrs[immediateindex].sh_size /
> sizeof(*mod->immediate);
> }
> #endif
> + if (markersindex)
> + sechdrs[markersindex].sh_flags |= SHF_ALLOC;
> + if (markersstringsindex)
> + sechdrs[markersstringsindex].sh_flags |= SHF_ALLOC;
>
Perhaps I'm missing something, but I don't see why these sections
wouldn't be SHF_ALLOC already.
> mod->unused_syms = (void *)sechdrs[unusedindex].sh_addr;
> if (unusedcrcindex)
> @@ -2021,6 +2031,13 @@ static struct module *load_module(void _
> if (err < 0)
> goto cleanup;
> }
> +#ifdef CONFIG_MARKERS
> + if (markersindex) {
> + mod->markers = (void *)sechdrs[markersindex].sh_addr;
> + mod->num_markers =
> + sechdrs[markersindex].sh_size / sizeof(*mod->markers);
> + }
> +#endif
Because of the wonders of ELF, section 0 has sh_addr and sh_size 0. So
the if (markersindex) is unnecessary here.
> +/*
> + * Get marker if the marker is present in the marker hash table.
> + * Must be called with markers_mutex held.
> + * Returns NULL if not present.
> + */
> +static struct marker_entry *_get_marker(const char *name)
You seem to really enjoy underscores, yet I'm having trouble
understanding why this would have an underscore in it.
> +{
> + struct hlist_head *head;
> + struct hlist_node *node;
> + struct marker_entry *e;
> + size_t len = strlen(name) + 1;
> + u32 hash = jhash(name, len-1, 0);
> +
> + head = &marker_table[hash & ((1 << MARKER_HASH_BITS)-1)];
> + hlist_for_each_entry(e, node, head, hlist) {
> + if (!strcmp(name, e->name))
> + return e;
> + }
> + return NULL;
> +}
OK, don't understand the strlen, len, len-1 dance here?
> +/*
> + * Updates the probe callback corresponding to a range of markers.
> + * Must be called with markers_mutex held.
> + */
> +static void _marker_update_probe_range(
And yet:
> +void module_marker_update(struct module *mod)
> +{
> + if (!mod->taints)
> + _marker_update_probe_range(mod->markers,
> + mod->markers+mod->num_markers, NULL, NULL);
> +}
This doesn't hold the markers_mutex.
Cheers,
Rusty.
-
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/
