[Ocfs2-tools-devel] [PATCH 1/1] wireshark-ocfs2: add dlm_query_join handler to support both ocfs2-1.2 and the newer format

[jeff.liu]

Hi Sunil,

Sunil Mushran 写道:
> Can we split this into two. One for request and the other for response.
> While they are related, the changes are enough that it would be easier
> to review them separately.
>
Yes, I will send the separated patches out later.
> Secondly, always treat the current version of the structure as the 
> correct
> one. Meaning, dlm_query_join_request_latest should be 
> dlm_query_join_request
> and the older one should be marked with some other decoration.
>
For the old version, I append the version minor number to the ending of 
structure name.
In this case, it named as dlm_query_join_request_129, since we always 
dealing with the latest one
for each release.
> One small point I had not mentioned and may be causing some confusion wrt
> dlm_query_join_request. The original structure in the dissector does not
> include the node_map. Yes, we made this change in 1.2.5 or so. But we'll
> ignore that case. All this dissector needs to do is handle these two 
> variations.
>
> struct dlm_query_join_request {
> guint8 node_idx;
> guint8 pad1[2];
> guint8 name_len;
> struct dlm_protocol_version dlm_proto;
> struct dlm_protocol_version fs_proto;
> guint8 domain[O2NM_MAX_NAME_LEN];
> guint8 node_map[BITS_TO_BYTES(O2NM_MAX_NODES)];
> };
>
> struct _dlm_query_join_request {
> guint8 node_idx;
> guint8 pad1[2];
> guint8 name_len;
> guint8 domain[O2NM_MAX_NAME_LEN];
> guint8 node_map[BITS_TO_BYTES(O2NM_MAX_NODES)];
> };
>
the node_map will added into the original structure, this change will 
reflected in my patches later.
> Lastly, don't write details source logic in the patch description. Let 
> the
> code talk for itself. The description should be what-it-is rather than 
> how-it-was-done.
>
I shall bear in mind your advice. :)

> Thanks
> Sunil
>

