nns: Mention domain in panic messages #98

Merged
fyrchik merged 1 commits from elebedeva/frostfs-contract:fix/nns-panic-message into master 2024-06-21 13:58:28 +00:00
Collaborator

Close #92

Output after fix:

  1. Domain name too short
❯ ./bin/frostfs-adm -c ./dev/adm/frostfs-adm.yml morph nns register --name x
unable to register domain: script failed (FAULT state) due to an error: at instruction 1403 (THROW): unhandled exception: "invalid domain name format: 'x': domain name too short: got = 1, min = 2"
  1. Domain name too long
❯ ./bin/frostfs-adm -c ./dev/adm/frostfs-adm.yml morph nns register --name makaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.ns
unable to register domain: script failed (FAULT state) due to an error: at instruction 1403 (THROW): unhandled exception: "invalid domain name format: 'makaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.ns': domain name too long: got = 264, max = 255"
  1. Invalid fragment in domain name
❯ ./bin/frostfs-adm -c ./dev/adm/frostfs-adm.yml morph nns register --name -makaroni.ns
unable to register domain: script failed (FAULT state) due to an error: at instruction 1403 (THROW): unhandled exception: "invalid domain name format: '-makaroni.ns': invalid fragment '-makaroni'"

Signed-off-by: Ekaterina Lebedeva ekaterina.lebedeva@yadro.com

