On Mon, Apr 12, 2021 at 10:25:35AM -0600, Theo de Raadt wrote:
> > + line = XML_GetCurrentLineNumber(p);
>
> I think you can simplify your larger diff and avoid the temporary variable
> by doing:
>
> warnx("%s: XML error at line %llu: %s", s->local,
> (unsigned long long)XML_GetCurrentLineNumber(p),
> XML_ErrorString(XML_GetErrorCode(s->parser)));
>
> This is the explicit upcast model we use for a similar problem: time_t
>
> The benefit of doing the upcast directly in front of the function call
> rather than defining a temporary variable with implicit upcast, became
> clear during our time_t conversion process: we saw developers creating
> implicit upcast temporary variables, and then passing the temporary time
> variables (with a different type) to more than printf related code.
> That felt wrong. We wanted to aovid breaking the non-64bit time_t use
> cases of the code. Carrying time values in non-time_t types felt wrong, so
> we preferred keeping the values in the correct type until the last moment,
> hence (explicit cast)function() just for the "%llu".
In that case how about this?
--
:wq Claudio
Index: rrdp.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/rrdp.c,v
retrieving revision 1.3
diff -u -p -r1.3 rrdp.c
--- rrdp.c 7 Apr 2021 16:29:14 -0000 1.3
+++ rrdp.c 12 Apr 2021 16:50:05 -0000
@@ -303,18 +303,18 @@ rrdp_finished(struct rrdp *s)
}
if (s->res == HTTP_OK) {
+ XML_Parser p = s->parser;
+
/*
* Finalize parsing on success to be sure that
* all of the XML is correct. Needs to be done here
* since the call would most probably fail for non
* successful data fetches.
*/
- if (XML_Parse(s->parser, NULL, 0, 1) != XML_STATUS_OK) {
- warnx("%s: XML error at line %lu: %s",
- s->local,
- XML_GetCurrentLineNumber(s->parser),
- XML_ErrorString(XML_GetErrorCode(s->parser))
- );
+ if (XML_Parse(p, NULL, 0, 1) != XML_STATUS_OK) {
+ warnx("%s: XML error at line %llu: %s", s->local,
+ (unsigned long long)XML_GetCurrentLineNumber(p),
+ XML_ErrorString(XML_GetErrorCode(p)));
rrdp_failed(s);
return;
}
@@ -340,16 +340,14 @@ rrdp_finished(struct rrdp *s)
case SNAPSHOT:
warnx("%s: downloading snapshot",
s->local);
- s->sxml = new_snapshot_xml(s->parser,
- &s->current, s);
+ s->sxml = new_snapshot_xml(p, &s->current, s);
s->state = RRDP_STATE_REQ;
break;
case DELTA:
warnx("%s: downloading %lld deltas",
s->local, s->repository.serial -
s->current.serial);
- s->dxml = new_delta_xml(s->parser,
- &s->current, s);
+ s->dxml = new_delta_xml(p, &s->current, s);
s->state = RRDP_STATE_REQ;
break;
}
@@ -368,8 +366,7 @@ rrdp_finished(struct rrdp *s)
} else {
/* reset delta parser for next delta */
free_delta_xml(s->dxml);
- s->dxml = new_delta_xml(s->parser,
- &s->current, s);
+ s->dxml = new_delta_xml(p, &s->current, s);
s->state = RRDP_STATE_REQ;
}
break;
@@ -498,10 +495,10 @@ rrdp_data_handler(struct rrdp *s)
SHA256_Update(&s->ctx, buf, len);
if ((s->state & RRDP_STATE_PARSE_ERROR) == 0 &&
XML_Parse(p, buf, len, 0) != XML_STATUS_OK) {
- s->state |= RRDP_STATE_PARSE_ERROR;
- warnx("%s: parse error at line %lu: %s", s->local,
- XML_GetCurrentLineNumber(p),
+ warnx("%s: parse error at line %llu: %s", s->local,
+ (unsigned long long)XML_GetCurrentLineNumber(p),
XML_ErrorString(XML_GetErrorCode(p)));
+ s->state |= RRDP_STATE_PARSE_ERROR;
}
}
