[bitbake-devel] [PATCH] server/process: Various server startup logging fixes

[richard.purdie at linuxfoundation.org]

On Wed, 2018-09-05 at 12:19 -0500, Joshua Watt wrote:
> Looks good to me. I have a few (optional) suggestions.
> 
> On Wed, 2018-09-05 at 18:01 +0100, Richard Purdie wrote: 
> >      try:
> >          so = open(logfile, 'a+')
> > -        se = so
> >          os.dup2(so.fileno(), sys.stdout.fileno())
> > -        os.dup2(se.fileno(), sys.stderr.fileno())
> > +        os.dup2(so.fileno(), sys.stderr.fileno())
> 
> I think so "leaks" here. Granted, it was doing that before so perhaps
> it could be saved for a separate patch:
> 
>  with open(logfile, 'a+') as so:
>      os.dup2(so.fileno(), sys.stdout.fileno())
>      os.dup2(so.fileno(), sys.stderr.fileno())

I'd take a patch for it.

> > +        os.close(self.readypipe)
> >          writer = ConnectionWriter(self.readypipein)
> > -        try:
> > -            self.cooker = bb.cooker.BBCooker(self.configuration,
> > self.featureset)
> > -            writer.send("ready")
> > -        except:
> > -            writer.send("fail")
> > -            raise
> > -        finally:
> > -            os.close(self.readypipein)
> > +        self.cooker = bb.cooker.BBCooker(self.configuration,
> > self.featureset)
> > +        writer.send("ready")
> > +        writer.close()
> 
> If we want to ensure that writer is always explicitly closed, would
> this be better?
> 
>  import contextlib
> 
>  with contextlib.closing(ConnectionWriter(self.readypipein)) as
> writer:
>      self.cooker = bb.cooker.BBCooker(...)
>      writer.send("ready")
> 

We don't want to do that. The closing of the fd triggers the client to
decide something went wrong and check the logs so we want that to
happen after everything has flushed. The process takedown closing the
pipe is therefore a better option in this case. You could argue we
should flush here but the general exception handling and so on is later
down the exception path so I think what we're doing after this patch is
a better option.

Cheers,

Richard