On Thu, Jun 01, 2023 at 05:48:22PM -0400, Demi Marie Obenour wrote:
> + if (diskseq) {
> + struct gendisk *disk = bdev->bd_disk;
> +
> + if (unlikely(disk == NULL)) {
> + pr_err("%s: device %08x has no gendisk\n",
> + __func__, vbd->pdevice);
> + xen_vbd_free(vbd);
> + return -EFAULT;
> + }
bdev->bd_disk is never NULL.
> + diskseq_str = xenbus_read(XBT_NIL, dev->nodename, "diskseq",
> &diskseq_len);
Please avoid the overly long line.
> + if (IS_ERR(diskseq_str)) {
> + int err = PTR_ERR(diskseq_str);
> + diskseq_str = NULL;
> +
> + /*
> + * If this does not exist, it means legacy userspace that does
> not
.. even more so in comments.
> + * support diskseq.
> + */
> + if (unlikely(!XENBUS_EXIST_ERR(err))) {
> + xenbus_dev_fatal(dev, err, "reading diskseq");
> + return;
> + }
> + diskseq = 0;
> + } else if (diskseq_len <= 0) {
> + xenbus_dev_fatal(dev, -EFAULT, "diskseq must not be empty");
> + goto fail;
> + } else if (diskseq_len > 16) {
No need for a else after a return.
> + xenbus_dev_fatal(dev, -ERANGE, "diskseq too long: got %d but
> limit is 16",
> + diskseq_len);
> + goto fail;
> + } else if (diskseq_str[0] == '0') {
> + xenbus_dev_fatal(dev, -ERANGE, "diskseq must not start with
> '0'");
> + goto fail;
> + } else {
> + char *diskseq_end;
> + diskseq = simple_strtoull(diskseq_str, &diskseq_end, 16);
> + if (diskseq_end != diskseq_str + diskseq_len) {
> + xenbus_dev_fatal(dev, -EINVAL, "invalid diskseq");
> + goto fail;
> + }
> + kfree(diskseq_str);
> + diskseq_str = NULL;
> + }
And I suspect the code will be a lot easier to follow if you move
the diskseq validation into a separate helper.
