On 05/04/2017 05:58 AM, Vladimir Sementsov-Ogievskiy wrote:

>>> @@ -70,6 +70,25 @@ struct NBDSimpleReply {
>>>   };
>>>   typedef struct NBDSimpleReply NBDSimpleReply;
>>>   +typedef struct NBDStructuredReplyChunk {
>>> +    uint32_t magic;
>>> +    uint16_t flags;
>>> +    uint16_t type;
>>> +    uint64_t handle;
>>> +    uint32_t length;
>>> +} QEMU_PACKED NBDStructuredReplyChunk;
>>> +
>>> +typedef struct NBDStructuredRead {
>>> +    NBDStructuredReplyChunk h;
>>> +    uint64_t offset;
>>> +} QEMU_PACKED NBDStructuredRead;
>>> +
>>> +typedef struct NBDStructuredError {
>>> +    NBDStructuredReplyChunk h;
>>> +    uint32_t error;
>>> +    uint16_t message_length;
>>> +} QEMU_PACKED NBDStructuredError;
>> Definitely a subset of all types added in the NBD protocol extension,
>> but reasonable for a minimal implementation.  Might be worth adding
>> comments to the types...
> 
> Hmm, for me their names looks descriptive enough, but my sight may be
> biased.. What kind of comments do you want?

I guess I was thinking of existing structs in include/block/nbd.h:

/* Handshake phase structs - this struct is passed on the wire */

struct nbd_option {
    uint64_t magic; /* NBD_OPTS_MAGIC */
    uint32_t option; /* NBD_OPT_* */
    uint32_t length;
} QEMU_PACKED;
typedef struct nbd_option nbd_option;


and compared to:

/* Transmission phase structs
 *
 * Note: these are _NOT_ the same as the network representation of an NBD
 * request and reply!
 */
struct NBDRequest {
    uint64_t handle;
    uint64_t from;
    uint32_t len;
    uint16_t flags; /* NBD_CMD_FLAG_* */
    uint16_t type; /* NBD_CMD_* */
};
typedef struct NBDRequest NBDRequest;

where the comments make it obvious whether QEMU_PACKED matters because
we are using the struct to directly map bytes read/written on the wire.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org