On 9/29/22 18:40, Mike Pattrick wrote:
> On Thu, Sep 1, 2022 at 11:43 AM Ilya Maximets <i.maxim...@ovn.org> wrote:
>>
>> When OVS fails to find an OpenFlow port for a packet received
>> from the upcall it just prints the warning like this:
>>
>> |INFO|received packet on unassociated datapath port N
>>
>> However, during the flow translation more information is available
>> as if the recirculation id wasn't found or it was a packet from
>> unknown tunnel port. Printing that information might be useful
>> to understand the origin of the problem.
>>
>> Port translation functions already support extended error strings,
>> we just need to pass a variable where to store them.
>>
>> With the change the output may be:
>>
>> |INFO|received packet on unassociated datapath port N
>> (no OpenFlow port for datapath port N)
>> or
>> |INFO|received packet on unassociated datapath port N
>> (no OpenFlow tunnel port for this packet)
>> or
>> |INFO|received packet on unassociated datapath port N
>> (no recirculation data for recirc_id M)
>>
>> Unfortunately, there is no good way to trigger this code from
>> current unit tests.
>>
>> Signed-off-by: Ilya Maximets <i.maxim...@ovn.org>
>> ---
>> ofproto/ofproto-dpif-upcall.c | 27 +++++++++++++++++++--------
>> ofproto/ofproto-dpif-xlate.c | 6 ++++--
>> ofproto/ofproto-dpif-xlate.h | 2 +-
>> 3 files changed, 24 insertions(+), 11 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index 57f94df54..58671a8aa 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -398,7 +398,8 @@ static int upcall_receive(struct upcall *, const struct
>> dpif_backer *,
>> const struct dp_packet *packet, enum
>> dpif_upcall_type,
>> const struct nlattr *userdata, const struct flow
>> *,
>> const unsigned int mru,
>> - const ovs_u128 *ufid, const unsigned pmd_id);
>> + const ovs_u128 *ufid, const unsigned pmd_id,
>> + char **errorp);
>> static void upcall_uninit(struct upcall *);
>>
>> static void udpif_flow_rebalance(struct udpif *udpif);
>> @@ -819,6 +820,7 @@ recv_upcalls(struct handler *handler)
>> struct upcall *upcall = &upcalls[n_upcalls];
>> struct flow *flow = &flows[n_upcalls];
>> unsigned int mru = 0;
>> + char *errorp = NULL;
>> uint64_t hash = 0;
>> int error;
>>
>> @@ -845,7 +847,7 @@ recv_upcalls(struct handler *handler)
>>
>> error = upcall_receive(upcall, udpif->backer, &dupcall->packet,
>> dupcall->type, dupcall->userdata, flow, mru,
>> - &dupcall->ufid, PMD_ID_NULL);
>> + &dupcall->ufid, PMD_ID_NULL, &errorp);
>> if (error) {
>> if (error == ENODEV) {
>> /* Received packet on datapath port for which we couldn't
>> @@ -856,8 +858,11 @@ recv_upcalls(struct handler *handler)
>> dupcall->key_len, NULL, 0, NULL, 0,
>> &dupcall->ufid, PMD_ID_NULL, NULL);
>> VLOG_INFO_RL(&rl, "received packet on unassociated datapath
>> "
>> - "port %"PRIu32, flow->in_port.odp_port);
>> + "port %"PRIu32"%s%s%s", flow->in_port.odp_port,
>> + errorp ? " (" : "", errorp ? errorp : "",
>> + errorp ? ")" : "");
>
> Wouldn't it just be easier to wrap the whole VLOG_INFO_RL() in an if
> statement than have this piecewise string format construction?
>
> I guess that would add a bunch of extra lines, so either is fine.
I kept it as-is for now.
>
> Acked-by: Mike Pattrick <m...@redhat.com>
Thanks! Applied.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
