| Message ID | 20250120150046.1353369-1-roid@nvidia.com |
|---|---|
| State | Accepted |
| Commit | 2472845c39b41473cff56a99bad029bc6c85489b |
| Delegated to: | Eelco Chaudron |
| Headers | show |
| Series | [ovs-dev,v2] general: Use ovs_get_program_version(). | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
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 <roid@nvidia.com> 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; Acked-by: Eelco Chaudron <echaudro@redhat.com> > --- > > 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. > > 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
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 <roid@nvidia.com> > > 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 <echaudro@redhat.com> > >> --- >> >> 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 > >> >> 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 >
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 <roid@nvidia.com> >> >> 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 <echaudro@redhat.com> >> >>> --- >>> >>> 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’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 >>
On Tue, Jan 21, 2025 at 3:57 AM Eelco Chaudron <echaudro@redhat.com> 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 <roid@nvidia.com> > >> > >> 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 <echaudro@redhat.com> > >> > >>> --- > >>> > >>> 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 > >> >
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 <roid@nvidia.com> Thanks Roi and Mike, I’ve applied the patch to main. //Eelco
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()); 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()); } }
ovs_get_program_version() already returns the formatted program name and version instead of doing it again. Signed-off-by: Roi Dayan <roid@nvidia.com> --- 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(-)