On Mon, Jun 30, 2014 at 1:38 PM, Julian Foad <julianfoad_at_btopenworld.com>
wrote:
> Stefan Fuhrmann wrote:
> > r1606554 generates the index data dynamically now.
>
> That looks *much* better to my eyes. Now it only has a very few magic
> "offset" and "length" constants which are on a par with those we already
> had in the r0 header in the previous format. I don't much mind those,
> although I'd be even happier if they weren't there either.
>
> > + /* If the config file has not been initialized, yet, set some
> defaults
> > + here for r0. r0 is so small we could do with any non-zero
> values. */
> > + if (ffd->l2p_page_size == 0)
> > + ffd->l2p_page_size = 0x2000;
> > + if (ffd->p2l_page_size == 0)
> > + ffd->p2l_page_size = 0x10000;
>
> It's not clear why it's acceptable to initialize just those two parameters
> -- it looks like an implementation detail of the particular calls you make.
> It would be better if the caller set up the config before calling this.
> There is only one caller and it sets up the default config immediately
> after calling this. Why not swap the order of calls so that it sets the
> config first and then calls this write_revision_zero()? It would make sense
> from a logical point of view that the configuration is needed to tell the
> system how to write a revision, whereas r0 doesn't need to exist in order
> to create a default config.
>
r1606744 now delays the creation of r0 until after the config
structures have been fully initialized. The only downside is
that we now use slightly different values than in the original
template. But apart from r0 getting 1 or 2 bytes larger, that
has zero effect further down the path.
-- Stefan^2.
Received on 2014-06-30 16:12:23 CEST