On 19 Mar 2024, at 16:19, Ilya Maximets wrote:
> 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, 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 :)
>>>
>>> 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.
I was looking at this from a single transaction perspective, and then I’m fine
with it failing. If you ask for json on the list-command and it does not
support it, the error would be clear enough. Not sure why you would feel like
this is ugly.
But you are right, in the context of keeping the socket open and doing multiple
transactions, this might not be desirable. If we go for the below option, we
can have a WARNING on stderr that the command does not support JSON output, so
it uses the “plain” output method.
>>
>>> 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
