On Feb  9 14:12, Ken Brown via Cygwin-developers wrote:
> On 2/9/2021 12:13 PM, Ken Brown via Cygwin-developers wrote:
> > On 2/9/2021 11:12 AM, Corinna Vinschen via Cygwin-developers wrote:
> > > On Feb  9 10:31, Ken Brown via Cygwin-developers wrote:
> > > > On 2/9/2021 10:02 AM, Corinna Vinschen via Cygwin-developers wrote:
> > > > > On Feb  9 09:19, Ken Brown via Cygwin-developers wrote:
> > > > > > On 2/9/2021 4:47 AM, Corinna Vinschen via Cygwin-developers wrote:
> > > > > > > On Feb  8 12:39, Ken Brown via Cygwin-developers wrote:
> > > > > > > > I've had occasion to work through dtable::dup_worker, and I'm seeing the
> > > > > > > > potential for leaks of path_conv handles.
> > > > > > > [...]
> > > > > > > Nevertheless, it's a bad idea to keep this code.  So the question is
> > > > > > > this:  Do we actually *need* to duplicate the conv_handle at all?
> > > > > > I've come across one place where I think it's needed.
> > > > > > [...]
> > > > > Indeed, you're right.  I just found that the fhandler_base::reset method
> > > > > is only called from copyto.  Given that fhandler::operator= already
> > > > > calls path_conv::operator=, and that duplicates the conv handle, why
> > > > > call path_conv::operator<< from fhandler_base::reset at all?  It looks
> > > > > like this is only duplicating what already has been done.
> > > > 
> > > > I think that's right.  It looks like operator<< differs from operator= only
> > > > in being careful not to overwrite an existing path.  So I can't see that it
> > > > ever makes sense to call operator<< right after calling operator=.
> > > 
> > > It might be helpful not only to move reset to a protected inline method,
> > > but also to rename it, making entirely clear that this is just a copyto
> > > helper and nothing else.  I. e., something like _copyto_reset_helper().
> [...]
> Trying to make _copyto_reset_helper() protected leads to errors like this:
> 
> In file included from ../../../../newlib-cygwin/winsup/cygwin/spawn.cc:22:
> ../../../../newlib-cygwin/winsup/cygwin/fhandler.h: In member function
> ‘virtual void fhandler_pty_master::copyto(fhandler_base*)’:
> ../../../../newlib-cygwin/winsup/cygwin/fhandler.h:2461:30: error: ‘void
> fhandler_base::_copyto_reset_helper()’ is protected within this context
>  2461 |     x->_copyto_reset_helper ();
> 
> As usual, my C++ knowledge is limited, but I guess the issue is that we're
> calling _copyto_reset_helper() on behalf of x.  As an experiment, I removed
> 'x->', and the error message went away.  I don't fully understand this.  Do
> you see an easy way around it, or should I just leave _copyto_reset_helper()
> public?

There's no easy way as such.  I think the right approach is to turn the
copy-to method into a copy-from method, modifying "this", rather than
another object given as parameter.  This is a more object-oriented
approach, IMHO.  This also automagically fixes the problem that a
protected method can't be called in this context.

Here's a patch fixing it this way.  Can you please review it and see
if I missed something?

Actually, on second thought, this should be split into two patches, one
removing the call to path_conv::operator<<, and the other one
reshuffling the code.  If the patch is ok, I'll do that before pushing
it.


Thanks,
Corinna