On 10/3/18 9:00 AM, Carlos Maiolino wrote: > On Wed, Oct 03, 2018 at 07:51:43AM -0500, Eric Sandeen wrote: >> On 10/3/18 7:35 AM, Carlos Maiolino wrote: >>> xqmstat information under __xfsstats is used directly by the >>> /proc/fs/xfs/xqmstat show method, which makes use of struct offsets to >>> print out the values. >>> >>> Currently, the function is setup to start printing values from the offset of >>> the last marker before xqmstat, so, an update to __xfsstats structure >>> may also require an update of xqmstat_proc_show() function. >>> >>> By adding a new constant, we concentrate any updates do __xfsstats >>> structure fields locally to the file xfs_stats.h, reducing the >>> likelyhood of future bugs if an update to xqmstat_proc_show is >>> forgotten as have happened before. >> >> did >> >> BUILD_BUG_ON(offsetof(struct __xfsstats, xs_qm_dqreclaims)/sizeof(uint32_t)) != XFSSTAT_START_XQMSTAT); >> >> and/or >> >> #define XFSSTAT_START_XQMSTAT (offsetof(struct __xfsstats, xs_qm_dqreclaims)/sizeof(uint32_t)) > > Yup, it worked, although, it still doesn't really close the possibility for > mistakes here, the first field may change for example and we may end up in the > same situation. > > I think only the 2nd option (defining START via offsetof), doesn't differ much > from the approach in the patch, and, using the 1st option (BUILD_BUG_ON), will > require an update to xqmstat_proc_show() if by any reason we change the quota > values, so, I thought the simpler version as the patch would suffice and be a > bit less confusing. But well, feel free to disagree, it's my opinion only and > not written on stone :P I'd argue that the first field of xqmstats changing not possible, because this is essentially a userspacce API. If we're going to change the format of xqmstats, we should only add to the end of it. The thing about using offsetof() is that we'll never have to update the START define again, even if we add more records to the larger stats structure. But we can see what others think as well. -Eric
