> Em Sun, 2 Aug 2009 12:10:04 +0200 > Hans Verkuil <hverkuil@xxxxxxxxx> escreveu: > >> Hi Mauro, >> >> Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb for the >> following: >> >> - v4l: introduce string control support. > > Hans, > > This looks very weird: > > +/* The following two functions really belong in v4l2-common, but that > causes > + a circular dependency between modules. We need to think about this, > but > + for now this will do. */ > + > +/* Return non-zero if this control is of type VALUE64. > + * > + * This information is used inside v4l2_compat_ioctl32. */ > +static int ctrl_is_value64(u32 id) > +{ > + return 0; > +} > + > +/* Return non-zero if this control is a pointer type. Currently only > + * type STRING is a pointer type. > + * > + * This information is used inside v4l2_compat_ioctl32. */ > +static int ctrl_is_pointer(u32 id) > +{ > + return 0; > +} > + > ... > + } else if (ctrl_is_value64(kctrl->id)) { > + if (copy_in_user(&kctrl->value64, &uctrl->value64, > + sizeof(uctrl->value64))) > + return -EFAULT; > ... > + if (ctrl_is_value64(kctrl->id)) { > + if (copy_in_user(&uctrl->value64, &kctrl->value64, > + sizeof(uctrl->value64))) > + return -EFAULT; > + } else if (!ctrl_is_pointer(kctrl->id) && > + copy_in_user(&uctrl->value, &kctrl->value, > + sizeof(uctrl->value))) { > > Why do you need two routines that will always return zero? Why to create a > code > that will never be used? v4l2-compat-ioctl32.c is already complex enough > without adding any bogus code. When the RDS encoder driver from Eduardo is added, then he will add the string controls to ctrl_is_pointer() since his driver is the first to actually implement string controls. There is indeed no value64 control yet, but I think it is wise to at least have the code in place to correctly handle such controls. I can remove that if you want. > > I noticed that you're also renaming the names of some fields there: > > - struct v4l2_ext_control __user *ucontrols; > - struct v4l2_ext_control __user *kcontrols = kp->controls; > + struct v4l2_ext_control __user *uctrl; > + struct v4l2_ext_control __user *kctrl = kp->controls; > > I dunno why you decided to do it, but having this together with the > changes at > v4l2-compat-ioctl32.c makes the code even harder to understand. Could you > please move the v4l2-compat-ioctl32 "cosmetic" changes like above into a > separate patch? OK. It was done to avoid creating lots of 'longer than 80 characters' warnings. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
