Received-SPF: pass (google.com: domain of linux-kernel+bounces-144543-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240329133458.323041-2-valla.francesco@gmail.com>
 <20240329133458.323041-3-valla.francesco@gmail.com> <CAMZ6RqKLaYb+8EaeoFMHofcaBT5G2-qdqSb4do73xrgMvWMZaA@mail.gmail.com>
 <9f5ad308-f2a0-47be-85f3-d152bc98099a@hartkopp.net> <CAMZ6RqKGKcYd4hAM8AVV72t78H-Kt92NXowx6Q+YCw=AuSxKuw@mail.gmail.com>
 <64586257-3cf6-4c10-a30b-200b1ecc5e80@hartkopp.net>
In-Reply-To: <64586257-3cf6-4c10-a30b-200b1ecc5e80@hartkopp.net>
From: Vincent Mailhol <vincent.mailhol@gmail.com>
Date: Mon, 15 Apr 2024 14:29:21 +0900
Message-ID: <CAMZ6RqKJH-Qmh4uXHj5Opj4PxDX=JDEBRR8gzsCuQ6pKHz1MfA@mail.gmail.com>
Subject: Re: [PATCH v2 1/1] Documentation: networking: document ISO 15765-2:2016
To: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: Francesco Valla <valla.francesco@gmail.com>, Marc Kleine-Budde <mkl@pengutronix.de>, 
	linux-can@vger.kernel.org, netdev@vger.kernel.org, 
	linux-kernel@vger.kernel.org, Simon Horman <horms@kernel.org>, 
	Bagas Sanjaya <bagasdotme@gmail.com>, fabio@redaril.me
Content-Type: text/plain; charset="UTF-8"

On Mon. 15 Apr. 2024 at 05:22, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 14.04.24 06:03, Vincent Mailhol wrote:
> >
> > This doesn't remove the fact that I think that this naming convention
> > is stupid because of the RAS syndrome, but I acknowledge that CAN CC
> > is now the official denomination and thus, that we should adopt it in
> > our documentation as well.
> >
>
> ;-)
>
>
> >>> Add a space between ISO and the number. Also, update the year:
> >>>
> >>>     ISO 15765-2:2024
> >>>
> >>
> >> Interesting! Didn't know there's already a new version.
> >>
> >> Will check this out whether we really support ISO 15765-2:2024 ...
> >>
> >> Do you have the standard at hand right now or should we leave this as
> >> ISO15765-2:2016 until we know?
> >
> > I have access to the newer revisions. But I never really invested time
> > into reading that standard (neither the 2016 nor the 2024 versions).
> >
> > Regardless, here is a verbatim extract from the Foreworld section of
> > ISO 15765-2:2024
> >
> >    This fourth edition cancels and replaces the third edition (ISO
> >    15765-2:2016), which has been technically revised.
> >
> >    The main changes are as follows:
> >
> >      - restructured the document to achieve compatibility with OSI
> >        7-layers model;
> >
> >      - introduced T_Data abstract service primitive interface to
> >        achieve compatibility with ISO 14229-2;
> >
> >      - moved all transport layer protocol-related information to Clause 9;
> >
> >      - clarification and editorial corrections
> >
>
> Yes, I've checked the release notes on the ISO website too.
> This really looks like editorial stuff that has nothing to do with the
> data protocol and its segmentation.

This is also my feeling. Short story, I think it is reasonable to
quote ISO 15765-2:2024 instead of ISO 15765-2:2016 in this document.

