Add new NVMe syntax to add and manage NQN, hosts and ports. #173

matteotenca · 2023-08-23T21:47:55Z

Change nvmet_scan_ports() to handle ports not linked to any NQN. Fix a potential mem leak in ip_validate().

This commit adds some new command line switches to rapiddisk to add/remove/link/unlink NVMe PORTs, NVMe allowed HOSTS and NVMe NQN.

The goal is to be able to add a PORT, HOST or NQN without the need to bind them one to each other upon creation.

Examples:

-y/-Y switches create/remove a NQN. The creation/removal just requires a ramdisk name. The new NQN is configured with an empty list of allowed hosts but is marked as accessible from any host. The new NQN is not usable/reachable since it is not linked to any PORT. If the NQN is linked to a PORT or to a HOST, it is not possible to directly remove it. It must be unlinked from that PORT/HOST first.
es.
$ sudo ./rapiddisk -y -b rd0
-i/-X switches create/remove a PORT and work the same way as before, with some differences. Now it is possible to specify an ip address instead of an network interface name. If a PORT is linked to a NQN, it cannot be removed. If loop is specfified after the -t switch, the interface name/ip address following the -i switch is simply ignored, but a string of any sort must be present, such as NULL. NOTE: the ip address is not checked against existing network interfaces, so PORTs can exist which refers to unassigned IP addresses. These PORTs cannot be linked to a NQN until their IP address is assigned to an interface.
es.
$ sudo ./rapiddisk -i 192.168.20.40 -P 2 -t tcp
-o/-O switches link/unlink an existing NQN to an existing PORT.
es.
$ sudo ./rapiddisk -o -b rd0 -P 2
-z/-Z switches add/remove a new HOST to the global allowed host list. You can't remove an HOST globally if it is linked to a NQN.
es.
$ sudo ./rapiddisk -z -H myhost
-T/-M switches link/unlink an existing HOST to an existing NQN. When a HOST is linked to an NQN with no other HOSTs already linked to it, the NQN will change its policy from "accept connection from all" to "accept connection from linked allowed HOSTs only". The other way around, when the last HOST linked to a NQN is unlinked from it, the NQN is put back into "accept connection from all" mode.
es.
$ sudo ./rapiddisk -T -b rd0 -H myhost

As a general rule, when something is linked to something else, it cannot be removed from the subsystem. This means that to remove a NQN, it must be not linked to PORTs or HOSTs; to remove a PORT, it must not be linked to any NQN; to remove a HOST, it must not be linked to any NQN.

TODO:

Unify the sintax of the switches this way:
rapiddisk -action_name -param_name_01 param_value_01 -param_name_02 param_value_02
So to add a new ramdisk of 10 MB, the command should change from
rapiddisk -a 10
to (example)
rapiddisk -a -s 10
and so on. This is good from bash command args autocompletion
Add long switches i.e. --add
Add the possibility to specify a hostname other than an interface name/ip address when adding new port
Add the possibility to specify the NQN by full name instead of referring to its ramdisk name (?)
IPv6 (?)
Add JSON answers to new switches

EXAMPLE:

$ sudo ./rapiddisk -a 10 # creates a small ramdisk, rd0
rapiddisk 9.1.0
Copyright 2011 - 2023 Petros Koutoupis

Attached device rd0 of size 10 Mbytes.
$ sudo ./rapiddisk -y -b rd0 # creates a NQN using ramdisk rd0
rapiddisk 9.1.0
Copyright 2011 - 2023 Petros Koutoupis

NQN nqn.2021-06.org.rapiddisk:ubuserver-rd0 successfully created.
$ sudo ./rapiddisk -n
rapiddisk 9.1.0
Copyright 2011 - 2023 Petros Koutoupis

NVMe Target Exports

        1: NQN: nqn.2021-06.org.rapiddisk:ubuserver-rd0         Namespace: 1    Device: /dev/rd0        Enabled: True
