Re: [PATCH v2 for-7.1 6/9] nbd: code motion and function renaming

[Paolo Bonzini]

On 4/16/22 14:37, Vladimir Sementsov-Ogievskiy wrote:
> 14.04.2022 20:57, Paolo Bonzini wrote:
> > Prepare for the next patch, so that the diff is less confusing.
> >
> > nbd_client_connecting is moved closer to the definition point.
>
> Amm. To usage-point you mean?
> The original idea was to keep simple state-reading helpers definitions 
> together :)

Yes and it makes sense.  The new idea is to keep requests_lock functions 
together instead. :)

Paolo


> > nbd_client_connecting_wait() is kept only for the reconnection
> > logic; when it is used to check if a request has to be reissued,
> > use the renamed function nbd_client_will_reconnect().  In the
> > next patch, the two cases will have different locking requirements.
> >
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
>
> > ---
> >   block/nbd.c | 24 ++++++++++++++----------
> >   1 file changed, 14 insertions(+), 10 deletions(-)
> >
> > diff --git a/block/nbd.c b/block/nbd.c
> > index a2414566d1..37d466e435 100644
> > --- a/block/nbd.c
> > +++ b/block/nbd.c
> > @@ -254,18 +254,15 @@ static void open_timer_init(BDRVNBDState *s, 
> > uint64_t expire_time_ns)
> >       timer_mod(s->open_timer, expire_time_ns);
> >   }
> > -static bool nbd_client_connecting(BDRVNBDState *s)
> > -{
> > -    NBDClientState state = qatomic_load_acquire(&s->state);
> > -    return state == NBD_CLIENT_CONNECTING_WAIT ||
> > -        state == NBD_CLIENT_CONNECTING_NOWAIT;
> > -}
> > -
> >   static bool nbd_client_connecting_wait(BDRVNBDState *s)
> >   {
> >       return qatomic_load_acquire(&s->state) == 
> > NBD_CLIENT_CONNECTING_WAIT;
> >   }
> > +static bool nbd_client_will_reconnect(BDRVNBDState *s)
> > +{
> > +    return qatomic_load_acquire(&s->state) == 
> > NBD_CLIENT_CONNECTING_WAIT;
> > +}
> >   /*
> >    * Update @bs with information learned during a completed 
> > negotiation process.
> >    * Return failure if the server's advertised options are 
> > incompatible with the
> > @@ -355,6 +352,13 @@ int coroutine_fn 
> > nbd_co_do_establish_connection(BlockDriverState *bs,
> >       return 0;
> >   }
> > +static bool nbd_client_connecting(BDRVNBDState *s)
> > +{
> > +    NBDClientState state = qatomic_load_acquire(&s->state);
> > +    return state == NBD_CLIENT_CONNECTING_WAIT ||
> > +        state == NBD_CLIENT_CONNECTING_NOWAIT;
> > +}
> > +
> >   /* Called with s->requests_lock taken.  */
> >   static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s)
> >   {
> > @@ -1190,7 +1194,7 @@ static int coroutine_fn 
> > nbd_co_request(BlockDriverState *bs, NBDRequest *request
> >               error_free(local_err);
> >               local_err = NULL;
> >           }
> > -    } while (ret < 0 && nbd_client_connecting_wait(s));
> > +    } while (ret < 0 && nbd_client_will_reconnect(s));
> >       return ret ? ret : request_ret;
> >   }
> > @@ -1249,7 +1253,7 @@ static int coroutine_fn 
> > nbd_client_co_preadv(BlockDriverState *bs, int64_t offse
> >               error_free(local_err);
> >               local_err = NULL;
> >           }
> > -    } while (ret < 0 && nbd_client_connecting_wait(s));
> > +    } while (ret < 0 && nbd_client_will_reconnect(s));
> >       return ret ? ret : request_ret;
> >   }
> > @@ -1407,7 +1411,7 @@ static int coroutine_fn nbd_client_co_block_status(
> >               error_free(local_err);
> >               local_err = NULL;
> >           }
> > -    } while (ret < 0 && nbd_client_connecting_wait(s));
> > +    } while (ret < 0 && nbd_client_will_reconnect(s));
> >       if (ret < 0 || request_ret < 0) {
> >           return ret ? ret : request_ret;