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.