$ sudo ./rapiddisk -N
rapiddisk 9.1.0
Copyright 2011 - 2023 Petros Koutoupis

Exported NVMe Ports

        None.

$ sudo ./rapiddisk -i 192.168.1.1 -P 1000 -t tcp # creates PORT 1000 of type tcp using an ip address
rapiddisk 9.1.0
Copyright 2011 - 2023 Petros Koutoupis

Successfully created port 1000 set to tcp for IP address 192.168.1.1.
$ sudo ./rapiddisk -N
rapiddisk 9.1.0
Copyright 2011 - 2023 Petros Koutoupis

Exported NVMe Ports

        1: Port: 1000 - 192.168.1.1 (tcp)
$ sudo ./rapiddisk -n
rapiddisk 9.1.0
Copyright 2011 - 2023 Petros Koutoupis

NVMe Target Exports

        1: NQN: nqn.2021-06.org.rapiddisk:ubuserver-rd0         Namespace: 1    Device: /dev/rd0        Enabled: True
$ sudo ./rapiddisk -o -P 1000 -b rd0
rapiddisk 9.1.0
Copyright 2011 - 2023 Petros Koutoupis

Target Port 1000 linked to NQN nqn.2021-06.org.rapiddisk:ubuserver-rd0 successfully.
$  sudo ./rapiddisk -n
rapiddisk 9.1.0
Copyright 2011 - 2023 Petros Koutoupis

NVMe Target Exports

        1: NQN: nqn.2021-06.org.rapiddisk:ubuserver-rd0         Namespace: 1    Device: /dev/rd0        Enabled: True
                Linked port: 1000       IP address: 192.168.1.1 (tcp)
$ sudo nvme discover -t tcp -a 192.168.1.1 -s 4420

Discovery Log Number of Records 1, Generation counter 7
=====Discovery Log Entry 0======
trtype:  tcp
adrfam:  ipv4
subtype: nvme subsystem
treq:    not specified, sq flow control disable supported
portid:  1000
trsvcid: 4420
subnqn:  nqn.2021-06.org.rapiddisk:ubuserver-rd0
traddr:  192.168.1.1
sectype: none

$ sudo ./rapiddisk -z -H nqn.2014-08.org # creates a new global HOST
rapiddisk 9.1.0
Copyright 2011 - 2023 Petros Koutoupis

Added nqn.2014-08.org to the list of allowable hosts.
$ sudo ./rapiddisk -T -H nqn.2014-08.org -b rd0 # links the HOST to the NQN
rapiddisk 9.1.0
Copyright 2011 - 2023 Petros Koutoupis

Added nqn.2014-08.org to the list of allowed hosts for NQN nqn.2021-06.org.rapiddisk:ubuserver-rd0. Now it is reachable from allowed hosts only.
$ sudo ./rapiddisk -n
rapiddisk 9.1.0
Copyright 2011 - 2023 Petros Koutoupis

NVMe Target Exports

        1: NQN: nqn.2021-06.org.rapiddisk:ubuserver-rd0         Namespace: 1    Device: /dev/rd0        Enabled: True
                Allowed host: nqn.2014-08.org
                Linked port: 1000       IP address: 192.168.1.1 (tcp)
$

Change nvmet_scan_ports() to handle ports not linked to any NQN. Fix a potential mem leak in ip_validate().

pkoutoupis

Starting review. Not finished yet.

pkoutoupis · 2023-10-13T19:34:12Z

src/main.c

@@ -112,7 +135,7 @@ int exec_cmdline_arg(int argcin, char *argvin[])

 	sprintf(header, "%s %s\n%s\n\n", PROCESS, VERSION_NUM, COPYRIGHT);

