On Thu, May 14, 2009 9:43 pm, SandeepKsinha wrote:
> Hi Andre,
>
> On Thu, May 14, 2009 at 4:13 PM, Andre Noll <maan@xxxxxxxxxxxxxxx> wrote:
>> Get rid of the local variable "conf" and of an unnecessary cast.
>>
>> Signed-off-by: Andre Noll <maan@xxxxxxxxxxxxxxx>
>> ---
>> drivers/md/raid0.c | 20 +++++++-------------
>> 1 files changed, 7 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
>> index 5ff6290..36b747a 100644
>> --- a/drivers/md/raid0.c
>> +++ b/drivers/md/raid0.c
>> @@ -261,7 +261,6 @@ static sector_t raid0_size(mddev_t *mddev, sector_t
>> sectors, int raid_disks)
>>
>> static int raid0_run(mddev_t *mddev)
>> {
>> - raid0_conf_t *conf;
>> int ret;
>>
>> if (mddev->chunk_size == 0) {
>> @@ -276,14 +275,15 @@ static int raid0_run(mddev_t *mddev)
>> blk_queue_segment_boundary(mddev->queue, (mddev->chunk_size>>1) -
>> 1);
>> mddev->queue->queue_lock = &mddev->queue->__queue_lock;
>>
>> - conf = kmalloc(sizeof (raid0_conf_t), GFP_KERNEL);
>> - if (!conf)
>> + mddev->private = kmalloc(sizeof(raid0_conf_t), GFP_KERNEL);
>> + if (!mddev->private)
>> return -ENOMEM;
>> - mddev->private = (void *)conf;
>> -
>> ret = create_strip_zones(mddev);
>> - if (ret < 0)
>> - goto out_free_conf;
>> + if (ret < 0) {
>> + kfree(mddev->private);
>> + mddev->private = NULL;
>> + return ret;
>> + }
>>
>
>
> I believe the use of goto statements keep the code more structured,
> especially for code cleanup in case of failures.
> Its better that you revert back to the goto's so that later if a new
> piece of code is added and there would be cases of failure to handle,
> you will have a common cleanup routine.
>
> I think Neil can comment better on this one.
I'm in two minds about this.
In general I prefer
if (something went wrong)
return error;
to
if (something went wrong)_
goto handle_error;
....
handle_error:
return error;
as it is easier to read.
However the point about maintainability is valid. Having to put all
those gotos back in if you find that you need to free something for
unlock something in error prone (you might miss a return)
So having exactly one return for each function is a valuable
goal.
So in the general case there are good arguments in both directions.
Which means you have to make a decision based on the specifics of a
given case.
In this case, I think raid0_run is sufficiently small (now that all
the hash related code is gone) that the benefits of having gotos
are minimal. So I would be inclined to leave it as it is.
Thanks for the suggestion.
NeilBrown
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
