[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On 05/02/10 12:46, Richard W.M. Jones wrote:
> 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 ...

I meant storing a value that is already multiplied by 4096, because
that's how it's used. Not important.

>>> +  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.

Ok.

>>> +  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.

The above wasn't intended to be syntactically correct. I think, however,
you can do something like:

sizeof(((struct ntreg_hbin_block *) void)->seg_len)

This would be best implemented as a macro.

>>> +  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.

If you redefine struct ntreg_hbin_block as:

struct ntreg_hbin_block {
  int32_t seg_len;
  char id[2];
  char data[];
} __attribute__((package__));

You can use:

offsetof(struct ntreg_hbin_block, data)

I haven't looked, but I'll bet gnulib contains a portable offsetof.

>>> +    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.

Ok.

>>> +    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.

Ok.

>>> +    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.

Hehe, ok. Can you at least put in a comment to that effect, preferrably
with a pointer to your source.

>>> +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.

This is a stylistic choice, I guess. I would say that incorrect use of
this function is a bug, therefore not something the user can do anything
about. The best outcome for the user is that it gets fixed :) I'd use an
assert. Not important.

>>> +  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__));

That's nasty. Is there a format reason for doing this, or could you
redefine this struct as:

struct ntreg_value_list {
  int32_t seg_len;
  uint32_t offset[];	/* 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.

Can you give the magic number a name and #define it up the top to be 4?

>>> +      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.

offsetof(struct ntreg_hbin_block, id)

and include a comment about overwriting id.

>>> +      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.

Ditto ;)

Matt
-- 
Matthew Booth, RHCA, RHCSS
Red Hat Engineering, Virtualisation Team

M:       +44 (0)7977 267231
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]