[PATCH rdma-core 0/5] srp_daemon: Rework systemd integration

[PATCH rdma-core 0/5] srp_daemon: Rework systemd integration

[Bart Van Assche]

Hello,

This patch series implements the changes that were discussed a few days
ago on the linux-rdma mailing list:
- srp_daemon.service is split into srp_daemon.service and the
  srp_daemon_port@.service template. The former does not start any srp_daemon
  process but controls the latter.
- srp_daemon_port@.service  instances are started automatically upon hot-add of
  an RDMA port to support hot-plugging.

As usual, comments and feedback are welcome.

Bart.

Bart Van Assche (5):
  srp_daemon.sh: Improve robustness
  srp_daemon: Use the recommended style in the man page
  srp_daemon: Add command-line option -j
  srp_daemon: Move srp_daemon.service from the redhat directory to the
    srp_daemon directory
  srp_daemon.service: Add support for hot-plugging

 debian/srptools.install                            |  5 ++
 redhat/rdma-core.spec                              |  8 ++--
 srp_daemon/90-srp-daemon.rules                     |  1 +
 srp_daemon/CMakeLists.txt                          |  7 +++
 srp_daemon/srp_daemon.1.in                         | 54 +++++++++++++++-------
 srp_daemon/srp_daemon.c                            | 16 ++++++-
 srp_daemon/srp_daemon.service                      | 15 ++++++
 srp_daemon/srp_daemon.service.5                    | 30 ++++++++++++
 srp_daemon/srp_daemon.sh.in                        | 34 +++++++-------
 .../srp_daemon_port@.service                       |  7 +--
 srp_daemon/srp_daemon_port@.service.5              | 46 ++++++++++++++++++
 11 files changed, 181 insertions(+), 42 deletions(-)
 create mode 100644 srp_daemon/90-srp-daemon.rules
 create mode 100644 srp_daemon/srp_daemon.service
 create mode 100644 srp_daemon/srp_daemon.service.5
 rename redhat/srp_daemon.service => srp_daemon/srp_daemon_port@.service (63%)
 create mode 100644 srp_daemon/srp_daemon_port@.service.5

-- 
2.12.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-core 1/5] srp_daemon.sh: Improve robustness

[Bart Van Assche]

Surround expansions of shell variables that can contain spaces with
double quotes. Use arrays instead of a space-separated strings to
store lists to avoid that strings that contain whitespace get split
when iterating over a list.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
---
 srp_daemon/srp_daemon.sh.in | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/srp_daemon/srp_daemon.sh.in b/srp_daemon/srp_daemon.sh.in
index cb0b81ef..75e8a31b 100755
--- a/srp_daemon/srp_daemon.sh.in
+++ b/srp_daemon/srp_daemon.sh.in
@@ -31,35 +31,35 @@
 shopt -s nullglob
 
 prog=@CMAKE_INSTALL_FULL_SBINDIR@/srp_daemon
-params=$@
+params=("$@")
 ibdir="/sys/class/infiniband"
 rescan_interval=60
-pids=""
-pidfile=@CMAKE_INSTALL_FULL_RUNDIR@/srp_daemon.sh.pid
+pids=()
+pidfile="@CMAKE_INSTALL_FULL_RUNDIR@/srp_daemon.sh.pid"
 mypid=$$
 
 trap_handler()
 {
-    if [ -n "$pids" ]; then
-        kill -15 $pids > /dev/null 2>&1
-        wait $pids
+    if [ "${#pids[@]}" ]; then
+        kill -15 "${pids[@]}" > /dev/null 2>&1
+        wait "${pids[@]}"
     fi
-    logger -i -t "$(basename $0)" "killing $prog."
-    /bin/rm -f $pidfile
+    logger -i -t "$(basename "$0")" "killing $prog."
+    /bin/rm -f "$pidfile"
     exit 0
 }
 
 # Check if there is another copy running of srp_daemon.sh
-if [ -f $pidfile ]; then
-    if [ -e /proc/$(cat $pidfile 2>/dev/null)/status ]; then
-        echo "$(basename $0) is already running. Exiting."
+if [ -f "$pidfile" ]; then
+    if [ -e "/proc/$(cat "$pidfile" 2>/dev/null)/status" ]; then
+        echo "$(basename "$0") is already running. Exiting."
         exit 1
     else
-        /bin/rm -f $pidfile
+        /bin/rm -f "$pidfile"
     fi
 fi
 
-if ! echo $mypid > $pidfile; then
+if ! echo $mypid > "$pidfile"; then
     echo "Creating $pidfile for pid $mypid failed"
     exit 1
 fi
@@ -72,12 +72,12 @@ do
 done
 
 for d in ${ibdir}_mad/umad*; do
-    hca_id="$(<$d/ibdev)"
-    port="$(<$d/port)"
+    hca_id="$(<"$d/ibdev")"
+    port="$(<"$d/port")"
     add_target="${ibdir}_srp/srp-${hca_id}-${port}/add_target"
     if [ -e "${add_target}" ]; then
-        ${prog} -e -c -n -i ${hca_id} -p ${port} -R ${rescan_interval} ${params} >/dev/null 2>&1 &
-        pids="$pids $!"
+        ${prog} -e -c -n -i "${hca_id}" -p "${port}" -R "${rescan_interval}" "${params[@]}" >/dev/null 2>&1 &
+        pids+=($!)
     fi
 done
 
-- 
2.12.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-core 2/5] srp_daemon: Use the recommended style in the man page

[Bart Van Assche]

>From https://liw.fi/manpages/, about the SYNOPSIS section:

Font usage is important here, and carries information. All the
parts that are in bold are things that the user is expected to
write verbatim. Italic indicates values the user is expected to
fill in. Normal font is used for syntax meta-characters: for
the brackets that indicate optionality, and the ellipsis that
indicates repetition.

Additionally, change the identifiers that refer to arguments to
lower case.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
---
 srp_daemon/srp_daemon.1.in | 41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/srp_daemon/srp_daemon.1.in b/srp_daemon/srp_daemon.1.in
index 008d53a2..02e1f2df 100644
--- a/srp_daemon/srp_daemon.1.in
+++ b/srp_daemon/srp_daemon.1.in
@@ -5,18 +5,30 @@
 srp_daemon \- Discovers SRP targets in an InfiniBand Fabric
 
 .SH SYNOPSIS
-.B srp_daemon [-vVcaeon] [-d \fIumad-device\fR | -i \fIinfiniband-device\fR [-p \fIport-num\fR]] [-t \fItimeout(ms)\fR] [-r \fIretries\fR] [-R \fIRescan-time\fR] [-f \fIrules-File\fR]
+.B srp_daemon\fR [\fB-vVcaeon\fR] [\fB-d \fIumad-device\fR | \fB-i \fIinfiniband-device\fR [\fB-p \fIport-num\fR]] [\fB-t \fItimeout(ms)\fR] [\fB-r \fIretries\fR] [\fB-R \fIrescan-time\fR] [\fB-f \fIrules-file\fR]
 
 
 .SH DESCRIPTION
 .PP
 Discovers and connects to InfiniBand SCSI RDMA Protocol (SRP) targets in an IB fabric.
 
-Each srp_daemon instance operates on one local port. Upon boot it performs a full rescan of the fabric then waits for an srp_daemon event. An srp_daemon event can be a join of a new machine to the fabric, a change in the capabilities of a machine, an SA change, or an expiration of a predefined timeout.
+Each srp_daemon instance operates on one local port. Upon boot it performs a
+full rescan of the fabric and then waits for an srp_daemon event. An
+srp_daemon event can be a join of a new machine to the fabric, a change in the
+capabilities of a machine, an SA change, or an expiration of a predefined
+timeout.
 
-When a new machine joins the fabric, srp_daemon checks if it is a target. When there is a change of capabilities, srp_daemon checks if the machine has turned into a target. When there is an SA change or a timeout expiration, srp_daemon performs a full rescan of the fabric.
+When a new machine joins the fabric, srp_daemon checks if it is an SRP
+target. When there is a change of capabilities, srp_daemon checks if the
+machine has turned into an SRP target. When there is an SA change or a timeout
+expiration, srp_daemon performs a full rescan of the fabric.
 
-For each target srp_daemon finds, it checks if it should connect to this target according to its rules (default rules file is @CMAKE_INSTALL_FULL_SYSCONFDIR@/srp_daemon.conf) and if it is already connected to the local port. If it should connect to this target and if it is not connected yet, srp_daemon can either print the target details or connect to it.
+For each target srp_daemon finds, it checks if it should connect to this
+target according to its rules (the default rules file is
+@CMAKE_INSTALL_FULL_SYSCONFDIR@/srp_daemon.conf) and if it is already
+connected to the local port. If it should connect to this target and if it is
+not connected yet, srp_daemon can either print the target details or connect
+to it.
 
 .SH OPTIONS
 
@@ -42,24 +54,25 @@ Generate output suitable for piping directly to a
 /sys/class/infiniband_srp/srp\-<device>\-<port>/add_target file. 
 .TP
 \fB\-a\fR
-Prints all the targets in the fabric, not only targets that are not connected through the local port. (The same as ibsrpdm.)
+Prints all the targets in the fabric, not only targets that are not connected
+through the local port. This is the same behavior as that of ibsrpdm.
 .TP
 \fB\-e\fR
 Execute the connection command, i.e., make the connection to the target.
 .TP
 \fB\-o\fR
