Resources cleanup upon using received data

[ferran]

Hi,

I just wanted to note that assigning a fixed port number to the recently
allocated socket solved the problem. For some reason, nl_msg_alloc
returned a pointer to an already open socket, instead of a new one.

Actually, nl_connect always returned NLE_EXIST, but it wasn't a problem
when not freeing the socket every time.

In short, you can omit this issue, another part of the project was
interferring with mine.

Thank you for the help!

Best regards,
Ferran

On 18/06/15 15:50, ferran wrote:
> Hi there,
>
> I have advanced on this. The reason why the callback isn't executed is
> that actually the socket hasn't been allocated. Capturing the error, I
> get error code -6, that is,  NLE_EXIST, "Object exists" and system's
> EEXIST or EADDRINUSE.
>
> When I didn't do nl_socket_free and consequently, didn't close the
> socket, nl lib kept assigning me sockets with different local ports.
> However, if I do free the socket, for some nl lib tries to assign me a
> socket with a local port that is still in use. This is my problem.
>
> For some reason the library thinks that a local port is available when
> it really isn't. What could motivate this misbehavior?
>
> Regards,
> Ferran
>
> On 15/06/15 21:16, ferran wrote:
>> 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>
>>>>
>