Em Wed, 20 Sep 2017 14:11:39 -0600
Shuah Khan <shuah@xxxxxxxxxx> escreveu:
> On 09/19/2017 07:42 AM, Mauro Carvalho Chehab wrote:
> > Use a switch() on this function, just like on other ioctl
> > handlers and handle parameters inside each part of the
> > switch.
> >
> > That makes it easier to integrate with the already existing
> > ioctl handler function.
> >
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxxx>
>
> The change looks good. Couple of comments below:
>
> > ---
> > drivers/media/dvb-core/dvb_frontend.c | 83 +++++++++++++++++++++--------------
> > 1 file changed, 51 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
> > index 8abe4f541a36..725cb1c8a088 100644
> > --- a/drivers/media/dvb-core/dvb_frontend.c
> > +++ b/drivers/media/dvb-core/dvb_frontend.c
> > @@ -1971,21 +1971,25 @@ static int dvb_frontend_ioctl_properties(struct file *file,
> > struct dvb_frontend *fe = dvbdev->priv;
> > struct dvb_frontend_private *fepriv = fe->frontend_priv;
> > struct dtv_frontend_properties *c = &fe->dtv_property_cache;
> > - int err = 0;
> > -
> > - struct dtv_properties *tvps = parg;
> > - struct dtv_property *tvp = NULL;
> > - int i;
> > + int err, i;
> >
> > dev_dbg(fe->dvb->device, "%s:\n", __func__);
> >
> > - if (cmd == FE_SET_PROPERTY) {
> > - dev_dbg(fe->dvb->device, "%s: properties.num = %d\n", __func__, tvps->num);
> > - dev_dbg(fe->dvb->device, "%s: properties.props = %p\n", __func__, tvps->props);
> > + switch(cmd) {
> > + case FE_SET_PROPERTY: {
> > + struct dtv_properties *tvps = parg;
> > + struct dtv_property *tvp = NULL;
> >
> > - /* Put an arbitrary limit on the number of messages that can
> > - * be sent at once */
> > - if ((tvps->num == 0) || (tvps->num > DTV_IOCTL_MAX_MSGS))
> > + dev_dbg(fe->dvb->device, "%s: properties.num = %d\n",
> > + __func__, tvps->num);
> > + dev_dbg(fe->dvb->device, "%s: properties.props = %p\n",
> > + __func__, tvps->props);
> > +
> > + /*
> > + * Put an arbitrary limit on the number of messages that can
> > + * be sent at once
> > + */
> > + if (!tvps->num || (tvps->num > DTV_IOCTL_MAX_MSGS))
> > return -EINVAL;
> >
> > tvp = memdup_user(tvps->props, tvps->num * sizeof(*tvp));
> > @@ -1994,23 +1998,34 @@ static int dvb_frontend_ioctl_properties(struct file *file,
> >
> > for (i = 0; i < tvps->num; i++) {
> > err = dtv_property_process_set(fe, tvp + i, file);
> > - if (err < 0)
> > - goto out;
> > + if (err < 0) {
> > + kfree(tvp);
> > + return err;
> > + }
> > (tvp + i)->result = err;
> > }
> >
> > if (c->state == DTV_TUNE)
> > dev_dbg(fe->dvb->device, "%s: Property cache is full, tuning\n", __func__);
> >
> > - } else if (cmd == FE_GET_PROPERTY) {
> > + kfree(tvp);
> > + break;
> > + }
> > + case FE_GET_PROPERTY: {
> > + struct dtv_properties *tvps = parg;
> > + struct dtv_property *tvp = NULL;
> > struct dtv_frontend_properties getp = fe->dtv_property_cache;
> >
> > - dev_dbg(fe->dvb->device, "%s: properties.num = %d\n", __func__, tvps->num);
> > - dev_dbg(fe->dvb->device, "%s: properties.props = %p\n", __func__, tvps->props);
> > + dev_dbg(fe->dvb->device, "%s: properties.num = %d\n",
> > + __func__, tvps->num);
> > + dev_dbg(fe->dvb->device, "%s: properties.props = %p\n",
> > + __func__, tvps->props);
> >
> > - /* Put an arbitrary limit on the number of messages that can
> > - * be sent at once */
> > - if ((tvps->num == 0) || (tvps->num > DTV_IOCTL_MAX_MSGS))
> > + /*
> > + * Put an arbitrary limit on the number of messages that can
> > + * be sent at once
> > + */
> > + if (!tvps->num || (tvps->num > DTV_IOCTL_MAX_MSGS))
> > return -EINVAL;
> >
> > tvp = memdup_user(tvps->props, tvps->num * sizeof(*tvp));
> > @@ -2025,28 +2040,32 @@ static int dvb_frontend_ioctl_properties(struct file *file,
> > */
> > if (fepriv->state != FESTATE_IDLE) {
> > err = dtv_get_frontend(fe, &getp, NULL);
> > - if (err < 0)
> > - goto out;
> > + if (err < 0) {
> > + kfree(tvp);
> > + return err;
> > + }
>
> Could avoid duplicate code keeping out logic perhaps? Is there a reason
> for removing this?
Yes. See the next patch :-)
Basically, the next patch remove dvb_frontend_ioctl_properties(), merging
it with another ioctl handler. On such handler, the error handling path
is different for each ioctl.
We might still use gotos there, but that would be messy.
>
> > }
> > for (i = 0; i < tvps->num; i++) {
> > err = dtv_property_process_get(fe, &getp, tvp + i, file);
> > - if (err < 0)
> > - goto out;
> > + if (err < 0) {
> > + kfree(tvp);
> > + return err;
> > + }
> > (tvp + i)->result = err;
> > }
> >
> > if (copy_to_user((void __user *)tvps->props, tvp,
> > tvps->num * sizeof(struct dtv_property))) {
> > - err = -EFAULT;
> > - goto out;
> > + kfree(tvp);
> > + return -EFAULT;
> > }
>
> Could avoid duplicate code keeping out logic perhaps? Is there a reason
> for removing this?
Same as above.
>
> > -
> > - } else
> > - err = -EOPNOTSUPP;
> > -
> > -out:
> > - kfree(tvp);
> > - return err;
> > + kfree(tvp);
> > + break;
> > + }
> > + default:
> > + return -ENOTSUPP;
> > + } /* switch */
> > + return 0;
> > }
> >
> > static int dtv_set_frontend(struct dvb_frontend *fe)
> >
>
> Reviewed-by: Shuah Khan <shuahkh@xxxxxxxxxxxxxxx>
>
> thanks,
> -- Shuah
Thanks,
Mauro
