On 3/19/24 15:17, Eelco Chaudron wrote:
>
>
> 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, [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?
>>
>> 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 :)
The main problem is that we shouldn't fail commands after they are
already executed. It may work for 'show' commands, but it's not
a thing that can be done for 'set' commands.
The suggestion below shouldn't be confusing for scripts.
>
>> 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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
