Re: [Qemu-block] [Qemu-devel] [PATCH v5 1/9] util: Add UUID API

[Fam Zheng]

On Fri, 08/12 07:24, Richard Henderson wrote:
> On 08/12/2016 05:52 AM, Fam Zheng wrote:
> >+/* Version 4 UUID (pseudo random numbers), RFC4122 4.4. */
> >+
> >+typedef struct {
> >+    union {
> >+        unsigned char data[16];
> >+        struct {
> >+            /* Generated in BE endian, can be swapped with qemu_uuid_bswap. 
> >*/
> 
> Nit: BE endian is redundant.  big-endian or just BE, please.
> 
> >+            uint32_t time_low;
> >+            uint16_t time_mid;
> >+            uint16_t time_high_and_version;
> >+            uint8_t  clock_seq_and_reserved;
> >+            uint8_t  clock_seq_low;
> >+            uint8_t  node[6];
> >+        } fields;
> >+    };
> >+} QemuUUID;
> 
> Wait, didn't you just tell me that you *couldn't* add this union
> because of the packed usage within some file?

I was just saying even with this union added, packed usage can still result in
unaligned QemuUUID allocation, in theory. That's why I said I could assert too.
(In practise the only qemu_uuid_bswap is putting QemuUUID in a packed struct,
but that is happily aligned.)

> 
> But of course if you're going to do this, you should actually use these 
> fields,
> 
> >+    /* Set the two most significant bits (bits 6 and 7) of the
> >+      clock_seq_hi_and_reserved to zero and one, respectively. */
> >+    uuid->data[8] = (uuid->data[8] & 0x3f) | 0x80;
> >+    /* Set the four most significant bits (bits 12 through 15) of the
> >+      time_hi_and_version field to the 4-bit version number.
> >+      */
> >+    uuid->data[6] = (uuid->data[6] & 0xf) | 0x40;
> 
> Here,

Using these fields here requires an extra cpu_to_be16s for no compelling
reason, so I refrained to.

Fam

> 
> >+void qemu_uuid_bswap(QemuUUID *uuid)
> >+{
> >+    assert(QEMU_IS_ALIGNED((uint64_t)uuid, sizeof(uint32_t)));
> >+    bswap32s(&uuid->fields.time_low);
> >+    bswap16s(&uuid->fields.time_mid);
> >+    bswap16s(&uuid->fields.time_high_and_version);
> 
> And then you don't need this assert.
> 
> 
> r~
>