-	while ((i = getopt(argcin, argvin, "?:a:b:c:d:ef:gH:hi:jL:lm:NnP:p:qRr:s:t:U:u:VvXx")) != INVALID_VALUE) {
+	while ((i = getopt(argcin, argvin, "?:a:b:c:d:ef:gH:hi:jL:lm:NnP:p:qRr:s:t:U:u:VvXxYyzZoOTM")) != INVALID_VALUE) {


It is much easier to follow if these are placed in alphabetical order (capital first, then lower case).

It is much easier to follow if these are placed in alphabetical order (capital first, then lower case).

Ok! Please consider all the new swicth's names as completely temptative.

pkoutoupis · 2023-10-13T19:35:29Z

src/main.c

@@ -217,6 +242,30 @@ int exec_cmdline_arg(int argcin, char *argvin[])
 			case 'x':
 				action = ACTION_UNEXPORT_NVMET;
 				break;
+			case 'y':


Same here. It is much easier to follow if in alphabetical order (capital first, then lower case).

pkoutoupis · 2023-11-17T20:30:25Z

@matteotenca

OK. I do apologize for taking as long as I did to review this but overall, I like it. Here are my thoughts:

Coding style
I tend to conform to coding practices adopted by the kernel development community. That means that when defining a function (and for consistency between module and user space code0, the opening brace be on the next line and not the same line. This does not apply to conditional statements.

For example:

int nvmet_create_port(char *interface, int port, int protocol, char *return_message) {

Should be:

int nvmet_create_port(char *interface, int port, int protocol, char *return_message)
{

Command Options
With all the new command line options, care attention needs to be made when updated the online menu and the manpage.

Redundancy
I see the old code to enable/disable ports present. I saw your reasoning above although I believe that we can figure out something that takes either the interface or an IP address instead of having two mostly redundant sets.

Client Host NQNs
We currently don't have any means of listing client host NQNs to connect to (to view what is presently listed). I propose that we rely on rapiddisk -N to include a list, alongside the ports. This will definitely affect the JSON format which will need modification to accommodate the change.

--

I did have a question though: what are your thoughts on breaking this out of the main rapiddisk utility and instead place it in a more dedicated rapiddisk-nvme utility? This gives us more flexibility with command line args and helps simplify logic. We can also use that as an opportunity to introduce logic to save/restore configs/exports. I also foresee a near-future addition of rapiddisk-client (client-side remote management to a remote daemon) or even a curses wrapper like rapiddisk-tui.

EDIT
OH, and in response to your TODO list.

Unify the sintax of the switches this way:

I agree!

Add long switches i.e. --add

I disagree. I used to have this and the logic to maintain it got overly complicated which is why I switched to getopt in version 7.0.

Add the possibility to specify a hostname other than an interface name/ip address when adding new port

But what if more than one IP/interface is tied to that hostname?

Add the possibility to specify the NQN by full name instead of referring to its ramdisk name

Sure, I agree.

IPv6 (?)

I do not know if the NVMe Target infrastructure on Linux support that, yet.

Add JSON answers to new switches

I agree.

matteotenca · 2023-12-08T06:33:01Z

@pkoutoupis

[..] the opening brace be on the next line and not the same line. This does not apply to conditional statements.

For example:
int nvmet_create_port(char *interface, int port, int protocol, char *return_message) {
Should be:
int nvmet_create_port(char *interface, int port, int protocol, char *return_message)
{

Understood. I had/have a problem with my IDE, which suddenly started changing these code style details in files without asking or warning. So I ended up with dozens of small changes in the files, related to indent/brace positions. I reverted the changes and the IDE reverted my reverts... eventually I gave up. From now on I will try to respect these rules.

Command Options With all the new command line options, care attention needs to be made when updated the online menu and the manpage.

Ok.

Redundancy I see the old code to enable/disable ports present. I saw your reasoning above although I believe that we can figure out something that takes either the interface or an IP address instead of having two mostly redundant sets.

Consider that I left all the "old" code in place, to be sure the old switches kept running. So, if I can remember well, as of today there is a lot of double/rendunant code, which must be be cleaned up.

Client Host NQNs We currently don't have any means of listing client host NQNs to connect to (to view what is presently listed). I propose that we rely on rapiddisk -N to include a list, alongside the ports. This will definitely affect the JSON format which will need modification to accommodate the change.

Sorry, I did not understand very well. Do you want the -N switch to print all the PORTs related info, plus the NQNs related info?

I did have a question though: what are your thoughts on breaking this out of the main rapiddisk utility and instead place it in a more dedicated rapiddisk-nvme utility? This gives us more flexibility with command line args and helps simplify logic.

We can also use that as an opportunity to introduce logic to save/restore configs/exports. I also foresee a near-future addition of rapiddisk-client (client-side remote management to a remote daemon) or even a curses wrapper like rapiddisk-tui.

These two points can merge: we can end up with a daemon-centric approach, which would carry out all the operations, while the two very small/lean command line utilities would simply forward requests to it via UNIX socket or REST (hence also remote management would in fact already be implemented). The daemon could have its own configuration file, so that it is possible to choose for example the interfaces/IPs on which it should listen, and more.

Curses... Well, nomen omen ...

EDIT OH, and in response to your TODO list.

Unify the sintax of the switches this way:

I agree!

Ok!

Add long switches i.e. --add

I disagree. I used to have this and the logic to maintain it got overly complicated which is why I switched to getopt in version 7.0.

While writing the bash script to check for BTRFS issues, I ended up with nonsense long switches' names - so I believe you're right. We can go for long switches OR for short ones - using both is a mess, I agree with you. Let's stick to short ones.

Add the possibility to specify a hostname other than an interface name/ip address when adding new port

But what if more than one IP/interface is tied to that hostname?

Well I would say that it is the user's responsibility to make sure the resolved IP is correct. If a hostname resolves to more than one IP address, the first one is used, which can still be shown immediately to the user and displayed later with the -N switch...
The same already happens when the user specifies an interface name, the first IPv4 address assigned to the interface is used, regardless of the presence of others.

Add the possibility to specify the NQN by full name instead of referring to its ramdisk name

Sure, I agree.

Ok.

IPv6 (?)

I do not know if the NVMe Target infrastructure on Linux support that, yet.

It seems it is supported: I tried with an Ubuntu 22.04 6.1.0-060100-generic with an exported NQN on an on-link IPv6 address connected to an Ubuntu 20.04 5.15.0-69-generic, and worked.

Add JSON answers to new switches

I agree.

Ok.

Add new NVMe syntax to add and manage NQN, hosts and ports.

7273665

Change nvmet_scan_ports() to handle ports not linked to any NQN. Fix a potential mem leak in ip_validate().

matteotenca requested a review from pkoutoupis August 23, 2023 21:48

matteotenca self-assigned this Aug 23, 2023

pkoutoupis added the enhancement label Aug 25, 2023

pkoutoupis requested changes Oct 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new NVMe syntax to add and manage NQN, hosts and ports. #173

Add new NVMe syntax to add and manage NQN, hosts and ports. #173

matteotenca commented Aug 23, 2023

pkoutoupis left a comment

pkoutoupis Oct 13, 2023

matteotenca Oct 15, 2023

pkoutoupis Oct 13, 2023

pkoutoupis commented Nov 17, 2023 •

edited

matteotenca commented Dec 8, 2023

Add new NVMe syntax to add and manage NQN, hosts and ports. #173

Are you sure you want to change the base?

Add new NVMe syntax to add and manage NQN, hosts and ports. #173

Conversation

matteotenca commented Aug 23, 2023

pkoutoupis left a comment

Choose a reason for hiding this comment

pkoutoupis Oct 13, 2023

Choose a reason for hiding this comment

matteotenca Oct 15, 2023

Choose a reason for hiding this comment

pkoutoupis Oct 13, 2023

Choose a reason for hiding this comment

pkoutoupis commented Nov 17, 2023 • edited

matteotenca commented Dec 8, 2023

pkoutoupis commented Nov 17, 2023 •

edited