On Sun, Aug 19, 2018 at 11:46:12PM -0700, Ori Bernstein wrote:
> One more minor update to this patch:
>
> - Remove unused enum
> - Remove unused function prototype
> - Move some qcow2-specific headers into the qcow2 patch.
Ori, it's nice seeing good progress on this. I have a couple of
questions inline below.
+--+
Carlos
>
> +/*
> + * Initializes a raw disk image backing file from an fd.
> + * Stores the number of 512 byte sectors in *szp,
> + * returning -1 for error, 0 for success.
> + */
> +int
> +virtio_init_raw(struct virtio_backing *file, off_t *szp, int fd)
> +{
> + off_t sz;
> + int *fdp;
> +
> + sz = lseek(fd, 0, SEEK_END);
> + if (sz == -1)
> + return -1;
> +
> + fdp = malloc(sizeof(int));
> + *fdp = fd;
> + file->p = fdp;
> + file->pread = raw_pread;
> + file->pwrite = raw_pwrite;
> + file->close = raw_close;
> + *szp = sz / 512;
> + return 0;
> +}
Why are we making sz represent the number of 512 byte sectors? This may
be true for hd disks but it needs to be *bytes* for ISO/cdrom.
[snip]
> @@ -1944,12 +1957,12 @@ virtio_init(struct vmd_vm *vm, int child_cdrom, int
> *child_disks,
> + sizeof(uint16_t) * (2 + VIOSCSI_QUEUE_SIZE));
> vioscsi->vq[i].last_avail = 0;
> }
> - sz = lseek(child_cdrom, 0, SEEK_END);
> - vioscsi->fd = child_cdrom;
> + if (virtio_init_disk(&vioscsi->file, &vioscsi->sz,
> + child_cdrom) == -1)
> + return;
> vioscsi->locked = 0;
> vioscsi->lba = 0;
> - vioscsi->sz = sz;
> - vioscsi->n_blocks = sz >> 11; /* num of 2048 blocks in file */
> + vioscsi->n_blocks = vioscsi->sz >> 11; /* num of 2048 blocks in
> file */
This has to be the number of bytes shifted to get the number of 2K
blocks. As the patch is, this yields the incorrect number of blocks an
ISO has. This breaks ISO/cdrom support.
> vioscsi->max_xfer = VIOSCSI_BLOCK_SIZE_CDROM;
> vioscsi->pci_id = id;
> vioscsi->vm_id = vcp->vcp_id;
[snip]
> +struct virtio_backing {
> + void *p;
> + char *path;
> + ssize_t (*pread)(void *p, char *buf, size_t len, off_t off);
> + ssize_t (*pwrite)(void *p, char *buf, size_t len, off_t off);
> + void (*close)(void *p);
> +};
What is path used for? I don't see it used anywhere in this patch or in
the qcow2 patch.
