[systemd-devel] 60-persistent-storage.rules: add NVMe disks and partitions (again)

[Lennart Poettering]

On Fri, 15.05.15 20:53, Per Bergqvist (per at 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