Regards,
Jeff
> Jeff Liu wrote:
>> dlm_query_join(request/response) messages structure have been changed 
>> from ocfs2-1.4.
>> we want the dissector can handle both ocfs2-1.2 and the newer format.
>>
>> the method used to distinct the different ocfs2 version:
>> 1.to handle the dlm_query_join_request
>> check whether the name_len is eqaul to the left msg length or not.
>> if name_len == left_msg_len, dissect the domain_name as ocfs2-1.2,
>> else if the name_len > left_msg_len, dissect domain_name as ocfs2-1.4 
>> and newer.
>>
>> 2.to handle the dlm_query_join_response
>> for ocfs-1.2, the response value is converted to big-endian before
>> putting on the wire, so swap it back to little-endian in dissector,
>> then check if its value is less-than or eual to 2(JOIN_OK_NO_MAP),
>> if so, NOP here.
>>
>> for ocfs-1.4 and newer, the response code is in big-endian before 
>> sending over wire as well,
>> tvb_get_ntohl() convert it to little-endian, but it is made up of 
>> four one-byte fields and in
>> big-endian order originally, so convert it to big-endian first.
>> Also, the 2nd and 3rd byte are valid only if the first byte is 
>> JOIN_OK. In this case, wrap these 3 values
>> in parenthesis separated by colon, then append to status line, 
>> otherwise, just the code value appended.
>>
>> Signed-off-by: Jeff Liu <jeff.liu at oracle.com>
>> ---
>> epan/dissectors/packet-ocfs2.c | 123 
>> ++++++++++++++++++++++++++++++++++-----
>> 1 files changed, 107 insertions(+), 16 deletions(-)
>>
>> diff --git a/epan/dissectors/packet-ocfs2.c 
>> b/epan/dissectors/packet-ocfs2.c
>> index ce57c3f..f89d85d 100644
>> --- a/epan/dissectors/packet-ocfs2.c
>> +++ b/epan/dissectors/packet-ocfs2.c
>> @@ -229,12 +229,6 @@ const char * 
>> decode_enumerated_bitfield_full(guint32 val, guint32 mask, int widt
>> #define DLM_LVB_LEN 64
>> #define DLM_MOD_KEY (0x666c6172)
>>
>> -enum dlm_query_join_response {
>> - JOIN_DISALLOW = 0,
>> - JOIN_OK,
>> - JOIN_OK_NO_MAP,
>> -};
>> -
>>
>> /* DLM lock modes */
>> enum {
>> @@ -811,6 +805,46 @@ struct dlm_finalize_reco
>> guint32 pad2; // unused
>> };
>>
>> +struct dlm_protocol_version {
>> + guint8 pv_major;
>> + guint8 pv_minor;
>> +};
>> +
>> +#define O2NM_MAX_NODES 255
>> +#define BITS_PER_BYTE 8
>> +#define BITS_TO_BYTES(bits) (((bits)+BITS_PER_BYTE-1)/BITS_PER_BYTE)
>> +struct dlm_query_join_request_latest {
>> + guint8 node_idx;
>> + guint8 pad1[2];
>> + guint8 name_len;
>> + struct dlm_protocol_version dlm_proto;
>> + struct dlm_protocol_version fs_proto;
>> + guint8 domain[O2NM_MAX_NAME_LEN];
>> + guint8 node_map[BITS_TO_BYTES(O2NM_MAX_NODES)];
>> +};
>> +
>> +enum dlm_query_join_response_code {
>> + JOIN_DISALLOW = 0,
>> + JOIN_OK,
>> + JOIN_OK_NO_MAP,
>> + JOIN_PROTOCOL_MISMATCH,
>> +};
>> +
>> +struct dlm_query_join_packet {
>> + guint8 code; /* Response code. dlm_minor and fs_minor
>> + are only valid if this is JOIN_OK */
>> + guint8 dlm_minor; /* The minor version of the protocol the
>> + dlm is speaking. */
>> + guint8 fs_minor; /* The minor version of the protocol the
>> + filesystem is speaking. */
>> + guint8 reserved;
>> +};
>> +
>> +union dlm_query_join_response {
>> + guint32 intval;
>> + struct dlm_query_join_packet packet;
>> +};
>> +
>> #define DLM_FIELD_BYTES_LEN 24
>> #define LVB_REMAIN_BYTES_LEN (DLM_LVB_LEN - (DLM_FIELD_BYTES_LEN << 1))
>> #define LVB1_OFFSET(x) offsetof(x, lvb[0])
>> @@ -946,12 +980,6 @@ static struct dlm_msg_struct_def 
>> dlm_struct_defs[] = {
>> { "dead_node", &hf_dlm_dead_node, FIELD_OFFSET_AND_SIZE(struct 
>> dlm_begin_reco,dead_node), dlm_node_idx_handler},
>> { DLM_MSG_STRUCT_DEF_END } }
>> },
>> -{ "dlm_query_join_request", "DLM Query Join Request", 
>> DLM_QUERY_JOIN_MSG, &ett_dlm_query_join, {
>> - { "node_idx", &hf_dlm_node_idx, FIELD_OFFSET_AND_SIZE(struct 
>> dlm_query_join_request,node_idx), dlm_node_idx_handler},
>> - { "name_len", &hf_dlm_domain_name_len,FIELD_OFFSET_AND_SIZE(struct 
>> dlm_query_join_request,name_len), dlm_domain_namelen_handler},
>> - { "domain", &hf_dlm_domain_name, FIELD_OFFSET_AND_SIZE(struct 
>> dlm_query_join_request,domain), dlm_domain_name_handler},
>> - { DLM_MSG_STRUCT_DEF_END } }
>> -},
>> { "dlm_assert_joined", "DLM Assert Joined", DLM_ASSERT_JOINED_MSG, 
>> &ett_dlm_assert_joined, {
>> { "node_idx", &hf_dlm_node_idx, FIELD_OFFSET_AND_SIZE(struct 
>> dlm_assert_joined,node_idx), dlm_node_idx_handler},
>> { "name_len", &hf_dlm_domain_name_len,FIELD_OFFSET_AND_SIZE(struct 
>> dlm_assert_joined,name_len), dlm_domain_namelen_handler},
>> @@ -1148,10 +1176,64 @@ static void 
>> msg_struct_enumerate_fields(struct dlm_msg_struct_def *def, proto_tr
>> }
>> }
>>
>> +static void dissect_dlm_query_join_request(proto_tree *tree, 
>> tvbuff_t *tvb,
>> + int offset)
>> +{
>> + guint8 namelen;
>> + gint len;
>> +
>> + proto_tree_add_item(tree, hf_dlm_node_idx, tvb, offset, 1, FALSE);
>> +
>> + /* skip the following 2 padding bytes */
>> + offset += 3;
>> + namelen = tvb_get_guint8(tvb, offset);
>> + proto_tree_add_item(tree, hf_dlm_namelen, tvb, offset, 1, FALSE);
>> +
>> + len = tvb_reported_length_remaining(tvb, offset);
>> +
>> + ++offset;
>> + /* if the left message length more than namelen, its most likely
>> + * ocfs2-1.4 or later we are dealing with, or else ocfs2-1.2 or
>> + * mismatch */
>> + if (len > namelen)
>> + proto_tree_add_item(tree, hf_dlm_domain_name, tvb,
>> + offset + 4, namelen, FALSE);
>> + else if (len == namelen)
>> + proto_tree_add_item(tree, hf_dlm_domain_name, tvb,
>> + offset, namelen, FALSE);
>> +}
>> +
>> +static void dissect_dlm_query_join_response(proto_tree *tree,
>> + proto_item *item,
>> + tvbuff_t *tvb,
>> + int offset)
>> +{
>> + union dlm_query_join_response response;
>> + struct dlm_query_join_packet packet;
>> + guint32 status;
>> +
>> + status = tvb_get_ntohl(tvb, offset);
>> +
>> + /* NOP if ocfs2-1.2.x message coming */
>> + if (status < JOIN_PROTOCOL_MISMATCH)
>> + return ;
>> +
>> + /* for ocfs2-1.4.x and newer */
>> + response.intval = htonl(status);
>> + packet = response.packet;
>> + proto_item_append_text(item, " (%u:", packet.code);
>> +
>> + if (packet.code == JOIN_OK)
>> + proto_item_append_text(item, "%u:%u",
>> + packet.dlm_minor, packet.fs_minor);
>> +
>> + proto_item_append_text(item, ")");
>> +}
>> +
>> static int dissect_ocfs2(tvbuff_t *tvb, packet_info *pinfo, 
>> proto_tree *tree)
>> {
>> proto_tree *subtree = NULL;
>> - proto_item *ti;
>> + proto_item *ti, *si;
>> guint16 len, magic, msg_type;
>> int ret;
>>
>> @@ -1188,9 +1270,9 @@ static int dissect_ocfs2(tvbuff_t *tvb, 
>> packet_info *pinfo, proto_tree *tree)
>> proto_tree_add_item(subtree, hf_msg_sys_status,
>> tvb, O2NET_MSG_HDR_OFF_SYS_STATUS, 4,
>> FALSE);
>> - proto_tree_add_item(subtree, hf_msg_status,
>> - tvb, O2NET_MSG_HDR_OFF_STATUS, 4,
>> - FALSE);
>> + si = proto_tree_add_item(subtree, hf_msg_status,
>> + tvb, O2NET_MSG_HDR_OFF_STATUS, 4,
>> + FALSE);
>> proto_tree_add_item(subtree, hf_msg_key,
>> tvb, O2NET_MSG_HDR_OFF_KEY, 4,
>> FALSE);
>> @@ -1247,6 +1329,15 @@ static int dissect_ocfs2(tvbuff_t *tvb, 
>> packet_info *pinfo, proto_tree *tree)
>> if (def)
>> msg_struct_enumerate_fields(def, subtree, tvb, 
>> O2NET_MSG_HDR_OFF_PAYLOAD);
>> }
>> +
>> + if (msg_type == DLM_QUERY_JOIN_MSG) {
>> + if (magic == O2NET_MSG_MAGIC)
>> + dissect_dlm_query_join_request(subtree,
>> + tvb, O2NET_MSG_HDR_OFF_PAYLOAD);
>> + else if (magic == O2NET_MSG_STATUS_MAGIC)
>> + dissect_dlm_query_join_response(subtree,
>> + si, tvb, O2NET_MSG_HDR_OFF_STATUS);
>> + }
>> }
>> out:
>> return ret;
>