Re: [Qemu-devel] [PULL 03/10] pc-bios/s390-ccw: handle different sector sizes

[Eugene \"jno\" Dvurechenski]

Yes, this patch doesn't make the code worse.

On 06/27/2014 05:11 PM, Christian Borntraeger wrote:
> On 27/06/14 15:04, Eugene "jno" Dvurechenski wrote:
>>
>>
>> On 06/27/2014 03:55 PM, Christian Borntraeger wrote:
>>>>> -    const int max_entries = (SECTOR_SIZE / sizeof(ScsiBlockPtr));
>>>>> +    const int max_entries = (MAX_SECTOR_SIZE / sizeof(ScsiBlockPtr));
>>>>
>>>> Is this really safe to increase? Doesn't max_entries depend on the real 
>>>> sector size?
>>>
>>> I think this is now covered by this if statement:
>>>             if (bprs[i].blockct == 0 && unused_space(&bprs[i + 1],
>>>                 sizeof(ScsiBlockPtr))) {
>>>
>>> which was introduced by commit c77cd87cf54f003748f29c14ea1ddaecfc5c653f 
>>> (pc-bios/s390-ccw: fix for fragmented SCSI bootmap).
>>>
>>> So strictly speaking this if statement might not be needed any more:
>>>         if (i == (max_entries - 1)) {
>>>
>>> Eugene, can you confirm?  If yes we could add this patch later on as a 
>>> cleanup:
>>
>> I'd preserve both checks.
>> In theory, we may catch a table that consumes all scratch space and
>> leave no unused entry.
>>
>> Plus, this check for zero counter and last entry is for "continuation"
>> pointer, not for end-of-table by itself.
>>
>> I think now, this code may need even few more checks to cover more cases...
>>
> Ok. That means, that this patch as is, doesnt make anything worse. Correct?
> 
> I am expecting more fixes and cleanups for the bios code anyway, so as long 
> as we dont add a regression here this should be good to go as it makes the 
> whole code more flexible.
> 
> Christian
> 

-- 
Best Regards,
Eugene "jno" Dvurechenski
zLinux (KVM) Development - Software Engineer
IBM Russia - Science & Technology Center
phone: +7 (495) 660 8940 ext. 1021

jno.vcf
Description: Vcard

signature.asc
Description: OpenPGP digital signature