On Tue, Jan 21, 2025 at 3:57 AM Eelco Chaudron <[email protected]> wrote:
>
>
>
> On 21 Jan 2025, at 9:01, Roi Dayan wrote:
>
> > On 20/01/2025 17:38, Eelco Chaudron wrote:
> >>
> >>
> >> On 20 Jan 2025, at 16:00, Roi Dayan wrote:
> >>
> >>> ovs_get_program_version() already returns the formatted program name and
> >>> version instead of doing it again.
> >>>
> >>> Signed-off-by: Roi Dayan <[email protected]>
> >>
> >> One small issue below, but we should be able to apply those at commit time.
> >> Did some tests with these change, and with the comment below folded in;
> >
> > ok so not sending v3. thanks.
> >
> >>
> >> Acked-by: Eelco Chaudron <[email protected]>
> >>
> >>> ---
> >>>
> >>> Notes:
> >>> v2
> >>> - Do the same for ovsdb-server.c and ovsdb-error.c.
> >>>
> >>> lib/ovsdb-error.c | 2 +-
> >>> ovsdb/ovsdb-server.c | 3 +--
> >>> vswitchd/bridge.c | 3 +--
> >>> 3 files changed, 3 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/lib/ovsdb-error.c b/lib/ovsdb-error.c
> >>> index 56512fc28dda..036897db0cd7 100644
> >>> --- a/lib/ovsdb-error.c
> >>> +++ b/lib/ovsdb-error.c
> >>> @@ -146,7 +146,7 @@ ovsdb_internal_error(struct ovsdb_error *inner_error,
> >>> ds_put_char(&ds, ')');
> >>> }
> >>>
> >>> - ds_put_format(&ds, " (%s %s)", program_name, VERSION VERSION_SUFFIX);
> >>> + ds_put_format(&ds, " %s", ovs_get_program_version());
> >>
> >> We should keep the () here.
> >
> >
> > I just thought it looks good without the brackets as the last string
> > showing the program name and version without the brackets.
> > looks like this:
> >
> > ovsdb-tool: internal error: ovsdb/log.c:610: fake error
> > (backtrace:./ovsdb/ovsdb-tool(backtrace_capture+0x20) [0x4eee6b],
> > ./ovsdb/ovsdb-tool(ovsdb_internal_error+0x118) [0x4cc38e],
> > ./ovsdb/ovsdb-tool(ovsdb_log_write+0x33) [0x481ad5],
> > ./ovsdb/ovsdb-tool(ovsdb_log_write_and_free+0x22) [0x481afa],
> > ./ovsdb/ovsdb-tool() [0x478cf9], ./ovsdb/ovsdb-tool() [0x478e9f],
> > ./ovsdb/ovsdb-tool() [0x478f99], ./ovsdb/ovsdb-tool() [0x4b157a],
> > ./ovsdb/ovsdb-tool(ovs_cmdl_run_command+0x27) [0x4b162a],
> > ./ovsdb/ovsdb-tool(main+0x97) [0x478541],
> > /lib64/libc.so.6(__libc_start_main+0xf2) [0x7ff7ccc4a6a2],
> > ./ovsdb/ovsdb-tool(_start+0x2d) [0x477edd]) ovsdb-tool (Open vSwitch) 3.4.90
>
> Yes, I noticed the same while testing, however, people might have scripts to
> grep for this (hoping the addition of ‘(Open vSwitch)’ is not messing it up
> already).
I'd generally agree, but I hope no one's needed to automate the
handling of internal errors to such an extent. This function is called
very sparringly, and seemingly only on ovsdb bug.
-M
>
> I’ve copied in Mike and Ilya who work more on the ovsdb. Any option on this?
>
> Cheers,
>
> Eelco
>
> >>> if (inner_error) {
> >>> char *s = ovsdb_error_to_string_free(inner_error);
> >>> diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
> >>> index fbc7ad5efe32..7fde858724b8 100644
> >>> --- a/ovsdb/ovsdb-server.c
> >>> +++ b/ovsdb/ovsdb-server.c
> >>> @@ -817,8 +817,7 @@ main(int argc, char *argv[])
> >>> /* ovsdb-server is usually a long-running process, in which case
> >>> it
> >>> * makes plenty of sense to log the version, but --run makes
> >>> * ovsdb-server more like a command-line tool, so skip it. */
> >>> - VLOG_INFO("%s (Open vSwitch) %s", program_name,
> >>> - VERSION VERSION_SUFFIX);
> >>> + VLOG_INFO("%s", ovs_get_program_version());
> >>> }
> >>>
> >>> unixctl_command_register("exit", "", 0, 0, ovsdb_server_exit,
> >>> &exiting);
> >>> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> >>> index 509ea19ecc99..456b784c01d0 100644
> >>> --- a/vswitchd/bridge.c
> >>> +++ b/vswitchd/bridge.c
> >>> @@ -3471,8 +3471,7 @@ bridge_run(void)
> >>>
> >>> vlog_enable_async();
> >>>
> >>> - VLOG_INFO_ONCE("%s (Open vSwitch) %s", program_name,
> >>> - VERSION VERSION_SUFFIX);
> >>> + VLOG_INFO_ONCE("%s", ovs_get_program_version());
> >>> }
> >>> }
> >>>
> >>> --
> >>> 2.21.0
> >>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
