Andreas Oberritter wrote:
Hi Marcel,
hi andreas,
On Fri, 2006-03-31 at 21:26 +0200, Marcel Siegert wrote:
added new FE_[GET|SET]_FRONTEND_NEW ioctls for new applications.
as time goes by the new stuff will become current or old, but the name
won't be able to reflect that. What will be the name in case another new
ioctl will be introduced in the future? FE_SET_FRONTEND_EVEN_NEWER?
I think this is much worse than FE_SET_FRONTEND2 - of course meaning the
second version of the ioctl, not the new ioctl for DVB-S2. ;-)
it was just chosen by me to reflect the difference between this patch version
and the already discussed. i do agree to select e.g. FE_SET_FRONTEND2 as ioctl name,
but if you look at its parameters, you may come to the conclusion that we will
not have to have a FE_SET_FRONTEND3 or something similar added. the union is now
passed as a pointer so if next standard needs to be added just the union is extended.
no need to change the ioctl itself. so i thought FE_SET_FRONTEND_NEW could be more
signalling to the user that THIS is the actual ioctl used. FE_SET_FRONTEND2 or something
similar, could be interpreted as an alternative. but i do see your arguments.
Sending your patch inline would make it easier to comment on it.
normally my kmail does insert patches inline. maybe it hasn't been inlined because
the message was signed and the pgp key has been attached. i'll check that.
+ int act_standard_set;
+ fe_type_t act_standard;
What does act mean here? If it's supposed to mean actual, then I think
its a wrong translation from German, and should mean "current".
ok, will change to current
What's the difference between those two variables?
the difference is as follows:
act_standard_set has got an value != 0 if FE_SET_STANDARD has been called at least once
and the wanted standard has been accepted by the frontend driver.
FE_SET_FRONTEND_NEW is checking this flag and if not set, returns a -EINVAL.
act_standard is just the val of FE_SET_STANDARD if it has been successfull.
atm it is just used to double check that the frontend mode is the same like
the wanted mode set by FE_SET_STANDARD, it also may be used for ommitting the
size param from the user passed struct. (pls. read on)
we are not able to use the act_standard as the set flag itself because FE_QPSK was enumerated as 0.
+ err = fe->ops->get_frontend_new(fe, (struct
dvb_frontend_parameters_new*) parg);
In case parg is of type void *, then the cast is unneeded. Casts should
always be avoided from and to void *, because they possibly modify
annotations used by the "sparse" source code checker.
int (*get_frontend_new)(struct dvb_frontend* fe, struct dvb_frontend_parameters_new* params);
imho as the ioctl is declared having dvb_frontend_parameters_new* as a param the cast must be done.
if not, the "old" get_frontend implementation is also doing a unneeded casting for ages now :)
case FE_GET_FRONTEND:
if (fe->ops->get_frontend) {
memcpy (parg, &fepriv->parameters, sizeof (struct dvb_frontend_parameters));
err = fe->ops->get_frontend(fe, (struct dvb_frontend_parameters*) parg);
}
break;
just wondering if the size member ( or a fe_type_t member ) or could be automatically assigned after FE_SET_STANDARD
+ /* be sure struct union pointer is set - even if it is
NULL */
Why not just return -EINVAL and do nothing instead?
+ fetunesettings.parameters_new.u = (struct
frontend_parameters_union *) kmalloc(castedparg->size, GFP_KERNEL);
What happens, if a malicious TV consumer sets size to a value which
kmalloc can't handle?
that's also a reason why act_standard was added to the private struct. i want to prevent
setting size parameters within a user application passed struct. depending on the
act_standard the size could be selected automatically, but i haven't been exactly sure,
if that may break back compat in future. i'll have to think on it more and do a final
design.
the whole patch should just create a common ground to discuss on further development of
dvb-s2 support. if we would support dvb-s2 as described in the old patches, within dvb-core/dvb_frontend.h/c
we would run into a chaos of workaround code.
Regards,
Andreas
thanks for commenting like always :)
marcel
_______________________________________________
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
