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:
signature.asc
Description: Digital signature
