Re: [Qemu-devel] [PATCH v4 09/11] DumpState: adding total_size and written_size fields

[Peter Xu <peterx@redhat.com>]

On Wed, Dec 02, 2015 at 09:32:57AM +0800, Fam Zheng wrote:
> On Tue, 12/01 21:28, Peter Xu wrote:
> > @@ -333,6 +333,8 @@ static void write_data(DumpState *s, void *buf, int length, Error **errp)
> >      if (ret < 0) {
> >          error_setg(errp, "dump: failed to save memory");
> >      }
> > +
> > +    s->written_size += length;
> 
> If (ret < 0), the incremental is inaccurate, do we want an accurate
> written_size in that case? I.e. put this in "else" branch?

Yes, I think I'd better put it into an else block... Thanks.

Actually even that could not be accurate, since the write() could be
partly done and failed. But I hope that's good enough for dump
status queries. :)

> 
> >  }
> >  
> >  /* write the memory to vmcore. 1 page per I/O. */
> > @@ -1301,6 +1303,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
> >                  goto out;
> >              }
> >          }
> > +        s->written_size += TARGET_PAGE_SIZE;
> 
> The same question applies here.

For kdump case, it is using "goto out" when error happens. So it
seems to have no problem here?

> 
> >      }
> >  
> >      ret = write_cache(&page_desc, NULL, 0, true);
> > @@ -1433,6 +1436,30 @@ bool dump_in_progress(void)
> >      return (state->status == DUMP_STATUS_ACTIVE);
> >  }
> >  
> > +/* calculate total size of memory to be dumped (taking filter into
> > + * acoount.) */
> > +static size_t dump_calculate_size(DumpState *s)
> 
> Is size_t big enough for 64 bit guest on 32 bit host (with 4 bytes size_t)?

Thanks to point out. Will use int64_t here (and also written_size
and total_size).

> > +    size_t written_size;        /* written memory size (in bytes),
> > +                                 * this could be used to calculate
> > +                                 * how many work we have
> 
> s/many/much/

Will fix it. Thanks!

Peter