| Message ID | 1452242274-8345-2-git-send-email-caoj.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On 01/08/2016 01:37 AM, Cao jin wrote: > strtol() don`t guarantee errno to be ERANGE on overflow. I stand slightly corrected: C99 requires ERANGE on overflow, but not EINVAL; it is POSIX that adds EINVAL, but does not properly require it. At any rate, my main point was that errno is not always properly set by all strtol implementations, and furthermore that you can't rely on it being set to a sane value if you didn't pre-set it to 0. > This wrapper returns either -EINVAL or the errno set by strtol() > function (e.g -ERANGE). The subject line doesn't start with a topic. Maybe a better commit message would be: xen: Use qemu_strtoul instead of strtol No need to roll our own (with slightly incorrect handling of errno), when we can use the common version. > > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> > --- > hw/xen/xen-host-pci-device.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > buf[rc] = 0; > - value = strtol(buf, &endptr, base); > - if (endptr == buf || *endptr != '\n') { > - rc = -1; > - } else if ((value == LONG_MIN || value == LONG_MAX) && errno == ERANGE) { > - rc = -errno; > - } else { > - rc = 0; > + rc = qemu_strtoul(buf, &endptr, base, &value); Why did you switch from strtol() to qemu_strtoul()? Was signed parsing incorrect, and unsigned parsing a bug fix? If so, please mention it in the commit message as intentional. Otherwise, use qemu_strtol() (and adjust the commit message accordingly).
On 01/09/2016 01:35 AM, Eric Blake wrote: > On 01/08/2016 01:37 AM, Cao jin wrote: >> strtol() don`t guarantee errno to be ERANGE on overflow. > > I stand slightly corrected: C99 requires ERANGE on overflow, but not > EINVAL; it is POSIX that adds EINVAL, but does not properly require it. > At any rate, my main point was that errno is not always properly set by > all strtol implementations, and furthermore that you can't rely on it > being set to a sane value if you didn't pre-set it to 0. > Got you:) >> This wrapper returns either -EINVAL or the errno set by strtol() >> function (e.g -ERANGE). > > The subject line doesn't start with a topic. Maybe a better commit > message would be: > > xen: Use qemu_strtoul instead of strtol > > No need to roll our own (with slightly incorrect handling of errno), > when we can use the common version. > ok, looks good. >> >> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> >> --- >> hw/xen/xen-host-pci-device.c | 11 +++-------- >> 1 file changed, 3 insertions(+), 8 deletions(-) > >> buf[rc] = 0; >> - value = strtol(buf, &endptr, base); >> - if (endptr == buf || *endptr != '\n') { >> - rc = -1; >> - } else if ((value == LONG_MIN || value == LONG_MAX) && errno == ERANGE) { >> - rc = -errno; >> - } else { >> - rc = 0; >> + rc = qemu_strtoul(buf, &endptr, base, &value); > > Why did you switch from strtol() to qemu_strtoul()? Was signed parsing > incorrect, and unsigned parsing a bug fix? If so, please mention it in > the commit message as intentional. Otherwise, use qemu_strtol() (and > adjust the commit message accordingly). > Yes, I should mention it, sorry for forgetting to explain it. Because what it want to read from host are values in pci device config space, which are non-negative, like "vendor ID", "device ID", "class code", etc. so unsigned parsing seems more suitable in this case. we can tell it also from the local variable "unsigned long value;"
diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c index 7d8a023..351b61a 100644 --- a/hw/xen/xen-host-pci-device.c +++ b/hw/xen/xen-host-pci-device.c @@ -148,7 +148,7 @@ static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name, char buf[XEN_HOST_PCI_GET_VALUE_BUFFER_SIZE]; int fd, rc; unsigned long value; - char *endptr; + const char *endptr; rc = xen_host_pci_sysfs_path(d, name, path, sizeof (path)); if (rc) { @@ -167,13 +167,8 @@ static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name, } } while (rc < 0); buf[rc] = 0; - value = strtol(buf, &endptr, base); - if (endptr == buf || *endptr != '\n') { - rc = -1; - } else if ((value == LONG_MIN || value == LONG_MAX) && errno == ERANGE) { - rc = -errno; - } else { - rc = 0; + rc = qemu_strtoul(buf, &endptr, base, &value); + if (!rc) { *pvalue = value; } out:
strtol() don`t guarantee errno to be ERANGE on overflow. This wrapper returns either -EINVAL or the errno set by strtol() function (e.g -ERANGE). Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> --- hw/xen/xen-host-pci-device.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)