Hi, > -----Original Message----- > From: Michal Nazarewicz [mailto:mpn@xxxxxxxxxx] On Behalf Of Michal > Nazarewicz > Sent: Monday, June 09, 2014 3:28 PM > To: Krzysztof Opasiak; 'Felipe Balbi'; Krzysztof Opasiak > Cc: linux-usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > Subject: Re: [PATCHv2 5/5] tools: ffs-test: add compatibility code > for old kernels > > On Mon, Jun 09 2014, Krzysztof Opasiak wrote: > > As this is an example which will be copy-paste by many users > maybe you > > should you struct usb_functionfs_descs_head and struct > > usb_functionfs_descs_head_v2 instead of direct operations using > > hard-coded offsets to make this function more readable? > > v3 uses usb_functionfs_descs_head{_v2,} instead of hard-coded > offsets. > I also started wondering if it would make sense to go one step > further > and define a temporary structure holding the headers. Something > like: > > ----------- >8 ---------------------------------------------------- > ----- > /* Read v2 header */ > { > const struct { > const struct usb_functionfs_descs_head_v2 header; > const __le32 counts[]; > } __attribute__((packed)) *const in = descriptors; > const __le32 *counts = in->counts; > __u32 flags; > > if (le32_to_cpu(in->header.magic) != > FUNCTIONFS_DESCRIPTORS_MAGIC_V2) > return 0; > length = le32_to_cpu(in->header.length); > if (length <= sizeof in->header) > return 0; > length -= sizeof in->header; > flags = le32_to_cpu(in->header.flags); > if (flags & ~(FUNCTIONFS_HAS_FS_DESC | > FUNCTIONFS_HAS_HS_DESC | > FUNCTIONFS_HAS_SS_DESC)) > return 0; > > #define GET_NEXT_COUNT_IF_FLAG(ret, flg) do { \ > if (!(flags & (flg))) \ > break; \ > if (length < 4) \ > return 0; \ > ret = le32_to_cpu(*counts); \ > length -= 4; \ > ++counts; \ > } while (0) > > GET_NEXT_COUNT_IF_FLAG(fs_count, > FUNCTIONFS_HAS_FS_DESC); > GET_NEXT_COUNT_IF_FLAG(hs_count, > FUNCTIONFS_HAS_HS_DESC); > GET_NEXT_COUNT_IF_FLAG(count, FUNCTIONFS_HAS_SS_DESC); > > count = fs_count + hs_count; > if (!count) > return 0; > descs_start = descs_end = (const void *)counts; > > #undef GET_NEXT_COUNT_IF_FLAG > } > > ----------- >8 ---------------------------------------------------- > ----- > > /* Allocate legacy descriptors and copy the data. */ > { > struct { > struct usb_functionfs_descs_head header; > __u8 descriptors[]; > } __attribute__((packed)) *out; > > length = sizeof out->header + (descs_end - > descs_start); > out = malloc(length); > out->header.magic = > cpu_to_le32(FUNCTIONFS_DESCRIPTORS_MAGIC); > out->header.length = cpu_to_le32(length); > out->header.fs_count = cpu_to_le32(fs_count); > out->header.hs_count = cpu_to_le32(hs_count); > memcpy(out->descriptors, descs_start, descs_end - > descs_start); > *legacy = out; > } > ----------- >8 ---------------------------------------------------- > ----- > > Thoughts? Looks very good to me! In my opinion this one should be used, it emphasizes your intentions very well so it's perfect code for sample app. -- BR's Krzysztof Opasiak Samsung R&D Institute Poland Samsung Electronics k.opasiak@xxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
