Re: CVS commit: src/sys

2021-02-07 Thread Roy Marples

On 04/02/2021 20:18, matthew green wrote:

Roy Marples writes:

On 03/02/2021 21:45, David Young wrote:

On Wed, Feb 03, 2021 at 05:51:40AM +, Roy Marples wrote:

Module Name:src
Committed By:   roy
Date:   Wed Feb  3 05:51:40 UTC 2021

Modified Files:
src/sys/net: if_arp.h if_ether.h if_gre.h
src/sys/netinet: if_ether.h igmp.h in.h ip.h ip6.h ip_carp.h ip_icmp.h
ip_mroute.h ip_var.h tcp.h tcp_debug.h tcp_var.h udp.h udp_var.h

Log Message:
Remove __packed from various network structures

They are already network aligned and adding the __packed attribute
just causes needless compiler warnings about accssing members of packed

This change looks a little hasty to me.

It looks to me like some of these structs were __packed so that
they could be read/written directly from/to any offset in a packet
chain using mtod(), which does not pay any mind to the alignment
of `*t`:

#define mtod(m, t)  ((t)((m)->m_data))

I see gre_h is accessed in that way, just for one example.  I don't
see any reason in principle that every gre_h accessed through mtod()
should be 16-bit aligned in its buffer, but that is the alignment
the compiler will expect if gre_h is not __packed.  If the actual
alignment ever differs from compiler's expectation, then there
could be a bus error or an unwanted byte rotation/shift.

It looks to me like there's a bit of cleanup to do elsewhere before
removing __packed from network structures.

ssh over a gre tunnel using erlite (mips64) and pinebook (aarch64) as both ends
seems to work fine. I also tested an amd64 endpoint.

Not that I disagree with your assessment that the code can always be improved.

i looked at removing __packed from these when GCC 9 came
around and really started complaining about them.  however,
i was not able to convince myself that all the users were
actually safe if __packed was removed.

in particular, 'struct ip' has 4-byte objects at offset
14, 18, 22, and 26.  code accessing data directly from the
network may fail, and eg, mtod() makes it virtually
impossible to check for this at compile time.  sanitizers
could check at run time.

Isn't that already solved here?

And if I'm not wrong, just ignoring the warning causes alignment failures as it 
stands? So it's just swapping one bad thing with another? If so, then there is 
no right answer other than doing similar alignment checks for our mbufs.


Re: CVS commit: src/share/man/man9

2021-02-07 Thread nia
On Sun, Feb 07, 2021 at 12:43:40PM +0900, Tetsuya Isaki wrote:
> > @@ -175,9 +175,9 @@
> >  .Vt audio_format_t
> >  structure according to given number
> >  .Va afp->index .
> > -If there is no format with given number, return
> > +If there is no format with the given number, return
> >  .Er EINVAL .
> > -It is called at any time.
> > +It can be called at any time.
> The later sounds to me "You(developer of MD driver) can call
> it at any time".  If so, it's incorrect.

Maybe "it can be called by the MI layer at any time" is clearer
here, then? I can change it to that.

> >  Similarly, if the driver supports
> >  .Dv SLINEAR_OE:16
> >  and the upper layer chooses it,
> > -the driver does not need to provide a conversion function.
> > -Because the upper layer only supports conversion between
> > +the driver does not need to provide a conversion function,
> > +because the upper layer supports conversion between
> Is "only" a typo?  or is it better to remove it in English?
> Thanks,
> ---
> Tetsuya Isaki 

I think it's clear that conversion of other formats is not
supported by the rest of the paragraph, so it doesn't need to
be mentioned here, where the primary purpose of the sentence
is to explain why you don't need to handle conversion in that
case yourself.

That's just my opinion, though - I'm not an English expert, there's
lots of rules I know but cannot explain.