[PATCH v2] makedumpfile: code cleanup: set_bitmap

[Petr Tesarik]

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.

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