On 1/8/2025 9:38 PM, Roopni Devanathan wrote: > > > On 1/8/2025 5:08 AM, Jeff Johnson wrote: >> On 1/7/2025 11:45 AM, Kalle Valo wrote: >>> Roopni Devanathan <quic_rdevanat@xxxxxxxxxxx> writes: >>>> From: Pradeep Kumar Chitrapu <quic_pradeepc@xxxxxxxxxxx> >>>> + len--; >>>> + *(buf + len) = '\0'; >>> >>> Please avoid pointer arithmetic, this is simpler: >>> >>> buf[len] = '\0'; >>> >>> Or should it be just 0 instead of '\0'? Don't know which one is >>> preferred. >>> >> >> well my take on this is we have a lot of similar pointer arithmetic already in >> this and other debugfs files, including all of the scnprintf(buf + len, ...) >> calls which could be scnprintf(&buf[len], ...). And we have existing instances >> of trailing comma suppression in: >> print_array_to_buf_index() >> ath12k_htt_print_txbf_ofdma_ax_ndpa_stats_tlv() >> ath12k_htt_print_txbf_ofdma_ax_ndp_stats_tlv() >> and others, all of which assign '\0' >> >> so this code looks ok to me. >> >> that said, while i've been reviewing the debugfs implementations I thought the >> code would look much cleaner if we had a wrapper function (or macro). >> >> Note that the input to the current debugfs functions includes a struct >> debug_htt_stats_req *stats_req that maintains the buffer pointer and filled >> length. what if we had a wrapper that took just that, the format string, and >> the optional arguments, and all of the pointer arithmetic is encapsulated in >> that function, rather than being open coded? >> >> so instead of: >> len += scnprintf(buf + len, buf_len - len, "some string""); >> have simply: >> ath12k_scnprintf(stats_req, "some string"); >> >> that function would extract the buffer pointer and current filled length from >> stats_req, calculate where to write the next string, call scnprintf() or >> vscnprintf() to write the string, and then update the current filled length in >> the stats_req. That would eliminate all of this housekeeping from the callers: >> buf + len >> buf_len - len >> len += scnprintf(..) >> >> /jeff >> > > Since there are a lot of instances where this change is needed, can we raise a > separate patch for this? If that is okay, we can go ahead with the current > implementation in this patch. Yes, I'm taking the current implementation to ath-next /jeff
