On Thu, Sep 13, 2018 at 11:10:50PM -0700, Ori Bernstein wrote:
> On Fri, 14 Sep 2018 13:50:40 +0800, Michael Mikonos <[email protected]> wrote:
>
> > On Thu, Sep 13, 2018 at 10:08:16PM -0700, Ori Bernstein wrote:
> > > On Thu, 13 Sep 2018 10:30:54 +0800, Michael Mikonos <[email protected]> wrote:
> > >
> > I think a check for !disk->base belongs here as done above for disk->l1.
> >
>
> I think that co
OK miko@, in case someone with a current vmd setup wants to commit this.
>
> ---
> usr.sbin/vmd/vioqcow2.c | 61 +++++++++++++++++++++++++++--------------
> usr.sbin/vmd/vioraw.c | 2 ++
> usr.sbin/vmd/virtio.c | 11 ++++++++
> usr.sbin/vmd/virtio.h | 1 +
> usr.sbin/vmd/vm.c | 3 ++
> 5 files changed, 57 insertions(+), 21 deletions(-)
>
> diff --git usr.sbin/vmd/vioqcow2.c usr.sbin/vmd/vioqcow2.c
> index 7c7e4857695..b3e4e1ec058 100644
> --- usr.sbin/vmd/vioqcow2.c
> +++ usr.sbin/vmd/vioqcow2.c
> @@ -77,7 +77,6 @@ struct qcdisk {
>
> int fd;
> uint64_t *l1;
> - char *scratch;
> off_t end;
> uint32_t clustersz;
> off_t disksz; /* In bytes */
> @@ -161,17 +160,19 @@ qc2_open(struct qcdisk *disk, int fd)
> size_t i;
> int version;
>
> + pthread_rwlock_init(&disk->lock, NULL);
> + disk->fd = fd;
> + disk->base = NULL;
> + disk->l1 = NULL;
> +
> if (pread(fd, &header, sizeof header, 0) != sizeof header) {
> log_warn("%s: short read on header", __func__);
> - return -1;
> + goto error;
> }
> if (strncmp(header.magic, "QFI\xfb", 4) != 0) {
> log_warn("%s: invalid magic numbers", __func__);
> - return -1;
> + goto error;
> }
> - pthread_rwlock_init(&disk->lock, NULL);
> - disk->fd = fd;
> - disk->base = NULL;
>
> disk->clustersz = (1ull << be32toh(header.clustershift));
> disk->disksz = be64toh(header.disksz);
> @@ -182,6 +183,7 @@ qc2_open(struct qcdisk *disk, int fd)
> disk->refoff = be64toh(header.refoff);
> disk->nsnap = be32toh(header.snapcount);
> disk->snapoff = be64toh(header.snapsz);
> +
> /*
> * The additional features here are defined as 0 in the v2 format,
> * so as long as we clear the buffer before parsing, we don't need
> @@ -196,23 +198,25 @@ qc2_open(struct qcdisk *disk, int fd)
> * We only know about the dirty or corrupt bits here.
> */
> if (disk->incompatfeatures & ~(QCOW2_DIRTY|QCOW2_CORRUPT)) {
> - log_warn("%s: unsupported features %llx", __func__,
> + log_warnx("%s: unsupported features %llx", __func__,
> disk->incompatfeatures & ~(QCOW2_DIRTY|QCOW2_CORRUPT));
> - return -1;
> + goto error;
> }
>
> disk->l1 = calloc(disk->l1sz, sizeof *disk->l1);
> - if (pread(disk->fd, (char*)disk->l1, 8*disk->l1sz, disk->l1off)
> + if (!disk->l1)
> + goto error;
> + if (pread(disk->fd, disk->l1, 8*disk->l1sz, disk->l1off)
> != 8*disk->l1sz) {
> - free(disk->l1);
> - return -1;
> + log_warn("%s: unable to read qcow2 L1 table", __func__);
> + goto error;
> }
> for (i = 0; i < disk->l1sz; i++)
> disk->l1[i] = be64toh(disk->l1[i]);
> version = be32toh(header.version);
> if (version != 2 && version != 3) {
> log_warn("%s: unknown qcow2 version %d", __func__, version);
> - return -1;
> + goto error;
> }
>
> backingoff = be64toh(header.backingoff);
> @@ -223,34 +227,47 @@ qc2_open(struct qcdisk *disk, int fd)
> * otherwise we just crash with a pledge violation.
> */
> log_warn("%s: unsupported external snapshot images", __func__);
> - return -1;
> + goto error;
>
> if (backingsz >= sizeof basepath - 1) {
> log_warn("%s: snapshot path too long", __func__);
> - return -1;
> + goto error;
> }
> if (pread(fd, basepath, backingsz, backingoff) != backingsz) {
> log_warn("%s: could not read snapshot base name",
> __func__);
> - return -1;
> + goto error;
> }
> basepath[backingsz] = 0;
>
> disk->base = calloc(1, sizeof(struct qcdisk));
> + if (!disk->base)
> + goto error;
> if (qc2_openpath(disk->base, basepath, O_RDONLY) == -1) {
> - free(disk->base);
> - return -1;
> + log_warn("%s: could not open %s", basepath, __func__);
> + goto error;
> }
> if (disk->base->clustersz != disk->clustersz) {
> log_warn("%s: all disks must share clustersize",
> __func__);
> - free(disk->base);
> - return -1;
> + goto error;
> }
> }
> - fstat(fd, &st);
> + if (fstat(fd, &st) == -1) {
> + log_warn("%s: unable to stat disk", __func__);
> + goto error;
> + }
> +
> disk->end = st.st_size;
> +
> + log_debug("opened qcow2 disk version %d:", version);
> + log_debug("size:\t%lld", disk->disksz);
> + log_debug("end:\t%lld", disk->end);
> + log_debug("nsnap:\t%d", disk->nsnap);
> return 0;
> +error:
> + qc2_close(disk);
> + return -1;
> }
>
> static ssize_t
> @@ -362,8 +379,10 @@ qc2_close(void *p)
> struct qcdisk *disk;
>
> disk = p;
> - pwrite(disk->fd, disk->l1, disk->l1sz, disk->l1off);
> + if (disk->base)
> + qc2_close(disk->base);
> close(disk->fd);
> + free(disk->l1);
> free(disk);
> }
>
> diff --git usr.sbin/vmd/vioraw.c usr.sbin/vmd/vioraw.c
> index 4b87c649407..9c76b1356f9 100644
> --- usr.sbin/vmd/vioraw.c
> +++ usr.sbin/vmd/vioraw.c
> @@ -62,6 +62,8 @@ virtio_init_raw(struct virtio_backing *file, off_t *szp,
> int fd)
> return -1;
>
> fdp = malloc(sizeof(int));
> + if (!fdp)
> + return -1;
> *fdp = fd;
> file->p = fdp;
> file->pread = raw_pread;
> diff --git usr.sbin/vmd/virtio.c usr.sbin/vmd/virtio.c
> index c774cc7813a..20a94e6fa4d 100644
> --- usr.sbin/vmd/virtio.c
> +++ usr.sbin/vmd/virtio.c
> @@ -2009,6 +2009,17 @@ virtio_init(struct vmd_vm *vm, int child_cdrom, int
> *child_disks,
> evtimer_set(&vmmci.timeout, vmmci_timeout, NULL);
> }
>
> +void
> +virtio_shutdown(struct vmd_vm *vm)
> +{
> + int i;
> +
> + /* ensure that our disks are synced */
> + vioscsi->file.close(vioscsi->file.p);
> + for (i = 0; i < nr_vioblk; i++)
> + vioblk[i].file.close(vioblk[i].file.p);
> +}
> +
> int
> vmmci_restore(int fd, uint32_t vm_id)
> {
> diff --git usr.sbin/vmd/virtio.h usr.sbin/vmd/virtio.h
> index 86ee6d21f9f..574f77d5fe8 100644
> --- usr.sbin/vmd/virtio.h
> +++ usr.sbin/vmd/virtio.h
> @@ -258,6 +258,7 @@ struct ioinfo {
>
> /* virtio.c */
> void virtio_init(struct vmd_vm *, int, int *, int *);
> +void virtio_shutdown(struct vmd_vm *);
> int virtio_dump(int);
> int virtio_restore(int, struct vmd_vm *, int, int *, int *);
> uint32_t vring_size(uint32_t);
> diff --git usr.sbin/vmd/vm.c usr.sbin/vmd/vm.c
> index 046b2be8503..491ca2eae3f 100644
> --- usr.sbin/vmd/vm.c
> +++ usr.sbin/vmd/vm.c
> @@ -371,6 +371,9 @@ start_vm(struct vmd_vm *vm, int fd)
> /* Execute the vcpu run loop(s) for this VM */
> ret = run_vm(vm->vm_cdrom, vm->vm_disks, nicfds, &vm->vm_params, &vrs);
>
> + /* Ensure that any in-flight data is written back */
> + virtio_shutdown(vm);
> +
> return (ret);
> }
>
>
> --
> Ori Bernstein
>
