On Sat, Jul 28, 2012 at 4:43 AM, Bharata B Rao <bhar...@linux.vnet.ibm.com> wrote: > On Fri, Jul 27, 2012 at 06:44:04PM +0000, Blue Swirl wrote: >> > +struct GlusterOpts { >> >> static > > Sure. > >> >> > + bool optional; >> > + char defval[10]; >> >> const char *defval? > > Sure I could. > >> >> > + char *value; >> > +} GlusterOpts[] = { >> > + {false, "", NULL }, >> > + {false, "", NULL }, >> > + {true, "0", NULL }, >> > + {true, "socket", NULL }, >> > + {false, "", NULL }, >> > + {false, "", NULL }, >> > +}; >> > + >> > + if (i == GOPT_LAST-1 && strlen(q)) { >> >> Spaces around '-'. > > checkpatch.pl doesn't enforce this, but I can change. > >> > + >> > + port = strtoul(GlusterOpts[GOPT_PORT].value, NULL, 0); >> > + if (port < 0) { >> >> port > 65535 could be bad too. > > Actually I am just checking if strtoul gave me a valid integer only > and depending on gluster to flag an error for invalid port number. > But I guess no harm in checking for valid port range here. Is there > a #define equivalent for 65535 ?
I don't think there is. It may also be possible to omit the check if the connection function checks it and fails. Accidental modular arithmetic for the port numbers (65537 % 65536 == 1) would not be so nice if there are no checks. > >> >> > + >> > + fd = glfs_creat(glfs, GlusterOpts[GOPT_IMAGE].value, >> > + O_WRONLY|O_CREAT|O_TRUNC|O_BINARY, S_IRUSR|S_IWUSR); >> >> Spaces around '|'. > > Again, checkpatch.pl doesn't enforce this, but I can change. > > Thanks for take time to review. > > Regards, > Bharata. >