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;
        }
 }
 

Reply via email to