Re: Bug in S2 API...

2009-09-23 Thread hermann pitton 

Am Dienstag, den 22.09.2009, 07:54 +0200 schrieb Markus Rechberger:
 On Tue, Sep 22, 2009 at 7:08 AM, hermann pitton hermann-pit...@arcor.de 
 wrote:
 
  All fine.
 
  But you do cut off parts of my messages.
 
  I would like to ask you to exercise a breakage, not only for stuff six
  years back, but on recent S2 API.
 
 
 at this time there are definitely no DVB applications available which
 check the flags of the encoded IOCTL values for the S2-API
 definitions, there are just very few applications available which use
 this API at all.
 
 if (IOCTL_WRITE(cmd))
   copy data into buffer for ioctl
 
 -- from the driver side --
 if (IOCTL_WRITE(cmd))
   copy data from the other side
 process_ioctl()
 
 At least the API specification should be valid however the rest is
 implemented is another story. It works as it is but it's still wrong.
 Since the S2-API is rather new it might be better to fix it up. It
 would be a little bit weird too that coding style has a higher
 priority in Linux than correct code.
 
 it breaks the macros in asm-generic/ioctl.h
 
 #define IOC_INOUT   ((_IOC_WRITE|_IOC_READ)  _IOC_DIRSHIFT)
 #define IOC_IN  (_IOC_WRITE  _IOC_DIRSHIFT)
 #define IOC_OUT (_IOC_READ  _IOC_DIRSHIFT)
 #define _IOC_DIR(nr)(((nr)  _IOC_DIRSHIFT)  _IOC_DIRMASK)
 
 Basically I don't mind too much since we already maintain our own
 version which also supports other unix systems and is not limited to
 linux only...
 
 Markus

Markus, this was granted to honor their prior efforts.

Please, don't use it against us.

Cheers,
Hermann


--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug in S2 API...

2009-09-21 Thread Markus Rechberger 

