Em Thu, 6 Aug 2009 13:22:18 +0200 "Hans Verkuil" <hverkuil@xxxxxxxxx> escreveu: > > > 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. It is better to add this together with his patch, since, currently, the code makes no sense to anyone that are analyzing it. Btw, as this kind of controls will be used firstly on his driver, please add his patches on your tree. This will make easier to analyze the required core changes. > 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. I was suspecting so. Yet, especially on more complex code, the better is to do those changes on a separate changeset, to avoid polluting the changeset. > > Regards, > > Hans > Cheers, Mauro -- 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
