Re: [PATCH -mm 9/9] netconsole: Support dynamic reconfigurationusing configfs

[Satyam Sharma]

[ Some more thoughts I had on this, for those interested. ]

> +#ifdef	CONFIG_NETCONSOLE_DYNAMIC
> +
> +/*
> + * Targets that were created by parsing the boot/module option string
> + * do not exist in the configfs hierarchy and will never go away (and
> + * have zeroed-out config_item members). So make these a no-op for them.
> + */
> +static void netconsole_target_get(struct netconsole_target *nt)
> +{
> +	static struct config_item empty_item;	/* Zeroed-out config_item */
> +
> +	if (memcmp(&nt->item, &empty_item, sizeof(struct config_item)))
> +		config_item_get(&nt->item);
> +}
> +
> +static void netconsole_target_put(struct netconsole_target *nt)
> +{
> +	static struct config_item empty_item;	/* Zeroed-out config_item */
> +
> +	if (memcmp(&nt->item, &empty_item, sizeof(struct config_item)))
> +		config_item_put(&nt->item);
> +}
> +
> +#else	/* !CONFIG_NETCONSOLE_DYNAMIC */
> +
>  /*
> - * Allocate new target and setup netpoll for it.
> + * No danger of targets going away from under us when dynamic
> + * reconfigurability is off.
> + */
> +static void netconsole_target_get(struct netconsole_target *nt)
> +{
> +}
> +
> +static void netconsole_target_put(struct netconsole_target *nt)
> +{
> +}
> +
> +#endif	/* CONFIG_NETCONSOLE_DYNAMIC */

This whole stuff (and calls to netconsole_target_get and _put from code
below) is not necessary. All the access to the netconsole_target objects
(that are present in the target_list) happens inside
spin_lock_irqsave(target_list_lock), so there's no need to do a
config_item_get() on these as there is no race window in which userspace
can delete (and thus release) these items from under us. Note that
drop_netconsole_target() removes target-to-delete from target_list
(under spin_lock_irqsave) _before_ free'ing the objects, and that both
_get() and _put() are balanced *inside* a spin_lock_irqsave /
spin_unlock_irqrestore pair -- so all this business is quite pointless.

I've kept this stuff in for now, but would like to get rid of it, really.
It's an unnecessary #ifdef-#else-#endif kludge. Comments?

> +	if (nt->enabled) {
> +		printk(KERN_ERR "netconsole: target (id %d) is enabled, "
> +				"disable to update parameters\n", nt->id);
> +		return -EINVAL;
> +	}

[ Such code exists in all the store() operations for all attributes other
than "enabled" itself. ]

Anyway, the problem is that the error does not show up violently in the
shell. The write fails, of course, values are unchanged, and dmesg shows
the diagnostic outputted above -- but "echo" does not complain about
-EINVAL returning from write(2) for some reason, neither did -EACCES.
I guess "echo" needs to be fixed. BTW would -EACCES be more appropriate?

Satyam
-
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/