[LU-12521] print_instance() incorrect if fsname contains a dash

• Type: Improvement
• Resolution: Fixed
• Priority: Minor
• Fix Version/s: Lustre 2.14.0
• Assignee: Andreas Dilger
• Reporter: CEA

Description

As discussed on gerrit comments, llapi_getname() is hard to use.

v2_12_0-RC3-2-gcd294a1255 added this comment that explains the problem:

/*
 * We want to turn testfs-clilov-ffff88002738bc00 into
 * testfs-ffff88002738bc00 in a portable way that doesn't depend
 * on what is after "-clilov-" as it may change in the future.
 * Unfortunately, the "fsname" part may contain a dash, so we
 * can't just skip to the first dash, and the "instance" may be a
 * UUID in the future, so we can't necessarily go to the last dash.
 */

so the string returned by llapi_getname() can contain dashes in the fsname now and might contain dashes in the instance part later on, meaning that users who need either just the fsname or just the instance will be troubled to split the string again if necessary.

I had misread Andreas' comment as that llapi_getname() was new with v2_12_53-6-g2a4821b836 and its API could be changed but llapi_getname() itself is fairly old and we probably do not want to change it, so I assume we should at least fix print_instance to use strrchr instead of strchr for now.

Ideally I'd want some version of llapi_getname where we could add an argument saying we want either one, the other or both informations in the returned string, or maybe have it also return the position of the beginning of the instance so the user could split it themselves if required? But I do not see how to change that cleanly without adding yet another hardly used function, so I do not think this is a good idea anymore.

Happy to provide a patch either way if you tell me where we want to go, this also is fairly simple.

Thanks,
–
Dominique

Issue Links

• is related to: LU-11803 sanity test 255c fails with 'Ladvise test 12, bad lock count, returned 1, actual 0'
• is related to: LU-12473 pool_list by path incorrectly prints path as pool prefix
• is related to: LU-13118 change client instance to respect ASLR
• is related to: LU-12159 allow lfs getname to only print fsname

Comments

[Gerrit Updater]

Oleg Drokin ([e-mail]) merged in patch https://review.whamcloud.com/35451/
Subject: LU-12521 llapi: add separate fsname and instance API
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: 00d14521ca1cd38a5fff82e1639ad3cc944ee13d

[Joseph Gmitter (Inactive)]

Andreas already has a patch for this.

[Gerrit Updater]

Andreas Dilger ([e-mail]) uploaded a new patch: https://review.whamcloud.com/35451
Subject: LU-12521 llapi: add separate fsname and instance API
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: f8bca0c11962797a7c6431235b55811aa608f89c

[Dominique Martinet (Inactive)]

I'd rather not add a new function unless we have a way to mark the old one as deprecated and eventually remove it altogether (in e.g. 2.14 or whatever target is appropriate), but I do not see any deprecated attribute or equivalent warning.

We could certainly make the old function use the new one and keep it around forever but that seems counterproductive in the long term

[Andreas Dilger]

Sorry, I thought my patch v2_12_53-6-g2a4821b836 added llapi_getname(), but it was only fixing lfs getname and the llapi_getname() interface has been around since 2.1.52-55-g8d935e6d31. It seems the function wasn't exported from liblustreapi.h until my patch, but I think we shouldn't change the API and need a new function name instead, like llapi_get_name_instance().