Re: [libgpiod][PATCH v2 2/4] tools: use ppoll() where higher timeout resolution makes sense

[Kent Gibson <warthog618@xxxxxxxxx>]

On Mon, Apr 22, 2024 at 08:15:46PM +0200, Bartosz Golaszewski wrote:
> On Wed, Apr 17, 2024 at 9:23 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> >
> > On Tue, Apr 16, 2024 at 11:52:20PM +0200, Bartosz Golaszewski wrote:
> > >
> > > -             ret = poll(pollfds, resolver->num_chips, cfg.idle_timeout);
> > > +             if (cfg.idle_timeout > 0) {
> > > +                     idle_timeout.tv_sec = cfg.idle_timeout / 1000000;
> > > +                     idle_timeout.tv_nsec =
> > > +                                     (cfg.idle_timeout % 1000000) * 1000;
> > > +             }
> > > +
> > > +             ret = ppoll(pollfds, resolver->num_chips,
> > > +                         cfg.idle_timeout > 0 ? &idle_timeout : NULL, NULL);
> > >               if (ret < 0)
> > >                       die_perror("error polling for events");
> > >
> >
> > One minor nit - I would introduce a timespec pointer initialised to NULL
> > and set to point to idle_timeout within the if rather than repeat the
> > cfg.idle_timeout > 0 test.
> >
>
> Actually we can avoid it by doing it once before we enter the for(;;)
> loop. It's passed by constant pointer to ppoll() anyway and having the
> struct AND pointer to it initialized to NULL sounds more complex than
> it needs to be. I'll do it in v2.
>

Ah, I overlooked the for loop, so you are right that it should be
initialised before that.

> > But that is just personal preference, so either way,
> >
> > Reviewed-by: Kent Gibson <warthog618@xxxxxxxxx>
> >
>
> Thanks, I'll keep the tag if you don't mind the above solution?
>

Sure.

Cheers,
Kent.