Re: [PATCH 2/5] CPER: Adjust code flow of some functions

["Chen, Gong" <gong.chen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2268 bytes --]

On Mon, Apr 14, 2014 at 03:39:24PM +0200, Borislav Petkov wrote:
> > +char *cper_mem_err_location(const struct cper_sec_mem_err *mem)
> > +{
> > +	char *p;
> > +	u32 n = 0;
> > +
> > +	memset(mem_location, 0, CPER_REC_LEN);
> > +	p = mem_location;
> >  	if (mem->validation_bits & CPER_MEM_VALID_NODE)
> > -		pr_debug("node: %d\n", mem->node);
> > +		n += sprintf(p + n, " node: %d", mem->node);
> > +	if (n >= CPER_REC_LEN)
> > +		goto end;
> 
> 	n += vscnprintf(p + n, CPER_REC_LEN - n - 1, "... ", ...);
> 
> and you can drop those
> 
> 	if (n >= CPER_REC_LEN)
> 
> tests.
> 
> vscnprintf() because it returns the actual number of characters written
> and not what snprintf does and return the chars count it would've
> written. (Thx Richard for the hint!).
You are absolutely right. This function is perfect for this usage.

> > +char *cper_dimm_err_location(const struct cper_sec_mem_err *mem)
> > +{
> > +	const char *bank = NULL, *device = NULL;
> > +
> > +	memset(dimm_location, 0, CPER_REC_LEN);
> > +	if (!(mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE))
> > +		goto end;
> 
> a goto label which is used only once?? What's wrong with
> 
> 		return dimm_location;
> 
I just feel repeating the same words looks ugly. OK, it is fine to me
to adopt your suggestion.

> > +	dmi_memdev_name(mem->mem_dev_handle, &bank, &device);
> > +	if (bank != NULL && device != NULL)
> 
> 	if (bank && device)
> 
> > +		snprintf(dimm_location, CPER_REC_LEN - 1,
> > +			 "DIMM location: %s %s", bank, device);
> > +	else
> > +		snprintf(dimm_location, CPER_REC_LEN - 1, "DMI handle: 0x%.4x",
> 
> Why the DMI handle? Why not say: "unable to find location" or "location
> not present in DMI" or something more user-friendly.
In essential, I use the suggestion from Tony Luck. DMI handle can't be found
so I just list the original info for it. Maybe I can merge them into one.

> > +#define CPER_REC_LEN				512
> 
> How did we come up with this? Some spec? 512 chars for an error record?
> That's a bit too much in my book.
> 
Because 128 is not enough once all fields in error record exist, and 256 looks
a little bit tough so I choose a bigger value. No spec for it. I just hope
it can contain everything.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]