Skip to content

Allow to provide multiple host names#39

Open
davidgraeff wants to merge 2 commits intotklauser:masterfrom
davidgraeff:dg_multi_names
Open

Allow to provide multiple host names#39
davidgraeff wants to merge 2 commits intotklauser:masterfrom
davidgraeff:dg_multi_names

Conversation

@davidgraeff
Copy link
Copy Markdown

@davidgraeff davidgraeff commented Apr 1, 2021 •
edited
Loading

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
Copy Markdown
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.

Comment thread README.md Outdated
Comment thread llmnr.c Outdated
Comment thread llmnr.c Outdated
Comment thread llmnr.c Outdated
Comment thread llmnrd.c Outdated
Comment thread llmnrd.c
Comment thread llmnr.c Outdated
Comment thread llmnr.c Outdated
Comment thread llmnr.c Outdated
Comment thread llmnr.c Outdated
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 <david.graeff@web.de>
Address review comments
c432935
@davidgraeff davidgraeff requested a review from tklauser April 11, 2021 13:05
tklauser
tklauser requested changes Apr 12, 2021
View reviewed changes
Copy link
Copy Markdown
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.

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

@tklauser tklauser Apr 12, 2021

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;

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

@tklauser tklauser Apr 12, 2021

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).

Comment thread 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
Copy Markdown
Owner

@tklauser tklauser Apr 12, 2021

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.

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

@tklauser tklauser Apr 12, 2021

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) {

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

@tklauser tklauser Apr 12, 2021

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];

Comment thread llmnr.c
const uint8_t *query;
size_t query_len;
uint8_t name_len;
char* matched_hostname_entry;
Copy link
Copy Markdown
Owner

@tklauser tklauser Apr 12, 2021

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;

Comment thread 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
Copy Markdown
Owner

@tklauser tklauser Apr 12, 2021

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 :-(

Comment thread llmnrd.c

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

@tklauser tklauser Apr 12, 2021

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).

Comment thread llmnrd.c
if (llmnrd_sock_ipv4 >= 0)
close(llmnrd_sock_ipv4);
free(hostname);
for(name_i = 0; name_i < name_count; ++name_i) {
Copy link
Copy Markdown
Owner

@tklauser tklauser Apr 12, 2021

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) {

Comment thread 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
Copy Markdown
Owner

@tklauser tklauser Apr 12, 2021

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