On Wed, Aug 02, 2017 at 04:30:09PM +0200, Jan Tulak wrote:
> On 29/07/2017 19:12, Luis R. Rodriguez wrote:
> > On Fri, Jul 28, 2017 at 04:45:58PM +0200, Jan Tulak wrote:
> > >
> > > On 27/07/2017 18:27, Luis R. Rodriguez wrote:
> > > > On Thu, Jul 20, 2017 at 11:29:26AM +0200, Jan Tulak wrote:
> > > > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > > > > index a69190b9..4b030101 100644
> > > > > --- a/mkfs/xfs_mkfs.c
> > > > > +++ b/mkfs/xfs_mkfs.c
> > > > > @@ -107,6 +107,11 @@ unsigned int sectorsize;
> > > > > * sets what is used with simple specifying the subopt (-d file).
> > > > > * A special SUBOPT_NEEDS_VAL can be used to require a user-given
> > > > > * value in any case.
> > > > > + *
> > > > > + * raw_input INTERNAL
> > > > > + * Filled raw string from the user, so we never lose that information e.g.
> > > > > + * to print it back in case of an issue.
> > > > > + *
> > > > > */
> > > > > struct opt_params {
> > > > > const char name;
> > > > > @@ -122,6 +127,7 @@ struct opt_params {
> > > > > long long minval;
> > > > > long long maxval;
> > > > > long long defaultval;
> > > > > + const char *raw_input;
> > > > > } subopt_params[MAX_SUBOPTS];
> > > > > };
> > > > > @@ -729,6 +735,18 @@ struct opt_params mopts = {
> > > > > */
> > > > > #define WHACK_SIZE (128 * 1024)
> > > > > +static inline void
> > > > > +set_conf_raw(struct opt_params *opt, int subopt, const char *value)
> > > > > +{
> > > > > + opt->subopt_params[subopt].raw_input = value;
> > > > > +}
> > > > There are no bounds check on the array here, I think set_conf_raw()
> > > > should return int and we would check the return value. It could
> > > > return -EINVAL if the subopt is invalid for instance.
> > > Good idea. The only issue is with the return code, that causes some issues
> > > when we are also returning values - I wanted the values to be turned into
> > > uint64. But do we need to return an error? I don't see what usecase there
> > > would be for it, other than detecting a bug. So an assert might be a better
> > > solution - then it can't happen that a wrong index is used and result not
> > > tested.
> > The setting of the value can be done by using an extra argument pointer. Then
> > if its set it be assigned. Otherwise it would be left alone. The return value
> > would return 0 on success, otherwise a standard return value indicating the
> > cause of the error.
> I strongly prefer to return the value, not an error code. We can do the
> other way around, put the error code into an argument to get roughly the
> same result, while constructions like set_conf_raw(FOO, BAR, baz *
> get_conf_raw(FOO, BAR)) will continue to work without the need for
> intermediate variables.
>
> The *_raw functions are used on few places only, so it would be only a small
> issue there, but for consistency, (get|set)_conf_val should have the same
> behavior and an intermediate variable for every use of those would be really
> annoying. So, how about this?
It would not be intermediate, the main error variable from the start of
each function could be used, as is typical in many properly written C
programs.
> static inline void
> set_conf_raw(struct opt_params *opt, int subopt, const char *value, int
> *err)
> {
> if (subopt < 0 || subopt >= MAX_SUBOPTS) {
> if (err != NULL) *err = EINVAL;
> return;
> }
> opt->subopt_params[subopt].raw_input = value;
> }
If you go with the strdup thing to avoid limiting the context of the use of
the pointer then you'll still have to return an error or abort, and I think
returning an error is best.
> > I don't think we need the too small or too big, a simple range issue should
> > suffice and we have -ERANGE.
> >
> At this moment, we are telling if it is too small or too big, but when there
> is no standard error code for that, ERANGE has to suffice.
Sure, my point was that we have special values for too big or too small, and
I consider that hacky, we could just *say* if it was too big or too small
but just use ERANGE as its standard and non-hacky.
Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
