Hi Bjorn,
Thanks for the fix. I have tested this and it works perfectly!
On Sun, Mar 01, 2020 at 07:25:26PM -0800, Bjorn Andersson wrote:
> Lost in the translation from the user space implementation was the
> detail that HELLO mesages must be exchanged between each node pair. As
> such the incoming HELLO must be replied to.
>
Err. I thought the say_hello() part in ctrl_cmd_hello() was redundant, so
removed it :P
Sorry for that.
> Similar to the previous implementation no effort is made to prevent two
> Linux boxes from continuously sending HELLO messages back and forth,
> this is left to a follow up patch.
>
> say_hello() is moved, to facilitate the new call site.
>
> Fixes: 0c2204a4ad71 ("net: qrtr: Migrate nameservice to kernel from userspace")
> Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> ---
> net/qrtr/ns.c | 54 ++++++++++++++++++++++++++++-----------------------
> 1 file changed, 30 insertions(+), 24 deletions(-)
>
> diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
> index 7bfde01f4e8a..e3f11052b5f6 100644
> --- a/net/qrtr/ns.c
> +++ b/net/qrtr/ns.c
> @@ -286,9 +286,38 @@ static int server_del(struct qrtr_node *node, unsigned int port)
> return 0;
> }
>
> +static int say_hello(struct sockaddr_qrtr *dest)
> +{
> + struct qrtr_ctrl_pkt pkt;
> + struct msghdr msg = { };
> + struct kvec iv;
> + int ret;
> +
> + iv.iov_base = &pkt;
> + iv.iov_len = sizeof(pkt);
> +
> + memset(&pkt, 0, sizeof(pkt));
> + pkt.cmd = cpu_to_le32(QRTR_TYPE_HELLO);
> +
> + msg.msg_name = (struct sockaddr *)dest;
> + msg.msg_namelen = sizeof(*dest);
> +
> + ret = kernel_sendmsg(qrtr_ns.sock, &msg, &iv, 1, sizeof(pkt));
> + if (ret < 0)
> + pr_err("failed to send hello msg\n");
> +
> + return ret;
> +}
> +
> /* Announce the list of servers registered on the local node */
> static int ctrl_cmd_hello(struct sockaddr_qrtr *sq)
> {
> + int ret;
> +
> + ret = say_hello(sq);
> + if (ret < 0)
> + return ret;
> +
> return announce_servers(sq);
> }
>
> @@ -566,29 +595,6 @@ static void ctrl_cmd_del_lookup(struct sockaddr_qrtr *from,
> }
> }
>
> -static int say_hello(void)
> -{
> - struct qrtr_ctrl_pkt pkt;
> - struct msghdr msg = { };
> - struct kvec iv;
> - int ret;
> -
> - iv.iov_base = &pkt;
> - iv.iov_len = sizeof(pkt);
> -
> - memset(&pkt, 0, sizeof(pkt));
> - pkt.cmd = cpu_to_le32(QRTR_TYPE_HELLO);
> -
> - msg.msg_name = (struct sockaddr *)&qrtr_ns.bcast_sq;
> - msg.msg_namelen = sizeof(qrtr_ns.bcast_sq);
> -
> - ret = kernel_sendmsg(qrtr_ns.sock, &msg, &iv, 1, sizeof(pkt));
> - if (ret < 0)
> - pr_err("failed to send hello msg\n");
> -
> - return ret;
> -}
> -
> static void qrtr_ns_worker(struct work_struct *work)
> {
> const struct qrtr_ctrl_pkt *pkt;
> @@ -725,7 +731,7 @@ void qrtr_ns_init(struct work_struct *work)
> if (!qrtr_ns.workqueue)
> goto err_sock;
>
> - ret = say_hello();
> + ret = say_hello(&qrtr_ns.bcast_sq);
Why do you want to pass a global variable here? Why can't it be used directly
in say_hello() as done before?
Other than that,
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
Thanks,
Mani
> if (ret < 0)
> goto err_wq;
>
> --
> 2.24.0
>
