On Mon, Jan 14, 2019 at 3:55 AM David Marchand <[email protected]> wrote:
> On Fri, Jan 11, 2019 at 8:05 PM Darrell Ball <[email protected]> wrote: > >> On Thu, Jan 10, 2019 at 12:58 PM Darrell Ball <[email protected]> wrote: >> >>> On Thu, Jan 10, 2019 at 1:03 AM David Marchand < >>> [email protected]> wrote: >>> >>>> Hello, >>>> >>>> Just wondering, can't we rely integer promotion then truncation on >>>> 32bits ? >>>> My test program tends to show it works. >>>> >>> >>> I believe a compiler is free to convert the operands to int64 and AFAIK >>> overflow/underflow is undefined for signed variables. >>> >> >> nevermind; it appears we can trust the compiler in this case and simplify. >> > > Ok, I had missed the int64_t for seq_skew. > > More than trusting the compiler, I understand we only want to deal with > 32bits integers, since, at the end of the game, we want any offset bigger > than this to wrap on the 32bits range. > If we change it to int32_t, we are back with only 32bits integers: signed > for the seq_skew variable, and unsigned for the seq/ack values => addition > and substraction gives a uint32_t thanks to the "Usual arithmetic > conversions" ? > > what I meant by the last response is I don't think there is an issue either way Generally, I have a bias in not trusting the compiler where unnecessary, but in this case, there is a marginal benefit. I forgot to send my patch which will make it more clear, which you might rebase and fold in at the end of your series. > diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h > index a344801..c261f80 100644 > --- a/lib/conntrack-private.h > +++ b/lib/conntrack-private.h > @@ -99,7 +99,7 @@ struct conn { > /* XXX: consider flattening. */ > struct nat_action_info_t *nat_info; > char *alg; > - int seq_skew; > + int32_t seq_skew; > uint32_t mark; > uint8_t conn_type; > /* TCP sequence skew due to NATTing of FTP control messages. */ > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 6f6021a..206f5f1 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -756,7 +756,7 @@ conn_lookup(struct conntrack *ct, const struct > conn_key *key, long long now) > > static void > conn_seq_skew_set(struct conntrack *ct, const struct conn_key *key, > - long long now, int seq_skew, bool seq_skew_dir) > + long long now, int32_t seq_skew, bool seq_skew_dir) > { > unsigned bucket = hash_to_bucket(conn_key_hash(key, ct->hash_basis)); > ct_lock_lock(&ct->buckets[bucket].lock); > @@ -3192,7 +3192,7 @@ handle_ftp_ctl(struct conntrack *ct, const struct > conn_lookup_ctx *ctx, > } > > struct ovs_16aligned_ip6_hdr *nh6 = dp_packet_l3(pkt); > - int64_t seq_skew = 0; > + int32_t seq_skew = 0; > > if (ftp_ctl == CT_FTP_CTL_OTHER) { > seq_skew = conn_for_expectation->seq_skew; > @@ -3251,31 +3251,13 @@ handle_ftp_ctl(struct conntrack *ct, const struct > conn_lookup_ctx *ctx, > > if (do_seq_skew_adj && seq_skew != 0) { > if (ctx->reply != conn_for_expectation->seq_skew_dir) { > - > uint32_t tcp_ack = ntohl(get_16aligned_be32(&th->tcp_ack)); > > - if ((seq_skew > 0) && (tcp_ack < seq_skew)) { > - /* Should not be possible; will be marked invalid. */ > - tcp_ack = 0; > - } else if ((seq_skew < 0) && (UINT32_MAX - tcp_ack < > -seq_skew)) { > - tcp_ack = (-seq_skew) - (UINT32_MAX - tcp_ack); > - } else { > - tcp_ack -= seq_skew; > - } > - ovs_be32 new_tcp_ack = htonl(tcp_ack); > - put_16aligned_be32(&th->tcp_ack, new_tcp_ack); > + put_16aligned_be32(&th->tcp_ack, htonl(tcp_ack - seq_skew)); > } else { > uint32_t tcp_seq = ntohl(get_16aligned_be32(&th->tcp_seq)); > - if ((seq_skew > 0) && (UINT32_MAX - tcp_seq < seq_skew)) { > - tcp_seq = seq_skew - (UINT32_MAX - tcp_seq); > - } else if ((seq_skew < 0) && (tcp_seq < -seq_skew)) { > - /* Should not be possible; will be marked invalid. */ > - tcp_seq = 0; > - } else { > - tcp_seq += seq_skew; > - } > - ovs_be32 new_tcp_seq = htonl(tcp_seq); > - put_16aligned_be32(&th->tcp_seq, new_tcp_seq); > + > + put_16aligned_be32(&th->tcp_seq, htonl(tcp_seq + seq_skew)); > } > } > > What do you think ? > Do you want me to send a patch ? (I could take it as part of my other > fixes) > > > -- > David Marchand > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
