On Wed, Jun 9, 2021, at 12:04, Andrew Rybchenko wrote:
> On 6/8/21 11:48 PM, Stephen Hemminger wrote:
> > On Tue, 8 Jun 2021 18:55:17 +0300
> > Andrew Rybchenko <andrew.rybche...@oktetlabs.ru> wrote:
> >
> >> On 6/8/21 6:42 PM, Stephen Hemminger wrote:
> >>> On Tue, 8 Jun 2021 11:00:37 +0300
> >>> Andrew Rybchenko <andrew.rybche...@oktetlabs.ru> wrote:
> >>>
> >>>> On 4/19/21 8:08 PM, Thomas Monjalon wrote:
> >>>>> About the title, better to speak about multi-process,
> >>>>> it is less confusing than primary/secondary.
> >>>>>
> >>>>> 15/03/2021 20:27, Stephen Hemminger:
> >>>>>> Set mutex used in failsafe driver to protect when used by
> >>>>>> both primary and secondary process. Without this fix, the failsafe
> >>>>>> lock is not really locking when there are multiple secondary processes.
> >>>>>>
> >>>>>> Bugzilla ID: 662
> >>>>>> Signed-off-by: Stephen Hemminger <step...@networkplumber.org>
> >>>>>> Fixes: 655fcd68c7d2 ("net/failsafe: fix hotplug races")
> >>>>>> Cc: ma...@mellanox.com
> >>>>>
> >>>>> The correct order for above lines is:
> >>>>>
> >>>>> Bugzilla ID: 662
> >>>>> Fixes: 655fcd68c7d2 ("net/failsafe: fix hotplug races")
> >>>>>
> >>>>> Signed-off-by: Stephen Hemminger <step...@networkplumber.org>
> >>>>>
> >>>>>> ---
> >>>>>> --- a/drivers/net/failsafe/failsafe.c
> >>>>>> +++ b/drivers/net/failsafe/failsafe.c
> >>>>>> @@ -140,6 +140,11 @@ fs_mutex_init(struct fs_priv *priv)
> >>>>>> ERROR("Cannot initiate mutex attributes - %s",
> >>>>>> strerror(ret));
> >>>>>> return ret;
> >>>>>> }
> >>>>>> + /* Allow mutex to protect primary/secondary */
> >>>>>> + ret = pthread_mutexattr_setpshared(&attr,
> >>>>>> PTHREAD_PROCESS_SHARED);
> >>>>>> + if (ret)
> >>>>>> + ERROR("Cannot set mutex shared - %s", strerror(ret));
> >>>>>>
> >>>>>
> >>>>> Why not returning an error here?
> >>>>
> >>>> +1
> >>>>
> >>>> I think it would be safer to return an error here.
> >>>
> >>> Ok but it never happens.
> >>>
> >>
> >> May I ask why? 'man pthread_mutexattr_setpshared' says that it
> >> is possible.
> >>
> >
> > The glibc implementation of pthread_mutexattr_setpshared is:
> >
> >
> > int
> > pthread_mutexattr_setpshared (pthread_mutexattr_t *attr, int pshared)
> > {
> > struct pthread_mutexattr *iattr;
> >
> > int err = futex_supports_pshared (pshared);
> > if (err != 0)
> > return err;
> >
> > iattr = (struct pthread_mutexattr *) attr;
> >
> > if (pshared == PTHREAD_PROCESS_PRIVATE)
> > iattr->mutexkind &= ~PTHREAD_MUTEXATTR_FLAG_PSHARED;
> > else
> > iattr->mutexkind |= PTHREAD_MUTEXATTR_FLAG_PSHARED;
> >
> > return 0;
> > }
> >
> > And
> >
> > /* FUTEX_SHARED is always supported by the Linux kernel. */
> > static __always_inline int
> > futex_supports_pshared (int pshared)
> > {
> > if (__glibc_likely (pshared == PTHREAD_PROCESS_PRIVATE))
> > return 0;
> > else if (pshared == PTHREAD_PROCESS_SHARED)
> > return 0;
> > else
> > return EINVAL;
> > }
> >
> >
> > There for the code as written can not return an error.
> > The check was only because someone could report a bogus
> > issue from a broken c library.
> >
>
> Many thanks for detailed description.
> I thought that it is better to follow API
> definition and it is not that hard to check
> return code and handle it. Yes, glibc is not
> the only C library.
>
On principle the API spec should be respected without assuming a specific
implementation.
Another way to think about it is that a future dev having zero knowledge of
this thread, reading this code and checking the POSIX manual, will also need to
check that usual c lib implementations are unlikely to generate an error before
concluding that this code is alright. It should not be necessary.
--
Gaetan Rivet
