Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vlib: add port() for net.Addr #21412

Merged
merged 2 commits into from
May 5, 2024
Merged

Conversation

vcker
Copy link
Contributor

@vcker vcker commented May 4, 2024

When I start write a reverse proxy for my android / ios app, I need a port that is always available, so I set address ':0', then I try to obtain port that automatically assigned, but i can't find a function like a.port(). Although I can parse the address to get the port, but this is inelegant, so i add function .port().

fn main() {
	mut server := net.listen_tcp(.ip, ':0') or { panic(err) }
	println(server.addr()!)
	println(server.addr()!.port()!) // add port func
	mut socket := server.accept()!
	// ...
}
// port returns the ip or ip6 port of the given address `a`
pub fn (a Addr) port() !u16 {
	match unsafe { AddrFamily(a.f) } {
		.ip {
			unsafe {
				return conv.ntoh16(a.addr.Ip.port)
			}
		}
		.ip6 {
			unsafe {
				return conv.ntoh16(a.addr.Ip6.port)
			}
		}
		.unix {
			return error('unix addr has no port')
		}
		.unspec {
			return error('unspec addr family when obtain port')
		}
	}
}
// address_port_test.v
import net
import strconv

fn test_ip_port(){
	mut server := net.listen_tcp(.ip, ':0') or {panic(err)}
	port_str := server.addr()!.str().split(':').last()
	port  := strconv.parse_uint(port_str, 10, 16)!
	assert port == server.addr()!.port()!
}

fn test_ip6_port(){
	mut server := net.listen_tcp(.ip6, ':0') or {panic(err)}
	port_str := server.addr()!.str().split(':').last()
	port  := strconv.parse_uint(port_str, 10, 16)!
	assert port == server.addr()!.port()!
}

fn test_unix_port(){
	a := net.Addr{
		f: u8(net.AddrFamily.unix)
	}

	p := a.port() or {
		assert err.str() == 'unix addr has no port'
		0
	}
	assert p == 0
}

fn test_unspec_port(){
	a := net.Addr{
		f: u8(net.AddrFamily.unspec)
	}

	p := a.port() or {
		assert err.str() == 'unspec addr family when obtain port'
		0
	}
	assert p == 0
}

Copy link
Member

@Delta456 Delta456 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run v fmt -w on this file.

@vcker
Copy link
Contributor Author

vcker commented May 4, 2024

Run v fmt -w on this file.

no difference after do this

@vcker
Copy link
Contributor Author

vcker commented May 5, 2024

@Delta456 it looks v up auto checks faild, is it my wrong? what should i do next?

@JalonSolov
Copy link
Contributor

The only 2 failed checks were v up checks, which were changed recently. @spytheman @ttytm

@spytheman
Copy link
Member

spytheman commented May 5, 2024

I think, that it will need rebasing over current master for all CI checks to pass.

The most likely reason for it to fail like that with:

Commit: "206441c9df3413dd2e877d35bd3d4b8f3edb69ac" does not exist in the current repository.

... even though that commit is present on master, is that bootstrapping_ci.yml checkouts the branch of the PR, which was probably made before that 206441c9 was merged on master.

We would have to correct bootstrapping_ci.yml so that it is more reliable, however that is an unrelated change, and in the meantime, rebasing with git pull --rebase origin master, then pushing with git push -f should fix it.

@ttytm what do you think?

@spytheman
Copy link
Member

That
image
cancellation is a bit more puzzling, since our FreeBSD runner script in .cirrus.yml does not do anything that is against their rules: https://cirrus-ci.org/faq/#repository-is-blocked 🤔.

It may be just due to hitting some limit, which unfortunately does not seem to be documented.

I'll test manually the PR on a FreeBSD vps.

@spytheman
Copy link
Member

spytheman commented May 5, 2024

@ttytm, about the bootstrapping CI, what do you think of changing it to fetch an older master commit, say 20 or 30 commits older than the current head?

We do merge 10 commits per day relatively frequently, but very rarely over 12. If it checkouts a commit that is over 30 steps removed from the present, I think it will minimize the chance of similar failures (and people that work for several days before making their PRs usually already do rebase or merge before that).

@spytheman
Copy link
Member

spytheman commented May 5, 2024

As expected, tests on FreeBSD do pass (except for ftp_test.v, but that is a known failure and is not related to this PR), when run manually.

@spytheman
Copy link
Member

spytheman commented May 5, 2024

@vcker everything looks good to me.

Can you please just add a small test for the new method in vlib/net/address_test.c.v, something like:

fn test_ip_port() {
        assert new_ip(1234, addr_ip_any).port()! == 1234
        assert new_ip6(1234, addr_ip6_any).port()! == 1234
}

before doing v fmt -w vlib/net/ && git pull --rebase origin master && git push -f ?

@ttytm
Copy link
Member

ttytm commented May 5, 2024

I think, that it will need rebasing over current master for all CI checks to pass.

The most likely reason for it to fail like that with:

Commit: "206441c9df3413dd2e877d35bd3d4b8f3edb69ac" does not exist in the current repository.

... even though that commit is present on master, is that bootstrapping_ci.yml checkouts the branch of the PR, which was probably made before that 206441c9 was merged on master.

We would have to correct bootstrapping_ci.yml so that it is more reliable, however that is an unrelated change, and in the meantime, rebasing with git pull --rebase origin master, then pushing with git push -f should fix it.

@ttytm what do you think?

Already addressed here: #21414. So running into this issue should not happen anymore.

@ttytm
Copy link
Member

ttytm commented May 5, 2024

If re-running doesn't solve this, it shouldn't happen anymore after a rebase. New PRs should be free of this.

@spytheman
Copy link
Member

Already addressed here: #21414. So running into this issue should not happen anymore.

Excellent. Thank you.

@spytheman
Copy link
Member

The bootstrapping CI passed, and the Cirrus FreeBSD job also passed after the rebasing.

Imho it is good to merge.

@spytheman spytheman merged commit a4cdc48 into vlang:master May 5, 2024
57 checks passed
ttytm pushed a commit to ttytm/v that referenced this pull request May 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants