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