Jeff Cody <jc...@redhat.com> writes:
> On Tue, Aug 09, 2016 at 02:20:09PM +0530, Prasanna Kumar Kalever wrote:
>> 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()
>>
>> However It is a good practice to use function like parse_uint_full()
>> over atoi() to keep port self defended
>>
>> Note: As now the port string to int conversion has its defence code set,
>> and also we understand that port argument is actually a string type,
>> in the follow up patch let's move port type from QEMU_OPT_NUMBER to
>> QEMU_OPT_STRING
>>
>> Signed-off-by: Prasanna Kumar Kalever <prasanna.kale...@redhat.com>
>> ---
>> v1: Initial patch
>> v2: Address comments on v1 given by Markus
>> ---
>> block/gluster.c | 11 ++++++++++-
>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/gluster.c b/block/gluster.c
>> index 01b479f..edde1ad 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;
>> + unsigned long long port;
>>
>> glfs = glfs_new(gconf->volume);
>> if (!glfs) {
>> @@ -330,10 +332,17 @@ 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, 10) < 0)
>> ||
>> + (port > 65535)) {
Two pairs of superfluous parenthesis. Better:
if (parse_uint_full(server->value->u.tcp.port, &port, 10) < 0 ||
port > 65535) {
or, if you prefer to break before the operator (I do):
if (parse_uint_full(server->value->u.tcp.port, &port, 10) < 0
|| port > 65535) {
or, if you prefer to keep side effects out of complex conditionals:
ret = parse_uint_full(server->value->u.tcp.port, &port, 10);
if (ret < 0 || port > 65535) {
Maintainer's choice. Perhaps Jeff can touch this up on commit.
>> + error_setg(errp, "'%s' is not a valid port number",
>> + server->value->u.tcp.port);
>> + errno = EINVAL;
>
> As long as we are range checking, we should probably kick back 0 as an
> invalid port number as well, right?
Port 0 is an oddity. On the one hand, bind() interprets it as "pick an
ephemeral port for me". That makes port 0 unusable with the classical
sockets interface. On the other hand, IP doesn't treat port 0
specially. You *can* send and receive port 0 packets with a raw socket
interface.
Anyway, I wouldn't bother rejecting port 0 here. Just let the system do
its "pick an ephemeral port" magic.
>> + goto out;
>> + }
>> 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) {
>> --
>> 2.7.4
>>
Preferrably with the parenthesis cleaned up:
Reviewed-by: Markus Armbruster <arm...@redhat.com>
