On Fri, Feb 11, 2022 at 9:15 AM Richard Guy Briggs <[email protected]> wrote: > > On 2022-02-10 21:08, Paul Moore wrote: > > On Thu, Feb 10, 2022 at 6:46 PM Richard Guy Briggs <[email protected]> wrote: > > > On 2022-01-31 20:29, Paul Moore wrote: > > > > On Mon, Jan 31, 2022 at 6:29 PM Richard Guy Briggs <[email protected]> > > > > wrote: > > > > > On 2022-01-31 17:02, Paul Moore wrote: > > > > > > On Wed, Jan 26, 2022 at 8:52 AM Richard Guy Briggs > > > > > > <[email protected]> wrote: > > > > > > > On 2022-01-25 22:24, Richard Guy Briggs wrote: > > > > This isn't as complete of a response as I would like, but I wanted to > > get *something* back to you same-day since the delays are getting a > > bit long ... > > > > > > > > [WARNING: not compiled, tested, yadda yadda] > > > > > > > > > > > > void audit_log_time(struct audit_context ctx, struct audit_buffer > > > > > > **abp) > > > > > > { > > > > > > int i; > > > > > > int type = ctx->type; > > > > > > struct audit_buffer *ab = *abp; > > > > > > struct audit_ntp_val *ntp; > > > > > > const struct timespec64 *tk; > > > > > > const char *ntp_name[] = { > > > > > > "offset", > > > > > > "freq", > > > > > > "status", > > > > > > "tai", > > > > > > "tick", > > > > > > "adjust", > > > > > > }; > > > > > > > > > > > > do { > > > > > > if (type == AUDIT_TIME_ADJNTPVAL) { > > > > > > ntp = ctx->time.ntp_data.val; > > > > > > for (i = 0; i < AUDIT_NTP_NVALS; i++) { > > > > > > if (ntp[i].newval != ntp[i].oldval) { > > > > > > audit_log_format(ab, > > > > > > "op=%s old=%lli new=%lli", > > > > > > ntp_name[type], > > > > > > ntp[i].oldval, ntp[i].newval); > > > > > > } > > > > > > } > > > > > > } else { > > > > > > tk = &ctx->time.tk_injoffset; > > > > > > audit_log_format(ab, "sec=%lli nsec=%li", > > > > > > (long long)tk->tv_sec, tk->tv_nsec); > > > > > > } > > > > > > audit_log_end(ab); > > > > > > > > > > There is an audit_log_end() in the calling function, show_special(), > > > > > so > > > > > it should only be called here if there is another buffer allocated in > > > > > this function after it. audit_log_end() is protected should it be > > > > > called with no valid buffer so this wouldn't create a bug. > > > > > > > > As audit_log_end() can safely take a NULL audit_buffer I don't care if > > > > we > > > > send it back a valid buffer or a NULL. IMO it happens to be easier (and > > > > cleaner) to send back a NULL. > > > > > > None of the other callees in show_special() do that, so this would be > > > surprising behaviour that could cause a future bug ... > > > > To be both honest and frank: I disagree with your assessment. If you > > really want to be concerned about this, there are plenty of ways to > > mitigate the "risk" depending on your comfort level; comments and > > returning within the switch/case block are just some of the options. > > > > > > > > if (*abp) { > > > > > > *abp = NULL; > > > > > > type = (type == AUDIT_TIME_ADJNTPVAL ? > > > > > > AUDIT_TIME_INJOFFSET : AUDIT_TIME_ADJNTPVAL); > > > > > > > > > > This cannot be allocated if there are no more needed above ... > > > > > > > > My mistake, I was distracted a few times while typing up my reply and > > > > the > > > > code within; while I had that detail in mind when I started I lost it > > > > during one of the interruptions. As penance, I wrote up some slightly > > > > more > > > > proper code and at least made sure it complied, although I've not tried > > > > booting it ... > > > > > > Did you test the code I submitted? It compiles and works. I found this > > > code harder to follow. This was partly why I wanted to leave one of the > > > record types outside of show_special() but I did find a way to > > > accomodate both with a minimum of overhead. > > > > Once again, I disagree with your assessment of the code. I'm not sure > > how to put this politely, but I personally found your audit_log_time() > > implementation to be awkward; the AUDIT_TIME_INJOFFSET goto/jump to > > "tk" at the top of the function was not something I'm going to merge. > > You don't have to like my suggestion, but please send me a patch that > > has a somewhat reasonable code flow. I know you often want, or at > > least ask for explicit suggestions (hence my untested code example), > > but since you didn't like that let me just say this: when in doubt, > > limit your use of gotos/jumps to error handling unless there is > > something significantly unusual about the function. In my opinion > > there is nothing significantly unusual about the audit_log_time() > > function to require a jump as you've currently done. > > I hate gotos. I first learned to program on Waterloo Structured BASIC > 4.0 on a Commodore PET 2001 where the language structure provided the > tools to be able to avoid them. I've avoided them and reluctantly used > them at your urging, so now I'm a bit confused.
If you're confused follow the recommendations at the end of the paragraph directly above your reply. Or look at the sample code I proposed. You've been contributing long enough that this shouldn't be that hard. -- paul-moore.com -- Linux-audit mailing list [email protected] https://listman.redhat.com/mailman/listinfo/linux-audit
