Re: [Xen-devel] [PATCH v2 05/20] block/xen-blkfront: Split blkif_queue_request in 2

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Roger,

On 21/07/15 10:54, Roger Pau Monnà wrote:
> El 09/07/15 a les 22.42, Julien Grall ha escrit:
>> Currently, blkif_queue_request has 2 distinct execution path:
>>     - Send a discard request
>>     - Send a read/write request
>>
>> The function is also allocating grants to use for generating the
>> request. Although, this is only used for read/write request.
>>
>> Rather than having a function with 2 distinct execution path, separate
>> the function in 2. This will also remove one level of tabulation.
>>
>> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>> Cc: Roger Pau Monnà <roger.pau@xxxxxxxxxx>
>> Cc: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
>> Cc: David Vrabel <david.vrabel@xxxxxxxxxx>
> 
> Patch looks fine, although with so much indentation changes it's kind of
> hard to review.

I wasn't sure how to make this patch more easy to review and it seems
like diff is getting confused.

It's mostly removing one indentation layer (the if (req->cmd_flags ...))
and move the discard code in a separate function.

> Acked-by: Roger Pau Monnà <roger.pau@xxxxxxxxxx>

Thank you.

> Just one minor change below.
> 
> [...]
> 
>> @@ -595,6 +603,24 @@ static int blkif_queue_request(struct request *req)
>>      return 0;
>>  }
>>  
>> +/*
>> + * Generate a Xen blkfront IO request from a blk layer request.  Reads
>> + * and writes are handled as expected.
>> + *
>> + * @req: a request struct
>> + */
>> +static int blkif_queue_request(struct request *req)
>> +{
>> +    struct blkfront_info *info = req->rq_disk->private_data;
>> +
>> +    if (unlikely(info->connected != BLKIF_STATE_CONNECTED))
>> +            return 1;
>> +
>> +    if (unlikely(req->cmd_flags & (REQ_DISCARD | REQ_SECURE)))
>> +            return blkif_queue_discard_req(req);
>> +    else
>> +            return blkif_queue_rw_req(req);
> 
> There's no need for the else clause.

I find it more readable and obvious to understand than:

if ( ... )
  return
return;

when there is only one line in the else. IIRC, the resulting assembly
will be the same.

Anyway, I can drop the else if you really want.

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel