Hi Sjur, > > > +static GSList *pending_requests; > > > +static GIOChannel *channel; > > > +static guint32 rtnl_seqnr; > > > +static guint rtnl_watch; > > > > To be fair, you don't need both, the GIOChannel and the watch. You can > > just add the watch and then unref the GIOChannel and set it to close on > > unref. That way when the watch gets removed or you remove it, the > > GIOChannel and the underlaying socket will be automatically closed as > > well. > .... > > As described above. Just unref the GIOChannel and forget about it. And > > then you can make the GIOChannel a local variable. > > I think I have to keep the channel, because I do need the > file descriptor derived from the channel when doing sendto.
fair enough. Anything wrong with just using g_io_channel_write_chars and setting the channel to not-buffered and no-encoding? > > > > You don't really have to play this append/remove game. You can just > > > > append it after send_rtnl_req succeed. The read goes via the > > mainloop > > > > anyway and we are a single threaded program. So the order is > > > > guaranteed. > ... > > You don't need to append it first, send the message, check error and in > > case of an error remove it again. > > OK, thanks - now I get what your after. I completely misunderstood your > previous comment, my bad. That is what I figured. I think that I wasn't clear enough here. > > And while we are at it. Essentially you also need to use asynchronous > > and non-blocking watch driven sending of your message in the first > > place. I did forget to check if you open the RTNL socket non-blocking, > > but you should be doing that. > > So I'll add something like this then: > fl = fcntl(sk, F_GETFL); > fcntl(sk, F_SETFL, fl | O_NONBLOCK); You can also just use the one from GIOChannel to do this since you wrap it into a GIOChannel right away anyway. > > > + } > > > + > > > + g_slist_free(pending_requests); > > > + pending_requests = NULL; > > > > No need to set pending_requests to NULL here. > > But I will have to move this to the caif_rtnl_init function then, > I think pending_requests needs to be NULL before using it. That is given by the fact that it is a global static variable. And when the exit gets called, we are ending the ofonod. Not a big thing. Just a minor nitpick. And if you look you will find cases where even I kept doing it ;) > > > > I am not really sold on this param stuct thingy btw. Why do you > > need > > > > them? Just proper input params and output params for the callback > > > > handler would do them same. Won't they? > > > > > > Yes, I could do that. But in that case I would prefer to typedef the > > > response function to simplify create function's signature. Otherwise > > > I think the create function's signature gets too complex. > > > > typedef for function callbacks are just fine. I have no problem with > > that at all. > > I got rid of the structs and typedef'ed response function - I agree this > better. > > <snip> > typedef void (*rtnl_create_cb_t) (int result, gpointer user_data, > char *ifname, int ifindex); > > extern int caif_rtnl_create_interface(gpointer user_data, int type, int > connid, > gboolean loop, rtnl_create_cb_t cb); > > > Hoping to get the next patch out later today. Please don't use rtnl_ prefix. That is not your namespace. So it should be caif_rtnl_create_cb. Regards Marcel _______________________________________________ ofono mailing list [email protected] http://lists.ofono.org/listinfo/ofono
