[U-Boot] [PATCH v2] Add a CBFS driver and commands to u-boot

[Wolfgang Denk]

Dear Gabe Black,

In message <CAPPXG1nqDa_UgZsJx_g_RDCQOfJLjM=zaK5K7W9pgcsRrc9kig at mail.gmail.com> you wrote:
>
> > WARNING: do not add new typedefs
> > #853: FILE: include/cbfs.h:61:
> > +typedef const struct cbfs_cache_node *cbfs_file;
> 
> This typedef is to create an opaque handle for CBFS files. If you look here:
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/CodingStyle
> 
> In chapter 5 in the list of acceptable uses for typedefs, creating an
> opaque type is item a. This use of typedef is consistent with the Linux
> coding standards as described in that document and functionally important
> for this change and should stay.

Sorry, but you have to explain this better to me.

What would be wrong with using "struct cbfs_cache_node *" ?

And what of this exactly is "functionally important" here?

While we are quoting the CodingStyle, we should also code the next
lines:

	NOTE! Opaqueness and "accessor functions" are not good in
	themselves.

Why do you think it is needed here?

> > > +     if (file_cbfs_result != CBFS_SUCCESS) {
> > > +             printf("%s.\n", file_cbfs_error());
> >
> > Use
> >                puts(file_cbfs_error());
> > ?
> 
> That would leave out the \n. Whatever came next (like the prompt) would
> continue on the same line which would be wrong.

It appears your only use of file_cbfs_error() is in such constructs,
so just change file_cbfs_error() to include the newline.  There is
only a single call that doesn't really fit:

	+               if (file_cbfs_result == CBFS_FILE_NOT_FOUND)
===>	+                       printf("%s: %s\n", file_cbfs_error(), argv[2]);
	+               else
	+                       printf("%s.\n", file_cbfs_error());

but that would be better written with arguments swapped anyway:

	printf("%s: %s", argv[2], file_cbfs_error());

While we are at it: for consistent style, please also omit the
trailing '.' in all such output.

Actually you can then even simplify the code:

	if (file_cbfs_result == CBFS_FILE_NOT_FOUND)
		printf("%s: argv[2]);
	puts(file_cbfs_error());

> > > +             if (type_name)
> > > +                     printf("  %16s", type_name);
> > > +             else
> > > +                     printf("  %16d", type);
> >
> > This is probably confusing to the user.  How can he know if the file
> > type has the numerical value of "123" or if the file name is "123" ?
> 
> 
> This isn't  file name, it's the file type. No file type is named after a
> number. There are also labels on the columns indicating which is which.

Please explain the meaning of this numeric versus string value is,
then, and how the user is supposed to understand when he sees a string
and when a numeric value.

> > > +             if (filename[0])
> > > +                     printf("  %s\n", filename);
> > > +             else
> > > +                     printf("  %s\n", "(empty)");
> >
> > Use puts().
> 
> 
> The first one can't be a puts, it would have to be three. That's a lot of
> clutter to avoid a slight performance penalty when interacting with a human
> that won't notice such a tiny delay anyway. The second one could be a puts,
> but that would make the two branches of the if uneven. I'll change the
> second one.

Come on.  It this really that difficlt?  How about:

		puts("  ");
		puts(filename[0] ? filename : "(empty)");

> I put newlines there because the FAT commands do, and I have no problem
> removing them. You may want to look at the other commands and remove them
> there too.

Patches are welcome.

> Readability? I don't care that much one way or the other though. I'll
> change it.

Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
How many hardware guys does it take to change a light bulb? "Well the
diagnostics say it's fine buddy, so it's a software problem."