-Perform only one rescan and exit. (The same as ibsrpdm.)
+Perform only one rescan and exit just like ibsrpdm.
 .TP
-\fB\-R\fR \fIRescan-time\fR
-Force a complete rescan every \fIRescan-time\fR seconds. If -R is not specified, no timeout rescans will be performed.
+\fB\-R\fR \fIrescan-time\fR
+Force a complete rescan every \fIrescan-time\fR seconds. If -R is not specified, no timeout rescans will be performed.
 .TP
-\fB\-T\fR \fIretry-Timeout\fR
-Retries to connect to existing target after \fIretry-Timeout\fR seconds. If -R is not specified, uses 5 Seconds timeout. if retry-Timeout is 0, will not try to reconnect. The reason srp_daemon retries to connect to the target is because there may be a rare scnerio in which srp_daemon will try to connect to add a target when the target is about to be removed, but is not removed yet.
+\fB\-T\fR \fIretry-timeout\fR
+Retries to connect to existing target after \fIretry-timeout\fR seconds. If -R is not specified, uses 5 Seconds timeout. if retry-timeout is 0, will not try to reconnect. The reason srp_daemon retries to connect to the target is because there may be a rare scnerio in which srp_daemon will try to connect to add a target when the target is about to be removed, but is not removed yet.
 .TP
-\fB\-f\fR \fIrules-File\fR
-Decide to which targets to connect according to the rules in \fIrules-File\fR. 
+\fB\-f\fR \fIrules-file\fR
+Decide to which targets to connect according to the rules in \fIrules-file\fR.
 If \fB\-f\fR is not specified, uses the default rules file @CMAKE_INSTALL_FULL_SYSCONFDIR@/srp_daemon.conf.
-Each line in the \fIrules-File\fR is a rule which can be either an allow connection or a disallow connection according to 
+Each line in the \fIrules-file\fR is a rule which can be either an allow connection or a disallow connection according to
 the first character in the line (a or d accordingly). The rest of the line is values for id_ext, ioc_guid, dgid, 
 service_id. Please take a look at the example section for an example of the file. srp_daemon decide whether to allow or disallow each target according  to first rule that match the target. If no rule matches the target, the target is allowed and will be connected. In an allow rule it is possible to set attributes for the connection to the target. Supported attributes are max_cmd_per_lun and max_sect.
 .TP
@@ -74,7 +87,7 @@ New format - use also initiator_ext in the connection command.
 
 .SH FILES
 @CMAKE_INSTALL_FULL_SYSCONFDIR@/srp_daemon.conf -
-Default rules configuration file that indicates to which targets to connect. Can be overridden using the \fB\-f\fR \fIrules-File\fR option. 
+Default rules configuration file that indicates to which targets to connect. Can be overridden using the \fB\-f\fR \fIrules-file\fR option.
 Each line in this file is a rule which can be either an allow connection or a disallow connection according to 
 the first character in the line (a or d accordingly). The rest of the line is values for id_ext, ioc_guid, dgid, 
 service_id. Please take a look at the example section for an example of the file. srp_daemon decide whether to allow or disallow each target according  to first rule that match the target. If no rule matches the target, the target is allowed and will be connected. In an allow rule it is possible to set attributes for the connection to the target. Supported attributes are max_cmd_per_lun and max_sect.
-- 
2.12.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-core 3/5] srp_daemon: Add command-line option -j

[Bart Van Assche]

This command-line option is used in a later patch to avoid having
to start a shell script from a udev rule.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
---
 srp_daemon/srp_daemon.1.in | 15 +++++++++++----
 srp_daemon/srp_daemon.c    | 16 +++++++++++++++-
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/srp_daemon/srp_daemon.1.in b/srp_daemon/srp_daemon.1.in
index 02e1f2df..82dc3241 100644
--- a/srp_daemon/srp_daemon.1.in
+++ b/srp_daemon/srp_daemon.1.in
@@ -5,7 +5,7 @@
 srp_daemon \- Discovers SRP targets in an InfiniBand Fabric
 
 .SH SYNOPSIS
-.B srp_daemon\fR [\fB-vVcaeon\fR] [\fB-d \fIumad-device\fR | \fB-i \fIinfiniband-device\fR [\fB-p \fIport-num\fR]] [\fB-t \fItimeout(ms)\fR] [\fB-r \fIretries\fR] [\fB-R \fIrescan-time\fR] [\fB-f \fIrules-file\fR]
+.B srp_daemon\fR [\fB-vVcaeon\fR] [\fB-d \fIumad-device\fR | \fB-i \fIinfiniband-device\fR [\fB-p \fIport-num\fR] | \fB-j \fIdev:port\fR] [\fB-t \fItimeout(ms)\fR] [\fB-r \fIretries\fR] [\fB-R \fIrescan-time\fR] [\fB-f \fIrules-file\fR]
 
 
 .SH DESCRIPTION
@@ -41,13 +41,20 @@ Print more verbose output
 Print even more verbose output (debug mode)
 .TP
 \fB\-i\fR \fIinfiniband-device\fR
-Work on \fIinfiniband-device\fR. This option should not be used with -d.
+Work on \fIinfiniband-device\fR. This option should not be used with -d nor
+with -j.
 .TP
 \fB\-p\fR \fIport-num\fR
-Work on port \fIport-num\fR (default 1). This option must be used with -i and should not be used with -d.
+Work on port \fIport-num\fR (default 1). This option must be used with -i and
+should not be used with -d nor with -j.
+.TP
+\fB\-j\fR \fIdev:port\fR
+Work on port number \fIport\fR of InfiniBand device \fIdev\fR. This option
+should not be used with -d, -i nor with -p.
 .TP
 \fB\-d\fR \fIumad-device\fR
-Use device file \fIumad-device\fR (default /dev/infiniband/umad0) This option should not be used with -i or -p.
+Use device file \fIumad-device\fR (default /dev/infiniband/umad0) This option
+should not be used with -i, -p nor with -j.
 .TP
 \fB\-c\fR
 Generate output suitable for piping directly to a
diff --git a/srp_daemon/srp_daemon.c b/srp_daemon/srp_daemon.c
index 9011fe5e..c0e8d23d 100644
--- a/srp_daemon/srp_daemon.c
+++ b/srp_daemon/srp_daemon.c
@@ -226,6 +226,7 @@ static void usage(const char *argv0)
 	fprintf(stderr, "-d <umad device>	use umad Device \n");
 	fprintf(stderr, "-i <infiniband device>	use InfiniBand device \n");
 	fprintf(stderr, "-p <port_num>		use Port num \n");
+	fprintf(stderr, "-j <dev>:<port_num>	use the IB dev / port_num combination \n");
 	fprintf(stderr, "-R <rescan time>	perform complete Rescan every <rescan time> seconds\n");
 	fprintf(stderr, "-T <retry timeout>	Retries to connect to existing target after Timeout of <retry timeout> seconds\n");
 	fprintf(stderr, "-l <tl_retry timeout>	Transport retry count before failing IO. should be in range [2..7], (default 2)\n");
@@ -1623,7 +1624,7 @@ static int get_config(struct config_t *conf, int argc, char *argv[])
 	while (1) {
 		int c;
 
-		c = getopt(argc, argv, "caveod:i:p:t:r:R:T:l:Vhnf:");
+		c = getopt(argc, argv, "caveod:i:j:p:t:r:R:T:l:Vhnf:");
 		if (c == -1)
 			break;
 
@@ -1645,6 +1646,19 @@ static int get_config(struct config_t *conf, int argc, char *argv[])
 				return -1;
 			}
 			break;
+		case 'j': {
+			char dev[32];
+			int port_num;
+
+			if (sscanf(optarg, "%31[^:]:%d", dev, &port_num) != 2) {
+				pr_err("Bad dev:port specification %s\n",
+				       optarg);
+				return -1;
+			}
+			conf->dev_name = strdup(dev);
+			conf->port_num = port_num;
+			}
+			break;
 		case 'c':
 			++conf->cmd;
 			break;
-- 
2.12.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-core 4/5] srp_daemon: Move srp_daemon.service from the redhat directory to the srp_daemon directory

[Bart Van Assche]

Since this systemd unit file works fine on at least one other distro
(openSuSE), move it from the redhat/ directory into the srp_daemon/
directory. Move the code for installing this service from the Red Hat
RPM spec file into srp_daemon/CMakeLists.txt.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
---
 redhat/rdma-core.spec                     | 3 ---
 srp_daemon/CMakeLists.txt                 | 2 ++
 {redhat => srp_daemon}/srp_daemon.service | 0
 3 files changed, 2 insertions(+), 3 deletions(-)
 rename {redhat => srp_daemon}/srp_daemon.service (100%)

diff --git a/redhat/rdma-core.spec b/redhat/rdma-core.spec
index 1095b2fc..993a6c80 100644
--- a/redhat/rdma-core.spec
+++ b/redhat/rdma-core.spec
@@ -273,9 +273,6 @@ bin/ib_acme -D . -O
 install -D -m0644 ibacm_opts.cfg %{buildroot}%{_sysconfdir}/rdma/
 install -D -m0644 redhat/ibacm.service %{buildroot}%{_unitdir}/
 
-# srp_daemon
-install -D -m0644 redhat/srp_daemon.service %{buildroot}%{_unitdir}/
-
 # Delete the package's init.d scripts
 rm -rf %{buildroot}/%{_initrddir}/
 
