On Tue, 30 Apr 2024 18:11:18 +0000 Danielle Ratson wrote: > > Do we want to blast it to all listeners or treat it as an async reply? > > We can save the seq and portid of the original requester and use reply, I > > think. > > I am sorry, I am not sure I understood what you meant here... it > should be an async reply, but not sure I understood your suggestion. > > Can you explain please? Make sure you have read the netlink intro, it should help fill in some gaps I won't explicitly cover: https://docs.kernel.org/next/userspace-api/netlink/intro.html "True" notifications will have pid = 0 and seq = 0, while replies to commands have those fields populated based on the request. pid identifies the socket where the message should be delivered. ethnl_multicast() assumes that it's zero (since it's designed to work for notifications) and sends the message to all sockets subscribed to a multicast / notification group (ETHNL_MCGRP_MONITOR). So that's the background. What you're doing isn't incorrect but I think it'd be better if we didn't use the multicast group here, and sent the messages as a reply - just to the socket which requested the flashing. Still asynchronously, we just need to save the right pid and seq to use. Two reasons for this: 1) convenience, the user space socket won't have to subscribe to the multicast group 2) the multicast group is really intended for notifying about _configuration changes_ done to the system. If there is a daemon listening on that group, there's a very high chance it won't care about progress of the flashing. Maybe we can send a single notification that flashing has been completed but not "progress updates" I think it should work. > > > +void ethnl_module_fw_flash_ntf_err(struct net_device *dev, > > > + char *err_msg, char *sub_err_msg) { > > > + char status_msg[120]; > > > + > > > + if (sub_err_msg) > > > + sprintf(status_msg, "%s, %s.", err_msg, sub_err_msg); > > > + else > > > + sprintf(status_msg, "%s.", err_msg); > > > > Hm, printing in the dot, and assuming sizeof err_msg + sub_err < 116 is a bit > > surprising. But I guess you have a reason... > > > > Maybe pass them separately to ethnl_module_fw_flash_ntf() then you can > > nla_reserve() the right amount of space and sprintf() directly into the skb? > > I can get rid of the dot actually, would it be ok like that? It'd still be better to splice the two strings and the comma directly to the skb, rather than on the stack using a function which doesn't check the bounds of the buffer :S
