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

Jump to bottom

Allow to provide multiple host names #39

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

Allow to provide multiple host names #39

wants to merge 2 commits into from

Conversation

davidgraeff
Copy link

@davidgraeff davidgraeff commented Apr 1, 2021 •
edited

Allow to provide multiple host names

  • Readme amended
  • Dynamic array of host name strings; Requires addition of void llmnr_release(void); method for string array cleanup
  • Keep llmnr_set_hostname function signature. This will always only update the first entry.
  • Adapt llmnr_init to take an array of host name strings.

Fixes #38

@davidgraeff davidgraeff force-pushed the dg_multi_names branch 2 times, most recently from 1e51de4 to 08c2393 Compare April 1, 2021 22:42
tklauser
tklauser reviewed Apr 6, 2021
View reviewed changes
Copy link
Owner

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Thanks. Haven't looked at the PR in detail yet, a few minor comments inline.

Please also add a commit message (not just a PR description) and also add Fixes #38 to it.

I'll give this a closer look some time later this week.

README.md Outdated Show resolved Hide resolved
llmnr.c Outdated Show resolved Hide resolved
llmnr.c Outdated Show resolved Hide resolved
llmnr.c Outdated Show resolved Hide resolved
llmnrd.c Outdated Show resolved Hide resolved
llmnrd.c Show resolved Hide resolved
llmnr.c Outdated Show resolved Hide resolved
llmnr.c Outdated Show resolved Hide resolved
llmnr.c Outdated Show resolved Hide resolved
llmnr.c Outdated Show resolved Hide resolved
David Graeff added 2 commits April 6, 2021 20:42
Allow to provide multiple host names
26c433e 
RFC 4795 allows an IP to be represented by not just one but multiple names.

The "--hostname" / "-H" parameter can be given multiple times now.

The use of a dynamic array of host names require a new public API method:
`llmnr_release(void)`

Keep llmnr_set_hostname function signature.
This will always only update the first entry and is mainly used when no
--hostname parameter has been provided and llmnrd reacts to system hostname
changes.

API changes:
* Adapt llmnr_init to take an array of host name strings.

Signed-off-by: David Graeff <[email protected]>
Address review comments
c432935
tklauser
tklauser requested changes Apr 12, 2021
View reviewed changes
Copy link
Owner

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Sorry for the delay with reviewing this. Some more comments inline, mostly minor formatting nits (yes, I should add a .clang-format file 😃).

In addition to README.md, please also update the usage of the -H option (i.e. that it can be specified multiple times) in the llmnrd.1 man page and in the in-program usage text.

llmnr.c
Comment on lines +66 to +67
size_t i;
llmnr_hostname_count = hostname_count;
Copy link
Owner

Choose a reason for hiding this comment

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

Add an empty line after variable declaration:

Suggested change
size_t i;
llmnr_hostname_count = hostname_count;
size_t i;
llmnr_hostname_count = hostname_count;

llmnr.c
llmnr_set_hostname(hostname);
size_t i;
llmnr_hostname_count = hostname_count;
llmnr_hostnames = xzalloc(hostname_count);
Copy link
Owner

Choose a reason for hiding this comment

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

This should allocate hostname_count * sizeof(*llmnr_hostnames).

llmnr.c
static char llmnr_hostname[LLMNR_LABEL_MAX_SIZE + 2];
#define LLMNR_LABEL_LEN (LLMNR_LABEL_MAX_SIZE + 2)
static char** llmnr_hostnames = NULL;
size_t llmnr_hostname_count = 0;
Copy link
Owner

Choose a reason for hiding this comment

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

Make this static, it's not used outside the file.

llmnr.c
Comment on lines +77 to +78
size_t i;
for(i = 0; i < llmnr_hostname_count; ++i) {
Copy link
Owner

Choose a reason for hiding this comment

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

Add an empty line after variable declaration and add a space after the for keyword:

Suggested change
size_t i;
for(i = 0; i < llmnr_hostname_count; ++i) {
size_t i;
for (i = 0; i < llmnr_hostname_count; ++i) {

llmnr.c
Comment on lines +90 to +91
uint8_t n;
n = llmnr_hostnames[i][0];
Copy link
Owner

Choose a reason for hiding this comment

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

Assign this on declaration:

Suggested change
uint8_t n;
n = llmnr_hostnames[i][0];
uint8_t n = llmnr_hostnames[i][0];

llmnr.c
@@ -196,6 +232,7 @@ static void llmnr_packet_process(int ifindex, const uint8_t *pktbuf, size_t len,
const uint8_t *query;
size_t query_len;
uint8_t name_len;
char* matched_hostname_entry;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
char* matched_hostname_entry;
const char* matched_hostname;

llmnrd.c
Comment on lines +200 to +204
/* First count given (host)names, if any, to allocate memory */
while ((c = getopt_long(argc, argv, short_opts, long_opts, NULL)) != -1) {
if (c == 'H')
++name_count;
}
Copy link
Owner

Choose a reason for hiding this comment

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

I don't really like the idea of iterating the options twice. But I guess there is no alternative to this :-(

llmnrd.c

if (!name_count)
name_count = 1;
hostnames = xzalloc(name_count);
Copy link
Owner

Choose a reason for hiding this comment

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

This should allocate name_count * sizeof(*hostnames).

llmnrd.c
@@ -332,7 +349,11 @@ int main(int argc, char **argv)
close(llmnrd_sock_ipv6);
if (llmnrd_sock_ipv4 >= 0)
close(llmnrd_sock_ipv4);
free(hostname);
for(name_i = 0; name_i < name_count; ++name_i) {
Copy link
Owner

Choose a reason for hiding this comment

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

Add a space after the for keyword:

Suggested change
for(name_i = 0; name_i < name_count; ++name_i) {
for (name_i = 0; name_i < name_count; ++name_i) {

llmnrd.c
log_info("Starting llmnrd on port %u, hostname %s\n", port, hostname);
log_info("Starting llmnrd on port %u. Assigned hostname(s):\n", port);

for(name_i = 0; name_i < name_count; ++name_i)
Copy link
Owner

Choose a reason for hiding this comment

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

Add a space after the for keyword:

Suggested change
for(name_i = 0; name_i < name_count; ++name_i)
for (name_i = 0; name_i < name_count; ++name_i)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tklauser tklauser tklauser requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Respond to multiple names
2 participants
@davidgraeff @tklauser