[LU-12521] print_instance() incorrect if fsname contains a dash Created: 08/Jul/19 Updated: 30/Apr/21 Resolved: 23/Jan/20 |
|
| Status: | Resolved |
| Project: | Lustre |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | Lustre 2.14.0 |
| Type: | Improvement | Priority: | Minor |
| Reporter: | CEA | Assignee: | Andreas Dilger |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Issue Links: |
|
||||||||||||||||||||
| Rank (Obsolete): | 9223372036854775807 | ||||||||||||||||||||
| 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, |
| Comments |
| Comment by Andreas Dilger [ 08/Jul/19 ] |
|
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(). |
| Comment by Dominique Martinet (Inactive) [ 09/Jul/19 ] |
|
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 |
| Comment by Gerrit Updater [ 09/Jul/19 ] |
|
Andreas Dilger (adilger@whamcloud.com) uploaded a new patch: https://review.whamcloud.com/35451 |
| Comment by Joseph Gmitter (Inactive) [ 10/Jul/19 ] |
|
Andreas already has a patch for this. |
| Comment by Gerrit Updater [ 23/Jan/20 ] |
|
Oleg Drokin (green@whamcloud.com) merged in patch https://review.whamcloud.com/35451/ |