On Wed, Jun 07, 2023 at 12:44:22AM -0700, Christoph Hellwig wrote:
> 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.
Fixed in v3.
> > + diskseq_str = xenbus_read(XBT_NIL, dev->nodename, "diskseq", &diskseq_len);
>
> Please avoid the overly long line.
Fixed in v3.
> > + 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.
Fixed in v3.
> > + * 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.
Fixed in v3.
> > + 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.
Fixed in v3.
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
Attachment:
signature.asc
Description: PGP signature
