There are some other problems too:
makebuf() [ln 701] does this:
uint64_t* p = (uint64_t*)buf;
in this, buf is a char*. This violates the strict aliasing rules that say a
char* may alias any type, but not the other way around: uint64_t* may not
alias a char*. The difference is subtle: modifying buf[] will cause a
reload of p[] values, but modifying p[] will not cause a reload of buf[].
Though a violation, it is probably safe since only one is accessed in this
scope.
Also, checkbuf() [ln 717] does this:
char cmp[512];
makebuf(cmp, seq, blknum);
This means 'cmp' is going to be reinterpreted as a uint64_t in makebuf() --
but the char[] base type does guarantee 8-byte alignment, so this may or
may not fail. It should probably be declared as:
uint64_t cmp[64]; // 512/8 = 64
makebuf((char*)cmp, seq, blknum);
Changing the protoype of makebuf() to take uint64_t may be appropriate as
well since it seems to be called in that context.
Patrick
On Sat, Mar 3, 2012 at 10:11 AM, Julien Cristau <jcris...@debian.org> wrote:
> On Sat, Mar 3, 2012 at 09:55:51 -0600, Patrick Baggett wrote:
>
> > Where can I read the source for "nbd-tester-client.c"? I just did a quick
> > scan of "ghash.c" but I didn't see anything really silly going on, so I'd
> > like to check this file. All copies I can find of "nbd-tester-client.c"
> > seem to be short (<600 lines) and not use glib at all.
> >
> It does this:
>
> /*
> * This is the reply packet that nbd-server sends back to the client after
> * it has completed an I/O request (or an error occurs).
> */
> struct nbd_reply {
> __be32 magic;
> __be32 error; /* 0 = ok, else error */
> char handle[8]; /* handle you got from request */
> };
> [...]
>
> GHashTable *handlehash = g_hash_table_new(g_int64_hash, g_int64_equal);
> struct nbd_reply rep;
>
> READ_ALL_ERRCHK(sock,
> &rep,
> sizeof(struct nbd_reply),
> err_open,
> "Could not read from server socket: %s",
> strerror(errno));
> [...]
> prc = g_hash_table_lookup(handlehash, rep.handle);
>
> So alignof(struct nbd_reply) is 4, and rep.handle is not always 8-byte
> aligned. Either the handle should be made a uint64_t, or the test
> should do
>
> uint64_t handle;
> memcpy(&handle, &rep.handle, 8);
> prc = g_hash_table_lookup(handlehash, &handle);
>
> Cheers,
> Julien
>
