On 12/6/22 07:29, Ales Musil wrote:
> 
> 
> On Mon, Dec 5, 2022 at 10:38 PM Ilya Maximets <[email protected] 
> <mailto:[email protected]>> wrote:
> 
>     On 12/1/22 14:19, Ales Musil wrote:
>     >     +
>     >     +static enum ofperr
>     >     +handle_nxt_ct_flush(struct ofconn *ofconn, const struct ofp_header 
> *oh)
>     >     +{
>     >     +    struct ofproto *ofproto = ofconn_get_ofproto(ofconn);
>     >     +    struct ofputil_ct_match match = {0};
>     >     +    uint16_t zone_id;
>     >     +
>     >     +    ofputil_ct_match_decode(&match, &zone_id, oh);
>     >     +
>     >     +    if (ofproto->ofproto_class->ct_flush) {
>     >     +        ofproto->ofproto_class->ct_flush(ofproto, &zone_id, 
> &match);
>     >
>     >
>     > I've realized that v1 doesn't allow zone_id being NULL. However I'm not 
> sure how to put that information into the extension struct.
>     > I'm open to any suggestion, I was thinking about flags field, which 
> would grow the whole struct by 4 bytes.
> 
> 
>     IIUC, you're talking about OpenFlow interface that you created
>     requiring zone_id to be provided, right?
> 
> 
> Yes.
>  
> 
> 
>     Optional arguments and multiple arguments with mixed order are
>     usually solved by using TLVs.  You may shrink down the
>     'struct nx_ct_flush' structure to only mandatory elements and
>     make all the 'match' fields including zone_id as TLVs with the
>     help of include/openvswitch/ofp-prop.h.
> 
> 
> That sounds good. I'll look into it, thanks.
>  
> 
> 
>     One more thing: I don't think we should add dpctl interface for
>     the flushing.  We don't have such interface for ct-flush-zone
>     so we should not have it for ct-flush.  Instead, what is missing
>     in your implementation, is the native OpenFlow interface via
>     ovs-ofctl, i.e. 'ovs-ofctl ct-flush' command.  And all the tests
>     should use it instead.  This way we will also have test coverage
>     for the code that will actually be used by OVN/CMS.
> 
> 
> The problem is that the dpctl interface actually already exists, and we 
> shouldn't
> just remove it, this wouldn't make sense IMO. The first patch just extends 
> that interface.

Hmm, OK.

> Adding the ovs-ofctl makes, I'll work on it for v2.

Thanks.  And we'll need tests for ovs-ofctl part, of course.

>  
> 
> 
>     Best regards, Ilya Maximets.
> 
> 
> Thanks,
> Ales.
> 
> -- 
> 
> Ales Musil
> 
> Senior Software Engineer - OVN Core
> 
> Red Hat EMEA <https://www.redhat.com>
> 
> [email protected] <mailto:[email protected]>    IM: amusil
> 
> <https://red.ht/sig>
> 

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to