[libvirt] [PATCHv2] maint: Kill usage of atoi()
Kill the use of atoi() and introduce syntax check to forbit it and it's friends (atol, atoll, atof, atoq). Also fix a typo in variable name holding the cylinders count of a disk pool (apparently unused). --- cfg.mk | 9 +++++++++ src/conf/storage_conf.h | 2 +- src/qemu/qemu_command.c | 8 ++++++-- src/storage/storage_backend_disk.c | 14 +++++++++----- src/xen/xend_internal.c | 17 +++++++++++++---- 5 files changed, 38 insertions(+), 12 deletions(-) diff --git a/cfg.mk b/cfg.mk index befd231..901e307 100644 --- a/cfg.mk +++ b/cfg.mk @@ -869,6 +869,12 @@ sc_prohibit_getenv: halt='Use virGetEnv{Allow,Block}SUID instead of getenv' \ $(_sc_search_regexp) +sc_prohibit_atoi: + @prohibit='\bato(i|f|l|ll|q) *\(' \ + halt='Use virStrToLong* instead of atoi, atol, atof, atoq, atoll' \ + $(_sc_search_regexp) + + # We don't use this feature of maint.mk. prev_version_file = /dev/null @@ -1040,3 +1046,6 @@ exclude_file_name_regexp--sc_prohibit_int_ijk = \ exclude_file_name_regexp--sc_prohibit_getenv = \ ^tests/.*\.[ch]$$ + +exclude_file_name_regexp--sc_prohibit_atoi= \ + ^examples/.*\.[ch]$$ diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index f062bd8..05c291e 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -227,7 +227,7 @@ struct _virStoragePoolSourceDevice { * the geometry data is needed */ struct _geometry { - int cyliders; + int cylinders; int heads; int sectors; } geometry; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 966aa0d..25beedf 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3663,8 +3663,12 @@ qemuBuildDriveURIString(virConnectPtr conn, if (disk->src && virAsprintf(&volimg, "/%s", disk->src) < 0) goto cleanup; - if (disk->hosts->port) { - port = atoi(disk->hosts->port); + if (disk->hosts->port && + virStrToLong_i(disk->hosts->port, NULL, 0, &port) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to parse network port '%s'"), + disk->hosts->port); + goto cleanup; } if (disk->hosts->socket && diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 4e53ec5..a7a7d0e 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -280,12 +280,16 @@ virStorageBackendDiskMakePoolGeometry(virStoragePoolObjPtr pool, char **const groups, void *data ATTRIBUTE_UNUSED) { + virStoragePoolSourceDevicePtr device = &(pool->def->source.devices[0]); + if (virStrToLong_i(groups[0], NULL, 0, &device->geometry.cylinders) < 0 || + virStrToLong_i(groups[1], NULL, 0, &device->geometry.heads) < 0 || + virStrToLong_i(groups[2], NULL, 0, &device->geometry.sectors) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to create disk pool geometry")); + return -1; + } - pool->def->source.devices[0].geometry.cyliders = atoi(groups[0]); - pool->def->source.devices[0].geometry.heads = atoi(groups[1]); - pool->def->source.devices[0].geometry.sectors = atoi(groups[2]); - - return 0; + return 0; } static int diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index dc57350..771288c 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -290,10 +290,19 @@ xend_req(int fd, char **content) if (STREQ(buffer, "\r\n")) break; - if (istartswith(buffer, "Content-Length: ")) - content_length = atoi(buffer + 16); - else if (istartswith(buffer, "HTTP/1.1 ")) - retcode = atoi(buffer + 9); + if (istartswith(buffer, "Content-Length: ")) { + if (virStrToLong_i(buffer + 16, NULL, 10, &content_length) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to parse Xend response content length")); + return -1; + } + } else if (istartswith(buffer, "HTTP/1.1 ")) { + if (virStrToLong_i(buffer + 9, NULL, 10, &retcode) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to parse Xend response return code")); + return -1; + } + } } VIR_FREE(buffer); -- 1.8.4.3
On 11/14/2013 09:48 AM, Peter Krempa wrote:
Kill the use of atoi() and introduce syntax check to forbit it and it's
s/forbit/forbid/
friends (atol, atoll, atof, atoq).
Also fix a typo in variable name holding the cylinders count of a disk pool (apparently unused). --- cfg.mk | 9 +++++++++ src/conf/storage_conf.h | 2 +- src/qemu/qemu_command.c | 8 ++++++-- src/storage/storage_backend_disk.c | 14 +++++++++----- src/xen/xend_internal.c | 17 +++++++++++++---- 5 files changed, 38 insertions(+), 12 deletions(-)
diff --git a/cfg.mk b/cfg.mk
@@ -1040,3 +1046,6 @@ exclude_file_name_regexp--sc_prohibit_int_ijk = \
exclude_file_name_regexp--sc_prohibit_getenv = \ ^tests/.*\.[ch]$$ + +exclude_file_name_regexp--sc_prohibit_atoi= \ + ^examples/.*\.[ch]$$
Drop this hunk. None of our examples/* use atoi to begin with, so there's no reason to exclude them from the syntax check. ACK with those fixes. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/14/13 17:53, Eric Blake wrote:
On 11/14/2013 09:48 AM, Peter Krempa wrote:
Kill the use of atoi() and introduce syntax check to forbit it and it's
s/forbit/forbid/
friends (atol, atoll, atof, atoq).
Also fix a typo in variable name holding the cylinders count of a disk pool (apparently unused). --- cfg.mk | 9 +++++++++ src/conf/storage_conf.h | 2 +- src/qemu/qemu_command.c | 8 ++++++-- src/storage/storage_backend_disk.c | 14 +++++++++----- src/xen/xend_internal.c | 17 +++++++++++++---- 5 files changed, 38 insertions(+), 12 deletions(-)
diff --git a/cfg.mk b/cfg.mk
@@ -1040,3 +1046,6 @@ exclude_file_name_regexp--sc_prohibit_int_ijk = \
exclude_file_name_regexp--sc_prohibit_getenv = \ ^tests/.*\.[ch]$$ + +exclude_file_name_regexp--sc_prohibit_atoi= \ + ^examples/.*\.[ch]$$
Drop this hunk. None of our examples/* use atoi to begin with, so there's no reason to exclude them from the syntax check.
ACK with those fixes.
This isn't true unfortunately: ~/libvirt $ git grep atoi examples/ examples/domsuspend/suspend.c: id = atoi(argv[1]); and I wanted to avoid changing that file. But if you insist I can tune that one too. Peter
On 11/14/2013 02:22 PM, Peter Krempa wrote:
+ +exclude_file_name_regexp--sc_prohibit_atoi= \ + ^examples/.*\.[ch]$$
Drop this hunk. None of our examples/* use atoi to begin with, so there's no reason to exclude them from the syntax check.
ACK with those fixes.
This isn't true unfortunately:
Huh, wonder what grep I did that missed that fact?
~/libvirt $ git grep atoi examples/ examples/domsuspend/suspend.c: id = atoi(argv[1]);
and I wanted to avoid changing that file. But if you insist I can tune that one too.
Yes, it's worth fixing, because our examples should never teach people to use bad coding practices. atoi() is broken by design, and strtol(), while more awkward to use, is just as portably present as atoi(). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/14/13 22:53, Eric Blake wrote:
On 11/14/2013 02:22 PM, Peter Krempa wrote:
+ +exclude_file_name_regexp--sc_prohibit_atoi= \ + ^examples/.*\.[ch]$$
Drop this hunk. None of our examples/* use atoi to begin with, so there's no reason to exclude them from the syntax check.
ACK with those fixes.
This isn't true unfortunately:
Huh, wonder what grep I did that missed that fact?
~/libvirt $ git grep atoi examples/ examples/domsuspend/suspend.c: id = atoi(argv[1]);
and I wanted to avoid changing that file. But if you insist I can tune that one too.
Yes, it's worth fixing, because our examples should never teach people to use bad coding practices. atoi() is broken by design, and strtol(), while more awkward to use, is just as portably present as atoi().
Well, the file is a bad example by itself. The first issue would be that it doesn't work. It tries to suspend(pause) a VM using the RO connection. That file should be refactored as a whole piece instead of trying to polish parts that won't even work. Peter
On 11/15/2013 02:47 AM, Peter Krempa wrote:
On 11/14/13 22:53, Eric Blake wrote:
On 11/14/2013 02:22 PM, Peter Krempa wrote:
+ +exclude_file_name_regexp--sc_prohibit_atoi= \ + ^examples/.*\.[ch]$$
Drop this hunk. None of our examples/* use atoi to begin with, so there's no reason to exclude them from the syntax check.
Well, the file is a bad example by itself. The first issue would be that it doesn't work. It tries to suspend(pause) a VM using the RO connection.
Hmm. Then let's write that file name exactly (instead of exempting ALL of examples/*) along with a todo reminder to remove the exemption when the example is fixed.
That file should be refactored as a whole piece instead of trying to polish parts that won't even work.
Eww, you're right. All right, I'm okay saving that for another day, so we don't hold up getting the syntax check in. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Peter Krempa