Re: [PATCH] Define SO_RCVTIMEO and SO_SNDTIMEO.

[Christopher Baines]

Thanks for the feedback Maxime, I obviously lost track of some things
after letting this patch languish for a while.

Maxime Devos <maximedevos@telenet.be> writes:

> On 17-09-2022 10:05, Christopher Baines wrote:
>> +  if (ioptname == SO_RCVTIMEO || ioptname == SO_SNDTIMEO)
>> +    {
>> +      SCM_ASSERT (scm_is_pair (value), value, SCM_ARG4, FUNC_NAME);
>
> How about using SCM_ASSERT_TYPE instead to improve the error message a
> little?

I've switched to using SCM_ASSERT_TYPE now.

>> +      opt_time.tv_sec = scm_to_ulong (SCM_CAR (value));
>> +      opt_time.tv_usec = scm_to_ulong (SCM_CDR (value));
> I think it needs to be documented how to actually use SO_RCVTIMEO /
> SO_SNDTIMEO (more specifically, the types).  For example, the user
> might expect to enter flonums 1.567 as time durations (in seconds)
> instead, or a duration from SRFI 19.  How is the user supposed to know
> the appropriate type, aside from experimenting and reading the source
> code?
>
> Also, it is not documented there is a maximum on the number of
> seconds, I recommend to document that there is some limit, or
> alternatively clip numbers to the maximum.

I've added some documentation now, I'm not sure the interface is
perfect, but it should work.

>> +      optlen = sizeof (opt_time);
>> +      optval = &opt_time;
>> +    }
>> +
>
> I think this needs a #if defined(SO_RCVTIMEO) && defined(SO_SNDTIMEO),
> for systems that don't have those.  Alternatively, if all systems have
> those, it seems to me that the #ifdef in

I guess it should be conditional, so I've added the if in to getsockopt
and setsockopt.

>> +#ifdef SO_RCVTIMEO
>> +  scm_c_define ("SO_RCVTIMEO", scm_from_int (SO_RCVTIMEO));
>> +#endif
>> +#ifdef SO_SNDTIMEO
>> +  scm_c_define ("SO_SNDTIMEO", scm_from_int (SO_SNDTIMEO));
>> +#endif
>
> can be removed.

signature.asc
Description: PGP signature