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

[Lennart Poettering]

On Sat, 16.05.15 10:18, Per Bergqvist (per at bst.lu) wrote:

> 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.

Hmm? This is from CODING_STYLE:

        - Do not write "foo ()", write "foo()".

> +
> +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 };

Hmm, why __u32? First of all, try to use userspace types,
i.e. uint32_t. But secondly, shouldn't this be size_t?

> +int main(int argc, char *argv[]) {
> +        _cleanup_udev_unref_ struct udev *udev = NULL;
> +
> +        struct nvme_id_ctrl nvme_id_ctrl = { .vid = 0 };

initializer appears unnecessary, as the compiler initializes all
fields to 0 anyway if they are unspecified. i.e. just write "= { };"
here...

> +        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;

needs to be "return EXIT_FAILURE". After all this is the main()
function, where errors are not errno-style, but EXIT_FAILURE,
EXIT_SUCCESS or something else betwee 0..255...

> +        }
> +
> +        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));

Please remove the space...

> +                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);
> +                }

Please no {} for single-line if blocks (or else blocks...)

To me this looks fine otherwise, but I figure Kay has to decide if
this goes in or not. He might want this as built-in rather than as
external tool though...

Lennart

-- 
Lennart Poettering, Red Hat