<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Lennart,<div><br></div><div>Thank you for all the comments. </div><div><br></div><div>I have changed everything except the 'No space between function name and opening У(УС.</div><div>Cannot find anything about that in CODING_STYLE or evidence in other sources.</div><div>Kept the call to nvme_identify more or less as before, please let me know if it has to be changed.</div><div><br></div><div>Please find the new patch below.</div><div><br></div><div>BR</div><div>Per</div><div><br></div><div><div>From 6cf318688baead0b4a737cf2c2d7618b6a131954 Mon Sep 17 00:00:00 2001</div><div>From: Per Bergqvist <<a href="mailto:per@bst.lu">per@bst.lu</a>></div><div>Date: Sat, 16 May 2015 11:01:17 +0200</div><div>Subject: [PATCH] nvme_id following CODING_STYLE</div><div><br></div><div>---</div><div> Makefile.am | 11 +++</div><div> rules/60-persistent-storage.rules | 5 ++</div><div> src/udev/nvme_id/nvme_id.c | 149 ++++++++++++++++++++++++++++++++++++++</div><div> 3 files changed, 165 insertions(+)</div><div> create mode 100644 src/udev/nvme_id/nvme_id.c</div><div><br></div><div>diff --git a/Makefile.am b/Makefile.am</div><div>index bf04d31..0de0d18 100644</div><div>--- a/Makefile.am</div><div>+++ b/Makefile.am</div><div>@@ -3904,6 +3904,17 @@ dist_udevrules_DATA += \</div><div> <span class="Apple-tab-span" style="white-space:pre"> </span>rules/60-persistent-v4l.rules</div><div> </div><div> # ------------------------------------------------------------------------------</div><div>+nvme_id_SOURCES = \</div><div>+<span class="Apple-tab-span" style="white-space:pre"> </span>src/udev/nvme_id/nvme_id.c</div><div>+</div><div>+nvme_id_LDADD = \</div><div>+<span class="Apple-tab-span" style="white-space:pre"> </span>libudev-internal.la \</div><div>+<span class="Apple-tab-span" style="white-space:pre"> </span>libsystemd-shared.la</div><div>+</div><div>+udevlibexec_PROGRAMS += \</div><div>+<span class="Apple-tab-span" style="white-space:pre"> </span>nvme_id</div><div>+</div><div>+# ------------------------------------------------------------------------------</div><div> accelerometer_SOURCES = \</div><div> <span class="Apple-tab-span" style="white-space:pre"> </span>src/udev/accelerometer/accelerometer.c</div><div> </div><div>diff --git a/rules/60-persistent-storage.rules b/rules/60-persistent-storage.rules</div><div>index 25b44a5..d3368a5 100644</div><div>--- a/rules/60-persistent-storage.rules</div><div>+++ b/rules/60-persistent-storage.rules</div><div>@@ -42,6 +42,11 @@ KERNEL=="cciss*", ENV{DEVTYPE}=="disk", ENV{ID_SERIAL}!="?*", IMPORT{program}="s</div><div> KERNEL=="sd*|sr*|cciss*", ENV{DEVTYPE}=="disk", ENV{ID_SERIAL}=="?*", SYMLINK+="disk/by-id/$env{ID_BUS}-$env{ID_SERIAL}"</div><div> KERNEL=="sd*|cciss*", ENV{DEVTYPE}=="partition", ENV{ID_SERIAL}=="?*", SYMLINK+="disk/by-id/$env{ID_BUS}-$env{ID_SERIAL}-part%n"</div><div> </div><div>+# NVMe</div><div>+KERNEL=="nvme*", ENV{ID_SERIAL}!="?*", IMPORT{program}="nvme_id --export $devnode"</div><div>+KERNEL=="nvme*", ENV{DEVTYPE}=="disk", ENV{ID_SERIAL}=="?*", SYMLINK+="disk/by-id/nvme-$env{ID_SERIAL}"</div><div>+KERNEL=="nvme*", ENV{DEVTYPE}=="partition", ENV{ID_SERIAL}=="?*", SYMLINK+="disk/by-id/nvme-$env{ID_SERIAL}-part%n"</div><div>+</div><div> # firewire</div><div> KERNEL=="sd*[!0-9]|sr*", ATTRS{ieee1394_id}=="?*", SYMLINK+="disk/by-id/ieee1394-$attr{ieee1394_id}"</div><div> KERNEL=="sd*[0-9]", ATTRS{ieee1394_id}=="?*", SYMLINK+="disk/by-id/ieee1394-$attr{ieee1394_id}-part%n"</div><div>diff --git a/src/udev/nvme_id/nvme_id.c b/src/udev/nvme_id/nvme_id.c</div><div>new file mode 100644</div><div>index 0000000..8941977</div><div>--- /dev/null</div><div>+++ b/src/udev/nvme_id/nvme_id.c</div><div>@@ -0,0 +1,149 @@</div><div>+/* -*- mode:c; tab-width:8; intent-tabs-mode:nil; -*-</div><div>+ *</div><div>+ * nvme_id - reads model/serial/firmware revision numbers from nvme drives</div><div>+ *</div><div>+ * Copyright (C) 2015 Per Bergqvist <<a href="mailto:per@bst.lu">per@bst.lu</a>></div><div>+ *</div><div>+ * This program is free software: you can redistribute it and/or modify</div><div>+ * it under the terms of the GNU General Public License as published by</div><div>+ * the Free Software Foundation, either version 2 of the License, or</div><div>+ * (at your option) any later version.</div><div>+ *</div><div>+ * This program is distributed in the hope that it will be useful,</div><div>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of</div><div>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the</div><div>+ * GNU General Public License for more details.</div><div>+ *</div><div>+ * You should have received a copy of the GNU General Public License</div><div>+ * along with this program. If not, see <<a href="http://www.gnu.org/licenses/">http://www.gnu.org/licenses/</a>>.</div><div>+ */</div><div>+</div><div>+#include <stdio.h></div><div>+#include <stdlib.h></div><div>+#include <stdint.h></div><div>+#include <unistd.h></div><div>+#include <fcntl.h></div><div>+#include <ctype.h></div><div>+#include <string.h></div><div>+#include <errno.h></div><div>+#include <getopt.h></div><div>+#include <linux/nvme.h></div><div>+#include <sys/ioctl.h></div><div>+#include <sys/types.h></div><div>+#include <sys/stat.h></div><div>+</div><div>+#include "libudev.h"</div><div>+#include "libudev-private.h"</div><div>+#include "udev-util.h"</div><div>+#include "log.h"</div><div>+</div><div>+static int nvme_identify(struct udev *udev, int fd, void *buf, __u32 buf_len) {</div><div>+ struct nvme_admin_cmd command = {</div><div>+ .opcode = nvme_admin_identify,</div><div>+ .addr = (unsigned long)buf,</div><div>+ .data_len = buf_len,</div><div>+ .cdw10 = 1 };</div><div>+</div><div>+ if (ioctl(fd, NVME_IOCTL_ADMIN_CMD, &command) < 0)</div><div>+ return -errno;</div><div>+</div><div>+ return 0;</div><div>+}</div><div>+</div><div>+int main(int argc, char *argv[]) {</div><div>+ _cleanup_udev_unref_ struct udev *udev = NULL;</div><div>+</div><div>+ struct nvme_id_ctrl nvme_id_ctrl = { .vid = 0 };</div><div>+</div><div>+ char model[sizeof(nvme_id_ctrl.mn)+1];</div><div>+ char model_enc[4*sizeof(nvme_id_ctrl.mn)+1];</div><div>+ char serial[sizeof(nvme_id_ctrl.sn)+1];</div><div>+ char revision[sizeof(nvme_id_ctrl.fr)+1];</div><div>+</div><div>+ const char *node = NULL;</div><div>+ int export = 0;</div><div>+</div><div>+ _cleanup_close_ int fd = -1;</div><div>+</div><div>+ static const struct option options[] = {</div><div>+ { "export", no_argument, NULL, 'x' },</div><div>+ { "help", no_argument, NULL, 'h' },</div><div>+ {}</div><div>+ };</div><div>+</div><div>+ log_parse_environment();</div><div>+ log_open();</div><div>+</div><div>+ udev = udev_new();</div><div>+ if (udev == NULL)</div><div>+ return EXIT_SUCCESS;</div><div>+</div><div>+ while (1) {</div><div>+ int option;</div><div>+</div><div>+ option = getopt_long(argc, argv, "xh", options, NULL);</div><div>+ if (option == -1)</div><div>+ break;</div><div>+</div><div>+ switch (option) {</div><div>+ case 'x':</div><div>+ export = 1;</div><div>+ break;</div><div>+ case 'h':</div><div>+ printf("Usage: nvme_id [--export] [--help] <device>\n"</div><div>+ " -x,--export print values as environment keys\n"</div><div>+ " -h,--help print this help text\n\n");</div><div>+ return EXIT_SUCCESS;</div><div>+ }</div><div>+ }</div><div>+</div><div>+ node = argv[optind];</div><div>+ if (node == NULL) {</div><div>+ log_error("no node specified");</div><div>+ return EXIT_FAILURE;</div><div>+ }</div><div>+</div><div>+ fd = open(node, O_RDONLY|O_NONBLOCK|O_CLOEXEC);</div><div>+ if (fd < 0) {</div><div>+ log_error_errno(errno, "Unable to open '%s': %m", node);</div><div>+ return -errno;</div><div>+ }</div><div>+</div><div>+ if (nvme_identify(udev, fd, &nvme_id_ctrl, sizeof(struct nvme_id_ctrl)) == 0) {</div><div>+ memcpy (model, nvme_id_ctrl.mn, sizeof(nvme_id_ctrl.mn));</div><div>+ model[sizeof(nvme_id_ctrl.mn)] = '\0';</div><div>+ strstrip(model);</div><div>+ udev_util_encode_string(model, model_enc, sizeof(model_enc));</div><div>+ util_replace_whitespace((char *) nvme_id_ctrl.mn, model, sizeof(nvme_id_ctrl.mn));</div><div>+ util_replace_chars(model, NULL);</div><div>+ util_replace_whitespace((char *) nvme_id_ctrl.sn, serial, sizeof(nvme_id_ctrl.sn));</div><div>+ util_replace_chars(serial, NULL);</div><div>+ util_replace_whitespace((char *) nvme_id_ctrl.fr, revision, sizeof(nvme_id_ctrl.fr));</div><div>+ util_replace_chars(revision, NULL);</div><div>+ } else {</div><div>+ log_debug_errno(errno, "nvme_identify failed for '%s': %m", node);</div><div>+ return EXIT_FAILURE;</div><div>+ }</div><div>+</div><div>+</div><div>+ if (export) {</div><div>+ printf("ID_TYPE=nvme\n");</div><div>+ printf("ID_MODEL=%s\n", model);</div><div>+ printf("ID_MODEL_ENC=%s\n", model_enc);</div><div>+ printf("ID_REVISION=%s\n", revision);</div><div>+ if (serial[0] != '\0') {</div><div>+ printf("ID_SERIAL=%s_%s\n", model, serial);</div><div>+ printf("ID_SERIAL_SHORT=%s\n", serial);</div><div>+ } else {</div><div>+ printf("ID_SERIAL=%s\n", model);</div><div>+ }</div><div>+</div><div>+ } else {</div><div>+ if (serial[0] != '\0')</div><div>+ printf("%s_%s\n", model, serial);</div><div>+ else</div><div>+ printf("%s\n", model);</div><div>+ }</div><div>+</div><div>+ return EXIT_SUCCESS;</div><div>+}</div><div>-- </div><div>2.4.0</div><div><br></div></div><div><br></div><div><div><div>On 15 May 2015, at 21:15 , Lennart Poettering <<a href="mailto:lennart@poettering.net">lennart@poettering.net</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">On Fri, 15.05.15 20:53, Per Bergqvist (<a href="mailto:per@bst.lu">per@bst.lu</a>) wrote:<br><br><blockquote type="cite">Hej,<br><br>Please (finally) find a patched for v219 to add a nvme_id utility<br>and add support for NVMe disks in 60-persistent-storage.rules.<br></blockquote><br>I figure Kay and Tom have to decide if this goes in, but here's a<br>quick code review, in case they are in favour:<br><br><blockquote type="cite">+static int nvme_identify(struct udev *udev,<br>+ int fd,<br>+ void *ptr)<br>+{<br></blockquote><br>Please see CODING_STYLE. We tend to place the opening bracket on the<br>same line as the function name. (yes some older code in the systemd<br>tree does not follow this rule, but please try to follow this for new<br>code).<br><br><blockquote type="cite">+<span class="Apple-tab-span" style="white-space:pre"> </span>struct nvme_admin_cmd command;<br>+<span class="Apple-tab-span" style="white-space:pre"> </span>memset(&command, 0, sizeof(command));<br></blockquote><br>See CODING_STYLE, we tend to prefer initializing structures on the<br>stack via structure initializers, so that we don't need explicit<br>memset(). i.e.<br><br>struct nvme_admin_cmd command = {<br> .opcode = ...,<br>}<br><br><blockquote type="cite">+<span class="Apple-tab-span" style="white-space:pre"> </span><br>+<span class="Apple-tab-span" style="white-space:pre"> </span>command.opcode = nvme_admin_identify;<br>+<span class="Apple-tab-span" style="white-space:pre"> </span>command.nsid = 0;<br>+<span class="Apple-tab-span" style="white-space:pre"> </span>command.addr = (unsigned long)ptr;<br>+<span class="Apple-tab-span" style="white-space:pre"> </span>command.data_len = sizeof(struct nvme_id_ctrl);<br>+ command.cdw10 = 1;<br></blockquote><br>Indenting is weird. Please strictly use 8ch space indenting.<br><br><blockquote type="cite">+ return ioctl(fd, NVME_IOCTL_ADMIN_CMD, &command);<br>+}<br></blockquote><br>We generally try to follow the rule to return kernel-style negative<br>errno error codes from the functions we define, even if the underlying<br>syscalls don't. Hence, please rewrite this as:<br><br> if (ioctl(...) < 0)<br> return -errno;<br><br> return 0;<br><br><blockquote type="cite">+<br>+int main(int argc, char *argv[])<br>+{<br></blockquote><br>The opening bracket on the same line as the function name please.<br><br><blockquote type="cite">+ _cleanup_udev_unref_ struct udev *udev = NULL;<br>+<br>+<span class="Apple-tab-span" style="white-space:pre"> </span>struct nvme_id_ctrl nvme_ctrl;<br>+<span class="Apple-tab-span" style="white-space:pre"> </span><br>+ char model[41];<br>+ char model_enc[256];<br>+ char serial[21];<br>+ char revision[9];<br></blockquote><br>Weird indenting...<br><br><blockquote type="cite">+<br>+ while (1) {<br>+ int option;<br>+<br>+ option = getopt_long(argc, argv, "xh", options, NULL);<br>+ if (option == -1)<br>+ break;<br>+<br>+ switch (option) {<br>+ case 'x':<br>+ export = 1;<br>+ break;<br>+ case 'h':<br>+ printf("Usage: nvme_id [--export] [--help] <device>\n"<br>+ " -x,--export print values as environment keys\n"<br>+ " -h,--help print this help text\n\n");<br>+ return 0;<br>+ }<br>+ }<br>+<br>+ node = argv[optind];<br>+ if (node == NULL) {<br>+ log_error("no node specified");<br>+ return 1;<br></blockquote><br>Please use libc's EXIT_FAILURE define here.<br><br><blockquote type="cite">+ }<br>+<br>+ fd = open(node, O_RDONLY|O_NONBLOCK|O_CLOEXEC);<br>+ if (fd < 0) {<br>+ log_error("unable to open '%s'", node);<br>+ return 1;<br>+ }<br></blockquote><br>Similar here.<br><br>Also, please use use this log_error_errno() syntax if there's an errno<br>to report:<br><br>log_error_errno(errno, "Unable to open '%s': %m", node);<br><br><br><blockquote type="cite">+<br>+<span class="Apple-tab-span" style="white-space:pre"> </span>memset(&nvme_ctrl, 0, sizeof(struct nvme_id_ctrl));<br></blockquote><br>Please initialize at time of declaration.<br><br><blockquote type="cite">+<span class="Apple-tab-span" style="white-space:pre"> </span><br>+ if (nvme_identify(udev, fd, &nvme_ctrl) == 0) {<br>+<span class="Apple-tab-span" style="white-space:pre"> </span> int i;<br>+<span class="Apple-tab-span" style="white-space:pre"> </span> memcpy (model, nvme_ctrl.mn, 40);<br></blockquote><br>No space between function name and opening "(", please, see CODING_STYLE.<br><br><blockquote type="cite">+<span class="Apple-tab-span" style="white-space:pre"> </span> for(i=39;i>=0;i--) if (model[i]== ' ') model[i] = '\0';<br>else break;<br></blockquote><br>Please use strstrip() for this.<br><br><blockquote type="cite">+<span class="Apple-tab-span" style="white-space:pre"> </span> model[40] = '\0';<br>+<span class="Apple-tab-span" style="white-space:pre"> </span> udev_util_encode_string(model, model_enc, sizeof(model_enc));<br>+<span class="Apple-tab-span" style="white-space:pre"> </span> util_replace_whitespace((char *) nvme_ctrl.mn, model,<br>40);<br></blockquote><br>Hmm, use "sizeof(model)" instead of "40"?<br><br><blockquote type="cite">+ if (export) {<br>+ printf("ID_TYPE=nvme\n");<br>+ printf("ID_MODEL=%s\n", model);<br>+ printf("ID_MODEL_ENC=%s\n", model_enc);<br>+ printf("ID_REVISION=%s\n", revision);<br>+ if (serial[0] != '\0') {<br>+ printf("ID_SERIAL=%s_%s\n", model, serial);<br>+ printf("ID_SERIAL_SHORT=%s\n", serial);<br>+ } else {<br>+ printf("ID_SERIAL=%s\n", model);<br>+ }<br>+<br>+ } else {<br>+ if (serial[0] != '\0')<br>+ printf("%s_%s\n", model, serial);<br>+ else<br>+ printf("%s\n", model);<br>+ }<br>+<br>+ return 0;<br></blockquote><br>Use EXIT_SUCCESS here...<br><br>Lennart<br><br>-- <br>Lennart Poettering, Red Hat<br></blockquote></div><br><div apple-content-edited="true"> <div style="color: rgb(0, 0, 0); font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div style="color: rgb(0, 0, 0); font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div><br></div></div></div></div><br class="Apple-interchange-newline"><br class="Apple-interchange-newline"> </div> <br></div></body></html>