Re: change ifmedia word from int to uint64_t

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

Hi, Jason.

On 2019/04/18 22:52, Jason Thorpe 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 usually use __BIT() macros for 32bit values, but I sometimes
hesitate to use it for 64bit (or 16bit) values because man bit(3)'s
BUGS section says...

	http://cvsweb.netbsd.org/bsdweb.cgi/src/share/man/man3/bits.3.diff?r1=1.16&r2=1.17&f=h

Oops. I had not known the BUGS sections was removed until now!
BTW, is ti safe for 16bit values? mii(4)'s register definitions
don't use __BIT(). If it's usable I might modify to use it.

> 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?

Right. Though I didn't write about my idea in the previous mail,
I'd like to split the ifmedia word into two part at least. One
is to just select media type and another is for control options.
It's a future work.

>  I guess it makes the "grep" phase of the next overhaul a little easier :-)

If we change the declaration twice, 3rd party program might confuse and
be complex. So, IMHO, It would be good to just change int to uint64_t
this time. What do you think about this idea?

>>
>> 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
> 


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