[Ocfs2-tools-devel] [PATCH 3/8] O2info: Build a main framework for o2info.

[Sunil Mushran]

tristan wrote:
>>> +#include "ocfs2/ocfs2.h"
>>> +#include "ocfs2/bitops.h"
>>> +#include "ocfs2-kernel/ocfs2_ioctl.h"
>>> +#include "ocfs2-kernel/kernel-list.h"
>>> +#include "tools-internal/verbose.h"
>>> +
>>> +#include "utils.h"
>>> +
>>> +static inline void __o2info_fill_request_header(struct 
>>> ocfs2_info_request *req,
>>> + size_t size,
>>> + enum ocfs2_info_type code,
>>> + int flags)
>>> +{
>>> + req->ir_magic = OCFS2_INFO_MAGIC;
>>> + req->ir_code = code;
>>> + req->ir_flags = flags;
>>> + req->ir_size = size;
>>> +}
>>> +
>>> +/*
>>> + * Use marco to let sizeof() act on the full request type.
>>> + */
>>> +#define o2info_fill_request(req, code, flags) do { \
>>> + memset(req, 0, sizeof(*req)); \
>>> + __o2info_fill_request_header((struct ocfs2_info_request *)req, \
>>> + sizeof(*req), code, flags); \
>>> +} while (0)
>>
>> Typically macros should be one liners. Also, here you have a macro
>> calling an inline.
>>
>> Why not just make it a function? The caller will look something like 
>> this.
>>
>> o2info_fill_request_header(&req, sizeof(req), CODE, FLAGS);
>
> Sunil,
>
> I still love the macro's way, though we're doing two operations in one 
> macro definition, that's still acceptable.
> The point why I prefer this way is, we'll let sizeof() act powerfully 
> on the full request type, since it's just a simple replace of code 
> strings.
>
> Otherwise, think about 'memset(req, 0, sizeof(*req))', we'll not be 
> able to initialize all request type here if using function. instead, 
> only the header of each request get initialized indeed.

Multiline macros are harder to debug. We made a conscious decision
to use inline functions in such cases. I just scanned the code. I
cannot see any use of multiline macros. We've used (inlined) functions
instead.

Just pass in the sizeof(). It is not the end of the world.