| Message ID | 1552941994-1839-1-git-send-email-greearb@candelatech.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | neighbor: Allow removing neigh by bssid alone. | expand |
On Mon, Mar 18, 2019 at 01:46:34PM -0700, greearb@candelatech.com wrote: > Let users delete a neighbor by BSSID alone if they prefer. > The underlying code already properly handled a null SSID, so > just relax the CLI calling restrictions. Thanks, applied. > And, if user tries to delete a neigh entry that is already deleted, > return OK instead of failure. But I dropped this part since it seems to be completely separate item from the main change and would change the existing interface in a manner that does not seem to be necessary and may be unexpected and furthermore, it breaks the rrm_neighbor_db test case.
On Thu, Dec 26, 2019 at 08:35:16AM -0800, Ben Greear wrote: > On 12/26/2019 02:05 AM, Jouni Malinen wrote: > > On Mon, Mar 18, 2019 at 01:46:34PM -0700, greearb@candelatech.com wrote: > > > And, if user tries to delete a neigh entry that is already deleted, > > > return OK instead of failure. > > > > But I dropped this part since it seems to be completely separate item > > from the main change and would change the existing interface in a manner > > that does not seem to be necessary and may be unexpected and > > furthermore, it breaks the rrm_neighbor_db test case. > > Would you consider a version of this patch that took an extra 'force' > or similar argument? While automating this, we would rather the > delete not fail if the target is already gone: The system is already in > the desired state, so automation should not need to deal with any > extra fault recovery in this state. Why would this extra complexity be needed in hostapd instead of the tool that issues these remove operations for not existing entries simply ignore the result code? The "target is already gone" is not really accurate, i.e., the target might never have been in the first place.. This sounds like a pretty special case and I'd have the caller ignore the result for such a special case; adding " force" or something similar to the command just to hardcode the result value to be "OK" seems excessive.
On Thu, Dec 26, 2019 at 09:14:22AM -0800, Ben Greear wrote: > I can check the code again when I get a chance, but from what I recall, > it was not convenient to make sure we removed entries exactly once in > all cases, so our code ended up doing duplicated removes in some cases. > It was easier to make hostap ignore the duplicate removes instead > of parsing errors to figure out if the error was related to removing > a non-existing entity or not. You don't need to remove the entries exactly once; you'd need to simply ignore the response value (FAIL or OK).. If you want to remove all added entries, it would be simpler to add "REMOVE_NEIGHBOR *" which would remove all neighbor DB entries except for the automatically added local entry and return "OK" regardless of whether entries were removed.
On 12/26/2019 02:05 AM, Jouni Malinen wrote: > On Mon, Mar 18, 2019 at 01:46:34PM -0700, greearb@candelatech.com wrote: >> Let users delete a neighbor by BSSID alone if they prefer. >> The underlying code already properly handled a null SSID, so >> just relax the CLI calling restrictions. > > Thanks, applied. > >> And, if user tries to delete a neigh entry that is already deleted, >> return OK instead of failure. > > But I dropped this part since it seems to be completely separate item > from the main change and would change the existing interface in a manner > that does not seem to be necessary and may be unexpected and > furthermore, it breaks the rrm_neighbor_db test case. Would you consider a version of this patch that took an extra 'force' or similar argument? While automating this, we would rather the delete not fail if the target is already gone: The system is already in the desired state, so automation should not need to deal with any extra fault recovery in this state. Thanks, Ben
On 12/26/2019 08:03 AM, Jouni Malinen wrote: > On Thu, Dec 26, 2019 at 08:35:16AM -0800, Ben Greear wrote: >> On 12/26/2019 02:05 AM, Jouni Malinen wrote: >>> On Mon, Mar 18, 2019 at 01:46:34PM -0700, greearb@candelatech.com wrote: >>>> And, if user tries to delete a neigh entry that is already deleted, >>>> return OK instead of failure. >>> >>> But I dropped this part since it seems to be completely separate item >>> from the main change and would change the existing interface in a manner >>> that does not seem to be necessary and may be unexpected and >>> furthermore, it breaks the rrm_neighbor_db test case. >> >> Would you consider a version of this patch that took an extra 'force' >> or similar argument? While automating this, we would rather the >> delete not fail if the target is already gone: The system is already in >> the desired state, so automation should not need to deal with any >> extra fault recovery in this state. > > Why would this extra complexity be needed in hostapd instead of the tool > that issues these remove operations for not existing entries simply > ignore the result code? The "target is already gone" is not really > accurate, i.e., the target might never have been in the first place.. > This sounds like a pretty special case and I'd have the caller ignore > the result for such a special case; adding " force" or something similar > to the command just to hardcode the result value to be "OK" seems > excessive. I can check the code again when I get a chance, but from what I recall, it was not convenient to make sure we removed entries exactly once in all cases, so our code ended up doing duplicated removes in some cases. It was easier to make hostap ignore the duplicate removes instead of parsing errors to figure out if the error was related to removing a non-existing entity or not. Either way, it will not be a problem for us to carry this additional patch in our own tree. Thanks, Ben
diff --git a/hostapd/ctrl_iface.c b/hostapd/ctrl_iface.c index f47d21e..ff2fd65 100644 --- a/hostapd/ctrl_iface.c +++ b/hostapd/ctrl_iface.c @@ -2779,6 +2779,7 @@ static int hostapd_ctrl_iface_remove_neighbor(struct hostapd_data *hapd, char *buf) { struct wpa_ssid_value ssid; + struct wpa_ssid_value *ssidp = &ssid; u8 bssid[ETH_ALEN]; char *tmp; @@ -2788,13 +2789,18 @@ static int hostapd_ctrl_iface_remove_neighbor(struct hostapd_data *hapd, } tmp = os_strstr(buf, "ssid="); - if (!tmp || ssid_parse(tmp + 5, &ssid)) { - wpa_printf(MSG_ERROR, - "CTRL: REMOVE_NEIGHBORr: Bad or missing SSID"); - return -1; + if (!tmp) { + ssidp = NULL; + } + else { + if (ssid_parse(tmp + 5, &ssid)) { + wpa_printf(MSG_ERROR, + "CTRL: REMOVE_NEIGHBORr: Bad SSID"); + return -1; + } } - return hostapd_neighbor_remove(hapd, bssid, &ssid); + return hostapd_neighbor_remove(hapd, bssid, ssidp); } diff --git a/hostapd/hostapd_cli.c b/hostapd/hostapd_cli.c index 4b1c877..14d0c3b 100644 --- a/hostapd/hostapd_cli.c +++ b/hostapd/hostapd_cli.c @@ -1316,13 +1316,13 @@ static int hostapd_cli_cmd_remove_neighbor(struct wpa_ctrl *ctrl, int argc, char cmd[400]; int res; - if (argc != 2) { - printf("Invalid remove_neighbor command: needs 2 arguments\n"); + if (argc < 1) { + printf("Invalid remove_neighbor command: needs at least bssid specified.\n"); return -1; } res = os_snprintf(cmd, sizeof(cmd), "REMOVE_NEIGHBOR %s %s", - argv[0], argv[1]); + argv[0], (argc > 1) ? argv[1] : ""); if (os_snprintf_error(sizeof(cmd), res)) { printf("Too long REMOVE_NEIGHBOR command.\n"); return -1; @@ -1637,7 +1637,7 @@ static const struct hostapd_cli_cmd hostapd_cli_commands[] = { { "show_neighbor", hostapd_cli_cmd_show_neighbor, NULL, " = show neighbor database entries" }, { "remove_neighbor", hostapd_cli_cmd_remove_neighbor, NULL, - "<addr> <ssid=> = remove AP from neighbor database" }, + "<addr> [ssid=] = remove AP from neighbor database" }, { "req_lci", hostapd_cli_cmd_req_lci, hostapd_complete_stations, "<addr> = send LCI request to a station"}, { "req_range", hostapd_cli_cmd_req_range, NULL, diff --git a/src/ap/neighbor_db.c b/src/ap/neighbor_db.c index f54b065..451716d 100644 --- a/src/ap/neighbor_db.c +++ b/src/ap/neighbor_db.c @@ -181,7 +181,7 @@ int hostapd_neighbor_remove(struct hostapd_data *hapd, const u8 *bssid, nr = hostapd_neighbor_get(hapd, bssid, ssid); if (!nr) - return -1; + return 0; /* already gone, treat as success */ hostapd_neighbor_clear_entry(nr); dl_list_del(&nr->list);