[flashrom] [PATCH 1/2] convert programmer print messages to msg_p*

[Michael Karcher]

Just some further notes:

Am Dienstag, den 30.03.2010, 07:33 +0200 schrieb Carl-Daniel Hailfinger:
> >  		if (board->dmi_pattern) {
> >  			if (!has_dmi_support) {
> > -				fprintf(stderr, "WARNING: Can't autodetect %s %s,"
> > +				msg_perr("WARNING: Can't autodetect %s %s,"
> >   
> Not sure about this one. Could be downgraded to _pinfo since this will
> clutter up the logs on machines without dmidecode.
Just as a note: This message is only printed if PCI IDs match for a
board and it's just the DMI string that's missing. So it's quite likely
that there really is a problem if dmidecode is not available. It might
be a board that doesn't need an enable - in this case the message is
spam, but it is important that the user gets this message if flashsrom
is running on the board that is printed here.

As _pinfo is also always printed, _pinfo would be fine, too.

> >  		if (board->enable != NULL) {
> > -			printf("Disabling flash write protection for "
> > +			msg_pinfo("Disabling flash write protection for "
> >  			       "board \"%s %s\"... ", board->vendor_name,
> >  			       board->board_name);
> >  
> >  			ret = board->enable(board->vendor_name);
> >  			if (ret)
> > -				printf("FAILED!\n");
> > +				msg_pinfo("FAILED!\n");
> Mh. _perr?
No. Not that easily. You can't write "FAILED!\n"  to a different channel
than the "Disabling..." message. Having the failure on stderr might be
desirable, but in that case you also would want to have the info about
*what* failed on stderr.

> > -		printf_debug("tried to set register 0x%x to 0x%x on %s failed (WARNING ONLY)\n", 0x40, new, name);
> > -		printf_debug("Stuck at 0x%x\n", newer);
> > +		msg_pdbg("tried to set register 0x%x to 0x%x on %s failed (WARNING ONLY)\n", 0x40, new, name);
> > +		msg_pdbg("Stuck at 0x%x\n", newer);
> Mh. _pinfo or even _perr?
Yes, please upgrade to at least _pinfo, so this warning appears even
without -V.

> > -		printf_debug("tried to set register 0x%x to 0x%x on %s failed (WARNING ONLY)\n", 0x45, new, name);
> > -		printf_debug("Stuck at 0x%x\n", newer);
> > +		msg_pdbg("tried to set register 0x%x to 0x%x on %s failed (WARNING ONLY)\n", 0x45, new, name);
> > +		msg_pdbg("Stuck at 0x%x\n", newer);
> _pinfo or even _perr?
Dito. Maybe we should make a pci_checked_write helper function of that.

> >  	if (idsel) {
> >  		idsel += strlen("fwh_idsel=");
> >  		fwh_conf = (uint32_t)strtoul(idsel, NULL, 0);
> >  
> >  		/* FIXME: Need to undo this on shutdown. */
> > -		printf("\nSetting IDSEL=0x%x for top 16 MB", fwh_conf);
> > +		msg_pdbg("\nSetting IDSEL=0x%x for top 16 MB", fwh_conf);
> This one is a bit difficult. We change some settings on user request.
> Should we thus make this action _pinfo?
Good question. I don't think we need _pinfo here, as long as we complain
for unrecognized options. But feedback whether the option was spelled
correctly is a good idea, just think of somebody using fwh-idsel instead
of fwh_idsel. There should be an obvious difference in output. I would
suggest to leave the _pdbg, and have a _perr for bad option.


> >  	if (pci_read_byte(dev, 0x40) != val) {
> > -		printf("\nWARNING: Failed to enable flash write on \"%s\"\n",
> > +		msg_pinfo("\nWARNING: Failed to enable flash write on \"%s\"\n",
> >  		       name);
> >  		return -1;
> >  	}
_pinfo or _perr...

> >  	dmidecode_pipe = popen(commandline, "r");
> >  	if (!dmidecode_pipe) {
> > -		printf_debug("DMI pipe open error\n");
> > +		msg_pdbg("DMI pipe open error\n");
> Hm. _pinfo or even _perr? Then again, dmidecode may not be installed on
> all systems, and we don't want to clutter up the log of innocent people.
At least on unix, if dmidecode is missing, pclose will return the error,
not popen, as starting the shell that should execute the dmidecode
command was successfull. So having _perr here sounds reasonable, but one
should check djgpp behaviour.

> >  		return NULL;
> >  	}
> >  	if (!fgets(answerbuf, DMI_MAX_ANSWER_LEN, dmidecode_pipe)) {
> >  		if(ferror(dmidecode_pipe)) {
> > -			printf_debug("DMI pipe read error\n");
> > +			msg_pdbg("DMI pipe read error\n");
> dito
definitely _perr. This should never ever happen.

> >  	if (pclose(dmidecode_pipe) != 0) {
> > -		printf_debug("DMI pipe close error\n");
> > +		msg_pdbg("DMI pipe close error\n");
> _pinfo or even _perr? This should never happen.
As *this* is the failure mode of missing dmidecode, have this _pinfo and
better print something like "dmidecode execution unsucessfull -
continuing without DMI info", to tell the user that the message "sh:
dmidecode not found" which has been printed by the shell has been
handled.


> > -		printf_debug("Laptop detected via DMI\n");
> > +		msg_pdbg("Laptop detected via DMI\n");
> _pinfo please
Why that? The general laptop warning should be pinfo, but the source of
the laptop detection is fine to be _pdbg, I think. That the Laptop was
detected by DMI is only important to know if you want to handle false
positives/negatives of the laptop detection, and in that case, doing a
-V run of flashrom is appropriate.

> > -	printf_debug("matching %s against %s\n", value, pattern);
> > +	msg_pdbg("matching %s against %s\n", value, pattern);
Make that _pspew even. No one really cares.

> > diff --git a/physmap.c b/physmap.c
> > index 38a395c..efd641c 100644
> > --- a/physmap.c
> > +++ b/physmap.c
> > @@ -160,76 +160,76 @@ void *sys_physmap_ro_cached(unsigned long phys_addr, size_t len)
> >  			perror("Critical error: open(" MEM_DEV ")");
> >   
> 
> Hm. How do we handle perror? It should definitely not happen in a library.
make that "msg_perr("Critical error: open(" MEM_DEV "): %s",
strerror(errno));" instead.

Regards,
  Michael Karcher