Re: [Qemu-devel] [PATCH 05/25] nbd: Avoid generic -EINVAL

[Max Reitz <mreitz@redhat.com>]

On 2015-03-11 at 07:22, Paolo Bonzini wrote:
>
> On 25/02/2015 19:08, Max Reitz wrote:
>> Just returning -EINVAL for everything is bad. -EIO is often better, and
>> sometimes there is an even more fitting value.
> Propagating the return value from write_sync is uglier, but it is even
> better in terms of returned value.

We can only return -errno values, but write_sync() may do partial writes 
so it may return non-negative values which still indicate an error. So 
we'd have to check whether the return value is negative, if it is, 
return that, if it isn't but if it's still below what we wanted to 
write, return a fixed error (such as -EIO). I'd rather just return -EIO 
and be done with it, but if you really want me to, I can of course do it 
differently.

Max

> Paolo
>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   nbd.c | 49 +++++++++++++++++++++++++------------------------
>>   1 file changed, 25 insertions(+), 24 deletions(-)
>>
>> diff --git a/nbd.c b/nbd.c
>> index b4776ce..d2eb1c5 100644
>> --- a/nbd.c
>> +++ b/nbd.c
>> @@ -238,22 +238,22 @@ static int nbd_send_rep(int csock, uint32_t type, uint32_t opt)
>>       magic = cpu_to_be64(NBD_REP_MAGIC);
>>       if (write_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
>>           LOG("write failed (rep magic)");
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>       opt = cpu_to_be32(opt);
>>       if (write_sync(csock, &opt, sizeof(opt)) != sizeof(opt)) {
>>           LOG("write failed (rep opt)");
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>       type = cpu_to_be32(type);
>>       if (write_sync(csock, &type, sizeof(type)) != sizeof(type)) {
>>           LOG("write failed (rep type)");
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>       len = cpu_to_be32(0);
>>       if (write_sync(csock, &len, sizeof(len)) != sizeof(len)) {
>>           LOG("write failed (rep data length)");
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>       return 0;
>>   }
>> @@ -267,31 +267,31 @@ static int nbd_send_rep_list(int csock, NBDExport *exp)
>>       magic = cpu_to_be64(NBD_REP_MAGIC);
>>       if (write_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
>>           LOG("write failed (magic)");
>> -        return -EINVAL;
>> +        return -EIO;
>>        }
>>       opt = cpu_to_be32(NBD_OPT_LIST);
>>       if (write_sync(csock, &opt, sizeof(opt)) != sizeof(opt)) {
>>           LOG("write failed (opt)");
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>       type = cpu_to_be32(NBD_REP_SERVER);
>>       if (write_sync(csock, &type, sizeof(type)) != sizeof(type)) {
>>           LOG("write failed (reply type)");
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>       len = cpu_to_be32(name_len + sizeof(len));
>>       if (write_sync(csock, &len, sizeof(len)) != sizeof(len)) {
>>           LOG("write failed (length)");
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>       len = cpu_to_be32(name_len);
>>       if (write_sync(csock, &len, sizeof(len)) != sizeof(len)) {
>>           LOG("write failed (length)");
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>       if (write_sync(csock, exp->name, name_len) != name_len) {
>>           LOG("write failed (buffer)");
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>       return 0;
>>   }
>> @@ -309,7 +309,7 @@ static int nbd_handle_list(NBDClient *client, uint32_t length)
>>       /* For each export, send a NBD_REP_SERVER reply. */
>>       QTAILQ_FOREACH(exp, &exports, next) {
>>           if (nbd_send_rep_list(csock, exp)) {
>> -            return -EINVAL;
>> +            return -EIO;
>>           }
>>       }
>>       /* Finish with a NBD_REP_ACK. */
>> @@ -331,6 +331,7 @@ static int nbd_handle_export_name(NBDClient *client, uint32_t length)
>>       }
>>       if (read_sync(csock, name, length) != length) {
>>           LOG("read failed");
>> +        rc = -EIO;
>>           goto fail;
>>       }
>>       name[length] = '\0';
>> @@ -376,22 +377,22 @@ static int nbd_receive_options(NBDClient *client)
>>   
>>           if (read_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
>>               LOG("read failed");
>> -            return -EINVAL;
>> +            return -EIO;
>>           }
>>           TRACE("Checking opts magic");
>>           if (magic != be64_to_cpu(NBD_OPTS_MAGIC)) {
>>               LOG("Bad magic received");
>> -            return -EINVAL;
>> +            return -EIO;
>>           }
>>   
>>           if (read_sync(csock, &tmp, sizeof(tmp)) != sizeof(tmp)) {
>>               LOG("read failed");
>> -            return -EINVAL;
>> +            return -EIO;
>>           }
>>   
>>           if (read_sync(csock, &length, sizeof(length)) != sizeof(length)) {
>>               LOG("read failed");
>> -            return -EINVAL;
>> +            return -EIO;
>>           }
>>           length = be32_to_cpu(length);
>>   
>> @@ -404,7 +405,7 @@ static int nbd_receive_options(NBDClient *client)
>>               break;
>>   
>>           case NBD_OPT_ABORT:
>> -            return -EINVAL;
>> +            return -ECONNABORTED;
>>   
>>           case NBD_OPT_EXPORT_NAME:
>>               return nbd_handle_export_name(client, length);
>> @@ -446,7 +447,7 @@ static int nbd_send_negotiate(NBDClient *client)
>>        */
>>   
>>       qemu_set_block(csock);
>> -    rc = -EINVAL;
>> +    rc = -EIO;
>>   
>>       TRACE("Beginning negotiation.");
>>       memset(buf, 0, sizeof(buf));
>> @@ -503,7 +504,7 @@ int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
>>   
>>       TRACE("Receiving negotiation.");
>>   
>> -    rc = -EINVAL;
>> +    rc = -EIO;
>>   
>>       if (read_sync(csock, buf, 8) != 8) {
>>           error_setg(errp, "Failed to read data");
>> @@ -752,7 +753,7 @@ ssize_t nbd_send_request(int csock, struct nbd_request *request)
>>   
>>       if (ret != sizeof(buf)) {
>>           LOG("writing to socket failed");
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>       return 0;
>>   }
>> @@ -770,7 +771,7 @@ static ssize_t nbd_receive_request(int csock, struct nbd_request *request)
>>   
>>       if (ret != sizeof(buf)) {
>>           LOG("read failed");
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>   
>>       /* Request
>> @@ -793,7 +794,7 @@ static ssize_t nbd_receive_request(int csock, struct nbd_request *request)
>>   
>>       if (magic != NBD_REQUEST_MAGIC) {
>>           LOG("invalid magic (got 0x%x)", magic);
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>       return 0;
>>   }
>> @@ -811,7 +812,7 @@ ssize_t nbd_receive_reply(int csock, struct nbd_reply *reply)
>>   
>>       if (ret != sizeof(buf)) {
>>           LOG("read failed");
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>   
>>       /* Reply
>> @@ -830,7 +831,7 @@ ssize_t nbd_receive_reply(int csock, struct nbd_reply *reply)
>>   
>>       if (magic != NBD_REPLY_MAGIC) {
>>           LOG("invalid magic (got 0x%x)", magic);
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>       return 0;
>>   }
>> @@ -858,7 +859,7 @@ static ssize_t nbd_send_reply(int csock, struct nbd_reply *reply)
>>   
>>       if (ret != sizeof(buf)) {
>>           LOG("writing to socket failed");
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>       return 0;
>>   }
>>