Re: [PULL 09/21] cutils: Adjust signature of parse_uint[_full]

From: Eric Blake <eblake@redhat.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: qemu-devel@nongnu.org, "Hanna Czenczek" <hreitz@redhat.com>,
	"Gerd Hoffmann" <kraxel@redhat.com>,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>,
	"Kevin Wolf" <kwolf@redhat.com>, "Peter Lieven" <pl@kamp.de>,
	"Michael Roth" <michael.roth@amd.com>,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	"open list:GLUSTER" <qemu-block@nongnu.org>,
	"open list:GLUSTER" <integration@gluster.org>
Subject: Re: [PULL 09/21] cutils: Adjust signature of parse_uint[_full]
Date: Fri, 2 Jun 2023 07:22:27 -0500	[thread overview]
Message-ID: <l32irl7qbhs2omzbxakm3n27vpjpmbvuscb4azzhgv7jx26nnz@dfjpfem52hz2> (raw)
In-Reply-To: <874jnqz8vt.fsf@pond.sub.org>

On Fri, Jun 02, 2023 at 08:16:38AM +0200, Markus Armbruster wrote:
> Sorry for being late to the party...
> 
> Eric Blake <eblake@redhat.com> writes:
> 
> > It's already confusing that we have two very similar functions for
> > wrapping the parse of a 64-bit unsigned value, differing mainly on
> > whether they permit leading '-'.  Adjust the signature of parse_uint()
> > and parse_uint_full() to be like all of qemu_strto*(): put the result
> > parameter last, use the same types (uint64_t and unsigned long long
> > have the same width, but are not always the same type), and mark
> > endptr const (this latter change only affects the rare caller of
> > parse_uint).  Adjust all callers in the tree.
> >
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
> > Message-Id: <20230522190441.64278-8-eblake@redhat.com>
> 
> [...]
> 
> > diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> > index 587f31baf6b..8812d23677a 100644
> > --- a/qapi/opts-visitor.c
> > +++ b/qapi/opts-visitor.c
> > @@ -454,8 +454,8 @@ opts_type_uint64(Visitor *v, const char *name, uint64_t *obj, Error **errp)
> >      OptsVisitor *ov = to_ov(v);
> >      const QemuOpt *opt;
> >      const char *str;
> > -    unsigned long long val;
> > -    char *endptr;
> > +    uint64_t val;
> 
> val changes from unsigned long long, which is at least 64 bits, to
> uint64_t, which is exactly 64 bits.

Except that we have:

util/cutils.c:    QEMU_BUILD_BUG_ON(sizeof(uint64_t) != sizeof(unsigned long long));

proving that all of our target platforms have unsigned long long at
the same size (but not necessarily same rank) as uint64_t...

> 
> > +    const char *endptr;
> >
> >      if (ov->list_mode == LM_UNSIGNED_INTERVAL) {
> >          *obj = ov->range_next.u;
> > @@ -471,17 +471,17 @@ opts_type_uint64(Visitor *v, const char *name, uint64_t *obj, Error **errp)
> >      /* we've gotten past lookup_scalar() */
> >      assert(ov->list_mode == LM_NONE || ov->list_mode == LM_IN_PROGRESS);
> >
> > -    if (parse_uint(str, &val, &endptr, 0) == 0 && val <= UINT64_MAX) {
> > +    if (parse_uint(str, &endptr, 0, &val) == 0 && val <= UINT64_MAX) {
> 
> val <= UINT64_MAX is now useless, isn't it?

...so we would have failed to build if the condition could have ever
been true before this patch.  The dead condition is thus pre-existing,
but I will touch it up, since I have to respin to work around a mingw
bug anyways.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org