-
Notifications
You must be signed in to change notification settings - Fork 9
Added the option to set fsname manually #28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Added the option to set fsname manually #28
Conversation
This was done to allow the copytool to archive to the same fs (TSM) from different lustre instances.
src/lhsmtool_tsm.c
Outdated
| {.name = "owner", .has_arg = required_argument, .flag = NULL, .val = 'o'}, | ||
| {.name = "servername", .has_arg = required_argument, .flag = NULL, .val = 's'}, | ||
| {.name = "conf", .has_arg = required_argument, .flag = NULL, .val = 'c'}, | ||
| {.name = "fsname", .has_arg = no_argument, .flag = NULL, .val = 'f'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why no_argument?
Why not required_argument (like with --conf)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the default should be that if no flag is given it defaults to assigning it to whatever mount is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
man getopt_long:
has_arg
is: no_argument (or 0) if the option does not take an argument;
required_argument (or 1) if the option requires an argument; or
optional_argument (or 2) if the option takes an optional argu‐
ment.
Reading that, I am surprised, that it works at all…
There are two questions:
- Is the option (including argument to the option) optional or required?
- IF one gives the option, is the argument optional, required (or ignored)?
I guess, you want (1) optional (2) required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think optional_argument means that you can give --fsname without any arguments. That does not feel right to me?
src/lhsmtool_tsm.c
Outdated
| if (!opt.o_fsname[0]) | ||
| strncpy(opt.o_fsname, opt.o_mnt, DSM_MAX_FSNAME_LENGTH); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider using braces, even for single line commands:
if (!opt.o_fsname[0]) {
strncpy(opt.o_fsname, opt.o_mnt, DSM_MAX_FSNAME_LENGTH);
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The braces were left out to stay somewhat congruent with another condition check 2 lines below it. Do you want me to change them both?
if (len_fsname > 2 && opt.o_fsname[len_fsname - 1] == '/')
opt.o_fsname[len_fsname - 1] = '\0';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just give new code braces. :-)
This was done to allow the copytool to archive to the same fs (TSM) from different lustre instances.