03.06.2021 19:29, Eric Blake wrote:
> On Fri, Apr 16, 2021 at 11:08:57AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> To be reused in the following patch.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> block/nbd.c | 99 ++++++++++++++++++++++++++++++-----------------------
>> 1 file changed, 57 insertions(+), 42 deletions(-)
>>
>> diff --git a/block/nbd.c b/block/nbd.c
>> index 5e63caaf4b..03ffe95231 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -318,6 +318,50 @@ static bool nbd_client_connecting_wait(BDRVNBDState *s)
>> return qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTING_WAIT;
>> }
>>
>> +/*
>> + * Check s->info updated by negotiation process.
>
> The parameter name is bs, not s; so this comment is a bit confusing...
>
>> + * Update @bs correspondingly to new options.
>> + */
>> +static int nbd_handle_updated_info(BlockDriverState *bs, Error **errp)
>> +{
>> + BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
>
> ...until here. Maybe rewrite the entire comment as:
>
> Update @bs with information learned during a completed negotiation
> process. Return failure if the server's advertised options are
> incompatible with the client's needs.
>
>> + int ret;
>> +
>> + if (s->x_dirty_bitmap) {
>> + if (!s->info.base_allocation) {
>> + error_setg(errp, "requested x-dirty-bitmap %s not found",
>> + s->x_dirty_bitmap);
>> + return -EINVAL;
>> + }
>> + if (strcmp(s->x_dirty_bitmap, "qemu:allocation-depth") == 0) {
>> + s->alloc_depth = true;
>> + }
>> + }
>> +
>> + if (s->info.flags & NBD_FLAG_READ_ONLY) {
>> + ret = bdrv_apply_auto_read_only(bs, "NBD export is read-only", errp);
>> + if (ret < 0) {
>> + return ret;
>> + }
>> + }
>> +
>> + if (s->info.flags & NBD_FLAG_SEND_FUA) {
>> + bs->supported_write_flags = BDRV_REQ_FUA;
>> + bs->supported_zero_flags |= BDRV_REQ_FUA;
>
> Code motion, so it is correct, but it looks odd to use = for one
> assignment and |= for the other. Using |= in both places would be
> more consistent.
Actually I see bugs here:
1. we should do =, not |=, as on reconnect info changes, so we should reset supported flags.
2. in-fligth requests that are in retying loops are not prepared to flags changing. I afraid, that some malicious server may even do some bad thing
Still, let's fix it after these series. To avoid more conflicts.
>
>> + }
>> +
>> + if (s->info.flags & NBD_FLAG_SEND_WRITE_ZEROES) {
>> + bs->supported_zero_flags |= BDRV_REQ_MAY_UNMAP;
>> + if (s->info.flags & NBD_FLAG_SEND_FAST_ZERO) {
>> + bs->supported_zero_flags |= BDRV_REQ_NO_FALLBACK;
>> + }
>> + }
>> +
>> + trace_nbd_client_handshake_success(s->export);
>> +
>> + return 0;
>> +}
>> +
>> static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s)
>> {
>> int ret;
>> @@ -1579,49 +1623,13 @@ static int nbd_client_handshake(BlockDriverState *bs, Error **errp)
>
> As updating the comment doesn't affect code correctness,
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
--
Best regards,
Vladimir