On Dec 17, 2014, at 5:03 PM, Wade <wadetregaskis@xxxxxxxxxx> wrote:
>
>> What's wrong with a pointer to a static string? Since the strings are
>>
>> static, none of the callers can change them. Since they are just pointers, they can be shared by any number of callers.
>
> The existing code doesn't always return a static string - sometimes it
> sprintfs one up...
I see that now. That said, your version now does this for _every_ caller
of e2p_feature2string(), while the original code only did it in the rare
case of an unsupported feature bit set on on a filesystem. I don't see
that as an improvement over the original.
One option, maybe not very pretty but not fatally ugly, is to just
initialize all of the "unknown" features as static strings. That ends
up being 23 extra "FEATURE_Cnn" strings, 21 extra "FEATURE_Rnn" strings,
and 17 extra "FEATURE_Inn" strings.
That ends up being less than 1KB of static text, which isn't bad at all.
This could potentially speed up e2p_feature2string(), since it could
index directly into three different arrays instead of having to scan all
of them to find the right f_compat type first. I'm not sure if it really
makes a noticeable difference in the end though.
>>> +const char *e2p_feature2string(int compat, unsigned int mask)
>>> +{
>>> + static char buf[20];
>>> + e2p_feature_to_string(compat, mask, buf, sizeof(buf) / sizeof(buf[0]));
>>
>> This is copying into a single shared buffer for all callers, which is
>> not thread safe.
>
> ... exactly.
So I'm not against your version, but not at the expense of making the
existing code worse.
Cheers, Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
