On 1/16/19 4:15 AM, Vladimir Sementsov-Ogievskiy wrote:

>> @@ -347,7 +350,8 @@ static int nbd_opt_go(QIOChannel *ioc, NBDExportInfo *info, Error **errp)
>>        * flags still 0 is a witness of a broken server. */
>>       info->flags = 0;
>>
>> -    trace_nbd_opt_go_start(info->name);
>> +    assert(opt == NBD_OPT_GO || opt == NBD_OPT_INFO);
>> +    trace_nbd_opt_go_start(nbd_opt_lookup(opt), info->name);
> 
> I'd prefer to upgrade trace-point name too, as well as other several trace_nbd_opt_go_* trace
> points in the function.
> 

Can do.


>> +        /* Send NBD_OPT_ABORT as a courtesy before hanging up */
>> +        nbd_send_opt_abort(ioc);
>> +        break;
>> +    case 1: /* newstyle, but limited to EXPORT_NAME */
>> +        error_setg(errp, "Server does not support export lists");
>> +        /* We can't even send NBD_OPT_ABORT, so merely hang up */
> 
> But, on the other hand, why not to send it? It will be unknown for the server,
> so, it will have to close the connection accordingly to the protocol, isn't it
> better a bit?

The NBD spec says that if the server did not advertise fixed newstyle,
then the client must not send any other NBD_OPT.  And servers that don't
support fixed newstyle are rather rare, so it doesn't really hurt if we
aren't courteous to them.

> 
>> +        goto out;
>> +    case 0: /* oldstyle, parse length and flags */
>> +        array = g_new0(NBDExportInfo, 1);
>> +        array->name = g_strdup("");
>> +        count = 1;
>> +
>> +        if (nbd_negotiate_finish_oldstyle(ioc, array, errp) < 0) {
>> +            return -EINVAL;
> 
> goto out, you mean.

Indeed. Thanks for spotting it.

> And with at least this one fixed:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> 
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org