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

some small bug fixes and add the possibility to build without using t… #44

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lherschi
Copy link

…estparm

wsd.c:
With the change Dolphin shows the hostname and not only the IP. Content-Type can additionally contain one or more parameters separated by semicolon. Dolphin also sends the charset among other things.

wsdd.h:
I had to remove the include because otherwise the following build error comes.
error: redefinition of 'struct sockaddr_in'
netinet/in.h and linux/in.h should never be used together. linux/in.h is for kernel builds and netinet/in.h is for userland builds. Also, in wsdd2.c _GNU_SOURCE is defined and thus struct ip_mreqn is available via bits/in.h.

wsdd2.c:
Here my changes should be self-explanatory.

@Andy2244 Andy2244 self-requested a review January 26, 2023 21:04
@Andy2244 Andy2244 self-assigned this Jan 26, 2023
@marcosfrm
Copy link
Contributor

Please list the new options in wsdd2.c's help() output and in wsdd2.8.

@lherschi
Copy link
Author

There are no new options.

@marcosfrm
Copy link
Contributor

Command line options -A and -B?

@lherschi
Copy link
Author

Please take a closer look at the code, they are not new.

4c4b44a

@marcosfrm
Copy link
Contributor

Right, but they were not implemented before. 1.8.7:

wsdd2[2066]: /usr/sbin/wsdd2: invalid option -- 'A'
wsdd2[2066]: Bad option '?'
wsdd2[2066]: WSDD and LLMNR daemon
wsdd2[2066]: Usage: wsdd2 [options]
...
wsdd2[2165]: /usr/sbin/wsdd2: invalid option -- 'B'
wsdd2[2165]: Bad option '?'
wsdd2[2165]: WSDD and LLMNR daemon
wsdd2[2165]: Usage: wsdd2 [options]
...

(the Bad option message is useless btw)

wsdd2.8 needs update still.

@lherschi
Copy link
Author

What is currently preventing the merge?

@marcosfrm
Copy link
Contributor

It would be better command line options override the testparm calls. You could drop WITHOUT_TESTPARM define then. In main(), moving init_sysinfo() after command line parsing would allow that.

Is not this enough?

diff --git a/wsdd2.c b/wsdd2.c
index 8f81936..52141a3 100644
--- a/wsdd2.c
+++ b/wsdd2.c
@@ -691,9 +691,7 @@ int main(int argc, char **argv)
 	const char *prog = basename(argv[0]);
 	unsigned int ipv46 = 0, tcpudp = 0, llmnrwsdd = 0;
 
