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

Add hostname support to pico network driver #1173

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

UncleGrumpy
Copy link
Collaborator

Adds support for setting a unique default device hostname (as the ESP32 platform does), and setting per interface hostname in the interface configuration (honoring the dhcp_hostname key value).

closes #1094

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

@bettio
Copy link
Collaborator

bettio commented Jun 30, 2024

@pguyot do you have any opinion about this?

} else {
buf = interop_term_to_string(hostname_term, &ok);
}
if (!ok || IS_NULL_PTR(buf)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if ok is false, we still need to free buf.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(!ok || IS_NULL_PTR(buf)) might also be normalized to (UNLIKELY(!ok || buf != NULL)).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I fixed both comments.

free(psk);
return BADARG_ATOM;
}
ap_hostname = malloc(strlen(buf) + 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am very confused about when this is freed.

Copy link
Collaborator Author

@UncleGrumpy UncleGrumpy Jul 3, 2024

Choose a reason for hiding this comment

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

At the beginning of this section (line 483 new):
char *ap_hostname = driver_data->ap_hostname;

This is freed when the driver is terminated.

void network_driver_destroy(GlobalContext *global)
{
    if (driver_data) {
        if (driver_data->hostname) {
            free(driver_data->hostname);
        }
        if (driver_data->ap_hostname) {
            free(driver_data->ap_hostname);
...

free(psk);
return BADARG_ATOM;
}
hostname = malloc(strlen(buf) + 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am confused about this allocation. When is it freed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

see above.

@@ -705,6 +810,12 @@ void network_driver_init(GlobalContext *global)
void network_driver_destroy(GlobalContext *global)
{
if (driver_data) {
if (driver_data->hostname) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if is not required here AFAIK

Copy link
Collaborator Author

@UncleGrumpy UncleGrumpy Jul 3, 2024

Choose a reason for hiding this comment

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

The per interface hostname resides in driver_data until the driver terminates at which point it is freed, but I realize now I wrote this before I fully understood how the driver_destroy functions work. This code should be placed in network:stop/0 which this driver does not yet support.

In the Pico-SDK netif_set_hostname() is defined as:

#define netif_set_hostname(netif, name) do { if((netif) != NULL) { (netif)->hostname = name; }}while(0)

If the hostname is freed the interface hostname will immediately revert to PicoW, since NULL has the same effect as the hostname being unset.

Copy link
Collaborator Author

@UncleGrumpy UncleGrumpy Jul 3, 2024

Choose a reason for hiding this comment

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

In examples for Pico-SDK that set a hostname it is always hardcoded in a string in flash, so there is no additional memory consumed, but to have this set programmatically we don't have that luxury. Perhaps we could come up with an NVS like solution to store this kind of data?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for the confusion. I meant that:

if (ptr) {
    free(ptr);
}

is equivalent to:

free(ptr);

as free accepts NULL.

@pguyot
Copy link
Collaborator

pguyot commented Jun 30, 2024

@pguyot do you have any opinion about this?

I was a little bit confused by the fact that DNS was involved in the conversation in #1094 and offline, but this PR is a welcome addition. We need to fix the memory management though.

@@ -705,6 +810,12 @@ void network_driver_init(GlobalContext *global)
void network_driver_destroy(GlobalContext *global)
{
if (driver_data) {
if (driver_data->hostname) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for the confusion. I meant that:

if (ptr) {
    free(ptr);
}

is equivalent to:

free(ptr);

as free accepts NULL.

}
return BADARG_ATOM;
}
driver_data->ap_hostname = malloc(strlen(buf) + 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could use strdup(3) which would do the computation of the length, the allocation and the copy in a single call. Alternatively, if it's not available in some SDKs (it's part of C89), you could save the length to avoid computing it twice.

free(psk);
return BADARG_ATOM;
}
driver_data->hostname = malloc(strlen(buf) + 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could use strdup(3) which would do the computation of the length, the allocation and the copy in a single call. Alternatively, if it's not available in some SDKs (it's part of C89), you could save the length to avoid computing it twice.

free(ssid);
free(psk);
return OUT_OF_MEMORY_ATOM;
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's be coherent with what is done on line 293 and remove else here.

free(ssid);
free(psk);
return OUT_OF_MEMORY_ATOM;
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's be coherent with what is done on line 293 and remove else here.

free(ssid);
free(psk);
return OUT_OF_MEMORY_ATOM;
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's be coherent with what is done on line 514 and remove else here.

free(ssid);
free(psk);
return OUT_OF_MEMORY_ATOM;
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's be coherent with what is done on line 514 and remove else here.

@UncleGrumpy UncleGrumpy requested a review from pguyot August 19, 2024 17:54
@UncleGrumpy UncleGrumpy changed the base branch from release-0.6 to main November 6, 2024 19:26
@UncleGrumpy UncleGrumpy marked this pull request as draft November 25, 2024 15:52
@UncleGrumpy UncleGrumpy force-pushed the pico_hostname branch 3 times, most recently from 3d6e4b8 to 08e8831 Compare December 23, 2024 09:25
Adds support for setting `dhcp_hostname` from `sta_config_options()` and/or
`ap_config_options()`, or, if undefined, sets the hostname to the default device
name of `atomvm-${DEVICE_MAC}`. Formerly all pico_w devices tried to register to
an access point with the factory default `PicoW` hostname, which would make all
subsequent pico devices to connect to an access point, after the first one,
unreachable by hostname.

Closes atomvm#1094

Signed-off-by: Winford <[email protected]>
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.

3 participants