Add rdr rules

This PR adds support for redirect rules. I'll leave comments with explanation directly in source code.

This change is

Review status: 0 of 8 files reviewed at latest revision, 1 unresolved discussion.

src/pooladdr.rs, line 20 at r1 (raw file):

#[derive(Debug)]
pub struct PoolAddrList {
    ips: Vec<Ip>,

The only reason I keep it is for Hash implementation

Comments from Reviewable

Review status: 0 of 8 files reviewed at latest revision, 1 unresolved discussion.

src/pooladdr.rs, line 20 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
The only reason I keep it is for Hash implementation

Oh and also for Clone

Comments from Reviewable

Review status: 0 of 8 files reviewed at latest revision, 1 unresolved discussion.

src/pooladdr.rs, line 20 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
Oh and also for Clone

hmm.. Actually every single trait here depends on that value

Comments from Reviewable

Review status: 0 of 8 files reviewed at latest revision, 2 unresolved discussions.

src/pooladdr.rs, line 81 at r1 (raw file):

impl PartialEq for PoolAddrList {
    fn eq(&self, other: &PoolAddrList) -> bool {
        self.ips == other.ips

Deceptive way to implement PartialEq, but it should mostly work if we don't mess around with underlying low-level structure, which we don't want to do anyway.

Comments from Reviewable

Review status: 0 of 8 files reviewed at latest revision, 4 unresolved discussions.

src/pooladdr.rs, line 95 at r1 (raw file):

impl Clone for PoolAddrList {
    fn clone(&self) -> Self {
        PoolAddrList::new(&self.ips)

Some of PF structures used in PoolAddrList do not implement Clone

src/rule/mod.rs, line 200 at r1 (raw file):

fn compatible_af(af1: AddrFamily, af2: AddrFamily) -> Result<AddrFamily> {

I moved it from FilterRule to here so we can share the implementation in RedirectRule too

Comments from Reviewable

Review status: 0 of 8 files reviewed at latest revision, 5 unresolved discussions.

src/rule/redirect_endpoint.rs, line 17 at r1 (raw file):

#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub struct RedirectEndpoint {

Only holds PoolAddrList and provides convenient traits to fill in certain pf_ structures.

Comments from Reviewable

This PR only adds ability to add single RDR rule into anchor. Transaction should be generalized in separate PR since it will be a big change and allow adding FilterRules and RedirectRules within single Transaction.

Review status: 0 of 8 files reviewed at latest revision, 5 unresolved discussions.

Comments from Reviewable

Review status: 0 of 8 files reviewed at latest revision, 6 unresolved discussions.

src/rule/mod.rs, line 195 at r1 (raw file):

impl CopyTo<ffi::pfvar::pf_pooladdr> for RedirectRule {
    fn copy_to(&self, pf_pooladdr: &mut ffi::pfvar::pf_pooladdr) {
        self.redirect_to.copy_to(pf_pooladdr);

This is a little unusual situation: since redirect_to is not public on we cannot access it directly from the outside, but we need it in order to register redirect address. This is necessary before we can add the rule.

Comments from Reviewable

Review status: 0 of 8 files reviewed at latest revision, 7 unresolved discussions.

src/rule/mod.rs, line 169 at r1 (raw file):

    fn get_af(&self) -> Result<AddrFamily> {
        let endpoint_af = compatible_af(self.from.get_af(), self.to.get_af())?;
        let rdr_af = compatible_af(endpoint_af, self.redirect_to.get_af())?;

I may be wrong but I don't think it's possible to redirect from IPv4 to IPv6, not at least until we use 6-to-4 NAT (PF_NAT64). So that's why I verify all involved address families.

Comments from Reviewable

Review status: 0 of 8 files reviewed at latest revision, 7 unresolved discussions.

src/rule/mod.rs, line 195 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
This is a little unusual situation: since redirect_to is not public on we cannot access it directly from the outside, but we need it in order to register redirect address. This is necessary before we can add the rule.

This call delegates work to CopyTo<ffi::pfvar::pf_pooladdr> for RedirectEndpoint implementation.

Comments from Reviewable

Review status: 0 of 8 files reviewed at latest revision, 7 unresolved discussions.

src/rule/mod.rs, line 195 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
This call delegates work to CopyTo<ffi::pfvar::pf_pooladdr> for RedirectEndpoint implementation.

We could avoid this trait if we were able to call RedirectRule.redirect_to.copy_to(...) directly. But we cannot since all fields on rules are private.

Comments from Reviewable

Review status: 0 of 8 files reviewed at latest revision, 8 unresolved discussions.

src/lib.rs, line 303 at r1 (raw file):

        let mut pfioc_pooladdr = unsafe { mem::zeroed::<ffi::pfvar::pfioc_pooladdr>() };
        ioctl_guard!(ffi::pf_begin_addrs(self.fd(), &mut pfioc_pooladdr))?;
        rule.copy_to(&mut pfioc_pooladdr.addr);

In the ideal world this should be:
rule.redirect_to.copy_to(&mut pfioc_pooladdr.addr);

Comments from Reviewable

Review status: 0 of 8 files reviewed at latest revision, 9 unresolved discussions.

src/ffi.rs, line 57 at r1 (raw file):

ioctl!(readwrite pf_begin_addrs with b'D', 51; pfvar::pfioc_pooladdr);
// DIOCADDADDR
ioctl!(readwrite pf_add_addr with b'D', 52; pfvar::pfioc_pooladdr);

Could you explain what this call actually does? I have a hard time understanding from only its name and the one usage it has.

src/lib.rs, line 303 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
In the ideal world this should be:
rule.redirect_to.copy_to(&mut pfioc_pooladdr.addr);

Could you explain why we can't have the ideal solution then?

Comments from Reviewable

Review status: 0 of 8 files reviewed at latest revision, 12 unresolved discussions.

src/rule/rule_action.rs, line 15 at r1 (raw file):

pub enum RuleAction {
    Pass,
    Drop,

Rename this enum to FilterRuleAction now maybe? Would make it more clear.

src/rule/rule_action.rs, line 32 at r1 (raw file):

pub enum RedirectRuleAction {
    Rdr,
    NoRdr,

I understand this is all part of PF and you created the enum out of what PF does. But could you explain why one would have a redirection rule that does not redirect (NoRdr)? That sounds a bit pointless to me, but I'm probably missing some detail :)
Also. I think it would be very nice if we could cut down on abberviations where possible. We should create a more user friendly API than PF has :) Could we call these variants Redirect and NoRedirect?

tests/redirect_rules.rs, line 21 at r1 (raw file):

fn after_each() {
    pfcli::flush_rules(ANCHOR_NAME, pfcli::FlushOptions::Nat).unwrap();

Maybe we want to remove the anchor as well? Good if the tests can clean up everything they have created.

Comments from Reviewable

Review status: 0 of 8 files reviewed at latest revision, 14 unresolved discussions.

src/pooladdr.rs, line 19 at r1 (raw file):

#[derive(Debug)]
pub struct PoolAddrList {

Could you maybe add a doc comment to this type? Partly because it's public, so it should be documented at some point anyway. But mainly because I don't understand exactly its usage. It would be good if the documentation described the following things:

What does the type represent on a conceptual level
What parts of this crate use it and for doing what.

src/pooladdr.rs, line 32 at r1 (raw file):

        PoolAddrList {
            ips: ips.to_vec(),

If this PoolAddrList should truly contain this extra field (that is a different discussion). Then it should probably take it by ownership Vec<Ip> instead of &[Ip] in this constructor. No meaning to borrow something just to allocate an owned instance. Then it's better to signal in the signature of the constructor that "hey, I need to own this list". Unless that has serious negative consequences to the ergonomics of using it, I have not studied that yet.

src/rule/redirect_endpoint.rs, line 17 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
Only holds PoolAddrList and provides convenient traits to fill in certain pf_ structures.

It does not seems that it actually have to hold a PoolAddrList inside it. The trait impls (CopyTo/TryCopyTo) on RedirectEndpoint could just locally create such PoolAddrList for the sake of filling in the FFI values it needs to fill in.
And thus this must not even be it's own type probably, since then it would only contain a Endpoint. But maybe if the CopyTo/TryCopyTo impls overlap in types between Endpoint and RedirectEndpoint but the implementations themselves are different, then a special type might be needed. But then the "newtype pattern" would be enough:
pub struct RedirectEndpoint(Endpoint);

It would be really very much nicer if both types of rules could share the same Endpoint type.

Comments from Reviewable

Review status: 0 of 8 files reviewed at latest revision, 14 unresolved discussions.

src/pooladdr.rs, line 20 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
hmm.. Actually every single trait here depends on that value

If you do manage to get rid of the PoolAddrList from RedirectEndpoint, as I suggested in the other thread. Then that would have the very nice effect of not having to implement Clone, Hash or Eq for this type, and thus this field could go, along with the impls below.

Comments from Reviewable

Review status: 0 of 8 files reviewed at latest revision, 14 unresolved discussions.

src/rule/mod.rs, line 169 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
I may be wrong but I don't think it's possible to redirect from IPv4 to IPv6, not at least until we use 6-to-4 NAT (PF_NAT64). So that's why I verify all involved address families.

Ignoring 6-to-4 sounds like a very reasonable limitation. And if we do have such a limitation then we should really make sure we don't allow that combination as well. So good you check that :+1:

Comments from Reviewable

Review status: 0 of 8 files reviewed at latest revision, 13 unresolved discussions.

src/ffi.rs, line 57 at r1 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Could you explain what this call actually does? I have a hard time understanding from only its name and the one usage it has.

In order to add rule we need to obtain two tickets:

Rule ticket – obtained via DIOCXBEGIN or we also do that via DIOCCHANGERULE with PF_CHANGE_GET_TICKET flag.
Pool ticket – obtained via call to  DIOCBEGINADDRS

DIOCBEGINADDRS allocates an address pool in PF and returns pool ticket for it. DIOCADDADDR allows to add address in the pool.
The sequence of calls usually looks as following:

DIOCBEGINADDRS - allocates an address pool in PF and returns pool_ticket for it.
DIOCADDADDR- adds address into the address pool using pool_ticket obtained in previous call. Filter rules do not need any address registration, but for redirect rules we have to register address that we will use to redirect traffic to.
DIOCADDRULE or DIOCCHANGERULE - adds rule using rule ticket obtained in traditional way and pool_ticket obtained from DIOCBEGINADDRS.

Comments from Reviewable

Review status: 0 of 8 files reviewed at latest revision, 13 unresolved discussions.

src/rule/mod.rs, line 200 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
I moved it from FilterRule to here so we can share the implementation in RedirectRule too

As long as there are not multiple definitions of what constitutes "compatible address families" it makes very much sense to have this as a global helper.

Comments from Reviewable

Review status: 0 of 8 files reviewed at latest revision, 13 unresolved discussions.

src/ffi.rs, line 57 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
In order to add rule we need to obtain two tickets:

Rule ticket – obtained via DIOCXBEGIN or we also do that via DIOCCHANGERULE with PF_CHANGE_GET_TICKET flag.
Pool ticket – obtained via call to  DIOCBEGINADDRS

DIOCBEGINADDRS allocates an address pool in PF and returns pool ticket for it. DIOCADDADDR allows to add address in the pool.
The sequence of calls usually looks as following:

DIOCBEGINADDRS - allocates an address pool in PF and returns pool_ticket for it.
DIOCADDADDR- adds address into the address pool using pool_ticket obtained in previous call. Filter rules do not need any address registration, but for redirect rules we have to register address that we will use to redirect traffic to.
DIOCADDRULE or DIOCCHANGERULE - adds rule using rule ticket obtained in traditional way and pool_ticket obtained from DIOCBEGINADDRS.

To be fair I don't know why that specifically works that way and why PF cannot derive the addresses from pf_rule.rpool which we fill in with redirect port. That's just bad design in my opinion.

Comments from Reviewable

Review status: 0 of 8 files reviewed at latest revision, 13 unresolved discussions.

src/ffi.rs, line 57 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
To be fair I don't know why that specifically works that way and why PF cannot derive the addresses from pf_rule.rpool which we fill in with redirect port. That's just bad design in my opinion.

"which we fill in with redirect port" = which we fill in with redirect address AND port

Comments from Reviewable

Review status: 0 of 8 files reviewed at latest revision, 13 unresolved discussions.

src/ffi.rs, line 57 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
"which we fill in with redirect port" = which we fill in with redirect address AND port

Ah. Good explanation! Now some things are clearer. I think some of the information you wrote here would be great to have in the code as well. This crate is becoming increasingly more complex.
Could you maybe add rustdoc to the DIOCBEGINADDR and DIOCADDADDR saying kind of what you just wrote above: "Allocates/Creates an address pool in PF and returns the pool ticket for it" and "Add an address (or can there be multiple?) to an address pool created with pf_begin_addrs.

Comments from Reviewable

Review status: 0 of 8 files reviewed at latest revision, 13 unresolved discussions.

src/ffi.rs, line 57 at r1 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Ah. Good explanation! Now some things are clearer. I think some of the information you wrote here would be great to have in the code as well. This crate is becoming increasingly more complex.
Could you maybe add rustdoc to the DIOCBEGINADDR and DIOCADDADDR saying kind of what you just wrote above: "Allocates/Creates an address pool in PF and returns the pool ticket for it" and "Add an address (or can there be multiple?) to an address pool created with pf_begin_addrs.

This information is available online, DIOC* calls are documented very well in MAN pages for PF, for example: http://man.openbsd.org/OpenBSD-3.9/pf.4

src/lib.rs, line 303 at r1 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Could you explain why we can't have the ideal solution then?

rule.redirect_to is private as all fields in our Rules

Comments from Reviewable

Review status: 0 of 8 files reviewed at latest revision, 13 unresolved discussions.

src/lib.rs, line 303 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
rule.redirect_to is private as all fields in our Rules

But I didn't feel like opening Rule fields to the public. First of all I enjoy immutable nature of FilterRule/RedirectRule. Once created with builder it remains sealed.

Comments from Reviewable

Review status: 0 of 8 files reviewed at latest revision, 13 unresolved discussions.

src/rule/mod.rs, line 200 at r1 (raw file):
Previously, faern (Linus Färnstrand) wrote…
As long as there are not multiple definitions of what constitutes "compatible address families" it makes very much sense to have this as a global helper.

So far so good I guess.

Comments from Reviewable

Review status: 0 of 8 files reviewed at latest revision, 13 unresolved discussions.

src/lib.rs, line 303 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
But I didn't feel like opening Rule fields to the public. First of all I enjoy immutable nature of FilterRule/RedirectRule. Once created with builder it remains sealed.

Hmm.. One could expose redirect_to via an immutable getter. That is one way to do it.
It feels strange to "copy the rule to this address". I think a getter would be cleaner?

Comments from Reviewable

Review status: 0 of 8 files reviewed at latest revision, 12 unresolved discussions.

src/rule/redirect_endpoint.rs, line 17 at r1 (raw file):
Previously, faern (Linus Färnstrand) wrote…
It does not seems that it actually have to hold a PoolAddrList inside it. The trait impls (CopyTo/TryCopyTo) on RedirectEndpoint could just locally create such PoolAddrList for the sake of filling in the FFI values it needs to fill in.
And thus this must not even be it's own type probably, since then it would only contain a Endpoint. But maybe if the CopyTo/TryCopyTo impls overlap in types between Endpoint and RedirectEndpoint but the implementations themselves are different, then a special type might be needed. But then the "newtype pattern" would be enough:
pub struct RedirectEndpoint(Endpoint);

It would be really very much nicer if both types of rules could share the same Endpoint type.

PoolAddrList creates a linked list with raw pointers and boxed pf_pooladdr in it to ensure that pointers are valid for the duration of its lifetime. How would you make sure that addresses in linked list returned from PoolAddrList are correct if you don't retain it?

Comments from Reviewable

Review status: 0 of 11 files reviewed at latest revision, 12 unresolved discussions.

src/rule/rule_action.rs, line 15 at r1 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Rename this enum to FilterRuleAction now maybe? Would make it more clear.

Done.

src/rule/rule_action.rs, line 32 at r1 (raw file):
Previously, faern (Linus Färnstrand) wrote…
I understand this is all part of PF and you created the enum out of what PF does. But could you explain why one would have a redirection rule that does not redirect (NoRdr)? That sounds a bit pointless to me, but I'm probably missing some detail :)
Also. I think it would be very nice if we could cut down on abberviations where possible. We should create a more user friendly API than PF has :) Could we call these variants Redirect and NoRedirect?

RedirectRuleAction::Redirect seemed like tautology but I didn't have better idea that's why I left Rdr and NoRdr.  Let me think about it.
Each action in PF has negated co-action, i.e pass and drop, redirect and not-redirect.
I think the point is to allow mass redirect for certain port range using PF_RDR allowing to exclude certain ports with PF_NORDR. For example I want to redirect all ports 5000-9999 to host <IP_ADDRESS> except port 6667 because the software I use has vulnerabilities that allow for remote code execution, so I temporarily block that redirect.

Comments from Reviewable

Review status: 0 of 11 files reviewed at latest revision, 12 unresolved discussions.

src/pooladdr.rs, line 19 at r1 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Could you maybe add a doc comment to this type? Partly because it's public, so it should be documented at some point anyway. But mainly because I don't understand exactly its usage. It would be good if the documentation described the following things:

What does the type represent on a conceptual level
What parts of this crate use it and for doing what.

Now you kind of described it in another thread here. But this crate could use some more documentation overall.

src/rule/redirect_endpoint.rs, line 17 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
PoolAddrList creates a linked list with raw pointers and boxed pf_pooladdr in it to ensure that pointers are valid for the duration of its lifetime. How would you make sure that addresses in linked list returned from PoolAddrList are correct if you don't retain it?

Wow. All of this is so dangerous. I wish we can find a safer way to manage these ffi objects, so it's not so easy to do these mistakes :/
Our CopyTo and TryCopyTo traits don't declare that "If you copy X into ffi instance Y, then you should only consider Y valid for as long as X exists". It's very hard to know which copy_to operations that make the result dependent on the source data and which are standalone.
Would be very good if we could find a design that expressed this in an enforced way.

Comments from Reviewable

Review status: 0 of 11 files reviewed at latest revision, 12 unresolved discussions.

src/rule/redirect_endpoint.rs, line 17 at r1 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Wow. All of this is so dangerous. I wish we can find a safer way to manage these ffi objects, so it's not so easy to do these mistakes :/
Our CopyTo and TryCopyTo traits don't declare that "If you copy X into ffi instance Y, then you should only consider Y valid for as long as X exists". It's very hard to know which copy_to operations that make the result dependent on the source data and which are standalone.
Would be very good if we could find a design that expressed this in an enforced way.

There is a reason why PoolAddrList exists and keeps boxed slice of addresses.
That's why RedirectEndpoint exists to make sure it's hard to make that mistake.
I am thinking of somehow enforced lifetime but then FilterRule and RedirectRule will both have to adopt lifetime parameters which can be annoying because we'll have to put <'a> in every single impl and trait implemented for it.
Also, it's expected that dropping FilterRule or RedirectRule draws all FFI structs useless.

Comments from Reviewable

Review status: 0 of 11 files reviewed at latest revision, 12 unresolved discussions.

src/pooladdr.rs, line 20 at r1 (raw file):
Previously, faern (Linus Färnstrand) wrote…
If you do manage to get rid of the PoolAddrList from RedirectEndpoint, as I suggested in the other thread. Then that would have the very nice effect of not having to implement Clone, Hash or Eq for this type, and thus this field could go, along with the impls below.

I agree but not that easy.

Comments from Reviewable

Review status: 0 of 11 files reviewed at latest revision, 11 unresolved discussions.

src/pooladdr.rs, line 32 at r1 (raw file):
Previously, faern (Linus Färnstrand) wrote…
If this PoolAddrList should truly contain this extra field (that is a different discussion). Then it should probably take it by ownership Vec<Ip> instead of &[Ip] in this constructor. No meaning to borrow something just to allocate an owned instance. Then it's better to signal in the signature of the constructor that "hey, I need to own this list". Unless that has serious negative consequences to the ergonomics of using it, I have not studied that yet.

It only needs this for Hash and Clone. Introducing lifetime boundaries will bubble up all the way to RedirectRule.

Comments from Reviewable

Review status: 0 of 11 files reviewed at latest revision, 11 unresolved discussions.

src/rule/redirect_endpoint.rs, line 17 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
There is a reason why PoolAddrList exists and keeps boxed slice of addresses.
That's why RedirectEndpoint exists to make sure it's hard to make that mistake.
I am thinking of somehow enforced lifetime but then FilterRule and RedirectRule will both have to adopt lifetime parameters which can be annoying because we'll have to put <'a> in every single impl and trait implemented for it.
Also, it's expected that dropping FilterRule or RedirectRule draws all FFI structs useless.

No I think FilterRule and RedirectRule should be kept completely separated from FFI. They should only be safe Rust.
Yeah, the idea of keeping stuff that depend on each other with unsafe pointers kept under the same object, to force them to be valid for the same time, is a good idea. Sadly we don't utilize that fully. Obviously we here allow copying unsafe pointers into ffi structs that then depend on the original object to be retained for long enough. Better if any such copy could only produce results valid for the correct time. Either if that can be enforced with lifetimes in some way. Or if we create new objects holding all the data and forcing them to be in memory for the same time.

src/rule/rule_action.rs, line 32 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
RedirectRuleAction::Redirect seemed like tautology but I didn't have better idea that's why I left Rdr and NoRdr.  Let me think about it.
Each action in PF has negated co-action, i.e pass and drop, redirect and not-redirect.
I think the point is to allow mass redirect for certain port range using PF_RDR allowing to exclude certain ports with PF_NORDR. For example I want to redirect all ports 5000-9999 to host <IP_ADDRESS> except port 6667 because the software I use has vulnerabilities that allow for remote code execution, so I temporarily block that redirect.

Having Rdr instead of Redirect is no less of a tautology. You just trick the brain into thinking that. Just because it's an abbreviation does not mean it's something else. In it's full form it still says RedirectRuleAction::Redirect just slightly harder to read.
Good explanation of PF_NORDR, thanks!

Comments from Reviewable

Reviewed 1 of 9 files at r2.
Review status: 1 of 11 files reviewed at latest revision, 11 unresolved discussions.

src/ffi.rs, line 57 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
This information is available online, DIOC* calls are documented very well in MAN pages for PF, for example: http://man.openbsd.org/OpenBSD-3.9/pf.4

Ok. If you don't want to duplicate the documentation here, which makes sense, then I think we should link to that documentation from here. Before all these ioctl definitions there can be a comment saying what they are and how they work is defined at

Comments from Reviewable

Review status: 1 of 11 files reviewed at latest revision, 10 unresolved discussions.

src/pooladdr.rs, line 32 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
It only needs this for Hash and Clone. Introducing lifetime boundaries will bubble up all the way to RedirectRule.

Huh? You don't need to introduce lifetime boundaries. Take the value by ownership instead of borrowing it:
pub fn new(ips: Vec<Ip>) -> Self {
   ...
}

To signal to the user/reader that we need to own our own IPs.

Comments from Reviewable

Review status: 1 of 11 files reviewed at latest revision, 10 unresolved discussions.

src/rule/rule_action.rs, line 32 at r1 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Having Rdr instead of Redirect is no less of a tautology. You just trick the brain into thinking that. Just because it's an abbreviation does not mean it's something else. In it's full form it still says RedirectRuleAction::Redirect just slightly harder to read.
Good explanation of PF_NORDR, thanks!

In a way Rdr = Pass, NoRdr = Drop :)

Comments from Reviewable

Review status: 1 of 11 files reviewed at latest revision, 11 unresolved discussions.

src/ffi.rs, line 57 at r1 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Ok. If you don't want to duplicate the documentation here, which makes sense, then I think we should link to that documentation from here. Before all these ioctl definitions there can be a comment saying what they are and how they work is defined at

OK Added link to latest docs
http://man.openbsd.org/pf.4
I don't think I explain better than official docs. Sadly Apple does not seem to have the same documentation for their own fork.

src/rule/rule_action.rs, line 32 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
In a way Rdr = Pass, NoRdr = Drop :)

Done.

tests/redirect_rules.rs, line 21 at r1 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Maybe we want to remove the anchor as well? Good if the tests can clean up everything they have created.

Done.

Comments from Reviewable

Review status: 1 of 11 files reviewed at latest revision, 10 unresolved discussions.

src/pooladdr.rs, line 19 at r1 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Now you kind of described it in another thread here. But this crate could use some more documentation overall.

Added few notes about PoolAddrList

Comments from Reviewable

Review status: 1 of 11 files reviewed at latest revision, 10 unresolved discussions.

src/pooladdr.rs, line 32 at r1 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Huh? You don't need to introduce lifetime boundaries. Take the value by ownership instead of borrowing it:
pub fn new(ips: Vec<Ip>) -> Self {
   ...
}

To signal to the user/reader that we need to own our own IPs.

Sorry I read that wrong, passing Vec instead should work yes!

Comments from Reviewable

Review status: 1 of 11 files reviewed at latest revision, 10 unresolved discussions.

src/lib.rs, line 303 at r1 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Hmm.. One could expose redirect_to via an immutable getter. That is one way to do it.
It feels strange to "copy the rule to this address". I think a getter would be cleaner?

I agree. Is it possible to expose immutable getter via builder options?

Comments from Reviewable

Review status: 1 of 11 files reviewed at latest revision, 10 unresolved discussions.

src/pooladdr.rs, line 32 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
Sorry I read that wrong, passing Vec instead should work yes!

Done.

Comments from Reviewable

Review status: 1 of 11 files reviewed at latest revision, 10 unresolved discussions.

src/lib.rs, line 303 at r1 (raw file):

redirect_to
Well I'll just add redirect_to() or something on struct RedirectRule

Comments from Reviewable

Review status: 1 of 11 files reviewed at latest revision, 10 unresolved discussions.

src/lib.rs, line 303 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…

redirect_to
Well I'll just add redirect_to() or something on struct RedirectRule

Added fn get_redirect_endpoint<'a>(&'a self) -> &'a RedirectEndpoint that allows to reference redirect_to from the outside, at the same time keeps it immutable.

src/pooladdr.rs, line 19 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
Added few notes about PoolAddrList

Done.

src/rule/mod.rs, line 195 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
We could avoid this trait if we were able to call RedirectRule.redirect_to.copy_to(...) directly. But we cannot since all fields on rules are private.

Done.

Comments from Reviewable

Review status: 1 of 11 files reviewed at latest revision, 10 unresolved discussions.

src/rule/redirect_endpoint.rs, line 17 at r1 (raw file):
Previously, faern (Linus Färnstrand) wrote…
No I think FilterRule and RedirectRule should be kept completely separated from FFI. They should only be safe Rust.
Yeah, the idea of keeping stuff that depend on each other with unsafe pointers kept under the same object, to force them to be valid for the same time, is a good idea. Sadly we don't utilize that fully. Obviously we here allow copying unsafe pointers into ffi structs that then depend on the original object to be retained for long enough. Better if any such copy could only produce results valid for the correct time. Either if that can be enforced with lifetimes in some way. Or if we create new objects holding all the data and forcing them to be in memory for the same time.

Fixing this would be tremendous work, most likely will pollute code with elision all over the place. Currently we are safe as long as we manipulate FFI structs within lib.rs. If they leak, Huston we have a problem.

Comments from Reviewable

Review status: 1 of 11 files reviewed at latest revision, 10 unresolved discussions.

src/rule/redirect_endpoint.rs, line 17 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
Fixing this would be tremendous work, most likely will pollute code with elision all over the place. Currently we are safe as long as we manipulate FFI structs within lib.rs. If they leak, Huston we have a problem.

I would be happy to get rid of RedirectEndpoint at once. It's wrapped Endpoint, we could have the same TryCopyTo<ffi::pfvar::pf_pool> implemented for Endpoint itself. The only problem to keep that PoolAddrList around. Maybe we could create it separately in lib.rs? A lot of boilerplate but less code in RedirectRule and less weird structs.

Comments from Reviewable

Reviewed 1 of 8 files at r3.
Review status: 5 of 11 files reviewed at latest revision, 9 unresolved discussions.

Comments from Reviewable

Review status: 5 of 11 files reviewed at latest revision, 9 unresolved discussions.

tests/redirect_rules.rs, line 31 at r2 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Rename required. But I guess the CI will catch that as well.

Rename of what?

Comments from Reviewable

Review status: 5 of 11 files reviewed at latest revision, 7 unresolved discussions.

src/rule/mod.rs, line 175 at r3 (raw file):

    /// Returns reference to inner redirect_to
    pub fn get_redirect_endpoint(&self) -> &RedirectEndpoint {
        &self.redirect_to

I think it's a bit confusing if the getter has a different name than the field. Now we have:
RedirectRuleBuilder::set_redirect_to()
RedirectRule::get_redirect_endpoint()

Which is a bit strange IMO. Getters usually have the same name as their corresponding setters.

Comments from Reviewable

Review status: 5 of 11 files reviewed at latest revision, 7 unresolved discussions.

src/rule/redirect_endpoint.rs, line 17 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
I would be happy to get rid of RedirectEndpoint at once. It's wrapped Endpoint, we could have the same TryCopyTo<ffi::pfvar::pf_pool> implemented for Endpoint itself. The only problem to keep that PoolAddrList around. Maybe we could create it separately in lib.rs? A lot of boilerplate but less code in RedirectRule and less weird structs.

I'm not sure if you left those last comments before or after our discussion on slack. But what about the solution where we introduce a new CopyTo trait that binds the lifetime of the target object to the source object, so the FFI version can't outlive anything it might point to?
That in itself will not get rid of RedirectEndpoint, but it will at least make the current solution memory safe.

Comments from Reviewable

Review status: 5 of 11 files reviewed at latest revision, 7 unresolved discussions.

tests/redirect_rules.rs, line 31 at r2 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
Rename of what?

Sorry. I guess Reviewable did not show me the latest version of the code. All is well! :+1:

Comments from Reviewable

Review status: 5 of 11 files reviewed at latest revision, 7 unresolved discussions.

tests/redirect_rules.rs, line 28 at r3 (raw file):

}

test!(flush_redirect_rules {

The two tests here are 99% identical. Would be nice if you could either:

Extract the common code to a helper function... or
Just test that the content of the rule is correct in the flush test and thus also verify the rule correctness at the same time.

Comments from Reviewable

Review status: 5 of 11 files reviewed at latest revision, 5 unresolved discussions.

src/lib.rs, line 303 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
Added fn get_redirect_endpoint<'a>(&'a self) -> &'a RedirectEndpoint that allows to reference redirect_to from the outside, at the same time keeps it immutable.

Not possible via the automatic builder struct no.

src/pooladdr.rs, line 19 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
Done.

You described on a very technical level. The actual representation I asked for is still missing. I'm not sure, but I guess that is something like "Represents a pool (table?) of addresses (Just IP? or entire endpoints?). Used in redirect rules to define where packets are redirected(????) and used for blabla.."
I want this description because I understand partly what it does. But a description might shed a lot of light on the parts I have missed. And it is surely completely unknown what it does for someone who did not work with PF before.

Comments from Reviewable

Review status: 5 of 11 files reviewed at latest revision, 5 unresolved discussions.

src/rule/mod.rs, line 175 at r3 (raw file):
Previously, faern (Linus Färnstrand) wrote…
I think it's a bit confusing if the getter has a different name than the field. Now we have:
RedirectRuleBuilder::set_redirect_to()
RedirectRule::get_redirect_endpoint()

Which is a bit strange IMO. Getters usually have the same name as their corresponding setters.

Done.

src/rule/redirect_endpoint.rs, line 17 at r1 (raw file):
Previously, faern (Linus Färnstrand) wrote…
I'm not sure if you left those last comments before or after our discussion on slack. But what about the solution where we introduce a new CopyTo trait that binds the lifetime of the target object to the source object, so the FFI version can't outlive anything it might point to?
That in itself will not get rid of RedirectEndpoint, but it will at least make the current solution memory safe.

For now I think less code => better. If two lines of code in lib.rs can solve the memory problem of PoolAddrListthen I'm willing to create it myself and assign it to pf_rule manually. At the same time that allows for a huge chunk of code to be removed. I'll show what I mean shortly...

Comments from Reviewable

Review status: 5 of 11 files reviewed at latest revision, 6 unresolved discussions.

src/rule/redirect_endpoint.rs, line 44 at r2 (raw file):

    fn try_copy_to(&self, pf_pool: &mut ffi::pfvar::pf_pool) -> Result<()> {
        pf_pool.list = self.pool.to_palist();
        pf_pool.af = self.endpoint.get_af().into();

Seems like rpool.af is automatically pre-filled by PF and we do not need to set up this value!
I tested adding tests with arbitrary pool.af and created rules were always the same.
Based on source code, that's the only reference of pool.af that I could find:
https://github.com/opensource-apple/xnu/blob/53c5e2e62fc4182595609153d4b99648da577c39/bsd/net/pf_ioctl.c#L3112
If you look at that line you'll see that pool.af is calculated from rule.af therefore we do not need to do anything with it.

Comments from Reviewable

Review status: 5 of 10 files reviewed at latest revision, 6 unresolved discussions.

tests/redirect_rules.rs, line 28 at r3 (raw file):
Previously, faern (Linus Färnstrand) wrote…
The two tests here are 99% identical. Would be nice if you could either:

Extract the common code to a helper function... or
Just test that the content of the rule is correct in the flush test and thus also verify the rule correctness at the same time.

Done.

Comments from Reviewable

Reviewed 1 of 5 files at r4.
Review status: 6 of 10 files reviewed at latest revision, 5 unresolved discussions.

Comments from Reviewable

Review status: 5 of 10 files reviewed at latest revision, 5 unresolved discussions.

src/pooladdr.rs, line 19 at r1 (raw file):
Previously, faern (Linus Färnstrand) wrote…
You described on a very technical level. The actual representation I asked for is still missing. I'm not sure, but I guess that is something like "Represents a pool (table?) of addresses (Just IP? or entire endpoints?). Used in redirect rules to define where packets are redirected(????) and used for blabla.."
I want this description because I understand partly what it does. But a description might shed a lot of light on the parts I have missed. And it is surely completely unknown what it does for someone who did not work with PF before.

Done.

Comments from Reviewable

Review status: 5 of 10 files reviewed at latest revision, 5 unresolved discussions.

src/rule/redirect_endpoint.rs, line 17 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
For now I think less code => better. If two lines of code in lib.rs can solve the memory problem of PoolAddrListthen I'm willing to create it myself and assign it to pf_rule manually. At the same time that allows for a huge chunk of code to be removed. I'll show what I mean shortly...

Done.

Comments from Reviewable

Review status: 5 of 10 files reviewed at latest revision, 5 unresolved discussions.

src/rule/redirect_endpoint.rs, line 44 at r2 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
Seems like rpool.af is automatically pre-filled by PF and we do not need to set up this value!
I tested adding tests with arbitrary pool.af and created rules were always the same.
Based on source code, that's the only reference of pool.af that I could find:
https://github.com/opensource-apple/xnu/blob/53c5e2e62fc4182595609153d4b99648da577c39/bsd/net/pf_ioctl.c#L3112
If you look at that line you'll see that pool.af is calculated from rule.af therefore we do not need to do anything with it.

Done.

Comments from Reviewable

Review status: 5 of 10 files reviewed at latest revision, 6 unresolved discussions.

src/lib.rs, line 304 at r5 (raw file):

        let mut pfioc_pooladdr = unsafe { mem::zeroed::<ffi::pfvar::pfioc_pooladdr>() };
        ioctl_guard!(ffi::pf_begin_addrs(self.fd(), &mut pfioc_pooladdr))?;
        redirect_to.0.copy_to(&mut pfioc_pooladdr.addr.addr);

We have trait to copy from Ip to pf_addr_wrap. But we don’t have a trait to do that for Endpoint, because it incorporates port too and pf_addr_wrap is purely Ipv4 or Ipv6 holder.
That’s primarily the reason why I do not rush to implement CopyTo.

Comments from Reviewable

Review status: 5 of 10 files reviewed at latest revision, 7 unresolved discussions.

src/lib.rs, line 313 at r5 (raw file):

        // copy address pool in pf_rule
        let redirect_pool = PoolAddrList::new(&[redirect_to.0]);

We could implement From<Endpoint> for PoolAddrList? I love that it's explicit though. But I don't see how to solve that otherwise. Maybe change endpoint to have two fields and problem solved?

Comments from Reviewable

Reviewed 1 of 1 files at r5.
Review status: 6 of 10 files reviewed at latest revision, 6 unresolved discussions.

src/pooladdr.rs, line 19 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
Done.

AwEsOmE! :rocket:

Comments from Reviewable

Review status: 6 of 10 files reviewed at latest revision, 7 unresolved discussions.

src/rule/mod.rs, line 182 at r5 (raw file):

        pf_rule.quick = self.quick as u8;
        pf_rule.af = self.get_af()?.into();
        pf_rule.proto = self.proto.into();

Now I'm extremely picky, sorry, but proto comes before af in the struct definition. If we keep them in the same order it becomes slightly easier to read plus make sure no field is missed.

Comments from Reviewable

Reviewed 2 of 5 files at r4.
Review status: 7 of 10 files reviewed at latest revision, 4 unresolved discussions.

Comments from Reviewable

Review status: 7 of 10 files reviewed at latest revision, 5 unresolved discussions.

src/main.rs, line 40 at r3 (raw file):

    let anchor_name = "test.anchor";
    match pf.add_anchor(anchor_name, pfctl::AnchorKind::Filter) {

I'm starting to think more and more that we should either just remove main.rs or completely reimplement it so that it is actually a real CLI interface. In its current form it's just a pretty sad combination of local test suite and examples implementation.
Since we have no need for a complete CLI tool, the other option is probably more viable. Or some of the code here can be kept, but broken out to examples in the examples/ directory.

Comments from Reviewable

Review status: 7 of 10 files reviewed at latest revision, 5 unresolved discussions.

src/lib.rs, line 304 at r5 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
We have trait to copy from Ip to pf_addr_wrap. But we don’t have a trait to do that for Endpoint, because it incorporates port too and pf_addr_wrap is purely Ipv4 or Ipv6 holder.
That’s primarily the reason why I do not rush to implement CopyTo.

Ok, I have now understood it like this now. Tell me if I missed something. So the Ip part of Endpoint is used to create this address pool here at the start, but also for setting pfioc_rule.rule.rpool.list, is that correct? Do you know why one must specify the IP in both these ways? Are you sure not one of them is enough?
The Port part of the redirect_to endpoint is only used inside the CopyTo impl for RedirectRule? So basically the Ip and Port are use more or less separately.
I'm just trying to understand what goes on to see if I can help figure out a better design that does not access .0 far away from the type definition.
If we really need to access the fields of Endpoint we could either change the struct so it has names for the fields, or give it getter methods. The reason for it being a struct without field names in the first place is because the fields were never used outside of it.

src/lib.rs, line 313 at r5 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
We could implement From<Endpoint> for PoolAddrList? I love that it's explicit though. But I don't see how to solve that otherwise. Maybe change endpoint to have two fields and problem solved?

Or expose an Endpoint::to_pool_addr_list() -> PoolAddrList ?

Comments from Reviewable

Review status: 7 of 10 files reviewed at latest revision, 4 unresolved discussions.

src/lib.rs, line 313 at r5 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Or expose an Endpoint::to_pool_addr_list() -> PoolAddrList ?

That is more explicit, plus you can make it take &self so you don't consume yourself in the process, which From does.

Comments from Reviewable

Review status: 7 of 10 files reviewed at latest revision, 5 unresolved discussions.

src/lib.rs, line 304 at r5 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Ok, I have now understood it like this now. Tell me if I missed something. So the Ip part of Endpoint is used to create this address pool here at the start, but also for setting pfioc_rule.rule.rpool.list, is that correct? Do you know why one must specify the IP in both these ways? Are you sure not one of them is enough?
The Port part of the redirect_to endpoint is only used inside the CopyTo impl for RedirectRule? So basically the Ip and Port are use more or less separately.
I'm just trying to understand what goes on to see if I can help figure out a better design that does not access .0 far away from the type definition.
If we really need to access the fields of Endpoint we could either change the struct so it has names for the fields, or give it getter methods. The reason for it being a struct without field names in the first place is because the fields were never used outside of it.

Correct, to conclude:

rpool.list only needs the linked list of IPs (created via PoolAddrList)
rpool.{port_op, proxy_port} are filled with help of TryCopyTo<ffi::pfvar::pf_pool> for Port.

Apparently (2) can be done from CopyTo in RedirectRule but (1) needs external help.
I was thinking of keeping Endpoint immutable but adding methods to obtain Ip or Port from it.

Comments from Reviewable

Review status: 7 of 10 files reviewed at latest revision, 5 unresolved discussions.

src/main.rs, line 40 at r3 (raw file):
Previously, faern (Linus Färnstrand) wrote…
I'm starting to think more and more that we should either just remove main.rs or completely reimplement it so that it is actually a real CLI interface. In its current form it's just a pretty sad combination of local test suite and examples implementation.
Since we have no need for a complete CLI tool, the other option is probably more viable. Or some of the code here can be kept, but broken out to examples in the examples/ directory.

Yeah, eventually it will go away I think.

Comments from Reviewable

Review status: 7 of 10 files reviewed at latest revision, 5 unresolved discussions.

src/rule/mod.rs, line 182 at r5 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Now I'm extremely picky, sorry, but proto comes before af in the struct definition. If we keep them in the same order it becomes slightly easier to read plus make sure no field is missed.

Done.

Comments from Reviewable

Review status: 7 of 10 files reviewed at latest revision, 5 unresolved discussions.

src/lib.rs, line 313 at r5 (raw file):
Previously, faern (Linus Färnstrand) wrote…
That is more explicit, plus you can make it take &self so you don't consume yourself in the process, which From does.

Done.

Comments from Reviewable

Review status: 7 of 11 files reviewed at latest revision, 5 unresolved discussions.

src/lib.rs, line 304 at r5 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
Correct, to conclude:

rpool.list only needs the linked list of IPs (created via PoolAddrList)
rpool.{port_op, proxy_port} are filled with help of TryCopyTo<ffi::pfvar::pf_pool> for Port.

Apparently (2) can be done from CopyTo in RedirectRule but (1) needs external help.
I was thinking of keeping Endpoint immutable but adding methods to obtain Ip or Port from it.

If we add port and ip accessors for Endpoint, do you think we should make the actual fields inside of Endpoint then and add new(ip, port) constructor?

Comments from Reviewable

Review status: 7 of 11 files reviewed at latest revision, 5 unresolved discussions.

src/lib.rs, line 304 at r5 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
If we add port and ip accessors for Endpoint, do you think we should make the actual fields inside of Endpoint then and add new(ip, port) constructor?

ps: I mean make the actual fields inside 1. named 2. private

Comments from Reviewable

Review status: 7 of 11 files reviewed at latest revision, 5 unresolved discussions.

src/lib.rs, line 304 at r5 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
ps: I mean make the actual fields inside 1. named 2. private

Another possibility here to unpack Endpoint as let Endpoint(rdr_ip, rdr_port) = redirect_to; and fill in rpool manually instead of doing half of work in CopyTo of RedirectRule and the rest in lib.rs. It's not ideal but until we have better abstractions it sounds reasonable to me.

Comments from Reviewable

Review status: 7 of 11 files reviewed at latest revision, 5 unresolved discussions.

src/lib.rs, line 304 at r5 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
Another possibility here to unpack Endpoint as let Endpoint(rdr_ip, rdr_port) = redirect_to; and fill in rpool manually instead of doing half of work in CopyTo of RedirectRule and the rest in lib.rs. It's not ideal but until we have better abstractions it sounds reasonable to me.

The latest commit destructures Endpoint nicely and moves rpool configuration into add_redirect_rule instead of having it half/half shared between both TryCopyTo for RedirectRule and lib.rs

Comments from Reviewable

Review status: 7 of 11 files reviewed at latest revision, 5 unresolved discussions.

src/lib.rs, line 304 at r5 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
The latest commit destructures Endpoint nicely and moves rpool configuration into add_redirect_rule instead of having it half/half shared between both TryCopyTo for RedirectRule and lib.rs

Yes, might make sense to make the fields private and introduce a real constructor along with getters. Good to avoid having people do .0, also good if it's immutable maybe.

Comments from Reviewable

Reviewed 1 of 3 files at r6.
Review status: 8 of 11 files reviewed at latest revision, 4 unresolved discussions.

Comments from Reviewable

Review status: 7 of 11 files reviewed at latest revision, 4 unresolved discussions.

src/rule/endpoint.rs, line 28 at r6 (raw file):

    /// Returns PoolAddrList initialized with receiver's IP address
    pub fn to_pool_addr_list(&self) -> PoolAddrList {
        PoolAddrList::new(&[self.0])

I just realized this. Since the PoolAddrList only uses the Ip it's kind of off to make an Endpoint create it. Would be a better mapping to have Ip::to_pool_addr_list(&self) -> PoolAddrList instead. I did not realize this before since I didn't understand that the pool addr list only used the IP.

Comments from Reviewable

Review status: 7 of 11 files reviewed at latest revision, 4 unresolved discussions.

src/rule/endpoint.rs, line 28 at r6 (raw file):
Previously, faern (Linus Färnstrand) wrote…
I just realized this. Since the PoolAddrList only uses the Ip it's kind of off to make an Endpoint create it. Would be a better mapping to have Ip::to_pool_addr_list(&self) -> PoolAddrList instead. I did not realize this before since I didn't understand that the pool addr list only used the IP.

It's mentally so hard to map all of this. And this method here really makes me think "Ah endpoint.to_pool_addr_list() gives me that endpoint as a pool addr list" which is a false statement. endpoint.ip().to_pool_addr_list() would much better represent what it is.

Comments from Reviewable

Review status: 6 of 12 files reviewed at latest revision, 4 unresolved discussions.

src/lib.rs, line 304 at r5 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Yes, might make sense to make the fields private and introduce a real constructor along with getters. Good to avoid having people do .0, also good if it's immutable maybe.

Done.

src/rule/endpoint.rs, line 28 at r6 (raw file):
Previously, faern (Linus Färnstrand) wrote…
It's mentally so hard to map all of this. And this method here really makes me think "Ah endpoint.to_pool_addr_list() gives me that endpoint as a pool addr list" which is a false statement. endpoint.ip().to_pool_addr_list() would much better represent what it is.

Done.

Comments from Reviewable

Reviewed 2 of 5 files at r7.
Review status: 8 of 12 files reviewed at latest revision, 3 unresolved discussions.

Comments from Reviewable

Reviewed 1 of 3 files at r6.
Review status: 9 of 12 files reviewed at latest revision, 3 unresolved discussions.

src/lib.rs, line 325 at r7 (raw file):

        ioctl_guard!(ffi::pf_change_rule(self.fd(), &mut pfioc_rule)).chain_err(
            || {
                ErrorKind::InvalidArgument("Couldn't add rdr rule")

Why chain the error from the ioctl call? We don't do that in the other methods, for example in add_rule for filter rules. It does not feel like failing to add a rule is an "Invalid argument".

src/rule/mod.rs, line 170 at r6 (raw file):

    }

    /// Returns reference to inner redirect_to

It's not a reference anymore. So just update docs.

Comments from Reviewable

Reviewed 1 of 5 files at r7.
Review status: 10 of 12 files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

Reviewed 1 of 5 files at r7.
Review status: 11 of 12 files reviewed at latest revision, 4 unresolved discussions.

tests/redirect_rules.rs, line 17 at r7 (raw file):

        .action(pfctl::RedirectRuleAction::Redirect)
        .to(pfctl::Endpoint::new(ip, pfctl::Port::from(3000)))
        .redirect_to(pfctl::Endpoint::new(ip, pfctl::Port::from(4000)))

Just a thought, for ergonomics. Since there are so many From<Foo> for Ip and for Port as well. Changing the constructor of Endpoint to:
pub fn new<IP: Into<Ip>, PORT: Into<Port>>(ip: IP, port: PORT)

Would make a lot of usecases much simpler. Here for example you could just do:
.redirect_to(pfctl::Endpoint::new(ip, 4000))

tests/redirect_rules.rs, line 23 at r7 (raw file):

fn redirect_rule_ipv4() -> pfctl::RedirectRule {
    port_mapping_rule(pfctl::Ip::from(Ipv4Addr::new(127, 0, 0, 1)))

Great cleanup!

Comments from Reviewable

Review status: 11 of 12 files reviewed at latest revision, 4 unresolved discussions.

src/lib.rs, line 325 at r7 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Why chain the error from the ioctl call? We don't do that in the other methods, for example in add_rule for filter rules. It does not feel like failing to add a rule is an "Invalid argument".

Old code (6 months old?). Fixed now.

Comments from Reviewable

Review status: 11 of 12 files reviewed at latest revision, 4 unresolved discussions.

src/rule/mod.rs, line 170 at r6 (raw file):
Previously, faern (Linus Färnstrand) wrote…
It's not a reference anymore. So just update docs.

Done.

Comments from Reviewable

Review status: 10 of 12 files reviewed at latest revision, 4 unresolved discussions.

tests/redirect_rules.rs, line 17 at r7 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Just a thought, for ergonomics. Since there are so many From<Foo> for Ip and for Port as well. Changing the constructor of Endpoint to:
pub fn new<IP: Into<Ip>, PORT: Into<Port>>(ip: IP, port: PORT)

Would make a lot of usecases much simpler. Here for example you could just do:
.redirect_to(pfctl::Endpoint::new(ip, 4000))

I agree. We should have a refactoring PR to add Into<> for parameters.

Comments from Reviewable

Review status: 10 of 12 files reviewed at latest revision, 4 unresolved discussions.

tests/redirect_rules.rs, line 23 at r7 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Great cleanup!

Thanks! It occurred to me that any kind of flexibility here is unnecessary.

Comments from Reviewable

Reviewed 1 of 2 files at r8.
Review status: 11 of 12 files reviewed at latest revision, 3 unresolved discussions.

tests/redirect_rules.rs, line 17 at r7 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
I agree. We should have a refactoring PR to add Into<> for parameters.

I just thought since you added that constructor in this PR then this PR makes sense to do it in.

Comments from Reviewable

Great improvements! The end result here is nice!  :+1:

Reviewed 1 of 2 files at r8.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

Review status: all files reviewed at latest revision, 1 unresolved discussion.

tests/redirect_rules.rs, line 17 at r7 (raw file):
Previously, faern (Linus Färnstrand) wrote…
I just thought since you added that constructor in this PR then this PR makes sense to do it in.

Btw we will need to use .into() within constructor, but outside it should work fine without explicit .into()

Comments from Reviewable

Review status: 10 of 12 files reviewed at latest revision, 1 unresolved discussion.

tests/redirect_rules.rs, line 17 at r7 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
Btw we will need to use .into() within constructor, but outside it should work fine without explicit .into()

Done.

Comments from Reviewable

Like working with clay. Great collab on shaping this PR.

Review status: 10 of 12 files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

Reviewed 2 of 2 files at r9.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

Reviewed 1 of 1 files at r10.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable


Fix plant MIN_TEMPERATURE, MAX_TEMPERATURE validation

small_float only allows values from 0 to 1 so we should use float instead
Description:
the plant component uses small_float validation for temperature. Small float only covers floats from 0 to 1.
Checklist:
If user exposed functionality or configuration variables are added/changed:

[ ] Documentation added/updated in home-assistant.github.io

If the code communicates with devices, web services, or third-party tools:

[ ] Local tests with tox run successfully. Your PR cannot be merged unless tests pass
[ ] New dependencies have been added to the REQUIREMENTS variable (example).
[ ] New dependencies are only imported inside functions that use them (example).
[ ] New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
[ ] New files were added to .coveragerc.

If the code does not interact with devices:

[ ] Local tests with tox run successfully. Your PR cannot be merged unless tests pass
[ ] Tests have been added to verify that the new code works.

Hi @frog32,
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!

Cherry-picked for 0.44.1


Provide the batch SDK plugin in a separate repo

Hi,
It would be great if the Batch SDK was available in a separate repo so my team wouldn't need to manually download the plugin to drop it into the build. It would mean the following would work in config.xml:
<plugin name="com.batch.cordova" spec="https://github.com/BatchLabs/[sdk-name].git" />

Thanks!

Hello!
Thanks for the suggestion. We'll think of a solution.
We usually advise developers to use the subdirectory feature, but it looks like it has been deprecated, so we'll have to make a new repo.

In the meantime, cordova plugin add https://github.com/BatchLabs/cordova-plugin.git#:SDK/batch-cordova-plugin should do the trick

I can confirm that the above url works. Thanks for replying so quickly.

Hi @abarisain.
Cordova 7 has just been released so this is a higher priority then it once was. Can the plugin be moved into a separate repo?
Cheers.

Well, first I'll have to check if the plugin still works okay with Cordova 7 :)
But sure, we'll look at it.

Quick update:
Looks like cordova 7 broke more than that, as plugins are now required to have a package.json.
Since Cordova 7 added breaking changes with not much of a warning, we'll proceed carefully, and make sure everything in the SDK works before officially stating that we support Cordova 7
The good news is that NPM publishing has been made easier, and we'll give it a shot :)

Hello!
Rather than creating a new repo, we published the SDK on npm. Note that the npm page is currently pretty empty, we'll take care of that later.
Integrating the sdk should be as simple as doing cordova plugin add batchsdk-cordova
Also, Cordova 7 is still untested with Batch, so we're not supporting this officially. Please do not hesitate to report any bug you encounter with the support team.

@abarisain Thanks for looking into this so quickly.

Hi @abarisain,
Using Cordova 6.5.0, I'm getting the following error when running cordova prepare:
Failed to restore plugin "com.batch.cordova" from config.xml. You might need to try adding it again. Error: Failed to fetch plugin com.batch.cordova@1.
7.4 via registry.
Probably this is either a connection problem, or plugin spec is incorrect.
Check your connection and plugin name/version/URL.
Error: Registry returned 404 for GET on https://registry.npmjs.org/com.batch.cordova

Oh, I didn't know that cordova's plugin name had to match npm's

Also FYI, I noticed the npm search for "batch sdk" came up with this, which is definitely not from yourselves! https://www.npmjs.com/package/batch-cordova-plugin

Thanks for the heads up, we're aware of this package, and we'll try to get in touch with the author to remove it, since we now have an official one :)
Also, I suggest that we forget that batchsdk-cordova has ever existed: The plugin has been republished as com.batch.cordova. It's not a very usual name for an npm package, but I don't want to change the plugin ID, as this might cause headaches to developers who integrated the SDK with other solutions.
cordova plugin add batchsdk-cordova should work.

I can confirm the above command works on Cordova 6.5.0 (not been able to test v7 yet).
Thanks for sorting so quickly.

Note: I edited my previous post, I meant "com.batch.cordova" rather than "batchsdk-cordova" in the fixed command.


first round of faq docs for traffic management task

Jenkins job istio.github.io/presubmit passed

cc @kyessenov and @ijsnellf to pile on other limitations.


What conference or journal did this paper submit to and when will the code release?

Hi, @lajoiepy! Thanks for your great work!
I'd like to know what conference or journal did this paper submit to? And when will the code release?

Hi! Thanks for your interest!
The code is already public, use this repo as a ROS 2 workspace and clone the sub-repos using vcs as described in our start-up instructions: https://lajoiepy.github.io/cslam_documentation/html/md_startup-instructions.html
The Swarm-SLAM stack includes 4 packages:

cslam: contains the Swarm-SLAM nodes;
cslam_interfaces: contains the custom ROS 2 messages;
cslam_experiments: contains examples of launch files and configurations for different setups;
cslam_visualization: contains an online (optional) visualization tool to run on your base station to monitor the mapping progress.

I'll let you know the journal it is submitted to once the paper is accepted!
Cheers!

In the mean time, here is a link to our paper on arXiv: https://arxiv.org/abs/2301.06230

To get the code:
sudo apt install python3-vcstool
git clone https://github.com/MISTLab/Swarm-SLAM.git
cd Swarm-SLAM
mkdir src
vcs import src < cslam.repos

Great! Thanks a lot!


Is it possible to provide a slot at the bottom

as title
thank you

@menglyf done  https://github.com/anish2690/vue-color-kit/pull/49


Fungi and mosses

[ ] Create a two nutritious fungi: Cyan Fungus and Amber Fungus
[ ] Make it so that two different Amber Fungi have the same name to the player but one heals and the other damages him
[ ] Add fungi to swamps
[ ] Add fungi to dungeon walls
[ ] Create a luminescent green moss that provides light to some underground locations

I have a branch with edited locations.json and items.json files to add the fungi and moss (still need to test). I noticed that the luminescent moss isn't useful for most of the rooms though because there is a guarantee of a torch spawn. I could decrease the spawn rate of the torch or scrap the the moss. If you want to keep the moss, it could also be a cool effect for the graveyard location.

IIRC, part of the idea was that you could then remove the torch (to take it with you) and the moss would stay there, lighting the room. Does this work?
It has been some time since I tested this.

I could decrease the spawn rate of the torch or scrap the the moss. If you want to keep the moss, it could also be a cool effect for the graveyard location.

I'd say scrap nothing, maybe add it to the graveyard (seems a good idea to me). This can be done in multiple PRs, and should preferrably be done this way.
Note that you do not have to follow any instructions, do as you please (feel free to put forth your own ideas) and we review and discuss them in the PRs.

I'll check out ways to get that functionality for the moss. I like the idea.


Parse 32 character auto-generated UUIDs correctly.

stag1/prepare-app/prepare-app.c:get_machine_name()
CHANGE:

stag1/prepare-app/prepare-app.c:get_machine_name() Now parses the expected
format of UUIDs in the format of 8-4-4-4-12. This is done becaues the
package which generates UUIDs generates 32-character long UUIDs instead of
the expected 28-character long UUIDs.

Fixes #2573

Well I'm actually super dumb. I completely forgot github added a contributing.md. I can go amend the commit and follow the contributing guide if necessary.  I realized you had a contributing.md after I had made a PR, realized my transgressions, and then opted out of repentance.

There was a reason why we had hostname not longer than 32 characters ( @steveeJ will be better in this area) and this was why uuid was chopped to 28 characters, because we are adding rkt- prefix.
This could be also connected with limits on kubernetes side (this is only a wild guess).

@jellonek Thanks for responding. It's entirely possible I jumped the gun on this one because it seemed obvious at the time, but as you mentioned, apending 4 characters to the beginning does make sense.
I probably should have waited for a bug verification first! I'll see if anyone responds over the next 2-3 days and if so, I'll see if I can be of any help. Thanks for responding though.

yw

@jellonek I don't recall stumbling upon this but I just checked on my host.
$ getconf HOST_NAME_MAX
64

A quick look against the systemd docs at https://www.freedesktop.org/software/systemd/man/machinectl.html#Machine and Image Names confirm this number, so I can't confirm the 32 character limit here, unless I'm looking in the wrong place.
@alban or @iaguis might know better

@vcaputo blame points to You as the author of these lines. Do you recall why hostname there is chopped down to 32 characters?

@nekdo, you're right the offset should still be 20. 24 leads to undefined behavior. I'm not too sure on the status of this bug fix (or even really if it is a bug),  but I'll push a commit regardless here in a few

Basing on @steveeJ comment - this looks also clean for me, but I think that @vcaputo also should have a chance to comment this.

any news here?

Can you squash and follow the commit message guidelines?

We should stop waiting for @vcaputo there.

@iaguis Updated.

LGTM


Update to use devops checkout (again)

Looks like a bad merge between https://github.com/Azure/azure-sdk-for-net/commit/ab85d3e6c9c13256c76b8d5184bcf75368ff52af and https://github.com/Azure/azure-sdk-for-net/commit/9fba7b26b29d3094d30f442050ca8eb20591d8de causes us to revert back to the old build-tools clone template.

test pipeline https://dev.azure.com/azure-sdk/internal/_build/results?buildId=3698829&view=logs&s=69878f40-1e39-53f0-a439-635d139ede3d

/check-enforcer override


Not getting promt on TV to Accept it

Hi and thank you for getting back to me.
I did not get the prompt you on the TV to accept commands from this code the first time your run it.
The prefix on the ip address with ws:// Did work on getting rid of the error.
Below is what I am getting:
C:\Users\TEST\Downloads\alexa-lgtv-remote-master (1)\alexa-lgtv-remote-master>npm run start

<EMAIL_ADDRESS>start C:\Users\TEST\Downloads\alexa-lgtv-remote-master (1)\alexa-lgtv-remote-master
gulp build && node dist/index.js

[14:56:30] Using gulpfile ~\Downloads\alexa-lgtv-remote-master (1)\alexa-lgtv-remote-master\gulpfile.js
[14:56:30] Starting 'build'...
[14:56:30] Starting 'lint'...
[14:57:00] Finished 'lint' after 31 s
[14:57:00] Starting 'builder'...
[14:57:01] Finished 'builder' after 607 ms
[14:57:01] Finished 'build' after 31 s
alexa-lgtv-remote listening on port 3000!
My tv is a [LG] webOS TVUJ6570  (a 2017 model)
Software Version 04.70.10
Also for AWS Lambda What part of the Program need to go up there Zipped I am guessing the contents in the SRC Folder.
Any help on Please Advise Thank you

OK, so good news is that now the program is running and can be called by the Amazon Skill. If you are not getting a timeout error, it means that it is connecting to the tv locally. I believe the first time you invoke one of the commands you should see the prompt on the TV.
Have you created the custom skill on Amazon and are ready to test it? For example, if you follow my instructions, you should have created an invocation name, added some slots for intentions, pasted the schema that goes into the JSON model, and designated an endpoint.
From the test tab, you can see if the endpoint is reachable and start sending commands. I suggest a simple mute command and you can see if the endpoint is reachable from Amazon's servers. I started with an ngrok endpoint to make sure that my local machine was working before pushing to the cloud.
For AWS, you would need to send the src and main directories and ensure that it compiles on the server with all dependencies, etc. Depending on your configuration it might require setting up environment variables for the instance or if you are using a remote server that's always running, you just need to make sure that the .env file points to your external ip address for your internet provider (please share details as to what you're intending).

I am using a Windows 10 PC to do this still have not gotten a accept Prompt on TV after
Running npm run start
On AWS Lambda what runtime should I use right now I am using Node.js4.3 any Higher I think I need to Convert the SCR fold Files to .js
I am not sure what you mean by a remote server always running and running what Program would it be running.
My intention is to make this work Like a Alexa Skill using AWS Lambda.
I helped doing a Alexa Skill for DirectTV. that works great now.
I did create on the custom skill on Amazon that was successful.
I ran the test this is what I got:
mute        (This worked but the rest did not**)
turn off tv
You don't have any Smart Home devices, to get started, go to the Smart Home section of the Alexa App.
mute tv
trun on tv
Sorry, I'm not sure.
turn on mute
Sorry, what device?^

So you created a skill, let's assume the invocation name was "el gee", in the test tab you would type
ask el gee to mute tv.
To turn off tv you would similarly type ask el gee to turn off tv. My experience with the invocation name is to be careful not to pick a reserved word, so if you pick something like "tv" and say "ask tv to mute tv" it might not respond as you would expect.
The documentation for AWS lambda is here but to be honest I haven't deployed this app using AWS lambda I deployed it on a Heroku server since it was free. I'm also on Windows and didn't want to mess with making sure the node_module versions were correct when zipping up the files.
You might consider checking out this documentation that walks through how to convert the nodejs app into a serverless app.
Good luck!

Thank you I am going to try and do it your way first on Heroku Server ,but what files go up there?
and what would my Endpoint be?

I suggest you confirm you can get the server running locally on your windows machine first to ensure the code is doing everything you want it to before pushing the application to a Heroku server (or AWS). If you've already done this apologize, as it's not clear if you've gotten the application running and working.
When building locally, you will need an endpoint that the AWS server can access that is using https. I used ngrok for that, which if you are running the app on port 3000, you can run

ngrok http 3000

and it will provide an endpoint that you can put into the Alexa skill to test.
When you build on Heroku, you can basically push your git repository directly to Heroku and they will provide you an endpoint that you can use for the Alexa skill. On Heroku you can set your environment variables (so you don't publish the .env to the repository) and you'll need to open up your router to accept those incoming calls.

Will closeout this item, let me know if you need anything else


Formik not updating values when order of keys in object changes

Bug report
Current Behavior
I'm trying to re-order an object in order to sort some fields. Using formik it sorts the first time but then it seems to do some kind of caching and wont refresh the order anymore. I cannot change the structure to an array.. It has to be an object. The only other thing I thought of doing was adding some random value to the object but that wont pass code review. Is there anything else I can do to get this object to sort when passed through formik?
// variationsByKey is a new object each time but has the same keys values as the old object (but in a diff order)
  console.log('before', variationsByKey);
  return (
    <Formik
      enableReinitialize={true}
      initialValues={variationsByKey}
      {({
        values,
      }) => {
        console.log('after', values);

Sorts a few times and then keeps memory of the object. before is the correct sorting, and after ends up never changing.
before {new_variation: {…}, on: {…}, off: {…}}

after {new_variation: {…}, off: {…}, on: {…}}

Expected behavior
When a new object is passed into initialValues formik should re-render even if the previous object has keys and values matching the old object.
Reproducible example
https://codesandbox.io/s/sweet-brattain-29z53?file=/src/App.js
Suggested solution(s)
If the order of the keys changes in an object then the values should be refreshed.
Additional context
I'm trying to solve this difficult problem at work where sorting is broken but I have no control over the input to formik.  We are changing the order of the keys in these objects and it sorts in every other table not wrapped in formik.
Your environment

Software
Version(s)

Formik
^2.1.4

React
16.14.0

TypeScript

Browser
chrome

npm/Yarn
yarn

Operating System
mac os big sur

If you use an array instead of an object it should work. the key of the values object exists already as the key of the object, so you should be able to call object.values on it and still do what you need to do.
I'm assume Formik's isEqual check doesn't compare the "sorting" of objects.

You can see the workaround in action here: https://codesandbox.io/s/angry-payne-q3njf?file=/src/App.js
For what it's worth I know that JS allows developers to use an object like an sorted, iterable array, but I prefer to treat it more like a key-indexed, unsorted bag of properties, especially because destructuring and spreading mean that objects are often in different orders even when they should be considered "equivalent" from a performance standpoint.

You can see the workaround in action here: https://codesandbox.io/s/angry-payne-q3njf?file=/src/App.js
For what it's worth I know that JS allows developers to use an object like an sorted, iterable array, but I prefer to treat it more like a key-indexed, unsorted bag of properties, especially because destructuring and spreading mean that objects are often in different orders even when they should be considered "equivalent" from a performance standpoint.

Yeah this workaround works but now youre using an array vs an object.  That totally makes sense and is a valid thing to do, but in my case I have a ton of unrelated existing code that I have to refactor just to correct sorting.

You can just convert Formik's version back  on submit by using reduce() instead of refactoring the whole project.

that is kind of what im doing but there are many pieces to it and its getting tricky to convert the data back and forth.  it would be nice if formik respected the order of javascript keys.

I think this would be a breaking change especially in terms of enableReinitialize. I'm not totally against it but probably 40/60 against it. Formik's values object is very much intended to be a bag of nested properties in no specific order, and I believe many people, when updating an object field's value will do something like:
setFieldValue("myObjectValue", {
  ...prevMyObjectValue,
  myUpdatedProp: 'updated',
});

In order to make a change like this, we would have to determine that even though myObjectValue is an object, and could be considered to have nested fields, it is in fact NOT nested fields and is a single field which may or may not be sorted. Even then, some people might not expect performing a spread operation like the above to cause updates to values, and will create the exact opposite issue of this: "Formik updating values when order of keys in object changes :("
My recommendation is to use TypeScript, which will make it very clear when you need to convert back and forth, and to create a selector for converting back and forth, so even if you need to make this conversion 1000 times in your code, all you need to do is:
interface KeyedValue<Key extends string> { key: Key, value: string };
interface KeyedObject = {
  [Key: string]: KeyedValue<Key>,
}

const selectKeyedObjectFromKeyedValues = (
  values: KeyedValue<unknown>[]
) => values.reduce<KeyedObject>((keyedObject, keyedValue) => {
  keyedObject[keyedValue.key] = keyedValue;
  return keyedObject;
}, {});

const selectKeyedValuesFromKeyedObject = (keyedObject: KeyedObject) => keyedObject.values();

// 1000 times? easy
const objectValue = selectKeyedObjectFromKeyedValues([ { key: 'abc', value: "1" } ]);
const arrayValue = selectKeyedValuesFromKeyedObject({ abc: { key: 'abc', value: "1" } } });

Now, TypeScript will yell at you whenever you forget to convert. If you can't use typescript, just create the selectors and pray you never forget. That's not a Formik issue, that's just the state of things until you migrate to TS.

That is a good point.  When are are saving values you'd have to make sure you are maintaining the same order in order to preserve the order of the fields.
My issue is that we are creating temp keys in the formik values object in order to create new rows or to-be-deleted rows that are applied on save.  When I convert the object into an array I lose those special keys.  I wish I could add those special keys to the field values themselves but the structure of those is set in stone and I have no ability to change it.  I think ultimately to solve this something with our data needs to change.

When working inside of Formik (or any form library), it's values shouldn't be considered your database object. It is only the inputs and values necessary to construct your database object.
When handling the data in Formik, you can add the special key to the Value object for use in an array, and remove it when it is converted back to Keyed Object format. What is inside Formik doesn't have to be the same as long as it's converted before entering into Formik (like via initialValues, setValues), and converted back when passed back out (like via onSubmit). This is why I recommend TS, it makes this enforceable.
Once you have an array, you can use FieldArray to help manage adding to, deleting from, and sorting your values array.
https://formik.org/docs/api/fieldarray


Slightly more verbose logging when assign types don't match

I found this handy when I was injecting named value types and the types were not matching.

nice - thanks!


Incorrect Nuget project referencing issue on VSTS however no issue locally

I have two .NET Standard 1.4 [tag:c#] projects

Ninja.Abstractions
Ninja.Domain

Ninja.Domain references Ninja.Abstraction.

GeneratePackageOnBuild is true for both projects

They both have code below in *.csproj file
  <PropertyGroup>
    <BuildNumber Condition=" '$(BuildNumber)' == '' ">0.0.0</BuildNumber>
    <VersionPrefix>$(BuildNumber)</VersionPrefix>
    <VersionSuffix Condition=" '$(Configuration)' == 'Debug' ">dev</VersionSuffix>
  </PropertyGroup>

When I compile the project, I do not receive error message and everything looks fine.

On [tag:vsts] I delete the 3rd project which is called Ninja.Host, so please ignore that project since it is just for demonstration purposes.

On [tag:vsts] I only do Restore and Build not Pack nor Publish.

I push the BuildNumber value via arguments as /p:BuildNumber=$(Build.BuildNumber) for Build task

I do copy **/*.nupkg to $(build.artifactstagingdirectory)\nuget directory via Copy Task

When I queue the build on [tag:vsts] I get warning below.

  2017-08-07T03:23:50.2098321Z [command]"C:\Program Files\dotnet\dotnet.exe" build d:\a\1\s\src\Ninja.Domain\Ninja.Domain.csproj --configuration release /p:BuildNumber=1.0.353
  2017-08-07T03:23:50.3518364Z Microsoft (R) Build Engine version 15.1.1012.6693
  2017-08-07T03:23:50.3518364Z Copyright (C) Microsoft Corporation. All rights reserved.
  2017-08-07T03:23:50.3518364Z
  2017-08-07T03:23:51.9897194Z   Ninja.Abstractions -> d:\a\1\s\src\Ninja.Abstractions\bin\release\netstandard1.4\Ninja.Abstractions.dll
  2017-08-07T03:23:52.1097005Z   Successfully created package 'd:\a\1\s\src\Ninja.Abstractions\bin\release\Ninja.Abstractions.1.0.353.nupkg'.
  2017-08-07T03:23:53.6697753Z   Ninja.Domain -> d:\a\1\s\src\Ninja.Domain\bin\release\netstandard1.4\Ninja.Domain.dll
  2017-08-07T03:23:53.7253932Z   Successfully created package 'd:\a\1\s\src\Ninja.Domain\bin\release\Ninja.Domain.1.0.353.nupkg'.
  2017-08-07T03:23:53.7267788Z C:\Program Files\dotnet\sdk\1.0.4\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(104,5): warning : Issue found with package 'Ninja.Domain'. [d:\a\1\s\src\Ninja.Domain\Ninja.Domain.csproj]
  2017-08-07T03:23:53.7277784Z C:\Program Files\dotnet\sdk\1.0.4\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(104,5): warning : Issue: Prerelease dependency in stable package. [d:\a\1\s\src\Ninja.Domain\Ninja.Domain.csproj]
  2017-08-07T03:23:53.7277784Z C:\Program Files\dotnet\sdk\1.0.4\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(104,5): warning : Description: A stable release of a package should not have a prerelease dependency. [d:\a\1\s\src\Ninja.Domain\Ninja.Domain.csproj]
  2017-08-07T03:23:53.7277784Z C:\Program Files\dotnet\sdk\1.0.4\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(104,5): warning : Solution: Either modify the version spec of dependency "Ninja.Abstractions [0.0.0-dev, )" or update the version field in the nuspec. [d:\a\1\s\src\Ninja.Domain\Ninja.Domain.csproj]
  2017-08-07T03:23:53.7297813Z
  2017-08-07T03:23:53.7307788Z Build succeeded.

Downloaded artifacts and have 2 files which has Ninja.Abstractions.1.0.353.nupkg and Ninja.Domain.1.0.353.nupkg

Opened Ninja.Abstractions.nuspec which is in Ninja.Abstractions.1.0.353.nupkg
  <?xml version="1.0" encoding="utf-8"?>
  <package xmlns="http://schemas.microsoft.com/packaging/2013/05/nuspec.xsd">
    <metadata>
      <id>Ninja.Abstractions</id>
      <version>1.0.353</version>
      <authors>Ninja.Abstractions</authors>
      <owners>Ninja.Abstractions</owners>
      <requireLicenseAcceptance>false</requireLicenseAcceptance>
      <description>Package Description</description>
      <dependencies>
        <group targetFramework=".NETStandard1.4">
          <dependency id="NETStandard.Library" version="1.6.1" exclude="Build,Analyzers" />
        </group>
      </dependencies>
    </metadata>
  </package>

Opened Ninja.Domain.nuspec which is in Ninja.Domain.1.0.353.nupkg
  <?xml version="1.0" encoding="utf-8"?>
  <package xmlns="http://schemas.microsoft.com/packaging/2013/05/nuspec.xsd">
    <metadata>
      <id>Ninja.Domain</id>
      <version>1.0.353</version>
      <authors>Ninja.Domain</authors>
      <owners>Ninja.Domain</owners>
      <requireLicenseAcceptance>false</requireLicenseAcceptance>
      <description>Package Description</description>
      <dependencies>
        <group targetFramework=".NETStandard1.4">
          <dependency id="Ninja.Abstractions" version="0.0.0-dev" exclude="Build,Analyzers" />
          <dependency id="NETStandard.Library" version="1.6.1" exclude="Build,Analyzers" />
        </group>
      </dependencies>
    </metadata>
  </package>

As you can see it has <dependency id="Ninja.Abstractions" version="0.0.0-dev" exclude="Build,Analyzers" /> which should be <dependency id="Ninja.Abstractions" version="1.0.353" exclude="Build,Analyzers" /> and this behaviour does not apply when I build it locally.  In Visual Studio 2017 it builds the way it is suppose to do with the correct dependency information.

I was thinking about adding explicit nuspec file but it will force me to track references manually which is not something I would like to do.

I appreciate any afford to help me solve this issue.

Thanks to  @dasMulli 🤗  passing /p:Configuration=$(BuildConfiguration) /p:BuildNumber=$(Build.BuildNumber) to dotnet restore solved the issue.


Plugin "@netlify/plugin-nextjs" must not be specified twice in the app

when running
netlify build

I'm getting:
Netlify Build
────────────────────────────────────────────────────────────────

❯ Version
  @netlify/build 29.51.4

❯ Flags
  {}

  Configuration error
────────────────────────────────────────────────────────────────

  Error message
  When resolving config file opendesk__tina--next/netlify.toml:
  Plugin "@netlify/plugin-nextjs" must not be specified twice in the app

netlify-cli/17.33.5 darwin-x64 node-v20.12.2
├──<EMAIL_ADDRESS>├──<EMAIL_ADDRESS>[build]
  command = "npm run build"
  publish = ".next"

[[plugins]]
  package = "@netlify/plugin-nextjs"

This is similar to https://answers.netlify.com/t/netlify-deploy-plugin-configuration-error/

Hi @jimlundblad
I did reproduce this issue. As a workaround I found that downgrading the CLI to 17.33.3 allow for successful builds until fix is authored and released

<EMAIL_ADDRESS>should fix this (I can no longer reproduce the problem)


Azure DevOps Pipeline Environment - Resource limit of 10

Hello,
Our team is trying to offer a self-servicing platform, where developers can create their own Kubernetes namespaces and destroy them, on demand.
This requires creating a Resource inside an Azure DevOps Environment and we have scripted this.  The issue is that we have hit a limit of 10 resources/environment and can't create any more.
Is there a way to lift this limitation of 10 resources per Environment, or to at least increase/double it?
Thank you

@razidare  Thank you for the question.
It sounds like you're hitting the 10 parallel jobs constraint (see Azure DevOps pricing) in which case the you'll need to purchase additional jobs.

Closing this issue now, please comment below if there are questions.

@msebolt
I was referring to the Azure Pipelines limit of 10 resources/Environment. Not to the agents concurrent job limits.
We have enabled the preview feature "Multi-stage pipelines" for our organisation and now our menu features "pipelines" and "environments" instead of "builds" and "releases".
The "Environment" menu can contain "Resources" which ATM support only AKS namespaces. There is a hard limit of 10 resources/environment. Can this be increased?

@razidare  Apologies for the confusion, assigning one of the AKS authors now.

Thanks for your question. It looks like your question isn't an issue with the documentation, but an issue or question specific to your implementation or a bug in the product itself.
For help with a specific task, here are a couple of options where you might consider asking your question:

Azure DevOps on Stack Overflow
Azure DevOps Support Bot

If you've encountered an issue with the product itself, to make sure it gets in front of the right people, please submit your bug here.
If you have an idea for improving the product, please submit your idea here.


macOS 14.1.2 bug: Volume control not working

Before opening the issue, have you...?

[X] Searched for existing issues
[X] Looked through the wiki
[X] Updated MonitorControl to the latest version (if applicable)

Describe the bug
I Just bought the M2 Mac mini, and when I connect to the monitor, the Volume control is not working, the volume indicator shows volume is changing when i press the volume UP/DOWN key, but when I open safari to test, it doesn't work as expect, and yet I have no clue how this happened , blow is the detail.
Steps to reproduce

Press UP/DOWN key of volume
The volume indicator shows the volume has changed
open any app could play music or video
the volume doesn't change at all

Expected behavior
Press UP/DOWN key and the volume change
Anything else?
No response
Environment Information (please complete the following information)
- macOS version:macOS 14.1.2 (23B92)
- Mac model:Mac mini M2
- MonitorControl version:4.2.0 Build 7048
- Monitor(s):LG HDR WFHD Display
- Apple Silicon/M1 (yes or no): yes

MonitorControl does not support the HDMI port of the entry level M2 mini. Use BetterDisplay instead. If you are using USB-C and DDC generally works, then it might be a display incompatibility issue.

Same here. USB-C connected monitor's (INNOCN 29C1F-D) volume won't change. Any possible fixes?
(brightness does thought)


[Feature Request] I hope el-radio and el-checkbox add slots in border mode to customized

Existing Component
是
Component Name
Radio、Checkbox
Description
希望增加Radio、Checkbox 在border模式下的自定义插槽 ，目前组件只支持单行文字的显示
I hope el-radio and el-checkbox add slots in border mode to customized, at present, the component only supports single line text display

This would be very helpful.

You can use el-button plain style and el-radio together.
<el-button type='primary' plain>
  <el-radio></el-radio>

  Label Name
</el-button>


Add taplo pre-commit hook to format and sort toml files

🚀 Pull Request
Description

resolve #1205

pre-commit.ci autofix

@tkoyama010 You need to add a changelog news fragment, see here.
I'd suggest its type is a contributor change :+1:


Define PLATFORM and PLATFORM_NUMBER based on the minimum Android SDK Version.

This PR updates PLATFORM and adds PLATFORM_NUMBER to match with the project's minimum Android SDK version, moving away from the previous hardcoding of android-21.

@player-03 May i get a review on this pr?

Does this break any existing workflow?

Does this break any existing workflow?

I don't think it would break anything. I've tested this on a project and it works just fine.

Does this break any existing workflow?

I don't think it would break anything. I've tested this on a project and it works just fine.

Wont developers need to include these in their project xml now?

Does this break any existing workflow?

I don't think it would break anything. I've tested this on a project and it works just fine.

Wont developers need to include these in their project xml now?

Noup, using project.config.getInt("android.minimum-sdk-version", 21); makes it so 21 is the default value if there no version found within project.xml

Sounds good. Close this and PR to the correct 8.3 branch though.

I changed the base branch to 8.3.0-Dev.

Uh, i think i broke something, I'll close this and change the branch


libsForQt5.qtstyleplugin-kvantum provides no programs

Describe the bug
libsForQt5.qtstyleplugin-kvantum v1.1.2 provides no programs while kdePackages.qtstyleplugin-kvantum v1.1.2  provides kvantummanager and kvantumpreview
Steps To Reproduce
Steps to reproduce the behavior:

Install libsForQt5.qtstyleplugin-kvantum and check if kvantum manager is installed

Expected behavior
Kvantum manager should be installed while installing libsForQt5.qtstyleplugin-kvantum
Screenshots

Additional context
I'm using plasma5 and I need install two packages to get Kvantum Manager
 environment.systemPackages = with pkgs; [
    kdePackages.qtstyleplugin-kvantum
    libsForQt5.qtstyleplugin-kvantum
  ];

Notify maintainers
@romildo @Scrumplex

Metadata
Please run nix-shell -p nix-info --run "nix-info -m" and paste the result.
[user@system:~]$ nix-shell -p nix-info --run "nix-info -m"
 - system: `"x86_64-linux"`
 - host os: `Linux 6.9.9, NixOS, 24.11 (Vicuna), 24.11.20240714.693bc46`
 - multi-user?: `yes`
 - sandbox: `yes`
 - version: `nix-env (Nix) 2.18.5`
 - channels(root): `"nixos-23.11"`
 - nixpkgs: `/nix/store/iqxdbcjlg9wx8gdc8bdhy9nsd5imcbjp-source`

Add a :+1: reaction to issues you find important.

With Kvantum version 1.1.0, most components are now only built for Qt 6.
See https://github.com/tsujan/Kvantum/releases/tag/V1.1.0
You will have to install both if you want to use this on Plasma 5.

Ok, thanks, I should check released packages changelog next time.


False alarm - CA3077

The following example triggers CA3077:
private sealed class ResXReader : XmlTextReader
{
    [SuppressMessage("Security", "CA3077:Insecure DTD processing in XML",
        Justification = "False alarm, DTD processing is set to prohibited in constructor body.")]
    internal ResXReader(Stream stream)
        : base(stream, InitNameTable())
    {
        DtdProcessing = DtdProcessing.Prohibit;
    }

    // [...]
}

The warning does not go away even if I include XmlResolver = null; as it is suggested in TestClass2

Document Details
⚠ Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

ID: 90516fdb-8445-97f4-364d-d59e4f07dca4
Version Independent ID: 2d057d34-c44d-98ce-62c4-da3bc9a6564a
Content: CA3077: Insecure Processing in API Design, XML Document and XML Text Reader - Visual Studio
Content Source: docs/code-quality/ca3077-insecure-processing-in-api-design-xml-document-and-xml-text-reader.md
Product: visual-studio-windows
Technology: vs-ide-code-analysis
GitHub Login: @gewarren
Microsoft Alias: gewarren

@dotpaul Can you take a look at this one? It seems like there are two issues: 1) setting XmlResolver to null doesn't stop the warning and 2) the warning shouldn't fire if DtdProcessing is set to DtdProcessing.Prohibit.

@koszeggy I've been advised to ask you to log this issue in the roslyn-analyzers repo, please. Unfortunately we don't have the ability to transfer the issue.


CI: fix issues causing failures on release branches

CI jobs were previously failing when run as part of PRs or
push builds against any release branches, because the image
tags in the YAML files were not "main", but rather a specific
release version.
Moves manifest image verification of "main" tag pull policy to
a separate workflow that runs only on PRs and pushes to main.
Also modifies load-image.sh and reset-image.sh to replace any
existing image tag in the manifest with the desired tag.
Closes #169.
Closes #289.
Signed-off-by: Steve Kriss<EMAIL_ADDRESS>

Success: https://github.com/projectcontour/contour-operator/pull/360/checks?check_run_id=2604103517

This was just for testing purposes, closing it out.


Question regarding calculation of vec_C in EMstep

Dear @brandynbok and @eric-qian ,
First of all, thank you for the code, which is very well written and documented!
I have a question of calculation of vec_C in the function EMstep:
In line 464 in dfm.m there is a - between the first part of the equation ( y(idx_iM, t) * Zsmooth(bl_idxM(i, :), t+1)') and the second part starting with the selection matrix W_t ( Wt(:, i_idio_i) ).
The code refers to Equation 13 in BGR2010, but there is +between the 2 parts of the equation. The same holds for BM2010, p15, 1st equation.
Could you elaborate or is this a mistake?
Thank you and best regards,
Stefan
References:
BM2010:  Marta Banbura & Michele Modugno (2010), Maximum likelihood estimation of factor models on data sets with arbitrary pattern of missing data, ECB Working Paper No 1189.
BGR2010: Marta Banbura, Domenico Giannone and Lucrezia Reichlin (2010), Nowcasting, in  Oxford Handbook on Economic Forecasting, Michael P. Clements and David F. Hendry, editors,

For anyone interested, here is the link to the official corrected Appendix where this mistake above is redacted: