On Dec 17, 2014, at 12:27 PM, Wade <wadetregaskis@xxxxxxxxxx> wrote:
>
> This is needed to provide a version of e2p_feature2string that doesn't
> use (and return a pointer to) an internal, static buffer.
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.
More inline below.
> diff --git a/lib/e2p/e2p.h b/lib/e2p/e2p.h
> index 5fa41f4..53f0183 100644
> --- a/lib/e2p/e2p.h
> +++ b/lib/e2p/e2p.h
> @@ -45,6 +45,8 @@ void print_fs_state (FILE * f, unsigned short state);
> int setflags (int fd, unsigned long flags);
> int setversion (int fd, unsigned long version);
>
> +void e2p_feature_to_string(int compat, unsigned int mask, char *buf,
> + size_t buf_len);
> const char *e2p_feature2string(int compat, unsigned int mask);
> const char *e2p_jrnl_feature2string(int compat, unsigned int mask);
> int e2p_string2feature(char *string, int *compat, unsigned int *mask);
> diff --git a/lib/e2p/feature.c b/lib/e2p/feature.c
> index 6e53cfe..6dca315 100644
> --- a/lib/e2p/feature.c
> +++ b/lib/e2p/feature.c
> @@ -117,17 +117,20 @@ static struct feature jrnl_feature_list[] = {
> { 0, 0, 0 },
> };
>
> -const char *e2p_feature2string(int compat, unsigned int mask)
> +void e2p_feature_to_string(int compat, unsigned int mask, char *buf,
> + size_t buf_len)
> {
> struct feature *f;
> - static char buf[20];
> char fchar;
> int fnum;
>
> for (f = feature_list; f->string; f++) {
> if ((compat == f->compat) &&
> - (mask == f->mask))
> - return f->string;
> + (mask == f->mask)) {
> + strncpy(buf, f->string, buf_len);
> + buf[buf_len - 1] = 0;
> + return;
> + }
> }
This adds a strncpy() for every caller, even when it isn't necessary
for callers that only want the existing e2p_feature2string() function.
> switch (compat) {
> case E2P_FEATURE_COMPAT:
> @@ -144,7 +147,13 @@ const char *e2p_feature2string(int compat,
> unsigned int mask)
> break;
> }
> for (fnum = 0; mask >>= 1; fnum++);
> - sprintf(buf, "FEATURE_%c%d", fchar, fnum);
> + snprintf(buf, buf_len, "FEATURE_%c%d", fchar, fnum);
> +}
> +
> +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.
A better approach would be to make e2p_feature_to_string() (or maybe
a better name like e2p_feature_string_copy() that distinguishes it
from the existing function) call e2p_feature2string() internally so
the copy is only in the function where it is needed:
char *e2p_feature_string_copy(int compat, unsigned int mask, char *buf,
size_t buf_len)
{
const char *feature = e2p_feature2string(compat, mask);
strncpy(buf, feature, buf_len);
buf[buf_len - 1] = '\0';
return buf;
}
Note that this version also returns "buf" since this can be convenient
for the caller in some cases, and doesn't hurt.
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
