Re: [PATCH v1] firmware loader: introduce module parameter tocustomize fw search path

[Linus Torvalds]

On Thu, Oct 25, 2012 at 5:46 PM, Ming Lei <ming.lei@xxxxxxxxxxxxx> wrote:
>                 struct file *file;
> -               snprintf(path, PATH_MAX, "%s/%s", fw_path[i], buf->fw_id);
> +
> +               if (i < 0) {
> +                       if (!fw_path_para[0])   /* No customized path */
> +                               continue;
> +                       snprintf(path, PATH_MAX, "%s/%s", fw_path_para,
> +                                buf->fw_id);
> +               } else {
> +                       snprintf(path, PATH_MAX, "%s/%s", fw_path[i],
> +                                buf->fw_id);
> +               }

Ugh. This is just disgusting.

Please just make "fw_path[0]" just be the pointer to fw_path_para[]
(which sounds like the cleanest fix) and get rid of the negative 'i'
and conditional entirely.

Or if there is some odd reason you don't want to do that, at least
make the conditional much smaller, without the snprintf() in both arms
(ie make the if-statement just set a "const char *dir" variable to
either fw_path[i] or fw_path_para or whatever).

Sure, the compiler *may* merge them (gcc does, but I've seen it miss
them too), but even if the compiler might fix up ugly code, that's not
a reason for it to be ugly in the source code anyway.

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