Close #92 Output after fix: 1. Domain name too short ``` ❯ ./bin/frostfs-adm -c ./dev/adm/frostfs-adm.yml morph nns register --name x unable to register domain: script failed (FAULT state) due to an error: at instruction 1403 (THROW): unhandled exception: "invalid domain name format: 'x': domain name too short: got = 1, min = 2" ``` 2. Domain name too long ``` ❯ ./bin/frostfs-adm -c ./dev/adm/frostfs-adm.yml morph nns register --name makaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.ns unable to register domain: script failed (FAULT state) due to an error: at instruction 1403 (THROW): unhandled exception: "invalid domain name format: 'makaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.nsmakaroni.ns': domain name too long: got = 264, max = 255" ``` 3. Invalid fragment in domain name ``` ❯ ./bin/frostfs-adm -c ./dev/adm/frostfs-adm.yml morph nns register --name -makaroni.ns unable to register domain: script failed (FAULT state) due to an error: at instruction 1403 (THROW): unhandled exception: "invalid domain name format: '-makaroni.ns': invalid fragment '-makaroni'" ``` Signed-off-by: Ekaterina Lebedeva <ekaterina.lebedeva@yadro.com>
elebedeva added 1 commit 2024-06-20 14:56:04 +00:00
DCO action / DCO (pull_request) Successful in 1m32s Details
Tests / Tests (1.21) (pull_request) Successful in 1m55s Details
Tests / Tests (1.22) (pull_request) Successful in 2m1s Details
52bf87411c
[#92] nns: Mention domain in panic messages
Signed-off-by: Ekaterina Lebedeva <ekaterina.lebedeva@yadro.com>
elebedeva requested review from storage-core-committers 2024-06-20 14:58:21 +00:00
elebedeva requested review from storage-core-developers 2024-06-20 14:58:25 +00:00
acid-ant reviewed 2024-06-21 07:13:56 +00:00
@ -222,3 +222,3 @@
fragments := splitAndCheck(name)
if fragments == nil {
panic("invalid domain name format")
panic(getDomainNamePanicMessage(name))
Collaborator

How about to panic inside splitAndCheck and remove this and bellow if statements?

How about to panic inside `splitAndCheck` and remove this and bellow `if` statements?
Poster
Collaborator

fixed

fixed
acid-ant marked this conversation as resolved
fyrchik requested changes 2024-06-21 07:26:34 +00:00
@ -913,0 +914,4 @@
// getDomainNamePanicMessage returns panic message for invalid domain name containing
// mentioned domain name
func getDomainNamePanicMessage(name string) string {
msg := "invalid domain name format: '" + name + "'"

You check something in splitAndCheck and then perform similar checks here.
They can diverge with time, so the message will cease reflecting the real error.

How about this:

  1. Check that if splitAndCheck returns nil we always panic.
  2. Move panic inside split and check and panic where the error occurs.
You check something in `splitAndCheck` and then perform similar checks here. They can diverge with time, so the message will cease reflecting the real error. How about this: 1. Check that if `splitAndCheck` returns nil we always `panic`. 2. Move panic inside split and check and panic where the error occurs.
Poster
Collaborator

done

done
elebedeva force-pushed fix/nns-panic-message from 52bf87411c to a6c20b344c 2024-06-21 07:51:30 +00:00 Compare
elebedeva force-pushed fix/nns-panic-message from a6c20b344c to 4e2882394c 2024-06-21 07:54:56 +00:00 Compare
acid-ant requested changes 2024-06-21 08:11:46 +00:00
@ -739,0 +724,4 @@
func checkDomainNameLength(name string) {
l := len(name)
if l > maxDomainNameLength {
panic(getDomainNamePanicMessage(name, "domain name too long: got = "+std.Itoa(len(name), 10)+", max = "+std.Itoa(maxDomainNameLength, 10)))
Collaborator

len(name) --> l

`len(name)` --> `l`
@ -739,0 +727,4 @@
panic(getDomainNamePanicMessage(name, "domain name too long: got = "+std.Itoa(len(name), 10)+", max = "+std.Itoa(maxDomainNameLength, 10)))
}
if l < minDomainNameLength {
panic(getDomainNamePanicMessage(name, "domain name too short: got = "+std.Itoa(len(name), 10)+", min = "+std.Itoa(minDomainNameLength, 10)))
Collaborator

len(name) --> l

`len(name)` --> `l`
elebedeva force-pushed fix/nns-panic-message from 4e2882394c to b8efeec81c 2024-06-21 08:17:17 +00:00 Compare
acid-ant approved these changes 2024-06-21 08:21:00 +00:00
dstepanov-yadro approved these changes 2024-06-21 08:30:04 +00:00
achuprov approved these changes 2024-06-21 08:30:06 +00:00
fyrchik reviewed 2024-06-21 10:19:17 +00:00
@ -913,0 +905,4 @@
// getDomainNamePanicMessage returns panic message for invalid domain name containing
// mentioned domain name
func getDomainNamePanicMessage(name string, problem string) string {

IMO a separate function is useless.
It takes space in the byte code, even though it is basically a string concatenation.
The compiler is not clever enough to inline even though problem != "" can be derived statically.

How about removing this function and moving common string in the constant?

IMO a separate function is useless. It takes space in the byte code, even though it is basically a string concatenation. The compiler is not clever enough to inline even though `problem != ""` can be derived statically. How about removing this function and moving common string in the constant?
Poster
Collaborator

Agree, removed getDomainNamePanicMessage() and added the constant

Agree, removed `getDomainNamePanicMessage()` and added the constant
fyrchik reviewed 2024-06-21 10:19:21 +00:00
fyrchik reviewed 2024-06-21 10:19:24 +00:00
elebedeva force-pushed fix/nns-panic-message from b8efeec81c to 439699236a 2024-06-21 12:31:24 +00:00 Compare
fyrchik approved these changes 2024-06-21 12:56:29 +00:00
elebedeva force-pushed fix/nns-panic-message from 439699236a to d70e971bd5 2024-06-21 13:02:12 +00:00 Compare
elebedeva force-pushed fix/nns-panic-message from d70e971bd5 to 49e5270f67 2024-06-21 13:13:03 +00:00 Compare
fyrchik merged commit 49e5270f67 into master 2024-06-21 13:58:28 +00:00
Sign in to join this conversation.
There is no content yet.