Re: Suspend 2 merge: 35/51: Code always built in to the kernel.

[Pavel Machek <pavel@ucw.cz>]

Hi!

> +/*
> + * generic_read_proc
> + *
> + * Generic handling for reading the contents of bits, integers,
> + * unsigned longs and strings.
> + */
> +static int generic_read_proc(char * page, char ** start, off_t off, int count,
> +		int *eof, void *data)
> +{
> +	int len = 0;
> +	struct suspend_proc_data * proc_data = (struct suspend_proc_data *) data;
> +
> +	switch (proc_data->type) {
> +		case SUSPEND_PROC_DATA_CUSTOM:
> +			printk("Error! /proc/suspend/%s marked as having custom"
> +				" routines, but the generic read routine has"
> +				" been invoked.\n",
> +				proc_data->filename);
> +			break;
> +		case SUSPEND_PROC_DATA_BIT:
> +			len = sprintf(page, "%d\n", 
> +				-test_bit(proc_data->data.bit.bit,
> +					proc_data->data.bit.bit_vector));
> +			break;

You have your own abstraction on the top of /proc? That's no-no.

> +/*
> + * Non-plugin proc entries.
> + *
> + * This array contains entries that are automatically registered at
> + * boot. Plugins and the console code register their own entries separately.
> + */
> +

...aha, you do that to enable plugin system. Take it as another reason
why plugins have to go.

> +/* 
> + * Basic keypress handler for suspend. This is extensible
> + * via the user interface modules.
> + */
> +
> +/* For simplicity, we convert keyboard key codes to ascii,
> + * except in the case of function keys, which are mapped
> + * to 1-12. We can then use the same case statement for
> + * serial keyboards (and from a serial keyboard, you can
> + * press Control-A..L to toggle sections.
> + */
> +static unsigned int kbd_keytable[] = {
> +	  0,  27,  49,  50,  51,  52,  53,  54,  55,  56,
> +	 57,  48,   0,   0,   0,   0,   0,   0,   0, 114,
> +	116,   0,   0,   0,   0, 112,   0,   0,   0,   0,
> +	  0, 115,   0,   0,   0,   0,   0,   0, 108,   0,
> +	  0, 122,   0,   0,   0,   0,  99,   0,   0,   0,
> +	  0,   0,   0,   0,   0,   0,   0,  32,   0,   1,
> +	  2,   3,   4,   5,   6,   7,   8,   9,  10,   0,
> +	  0,   0,   0,   0,   0,   0,   0,   0,   0,   0,
> +	  0,   0,   0,   0,   0,   0,   0,  11,  12,   0,
> +};
> +
> +/*
> + * keycode_to_action
> + *
> + * Convert a keycode (serial or keyboard) into our
> + * internal code (ascii, except for function keys).
> + */
> +static unsigned int keycode_to_action(unsigned int keycode, int source)
> +{
> +	if (source == SUSPEND_KEY_SERIAL) {
> +		if (keycode > 64)
> +			return (keycode | 32);
> +		else
> +			return keycode;
> +	}

And your own keyboard driver :-(.

> +		say("BIG FAT WARNING!! %s\n\n", suspend_print_buf);
> +		if (can_erase_image) {
> +			say("If you want to use the current suspend image, reboot and try\n");
> +			say("again with the same kernel that you suspended from. If you want\n");
> +			say("to forget that image, continue and the image will be erased.\n");
> +		} else {
> +			say("If you continue booting, note that any image WILL NOT BE REMOVED.\n");
> +			say("Suspend is unable to do so because the appropriate modules aren't\n");
> +			say("loaded. You should manually remove the image to avoid any\n");
> +			say("possibility of corrupting your filesystem(s) later.\n");
> +		}
> +		say("Press SPACE to reboot or C to continue booting with this kernel\n");

Plus kernel now actually expects user interaction to solve problems
during boot. No, no.
							Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!