[libvirt] [PATCH v4] Add macro for handling exponential backoff loops.

[Pavel Hrdina]

On Fri, Apr 15, 2016 at 10:48:30AM +0200, Jiri Denemark wrote:
> On Fri, Apr 15, 2016 at 09:38:42 +0100, Richard W.M. Jones wrote:
> > In a few places in libvirt we busy-wait for events, for example qemu
> > creating a monitor socket.  This is problematic because:
> > 
> >  - We need to choose a sufficiently small polling period so that
> >    libvirt doesn't add unnecessary delays.
> > 
> >  - We need to choose a sufficiently large polling period so that
> >    the effect of busy-waiting doesn't affect the system.
> > 
> > The solution to this conflict is to use an exponential backoff.
> > 
> > This patch adds a macro VIR_TIME_WHILE_WITH_BACKOFF to hide the
> > details, and modifies a few places where we currently busy-wait.
> > ---
> >  src/fdstream.c           | 10 +++++----
> >  src/libvirt_private.syms |  2 ++
> >  src/qemu/qemu_agent.c    |  9 ++++----
> >  src/qemu/qemu_monitor.c  | 10 +++++----
> >  src/util/virtime.c       | 54 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  src/util/virtime.h       | 34 ++++++++++++++++++++++++++++++
> >  6 files changed, 107 insertions(+), 12 deletions(-)
> > 
> > diff --git a/src/fdstream.c b/src/fdstream.c
> > index ef118b5..d9646d0 100644
> > --- a/src/fdstream.c
> > +++ b/src/fdstream.c
> > @@ -42,6 +42,7 @@
> >  #include "virfile.h"
> >  #include "configmake.h"
> >  #include "virstring.h"
> > +#include "virtime.h"
> >  
> >  #define VIR_FROM_THIS VIR_FROM_STREAMS
> >  
> > @@ -520,8 +521,7 @@ int virFDStreamConnectUNIX(virStreamPtr st,
> >                             bool abstract)
> >  {
> >      struct sockaddr_un sa;
> > -    size_t i = 0;
> > -    int timeout = 3;
> > +    virTimeBackOffVar timeout;
> >      int ret;
> >  
> >      int fd = socket(AF_UNIX, SOCK_STREAM, 0);
> > @@ -541,7 +541,9 @@ int virFDStreamConnectUNIX(virStreamPtr st,
> >              goto error;
> >      }
> >  
> > -    do {
> > +    if (virTimeBackOffStart(&timeout, 1, 3*1000 /* ms */) < 0)
> > +        goto error;
> > +    VIR_TIME_WHILE_WITH_BACKOFF(timeout) {
> 
> Uff, I don't think we should go this route. Open coding the ugly macro
> makes the code nicer and easier to read and understand:
> 
>     if (virTimeBackOffStart(&timeout, 1, 3*1000) < 0)
>         goto error;
>     while (virTimeBackOffCondition(&timeout)) {
>         ...
>     }
> 
> However, I'd suggest changing the name virTimeBackOffCondition to
> something like virTimeBackOffWait to make it sound better when used in
> the while loop:
> 
>     while (virTimeBackOffWait(&timeout))
>         ...
> 
> The rest of the patch looks fine, though.
> 
> Jirka

I must also agree with others that the MACRO is overkill in this case, since it
only hides simple while().  There is no benefit from using this MACRO and
personally for me it's more confusing then using the while itself.

Pavel