_IOWR when errno != 0

[Xin LI]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2010/04/13 05:53, John Baldwin wrote:
> On Monday 12 April 2010 8:26:48 pm Xin LI wrote:
>> On 2010/04/12 16:33, Alfred Perlstein wrote:
>>> * Xin LI <delphij at delphij.net> [100412 15:28] wrote:
>>>> -----BEGIN PGP SIGNED MESSAGE-----
>>>> Hash: SHA1
>>>>
>>>> Hi,
>>>>
>>>> Is there a sane way to copyout ioctl request when the returning errno !=
>>>> 0?  Looking at the code, currently, in sys/kern/sys_generic.c, we have:
>>>>
>>>> ===========
>>>>         error = kern_ioctl(td, uap->fd, com, data);
>>>>
>>>>         if (error == 0 && (com & IOC_OUT))
>>>>                 error = copyout(data, uap->data, (u_int)size);
>>>> ===========
>>>>
>>>> Is there any objection if I change it to something like:
>>>>
>>>> ===========
>>>>         saved_error = kern_ioctl(td, uap->fd, com, data);
>>>>
>>>>         if (com & IOC_OUT)
>>>>                 error = copyout(data, uap->data, (u_int)size);
>>>>         if (saved_error)
>>>>                 error = saved_error;
>>>> ===========
>>>
>>> Is this for linux compat?
>>
>> Do they do this way?  I'm not quite sure :-/
>>
>> I got a bug report and am thinking about how to fix it, it seems that we
>> do not have a generic way of returning an error number while giving some
>> "hints" about the error at the same time, for the ioctl() call.  Adding
>> an extra pointer to the request structure seems to be a last-resort way
>> and sounds to be ugly.
> 
> Actually, this pattern of embedding an error is quite common.  The mfi(4) and 
> mpt(4) pass-thru ioctls to send firmware commands embed the return status of 
> any firmware command in the structure that is passed in and out for example.
> 
>>> I'm not sure this would work, it might seriously break userland
>>> compat.  Have you looked around/queiried what the expected outcome
>>> is from a bad ioctl?  By default the buffer will be zero'd this
>>> might be unexpected by apps.  (all or nothing)
>>
>> Yes that's exactly why I'm asking, my understanding is that for normal
>> usages would be something like:
>>
>> 	if (ioctl(fd, SIOCSOMETHING, &req) < 0) {
>> 		// do something to handle the error
>> 	} else {
>> 		// use data fed back from req
>> 	}
>>
>> In this case, I think the result would not be affected.  Is there many
>> (if any) programs that don't bother to check return value of ioctl()?
>>
>> Speaking for the userland buffer, for _IOR ioctls, the side effect would
>> be that userland would see a zeroed out 'req' structure (kernel buffer
>> gets zeroed out before calling kern_ioctl), or "half-baked" one (the
>> kernel code may have only written partial data).  For _IOWR ioctls, the
>> side effect would be that the userland may get half-baked data.
> 
> You miss one important variation where the error handling involves adjusting 
> the request and retrying (or submitting the same request to a different ioctl 
> to handle renumbering conflicts, etc.).  Other APIs such as sysctl(2) and 
> setsockopt(2) can leave partial data, but the callers of those APIs expect 
> that (and in fact, those APIs return the actual length of data that is copied 
> out).  ioctl(2) has not had that behavior, however, and I would find it 
> surprising.

I see, that's what I am concerned about, thanks for the explanation.

In order to maintain ABI compatibility I now have a patch which changs
the current behavior of SIOCGIFDESCR to set the buffer field to NULL and
return no errno.  The existing code in -HEAD doesn't seem to work when
the field is big :(

I will post the patch to -net@ for review once I get it tested.

Cheers,
- -- 
Xin LI <delphij at delphij.net>	http://www.delphij.net/
FreeBSD - The Power to Serve!	       Live free or die
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.14 (FreeBSD)

iQEcBAEBAgAGBQJLxL96AAoJEATO+BI/yjfBkkMIAIVjUmwrfHLl5F+mIlRD+Zpv
hYZVBZaeu3/ymv0Zepo5vhbvJCOWxgdtRnJgoVlkpglZLwVrKkAdfxWp/di5n8xm
O4BMc+BIra6tYnqaxmbCYoigKGoLVhim1n6j2Xld/h0n91ErBDpdrWBdHVbs8uV+
mRFLCPbGzGnEXw68rdbWjXFIDRIe7btTdmyYotaHd5AFaqQw6EM+OAXRG3UqGtm3
92o+9TW2LcTTP9gyresbQGoXvITHXVfSdihhDVfDMCtbaClQ+IFlny0oGqg0DttR
OhnEWDvBgUQD+aADYx2k8YLXziUsQzvTc7WTZuoxdz3LzZVecyQSewiydEhor/U=
=IHjf
-----END PGP SIGNATURE-----