> >>>
> >>> Here, I would suggest the C99 designated field initialization:
> >>>
> >>>     struct sockaddr_can addr = {
> >>>             .can_family = AF_CAN;
> >>>             .can_ifindex = if_nametoindex("can0");
> >>>             .tp.tx_id = 0x18DA42F1 | CAN_EFF_FLAG;
> >>>             .tp.rx_id = 0x18DAF142 | CAN_EFF_FLAG;
> >>>     };
> >
> > Typo in my previous message: the designated initializers are not
> > separated by colon ";" but by comma ",". So it should have been:
> >
> >    struct sockaddr_can addr = {
> >          .can_family = AF_CAN,
> >          .can_ifindex = if_nametoindex("can0"),
> >          .tp.tx_id = 0x18DA42F1 | CAN_EFF_FLAG,
> >          .tp.rx_id = 0x18DAF142 | CAN_EFF_FLAG,
> >    };
> >
> >>> Well, this is just a suggestion, feel free to reject it if you do not like it.
> >>
> >> At least I don't like it.
> >>
> >> These values are usually interactively given on the command line:
> >>
> >>   >            .can_ifindex = if_nametoindex("can0");
> >>   >            .tp.tx_id = 0x18DA42F1 | CAN_EFF_FLAG;
> >>   >            .tp.rx_id = 0x18DAF142 | CAN_EFF_FLAG;
> >>
> >> So have it in a static field initialization leads to a wrong path IMO.
> >
> > There is no such limitation that C99 designated initializers should
> > only work with variables which have static storage duration. In my
> > suggested example, nothing is static.
> >
> > I see this as the same thing as below example:
> >
> >    int foo(void);
> >
> >    int bar()
> >    {
> >            int i = foo();
> >    }
> >
> >    int baz()
> >    {
> >            int i;
> >
> >            i = foo();
> >    }
> >
> > In bar(), the fact that the variable i is initialized at declaration
> > does not mean that it is static. In both examples, the variable i uses
> > automatic storage duration.
> >
> > Here, my preference goes to bar(), but I recognize that baz() is also
> > perfectly fine. Replace the int type by the struct sockaddr_can type
> > and the scalar initialization by designated initializers and you
> > should see the connection.
>
> Oh, sorry. Maybe I expressed myself wrong.
>
> IMHO your way to work with an initializer is correct from the C standpoint.
>
> But I think this is pretty unusual for a code example when an
> application programmer starts to work with ISO-TP.
>
> You usually get most of these values from the command line an fill the
> struct _by hand_ - and not with a static initialization.
>
> That was my suggestion.

OK. I get your point of view, which I think is a fair argument. So
let's keep it as is.

Just to conclude the debate, in real life, how to write it would
depend on the situation.

You may for example receive the values as function parameters:

  static int tp_bind(const char* ifname, canid_t rx_id, canid_t tx_id)
  {
          int s;
          struct sockaddr_can addr = {
                  .can_family = AF_CAN,
                  .can_ifindex = if_nametoindex(ifname),
                  .tp.rx_id = rx_id,
                  .tp.tx_id = tx_id,
          };
          int ret;

          s = socket(PF_CAN, SOCK_DGRAM, CAN_ISOTP);
          if (s < 0)
                  exit(1);

          ret = bind(s, (struct sockaddr *)&addr, sizeof(addr));
          if (ret < 0)
                  exit(1);

          return s;
  }

Or, if you get the values from the command line, you could have an
hybrid approach:

          int s;
          struct sockaddr_can addr = {
                  .can_family = AF_CAN,
          };
          char ifname[IF_NAMESIZE];
          canid_t rx_id, tx_id;
          int ret;

          /* Parse command line and fill ifname, rx_id and tx_id */
          addr.can_ifindex = if_nametoindex(ifname);
          addr.tp.rx_id = rx_id;
          addr.tp.tx_id = tx_id;

          s = socket(PF_CAN, SOCK_DGRAM, CAN_ISOTP);
          if (s < 0)
                  exit(1);

          ret = bind(s, (struct sockaddr *)&addr, sizeof(addr));
          if (ret < 0)
                  exit(1);

In the end, there is not enough context to decide which one is best.
And, I diverged too much. Francesco, keep the original.

> >
> > ** Different topic **
> >
> > While replying on this, I encountered something which made me worry a bit:
> >
> > The type of sockaddr_can.can_ifindex is a signed int:
> >
> >    https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/can.h#L243
> >
> > But if_nametoindex() returns an unsigned int:
> >
> >     https://man7.org/linux/man-pages/man3/if_nametoindex.3.html
> >
> > Shouldn't sockaddr_can.can_ifindex also be declared as an unsigned int?
> >
>
> The if_index derives from struct netdevice.if_index
>
> https://elixir.bootlin.com/linux/v6.8.6/source/include/linux/netdevice.h#L2158
>
> which is an int.

Ack.

> I don't think this would have an effect in real world to change
> sockaddr_can.can_ifindex to an unsigned int.
>
> I wonder if it is more critical to existing user space code to change it
> to unsigned int or to leave it as-is ...

The only impact is the warnings if compiled with the zealous flags
which warn on conversion between signed and unsigned, which would
normally be a sufficient reason to change. But, I understand that this
if_index is also a signed int at other locations in the kernel, so
let's keep it as such in our code.

> Best regards,
> Oliver