Re: [Qemu-devel] [PATCH 2/2] block/iscsi: handle failure on malloc of the allocationmap

[Paolo Bonzini]

Il 25/08/2014 13:10, Peter Lieven ha scritto:
> On 25.08.2014 12:37, Paolo Bonzini wrote:
>> Il 22/08/2014 11:26, Peter Lieven ha scritto:
>>> Signed-off-by: Peter Lieven <address@hidden>
>>> ---
>>>   block/iscsi.c |   22 +++++++++++++++-------
>>>   1 file changed, 15 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/block/iscsi.c b/block/iscsi.c
>>> index ed883c3..131357c 100644
>>> --- a/block/iscsi.c
>>> +++ b/block/iscsi.c
>>> @@ -325,6 +325,19 @@ static bool is_request_lun_aligned(int64_t
>>> sector_num, int nb_sectors,
>>>       return 1;
>>>   }
>>>   +static unsigned long *iscsi_allocationmap_init(IscsiLun *iscsilun)
>>> +{
>>> +    unsigned long *ptr;
>>> +    ptr =
>>> bitmap_try_new(DIV_ROUND_UP(sector_lun2qemu(iscsilun->num_blocks,
>>> +                                                      iscsilun),
>>> +                                      iscsilun->cluster_sectors));
>>> +    if (ptr == NULL) {
>>> +        error_report("iSCSI: could not initialize allocationmap. "
>>> +                     "Out of memory.");
>>> +    }
>>> +    return ptr;
>>> +}
>>> +
>>>   static void iscsi_allocationmap_set(IscsiLun *iscsilun, int64_t
>>> sector_num,
>>>                                       int nb_sectors)
>>>   {
>>> @@ -1413,9 +1426,7 @@ static int iscsi_open(BlockDriverState *bs,
>>> QDict *options, int flags,
>>>           iscsilun->cluster_sectors = (iscsilun->bl.opt_unmap_gran *
>>>                                        iscsilun->block_size) >>
>>> BDRV_SECTOR_BITS;
>>>           if (iscsilun->lbprz && !(bs->open_flags & BDRV_O_NOCACHE)) {
>>> -            iscsilun->allocationmap =
>>> -                bitmap_new(DIV_ROUND_UP(bs->total_sectors,
>>> -                                        iscsilun->cluster_sectors));
>>> +            iscsilun->allocationmap =
>>> iscsi_allocationmap_init(iscsilun);
>>>           }
>>>       }
>> iscsi_open has an Error ** argument.  Please pass it to
>> iscsi_allocationmap_init and use error_setg instead of error_report.
> 
> I could pass the Error argument and use error_report only if the pointer
> is null.

No, NULL means "I don't care about errors, or I don't care about precise
error messages and will use the return value to check for errors".

>>> @@ -1508,10 +1519,7 @@ static int iscsi_truncate(BlockDriverState
>>> *bs, int64_t offset)
>>>         if (iscsilun->allocationmap != NULL) {
>>>           g_free(iscsilun->allocationmap);
>>> -        iscsilun->allocationmap =
>>> -           
>>> bitmap_new(DIV_ROUND_UP(sector_lun2qemu(iscsilun->num_blocks,
>>> -                                                    iscsilun),
>>> -                                    iscsilun->cluster_sectors));
>>> +        iscsilun->allocationmap = iscsi_allocationmap_init(iscsilun);
>> Here you may have to use qerror_report_err, though I guess the failure
>> need not be fatal and you can leave the allocationmap set to NULL.
> 
> That was the plan. I would also not fail on iscsi_open or would you?

Hmm, good question.  If so, the patch is correct with error_report.  I
thought a failure in iscsi_open would be less surprising.  The question
then is, do we need an option to disable allocationmap queries?

Paolo