ClusterLabs / fence-agents

Fence agents
104 stars 160 forks source link

fence_kdump: properly support -v[X] and -vvv (and combinations) #430

Closed oalbrigt closed 3 years ago

oalbrigt commented 3 years ago

https://github.com/ClusterLabs/fence-agents/issues/429

nrwahl2 commented 3 years ago
09:18:49 In file included from kdump/fence_kdump_send.c:37:0:
09:18:49 kdump/options.h: In function ‘set_option_verbose’:
09:18:49 kdump/options.h:244:13: error: ‘for’ loop initial declarations are only allowed in C99 mode
09:18:49              for (int i = 0; i < strlen(arg); i++) {

We could either compile in C99 mode (which should be fine since it's 22 years old) or just declare the counter variable before the loop.

nrwahl2 commented 3 years ago

The logic looks good to me, and it seems to work fine (debug line added below). I think we're good to merge this after we fix the compile issue on certain platforms.

[root@fastvm-rhel-8-0-23 kdump]# ./fence_kdump -v 2>/dev/null
[debug]: final verbose: 1
[root@fastvm-rhel-8-0-23 kdump]# ./fence_kdump -vv 2>/dev/null
[debug]: final verbose: 2
[root@fastvm-rhel-8-0-23 kdump]# ./fence_kdump -vvv 2>/dev/null
[debug]: final verbose: 3
[root@fastvm-rhel-8-0-23 kdump]# ./fence_kdump -v1 2>/dev/null
[debug]: final verbose: 1
[root@fastvm-rhel-8-0-23 kdump]# ./fence_kdump -v2 2>/dev/null
[debug]: final verbose: 2
[root@fastvm-rhel-8-0-23 kdump]# ./fence_kdump -v2 -v 2>/dev/null
[debug]: final verbose: 3
[root@fastvm-rhel-8-0-23 kdump]# ./fence_kdump -v -v2 2>/dev/null
[debug]: final verbose: 3
[root@fastvm-rhel-8-0-23 kdump]# ./fence_kdump -vv -v2 2>/dev/null
[debug]: final verbose: 4
[root@fastvm-rhel-8-0-23 kdump]# ./fence_kdump -v2 -vv 2>/dev/null
[debug]: final verbose: 4
[root@fastvm-rhel-8-0-23 kdump]# ./fence_kdump -v2 -v3 2>/dev/null
[debug]: final verbose: 5

There are a couple of quirks. For example, invalid characters in the arg string get discarded quietly, but future vs in the string still get processed.

[root@fastvm-rhel-8-0-23 kdump]# ./fence_kdump -va 2>/dev/null
[debug]: final verbose: 1
[root@fastvm-rhel-8-0-23 kdump]# ./fence_kdump -vav 2>/dev/null
[debug]: final verbose: 2

As another example, digit arguments and sequential v arguments can be mixed, but not in the same arg string.

[root@fastvm-rhel-8-0-23 kdump]# ./fence_kdump -vv -v4 2>/dev/null
[debug]: final verbose: 6
[root@fastvm-rhel-8-0-23 kdump]# ./fence_kdump -vv4 2>/dev/null
[debug]: final verbose: 2

My preference would be that a number takes precedence over a sequence of vs and that the last number specified wins. For example, -vvv -v2 or -v2 -vvv would yeld verbose=2 (it currently yields 5); -v2 -v3 would yield verbose=3 (it currently yields 5); and -vv4 would yield verbose=4 (it currently yields 2).

But handling those edge cases in that way isn't worth the extra complexity that it would add. There's no valid reason to mix digit arguments with a sequence of vs. No one should be doing that unless they're trying to mess with the option parser, like I'm doing now.

The only reason we even need to accept number arguments is so that we can pass extra verbosity through stdin from stonith device configs.

oalbrigt commented 3 years ago

I added some logic to solve the edge cases, and throw an error to inform the user they've supplied an invalid value to the verbose parameter.

nrwahl2 commented 3 years ago

Thanks, LGTM.