On 19 Mar 2024, at 15:00, Ilya Maximets wrote:
> On 3/19/24 14:41, Jakob Meng wrote:
>>
>>
>> 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, jm...@redhat.com 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?
>
> I'm actually not sure why we need to register them differently in the
> first palace. Make the callback check what was the requested format
> and use appropriate reply function.
I guess this can be done with the unixctl_command_get_output_format() function?
> Why we need to fail the requests
> for callbacks that don't support JSON replies? Why can't we just
> pack the plain reply into a JSON string?
I don’t like this, this way we will not know if the command actually supports
native JSON or not. Else we might confuse scripts in the future if we start
sending some JSONized responses. returning an error might also trigger people
to submit a patch to convert more commands to native JSON support :)
> We could also invent a new format for the case where JSON format is
> requested, but the callback doesn't support it. E.g. by creating a
> reply like this: {"reply-format": "plain", "reply": "<plain text>"}
> (for the plain text replies only).
>>
>>
>>
>>>
>>>>>>> Some small style comments highlighted below.
>>>>>> I would like to have your eagle eyes...👀 Fixed them!
>>>>>>
>>>>>> Thanks,
>>>>>> Jakob
>>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