diff --git a/srp_daemon/CMakeLists.txt b/srp_daemon/CMakeLists.txt
index 726696ed..24e4c30d 100644
--- a/srp_daemon/CMakeLists.txt
+++ b/srp_daemon/CMakeLists.txt
@@ -27,6 +27,8 @@ rdma_subst_install(FILES "srp_daemon.sh.in"
 
 install(FILES srp_daemon.conf DESTINATION "${CMAKE_INSTALL_SYSCONFDIR}")
 
+install(FILES srp_daemon.service DESTINATION "${CMAKE_INSTALL_SYSTEMD_SERVICEDIR}")
+
 # FIXME: The ib init.d file should really be included in rdma-core as well.
 set(RDMA_SERVICE "openibd" CACHE STRING "init.d file service name to order srpd after")
 # NOTE: These defaults are for CentOS, packagers should override.
diff --git a/redhat/srp_daemon.service b/srp_daemon/srp_daemon.service
similarity index 100%
rename from redhat/srp_daemon.service
rename to srp_daemon/srp_daemon.service
-- 
2.12.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-core 5/5] srp_daemon.service: Add support for hot-plugging

[Bart Van Assche]

srp_daemon.service is modified such that instead of starting
/usr/sbin/srp_daemon.sh that it does not start any process. A new
template service is added, namely srp_daemon_port@.service. This
service replaces srp_daemon.sh and controls /usr/sbin/srp_daemon
for a single port. A udev rule is added that instantiates the
srp_daemon_port@.service every time an RDMA port is hot-added.
Since the per-port service depends on the srp_daemon service,
starting or stopping the srp_daemon service affects all per-port
services.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
---
 debian/srptools.install               |  5 ++++
 redhat/rdma-core.spec                 |  5 +++-
 srp_daemon/90-srp-daemon.rules        |  1 +
 srp_daemon/CMakeLists.txt             |  5 ++++
 srp_daemon/srp_daemon.service         | 12 ++++-----
 srp_daemon/srp_daemon.service.5       | 30 +++++++++++++++++++++++
 srp_daemon/srp_daemon_port@.service   | 18 ++++++++++++++
 srp_daemon/srp_daemon_port@.service.5 | 46 +++++++++++++++++++++++++++++++++++
 8 files changed, 114 insertions(+), 8 deletions(-)
 create mode 100644 srp_daemon/90-srp-daemon.rules
 create mode 100644 srp_daemon/srp_daemon.service.5
 create mode 100644 srp_daemon/srp_daemon_port@.service
 create mode 100644 srp_daemon/srp_daemon_port@.service.5

diff --git a/debian/srptools.install b/debian/srptools.install
index ecec5d9a..81dbd4cd 100644
--- a/debian/srptools.install
+++ b/debian/srptools.install
@@ -1,6 +1,11 @@
 etc/srp_daemon.conf
+lib/systemd/system/srp_daemon.service
+lib/systemd/system/srp_daemon_port@.service
+lib/udev/rules.d/90-srp-daemon.rules
 usr/sbin/ibsrpdm
 usr/sbin/srp_daemon
 usr/share/doc/rdma-core/ibsrpdm.md usr/share/doc/srptools/
 usr/share/man/man1/ibsrpdm.1
 usr/share/man/man1/srp_daemon.1
+usr/share/man/man5/srp_daemon.service.5
+usr/share/man/man5/srp_daemon_port@.service.5
diff --git a/redhat/rdma-core.spec b/redhat/rdma-core.spec
index 993a6c80..fc9edaa1 100644
--- a/redhat/rdma-core.spec
+++ b/redhat/rdma-core.spec
@@ -439,10 +439,13 @@ rm -rf %{buildroot}/%{_initrddir}/
 %files -n srp_daemon
 %config(noreplace) %{_sysconfdir}/srp_daemon.conf
 %{_unitdir}/srp_daemon.service
+%{_unitdir}/srp_daemon_port@.service
 %{_sbindir}/ibsrpdm
 %{_sbindir}/srp_daemon
-%{_sbindir}/srp_daemon.sh
 %{_sbindir}/run_srp_daemon
+%{_udevrulesdir}/90-srp-daemon.rules
 %{_mandir}/man1/ibsrpdm.1*
 %{_mandir}/man1/srp_daemon.1*
+%{_mandir}/man5/srp_daemon.service.5*
+%{_mandir}/man5/srp_daemon_port@.service.5*
 %doc %{_docdir}/%{name}-%{version}/ibsrpdm.md
