On 19.03.24 13:22, Ilya Maximets wrote: > On 3/19/24 13:21, Ilya Maximets wrote: >> On 3/19/24 13:17, Eelco Chaudron wrote: >>> >>> On 19 Mar 2024, at 13:11, Jakob Meng wrote: >>> >>>> Hi! >>>> >>>> On 15.03.24 11:19, Eelco Chaudron wrote: >>>>> On 18 Jan 2024, at 16:26, [email protected] wrote: >>>>> >>>>>> ... >>>>> Thank for the patch! What a beast to go trough ;) >>>> Thank you for doing it anyway ☺️ >>>> >>>>> I believe the current approach is acceptable. However, we could also >>>>> incorporate union callbacks: if registration only supports text, we would >>>>> use callback A, and if multiple formats exist, we could employ the new >>>>> style >>>>> callback. This would mitigate the need for a significant overhaul. Just a >>>>> suggestion, as I'm ok with the current approach. :) >>>> This trades many-initial-changes-across-a-lot-of-files for a >>>> more-complex-registration-and-callback-logic. >>>> >>>> We could also make struct unixctl_conn visible (move it from lib/unixctl.c >>>> to lib/unixctl.h) and let callees read the output format from its 'fmt' >>>> member. Then we would not have to change the callback signature at all. >>> Or have a utility function to return the format, which keeps the structure >>> invisible. >> +1, just add some access methods. >> >>>> I do not have a strong opinion here. What do you, Ilya and the others >>>> think? >>> I’ll wait for others to chime in... >> I don't think changing all the callbacks is a good thing to do. >> i.e. this patch should not exist or be very small. > * or it should be very small.
How about unixctl_command_register? Should we change the existing function signature (like I did)? Or add a second function, like unixctl_command_register_fmt? Or should we add another function to set/change supported formats of an already registered command? Example: // Get output format which the user has choosen (defaults to txt) enum ovs_output_fmt unixctl_command_get_output_format(struct unixctl_conn *); // Set output formats that a command supports int unixctl_command_set_output_formats(const char *name, unsigned int output_fmts); Wdyt? > >>>>> Some small style comments highlighted below. >>>> I would like to have your eagle eyes...👀 Fixed them! >>>> >>>> Thanks, >>>> Jakob _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
