| Message ID | 20200407082622.340671-1-numans@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,ovn] ovn-nbctl: Create daemon socket in ovn run dir | expand |
On 4/7/20 10:26 AM, numans@ovn.org wrote: > From: Numan Siddique <numans@ovn.org> > > ovn-nbctl when run as a daemon is creating the ctl socket in > the ovs rundir. This patch fixes this issue by creating it in > the ovn rundir. > > Reported-by: Dan Williams <dcbw@redhat.com> > Signed-off-by: Numan Siddique <numans@ovn.org> Hi Numan, Looks good to me. Acked-by: Dumitru Ceara <dceara@redhat.com> Thanks, Dumitru > --- > utilities/ovn-nbctl.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > index 59abe0051..412796e83 100644 > --- a/utilities/ovn-nbctl.c > +++ b/utilities/ovn-nbctl.c > @@ -6436,7 +6436,19 @@ server_loop(struct ovsdb_idl *idl, int argc, char *argv[]) > > service_start(&argc, &argv); > daemonize_start(false); > - int error = unixctl_server_create(unixctl_path, &server); > + > + char *abs_unixctl_path = NULL; > + if (!unixctl_path) { > + abs_unixctl_path = get_abs_unix_ctl_path(); > + } else { > + abs_unixctl_path = unixctl_path; > + } > + > + int error = unixctl_server_create(abs_unixctl_path, &server); > + if (!unixctl_path) { > + free(abs_unixctl_path); > + } > + > if (error) { > ctl_fatal("failed to create unixctl server (%s)", > ovs_retval_to_string(error)); >
On 4/7/20 10:26 AM, numans@ovn.org wrote: > From: Numan Siddique <numans@ovn.org> > > ovn-nbctl when run as a daemon is creating the ctl socket in > the ovs rundir. This patch fixes this issue by creating it in > the ovn rundir. > > Reported-by: Dan Williams <dcbw@redhat.com> > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > utilities/ovn-nbctl.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > index 59abe0051..412796e83 100644 > --- a/utilities/ovn-nbctl.c > +++ b/utilities/ovn-nbctl.c > @@ -6436,7 +6436,19 @@ server_loop(struct ovsdb_idl *idl, int argc, char *argv[]) > > service_start(&argc, &argv); > daemonize_start(false); > - int error = unixctl_server_create(unixctl_path, &server); > + > + char *abs_unixctl_path = NULL; > + if (!unixctl_path) { > + abs_unixctl_path = get_abs_unix_ctl_path(); > + } else { > + abs_unixctl_path = unixctl_path; But 'unixctl_path' might be not an absolute path. In this case unixctl_server_create() will still place it into ovs_rundir(). So, you need to have: abs_unixctl_path = unixctl_path ? abs_file_name(ovn_rundir(), unixctl_path) : get_abs_unix_ctl_path(); Another and, probably, better approach is to add an optional 'path' argument to get_abs_unix_ctl_path() so it might call abs_file_name() for us. so it will look like: abs_unixctl_path = get_abs_unix_ctl_path(unixctl_path); However, this seems like existing issue for all the OVN processes. None of the ovn applications checks if 'unixctl_path' is an absolute path or not. > + } > + > + int error = unixctl_server_create(abs_unixctl_path, &server); > + if (!unixctl_path) { > + free(abs_unixctl_path); > + } > + > if (error) { > ctl_fatal("failed to create unixctl server (%s)", > ovs_retval_to_string(error)); >
On Tue, Apr 7, 2020 at 2:54 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 4/7/20 10:26 AM, numans@ovn.org wrote: > > From: Numan Siddique <numans@ovn.org> > > > > ovn-nbctl when run as a daemon is creating the ctl socket in > > the ovs rundir. This patch fixes this issue by creating it in > > the ovn rundir. > > > > Reported-by: Dan Williams <dcbw@redhat.com> > > Signed-off-by: Numan Siddique <numans@ovn.org> > > --- > > utilities/ovn-nbctl.c | 14 +++++++++++++- > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > > index 59abe0051..412796e83 100644 > > --- a/utilities/ovn-nbctl.c > > +++ b/utilities/ovn-nbctl.c > > @@ -6436,7 +6436,19 @@ server_loop(struct ovsdb_idl *idl, int argc, char *argv[]) > > > > service_start(&argc, &argv); > > daemonize_start(false); > > - int error = unixctl_server_create(unixctl_path, &server); > > + > > + char *abs_unixctl_path = NULL; > > + if (!unixctl_path) { > > + abs_unixctl_path = get_abs_unix_ctl_path(); > > + } else { > > + abs_unixctl_path = unixctl_path; > > But 'unixctl_path' might be not an absolute path. In this case > unixctl_server_create() will still place it into ovs_rundir(). > So, you need to have: > > abs_unixctl_path = unixctl_path ? abs_file_name(ovn_rundir(), unixctl_path) > : get_abs_unix_ctl_path(); > > Another and, probably, better approach is to add an optional 'path' argument to > get_abs_unix_ctl_path() so it might call abs_file_name() for us. > so it will look like: > > abs_unixctl_path = get_abs_unix_ctl_path(unixctl_path); > > > However, this seems like existing issue for all the OVN processes. > None of the ovn applications checks if 'unixctl_path' is an absolute path or not. Thanks Ilya and Dumitru. I addressed this in the v2. Request to take a look at it. Thanks Numan > > > + } > > + > > + int error = unixctl_server_create(abs_unixctl_path, &server); > > + if (!unixctl_path) { > > + free(abs_unixctl_path); > > + } > > + > > if (error) { > > ctl_fatal("failed to create unixctl server (%s)", > > ovs_retval_to_string(error)); > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c index 59abe0051..412796e83 100644 --- a/utilities/ovn-nbctl.c +++ b/utilities/ovn-nbctl.c @@ -6436,7 +6436,19 @@ server_loop(struct ovsdb_idl *idl, int argc, char *argv[]) service_start(&argc, &argv); daemonize_start(false); - int error = unixctl_server_create(unixctl_path, &server); + + char *abs_unixctl_path = NULL; + if (!unixctl_path) { + abs_unixctl_path = get_abs_unix_ctl_path(); + } else { + abs_unixctl_path = unixctl_path; + } + + int error = unixctl_server_create(abs_unixctl_path, &server); + if (!unixctl_path) { + free(abs_unixctl_path); + } + if (error) { ctl_fatal("failed to create unixctl server (%s)", ovs_retval_to_string(error));