[PATCH v2] makedumpfile: code cleanup: set_bitmap

[Wang Nan]

On 2014/4/25 16:03, Petr Tesarik wrote:
> On Fri, 25 Apr 2014 13:25:23 +0800
> Wang Nan <wangnan0 at huawei.com> wrote:
> 
>> On 2014/4/24 17:28, Petr Tesarik wrote:
>>> On Thu, 24 Apr 2014 17:11:13 +0800
>>> Wang Nan <wangnan0 at huawei.com> wrote:
>> [...]
>>>>  set_bitmap(struct dump_bitmap *bitmap, unsigned long long pfn,
>>>>      int val)
>>>>  {
>>>> @@ -3317,20 +3345,11 @@ set_bitmap(struct dump_bitmap *bitmap, unsigned long long pfn,
>>>>  	old_offset = bitmap->offset + BUFSIZE_BITMAP * bitmap->no_block;
>>>>  	new_offset = bitmap->offset + BUFSIZE_BITMAP * (pfn / PFN_BUFBITMAP);
>>>
>>> After applying this patch, both old_offset and new_offset are
>>> calculated only to compare for equality. And new_offset is in fact
>>> computed twice (once in set_bitmap and once in sync_bitmap).
>>>
>>> This could be cleaned up by replacing the offsets with:
>>>
>>> 	int new_no_block = pfn / PFN_BUFBITMAP;
>>>
>>> Then change all occurrences of if (old_offset != new_offset) to:
>>>
>>> 	if (bitmap->no_block != new_no_block)
>>>
>>> and finally re-use new_no_block in the assignment to bitmap->no_block
>>> near the end of the function, like this:
>>>
>>> -		bitmap->no_block = pfn / PFN_BUFBITMAP;
>>> +		bitmap->no_block = new_no_block;
>>>
>>> Petr T
>>>
>>>>  
>>>> -	if (0 <= bitmap->no_block && old_offset != new_offset) {
>>>> -		if (lseek(bitmap->fd, old_offset, SEEK_SET) < 0 ) {
>>>> -			ERRMSG("Can't seek the bitmap(%s). %s\n",
>>>> -			    bitmap->file_name, strerror(errno));
>>>> -			return FALSE;
>>>> -		}
>>>> -		if (write(bitmap->fd, bitmap->buf, BUFSIZE_BITMAP)
>>>> -		    != BUFSIZE_BITMAP) {
>>>> -			ERRMSG("Can't write the bitmap(%s). %s\n",
>>>> -			    bitmap->file_name, strerror(errno));
>>>> +	if (old_offset != new_offset) {
>>>> +		if (!sync_bitmap(bitmap)) {
>>>> +			ERRMSG("Can't sync bitmap\n");
>>>>  			return FALSE;
>>>>  		}
>>>> -	}
>>>> -	if (old_offset != new_offset) {
>>>>  		if (lseek(bitmap->fd, new_offset, SEEK_SET) < 0 ) {
>>>>  			ERRMSG("Can't seek the bitmap(%s). %s\n",
>>>>  			    bitmap->file_name, strerror(errno));
>> [ .. see below .. ]
>>
>> Good suggestion, but new_offset is still needed to be calculated for these lseeks.
> 
> True, but it will be calculated only once (in sync_bitmap). OTOH the
> block number is always needed, because it is stored in bitmap->no_block.
> 
> Okay, that can be changed, and you can store the offset instead. I
> don't have a strong opinion on this.
> 
>> In addition, I have another idea: what about to replace all lseek .. read/write pattern to pread/pwrite?
> 
> Definitely! After doing that, we could even reuse the same file descriptor
> for all processes.
> 

I posted a series of patches for lseek. If Atsushi Kumagai accept them, I will
redo this patch on top of them.

> Now, since the original patch looks good as it is, let's see if Atsushi
> Kumagai takes it into the tree and post more clean up patches afterwards.
> 
> Petr T
>