in dvb-frontend.c:
 
         if(cmd == FE_GET_PROPERTY) {

                 tvps = (struct dtv_properties __user *)parg;
                 dprintk(%s() properties.num = %d\n, __func__, tvps-num);
                 dprintk(%s() properties.props = %p\n, __func__, tvps-props);
                 ...
                 if (copy_from_user(tvp, tvps-props, tvps-num *
 sizeof(struct dtv_property)))
 


 OK,

 thought I'll have never to care for it again.

 ENUM calls should never be W.

 Hit me for all I missed.

 Cheers,
 Hermann

you are not seeing the point of it it seems

Documentation/ioctl-number.txt


If you are adding new ioctl's to the kernel, you should use the _IO
macros defined in linux/ioctl.h:

_IOan ioctl with no parameters
_IOW   an ioctl with write parameters (copy_from_user)
_IOR   an ioctl with read parameters  (copy_to_user)
_IOWR  an ioctl with both write and read parameters.

copy from user is required in order to copy the keys for the requested
elements into the kernel.
copy to user is finally used to play them back.

Markus
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug in S2 API...

2009-09-21 Thread hermann pitton 
Hi Markus,

Am Montag, den 21.09.2009, 12:02 +0200 schrieb Markus Rechberger:
 
 in dvb-frontend.c:
  
  if(cmd == FE_GET_PROPERTY) {
 
  tvps = (struct dtv_properties __user *)parg;
  dprintk(%s() properties.num = %d\n, __func__, tvps-num);
  dprintk(%s() properties.props = %p\n, __func__, 
 tvps-props);
  ...
  if (copy_from_user(tvp, tvps-props, tvps-num *
  sizeof(struct dtv_property)))
  
 
 
  OK,
 
  thought I'll have never to care for it again.
 
  ENUM calls should never be W.
 
  Hit me for all I missed.
 
  Cheers,
  Hermann
 
 you are not seeing the point of it it seems

you are right, I do not see your point at all, but I was wrong for the
get calls.

We had such discussions on v4l ioctls previously.

The result was to keep them as is and not to change IOR to IOWR to keep
compatibility.

This is six years back.

If you point me to a bug ever caused by it, I'll happily try to look it
up again.

Cheers,
Hermann

 Documentation/ioctl-number.txt
 
 
 If you are adding new ioctl's to the kernel, you should use the _IO
 macros defined in linux/ioctl.h:
 
 _IOan ioctl with no parameters
 _IOW   an ioctl with write parameters (copy_from_user)
 _IOR   an ioctl with read parameters  (copy_to_user)
 _IOWR  an ioctl with both write and read parameters.
 
 copy from user is required in order to copy the keys for the requested
 elements into the kernel.
 copy to user is finally used to play them back.
 
 Markus

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug in S2 API...

2009-09-21 Thread Markus Rechberger 
On Tue, Sep 22, 2009 at 4:00 AM, hermann pitton hermann-pit...@arcor.de wrote:
 Hi Markus,

 Am Montag, den 21.09.2009, 12:02 +0200 schrieb Markus Rechberger:
 
 in dvb-frontend.c:
  
          if(cmd == FE_GET_PROPERTY) {

                  tvps = (struct dtv_properties __user *)parg;
                  dprintk(%s() properties.num = %d\n, __func__, tvps-num);
                  dprintk(%s() properties.props = %p\n, __func__, 
 tvps-props);
                  ...
                  if (copy_from_user(tvp, tvps-props, tvps-num *
  sizeof(struct dtv_property)))
  


  OK,
 
  thought I'll have never to care for it again.
 
  ENUM calls should never be W.
 
  Hit me for all I missed.
 
  Cheers,
  Hermann

 you are not seeing the point of it it seems

 you are right, I do not see your point at all, but I was wrong for the
 get calls.

 We had such discussions on v4l ioctls previously.

 The result was to keep them as is and not to change IOR to IOWR to keep
 compatibility.

 This is six years back.


I think they all have got fixed up for v4l2 back then

#ifdef __OLD_VIDIOC_
/* for compatibility, will go away some day */
#define VIDIOC_OVERLAY_OLD  _IOWR('V', 14, int)
#define VIDIOC_S_PARM_OLD_IOW('V', 22, struct v4l2_streamparm)
#define VIDIOC_S_CTRL_OLD_IOW('V', 28, struct v4l2_control)
#define VIDIOC_G_AUDIO_OLD  _IOWR('V', 33, struct v4l2_audio)
#define VIDIOC_G_AUDOUT_OLD _IOWR('V', 49, struct v4l2_audioout)
#define VIDIOC_CROPCAP_OLD   _IOR('V', 58, struct v4l2_cropcap)
#endif

to eg:
#define VIDIOC_OVERLAY   _IOW('V', 14, int)
#define VIDIOC_S_PARM   _IOWR('V', 22, struct v4l2_streamparm)
#define VIDIOC_S_CTRL   _IOWR('V', 28, struct v4l2_control)
#define VIDIOC_G_AUDIO   _IOR('V', 33, struct v4l2_audio)
#define VIDIOC_G_AUDOUT  _IOR('V', 49, struct v4l2_audioout)
#define VIDIOC_CROPCAP  _IOWR('V', 58, struct v4l2_cropcap)

so only the DVB-API remains bugged now.

Markus
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug in S2 API...

2009-09-21 Thread hermann pitton 

Am Dienstag, den 22.09.2009, 05:31 +0200 schrieb Markus Rechberger:
 On Tue, Sep 22, 2009 at 4:00 AM, hermann pitton hermann-pit...@arcor.de 
 wrote:
  Hi Markus,
 
  Am Montag, den 21.09.2009, 12:02 +0200 schrieb Markus Rechberger:
  
  in dvb-frontend.c:
   
   if(cmd == FE_GET_PROPERTY) {
 
   tvps = (struct dtv_properties __user *)parg;
   dprintk(%s() properties.num = %d\n, __func__, 
  tvps-num);
   dprintk(%s() properties.props = %p\n, __func__, 
  tvps-props);
   ...
   if (copy_from_user(tvp, tvps-props, tvps-num *
   sizeof(struct dtv_property)))
   
 
 
   OK,
  
   thought I'll have never to care for it again.
  
   ENUM calls should never be W.
  
   Hit me for all I missed.
  
   Cheers,
   Hermann
 
  you are not seeing the point of it it seems
 
  you are right, I do not see your point at all, but I was wrong for the
  get calls.
 
  We had such discussions on v4l ioctls previously.
 
  The result was to keep them as is and not to change IOR to IOWR to keep
  compatibility.
 
  This is six years back.
 
 
 I think they all have got fixed up for v4l2 back then
 
 #ifdef __OLD_VIDIOC_
 /* for compatibility, will go away some day */
 #define VIDIOC_OVERLAY_OLD  _IOWR('V', 14, int)
 #define VIDIOC_S_PARM_OLD_IOW('V', 22, struct v4l2_streamparm)
 #define VIDIOC_S_CTRL_OLD_IOW('V', 28, struct v4l2_control)
 #define VIDIOC_G_AUDIO_OLD  _IOWR('V', 33, struct v4l2_audio)
 #define VIDIOC_G_AUDOUT_OLD _IOWR('V', 49, struct v4l2_audioout)
 #define VIDIOC_CROPCAP_OLD   _IOR('V', 58, struct v4l2_cropcap)
 #endif
 
 to eg:
 #define VIDIOC_OVERLAY   _IOW('V', 14, int)
 #define VIDIOC_S_PARM   _IOWR('V', 22, struct v4l2_streamparm)
 #define VIDIOC_S_CTRL   _IOWR('V', 28, struct v4l2_control)
 #define VIDIOC_G_AUDIO   _IOR('V', 33, struct v4l2_audio)
 #define VIDIOC_G_AUDOUT  _IOR('V', 49, struct v4l2_audioout)
 #define VIDIOC_CROPCAP  _IOWR('V', 58, struct v4l2_cropcap)
 
 so only the DVB-API remains bugged now.
 
 Markus

All fine.

But you do cut off parts of my messages.

I would like to ask you to exercise a breakage, not only for stuff six
years back, but on recent S2 API.

Likely you get some more interested into it then.

Cheers,
Hermann


--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug in S2 API...

2009-09-21 Thread Markus Rechberger 
On Tue, Sep 22, 2009 at 7:08 AM, hermann pitton hermann-pit...@arcor.de wrote:

 All fine.

 But you do cut off parts of my messages.

 I would like to ask you to exercise a breakage, not only for stuff six
 years back, but on recent S2 API.


at this time there are definitely no DVB applications available which
check the flags of the encoded IOCTL values for the S2-API
definitions, there are just very few applications available which use
this API at all.

if (IOCTL_WRITE(cmd))
  copy data into buffer for ioctl

-- from the driver side --
if (IOCTL_WRITE(cmd))
  copy data from the other side
process_ioctl()

At least the API specification should be valid however the rest is
implemented is another story. It works as it is but it's still wrong.
Since the S2-API is rather new it might be better to fix it up. It
would be a little bit weird too that coding style has a higher
priority in Linux than correct code.

it breaks the macros in asm-generic/ioctl.h

#define IOC_INOUT   ((_IOC_WRITE|_IOC_READ)  _IOC_DIRSHIFT)
#define IOC_IN  (_IOC_WRITE  _IOC_DIRSHIFT)
#define IOC_OUT (_IOC_READ  _IOC_DIRSHIFT)
#define _IOC_DIR(nr)(((nr)  _IOC_DIRSHIFT)  _IOC_DIRMASK)

Basically I don't mind too much since we already maintain our own
version which also supports other unix systems and is not limited to
linux only...

Markus
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bug in S2 API...

2009-09-20 Thread Markus Rechberger 
while porting the S2api to userspace I came accross the S2-API definition itself

#define FE_SET_PROPERTY_IOW('o', 82, struct dtv_properties)
#define FE_GET_PROPERTY_IOR('o', 83, struct dtv_properties)

while looking at this, FE_GET_PROPERTY should very likely be _IOWR

in dvb-frontend.c:

if(cmd == FE_GET_PROPERTY) {

tvps = (struct dtv_properties __user *)parg;

dprintk(%s() properties.num = %d\n, __func__, tvps-num);
dprintk(%s() properties.props = %p\n, __func__, tvps-props);
...
if (copy_from_user(tvp, tvps-props, tvps-num *
sizeof(struct dtv_property)))


Regards,
Markus
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug in S2 API...

2009-09-20 Thread hermann pitton 

Am Montag, den 21.09.2009, 05:40 +0200 schrieb Markus Rechberger:
 while porting the S2api to userspace I came accross the S2-API definition 
 itself
 
 #define FE_SET_PROPERTY_IOW('o', 82, struct dtv_properties)
 #define FE_GET_PROPERTY_IOR('o', 83, struct dtv_properties)
 
 while looking at this, FE_GET_PROPERTY should very likely be _IOWR
 
 in dvb-frontend.c:
 
 if(cmd == FE_GET_PROPERTY) {
 
 tvps = (struct dtv_properties __user *)parg;
 
 dprintk(%s() properties.num = %d\n, __func__, tvps-num);
 dprintk(%s() properties.props = %p\n, __func__, 
 tvps-props);
 ...
 if (copy_from_user(tvp, tvps-props, tvps-num *
 sizeof(struct dtv_property)))
 
 
 Regards,
 Markus

Seems to be a big issue.

Why you ever want to write to a get property?

Cheers,
Hermann


--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug in S2 API...

2009-09-20 Thread Markus Rechberger 
On Mon, Sep 21, 2009 at 5:46 AM, hermann pitton hermann-pit...@arcor.de wrote:

 Am Montag, den 21.09.2009, 05:40 +0200 schrieb Markus Rechberger:
 while porting the S2api to userspace I came accross the S2-API definition 
 itself

 #define FE_SET_PROPERTY            _IOW('o', 82, struct dtv_properties)
 #define FE_GET_PROPERTY            _IOR('o', 83, struct dtv_properties)

 while looking at this, FE_GET_PROPERTY should very likely be _IOWR

 in dvb-frontend.c:
 
         if(cmd == FE_GET_PROPERTY) {

                 tvps = (struct dtv_properties __user *)parg;

                 dprintk(%s() properties.num = %d\n, __func__, tvps-num);
                 dprintk(%s() properties.props = %p\n, __func__, 
 tvps-props);
                 ...
                 if (copy_from_user(tvp, tvps-props, tvps-num *
 sizeof(struct dtv_property)))
 

 Regards,
 Markus

 Seems to be a big issue.

 Why you ever want to write to a get property?


to read out the API version for example.
tvps-num is also used in order to check the boundaries of the property array.

Markus
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug in S2 API...

2009-09-20 Thread hermann pitton 

Am Montag, den 21.09.2009, 06:09 +0200 schrieb Markus Rechberger:
 On Mon, Sep 21, 2009 at 5:46 AM, hermann pitton hermann-pit...@arcor.de 
 wrote:
 
  Am Montag, den 21.09.2009, 05:40 +0200 schrieb Markus Rechberger:
  while porting the S2api to userspace I came accross the S2-API definition 
  itself
 
  #define FE_SET_PROPERTY_IOW('o', 82, struct dtv_properties)
  #define FE_GET_PROPERTY_IOR('o', 83, struct dtv_properties)
 
  while looking at this, FE_GET_PROPERTY should very likely be _IOWR
 
  in dvb-frontend.c:
  
  if(cmd == FE_GET_PROPERTY) {
 
  tvps = (struct dtv_properties __user *)parg;
 
  dprintk(%s() properties.num = %d\n, __func__, tvps-num);
  dprintk(%s() properties.props = %p\n, __func__, 
  tvps-props);
  ...
  if (copy_from_user(tvp, tvps-props, tvps-num *
  sizeof(struct dtv_property)))
  
 
  Regards,
  Markus
 
  Seems to be a big issue.
 
  Why you ever want to write to a get property?
 
 
 to read out the API version for example.
 tvps-num is also used in order to check the boundaries of the property array.
 
 Markus

Their are no writes allowed to manipulate get properties.

Cheers,
Hermann


--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug in S2 API...

2009-09-20 Thread Markus Rechberger 
On Mon, Sep 21, 2009 at 6:10 AM, hermann pitton hermann-pit...@arcor.de wrote:

 Am Montag, den 21.09.2009, 06:09 +0200 schrieb Markus Rechberger:
 On Mon, Sep 21, 2009 at 5:46 AM, hermann pitton hermann-pit...@arcor.de 
 wrote:
 
  Am Montag, den 21.09.2009, 05:40 +0200 schrieb Markus Rechberger:
  while porting the S2api to userspace I came accross the S2-API definition 
  itself
 
  #define FE_SET_PROPERTY            _IOW('o', 82, struct dtv_properties)
  #define FE_GET_PROPERTY            _IOR('o', 83, struct dtv_properties)
 
  while looking at this, FE_GET_PROPERTY should very likely be _IOWR
 
  in dvb-frontend.c:
  
          if(cmd == FE_GET_PROPERTY) {
 
                  tvps = (struct dtv_properties __user *)parg;
 
                  dprintk(%s() properties.num = %d\n, __func__, 
  tvps-num);
                  dprintk(%s() properties.props = %p\n, __func__, 
  tvps-props);
                  ...
                  if (copy_from_user(tvp, tvps-props, tvps-num *
  sizeof(struct dtv_property)))
  
 
  Regards,
  Markus
 
  Seems to be a big issue.
 
  Why you ever want to write to a get property?
 

 to read out the API version for example.
 tvps-num is also used in order to check the boundaries of the property 
 array.

 Markus

 Their are no writes allowed to manipulate get properties.


the writes are needed in order to submit tvps-num, although _IOR will
work _IOWR is the correct one in that case, aside of that you can just
compare it with other calls (eg. v4l2), the ENUM calls are all _IOWR.
They submit the index and retrieve the rest.

Markus
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug in S2 API...

2009-09-20 Thread hermann pitton 

Am Montag, den 21.09.2009, 06:42 +0200 schrieb Markus Rechberger:
 On Mon, Sep 21, 2009 at 6:10 AM, hermann pitton hermann-pit...@arcor.de 
 wrote:
 
  Am Montag, den 21.09.2009, 06:09 +0200 schrieb Markus Rechberger:
  On Mon, Sep 21, 2009 at 5:46 AM, hermann pitton hermann-pit...@arcor.de 
  wrote:
  
   Am Montag, den 21.09.2009, 05:40 +0200 schrieb Markus Rechberger:
   while porting the S2api to userspace I came accross the S2-API 
   definition itself
  
   #define FE_SET_PROPERTY_IOW('o', 82, struct dtv_properties)
   #define FE_GET_PROPERTY_IOR('o', 83, struct dtv_properties)
  
   while looking at this, FE_GET_PROPERTY should very likely be _IOWR
  
   in dvb-frontend.c:
   
   if(cmd == FE_GET_PROPERTY) {
  
   tvps = (struct dtv_properties __user *)parg;
  
   dprintk(%s() properties.num = %d\n, __func__, 
   tvps-num);
   dprintk(%s() properties.props = %p\n, __func__, 
   tvps-props);
   ...
   if (copy_from_user(tvp, tvps-props, tvps-num *
   sizeof(struct dtv_property)))
   
  
   Regards,
   Markus
  
   Seems to be a big issue.
  
   Why you ever want to write to a get property?
  
 
  to read out the API version for example.
  tvps-num is also used in order to check the boundaries of the property 
  array.
 
  Markus
 
  Their are no writes allowed to manipulate get properties.
 
 
 the writes are needed in order to submit tvps-num, although _IOR will
 work _IOWR is the correct one in that case, aside of that you can just
 compare it with other calls (eg. v4l2), the ENUM calls are all _IOWR.
 They submit the index and retrieve the rest.
 
 Markus

OK,

thought I'll have never to care for it again.

ENUM calls should never be W.

Hit me for all I missed.

Cheers,
Hermann


--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html