[Python-bugs-list] [ python-Bugs-555817 ] Flawed fcntl.ioctl implementation.

noreply@sourceforge.net noreply@sourceforge.net
Thu, 16 May 2002 03:34:13 -0700


Bugs item #555817, was opened at 2002-05-14 19:06
You can respond by visiting: 
http://sourceforge.net/tracker/?func=detail&atid=105470&aid=555817&group_id=5470

Category: Python Interpreter Core
Group: Python 2.1.2
Status: Open
Resolution: None
Priority: 5
Submitted By: Graham Horler (grahamh)
Assigned to: Nobody/Anonymous (nobody)
Summary: Flawed fcntl.ioctl implementation.

Initial Comment:
I'm doing some USB user-space driver programming 
in Python 2.1.3 under Linux 2.4.

When using a particular ioctl (HIDIOCGNAME), the 
return value as well as the (copy_to_user) binary data 
is significant.

Here is the code from line 547 of hiddev.c (kernel 
2.4.19-pre7):
  if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGNAME(0))) {
    int len;
    if (!hid->name) return 0;
    len = strlen(hid->name) + 1;
    if (len > _IOC_SIZE(cmd)) len = _IOC_SIZE(cmd);
    return copy_to_user((char *) arg, hid->name, len) ?
      -EFAULT : len;
  }

So here is the problem:  fcntl.ioctl() will only give me 
one or the other value, but not both.

I can work around this by initialising the input buffer to 
all chr(0), and then strip them off after.  But there will 
be a time when an ioctl call *requires* both values.

Until now I have appreciated the simple ioctl interface, 
but now it is shown to be an oversimplification.

It may be that POSIX, or some standard somewhere 
says that ioctl should not be used to do this, but 
maybe Python should support it anyway.

I suggest either:
1.  A new function ioctl2(fd, op, arg) that returns a 
2-tuple of (return_value, binary_buffer) and does not 
try to interpret the return value.
2.  An optional 4th parameter whose presence (but 
not value) requests the 2-tuple mentioned in (1).

Gotta love Python!

----------------------------------------------------------------------

>Comment By: Anthony Baxter (anthonybaxter)
Date: 2002-05-16 20:34

Message:
Logged In: YES 
user_id=29957

Hm. The solaris /proc lib I did in python, I used 'struct'
to dismantle C structures, but I did consider using byte
arrays. So that's one data point. 
(ok, it's not ioctls, but it's similar sort of data).
I'm loathe to go down the 'argument is mutable' optional
flag, but I really would prefer to make this work rather
than the ioctl2() approach, which really ain't that nice.


----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2002-05-16 19:21

Message:
Logged In: YES 
user_id=6656

I'm not *trying* to be a party-pooper, but there's another
problem: the point of passing a mutable array is so that
fcntl.ioctl can return ioctl(2)'s return code rather than
the string it returns now.  So if someone does use arrays
(or I guess Numeric.arrays?) for ioctls this would very
likely cause breakage.  I don't know how likely that is. 
Probably not very.  But I know that if this change broke my
code, I'd be a bit miffed.

All this is a bit annoying, as mutating arrays is clearly
the right solution -- where's that time machine?

----------------------------------------------------------------------

Comment By: Graham Horler (grahamh)
Date: 2002-05-16 08:50

Message:
Logged In: YES 
user_id=543663

I just realised - using array objects and changing in-place 
oversomes the arbitrary limit of 1024 bytes.  We could then 
achieve the full 16384 byte nirvana - hurrah!


----------------------------------------------------------------------

Comment By: Martin v. Löwis (loewis)
Date: 2002-05-16 03:21

Message:
Logged In: YES 
user_id=21627

I would not be concerned about the backwards compatibility,
either: if anybody is passing arrays, there is a good chance
that the ioctl does not modify the buffer, or that the
caller would not worry about the (missing) modification - or
else we had seen reports before.

So documenting in Misc/NEWS and the documentation that
mutable  objects supporting the (write) buffer interface may
now mutate seems sufficient.

----------------------------------------------------------------------

