Resources cleanup upon using received data

[ferran]

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