diff --git a/srp_daemon/90-srp-daemon.rules b/srp_daemon/90-srp-daemon.rules
new file mode 100644
index 00000000..6d6788f0
--- /dev/null
+++ b/srp_daemon/90-srp-daemon.rules
@@ -0,0 +1 @@
+ACTION=="add", SUBSYSTEM=="infiniband_mad", PROGRAM:="/usr/bin/systemctl show srp_daemon -p UnitFileState", RESULT=="UnitFileState=enabled", TAG+="systemd", ENV{SYSTEMD_WANTS}+="srp_daemon_port@$attr{ibdev}:$attr{port}.service"
diff --git a/srp_daemon/CMakeLists.txt b/srp_daemon/CMakeLists.txt
index 24e4c30d..39f4bc99 100644
--- a/srp_daemon/CMakeLists.txt
+++ b/srp_daemon/CMakeLists.txt
@@ -3,6 +3,8 @@ set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${NO_STRICT_ALIASING_FLAGS}")
 rdma_man_pages(
   ibsrpdm.1
   srp_daemon.1.in
+  srp_daemon.service.5
+  srp_daemon_port@.service.5
   )
 
 rdma_sbin_executable(srp_daemon
@@ -28,6 +30,9 @@ rdma_subst_install(FILES "srp_daemon.sh.in"
 install(FILES srp_daemon.conf DESTINATION "${CMAKE_INSTALL_SYSCONFDIR}")
 
 install(FILES srp_daemon.service DESTINATION "${CMAKE_INSTALL_SYSTEMD_SERVICEDIR}")
+install(FILES srp_daemon_port@.service DESTINATION "${CMAKE_INSTALL_SYSTEMD_SERVICEDIR}")
+
+install(FILES 90-srp-daemon.rules DESTINATION "${CMAKE_INSTALL_UDEV_RULESDIR}")
 
 # FIXME: The ib init.d file should really be included in rdma-core as well.
 set(RDMA_SERVICE "openibd" CACHE STRING "init.d file service name to order srpd after")
diff --git a/srp_daemon/srp_daemon.service b/srp_daemon/srp_daemon.service
index 9510f5fb..5bcb1d53 100644
--- a/srp_daemon/srp_daemon.service
+++ b/srp_daemon/srp_daemon.service
@@ -1,17 +1,15 @@
 [Unit]
-Description=Start or stop the daemon that attaches to SRP devices
+Description=Daemon that discovers and logs in to SRP target systems
 Documentation=man:srp_daemon file:/etc/rdma/rdma.conf file:/etc/srp_daemon.conf
 DefaultDependencies=false
 Conflicts=emergency.target emergency.service
-Requires=rdma.service
-Wants=opensm.service
-After=rdma.service opensm.service
-After=network.target
 Before=remote-fs-pre.target
 
 [Service]
-Type=simple
-ExecStart=/usr/sbin/srp_daemon.sh
+Type=oneshot
+RemainAfterExit=yes
+ExecStart=/bin/true
+ExecStop=/bin/true
 
 [Install]
 WantedBy=remote-fs-pre.target
diff --git a/srp_daemon/srp_daemon.service.5 b/srp_daemon/srp_daemon.service.5
new file mode 100644
index 00000000..a6b25d6a
--- /dev/null
+++ b/srp_daemon/srp_daemon.service.5
@@ -0,0 +1,30 @@
+'\" t
+.TH "SRP_DAEMON\&.SERVICE" "5" "" "srp_daemon" "srp_daemon.service"
+.\" -----------------------------------------------------------------
+.\" * set default formatting
+.\" -----------------------------------------------------------------
+.\" disable hyphenation
+.nh
+.\" disable justification (adjust text to left margin only)
+.ad l
+.\" -----------------------------------------------------------------
+.\" * MAIN CONTENT STARTS HERE *
+.\" -----------------------------------------------------------------
+.SH "NAME"
+srp_daemon.service \- srp_daemon systemd service that controls all ports
+.SH "SYNOPSIS"
+.PP
+srp_daemon\&.service
+.SH "DESCRIPTION"
+.PP
+The srp_daemon\&.service controls whether or not any srp_daemon processes are
+running. Although no srp_daemon processes are controlled directly by the
+srp_daemon\&.service, this service controls whether or not any
+srp_daemon_port@\&.service are allowed to be active. Each
+srp_daemon_port@\&.service controls one srp_daemon process.
+
+.SH "SEE ALSO"
+.PP
+\fBsrp_daemon\fR(1),
+\fBsrp_daemon_port@.service\fR(5),
+\fBsystemctl\fR(1)
diff --git a/srp_daemon/srp_daemon_port@.service b/srp_daemon/srp_daemon_port@.service
new file mode 100644
index 00000000..666400ab
--- /dev/null
+++ b/srp_daemon/srp_daemon_port@.service
@@ -0,0 +1,18 @@
+[Unit]
+Description=SRP daemon that monitors port %i
+Documentation=man:srp_daemon file:/etc/rdma/rdma.conf file:/etc/srp_daemon.conf
+DefaultDependencies=false
+Conflicts=emergency.target emergency.service
+Requires=rdma.service
+Wants=opensm.service
+After=rdma.service opensm.service srp_daemon.service
+After=network.target
+Before=remote-fs-pre.target
+BindsTo=srp_daemon.service
+
+[Service]
+Type=simple
+ExecStart=/usr/sbin/srp_daemon -e -c -n -j %I -R 60
+
+[Install]
+WantedBy=remote-fs-pre.target
diff --git a/srp_daemon/srp_daemon_port@.service.5 b/srp_daemon/srp_daemon_port@.service.5
new file mode 100644
index 00000000..a755b2eb
--- /dev/null
+++ b/srp_daemon/srp_daemon_port@.service.5
@@ -0,0 +1,46 @@
+'\" t
+.TH "SRP_DAEMON_PORT@\&.SERVICE" "5" "" "srp_daemon" "srp_daemon_port@.service"
+.\" -----------------------------------------------------------------
+.\" * set default formatting
+.\" -----------------------------------------------------------------
+.\" disable hyphenation
+.nh
+.\" disable justification (adjust text to left margin only)
+.ad l
+.\" -----------------------------------------------------------------
+.\" * MAIN CONTENT STARTS HERE *
+.\" -----------------------------------------------------------------
+.SH "NAME"
+srp_daemon_port@.service \- srp_daemon_port@ systemd service that controls a
+single port
+.SH "SYNOPSIS"
+.PP
+srp_daemon_port@\&.service
+.SH "DESCRIPTION"
+.PP
+The srp_daemon_port@\&.service controls whether or not an srp_daemon process
+is monitoring the RDMA port specified as template argument. The format for the
+RDMA port name is \fIdev:port\fR where \fIdev\fR is the name of an RDMA device
+and \fIport\fR is an port number starting from one. Starting an instance of
+this template will start an srp_daemon process. Stopping an instance of this
+template will stop the srp_daemon process for the specified port.  Here is an
+example of how to obtain a list of all RDMA device and port number pairs:
+.PP
+.nf
+.RS
+$ for p in /sys/class/infiniband/*/ports/*; do
+    echo $p | sed 's,/sys/class/infiniband/,,;s,/ports/,:,'
+  done
+mlx4_0:1
+mlx4_0:2
+mlx4_1:1
+mlx4_1:2
+.RE
+.fi
+.PP
+
+.SH "SEE ALSO"
+.PP
+\fBsrp_daemon\fR(1),
+\fBsrp_daemon.service\fR(5),
+\fBsystemctl\fR(1)
-- 
2.12.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-core 0/5] srp_daemon: Rework systemd integration

[Bart Van Assche]

Hello,

This patch series implements the changes that were discussed a few days
ago on the linux-rdma mailing list:
- srp_daemon.service is split into srp_daemon.service and the
  srp_daemon_port@.service template. The former does not start any srp_daemon
  process but controls the latter.
- srp_daemon_port@.service  instances are started automatically upon hot-add of
  an RDMA port to support hot-plugging.

As usual, comments and feedback are welcome.

Bart.

Bart Van Assche (5):
  srp_daemon.sh: Improve robustness
  srp_daemon: Use the recommended style in the man page
  srp_daemon: Add command-line option -j
  srp_daemon: Move srp_daemon.service from the redhat directory to the
    srp_daemon directory
  srp_daemon.service: Add support for hot-plugging

 debian/srptools.install                            |  5 ++
 redhat/rdma-core.spec                              |  8 ++--
 srp_daemon/90-srp-daemon.rules                     |  1 +
 srp_daemon/CMakeLists.txt                          |  7 +++
 srp_daemon/srp_daemon.1.in                         | 54 +++++++++++++++-------
 srp_daemon/srp_daemon.c                            | 16 ++++++-
 srp_daemon/srp_daemon.service                      | 15 ++++++
 srp_daemon/srp_daemon.service.5                    | 30 ++++++++++++
 srp_daemon/srp_daemon.sh.in                        | 34 +++++++-------
 .../srp_daemon_port@.service                       |  7 +--
 srp_daemon/srp_daemon_port@.service.5              | 46 ++++++++++++++++++
 11 files changed, 181 insertions(+), 42 deletions(-)
 create mode 100644 srp_daemon/90-srp-daemon.rules
 create mode 100644 srp_daemon/srp_daemon.service
 create mode 100644 srp_daemon/srp_daemon.service.5
 rename redhat/srp_daemon.service => srp_daemon/srp_daemon_port@.service (63%)
 create mode 100644 srp_daemon/srp_daemon_port@.service.5

-- 
2.12.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-core 1/5] srp_daemon.sh: Improve robustness

[Bart Van Assche]

Surround expansions of shell variables that can contain spaces with
double quotes. Use arrays instead of a space-separated strings to
store lists to avoid that strings that contain whitespace get split
when iterating over a list.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
---
 srp_daemon/srp_daemon.sh.in | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/srp_daemon/srp_daemon.sh.in b/srp_daemon/srp_daemon.sh.in
index cb0b81ef..75e8a31b 100755
--- a/srp_daemon/srp_daemon.sh.in
+++ b/srp_daemon/srp_daemon.sh.in
@@ -31,35 +31,35 @@
 shopt -s nullglob
 
 prog=@CMAKE_INSTALL_FULL_SBINDIR@/srp_daemon
-params=$@
+params=("$@")
 ibdir="/sys/class/infiniband"
 rescan_interval=60
-pids=""
-pidfile=@CMAKE_INSTALL_FULL_RUNDIR@/srp_daemon.sh.pid
+pids=()
+pidfile="@CMAKE_INSTALL_FULL_RUNDIR@/srp_daemon.sh.pid"
 mypid=$$
 
 trap_handler()
 {
-    if [ -n "$pids" ]; then
-        kill -15 $pids > /dev/null 2>&1
-        wait $pids
+    if [ "${#pids[@]}" ]; then
+        kill -15 "${pids[@]}" > /dev/null 2>&1
+        wait "${pids[@]}"
     fi
-    logger -i -t "$(basename $0)" "killing $prog."
-    /bin/rm -f $pidfile
+    logger -i -t "$(basename "$0")" "killing $prog."
+    /bin/rm -f "$pidfile"
     exit 0
 }
 
 # Check if there is another copy running of srp_daemon.sh
-if [ -f $pidfile ]; then
-    if [ -e /proc/$(cat $pidfile 2>/dev/null)/status ]; then
-        echo "$(basename $0) is already running. Exiting."
+if [ -f "$pidfile" ]; then
+    if [ -e "/proc/$(cat "$pidfile" 2>/dev/null)/status" ]; then
+        echo "$(basename "$0") is already running. Exiting."
         exit 1
     else
-        /bin/rm -f $pidfile
+        /bin/rm -f "$pidfile"
     fi
 fi
 
-if ! echo $mypid > $pidfile; then
+if ! echo $mypid > "$pidfile"; then
     echo "Creating $pidfile for pid $mypid failed"
     exit 1
 fi
@@ -72,12 +72,12 @@ do
 done
 
 for d in ${ibdir}_mad/umad*; do
-    hca_id="$(<$d/ibdev)"
-    port="$(<$d/port)"
+    hca_id="$(<"$d/ibdev")"
+    port="$(<"$d/port")"
     add_target="${ibdir}_srp/srp-${hca_id}-${port}/add_target"
     if [ -e "${add_target}" ]; then
-        ${prog} -e -c -n -i ${hca_id} -p ${port} -R ${rescan_interval} ${params} >/dev/null 2>&1 &
-        pids="$pids $!"
+        ${prog} -e -c -n -i "${hca_id}" -p "${port}" -R "${rescan_interval}" "${params[@]}" >/dev/null 2>&1 &
+        pids+=($!)
     fi
 done
 
-- 
2.12.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-core 2/5] srp_daemon: Use the recommended style in the man page

[Bart Van Assche]

>From https://liw.fi/manpages/, about the SYNOPSIS section:

Font usage is important here, and carries information. All the
parts that are in bold are things that the user is expected to
write verbatim. Italic indicates values the user is expected to
fill in. Normal font is used for syntax meta-characters: for
the brackets that indicate optionality, and the ellipsis that
indicates repetition.

Additionally, change the identifiers that refer to arguments to
lower case.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
---
 srp_daemon/srp_daemon.1.in | 41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/srp_daemon/srp_daemon.1.in b/srp_daemon/srp_daemon.1.in
index 008d53a2..02e1f2df 100644
--- a/srp_daemon/srp_daemon.1.in
+++ b/srp_daemon/srp_daemon.1.in
@@ -5,18 +5,30 @@
 srp_daemon \- Discovers SRP targets in an InfiniBand Fabric
 
 .SH SYNOPSIS
-.B srp_daemon [-vVcaeon] [-d \fIumad-device\fR | -i \fIinfiniband-device\fR [-p \fIport-num\fR]] [-t \fItimeout(ms)\fR] [-r \fIretries\fR] [-R \fIRescan-time\fR] [-f \fIrules-File\fR]
+.B srp_daemon\fR [\fB-vVcaeon\fR] [\fB-d \fIumad-device\fR | \fB-i \fIinfiniband-device\fR [\fB-p \fIport-num\fR]] [\fB-t \fItimeout(ms)\fR] [\fB-r \fIretries\fR] [\fB-R \fIrescan-time\fR] [\fB-f \fIrules-file\fR]
 
 
 .SH DESCRIPTION
 .PP
 Discovers and connects to InfiniBand SCSI RDMA Protocol (SRP) targets in an IB fabric.
 
-Each srp_daemon instance operates on one local port. Upon boot it performs a full rescan of the fabric then waits for an srp_daemon event. An srp_daemon event can be a join of a new machine to the fabric, a change in the capabilities of a machine, an SA change, or an expiration of a predefined timeout.
+Each srp_daemon instance operates on one local port. Upon boot it performs a
+full rescan of the fabric and then waits for an srp_daemon event. An
+srp_daemon event can be a join of a new machine to the fabric, a change in the
+capabilities of a machine, an SA change, or an expiration of a predefined
+timeout.
 
-When a new machine joins the fabric, srp_daemon checks if it is a target. When there is a change of capabilities, srp_daemon checks if the machine has turned into a target. When there is an SA change or a timeout expiration, srp_daemon performs a full rescan of the fabric.
+When a new machine joins the fabric, srp_daemon checks if it is an SRP
+target. When there is a change of capabilities, srp_daemon checks if the
+machine has turned into an SRP target. When there is an SA change or a timeout
+expiration, srp_daemon performs a full rescan of the fabric.
 
-For each target srp_daemon finds, it checks if it should connect to this target according to its rules (default rules file is @CMAKE_INSTALL_FULL_SYSCONFDIR@/srp_daemon.conf) and if it is already connected to the local port. If it should connect to this target and if it is not connected yet, srp_daemon can either print the target details or connect to it.
+For each target srp_daemon finds, it checks if it should connect to this
+target according to its rules (the default rules file is
+@CMAKE_INSTALL_FULL_SYSCONFDIR@/srp_daemon.conf) and if it is already
+connected to the local port. If it should connect to this target and if it is
+not connected yet, srp_daemon can either print the target details or connect
+to it.
 
 .SH OPTIONS
 
@@ -42,24 +54,25 @@ Generate output suitable for piping directly to a
 /sys/class/infiniband_srp/srp\-<device>\-<port>/add_target file. 
 .TP
 \fB\-a\fR
-Prints all the targets in the fabric, not only targets that are not connected through the local port. (The same as ibsrpdm.)
+Prints all the targets in the fabric, not only targets that are not connected
+through the local port. This is the same behavior as that of ibsrpdm.
 .TP
 \fB\-e\fR
 Execute the connection command, i.e., make the connection to the target.
 .TP
 \fB\-o\fR
-Perform only one rescan and exit. (The same as ibsrpdm.)
+Perform only one rescan and exit just like ibsrpdm.
 .TP
-\fB\-R\fR \fIRescan-time\fR
-Force a complete rescan every \fIRescan-time\fR seconds. If -R is not specified, no timeout rescans will be performed.
+\fB\-R\fR \fIrescan-time\fR
+Force a complete rescan every \fIrescan-time\fR seconds. If -R is not specified, no timeout rescans will be performed.
 .TP
-\fB\-T\fR \fIretry-Timeout\fR
-Retries to connect to existing target after \fIretry-Timeout\fR seconds. If -R is not specified, uses 5 Seconds timeout. if retry-Timeout is 0, will not try to reconnect. The reason srp_daemon retries to connect to the target is because there may be a rare scnerio in which srp_daemon will try to connect to add a target when the target is about to be removed, but is not removed yet.
+\fB\-T\fR \fIretry-timeout\fR
+Retries to connect to existing target after \fIretry-timeout\fR seconds. If -R is not specified, uses 5 Seconds timeout. if retry-timeout is 0, will not try to reconnect. The reason srp_daemon retries to connect to the target is because there may be a rare scnerio in which srp_daemon will try to connect to add a target when the target is about to be removed, but is not removed yet.
 .TP
-\fB\-f\fR \fIrules-File\fR
-Decide to which targets to connect according to the rules in \fIrules-File\fR. 
+\fB\-f\fR \fIrules-file\fR
+Decide to which targets to connect according to the rules in \fIrules-file\fR.
 If \fB\-f\fR is not specified, uses the default rules file @CMAKE_INSTALL_FULL_SYSCONFDIR@/srp_daemon.conf.
-Each line in the \fIrules-File\fR is a rule which can be either an allow connection or a disallow connection according to 
+Each line in the \fIrules-file\fR is a rule which can be either an allow connection or a disallow connection according to
 the first character in the line (a or d accordingly). The rest of the line is values for id_ext, ioc_guid, dgid, 
 service_id. Please take a look at the example section for an example of the file. srp_daemon decide whether to allow or disallow each target according  to first rule that match the target. If no rule matches the target, the target is allowed and will be connected. In an allow rule it is possible to set attributes for the connection to the target. Supported attributes are max_cmd_per_lun and max_sect.
 .TP
@@ -74,7 +87,7 @@ New format - use also initiator_ext in the connection command.
 
 .SH FILES
 @CMAKE_INSTALL_FULL_SYSCONFDIR@/srp_daemon.conf -
-Default rules configuration file that indicates to which targets to connect. Can be overridden using the \fB\-f\fR \fIrules-File\fR option. 
+Default rules configuration file that indicates to which targets to connect. Can be overridden using the \fB\-f\fR \fIrules-file\fR option.
 Each line in this file is a rule which can be either an allow connection or a disallow connection according to 
 the first character in the line (a or d accordingly). The rest of the line is values for id_ext, ioc_guid, dgid, 
 service_id. Please take a look at the example section for an example of the file. srp_daemon decide whether to allow or disallow each target according  to first rule that match the target. If no rule matches the target, the target is allowed and will be connected. In an allow rule it is possible to set attributes for the connection to the target. Supported attributes are max_cmd_per_lun and max_sect.
-- 
2.12.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-core 3/5] srp_daemon: Add command-line option -j

[Bart Van Assche]

This command-line option is used in a later patch to avoid having
to start a shell script from a udev rule.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
---
 srp_daemon/srp_daemon.1.in | 15 +++++++++++----
 srp_daemon/srp_daemon.c    | 16 +++++++++++++++-
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/srp_daemon/srp_daemon.1.in b/srp_daemon/srp_daemon.1.in
index 02e1f2df..82dc3241 100644
--- a/srp_daemon/srp_daemon.1.in
+++ b/srp_daemon/srp_daemon.1.in
@@ -5,7 +5,7 @@
 srp_daemon \- Discovers SRP targets in an InfiniBand Fabric
 
 .SH SYNOPSIS
-.B srp_daemon\fR [\fB-vVcaeon\fR] [\fB-d \fIumad-device\fR | \fB-i \fIinfiniband-device\fR [\fB-p \fIport-num\fR]] [\fB-t \fItimeout(ms)\fR] [\fB-r \fIretries\fR] [\fB-R \fIrescan-time\fR] [\fB-f \fIrules-file\fR]
+.B srp_daemon\fR [\fB-vVcaeon\fR] [\fB-d \fIumad-device\fR | \fB-i \fIinfiniband-device\fR [\fB-p \fIport-num\fR] | \fB-j \fIdev:port\fR] [\fB-t \fItimeout(ms)\fR] [\fB-r \fIretries\fR] [\fB-R \fIrescan-time\fR] [\fB-f \fIrules-file\fR]
 
 
 .SH DESCRIPTION
@@ -41,13 +41,20 @@ Print more verbose output
 Print even more verbose output (debug mode)
 .TP
 \fB\-i\fR \fIinfiniband-device\fR
-Work on \fIinfiniband-device\fR. This option should not be used with -d.
+Work on \fIinfiniband-device\fR. This option should not be used with -d nor
+with -j.
 .TP
 \fB\-p\fR \fIport-num\fR
-Work on port \fIport-num\fR (default 1). This option must be used with -i and should not be used with -d.
+Work on port \fIport-num\fR (default 1). This option must be used with -i and
+should not be used with -d nor with -j.
+.TP
+\fB\-j\fR \fIdev:port\fR
+Work on port number \fIport\fR of InfiniBand device \fIdev\fR. This option
+should not be used with -d, -i nor with -p.
 .TP
 \fB\-d\fR \fIumad-device\fR
-Use device file \fIumad-device\fR (default /dev/infiniband/umad0) This option should not be used with -i or -p.
+Use device file \fIumad-device\fR (default /dev/infiniband/umad0) This option
+should not be used with -i, -p nor with -j.
 .TP
 \fB\-c\fR
 Generate output suitable for piping directly to a
diff --git a/srp_daemon/srp_daemon.c b/srp_daemon/srp_daemon.c
index 9011fe5e..c0e8d23d 100644
--- a/srp_daemon/srp_daemon.c
+++ b/srp_daemon/srp_daemon.c
@@ -226,6 +226,7 @@ static void usage(const char *argv0)
 	fprintf(stderr, "-d <umad device>	use umad Device \n");
 	fprintf(stderr, "-i <infiniband device>	use InfiniBand device \n");
 	fprintf(stderr, "-p <port_num>		use Port num \n");
+	fprintf(stderr, "-j <dev>:<port_num>	use the IB dev / port_num combination \n");
 	fprintf(stderr, "-R <rescan time>	perform complete Rescan every <rescan time> seconds\n");
 	fprintf(stderr, "-T <retry timeout>	Retries to connect to existing target after Timeout of <retry timeout> seconds\n");
 	fprintf(stderr, "-l <tl_retry timeout>	Transport retry count before failing IO. should be in range [2..7], (default 2)\n");
@@ -1623,7 +1624,7 @@ static int get_config(struct config_t *conf, int argc, char *argv[])
 	while (1) {
 		int c;
 
-		c = getopt(argc, argv, "caveod:i:p:t:r:R:T:l:Vhnf:");
+		c = getopt(argc, argv, "caveod:i:j:p:t:r:R:T:l:Vhnf:");
 		if (c == -1)
 			break;
 
@@ -1645,6 +1646,19 @@ static int get_config(struct config_t *conf, int argc, char *argv[])
 				return -1;
 			}
 			break;
+		case 'j': {
+			char dev[32];
+			int port_num;
+
+			if (sscanf(optarg, "%31[^:]:%d", dev, &port_num) != 2) {
+				pr_err("Bad dev:port specification %s\n",
+				       optarg);
+				return -1;
+			}
+			conf->dev_name = strdup(dev);
+			conf->port_num = port_num;
+			}
+			break;
 		case 'c':
 			++conf->cmd;
 			break;
-- 
2.12.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-core 4/5] srp_daemon: Move srp_daemon.service from the redhat directory to the srp_daemon directory

[Bart Van Assche]

Since this systemd unit file works fine on at least one other distro
(openSuSE), move it from the redhat/ directory into the srp_daemon/
directory. Move the code for installing this service from the Red Hat
RPM spec file into srp_daemon/CMakeLists.txt.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
---
 redhat/rdma-core.spec                     | 3 ---
 srp_daemon/CMakeLists.txt                 | 2 ++
 {redhat => srp_daemon}/srp_daemon.service | 0
 3 files changed, 2 insertions(+), 3 deletions(-)
 rename {redhat => srp_daemon}/srp_daemon.service (100%)

diff --git a/redhat/rdma-core.spec b/redhat/rdma-core.spec
index 1095b2fc..993a6c80 100644
--- a/redhat/rdma-core.spec
+++ b/redhat/rdma-core.spec
@@ -273,9 +273,6 @@ bin/ib_acme -D . -O
 install -D -m0644 ibacm_opts.cfg %{buildroot}%{_sysconfdir}/rdma/
 install -D -m0644 redhat/ibacm.service %{buildroot}%{_unitdir}/
 
-# srp_daemon
-install -D -m0644 redhat/srp_daemon.service %{buildroot}%{_unitdir}/
-
 # Delete the package's init.d scripts
 rm -rf %{buildroot}/%{_initrddir}/
 
diff --git a/srp_daemon/CMakeLists.txt b/srp_daemon/CMakeLists.txt
index 726696ed..24e4c30d 100644
--- a/srp_daemon/CMakeLists.txt
+++ b/srp_daemon/CMakeLists.txt
@@ -27,6 +27,8 @@ rdma_subst_install(FILES "srp_daemon.sh.in"
 
 install(FILES srp_daemon.conf DESTINATION "${CMAKE_INSTALL_SYSCONFDIR}")
 
+install(FILES srp_daemon.service DESTINATION "${CMAKE_INSTALL_SYSTEMD_SERVICEDIR}")
+
 # FIXME: The ib init.d file should really be included in rdma-core as well.
 set(RDMA_SERVICE "openibd" CACHE STRING "init.d file service name to order srpd after")
 # NOTE: These defaults are for CentOS, packagers should override.
diff --git a/redhat/srp_daemon.service b/srp_daemon/srp_daemon.service
similarity index 100%
rename from redhat/srp_daemon.service
rename to srp_daemon/srp_daemon.service
-- 
2.12.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-core 5/5] srp_daemon.service: Add support for hot-plugging

[Bart Van Assche]

srp_daemon.service is modified such that instead of starting
/usr/sbin/srp_daemon.sh that it does not start any process. A new
template service is added, namely srp_daemon_port@.service. This
service replaces srp_daemon.sh and controls /usr/sbin/srp_daemon
for a single port. A udev rule is added that instantiates the
srp_daemon_port@.service every time an RDMA port is hot-added.
Since the per-port service depends on the srp_daemon service,
starting or stopping the srp_daemon service affects all per-port
services.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
---
 debian/srptools.install               |  5 ++++
 redhat/rdma-core.spec                 |  5 +++-
 srp_daemon/90-srp-daemon.rules        |  1 +
 srp_daemon/CMakeLists.txt             |  5 ++++
 srp_daemon/srp_daemon.service         | 12 ++++-----
 srp_daemon/srp_daemon.service.5       | 30 +++++++++++++++++++++++
 srp_daemon/srp_daemon_port@.service   | 18 ++++++++++++++
 srp_daemon/srp_daemon_port@.service.5 | 46 +++++++++++++++++++++++++++++++++++
 8 files changed, 114 insertions(+), 8 deletions(-)
 create mode 100644 srp_daemon/90-srp-daemon.rules
 create mode 100644 srp_daemon/srp_daemon.service.5
 create mode 100644 srp_daemon/srp_daemon_port@.service
 create mode 100644 srp_daemon/srp_daemon_port@.service.5

diff --git a/debian/srptools.install b/debian/srptools.install
index ecec5d9a..81dbd4cd 100644
--- a/debian/srptools.install
+++ b/debian/srptools.install
@@ -1,6 +1,11 @@
 etc/srp_daemon.conf
+lib/systemd/system/srp_daemon.service
+lib/systemd/system/srp_daemon_port@.service
+lib/udev/rules.d/90-srp-daemon.rules
 usr/sbin/ibsrpdm
 usr/sbin/srp_daemon
 usr/share/doc/rdma-core/ibsrpdm.md usr/share/doc/srptools/
 usr/share/man/man1/ibsrpdm.1
 usr/share/man/man1/srp_daemon.1
+usr/share/man/man5/srp_daemon.service.5
+usr/share/man/man5/srp_daemon_port@.service.5
diff --git a/redhat/rdma-core.spec b/redhat/rdma-core.spec
index 993a6c80..fc9edaa1 100644
--- a/redhat/rdma-core.spec
+++ b/redhat/rdma-core.spec
@@ -439,10 +439,13 @@ rm -rf %{buildroot}/%{_initrddir}/
 %files -n srp_daemon
 %config(noreplace) %{_sysconfdir}/srp_daemon.conf
 %{_unitdir}/srp_daemon.service
+%{_unitdir}/srp_daemon_port@.service
 %{_sbindir}/ibsrpdm
 %{_sbindir}/srp_daemon
-%{_sbindir}/srp_daemon.sh
 %{_sbindir}/run_srp_daemon
+%{_udevrulesdir}/90-srp-daemon.rules
 %{_mandir}/man1/ibsrpdm.1*
 %{_mandir}/man1/srp_daemon.1*
+%{_mandir}/man5/srp_daemon.service.5*
+%{_mandir}/man5/srp_daemon_port@.service.5*
 %doc %{_docdir}/%{name}-%{version}/ibsrpdm.md
diff --git a/srp_daemon/90-srp-daemon.rules b/srp_daemon/90-srp-daemon.rules
new file mode 100644
index 00000000..6d6788f0
--- /dev/null
+++ b/srp_daemon/90-srp-daemon.rules
@@ -0,0 +1 @@
+ACTION=="add", SUBSYSTEM=="infiniband_mad", PROGRAM:="/usr/bin/systemctl show srp_daemon -p UnitFileState", RESULT=="UnitFileState=enabled", TAG+="systemd", ENV{SYSTEMD_WANTS}+="srp_daemon_port@$attr{ibdev}:$attr{port}.service"
diff --git a/srp_daemon/CMakeLists.txt b/srp_daemon/CMakeLists.txt
index 24e4c30d..39f4bc99 100644
--- a/srp_daemon/CMakeLists.txt
+++ b/srp_daemon/CMakeLists.txt
@@ -3,6 +3,8 @@ set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${NO_STRICT_ALIASING_FLAGS}")
 rdma_man_pages(
   ibsrpdm.1
   srp_daemon.1.in
+  srp_daemon.service.5
+  srp_daemon_port@.service.5
   )
 
 rdma_sbin_executable(srp_daemon
@@ -28,6 +30,9 @@ rdma_subst_install(FILES "srp_daemon.sh.in"
 install(FILES srp_daemon.conf DESTINATION "${CMAKE_INSTALL_SYSCONFDIR}")
 
 install(FILES srp_daemon.service DESTINATION "${CMAKE_INSTALL_SYSTEMD_SERVICEDIR}")
+install(FILES srp_daemon_port@.service DESTINATION "${CMAKE_INSTALL_SYSTEMD_SERVICEDIR}")
+
+install(FILES 90-srp-daemon.rules DESTINATION "${CMAKE_INSTALL_UDEV_RULESDIR}")
 
 # FIXME: The ib init.d file should really be included in rdma-core as well.
 set(RDMA_SERVICE "openibd" CACHE STRING "init.d file service name to order srpd after")
diff --git a/srp_daemon/srp_daemon.service b/srp_daemon/srp_daemon.service
index 9510f5fb..5bcb1d53 100644
--- a/srp_daemon/srp_daemon.service
+++ b/srp_daemon/srp_daemon.service
@@ -1,17 +1,15 @@
 [Unit]
-Description=Start or stop the daemon that attaches to SRP devices
+Description=Daemon that discovers and logs in to SRP target systems
 Documentation=man:srp_daemon file:/etc/rdma/rdma.conf file:/etc/srp_daemon.conf
 DefaultDependencies=false
 Conflicts=emergency.target emergency.service
-Requires=rdma.service
-Wants=opensm.service
-After=rdma.service opensm.service
-After=network.target
 Before=remote-fs-pre.target
 
 [Service]
-Type=simple
-ExecStart=/usr/sbin/srp_daemon.sh
+Type=oneshot
+RemainAfterExit=yes
+ExecStart=/bin/true
+ExecStop=/bin/true
 
 [Install]
 WantedBy=remote-fs-pre.target
diff --git a/srp_daemon/srp_daemon.service.5 b/srp_daemon/srp_daemon.service.5
new file mode 100644
index 00000000..a6b25d6a
--- /dev/null
+++ b/srp_daemon/srp_daemon.service.5
@@ -0,0 +1,30 @@
+'\" t
+.TH "SRP_DAEMON\&.SERVICE" "5" "" "srp_daemon" "srp_daemon.service"
+.\" -----------------------------------------------------------------
+.\" * set default formatting
+.\" -----------------------------------------------------------------
+.\" disable hyphenation
+.nh
+.\" disable justification (adjust text to left margin only)
+.ad l
+.\" -----------------------------------------------------------------
+.\" * MAIN CONTENT STARTS HERE *
+.\" -----------------------------------------------------------------
+.SH "NAME"
+srp_daemon.service \- srp_daemon systemd service that controls all ports
+.SH "SYNOPSIS"
+.PP
+srp_daemon\&.service
+.SH "DESCRIPTION"
+.PP
+The srp_daemon\&.service controls whether or not any srp_daemon processes are
+running. Although no srp_daemon processes are controlled directly by the
+srp_daemon\&.service, this service controls whether or not any
+srp_daemon_port@\&.service are allowed to be active. Each
+srp_daemon_port@\&.service controls one srp_daemon process.
+
+.SH "SEE ALSO"
+.PP
+\fBsrp_daemon\fR(1),
+\fBsrp_daemon_port@.service\fR(5),
+\fBsystemctl\fR(1)
diff --git a/srp_daemon/srp_daemon_port@.service b/srp_daemon/srp_daemon_port@.service
new file mode 100644
index 00000000..666400ab
--- /dev/null
+++ b/srp_daemon/srp_daemon_port@.service
@@ -0,0 +1,18 @@
+[Unit]
+Description=SRP daemon that monitors port %i
+Documentation=man:srp_daemon file:/etc/rdma/rdma.conf file:/etc/srp_daemon.conf
+DefaultDependencies=false
+Conflicts=emergency.target emergency.service
+Requires=rdma.service
+Wants=opensm.service
+After=rdma.service opensm.service srp_daemon.service
+After=network.target
+Before=remote-fs-pre.target
+BindsTo=srp_daemon.service
+
+[Service]
+Type=simple
+ExecStart=/usr/sbin/srp_daemon -e -c -n -j %I -R 60
+
+[Install]
+WantedBy=remote-fs-pre.target
diff --git a/srp_daemon/srp_daemon_port@.service.5 b/srp_daemon/srp_daemon_port@.service.5
new file mode 100644
index 00000000..a755b2eb
--- /dev/null
+++ b/srp_daemon/srp_daemon_port@.service.5
@@ -0,0 +1,46 @@
+'\" t
+.TH "SRP_DAEMON_PORT@\&.SERVICE" "5" "" "srp_daemon" "srp_daemon_port@.service"
+.\" -----------------------------------------------------------------
+.\" * set default formatting
+.\" -----------------------------------------------------------------
+.\" disable hyphenation
+.nh
+.\" disable justification (adjust text to left margin only)
+.ad l
+.\" -----------------------------------------------------------------
+.\" * MAIN CONTENT STARTS HERE *
+.\" -----------------------------------------------------------------
+.SH "NAME"
+srp_daemon_port@.service \- srp_daemon_port@ systemd service that controls a
+single port
+.SH "SYNOPSIS"
+.PP
+srp_daemon_port@\&.service
+.SH "DESCRIPTION"
+.PP
+The srp_daemon_port@\&.service controls whether or not an srp_daemon process
+is monitoring the RDMA port specified as template argument. The format for the
+RDMA port name is \fIdev:port\fR where \fIdev\fR is the name of an RDMA device
+and \fIport\fR is an port number starting from one. Starting an instance of
+this template will start an srp_daemon process. Stopping an instance of this
+template will stop the srp_daemon process for the specified port.  Here is an
+example of how to obtain a list of all RDMA device and port number pairs:
+.PP
+.nf
+.RS
+$ for p in /sys/class/infiniband/*/ports/*; do
+    echo $p | sed 's,/sys/class/infiniband/,,;s,/ports/,:,'
+  done
+mlx4_0:1
+mlx4_0:2
+mlx4_1:1
+mlx4_1:2
+.RE
+.fi
+.PP
+
+.SH "SEE ALSO"
+.PP
+\fBsrp_daemon\fR(1),
+\fBsrp_daemon.service\fR(5),
+\fBsystemctl\fR(1)
-- 
2.12.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core 5/5] srp_daemon.service: Add support for hot-plugging

[Jason Gunthorpe]

On Mon, May 15, 2017 at 03:47:33PM -0700, Bart Van Assche wrote:
> srp_daemon.service is modified such that instead of starting
> /usr/sbin/srp_daemon.sh that it does not start any process. A new
> template service is added, namely srp_daemon_port@.service. This
> service replaces srp_daemon.sh and controls /usr/sbin/srp_daemon
> for a single port. A udev rule is added that instantiates the
> srp_daemon_port@.service every time an RDMA port is hot-added.
> Since the per-port service depends on the srp_daemon service,
> starting or stopping the srp_daemon service affects all per-port
> services.

This looks pretty good to me ... You are happy with how the user experience works?

A few minor thoughts

> +++ b/srp_daemon/90-srp-daemon.rules
> @@ -0,0 +1 @@
> +ACTION=="add", SUBSYSTEM=="infiniband_mad", PROGRAM:="/usr/bin/systemctl show srp_daemon -p UnitFileState", RESULT=="UnitFileState=enabled", TAG+="systemd", ENV{SYSTEMD_WANTS}+="srp_daemon_port@$attr{ibdev}:$attr{port}.service"

I think we should have a main rdma udev rules file that always creates
.device targets inside systemd for umad, unconditionally. This should
be useful for other things like opensm/etc.

So we'd have a line like:

ACTION=="add", SUBSYSTEM=="infiniband_mad", TAG+="systemd", ENV{SYSTEMD_ALIAS}="/dev/infiniband/umad/$attr{ibdev}:$attr{port}"

So we get (unescaped)

/dev/infiniband/umad0.device
/dev/infiniband/umad/mlx4_0:0.device

Then the srp-daemon.rule would just be the wants:

ACTION=="add", SUBSYSTEM=="infiniband_mad", PROGRAM:="/usr/bin/systemctl show srp_daemon -p UnitFileState", RESULT=="UnitFileState=enabled", ENV{SYSTEMD_WANTS}+="srp_daemon_port@$attr{ibdev}:$attr{port}.service"

> index 00000000..666400ab
> +++ b/srp_daemon/srp_daemon_port@.service
> @@ -0,0 +1,18 @@
> +[Unit]
> +Description=SRP daemon that monitors port %i
> +Documentation=man:srp_daemon file:/etc/rdma/rdma.conf file:/etc/srp_daemon.conf
> +DefaultDependencies=false
> +Conflicts=emergency.target emergency.service
> +Requires=rdma.service
> +Wants=opensm.service
> +After=rdma.service opensm.service srp_daemon.service
> +After=network.target
> +Before=remote-fs-pre.target
> +BindsTo=srp_daemon.service

Using the new .device, we can add a requires:

Requires: -dev-infiniband-umad0-%i.device

Now, systemd will never start the port service until udev says that
the umad is available for that port. It will actually wait until udev
says that umad is present if something requires it.

This would allow the admin to setup various things to guarentee
ordering, most directly, adding a
    Requires: srp_daemon_port@mlx4_0:1
To a .mount unit will make everything very reliable. (Suggest adding
notes about this to a man page)

I also wonder if the systemd activation protocol should be used by
srp_daemon, this should defer mount until srp_daemon has done the
right stuff, but that might be no good if the 'right stuff' requires a
link up?

However, if the admin is using the above .mount unit Requires then
they would want to wait until the link is up and the SM has been
contacted before attempting the first mount? systemd obnoxiously does
not have mount retries so everything should be perfect before mount is
done.

Probably something about this should be in the man page?

[You could also consider using umad0 as the %I, which would avoid the
alias, unclear to me if that good/bad for an admin.]

With the other changes you did, this means we can drop this:

> +After=rdma.service opensm.service srp_daemon.service

rdma.service would now be obsolete because the Requires on the .device
guarentees this will not start until something has loaded the modules.

Also, when you fixed the port state issue, I think that solved the need for
opensm.service.

Thus, the after should be:

After=srp_daemon.service

Which is good because rdma.service or opensm.service should not appear
outside the redhat/ directory.

> +The srp_daemon_port@\&.service controls whether or not an srp_daemon process
> +is monitoring the RDMA port specified as template argument. The format for the
> +RDMA port name is \fIdev:port\fR where \fIdev\fR is the name of an RDMA device
> +and \fIport\fR is an port number starting from one. Starting an instance of
> +this template will start an srp_daemon process. Stopping an instance of this
> +template will stop the srp_daemon process for the specified port.  Here is an
> +example of how to obtain a list of all RDMA device and port number
> pairs:

Does
  sysemctl mask srp_daemon_port@mlx4_0:1

Work to inhibit the daemon on a specific port? If yes that should be
mentioned in here I think. Otherwise, we probably still need a way to
do that.

The final bits would be to see if any security sandboxing options are
appropriate for the .unit file, since srp_daemon is network
facing. Drop caps, ro filesystems, etc. It should also dump root

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core 5/5] srp_daemon.service: Add support for hot-plugging

[Bart Van Assche]

On Mon, 2017-05-15 at 17:25 -0600, Jason Gunthorpe wrote:
> On Mon, May 15, 2017 at 03:47:33PM -0700, Bart Van Assche wrote:
> > srp_daemon.service is modified such that instead of starting
> > /usr/sbin/srp_daemon.sh that it does not start any process. A new
> > template service is added, namely srp_daemon_port@.service. This
> > service replaces srp_daemon.sh and controls /usr/sbin/srp_daemon
> > for a single port. A udev rule is added that instantiates the
> > srp_daemon_port@.service every time an RDMA port is hot-added.
> > Since the per-port service depends on the srp_daemon service,
> > starting or stopping the srp_daemon service affects all per-port
> > services.
> 
> This looks pretty good to me ... You are happy with how the user experience works?

Yes, except for one aspect, namely that starting srp_daemon does not start
the per-port services. I have addressed that as follows in srp_daemon.service:

ExecStart=.../srp_daemon_start_all.sh

where srp_daemon_start_all.sh is the following script:

#!/bin/sh
for p in /sys/class/infiniband/*/ports/*; do
    [ -e "$p" ] || continue
    p=${p#/sys/class/infiniband/}
    nohup /usr/bin/systemctl start "srp_daemon_port@${p/\/ports\//:}" </dev/null >&/dev/null &
done

> A few minor thoughts
> 
> > +++ b/srp_daemon/90-srp-daemon.rules
> > @@ -0,0 +1 @@
> > +ACTION=="add", SUBSYSTEM=="infiniband_mad", PROGRAM:="/usr/bin/systemctl show srp_daemon -p UnitFileState", RESULT=="UnitFileState=enabled", TAG+="systemd", ENV{SYSTEMD_WANTS}+="srp_daemon_port@$attr{ibdev}:$attr{port}.service"
> 
> I think we should have a main rdma udev rules file that always creates
> .device targets inside systemd for umad, unconditionally. This should
> be useful for other things like opensm/etc.
> 
> So we'd have a line like:
> 
> ACTION=="add", SUBSYSTEM=="infiniband_mad", TAG+="systemd", ENV{SYSTEMD_ALIAS}="/dev/infiniband/umad/$attr{ibdev}:$attr{port}"
> 
> So we get (unescaped)
> 
> /dev/infiniband/umad0.device
> /dev/infiniband/umad/mlx4_0:0.device
> 
> Then the srp-daemon.rule would just be the wants:
> 
> ACTION=="add", SUBSYSTEM=="infiniband_mad", PROGRAM:="/usr/bin/systemctl show srp_daemon -p UnitFileState", RESULT=="UnitFileState=enabled", ENV{SYSTEMD_WANTS}+="srp_daemon_port@$attr{ibdev}:$attr{port}.service"
> 
> > index 00000000..666400ab
> > +++ b/srp_daemon/srp_daemon_port@.service
> > @@ -0,0 +1,18 @@
> > +[Unit]
> > +Description=SRP daemon that monitors port %i
> > +Documentation=man:srp_daemon file:/etc/rdma/rdma.conf file:/etc/srp_daemon.conf
> > +DefaultDependencies=false
> > +Conflicts=emergency.target emergency.service
> > +Requires=rdma.service
> > +Wants=opensm.service
> > +After=rdma.service opensm.service srp_daemon.service
> > +After=network.target
> > +Before=remote-fs-pre.target
> > +BindsTo=srp_daemon.service
> 
> Using the new .device, we can add a requires:
> 
> Requires: -dev-infiniband-umad0-%i.device
> 
> Now, systemd will never start the port service until udev says that
> the umad is available for that port. It will actually wait until udev
> says that umad is present if something requires it.

All of this has been addressed in the following updated pull request:
https://github.com/linux-rdma/rdma-core/pull/135

> I also wonder if the systemd activation protocol should be used by
> srp_daemon, this should defer mount until srp_daemon has done the
> right stuff, but that might be no good if the 'right stuff' requires a
> link up?
>
> However, if the admin is using the above .mount unit Requires then
> they would want to wait until the link is up and the SM has been
> contacted before attempting the first mount? systemd obnoxiously does
> not have mount retries so everything should be perfect before mount is
> done.

I think that systemd mount units should have a dependency on /dev/disk/by-id
or /dev/disk/by-uuid instead of srp_daemon. Even after an srp_daemon_port@
service has been started it can take some time
before login occurs. Using a
dependency on /dev/disk/by-*id avoids that the mount service gets started
before the path it needs is available.

> Does
>   systemctl mask srp_daemon_port@mlx4_0:1
> 
> Work to inhibit the daemon on a specific port? If yes that should be
> mentioned in here I think. Otherwise, we probably still need a way to
> do that.

Information about masking has been added to the srp_daemon_port@ man page.

> The final bits would be to see if any security sandboxing options are
> appropriate for the .unit file, since srp_daemon is network
> facing. Drop caps, ro filesystems, etc. It should also dump root.

Sorry but since srp_daemon writes into /sys/class/infiniband_srp/*/add_target
I don't think it's possible to drop root privileges for the srp_daemon_port@
service.

Bart.--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core 5/5] srp_daemon.service: Add support for hot-plugging

[Jason Gunthorpe]

On Wed, May 24, 2017 at 08:33:32PM +0000, Bart Van Assche wrote:
> On Mon, 2017-05-15 at 17:25 -0600, Jason Gunthorpe wrote:
> > On Mon, May 15, 2017 at 03:47:33PM -0700, Bart Van Assche wrote:
> > > srp_daemon.service is modified such that instead of starting
> > > /usr/sbin/srp_daemon.sh that it does not start any process. A new
> > > template service is added, namely srp_daemon_port@.service. This
> > > service replaces srp_daemon.sh and controls /usr/sbin/srp_daemon
> > > for a single port. A udev rule is added that instantiates the
> > > srp_daemon_port@.service every time an RDMA port is hot-added.
> > > Since the per-port service depends on the srp_daemon service,
> > > starting or stopping the srp_daemon service affects all per-port
> > > services.
> > 
> > This looks pretty good to me ... You are happy with how the user experience works?
> 
> Yes, except for one aspect, namely that starting srp_daemon does not
> start the per-port services. I have addressed that as follows in
> srp_daemon.service:

Interesting.. What use case do have in mind for start on all ports?

>    nohup /usr/bin/systemctl start "srp_daemon_port@${p/\/ports\//:}" </dev/null >&/dev/null &

I wonder, does this defeat the 'systemctl mask srp_daemon_port@mlx4_0:1' ?

Anyhow, I think it looks pretty good, the 'BindsTo' is a nice touch to
support hot-removal as well.

> > However, if the admin is using the above .mount unit Requires then
> > they would want to wait until the link is up and the SM has been
> > contacted before attempting the first mount? systemd obnoxiously does
> > not have mount retries so everything should be perfect before mount is
> > done.
> 
> I think that systemd mount units should have a dependency on
> /dev/disk/by-id or /dev/disk/by-uuid instead of srp_daemon. Even
> after an srp_daemon_port@ service has been started it can take some
> time before login occurs. Using a dependency on /dev/disk/by-*id
> avoids that the mount service gets started before the path it needs
> is available.

That makes some sense.

> > The final bits would be to see if any security sandboxing options are
> > appropriate for the .unit file, since srp_daemon is network
> > facing. Drop caps, ro filesystems, etc. It should also dump root.
> 
> Sorry but since srp_daemon writes into /sys/class/infiniband_srp/*/add_target
> I don't think it's possible to drop root privileges for the srp_daemon_port@
> service.

Dropping root is something that has to be done inside the daemon.. eg
it would open the required files as root and never close them, then
drop root. So eg it would lseek,write to add_target instead of
open,write. Similar with /dev/umad.

The sandboxing options are different, these do not have to impact
DAC_OVERRIDE for root, but permanently limit other things the daemon
can do.

Eg for reference here is what timesyncd.service sets:

CapabilityBoundingSet=CAP_SYS_TIME CAP_SETUID CAP_SETGID CAP_SETPCAP CAP_CHOWN CAP_DAC_OVERRIDE CAP_FOWNER
PrivateTmp=yes
PrivateDevices=yes
ProtectSystem=full
ProtectHome=yes
ProtectControlGroups=yes
ProtectKernelTunables=yes
MemoryDenyWriteExecute=yes
RestrictRealtime=yes
RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6
SystemCallFilter=~@cpu-emulation @debug @keyring @module @mount @obsolete @raw-io

Some subset of those ideas may ultimately apply to srp-daemon.

Anyhow, I think the series is basically fine by me..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html