Resources cleanup upon using received data

[ferran]

Hi,

I forgot to mention that the callback, when called, is always called
twice. However, I don't free any resource inside the callback, but after it.

I can't figure out how freeing the socket after the callback can prevent
the callback itself to be called.

Regards,
Ferran

On 15/06/15 20:31, ferran wrote:
> Hi Dan,
> Thank you for your kind reply.
>
> I agree that the nice way to go would be to reuse the same socket, but I
> can't access to the upper level, in the example, "caller.c". It's part
> of a wider project, and I'm afraid that it would break a level of
> abstraction.
>
> I have to add information: "get_wireless_stats" is called once per
> wireless device each 10 seconds. Therefore, "caller.c", in practice, is
> doing:
>
> int main ()
>     struct wireless_stats ofs;
>     while (true)
>     {
>         for_each_dev(device)
>         {
>             get_wireless_stats(device, &ofs);
>         }
>         sleep_seconds(10);
>     }
> return 0;
>
> Where "device" is a string that is translated with "if_nametoindex"
> inside "get_wireless_stats". Each device is queried one after the other
> without threading or alike.
>
> This said, I tried again with more verbose logging and freed the msg and
> socket resources before finishing "get_wireless_stats". I get the stats
> doing :
> ...
>
>     nlsmg_free(msg);
>     nl_close(sk);
>     return 0;
>     
> nla_put_failure:
>     nlmsg_free(msg);
>     nl_close(sk);
>     return err;
> }
>
> But if I do "nl_socket_free(sk)" instead of "nl_close(sk)", then the
> callback isn't called, and the stats don't get copied to the target
> structure. Does it make any sense to you?
>
> Regards,
> Ferran
>
> On 12/06/15 18:33, Dan Williams wrote:
>> On Fri, 2015-06-12 at 17:24 +0200, ferran wrote:
>>> Hello everyone,
>>> I have some issues at managing resources used to query wireless
>>> statistics, I hope you can help me.
>>>
>>> Currently I successfully query cfg80211 to get statistics about wireless
>>> devices such as tx bitrate, every 10 seconds. My problem is that I have
>>> been unable to do this without leaking file descriptors and memory. The
>>> code I use is inherited and I am trying to fix it, but I can't manage to
>>> release the resources without losing the retrieved data. Until now, I
>>> have read the documentation, compared with implementation examples found
>>> here and there, and consulted frequently the doxygen reference. However,
>>> I still need some more help.
>>>
>>> Issue 1: 'nl_recvmsgs_default()' will block until receiving a message if
>>> socket is configured as blocking (default), but won't wait for the
>>> callback to finish, right? So how could I possibly release the resources
>>> just after nl_recvmsgs if the callback needs the received data?
>> nl_recvmsgs_default() will block and process messages as long as (a)
>> there are messages in the receive buffer and (b) your callback returns
>> NL_SKIP.  It will wait for your callback to finish before proceeding.
>>
>> One thing I notice: there isn't a lot of sense in doing all the setup
>> every 10 seconds.  You might as well just do the setup once and then
>> every 10s do the nl_msg_alloc/send/recv.  
>>
>>> Issue 2: I have sumarized  the code so that you can check the run flow
>>> and the memory management, it's posted in pastebin:
>>> http://pastebin.com/EYADmDpy
>>> Could you have a look to it, please? It's less than 100 lines, comments
>>> included. How should I release the resources used by the nl socket and
>>> nl message?
>> The code you've got isn't freeing either 'msg' or 'sk', so you're
>> leaking both of those every 10 seconds.  How about something like this;
>> alternatively you can just make sure that both the normal and
>> nla_put_failure paths in get_wireless_stats() free both 'msg' and 'sk'.
>>
>> //// ----------- caller.c ------------------
>> /* includes */
>> int main ()
>>     struct wireless_stats ofs;
>>     struct nl_sock *sk;
>>     int family_id;
>>     int err;
>>
>>     sk = nl_socket_alloc();
>>     genl_connect(sk);
>>
>>     // WARNING, the docs say we must call genl_family_put() later and we don't!    
>>     family_id = genl_ctrl_resolve(sk, "nl80211");
>>     
>>     nl_socket_modify_cb(sk, NL_CB_VALID, NL_CB_CUSTOM,
>>         stats_handler, NULL);
>>
>>     while (true)
>>     {
>>         get_wireless_stats(sk, &ofs);
>>         /* use retrieved data */
>>     
>>         sleep_seconds(10);
>>     }
>>
>>     nl_socket_free(sk);
>> return 0;
>>
>> //// --------- wireless_stats.c -----------
>>
>> void get_wireless_stats(struct nl_socket *sk, struct wireless_stats *ofs)
>> {
>>     struct nl_msg *msg;
>>     int err;
>>
>>     shared_stats = ofs;
>>     
>>     msg = nlmsg_alloc();
>>     genlmsg_put(msg, 0, 0, family_id, 0, NLM_F_DUMP, NL80211_CMD_GET_STATION, 0;
>>     NLA_PUT_U32(msg, NL80211_ATTR_IFINDEX, if_nametoindex("mesh0"));
>>     err = nl_send_auto_complete(sk, msg);
>>     nl_recvmsgs_default(sk);
>>     nlsmg_free(msg);
>>     return 0;
>>     
>>     // Not sure who comes here, but it isn't part of the success workflow, and is required at compilation time.
>>     // -> the NLA_PUT_U32 macro has a 'goto nla_put_failure' in case there isn't enough memory
>> nla_put_failure:
>>     nlmsg_free(msg);
>>     return err;
>> }
>>
>> <no changes to stats_handler>
>>
>