On 3/19/24 15:54, Eelco Chaudron wrote:
>
>
> On 19 Mar 2024, at 15:46, Ilya Maximets wrote:
>
>> 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.
>
> I guess this means I would prefer to take a ‘bigger’ change hit and change
> the register procedure (not the actual callback), to include the supported
> output formats. This way we can still error out all the show, set, etc.
> commands that do not support the JSON format before we execute them.
Failing the request because it doesn't support the output format is
also ugly. For example, the list-commands doesn't support it, so
the user that enabled JSON format on the socket will not be able to
even list the commands. I don't think that's a good interface.
I'd rather have just a new command then, e.g. 'dpif/show+json' that
would reply in a JSON format and void all the questionable API changes.
>
>> 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
