Re: [PATCH v7] getrandom system call wrapper [BZ #17252]

[Torvald Riegel <triegel@redhat.com>]

On Fri, 2016-11-18 at 08:28 +0000, Szabolcs Nagy wrote:
> On 17/11/16 12:52, Torvald Riegel wrote:
> > On Wed, 2016-11-16 at 17:52 -0200, Adhemerval Zanella wrote:
> >>
> >> On 16/11/2016 16:02, Torvald Riegel wrote:
> >>> On Wed, 2016-11-16 at 16:52 +0100, Florian Weimer wrote:
> >>>> On 11/16/2016 04:20 PM, Zack Weinberg wrote:
> >>>>> On 11/16/2016 10:11 AM, Florian Weimer wrote:
> >>>>>> On 11/14/2016 07:29 PM, Zack Weinberg wrote:
> >>>>>>> On 11/14/2016 12:44 PM, Florian Weimer wrote:
> >>>>>>>> This patch switches back to the ssize_t return time.  This goes against
> >>>>>>>> Theodore Ts'o preference, but seems to reflect the consensus from the
> >>>>>>>> largery community.
> >>>>>>>
> >>>>>>> I still don't think this function should be a cancellation point.
> >>>>>>
> >>>>>> I guess we'll have to agree to disagree on this matter.
> >>>>>
> >>>>> I am seriously considering escalating my disagreement here to a formal
> >>>>> objection.  I would like to know why you think it is NECESSARY - not
> >>>>> merely convenient or consistent with other stuff - for this function to
> >>>>> be a cancellation point.
> >>>>
> >>>> It's necessary if you ever want to cancel a hanging getrandom in a 
> >>>> context where you cannot install a signal handler (so that you can 
> >>>> trigger EINTR when getrandom is stuck).
> >>>
> >>> I think it would be better to split the getrandom that is a cancellation
> >>> point into two functions, getrandom (not a cancellation point) and
> >>> getrandom_cancelable (is a cancellation point).  This way, the
> >>> functionality is available for programs but requires explicit opt-in,
> >>> while the default is not going to lead to surprises in the expected
> >>> common case (ie, when randomness is available).  I don't think the
> >>> opt-in is a problem because as you said, cancellation requires careful
> >>> programming anyway.
> >>>
> >>
> >> I would advise against adding another symbol, it only adds complexity
> >> and most likely one interface would be preferable from application
> >> point of view.
> > 
> > We could add a parameter too that determines whether it is a
> > cancellation point.  That avoids your concern about adding another
> 
> why?
> 
> use pthread_setcancelstate.
> 
> works with all cancellation points not just getrandom.

As discussed in the thread, there are different opinions about what the
default should be.  There are reasonable arguments for both options.  In
such a case, it seems better to make the choice explicit, simply from an
ease-of-use and interface design perspective.  This is not about whether
it's possible for users to do (they could build their own syscall
wrappers after all too, right? ;) ) but about clean interfaces.

With your proposal, one could argue that, for example, every library has
to be aware of cancellation and how it works if one of the clients of
the library could want to use cancellation; the library either has to
disable it explicitly, or it has to document which additional
cancellation points exist and has to implement the cleanup handlers.
This is error-prone and adds complexity to those use cases.  Therefore,
it seems better to avoid that potential source of bugs and either make
the default to not support cancellation (ie, an opt-in for the
cancellation facility), or make at least make the choice explicit for
users of getrandom (ie, two functions or an additional parameter).