[Libguestfs] [PATCH 9/12] hivex: Begin implementation of writing to hives.

[Richard W.M. Jones]

On Thu, Feb 04, 2010 at 04:54:53PM +0000, Matthew Booth wrote:
[...]

Thanks for this review.  Because I'm about 20 commits ahead of the
public git repo at the moment, what I will do is to add another commit
which addresses documentation issues in the code retrospectively.

More comments inline.

> > +static size_t
> > +allocate_page (hive_h *h, size_t allocation_hint)
> > +{
> 
> Every usage of nr_4k_pages in this function multiplies it by 4096. It
> would be more readable to choose a different name which reflects that
> it's simply a rounding.

It's the number of 4K pages, so it seems like a reasonable name to me ...

> > +  /* In almost all cases this will be 1. */
> > +  size_t nr_4k_pages =
> > +    1 + (allocation_hint + sizeof (struct ntreg_hbin_page) - 1) / 4096;
> > +  assert (nr_4k_pages >= 1);
> 
> I think this assert is impossible to trigger.

I hope so too, but the assertion is there to remind me and others that
this value can't be zero.

[in allocate_page]
> In general, are we worrying about overflow?

In general yes, but in this function it shouldn't be possible to
trigger any overflow.  (Having said that, who knows what strange
combination of inputs might trigger an overflow ...)

> > +  page->offset_first = htole32 (offset - 0x1000);
> 
> What's 0x1000? Can you comment this?

The first 4096 bytes of the file are the header.  Internal pointers
within the file are relative to the first block, which is to say,
relative to the file start + 4096 bytes.  Unfortunately the
representation I chose for the file is a bit stupid in that we store
the header + blocks as a single allocation, instead of storing those
separately.  (I didn't make the same mistake in the OCaml analysis
tools).  I intend to go back and correct this, but first I want a
comprehensive test suite so that we don't introduce regressions.

> > +  if (seg_len < 4) {
> 
> Can you replace this '4' with a sizeof(ntreg_hbin_block.seg_len)?

No because that's a syntax error ...  Apparently it's not possible to
portably get the size of a structure member.

> > +  blockhdr->seg_len = htole32 (- (int32_t) seg_len);
> > +  if (id[0] && id[1] && seg_len >= 6) {
> 
> Can you replace 6 with sizeof(ntreg_hbin_block)?

Or more correctly, offsetof (ntreg_hbin_block, <the field after
id[1]>), but of course you can't do that in C ...  Stupid language.

> > +    assert (rem >= 4);
> 
> This assertion doesn't look safe. Why must this be the case? In the
> event that rem < 4, would you allocate a new page for the dummy free block?

Allocations are aligned to and rounded up to 8 bytes, so rem should be
0 or >= 8.  We know rem > 0 because we're in a conditional.  We need
to write 4 bytes here.  Hence the assertion.

> > +    blockhdr = (struct ntreg_hbin_block *) (h->addr + h->endblocks);
> > +    blockhdr->seg_len = htole32 ((int32_t) rem);
> 
> Will blockhdr->id ever be read here? Is 0x00 valid?

blockhdr->id is not read for unused blocks, so this is safe.

> > +    size_t len;
> > +    len = le32toh (vk->data_len);
> > +    if (len == 0x80000000)      /* special case */
> > +      len = 4;
> > +    len &= 0x7fffffff;
> 
> Comment, please. What's going on with this record length?

This is a mysterious and undocumented part of the format.  I don't
claim to understand why, but it is necessary.

> > +int
> > +hivex_commit (hive_h *h, const char *filename, int flags)
> > +{
> > +  if (flags != 0) {
> > +    errno = EINVAL;
> > +    return -1;
> > +  }
> 
> What are you intending to use the flags for?

Future expansion.

> > +int
> > +hivex_node_set_values (hive_h *h, hive_node_h node,
> > +                       size_t nr_values, const hive_set_value *values,
> > +                       int flags)
> > +{
> > +  if (!h->writable) {
> > +    errno = EROFS;
> > +    return -1;
> > +  }
> > +
> > +  if (!IS_VALID_BLOCK (h, node) || !BLOCK_ID_EQ (h, node, "nk")) {
> > +    errno = EINVAL;
> > +    return -1;
> 
> This would be an internal inconsistency error, surely. If so, you
> probably want an assert here instead.

No, it could mean that someone passed in an offset which is a block
of some sort, but is not an nk-record.  eg:

  size_t *values = hivex_node_values (h, node);
  hivex_node_set_values (h, values[0], ...);

Unfortunately (and another stupidity about C) size_t and hive_node_h
are compatible types so this won't generate any compile-time error.

> > +  size_t seg_len =
> > +    sizeof (struct ntreg_value_list) + (nr_values - 1) * sizeof (uint32_t);
> 
> Why are you subtracting 1 from nr_values above?

There is already 1 value in the struct:

struct ntreg_value_list {
  int32_t seg_len;
  uint32_t offset[1];           /* list of pointers to vk records */
} __attribute__((__packed__));

> > +    if (values[i].len <= 4)     /* Store data inline. */
> 
> sizeof.

No, it's a fixed magic number in the format.  If the length is <= 4 it
stores it inline.

> > +      memcpy (&vk->data_offset, values[i].value, values[i].len);
> > +    else {
> > +      size_t offs = allocate_block (h, values[i].len + 4, nul_id);
> 
> sizeof.

There's no struct to take the sizeof here.  These are special id-less
blocks.

> > +      if (offs == 0)
> > +        return -1;
> > +      memcpy (h->addr + offs + 4, values[i].value, values[i].len);
> 
> sizeof, or the address of a struct member.

As above.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v