On 07/09/2012 08:22 AM, Dave Chinner wrote:
> On Sun, Jul 08, 2012 at 07:36:37PM +0800, Wanlong Gao wrote:
>> remove the mount options macro, use tokens instead.
>>
>> CC: Ben Myers <bpm@xxxxxxx>
>> CC: Christoph Hellwig <hch@xxxxxxxxxxxxx>
>> CC: Dave Chinner <david@xxxxxxxxxxxxx>
>> CC: Zach Brown <zab@xxxxxxxxx>
>> Signed-off-by: Wanlong Gao <gaowanlong@xxxxxxxxxxxxxx>
>> ---
>
> A "what's changed in this version" list would be handy here.
>
>> fs/xfs/xfs_super.c | 539 +++++++++++++++++++++++++++++++---------------------
>> 1 file changed, 320 insertions(+), 219 deletions(-)
>
> ....
>
>> -
>> -STATIC unsigned long
>> -suffix_strtoul(char *s, char **endp, unsigned int base)
>> +STATIC int
>> +suffix_match_int(substring_t *s, int *result)
>
> I'm not sure ints are the best unit to use here....
>
>> {
>> - int last, shift_left_factor = 0;
>> - char *value = s;
>> + int ret;
>> + int last, shift_left_factor = 0;
>> + char *value = s->to - 1;
>>
>> - last = strlen(value) - 1;
>> - if (value[last] == 'K' || value[last] == 'k') {
>> + if (*value == 'K' || *value == 'k') {
>> shift_left_factor = 10;
>> - value[last] = '\0';
>> + s->to--;
>> }
>> - if (value[last] == 'M' || value[last] == 'm') {
>> + if (*value == 'M' || *value == 'm') {
>> shift_left_factor = 20;
>> - value[last] = '\0';
>> + s->to--;
>> }
>> - if (value[last] == 'G' || value[last] == 'g') {
>> + if (*value == 'G' || *value == 'g') {
>> shift_left_factor = 30;
>> - value[last] = '\0';
>> + s->to--;
>> }
>>
>> - return simple_strtoul((const char *)s, endp, base) << shift_left_factor;
>> + ret = match_number(s, result, 0);
>> + *result = *result << shift_left_factor;
>
> Because this overflows or gives the negative values for numbers like
> 2G far too easily. I think this function needs to return an unsigned
> long.
Do you mean the "result" should be "unsigned long" but not the return value?
Because the return value is a error state.
>
>> + ret = ENOMEM;
>> + options = kstrdup(options, GFP_NOFS);
>> + if (!options)
>> + goto done;
>
> I commented on this error form previously. Can you convert them all
> to be consistent with the rest of the code? i.e:
OK, got it, will do in next version.
>
> options = kstrdup(options, GFP_NOFS);
> if (!options) {
> ret = ENOMEM;
> goto done;
> }
>
> i.e. set the error value (if necessary) in the error branch....
>
>> + orig = options;
>> +
>> + while ((p = strsep(&orig, ",")) != NULL) {
>> + int token;
>> + if (!*p)
>> continue;
>> +
>> + token = match_token(p, tokens, args);
>> + switch (token) {
>> + case Opt_logbufs:
>> + intarg = 0;
>
> Please move the initialisation of intarg up above the switch
> statement so that it is initialised once for all cases instead of
> individually in the cases that use it. That means we don't have to
> remember to do it in future for new or changed mount options...
OK, got it.
>
>> + ret = match_int(args, &intarg);
>> + if (ret)
>> + goto free_orig;
>> + mp->m_logbufs = intarg;
>> + break;
>> + case Opt_logbsize:
>> + ret = suffix_match_int(args, &intarg);
>> + if (ret)
>> + goto free_orig;
>> + mp->m_logbsize = intarg;
>> + break;
>> + case Opt_logdev:
>> + ret = ENOMEM;
>> + string = match_strdup(args);
>> + if (!string)
>> + goto free_orig;
>> +
>> + mp->m_logname = kstrndup(string, MAXNAMELEN, GFP_KERNEL);
>> if (!mp->m_logname)
>> + goto free_string;
>
> This match_strdup/kstrndup pattern is repeated a couple of times,
> and requires a special failure case (goto free_string) - wrapping it
> in helper function is probably a good idea so that the special
> failure case can be removed from this main parse loop...
Thank you for this suggestion, will do.
>
>> + case Opt_biosize:
>> + intarg = 0;
>> + ret = match_int(args, &intarg);
>> + if (ret)
>> + goto free_orig;
>> + iosizelog = ffs(intarg) - 1;
>> + break;
>> + case Opt_allocsize:
>> + ret = suffix_match_int(args, &intarg);
>> + if (ret)
>> + goto free_orig;
>> + iosizelog = ffs(intarg) - 1;
>> + break;
>
> These two can be collapsed into:
>
> case Opt_biosize:
> case Opt_allocsize:
> ret = suffix_match_int(args, &intarg);
> if (ret)
> goto free_orig;
> iosizelog = ffs(intarg) - 1;
> break;
>
> Also, these two a a good example of why intarg should be initialised
> to zero outside the switch statement - they both do almost exactly
> the same thing, but one initialises intarg and the other doesn't. Is
> that a bug? I can't tell without looking at the implementations of
> match_int and suffix_match_int....
Yeah, thank you.
>
>> + case Opt_delaylog:
>> + xfs_warn(mp,
>> + "delaylog is the default now, option is deprecated.");
>
> As preivously mentioned, duplication of the mount option is here.
Sorry, will update.
Thanks,
Wanlong Gao
>
> Adding something like:
>
> #define MNTOPT_DEPRECATED "has no effect, option is deprecated"
>
> Means these can become:
>
> case Opt_delaylog:
> xfs_warn(mp, MNTOPT_DELAYLOG MNTOPT_DEPRECATED);
> break;
>
>> + case Opt_nodelaylog:
>> + xfs_warn(mp,
>> + "nodelaylog support has been removed, option is deprecated.");
>
> xfs_warn(mp, MNTOPT_NODELAYLOG MNTOPT_DEPRECATED);
>
>> + break;
>> + case Opt_ihashsize:
>> xfs_warn(mp,
>> + "ihashsize no longer used, option is deprecated.");
>
> xfs_warn(mp, MNTOPT_IHASHSIZE MNTOPT_DEPRECATED);
>
> And so on for all the deprecated options. That way we get a
> consistent mesage for all deprecated options and it's easy to keep
> that way in the future.
>
> Cheers,
>
> Dave.
>
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
