Messages in this thread | | | Date | Wed, 26 May 2004 11:43:09 +0200 | From | Mikael Pettersson <> | Subject | Re: [PATCH] NMI trigger switch support for debugging |
| |
AKIYAMA Nobuyuki writes: > +int unknown_nmi_panic = 0;
It's a kernel coding standard to _not_ explicitly initialise static-extent data to zero.
> +/* > + * proc handler for /proc/sys/kernel/unknown_nmi_panic > + */ > +int proc_unknown_nmi_panic(ctl_table *table, int write, > + struct file *file, void __user *buffer, size_t *length) > +{ > + int old_state; > + > + old_state = unknown_nmi_panic; > + proc_dointvec(table, write, file, buffer, length); > + if (!old_state == !unknown_nmi_panic) > + return 0;
This conditional looks terribly obscure. Can you simplify it or explain your intention here?
> + if (unknown_nmi_panic) { > + if (reserve_lapic_nmi() < 0) { > + unknown_nmi_panic = 0; > + return -EBUSY; > + } else { > + set_nmi_callback(unknown_nmi_panic_callback); > + } > + } else { > + release_lapic_nmi();
You're invoking release_lapic_nmi() in response to user input, without having verified that _you_ had done a reserve_lapic_nmi() before.
It looks like the code will do horrible things if the operator invokes the sysctl incorrectly. Such errors do happen, so code should include basic sanity checks.
/Mikael - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |