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.

Sign up for GitHub

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
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 36 additions & 17 deletions tests/dtls-client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ usage( const char *program, const char *version) {
program = ++p;

fprintf(stderr, "%s v%s -- DTLS client implementation\n"
"(c) 2011-2014 Olaf Bergmann <[email protected]>\n\n"
"(c) 2011-2024 Olaf Bergmann <[email protected]>\n\n"
#ifdef DTLS_PSK
"usage: %s [-c cipher suites] [-e] [-i file] [-k file] [-o file]\n"
" %*s [-p port] [-r] [-v num] addr [port]\n",
Expand Down Expand Up @@ -411,6 +411,7 @@ int
main(int argc, char **argv) {
fd_set rfds, wfds;
struct timeval timeout;
unsigned short dst_port = 0;
unsigned short port = DEFAULT_PORT;
char port_str[NI_MAXSERV] = "0";
log_t log_level = DTLS_LOG_WARN;
Expand All @@ -423,7 +424,7 @@ main(int argc, char **argv) {
size_t len = 0;
int buf_ready = 0;


memset(&dst, 0, sizeof(session_t));
dtls_init();
snprintf(port_str, sizeof(port_str), "%d", port);

Expand All @@ -434,7 +435,8 @@ main(int argc, char **argv) {
memcpy(psk_key, PSK_DEFAULT_KEY, psk_key_length);
#endif /* DTLS_PSK */

while ((opt = getopt(argc, argv, "c:eo:p:rv:" PSK_OPTIONS)) != -1) {
while (optind < argc) {
opt = getopt(argc, argv, "c:eo:p:rv:z" PSK_OPTIONS);
switch (opt) {
#ifdef DTLS_PSK
case 'i' :
Expand Down Expand Up @@ -482,31 +484,48 @@ main(int argc, char **argv) {
case 'v' :
log_level = strtol(optarg, NULL, 10);
break;
case -1 :
/* handle arguments */
if (!dst.size) {
/* first argument: destination address */
/* resolve destination address of server where data should be sent */
res = resolve_address(argv[optind++], &dst.addr.sa);
if (res < 0) {
dtls_emerg("failed to resolve address\n");
exit(-1);
}
dst.size = res;
} else if (!dst_port) {
/* second argument: destination port (optional) */
dst_port = atoi(argv[optind++]);
} else {
dtls_warn("too many arguments!\n");
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 •
edited
Loading

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.

default:
usage(argv[0], dtls_package_version());
exit(1);
}
}

dtls_set_log_level(log_level);

if (argc <= optind) {
if (!dst.size) {
dtls_warn("missing destination address!\n");
usage(argv[0], dtls_package_version());
exit(1);
}

memset(&dst, 0, sizeof(session_t));
/* resolve destination address where server should be sent */
res = resolve_address(argv[optind++], &dst.addr.sa);
if (res < 0) {
dtls_emerg("failed to resolve address\n");
exit(-1);
if (!dst_port) {
/* destination port not provided, use default */
dst_port = DEFAULT_PORT;
}
if (dst.addr.sa.sa_family == AF_INET6) {
dst.addr.sin6.sin6_port = htons(dst_port);
} else {
dst.addr.sin.sin_port = htons(dst_port);
}
dst.size = res;

/* use port number from command line when specified or the listen
port, otherwise */
dst.addr.sin.sin_port = htons(atoi(optind < argc ? argv[optind++] : port_str));
dtls_set_log_level(log_level);

/* init socket and set it to non-blocking */
fd = socket(dst.addr.sa.sa_family, SOCK_DGRAM, 0);
Expand Down
Loading