Re: change ifmedia word from int to uint64_t

[Masanobu SAITOH <msaitoh%execsw.org@localhost>]

On 2019/04/18 22:57, Jason Thorpe wrote:
> 
> 
>> On Apr 18, 2019, at 6:52 AM, Jason Thorpe <thorpej%me.com@localhost> wrote:
>>
>>
>>
>>> On Apr 18, 2019, at 2:32 AM, Masanobu SAITOH <msaitoh%execsw.org@localhost> wrote:
>>>
>>> Hi.
>>>
>>> I'm now working to extend ifmedia word to be able to support
>>> more than 10Gbps media.
>>>
>>> Patch:
>>>
>>> 	http://www.netbsd.org/~msaitoh/ifmedia64-20190418-0.dif
>>
>> It might be nice to use __BIT(), __SHIFT*() etc. macros (if they can do ULL constants)... I think it would make all of the constants a lot easier to read.
>>
>> I suppose we could add an ifmword_t ... but if we have to enlarge it again, it's going to become a struct (or something else entirely), right?  I guess it makes the "grep" phase of the next overhaul a little easier :-)
> 
> Also:
> 
> Index: sys/sys/sockio.h
> ===================================================================
> RCS file: /cvsroot/src/sys/sys/sockio.h,v
> retrieving revision 1.36
> diff -u -p -r1.36 sockio.h
> --- sys/sys/sockio.h	21 Dec 2018 08:58:08 -0000	1.36
> +++ sys/sys/sockio.h	18 Apr 2019 09:12:46 -0000
> @@ -90,8 +90,9 @@
>  #define	SIOCGETVIFCNT	_IOWR('u', 51, struct sioc_vif_req)/* vif pkt cnt */
>  #define	SIOCGETSGCNT	_IOWR('u', 52, struct sioc_sg_req) /* sg pkt cnt */
>  
> -#define	SIOCSIFMEDIA	_IOWR('i', 53, struct ifreq)	/* set net media */
> -#define	SIOCGIFMEDIA	_IOWR('i', 54, struct ifmediareq) /* get net media */
> +/* 53 and 54 used to be SIOC[SG]IFMEDIA with a 32 bit media word */
> +#define	SIOCSIFMEDIA	_IOWR('i', 55, struct ifreq)	/* set net media */
> +#define	SIOCGIFMEDIA	_IOWR('i', 56, struct ifmediareq) /* get net media */
>  
>  #define	SIOCSIFGENERIC	 _IOW('i', 57, struct ifreq)	/* generic IF set op */
>  #define	SIOCGIFGENERIC	_IOWR('i', 58, struct ifreq)	/* generic IF get op */
> 
> You don't have to allocate new numbers; the size of the argument structure changes, and therefore the value of SIOCSIFMEDIA / SIOCGIFMEDIA changes.

Good catch! SIOCGIFMEDIA will have different size, so I'll keep the number.
For SIOCSIFMEDIA, the ifreq's member is union-ed and the max size is greater
than uint64_t. I think this new ioctl's number must be changed.

> 
>>
>>>
>>> I changed ifmedia's word from int to uint64_t. It's almost the
>>> same as OpenBSD who did 5 years ago.
>>>
>>> What do you think about the above diff?
>>> IMHO, it would be better to change ifmword_t(with uint64_t)
>>> than uint64_t. For example
>>>
>>> (from ifconfig/media.c's diff)
>>>> @@ -374,7 +374,8 @@ media_status(prop_dictionary_t env, prop
>>>> 	/* Interface link status is queried through SIOCGIFMEDIA.
>>>> 	 * Not all interfaces have actual media. */
>>>> 	if (ifmr.ifm_count != 0) {
>>>> -		media_list = (int *)malloc(ifmr.ifm_count * sizeof(int));
>>>> +		media_list = (uint64_t *)malloc(ifmr.ifm_count
>>>> +		    * sizeof(*media_list));
>>>> 		if (media_list == NULL)
>>>> 			err(EXIT_FAILURE, "malloc");
>>>> 		ifmr.ifm_ulist = media_list;
>>>
>>> If we use ifmword_t, it makes easy to change the size in future.
>>>
>>>
>>> This patch has some TODOs:
>>>
>>> - COMPAT_NETBSD32? (currently i386 old binary doesn't work on
>>>  amd64)
>>> - I would change the hook point. Current location is not good
>>>  because we have to add "case: OSIOC[GS]IFMEDIA" to all driver
>>>  which calls ifmedia_ioctl(). (see if_wm.c's patch)
>>>
>>>
>>> Thanks.
>>>
>>> -- 
>>> -----------------------------------------------
>>>               SAITOH Masanobu (msaitoh%execsw.org@localhost
>>>                                msaitoh%netbsd.org@localhost)
>>
>> -- thorpej
> 
> -- thorpej
> 


-- 
-----------------------------------------------
                SAITOH Masanobu (msaitoh%execsw.org@localhost
                                 msaitoh%netbsd.org@localhost)