On Mon, 31 Aug 2009, [iso-8859-2] Micha³ Nazarewicz wrote:
> Also, names of various functions, structures and variables
> have been changed. This is to prevent potential name
> collision when the common file will be used with other code.
Personally, I dislike seeing Changelog messages that are continuations
of the Subject line. They can easily get separated, making the context
difficult or impossible to figure out.
> @@ -170,10 +172,10 @@
> * REGISTERED atomic bitflag; the driver will only be deregistered once.
> * The second problem is resolved by having fsg_unbind() check
> * fsg->state; it won't try to stop the thread if the state is already
> - * FSG_STATE_TERMINATED.
> + * STOR_STATE_TERMINATED.
Did you really have to change all these things throughout the entire
file? It's hard to see the important parts of the patch, buried as
they are amidst all these inconsequential name changes.
> *
> * To provide maximum throughput, the driver uses a circular pipeline of
> - * buffer heads (struct fsg_buffhd). In principle the pipeline can be
> + * buffer heads (struct stor_buffhd). In principle the pipeline can be
Same here. And in many other places throughout the patch.
> -#ifdef DUMP_MSGS
> -
> -static void dump_msg(struct fsg_dev *fsg, const char *label,
> - const u8 *buf, unsigned int length)
> -{
> - if (length < 512) {
> - DBG(fsg, "%s, length %u:\n", label, length);
> - print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET,
> - 16, 1, buf, length, 0);
> - }
> -}
> -
> -static void dump_cdb(struct fsg_dev *fsg)
> -{}
> -
> -#else
> -
> -static void dump_msg(struct fsg_dev *fsg, const char *label,
> - const u8 *buf, unsigned int length)
> -{}
> -
> -#ifdef VERBOSE_DEBUG
> -
> -static void dump_cdb(struct fsg_dev *fsg)
> +static inline void dump_cdb(struct fsg_dev *fsg)
> {
> +#if defined DUMP_MSGS || !defined VERBOSE_DEBUG
> print_hex_dump(KERN_DEBUG, "SCSI CDB: ", DUMP_PREFIX_NONE,
> 16, 1, fsg->cmnd, fsg->cmnd_size, 0);
> +#endif
> }
Ugh. Please don't do this. The style in Linux is to avoid having
preprocessor conditionals inside functions, as much as possible.
> - DBG(fsg, "%s set halt\n", name);
> + SDBG(fsg, "%s set halt\n", name);
Here's another example of an apparently unnecessary name change.
Would DBG really clash with anything else?
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
