Re: [Qemu-devel] [PATCH v6 08/20] block: vhdx - log parsing, replay, and flush support

[Stefan Hajnoczi]

On Tue, Oct 01, 2013 at 09:24:53AM -0400, Jeff Cody wrote:
> On Tue, Oct 01, 2013 at 01:31:36PM +0200, Stefan Hajnoczi wrote:
> > On Wed, Sep 25, 2013 at 05:02:53PM -0400, Jeff Cody wrote:
> > > +    } else if (!memcmp(&desc->signature, "zero", 4)) {
> > > +        /* write 'count' sectors of sector */
> > > +        memset(buffer, 0, VHDX_LOG_SECTOR_SIZE);
> > > +        count = desc->zero_length / VHDX_LOG_SECTOR_SIZE;
> > 
> > Zero descriptors also have a sequence number that should be checked.
> > 
> 
> At this point, they already are - vhdx_log_desc_is_valid() has
> validated the descriptor sequence numbers by now.  This check is
> making sure the sequence number in the actual data sector is a match
> (there is no zero sector by definition).  
> 
> vhdx_log_desc_is_valid() is called from within vhdx_log_read_desc(),
> and will return -EINVAL if the sequence number is not a match.

Makes sense, thanks.  I missed it.

> > > @@ -821,20 +782,33 @@ static int vhdx_open(BlockDriverState *bs, QDict 
> > > *options, int flags,
> > >          goto fail;
> > >      }
> > >  
> > > -    ret = vhdx_parse_log(bs, s);
> > > +    ret = vhdx_open_region_tables(bs, s);
> > >      if (ret) {
> > >          goto fail;
> > >      }
> > >  
> > > -    ret = vhdx_open_region_tables(bs, s);
> > > +    ret = vhdx_parse_metadata(bs, s);
> > >      if (ret) {
> > >          goto fail;
> > >      }
> > >  
> > > -    ret = vhdx_parse_metadata(bs, s);
> > > +    ret = vhdx_parse_log(bs, s, &log_flushed);
> > >      if (ret) {
> > >          goto fail;
> > >      }
> > 
> > Why replay the log *after* reading logged metadata?  We could read stale
> > or corrupted values.
> > 
> > I guess there is a dependency here - the log replay code needs some
> > field that vhdx_open_region_tables() or vhdx_parse_metadata() load?
> 
> The main reason I did it this way was so that memory region overlap
> could be detected, prior to writing anything to the file.  If the log
> overlaps with any region, we can error out without modifying the image
> file.
> 
> Outside of the header section, there is no direct metadata or region
> table dependency for the log, except from needing to parse those to
> determine any potential overlaps with the log.
> 
> I originally had the log flushed first - but moved it down after
> adding the region table detection.

I'd opt for replaying the log.  The only things designed to be
consistent and reliable in VHDX are the header and the log.

Any other structure can be corrupted or stale.  Therefore, I think we
should trust the log and check for overlap later.