Skip to content
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

dtls-client.c: accept options after arguments. #221

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

boaks
Copy link
Contributor

@boaks boaks commented Jan 18, 2024

No description provided.

@boaks boaks force-pushed the support_cli_options_behind_arguments branch from 28da448 to bbe2108 Compare January 18, 2024 09:11
@boaks boaks mentioned this pull request Jan 18, 2024
usage(argv[0], dtls_package_version());
exit(1);
}
break;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From PR #222 (@mrdeep1 )

I'm not convinced that this is the best way do do this (very unusual), moving the logic processing the arguments to within the getopt() logic, and fail to see why it is actually needed. If this is done, at a minimum dtls_set_log_level(log_level); needs to be included at the start.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, if "very unusual" really applies.

What I found:
The dtls-client silently ignores options after the arguments. We can either stick to that or process arguments (opt == -1) while incrementing optind to over come that. If someone wants options to be treated as arguments, using "--" works well.
Assuming the support for options after arguments should be supported, the '-1' may be processed either within the switch or ahead by checking for -1. I decided for this way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a side note, Windows getopt() general implementation does not support having options after arguments as this is not in the POSIX standard, but is supported by GNU.

See https://stackoverflow.com/questions/58429368/getopt-does-not-order-arguments-in-windows

Copy link
Contributor Author

@boaks boaks Jan 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively would be to fail on additional arguments/options after the port. The code before just silently ignored that. The check if (argc <= optind) { doesn't work for

dtls-client localhost 5684 -e

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears (for GNU), that

dtls-client localhost 5684 -e

gets argv{} populated as

dtls-client -e localhost 5684

as per

Starting program: /Misc/github/tinydtls/tests/dtls-client 1.2.3.4 2222 -e

Breakpoint 2, main (argc=4, argv=0x7fffffffe688) at dtls-client.c:491
491       dtls_set_log_level(log_level);
(gdb) l
486           usage(argv[0], dtls_package_version());
487           exit(1);
488         }
489       }
490
491       dtls_set_log_level(log_level);
492
493       if (argc <= optind) {
494         usage(argv[0], dtls_package_version());
495         exit(1);
(gdb) p argv[1]
$1 = 0x7fffffffe8f0 "-e"
(gdb) p argv[2]
$2 = 0x7fffffffe8e3 "1.2.3.4"
(gdb) p argv[3]
$3 = 0x7fffffffe8eb "2222"
(gdb) p argv[4]
$4 = 0x0
(gdb) p optind
$5 = 2
(gdb) p argc
$6 = 4
(gdb)
```

> The code before just silently ignored that.

Only for non GNU implementations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only for non GNU implementations.

I run in this issue with Ubuntu 20.04 and 22.04 and the default toolchains that comes with them. AFAIK, that's GNU.

If "getopt()" stops the return options after first ".1", the code of this PR would then print an error and the usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests/dtls-client -e localhost -r
[0] "tests/dtls-client"
[1] "-e"
[2] "localhost"
[3] "-r"

that's what I get with gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah - I was running this on an older environment. If (for Ubuntu) you add in #define _GNU_SOURCE at the start of dtls-client.c, then my debug output of the above is true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:-).

My sum-up:

There are different implementations of getopt. And there are also some compiler specific options, which sorts the CLI arguments. If it's sorted, a simpler PR would also do it, but this one also doesn't harm.
If "getopt" doesn't support options after the arguments, this PR can't change that, but doesn't harm. In difference to the implementation before, it would report options after arguments as error.
If the arguments are not sorted before, and "getopt" still works after the arguments, this PR enables to use options after the arguments.

Therefore for me this PR helps to get the "best" out of the plenty implementations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

Comment on lines 497 to 500
} else if (!dst_port) {
dst_port = atoi(argv[optind++]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For code readability, I think this should say /* Optional second argument */

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@boaks boaks force-pushed the support_cli_options_behind_arguments branch from bbe2108 to 025bf55 Compare January 18, 2024 15:27
@boaks boaks added the available on develop Mark PRs (pre-)available only on develop label Feb 7, 2024
@boaks boaks force-pushed the support_cli_options_behind_arguments branch from 025bf55 to 0423efb Compare May 13, 2024 10:05
@boaks boaks force-pushed the support_cli_options_behind_arguments branch from 0423efb to fb33fdb Compare June 2, 2024 15:23
@boaks boaks force-pushed the support_cli_options_behind_arguments branch from fb33fdb to 86b209e Compare August 28, 2024 09:58
Copy link
Contributor

@obgm obgm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. There is only one strange word in a comment.

You could have used getaddrinfo() to also set the port, but this would have required a slightly bigger change in the code.

/* handle arguments */
if (!dst.size) {
/* first argument: destination address */
/* resolve destination address where server should be sent */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose the word "server" in this comment should be something else? Maybe "the data" or "the messages"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"resolve destination address of server where data should be sent"

I will change it ;-).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@boaks boaks force-pushed the support_cli_options_behind_arguments branch from 86b209e to 003053a Compare August 28, 2024 13:43
@boaks boaks merged commit c063d72 into eclipse:main Aug 28, 2024
6 checks passed
@boaks boaks deleted the support_cli_options_behind_arguments branch August 28, 2024 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
available on develop Mark PRs (pre-)available only on develop
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants