[LU-7557] Add fid_to_string and string_to_fid functions

• Status: Open
• Project: Lustre
• Type: Improvement
• Priority: Minor
• Reporter: Ben Evans (Inactive)
• Assignee: Ben Evans (Inactive)
• Resolution: Unresolved

Description

Add fid_to_string and string_to_fid functions for more readable print/scan statements. Allow for error checking, defaults, etc.

Example print statement:

Original: C_DEBUG(D_PAGE, "%lu@"DFID" %p %lx %d\n",
idx, PFID(&hdr->coh_lu.loh_fid), vmpage, vmpage->private, type);

New: C_DEBUG(D_PAGE, "%lu@%s %p %lx %d\n",
idx, fid_to_string(hdr->coh_lu.loh_fid), vmpage, vmpage->private, type);

Example scan:

Original: if (sscanf(opt.o_src, SFID, RFID(&old_fid)) != 3 || ...

New:
old_fid = string_to_fid(opt.o_src);
if ( ...

Comments

[Ben Evans (Inactive)]

Offshoot from discussion of LU-5011

[James A Simmons]

Yes please do this. The upstream reviewers hate macros so replacing this with inline functions would be the way to go.

[Robert Read (Inactive)]

The caller should be managing memory for fid_to_string, and you''ll need handle bad strings in string_to_fid, so how about these prototypes:

int fid_to_string(const struct lu_fid *fid, char * str, size_t length);
int string_to_fid(char * str, struct lu_fid *fid);

To keep things simple I suggest that fid_to_string should not include the braces, and let applications add the braces where needed.

[Ben Evans (Inactive)]

I was thinking:

const str* fid_to_string(const struct lu_fid fid, str string, int sz);

struct lu_fid string_to_fid(char* fidstr);

I'm fine with the nobraces version.

99% of all the uses of this are going to be for printk statements, and all but one place where fids are read in.

[Robert Read (Inactive)]

If these are going to be library functions then there will be more uses than just what you can find in the Lustre tree. We need to be able to handle whatever is thrown at us from external applications.

Returning a pointer to the passed in buffer is reasonable for fid_to_string, just make sure to handle case when the buffer is too small in a sane way.

I would prefer an actual return code from string_to_fid so applications using this can deal with garbage as early as possible.

[Ben Evans (Inactive)]

I was thinking it should look like atoi and itoa

[Robert Read (Inactive)]

Well, atoi does no error checking and is essentially the poster child for what not to do. There is strtol which does check errors, but with an overly complex interface for our needs.

Instead of returning the error code (which seems the most conventional to me), another option is to pass &error to the string_to_fid().

[James A Simmons]

Just as a note I had discussion with Greg about the policy of UAPI type headers. What I was told was UAPI headers, ones shared between kernel and user space, should only contain data structure or simple defines. No functions should be used in UAPI headers.

[Ben Evans (Inactive)]

I think we've got more flexability with str_to_fid in terms of return and fields than fid_to_str.

But yes, error checking is a good thing here.

[Robert Read (Inactive)]

James, that's right and also there shouldn't be any inline functions in GPL headers intended to be used by non-GPL code.

[Andreas Dilger]

I agree with Robert here. Having an explicit error returned from string_to_fid() is needed to make the error handling reasonable. It might make sense to have a separate fid_to_string_br() or something that prints the standard square brackets around the FID, or it will make for a lot of "[%s]" in the code, and easy to miss those and have inconsistent output.

[Gerrit Updater]

Ben Evans ([e-mail]) uploaded a new patch: http://review.whamcloud.com/17933
Subject: LU-7557 headers: add fid_to_str and str_to_fid
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: fc8efddcdf460b40a61acb83da86ed6cf8ae4689

[Ben Evans (Inactive)]

Started based on comments in: https://jira.hpdd.intel.com/browse/LU-5011