gluon independent respondd-wifi package #217
Conversation
|
@genofire have you changed anything besides the name? |
|
Remove a lot of parts, which ware not necessary to run it in a minimal version to keep fit with the current state of gluon. (but yes, it is nearly the same then #188) |
There was a problem hiding this comment.
I've found a number of issues; unfortunately, some of them will require conceptual changes. I don't think we want to iterate over all wireless interfaces anywhere in Gluon - it should always be limited to the interfaces we actually use for meshing, or the interfaces we use for mesh clients. This makes a respondd provider that is completely independent of Gluon's internals infeasible.
|
|
||
| clients = json_object_new_object(); | ||
| if (!clients) { | ||
| json_object_put(wireless); |
There was a problem hiding this comment.
There is no wireless variable in this function, did you compile test this?
|
|
||
| while (ifaces != NULL) { | ||
|
|
||
| if(ifaces->type != NL80211_IFTYPE_ADHOC) |
There was a problem hiding this comment.
What about MESH_POINT?
Also, missing space before opening parenthesis.
| free(freeptr); | ||
| } | ||
|
|
||
| //TODO maybe skip: if (wifi24 > 0 || wifi5 > 0) { |
There was a problem hiding this comment.
I think it would make sense to skip if no AP interfaces were found at all.
| //TODO maybe skip: if (wifi24 > 0 || wifi5 > 0) { | ||
| json_object_object_add(clients, "wifi24", json_object_new_int(wifi24)); | ||
| json_object_object_add(clients, "wifi5", json_object_new_int(wifi5)); | ||
| json_object_object_add(result, "clients", clients); |
There was a problem hiding this comment.
Hmm, I'm not sure if it really makes sense to count the clients in a generic respondd module after all. Since freifunk-gluon/gluon@b850fff, "wifi" is computed from "wifi24" and "wifi5" as well - which would need to be moved into this module as well; but the computation of "total" depends on "wifi" now.
A different approach would be to move this query into a reusable library included by both gluon-mesh-... respondd providers...
| } | ||
|
|
||
| if (ifaces->frequency) | ||
| json_object_object_add(station, "frequency", json_object_new_int(ifaces->frequency)); |
There was a problem hiding this comment.
The frequency does not belong in the "neighbours" respondd object. If anything, nodeinfo would be the correct place (but let's not discuss extensions of the current format in this PR, but rather keep the exact output that is currently produced by gluon-mesh-*)
| ifaces = get_ifaces(); | ||
| while (ifaces != NULL) { | ||
| if(ifaces->type != NL80211_IFTYPE_AP) | ||
| goto next_statistics; |
There was a problem hiding this comment.
This would also include AP interfaces that are not used for mesh clients (for example the private-wifi interface). Another reason to only provide this as a library for the gluon-mesh-* respondd providers rather than an independent module.
| next_statistics: ; | ||
| void *freeptr = ifaces; | ||
| ifaces = ifaces->next; | ||
| free(freeptr); |
There was a problem hiding this comment.
The memory handling in both loops processing the get_ifaces() output is quite ugly. I think a for_each_iface() taking a callback function would be much nicer, as we could avoid building a list of all interfaces in memory.
|
@genofire do you plan to work on the feedback you got? |
any feedback possible? :) |
|
oh sry, miss it -> my fault - was just looking for #189 and thought it was not anymore possible to disable it in batman that it does not have any collision .... I will work on it on Monday |
No description provided.