Re: [PATCH 8/12] IPMI: system interface hotplug

From: Andrew Morton
Date: Sun Dec 03 2006 - 16:27:24 EST


On Fri, 1 Dec 2006 22:36:55 -0600
Corey Minyard <minyard@xxxxxxx> wrote:

>
> Add the ability to hot add and remove interfaces in the ipmi_si
> driver. Any users who have the device open will get errors if they
> try to send a message.
>
> ...
>
> +static int ipmi_strcasecmp(const char *s1, const char *s2)
> +{
> + while (*s1 || *s2) {
> + if (!*s1)
> + return -1;
> + if (!*s2)
> + return 1;
> + if (*s1 != *s2)
> + return *s1 - *s2;
> + s1++;
> + s2++;
> + }
> + return 0;
> +}

Please put a newline between functions.

> +static int parse_str(struct hotmod_vals *v, int *val, char *name, char **curr)
> +{
> + char *s;
> + int i;
> +
> + s = strchr(*curr, ',');
> + if (!s) {
> + printk(KERN_WARNING PFX "No hotmod %s given.\n", name);
> + return -EINVAL;
> + }
> + *s = '\0';
> + s++;
> + for (i = 0; hotmod_ops[i].name; i++) {
> + if (ipmi_strcasecmp(*curr, v[i].name) == 0) {
> + *val = v[i].val;
> + *curr = s;
> + return 0;
> + }
> + }
> +
> + printk(KERN_WARNING PFX "Invalid hotmod %s '%s'\n", name, *curr);
> + return -EINVAL;
> +}

Does this driver really need case-insensitive string comparision?

<greps the tree for casecmp, sighs>

That doesn't look like a case-insensitive string compare to me. What's up?

> ...
>
> + while (s) {
> + curr = s;
> + s = strchr(curr, ',');
> + if (s) {
> + *s = '\0';
> + s++;
> + }
> + o = strchr(curr, '=');
> + if (o) {
> + *o = '\0';
> + o++;
> + }
> +#define HOTMOD_INT_OPT(name, val) \
> + if (ipmi_strcasecmp(curr, name) == 0) { \
> + if (!o) { \
> + printk(KERN_WARNING PFX \
> + "No option given for '%s'\n", \
> + curr); \
> + goto out; \
> + } \
> + val = simple_strtoul(o, &n, 0); \
> + if ((*n != '\0') || (*o == '\0')) { \
> + printk(KERN_WARNING PFX \
> + "Bad option given for '%s'\n", \
> + curr); \
> + goto out; \
> + } \
> + }
> +
> + HOTMOD_INT_OPT("rsp", regspacing)
> + else HOTMOD_INT_OPT("rsi", regsize)
> + else HOTMOD_INT_OPT("rsh", regshift)
> + else HOTMOD_INT_OPT("irq", irq)
> + else HOTMOD_INT_OPT("ipmb", ipmb)

oh yuk. Can't this be done via a function or something?


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/