| Message ID | 1563922721-122312-1-git-send-email-u9012063@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] netdev-afxdp: Error when no XDP program loaded. | expand |
On 24.07.2019 1:58, William Tu wrote: > netdev-afxdp requires XDP program to be loaded. When prog_id == 0, > it indicates no XDP program, so return error and free resources. > > Signed-off-by: William Tu <u9012063@gmail.com> > --- > lib/netdev-afxdp.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c > index 6b0b93e7f432..79c7a442e9cf 100644 > --- a/lib/netdev-afxdp.c > +++ b/lib/netdev-afxdp.c > @@ -283,6 +283,12 @@ xsk_configure_socket(struct xsk_umem_info *umem, uint32_t ifindex, > free(xsk); > return NULL; > } > + if (!prog_id) { > + VLOG_ERR("No XDP program is loaded at ifindex %d", ifindex); > + xsk_socket__delete(xsk->xsk); > + free(xsk); > + return NULL; > + } What do you think about combining this 'if' with the previous error checking like this: if (ret || !prog_id) { if (ret) { VLOG_ERR("Get XDP prog ID failed (%s)", ovs_strerror(errno)); } else { VLOG_ERR("No XDP program is loaded at ifindex %d", ifindex); } ... ? There is less code duplication this way. Best regards, Ilya Maximets.
On Wed, Jul 24, 2019 at 05:14:14PM +0300, Ilya Maximets wrote: > On 24.07.2019 1:58, William Tu wrote: > > netdev-afxdp requires XDP program to be loaded. When prog_id == 0, > > it indicates no XDP program, so return error and free resources. > > > > Signed-off-by: William Tu <u9012063@gmail.com> > > --- > > lib/netdev-afxdp.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c > > index 6b0b93e7f432..79c7a442e9cf 100644 > > --- a/lib/netdev-afxdp.c > > +++ b/lib/netdev-afxdp.c > > @@ -283,6 +283,12 @@ xsk_configure_socket(struct xsk_umem_info *umem, uint32_t ifindex, > > free(xsk); > > return NULL; > > } > > + if (!prog_id) { > > + VLOG_ERR("No XDP program is loaded at ifindex %d", ifindex); > > + xsk_socket__delete(xsk->xsk); > > + free(xsk); > > + return NULL; > > + } > > > What do you think about combining this 'if' with the previous error checking > like this: > > if (ret || !prog_id) { > if (ret) { > VLOG_ERR("Get XDP prog ID failed (%s)", ovs_strerror(errno)); > } else { > VLOG_ERR("No XDP program is loaded at ifindex %d", ifindex); > } > ... > > ? > There is less code duplication this way. > > Best regards, Ilya Maximets. Thanks, I will do that in next version. William
diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c index 6b0b93e7f432..79c7a442e9cf 100644 --- a/lib/netdev-afxdp.c +++ b/lib/netdev-afxdp.c @@ -283,6 +283,12 @@ xsk_configure_socket(struct xsk_umem_info *umem, uint32_t ifindex, free(xsk); return NULL; } + if (!prog_id) { + VLOG_ERR("No XDP program is loaded at ifindex %d", ifindex); + xsk_socket__delete(xsk->xsk); + free(xsk); + return NULL; + } while (!xsk_ring_prod__reserve(&xsk->umem->fq, PROD_NUM_DESCS, &idx)) {
netdev-afxdp requires XDP program to be loaded. When prog_id == 0, it indicates no XDP program, so return error and free resources. Signed-off-by: William Tu <u9012063@gmail.com> --- lib/netdev-afxdp.c | 6 ++++++ 1 file changed, 6 insertions(+)