On Thu, 29 Dec 2022 at 08:02, Devasish Dey <devasish....@syncmonk.net>
wrote:
> Hi Erez,
>
>
>> > + goto failed;
>> > + }
>>
>> I think it is better to merge this ioctl and the socket creation with
>> sk_get_ts_info().
>> No reason for duplication.
>> You can use a static function or inline one.
>
> The existing code seems to be more readable. One socket creation during
> link events and init time is not much heavy operation.
>
Code duplication is usually lighter, this is not an excuse to do so.
Readable is a lame excuse for laziness and bad coding, please avoid.
I respect your opinion anyhow.
Using shared code helps in avoiding duplicate bugs.
And when we improve code, we only need to do so once :-)
>
> > + /* clear data and ensure it is not marked valid */
>> > + memset(if_info, 0, sizeof(struct sk_if_info));
>> > + return -1;
>>
>> You need to close the socket!
>
> The close call is in place. The code structure has been kept similar
> to sk_get_ts_info. We want to follow the same
>
Yes, the same function structure in sk_get_ts_info().
That is why you should use common code, to avoid similar bugs in both
functions.
Also, we may want to use more functions like that in the future, or perhaps
using a new ioctl in the future..
This is why we try to avoid code duplications. (of more than 3 lines of
code :-)
> coding guidelines.
>
> Moreover, the bit period calculation has been updated with the new patch
> and will get called during initialization and whenever there is a link
> event.
>
Very good, as you submit a new version of the patch, we do try to catch up
and review them.
>
> Thanks,
> Devasish.
>
>
Erez
>
> On Wed, 14 Dec 2022 at 09:04, Richard Cochran <richardcoch...@gmail.com>
> wrote:
>
>> On Tue, Dec 13, 2022 at 10:39:59AM +0530, Devasish Dey wrote:
>>
>> > > +struct sk_if_info {
>> > > + bool valid;
>> > It is better not to use bool in a structure, as the size is usually
>> > int, i.e. 64 bits to hold 1 bit.
>>
>> Because we don't have huge arrays of these structs, the size isn't
>> critical IMO.
>>
>> > [Devasish]: Earlier we had it as an integer. We can update it to
>> uint8_t as
>> > well. But Richard suggested updating it to Boolean.
>> > So, leaving this to Richard.
>>
>> I think 'bool' is fine here.
>>
>> Thanks,
>> Richard
>>
> _______________________________________________
> Linuxptp-devel mailing list
> Linuxptp-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
>
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
