On Tue, Mar 24, 2009 at 6:27 PM, Yaniv Kamay <[email protected]> wrote: > Glauber Costa wrote: >> >> @@ -1223,12 +1244,44 @@ static int kvm_get_dirty_bitmap_cb(unsigned >> long start, unsigned long len, >> int kvm_update_dirty_pages_log(void) >> { >> int r = 0; >> + ram_addr_t now = 0; >> + ram_addr_t end = phys_ram_size; >> + ram_addr_t offset; >> + target_phys_addr_t area_start; >> + ram_addr_t area_size; >> + unsigned char *dirty_bitmap = kvm_dirty_bitmap; >> + >> + if (!dirty_bitmap) { >> + printf("%s: no dirty bitmap\n", __FUNCTION__); >> + return -1; >> + } >> nitpick: you probably want fprintf(stderr...). >> >> >> + while (now < end && !find_phys_area(now, &offset, &area_start, >> &area_size)) { >> + if ((offset & ~TARGET_PAGE_MASK) || (area_start & >> ~TARGET_PAGE_MASK) || >> + (area_size & >> ~TARGET_PAGE_MASK)) { >> + printf("%s: invalid mem area\n", __FUNCTION__); >> same here. >> >> >> >> + if ((now += offset) >= end) { >> + break; >> + } >> + >> + if (area_size > end - now) { >> + return -1; >> + } >> any reason why you're handling those two differently? >> > > The first is orderly end of mappings and the later is error. ok. I believe the code reads better if you handle all the errors together.
but this is nitpick too. The idea of the patch looks correct to me. >> >> -- >> Glauber Costa. >> "Free as in Freedom" >> http://glommer.net >> >> "The less confident you are, the more serious you have to act." >> > > -- Glauber Costa. "Free as in Freedom" http://glommer.net "The less confident you are, the more serious you have to act." -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
