[libvirt] [PATCHv3] maint: Kill usage of atoi()

[Laine Stump]

On 11/15/2013 12:22 PM, Peter Krempa wrote:
> Kill the use of atoi() and introduce syntax check to forbid 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).

Actually cyliders *was* set, but only in one of the lines that you
removed. The value was never again referenced though.


>
> examples/domsuspend/suspend.c will need a larger scale refactor as the
> whole example file is broken thus it will be exempted from the syntax
> check for now.
> ---
>
> Notes:
>     Version 3:
>     - add TODO to examples/domsuspend/suspend.c to remove the exemption of the syntax check
>     - exemption rule made specific for this file only
>
>  cfg.mk                             |  9 +++++++++
>  examples/domsuspend/suspend.c      |  1 +
>  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 +++++++++++++----
>  6 files changed, 39 insertions(+), 12 deletions(-)
>
> diff --git a/cfg.mk b/cfg.mk
> index befd231..838934a 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/domsuspend/suspend.c$$
> diff --git a/examples/domsuspend/suspend.c b/examples/domsuspend/suspend.c
> index 6a0f5c9..5f7a890 100644
> --- a/examples/domsuspend/suspend.c
> +++ b/examples/domsuspend/suspend.c
> @@ -104,6 +104,7 @@ int main(int argc, char **argv) {
>      }
>
>      if (argc > 1) {
> +        /* TODO: remove exclusion of this file from syntax check denying atoi */
>          id = atoi(argv[1]);
>      }
>      if (id == 0) {
> 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"));


Are we certain enough that this error will never happen that we don't
mind not including in the error log the contents of the strings that we
failed to parse? (and maybe also having a separate message for each
rather than a single catchall?)


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


If either of these errors were ever encountered, it might make debugging
simpler if the contents of buffer were included in the error log.


>      }
>
>      VIR_FREE(buffer);