-	init_sysinfo();
-
-	while ((opt = getopt(argc, argv, "hd46utlwLWi:H:N:G:b:")) != -1) {
+	while ((opt = getopt(argc, argv, "hd46utlwLWi:H:A:N:B:G:b:")) != -1) {
 		switch (opt) {
 		case 'h':
 			help(prog, EXIT_SUCCESS, NULL);
@@ -740,10 +738,18 @@ int main(int argc, char **argv)
 			if (optarg != NULL && strlen(optarg) > 0)
 				hostname = strdup(optarg);
 			break;
+		case 'A':
+			if (optarg != NULL && strlen(optarg) > 0)
+				hostaliases = strdup(optarg);
+			break;
 		case 'N':
 			if (optarg != NULL && strlen(optarg) > 0)
 				netbiosname = strdup(optarg);
 			break;
+		case 'B':
+			if (optarg != NULL && strlen(optarg) > 0)
+				netbiosaliases = strdup(optarg);
+			break;
 		case 'G':
 			if (optarg != NULL && strlen(optarg) > 0)
 				workgroup = strdup(optarg);
@@ -754,7 +760,7 @@ int main(int argc, char **argv)
 					help(prog, EXIT_FAILURE, "Bad key:val '%s'", optarg);
 			break;
 		case '?':
-			if (strchr("iHNGb", optopt))
+			if (strchr("iHANBGb", optopt))
 				printf("Option -%c requires an argument.\n", optopt);
 			/* ... fall through ... */
 		default:
@@ -765,6 +771,8 @@ int main(int argc, char **argv)
 	if (argc > optind)
 		help(prog, EXIT_FAILURE, "Unknown argument '%s'", argv[optind]);
 
+	init_sysinfo();
+
 	if (!ipv46)
 		ipv46 = _4 | _6;
 	if (!llmnrwsdd)

@marcosfrm
Copy link
Contributor

Sorry, init_sysinfo() needs changes:

diff --git a/wsdd2.c b/wsdd2.c
index 8f81936..1e716a8 100644
--- a/wsdd2.c
+++ b/wsdd2.c
@@ -656,24 +656,35 @@ static void init_sysinfo()
 {
 	char hostn[HOST_NAME_MAX + 1];
 
-	if (!hostname && gethostname(hostn, sizeof(hostn) - 1) != 0)
-		err(EXIT_FAILURE, "gethostname");
-
-	char *p = strchr(hostn, '.');
-	if (p) *p = '\0';
-	hostname = strdup(hostn);
+	/* command line options have preference */
+	if (!hostname) {
+		if (gethostname(hostn, sizeof(hostn) - 1) != 0)
+			err(EXIT_FAILURE, "gethostname");
+
+		char *p = strchr(hostn, '.');
+		if (p) *p = '\0';
+		hostname = strdup(hostn);
+	}
 
-	if (!hostaliases && !(hostaliases = get_smbparm("additional dns hostnames", "")))
-		err(EXIT_FAILURE, "get_smbparm");
+	if (!hostaliases) {
+		if (!(hostaliases = get_smbparm("additional dns hostnames", "")))
+			err(EXIT_FAILURE, "get_smbparm");
+	}
 
-	if (!netbiosname && !(netbiosname = get_smbparm("netbios name", hostname)))
-		err(EXIT_FAILURE, "get_smbparm");
+	if (!netbiosname) {
+		if (!(netbiosname = get_smbparm("netbios name", hostname)))
+			err(EXIT_FAILURE, "get_smbparm");
+	}
 
-	if (!netbiosaliases && !(netbiosaliases = get_smbparm("netbios aliases", "")))
-		err(EXIT_FAILURE, "get_smbparm");
+	if (!netbiosaliases) {
+		if (!(netbiosaliases = get_smbparm("netbios aliases", "")))
+			err(EXIT_FAILURE, "get_smbparm");
+	}
 
-	if (!workgroup && !(workgroup = get_smbparm("workgroup", "WORKGROUP")))
-		err(EXIT_FAILURE, "get_smbparm");
+	if (!workgroup) {
+		if (!(workgroup = get_smbparm("workgroup", "WORKGROUP")))
+			err(EXIT_FAILURE, "get_smbparm");
+	}
 
 	init_getresp();
 }
@@ -691,9 +702,7 @@ int main(int argc, char **argv)
 	const char *prog = basename(argv[0]);
 	unsigned int ipv46 = 0, tcpudp = 0, llmnrwsdd = 0;
 
-	init_sysinfo();
-
-	while ((opt = getopt(argc, argv, "hd46utlwLWi:H:N:G:b:")) != -1) {
+	while ((opt = getopt(argc, argv, "hd46utlwLWi:H:A:N:B:G:b:")) != -1) {
 		switch (opt) {
 		case 'h':
 			help(prog, EXIT_SUCCESS, NULL);
@@ -740,10 +749,18 @@ int main(int argc, char **argv)
 			if (optarg != NULL && strlen(optarg) > 0)
 				hostname = strdup(optarg);
 			break;
+		case 'A':
+			if (optarg != NULL && strlen(optarg) > 0)
+				hostaliases = strdup(optarg);
+			break;
 		case 'N':
 			if (optarg != NULL && strlen(optarg) > 0)
 				netbiosname = strdup(optarg);
 			break;
+		case 'B':
+			if (optarg != NULL && strlen(optarg) > 0)
+				netbiosaliases = strdup(optarg);
+			break;
 		case 'G':
 			if (optarg != NULL && strlen(optarg) > 0)
 				workgroup = strdup(optarg);
@@ -754,7 +771,7 @@ int main(int argc, char **argv)
 					help(prog, EXIT_FAILURE, "Bad key:val '%s'", optarg);
 			break;
 		case '?':
-			if (strchr("iHNGb", optopt))
+			if (strchr("iHANBGb", optopt))
 				printf("Option -%c requires an argument.\n", optopt);
 			/* ... fall through ... */
 		default:
@@ -765,6 +782,8 @@ int main(int argc, char **argv)
 	if (argc > optind)
 		help(prog, EXIT_FAILURE, "Unknown argument '%s'", argv[optind]);
 
+	init_sysinfo();
+
 	if (!ipv46)
 		ipv46 = _4 | _6;
 	if (!llmnrwsdd)

@marcosfrm
Copy link
Contributor

help() output will show defaults for -H -A -N -B -G as (null). I do not think run testparm/gethostname() only to show the help text is reasonable. I vote to axe all these strings from help() since they are already documented in the man page.

@lherschi
Copy link
Author

lherschi commented Oct 19, 2023

However, on systems without testparm I am forced to specify a hostalias.

Furthermore, I do not understand the meaning of the following amendment.

-	if (!hostaliases && !(hostaliases = get_smbparm("additional dns hostnames", "")))
-		err(EXIT_FAILURE, "get_smbparm");
+	if (!hostaliases) {
+		if (!(hostaliases = get_smbparm("additional dns hostnames", "")))
+			err(EXIT_FAILURE, "get_smbparm");
+	}

With double amperesand, the second condition is no longer evaluated if the first one already fails.

@marcosfrm
Copy link
Contributor

However, on systems without testparm I am forced to specify a hostalias.

Is it not enough?

Sure, you can keep the short-circuit evaluation. It is my personal preference only.

@lherschi
Copy link
Author

What does "not enough" mean? If I don't need an alias, I don't want to have to make one up.

@marcosfrm
Copy link
Contributor

Sorry, I misread. Right, need the #define. Scratch my suggestions, you patch is minimal and works. WITHOUT_TESTPARM could be documented in README.md.

@oldium
Copy link

oldium commented Apr 12, 2024

I just faced the issue with missing -A and -B options, so I think it would be good to resolve it somehow.

Regarding the WITHOUT_TESTPARM option. It does not change build dependencies/libraries, so I think that having this as a command-line option (to omit using testparm to query values) would make more sense - any user could use it without recompiling the tool.

@lherschi
Copy link
Author

I will no longer make any changes to the pull request. I have been waiting for the commit for a year and three months now and have made all the "required" changes and extensions during this time. Either it will now be committed or not.

@oldium
Copy link

oldium commented Apr 15, 2024

I will do it, no problem. Anyway, I plan to enhance the logic to accept config file value from smb.conf to read a different configuration file (if it exists) and also to allow specifying the configuration file as a command-line parameter. This should fix issue I have. I will also add logic to omit using testparm (plus add missing -A and -B handling), which is a workaround for my issue too.

@lherschi
Copy link
Author

I chose the build-switch for testparm because I want to build it for an embedded system. If a command line switch is used, I always have to set this switch on a system on which there is definitely no testparm. This is exactly what I wanted to avoid.

What functionality is missing from -A and -B? I have already done a lot in this regard in my PR.

@oldium
Copy link

oldium commented Apr 15, 2024

I chose the build-switch for testparm because I want to build it for an embedded system. If a command line switch is used, I always have to set this switch on a system on which there is definitely no testparm. This is exactly what I wanted to avoid.

So just fix usage of testparm - in case the hostname or netbiosname is specified on the command line, make testparm optional (i.e. call it only when it is found).

What functionality is missing from -A and -B? I have already done a lot in this regard in my PR.

Just what you did in this PR - add them to getopt. I expect that when I am finished with my changes, this PR will not be necessary.

@Andy2244 Andy2244 removed their assignment Apr 15, 2024
@oldium
Copy link

oldium commented Apr 16, 2024

Please find here result of 3-hours programming session, it should have everything I wanted (including handling of missing testparm), but I would like to test it more before I create a Pull Request (it is after midnight after all). The original code did not care much about freeing the allocated strings memory, but I kept it like that.

@lherschi
Copy link
Author

I'll do a build test in my cross-compiler environment in the next few days.

@oldium
Copy link

oldium commented Apr 17, 2024

I gave it a quick test and deployed a fix. I will not have time till next week to test it more I wonder.

@oldium
Copy link

oldium commented Apr 17, 2024

I made a draft Pull Request #51 with all the changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants