Re: [PATCH] Introduce Qualcomm PDS service support

[Gary E. Miller]

Yo Aníbal!

On Mon, 25 May 2020 14:19:12 -0500
Aníbal Limón <address@hidden> wrote:

> The Qualcomm PDS service provides location data on a wide range of
> Qualcomm platforms. It used QMI encoded messages sent over a shared
> memory link, implemented in Linux as AF_QIPCRTR.
> 
> A special service is available on port -2 on the local node in the
> network, which provides functionality to the node address and port of
> registered services by id. As the driver is opened this mechanism is
> used to search for a registered PDS service in the system.
> 
> As the PDS driver is activated two messages are sent to the PDS
> service, the first one configures which events the service will send
> to the client (in our case NMEA reports) and the second starts the
> transmission of these packets. Similarly when the driver is
> deactivated a stop request is sent to the service.
> 
> Between the start and stop request the PDS service will send NMEA
> messages to the PDS client at a rate of 1 Hz, the NMEA string is
> extracted from the QMI encoded message and handed to the nmea_parse()
> function.
> 
> The PDS driver is selected by the url pds://<host>, where host is
> either a numerical identifier of the node in the AF_QIPCRTR network
> or the string "any".

Doc, like yours above, belongs in the patch itelf, so it lives on
instead of getting lost in the mailing list archive.

Can you add a link to doc on this PDS thing?

Can you make this a merge request?  A lot of stuff in here to
go over.

Any way to create regresion tests for this?  We have no way to
test.

> +    if config.CheckHeader(["bits/sockaddr.h", "linux/qrtr.h"]):

As bits.sockaddr.h tells you at the top:

    /*
     * Never include this file directly; use <sys/socket.h> instead.
     */


> diff --git a/driver_pds.c b/driver_pds.c

ALL gpsd c files have copyright, SPD-X and this at the top:

#include "gpsd_config.h"  /* must be before all includes */


> +     ret = recvfrom(session->gpsdata.gps_fd, buf, buflen, 0,
> +                    (struct sockaddr *)&sq, &sl);

Drivers do not get to bypass packet.c.  This could hang the process.

> +     hostname = session->gpsdata.dev.path + 6;

Missing length checks.  This will fail coverity checks.

> +     ret = sendto(sock, &pkt, sizeof(pkt), 0, (struct sockaddr
> *)&sq_ctrl, sizeof(sq_ctrl));

Blocking?  Can't block here.

> +             ret = recvfrom(sock, &pkt, sizeof(pkt), 0, (struct
> sockaddr *)&sq, &sl);

Blocking.  Can't block here.

>  #define GREIS_PACKET            16
> +#define QMI_PDS_PACKET          16
>  #define MAX_GPSPACKET_TYPE      16      /* increment this as

You forgot to increment!  You can not e-use the GREIS_PACKET value!

Gonna take somme work...

RGDS
GARY
---------------------------------------------------------------------------
Gary E. Miller Rellim 109 NW Wilmington Ave., Suite E, Bend, OR 97703
        address@hidden  Tel:+1 541 382 8588

            Veritas liberabit vos. -- Quid est veritas?
    "If you can't measure it, you can't improve it." - Lord Kelvin

pgpFk8G05l0Km.pgp
Description: OpenPGP digital signature