Cc: Eric, because there's a relation to QAPI even though the patch
doesn't touch the schema.
Prasanna Kumar Kalever <prasanna.kale...@redhat.com> writes:
> using atoi() for converting string to int may be error prone in case if
> string supplied in the argument is not a fold of numerical number,
>
> This is not a bug because in the existing code,
>
> static QemuOptsList runtime_tcp_opts = {
> .name = "gluster_tcp",
> .head = QTAILQ_HEAD_INITIALIZER(runtime_tcp_opts.head),
> .desc = {
> ...
> {
> .name = GLUSTER_OPT_PORT,
> .type = QEMU_OPT_NUMBER,
> .help = "port number ...",
> },
> ...
> };
>
> port type is QEMU_OPT_NUMBER, before we actually reaches atoi() port is
> already
> defended by parse_option_number()
Should we use QEMU_OPT_STRING here, for the same reasons we use 'str' in
the QAPI schema?
> However It is a good practice to use function like parse_uint_full()
> over atoi() to keep port self defended
>
> Signed-off-by: Prasanna Kumar Kalever <prasanna.kale...@redhat.com>
> ---
> block/gluster.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/block/gluster.c b/block/gluster.c
> index 01b479f..e2aa0f3 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -14,6 +14,7 @@
> #include "qapi/qmp/qerror.h"
> #include "qemu/uri.h"
> #include "qemu/error-report.h"
> +#include "qemu/cutils.h"
>
> #define GLUSTER_OPT_FILENAME "filename"
> #define GLUSTER_OPT_VOLUME "volume"
> @@ -318,6 +319,7 @@ static struct glfs
> *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
> int ret;
> int old_errno;
> GlusterServerList *server;
> + long long unsigned port;
unsigned long long, please.
>
> glfs = glfs_new(gconf->volume);
> if (!glfs) {
> @@ -330,10 +332,16 @@ static struct glfs
> *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
>
> GlusterTransport_lookup[server->value->type],
> server->value->u.q_unix.path, 0);
> } else {
> + if (parse_uint_full(server->value->u.tcp.port, &port, 0) < 0) {
Should we require decimal by passing 10 rather than 0? Port numbers are
commonly decimal. For what it's worth, JSON numbers are always decimal.
> + error_setg(errp, "can't convert port to a number: %s",
> + server->value->u.tcp.port);
Suggest "'%s' is not a valid port number".
> + errno = EINVAL;
> + goto out;
> + }
Missing: a range check. No need to get fancy, something like this might
do:
if (parse_uint_full(server->value->u.tcp.port, &port, 0) < 0
|| port > 65535) {
> ret = glfs_set_volfile_server(glfs,
>
> GlusterTransport_lookup[server->value->type],
> server->value->u.tcp.host,
> - atoi(server->value->u.tcp.port));
> + (int)port);
> }
>
> if (ret < 0) {