Comment By: Graham Horler (grahamh)
Date: 2002-05-16 02:32

Message:
Logged In: YES 
user_id=543663

Thanks Michael, its all incredibly clear now (sorry Martin).

I think using arrays sounds a great idea.
IMVHO as to backward compatibility, AFAICT its not 
documented that arrays can be passed to ioctl() anyway (only 
says int and string no?), so it's only breaking undocumented 
behaviour.

----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2002-05-16 01:38

Message:
Logged In: YES 
user_id=6656

grahamh: I think Martin understood you fine.

Remember that arrays are mutable, whereas strings are not.

So the idea would be that you'd write

buf = array.array('c', '\000'*256)
ret = fcntl.ioctl(fd, IOCTL_NUMBER, buf)

then the first "ret" bytes of "buf" would be valid.

However this possibly breaks backwards compatibility, as you
can already pass arrays into the ioctl function and they are
not modified.

----------------------------------------------------------------------

Comment By: Graham Horler (grahamh)
Date: 2002-05-16 00:44

Message:
Logged In: YES 
user_id=543663

Re Loewis:
Not sure what you mean, perhaps an example of the 
problem from me would help:

In C:
    char buf[256], strbuf[256];
    int ret, fd;
    fd = open(...)
    ret = ioctl(fd, IOCTL_NUMBER, buf);
    strncpy(strbuf, buf, ret);

This is impossible with the current fcntl module:
    buf = "X" * struct.calcsize("256s")
    buf = fcntl.ioctl(fd, IOCTL_NUMBER, buf)
    # The positive ioctl() return value is lost, and
    # hence I dont know how much of buf is significant

Extract from asm/unistd.h:

#define __syscall_return(type, res) \
do { \
        if ((unsigned long)(res) >= (unsigned long)(-125)) { \
                errno = -(res); \
                res = -1; \
        } \
        return (type) (res); \
} while (0)

So positive values are valid, but are being lost.

Anthony's patch looks good (thanks, I have not had a 
chance to test it myself yet), and the above example would 
be:
    buf = "X" * struct.calcsize("256s")
    ret, buf = fcntl.ioctl2(fd, IOCTL_NUMBER, buf)
    strbuf = buf[:ret] # Result!

Now, to test my understanding of your last comment about 
array objects Loewis, do you mean:
- pass in a mutable list that gets changed in place to [ret, 
buf]
- return (ret, buf) if called like this: ioctl(fd, IOC, [buf])
- or something else - please give example.

Thanks people.

----------------------------------------------------------------------

Comment By: Martin v. Löwis (loewis)
Date: 2002-05-15 22:04

Message:
Logged In: YES 
user_id=21627

Thinking about the problem again, I think the right solution
would be to support array objects as arguments to ioctl -
those would then be passed into the system call.

Am I correctly understanding the problem, that this would
not require a new function?

----------------------------------------------------------------------

Comment By: Anthony Baxter (anthonybaxter)
Date: 2002-05-14 22:06

Message:
Logged In: YES 
user_id=29957

possible patch at
http://sourceforge.net/tracker/index.php?func=detail&aid=555883&group_id=5470&atid=305470
I've only lightly tested it, not with any calls that return
useful values in the return code (couldn't find any easily, 
didn't feel like figuring out userspace drivers right now :)
Give it a whirl...

----------------------------------------------------------------------

Comment By: Anthony Baxter (anthonybaxter)
Date: 2002-05-14 20:10

Message:
Logged In: YES 
user_id=29957

Ouch. I think you're right - and I think that the
ioctl2() implementation is probably the only solution.
(I'm not so keen on the name, but I guess it kinda 
follows the 'standard' of other python functions, e.g.
popen)
I know changing the return of ioctl would be mega-ugly,
unless there was a new, optional "return a tuple"
argument. I doubt that's the appropriate fix. 

If the former approach is the appropriate one, it should
be simple enough to add an ioctl2() to the C source. I 
can probably whip it up now...


----------------------------------------------------------------------

You can respond by visiting: 
http://sourceforge.net/tracker/?func=detail&atid=105470&aid=555817&group_id=5470