Patch Fix JSON API
Jeremy Allison
jra at samba.org
Tue Jul 24 22:46:31 UTC 2018
On Wed, Jul 25, 2018 at 08:57:31AM +1200, Andrew Bartlett wrote:
> On Tue, 2018-07-24 at 13:30 -0700, Jeremy Allison via samba-technical
> wrote:
> > On Wed, Jul 25, 2018 at 06:59:00AM +1200, Gary Lockyer wrote:
> > > Hopefully the final patch set, have added _WARN_UNUSED_RESULT_ to the
> > > function prototypes.
> > >
> > > Could I please get a review on this.
> >
> > Will do asap Gary - thanks !
> >
> > Jeremy.
>
> Everything except:
>
> [PATCH 3/7] audit logging tests: Only run JSON tests in ad dc
>
> Reviewed-by: Andrew Bartlett <abartlet at samba.org>
>
> (The 'none' environment is built in autobuild with the AD DC, and runs
> many other tests. We don't want to actually start the DC for unit
> tests).
>
> Jeremy,
>
> As you called for the change I'll look forward to your comments.
json_to_string() can return NULL, so in PATCH 1/7 we
need a == NULL check on the below (yeah I know the
json_is_invalid() check above might also catch it, but
still your test code does explicitly check json_to_string()
returning NULL so I feel justified :-).
* Write the json object to the audit logs as a formatted string
@@ -136,8 +115,16 @@ void audit_log_json(const char* prefix,
int debug_class,
int debug_level)
{
- TALLOC_CTX *ctx = talloc_new(NULL);
- char *s = json_to_string(ctx, message);
+ TALLOC_CTX *ctx = NULL;
+ char *s = NULL;
+
+ if (json_is_invalid(message)) {
+ DBG_ERR("Invalid JSON object, unable to log\n");
+ return;
+ }
+
+ ctx = talloc_new(NULL);
+ s = json_to_string(ctx, message);
DEBUGC(debug_class, debug_level, ("JSON %s: %s\n", prefix, s));
TALLOC_FREE(ctx);
Other than that, looks great ! RB+ from me. Thanks a *LOT*
for this cleanup (although I'd disagree with the error handling
merely being "aesthetically displeasing" comment, due to bitter
experience with similar ASN.1 code :-) :-).
I'll defer to Andrew on the PATCH 3/7 comment, but if you
can fix the minor nit I raised (or tell me why I'm wrong)
I'm good to go with this patchset !
Cheers,
Jeremy.
More information about the samba-technical
mailing list