Messages in this thread | | | Date | Fri, 29 May 2015 13:20:31 -0500 | From | Aravind Gopalakrishnan <> | Subject | Re: [PATCH 3/6] edac, mce_amd_inj: Modify flags attrigute to use string arguments |
| |
On 5/29/2015 8:49 AM, Borislav Petkov wrote: > On Wed, May 27, 2015 at 02:03:35PM -0500, Aravind Gopalakrishnan wrote: >> >> +#define MAX_FLAG_OPT_SIZE 10 > Why 10?
No specific reason. Just an arbitrary max value that we won't hit right now or in the future.
> This should be 2 and increased when another, longer injection type > string gets introduced.
Okay, I'll make it 2 here and will increase it to 3 in the next patch when I introduce injection types for the interrupts.
> >> >> - *val = m->inject_flags; >> + for (i = 0; i <= N_INJ_TYPES; i++) { >> + inj_op = flags_options[i]; >> + if (inj_op && strncmp(inj_op, buf, cnt) == 0) >> + break; >> + } > What's wrong with: > > for (i = 0; i < N_INJ_TYPES; i++) { > if (!strncmp(flags_options[i], buf, strlen(flags_options[i]))) { > inj_type = i; > return 0; > } > } > > return -EINVAL;
Hmm. That should work. Will simplify it in the next version. I think I had a NULL in flags_options[] to denote the invalid option. With your simplification, that can be removed. I'll make that change too.
And the 'cnt' value that I used won't be necessary. So, will remove that from __set_inj() definition as well.
>> +static ssize_t flags_write(struct file *filp, const char __user *ubuf, >> + size_t cnt, loff_t *ppos) >> +{ >> + char buf[MAX_FLAG_OPT_SIZE + 1]; >> + int err; >> + size_t ret; >> + >> + ret = cnt; > You're assigning cnt to ret here... > >> + >> + if (cnt > MAX_FLAG_OPT_SIZE) >> + cnt = MAX_FLAG_OPT_SIZE; > ... but correcting cnt afterwards. The assignment should be *after* that > correction.
With the change to MAX_FLAG_OPT_SIZE to 2 (or 3 depending on the max size), this check is really an error check.
Shall I make err = -EINVAL by default and return that here if the condition is not satisfied?
Thanks, -Aravind.
| |