Hi Shibin Thanks for the review. Comments inline. BR Maryam On 30/11/2023 12:14, Koikkara Reeny, Shibin wrote: > Hi Maryam, > > I have added some suggestion below. > > Regrads, > Shibin [snip] > NOTICE); > #define UDS_MAX_CMD_LEN 64 > #define UDS_MAX_CMD_RESP 128 > #define UDS_XSK_MAP_FD_MSG "/xsk_map_fd" > -#define UDS_SOCK "/tmp/afxdp.sock" > #define UDS_CONNECT_MSG "/connect" > #define UDS_HOST_OK_MSG "/host_ok" > #define UDS_HOST_NAK_MSG "/host_nak" > @@ -171,6 +170,7 @@ struct pmd_internals { > bool custom_prog_configured; > bool force_copy; > bool use_cni; > + char sock_path[PATH_MAX]; > I would recommend using variable name as "uds_path". No issues in renaming > >> struct bpf_map *map; >> >> struct rte_ether_addr eth_addr; >> @@ -191,6 +191,7 @@ struct pmd_process_private { >> #define ETH_AF_XDP_BUDGET_ARG "busy_budget" >> #define ETH_AF_XDP_FORCE_COPY_ARG "force_copy" >> #define ETH_AF_XDP_USE_CNI_ARG "use_cni" >> +#define ETH_AF_XDP_SOCK_ARG "sock" > To make it clear would recommend using "sock_path" and also ETH_AF_XDP_CNI_UDS_PATH_ARG or ETH_AF_XDP_SOCK_PATH_ARG No issues in renaming > . > >> static const char * const valid_arguments[] = { >> ETH_AF_XDP_IFACE_ARG, >> @@ -201,6 +202,7 @@ static const char * const valid_arguments[] = { >> ETH_AF_XDP_BUDGET_ARG, >> ETH_AF_XDP_FORCE_COPY_ARG, >> ETH_AF_XDP_USE_CNI_ARG, >> + ETH_AF_XDP_SOCK_ARG, >> NULL >> }; >> >> @@ -1351,7 +1353,7 @@ configure_preferred_busy_poll(struct >> pkt_rx_queue *rxq) } >> >> static int >> -init_uds_sock(struct sockaddr_un *server) >> +init_uds_sock(struct sockaddr_un *server, const char *sock_path) >> { >> int sock; >> >> @@ -1362,7 +1364,7 @@ init_uds_sock(struct sockaddr_un *server) >> } >> >> server->sun_family = AF_UNIX; >> - strlcpy(server->sun_path, UDS_SOCK, sizeof(server->sun_path)); >> + strlcpy(server->sun_path, sock_path, sizeof(server->sun_path)); >> >> if (connect(sock, (struct sockaddr *)server, sizeof(struct >> sockaddr_un)) < 0) { >> close(sock); >> @@ -1382,7 +1384,7 @@ struct msg_internal { }; >> >> static int >> -send_msg(int sock, char *request, int *fd) >> +send_msg(int sock, char *request, int *fd, const char *sock_path) >> { >> int snd; >> struct iovec iov; >> @@ -1393,7 +1395,7 @@ send_msg(int sock, char *request, int *fd) >> >> memset(&dst, 0, sizeof(dst)); >> dst.sun_family = AF_UNIX; >> - strlcpy(dst.sun_path, UDS_SOCK, sizeof(dst.sun_path)); >> + strlcpy(dst.sun_path, sock_path, sizeof(dst.sun_path)); >> >> /* Initialize message header structure */ >> memset(&msgh, 0, sizeof(msgh)); >> @@ -1471,7 +1473,7 @@ read_msg(int sock, char *response, struct >> sockaddr_un *s, int *fd) >> >> static int >> make_request_cni(int sock, struct sockaddr_un *server, char *request, >> - int *req_fd, char *response, int *out_fd) >> + int *req_fd, char *response, int *out_fd, const char >> *sock_path) >> { >> int rval; >> >> @@ -1483,7 +1485,7 @@ make_request_cni(int sock, struct sockaddr_un >> *server, char *request, >> if (req_fd == NULL) >> rval = write(sock, request, strlen(request)); >> else >> - rval = send_msg(sock, request, req_fd); >> + rval = send_msg(sock, request, req_fd, sock_path); >> >> if (rval < 0) { >> AF_XDP_LOG(ERR, "Write error %s\n", strerror(errno)); @@ >> -1507,7 +1509,7 @@ check_response(char *response, char *exp_resp, long >> size) } >> >> static int >> -get_cni_fd(char *if_name) >> +get_cni_fd(char *if_name, const char *sock_path) >> { >> char request[UDS_MAX_CMD_LEN], >> response[UDS_MAX_CMD_RESP]; >> char hostname[MAX_LONG_OPT_SZ], >> exp_resp[UDS_MAX_CMD_RESP]; @@ -1520,14 +1522,14 @@ >> get_cni_fd(char *if_name) >> return -1; >> >> memset(&server, 0, sizeof(server)); >> - sock = init_uds_sock(&server); >> + sock = init_uds_sock(&server, sock_path); >> if (sock < 0) >> return -1; >> >> /* Initiates handshake to CNI send: /connect,hostname */ >> snprintf(request, sizeof(request), "%s,%s", UDS_CONNECT_MSG, >> hostname); >> memset(response, 0, sizeof(response)); >> - if (make_request_cni(sock, &server, request, NULL, response, >> &out_fd) < 0) { >> + if (make_request_cni(sock, &server, request, NULL, response, >> &out_fd, >> +sock_path) < 0) { > Why do we need to pass "sock_path" here as we have already connected the sock with sock_path in init_uds_sock()? it's used in the send_msg() function insidemake_request_cni(). In the original implementation of send_msg used a #defined value which is why it wasn't needed before. so as far as I can tell send_msg still needs that path. but I'm open to suggestions. > >> AF_XDP_LOG(ERR, "Error in processing cmd [%s]\n", >> request); >> goto err_close; >> } >> @@ -1541,7 +1543,7 @@ get_cni_fd(char *if_name) >> /* Request for "/version" */ >> strlcpy(request, UDS_VERSION_MSG, UDS_MAX_CMD_LEN); >> memset(response, 0, sizeof(response)); >> - if (make_request_cni(sock, &server, request, NULL, response, >> &out_fd) < 0) { >> + if (make_request_cni(sock, &server, request, NULL, response, >> &out_fd, >> +sock_path) < 0) { > Same question as above. Please see previous ans > >> AF_XDP_LOG(ERR, "Error in processing cmd [%s]\n", >> request); >> goto err_close; >> } >> @@ -1549,7 +1551,7 @@ get_cni_fd(char *if_name) >> /* Request for file descriptor for netdev name*/ >> snprintf(request, sizeof(request), "%s,%s", >> UDS_XSK_MAP_FD_MSG, if_name); >> memset(response, 0, sizeof(response)); >> - if (make_request_cni(sock, &server, request, NULL, response, >> &out_fd) < 0) { >> + if (make_request_cni(sock, &server, request, NULL, response, >> &out_fd, >> +sock_path) < 0) { > Same question as above. Please see previous ans > >> AF_XDP_LOG(ERR, "Error in processing cmd [%s]\n", >> request); >> goto err_close; >> } >> @@ -1571,7 +1573,7 @@ get_cni_fd(char *if_name) >> /* Initiate close connection */ >> strlcpy(request, UDS_FIN_MSG, UDS_MAX_CMD_LEN); >> memset(response, 0, sizeof(response)); >> - if (make_request_cni(sock, &server, request, NULL, response, >> &out_fd) < 0) { >> + if (make_request_cni(sock, &server, request, NULL, response, >> &out_fd, >> +sock_path) < 0) { > Same question as above. Please see previous ans > >> AF_XDP_LOG(ERR, "Error in processing cmd [%s]\n", >> request); >> goto err_close; >> } >> @@ -1698,7 +1700,7 @@ xsk_configure(struct pmd_internals *internals, >> struct pkt_rx_queue *rxq, >> int err, fd, map_fd; >> >> /* get socket fd from CNI plugin */ >> - map_fd = get_cni_fd(internals->if_name); >> + map_fd = get_cni_fd(internals->if_name, internals- >>> sock_path); >> if (map_fd < 0) { >> AF_XDP_LOG(ERR, "Failed to receive CNI plugin >> fd\n"); >> goto out_xsk; >> @@ -2023,7 +2025,8 @@ xdp_get_channels_info(const char *if_name, int >> *max_queues, static int parse_parameters(struct rte_kvargs *kvlist, char >> *if_name, int *start_queue, >> int *queue_cnt, int *shared_umem, char *prog_path, >> - int *busy_budget, int *force_copy, int *use_cni) >> + int *busy_budget, int *force_copy, int *use_cni, >> + char *sock_path) >> { >> int ret; >> >> @@ -2069,6 +2072,11 @@ parse_parameters(struct rte_kvargs *kvlist, char >> *if_name, int *start_queue, >> if (ret < 0) >> goto free_kvlist; >> >> + ret = rte_kvargs_process(kvlist, ETH_AF_XDP_SOCK_ARG, >> + &parse_prog_arg, sock_path); > Parse_prog_arg does 2 things copy the sock_arg value and also check the access to the socket. > Checking access here has a chance of causing raise condition so I would recommend to skip this check here as this will be taken care in the init_uds_sock(). If there's an issue here processing is abandoned. Is it not better to catch any issue early on? I'm not sure I understand the race condition? > >> + if (ret < 0) >> + goto free_kvlist; >> + >> free_kvlist: >> rte_kvargs_free(kvlist); >> return ret; >> @@ -2108,7 +2116,7 @@ static struct rte_eth_dev * init_internals(struct >> rte_vdev_device *dev, const char *if_name, >> int start_queue_idx, int queue_cnt, int shared_umem, >> const char *prog_path, int busy_budget, int force_copy, >> - int use_cni) >> + int use_cni, const char *sock_path) >> { >> const char *name = rte_vdev_device_name(dev); >> const unsigned int numa_node = dev->device.numa_node; @@ - >> 2138,6 +2146,7 @@ init_internals(struct rte_vdev_device *dev, const char >> *if_name, >> internals->shared_umem = shared_umem; >> internals->force_copy = force_copy; >> internals->use_cni = use_cni; >> + strlcpy(internals->sock_path, sock_path, PATH_MAX); >> >> if (xdp_get_channels_info(if_name, &internals->max_queue_cnt, >> &internals->combined_queue_cnt)) { @@ - >> 2328,6 +2337,7 @@ rte_pmd_af_xdp_probe(struct rte_vdev_device *dev) >> int busy_budget = -1, ret; >> int force_copy = 0; >> int use_cni = 0; >> + char sock_path[PATH_MAX] = {'\0'}; >> struct rte_eth_dev *eth_dev = NULL; >> const char *name = rte_vdev_device_name(dev); >> >> @@ -2370,7 +2380,7 @@ rte_pmd_af_xdp_probe(struct rte_vdev_device >> *dev) >> >> if (parse_parameters(kvlist, if_name, &xsk_start_queue_idx, >> &xsk_queue_cnt, &shared_umem, prog_path, >> - &busy_budget, &force_copy, &use_cni) < 0) { >> + &busy_budget, &force_copy, &use_cni, >> sock_path) < 0) { >> AF_XDP_LOG(ERR, "Invalid kvargs value\n"); >> return -EINVAL; >> } >> @@ -2387,6 +2397,13 @@ rte_pmd_af_xdp_probe(struct rte_vdev_device >> *dev) >> return -EINVAL; >> } >> >> + if (use_cni && !strnlen(sock_path, PATH_MAX)) { >> + AF_XDP_LOG(ERR, "When '%s' parameter is used, '%s' must >> also be provided\n", >> + ETH_AF_XDP_USE_CNI_ARG, >> ETH_AF_XDP_SOCK_ARG); >> + return -EINVAL; >> + } >> + >> + >> if (strlen(if_name) == 0) { >> AF_XDP_LOG(ERR, "Network interface must be >> specified\n"); >> return -EINVAL; >> @@ -2410,7 +2427,7 @@ rte_pmd_af_xdp_probe(struct rte_vdev_device >> *dev) >> >> eth_dev = init_internals(dev, if_name, xsk_start_queue_idx, >> xsk_queue_cnt, shared_umem, prog_path, >> - busy_budget, force_copy, use_cni); >> + busy_budget, force_copy, use_cni, >> sock_path); >> if (eth_dev == NULL) { >> AF_XDP_LOG(ERR, "Failed to init internals\n"); >> return -1; >> @@ -2471,4 +2488,5 @@ >> RTE_PMD_REGISTER_PARAM_STRING(net_af_xdp, >> "xdp_prog= " >> "busy_budget= " >> "force_copy= " >> - "use_cni= "); >> + "use_cni= " >> + "sock= "); >> -- >> 2.41.0