Re: Qcow2: Clean up logging/error handling
On Sat, Nov 03, 2018 at 01:53:08PM -0700, Ori Bernstein wrote:
> On Tue, 30 Oct 2018 23:01:50 -0700, Mike Larkin wrote:
>
> > On Tue, Oct 30, 2018 at 10:41:21PM -0700, o...@eigenstate.org wrote:
> > > > On Tue, Oct 30, 2018 at 10:32:37PM -0700, Ori Bernstein wrote:
> > > >> On Tue, 30 Oct 2018 22:29:01 -0700, Mike Larkin
> > > >> wrote:
> > > >>
> > > >> > > - if (disk->base->clustersz != disk->clustersz) {
> > > >> > > - log_warn("%s: all disks must share clustersize",
> > > >> > > + fatal("%s: could not open %s", basepath,
> > > >> > > __func__);
> > > >> >
> > > >> > is this right?
> > > >> >
> > > >> > > + if (qc2_open(disk->base, fds + 1, nfd - 1) == -1)
> > > >> > > + fatalx("%s: could not open %s", basepath,
> > > >> > > __func__);
> > > >> >
> > > >> > same
> > > >> >
> > > >> > > + if (disk->base->clustersz != disk->clustersz)
> > > >> > > + fatalx("%s: all disk parts must share
> > > >> > > clustersize",
> > > >> > >__func__);
> > > >> > > - goto error;
> > > >> > > - }
> > > >> > > - }
> > > >>
> > > >> Good question. I think from earlier discussion, we wanted to fail if
> > > >> we could
> > > >> not open a disk image -- especially since these ones are actually
> > > >> *parts* of
> > > >> a disk image, meaning that we've got a corrupt image.
> > > >>
> > > >> I can easily change these back to warns + error returns.
> > > >>
> > > >> --
> > > >> Ori Bernstein
> > > >
> > > > I was referring to the argument order.
> > >
> > > Oh, that's obviously wrong. Updated.
> > >
> >
> > with that fix, go for it. we can hack on it further in-tree.
> >
>
> Anyone want to give me a second ok?
>
2nd OK reyk@
Re: Qcow2: Clean up logging/error handling
On Tue, 30 Oct 2018 23:01:50 -0700, Mike Larkin wrote:
> On Tue, Oct 30, 2018 at 10:41:21PM -0700, o...@eigenstate.org wrote:
> > > On Tue, Oct 30, 2018 at 10:32:37PM -0700, Ori Bernstein wrote:
> > >> On Tue, 30 Oct 2018 22:29:01 -0700, Mike Larkin
> > >> wrote:
> > >>
> > >> > > -if (disk->base->clustersz != disk->clustersz) {
> > >> > > -log_warn("%s: all disks must share clustersize",
> > >> > > +fatal("%s: could not open %s", basepath,
> > >> > > __func__);
> > >> >
> > >> > is this right?
> > >> >
> > >> > > +if (qc2_open(disk->base, fds + 1, nfd - 1) == -1)
> > >> > > +fatalx("%s: could not open %s", basepath,
> > >> > > __func__);
> > >> >
> > >> > same
> > >> >
> > >> > > +if (disk->base->clustersz != disk->clustersz)
> > >> > > +fatalx("%s: all disk parts must share
> > >> > > clustersize",
> > >> > > __func__);
> > >> > > -goto error;
> > >> > > -}
> > >> > > -}
> > >>
> > >> Good question. I think from earlier discussion, we wanted to fail if we
> > >> could
> > >> not open a disk image -- especially since these ones are actually
> > >> *parts* of
> > >> a disk image, meaning that we've got a corrupt image.
> > >>
> > >> I can easily change these back to warns + error returns.
> > >>
> > >> --
> > >> Ori Bernstein
> > >
> > > I was referring to the argument order.
> >
> > Oh, that's obviously wrong. Updated.
> >
>
> with that fix, go for it. we can hack on it further in-tree.
>
Anyone want to give me a second ok?
--
Ori Bernstein
Re: Qcow2: Clean up logging/error handling
On Tue, Oct 30, 2018 at 10:41:21PM -0700, o...@eigenstate.org wrote:
> > On Tue, Oct 30, 2018 at 10:32:37PM -0700, Ori Bernstein wrote:
> >> On Tue, 30 Oct 2018 22:29:01 -0700, Mike Larkin
> >> wrote:
> >>
> >> > > - if (disk->base->clustersz != disk->clustersz) {
> >> > > - log_warn("%s: all disks must share clustersize",
> >> > > + fatal("%s: could not open %s", basepath,
> >> > > __func__);
> >> >
> >> > is this right?
> >> >
> >> > > + if (qc2_open(disk->base, fds + 1, nfd - 1) == -1)
> >> > > + fatalx("%s: could not open %s", basepath,
> >> > > __func__);
> >> >
> >> > same
> >> >
> >> > > + if (disk->base->clustersz != disk->clustersz)
> >> > > + fatalx("%s: all disk parts must share
> >> > > clustersize",
> >> > >__func__);
> >> > > - goto error;
> >> > > - }
> >> > > - }
> >>
> >> Good question. I think from earlier discussion, we wanted to fail if we
> >> could
> >> not open a disk image -- especially since these ones are actually *parts*
> >> of
> >> a disk image, meaning that we've got a corrupt image.
> >>
> >> I can easily change these back to warns + error returns.
> >>
> >> --
> >> Ori Bernstein
> >
> > I was referring to the argument order.
>
> Oh, that's obviously wrong. Updated.
>
with that fix, go for it. we can hack on it further in-tree.
> diff --git usr.sbin/vmd/config.c usr.sbin/vmd/config.c
> index 6e7ed279d6b..0860ad0e7d2 100644
> --- usr.sbin/vmd/config.c
> +++ usr.sbin/vmd/config.c
> @@ -284,8 +284,7 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm,
> uint32_t peerid, uid_t uid)
>*/
> if (kernfd == -1 && !vmboot &&
> (kernfd = open(VM_DEFAULT_BIOS, O_RDONLY)) == -1) {
> - log_warn("%s: can't open %s", __func__,
> - VM_DEFAULT_BIOS);
> + log_warn("can't open %s", VM_DEFAULT_BIOS);
> errno = VMD_BIOS_MISSING;
> goto fail;
> }
> @@ -305,8 +304,7 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm,
> uint32_t peerid, uid_t uid)
> /* Stat cdrom to ensure it is a regular file */
> if ((cdromfd =
> open(vcp->vcp_cdrom, O_RDONLY)) == -1) {
> - log_warn("%s: can't open cdrom %s", __func__,
> - vcp->vcp_cdrom);
> + log_warn("can't open cdrom %s", vcp->vcp_cdrom);
> errno = VMD_CDROM_MISSING;
> goto fail;
> }
> @@ -325,14 +323,14 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm,
> uint32_t peerid, uid_t uid)
> for (i = 0 ; i < vcp->vcp_ndisks; i++) {
> if (strlcpy(path, vcp->vcp_disks[i], sizeof(path))
> >= sizeof(path))
> - log_warnx("%s, disk path too long", __func__);
> + log_warnx("disk path %s too long", vcp->vcp_disks[i]);
> memset(vmc->vmc_diskbases, 0, sizeof(vmc->vmc_diskbases));
> oflags = O_RDWR|O_EXLOCK|O_NONBLOCK;
> aflags = R_OK|W_OK;
> for (j = 0; j < VM_MAX_BASE_PER_DISK; j++) {
> /* Stat disk[i] to ensure it is a regular file */
> if ((diskfds[i][j] = open(path, oflags)) == -1) {
> - log_warn("%s: can't open disk %s", __func__,
> + log_warn("can't open disk %s",
> vcp->vcp_disks[i]);
> errno = VMD_DISK_MISSING;
> goto fail;
> @@ -487,7 +485,7 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm,
> uint32_t peerid, uid_t uid)
>
> fail:
> saved_errno = errno;
> - log_warnx("%s: failed to start vm %s", __func__, vcp->vcp_name);
> + log_warnx("failed to start vm %s", vcp->vcp_name);
>
> if (kernfd != -1)
> close(kernfd);
> diff --git usr.sbin/vmd/vioqcow2.c usr.sbin/vmd/vioqcow2.c
> index 28b087df39a..af762fcdf45 100644
> --- usr.sbin/vmd/vioqcow2.c
> +++ usr.sbin/vmd/vioqcow2.c
> @@ -102,9 +102,9 @@ struct qcdisk {
> extern char *__progname;
>
> static off_t xlate(struct qcdisk *, off_t, int *);
> -static int copy_cluster(struct qcdisk *, struct qcdisk *, off_t, off_t);
> +static void copy_cluster(struct qcdisk *, struct qcdisk *, off_t, off_t);
> +static void inc_refs(struct qcdisk *, off_t, int);
> static off_t mkcluster(struct qcdisk *, struct qcdisk *, off_t, off_t);
> -static int inc_refs(struct qcdisk *, off_t, int);
> static int qc2_open(struct qcdisk *, int *, size_t);
> static ssize_t qc2_pread(void *, char *, size_t, off_t);
> static ssize_t qc2_pwrite(void *, char *, size_t, off_t);
> @@ -126,7 +126,7 @@ virtio_init_qcow2(struct
Re: Qcow2: Clean up logging/error handling
> On Tue, Oct 30, 2018 at 10:32:37PM -0700, Ori Bernstein wrote:
>> On Tue, 30 Oct 2018 22:29:01 -0700, Mike Larkin wrote:
>>
>> > > -if (disk->base->clustersz != disk->clustersz) {
>> > > -log_warn("%s: all disks must share clustersize",
>> > > +fatal("%s: could not open %s", basepath,
>> > > __func__);
>> >
>> > is this right?
>> >
>> > > +if (qc2_open(disk->base, fds + 1, nfd - 1) == -1)
>> > > +fatalx("%s: could not open %s", basepath,
>> > > __func__);
>> >
>> > same
>> >
>> > > +if (disk->base->clustersz != disk->clustersz)
>> > > +fatalx("%s: all disk parts must share
>> > > clustersize",
>> > > __func__);
>> > > -goto error;
>> > > -}
>> > > -}
>>
>> Good question. I think from earlier discussion, we wanted to fail if we could
>> not open a disk image -- especially since these ones are actually *parts* of
>> a disk image, meaning that we've got a corrupt image.
>>
>> I can easily change these back to warns + error returns.
>>
>> --
>> Ori Bernstein
>
> I was referring to the argument order.
Oh, that's obviously wrong. Updated.
diff --git usr.sbin/vmd/config.c usr.sbin/vmd/config.c
index 6e7ed279d6b..0860ad0e7d2 100644
--- usr.sbin/vmd/config.c
+++ usr.sbin/vmd/config.c
@@ -284,8 +284,7 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm,
uint32_t peerid, uid_t uid)
*/
if (kernfd == -1 && !vmboot &&
(kernfd = open(VM_DEFAULT_BIOS, O_RDONLY)) == -1) {
- log_warn("%s: can't open %s", __func__,
- VM_DEFAULT_BIOS);
+ log_warn("can't open %s", VM_DEFAULT_BIOS);
errno = VMD_BIOS_MISSING;
goto fail;
}
@@ -305,8 +304,7 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm,
uint32_t peerid, uid_t uid)
/* Stat cdrom to ensure it is a regular file */
if ((cdromfd =
open(vcp->vcp_cdrom, O_RDONLY)) == -1) {
- log_warn("%s: can't open cdrom %s", __func__,
- vcp->vcp_cdrom);
+ log_warn("can't open cdrom %s", vcp->vcp_cdrom);
errno = VMD_CDROM_MISSING;
goto fail;
}
@@ -325,14 +323,14 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm,
uint32_t peerid, uid_t uid)
for (i = 0 ; i < vcp->vcp_ndisks; i++) {
if (strlcpy(path, vcp->vcp_disks[i], sizeof(path))
>= sizeof(path))
- log_warnx("%s, disk path too long", __func__);
+ log_warnx("disk path %s too long", vcp->vcp_disks[i]);
memset(vmc->vmc_diskbases, 0, sizeof(vmc->vmc_diskbases));
oflags = O_RDWR|O_EXLOCK|O_NONBLOCK;
aflags = R_OK|W_OK;
for (j = 0; j < VM_MAX_BASE_PER_DISK; j++) {
/* Stat disk[i] to ensure it is a regular file */
if ((diskfds[i][j] = open(path, oflags)) == -1) {
- log_warn("%s: can't open disk %s", __func__,
+ log_warn("can't open disk %s",
vcp->vcp_disks[i]);
errno = VMD_DISK_MISSING;
goto fail;
@@ -487,7 +485,7 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm,
uint32_t peerid, uid_t uid)
fail:
saved_errno = errno;
- log_warnx("%s: failed to start vm %s", __func__, vcp->vcp_name);
+ log_warnx("failed to start vm %s", vcp->vcp_name);
if (kernfd != -1)
close(kernfd);
diff --git usr.sbin/vmd/vioqcow2.c usr.sbin/vmd/vioqcow2.c
index 28b087df39a..af762fcdf45 100644
--- usr.sbin/vmd/vioqcow2.c
+++ usr.sbin/vmd/vioqcow2.c
@@ -102,9 +102,9 @@ struct qcdisk {
extern char *__progname;
static off_t xlate(struct qcdisk *, off_t, int *);
-static int copy_cluster(struct qcdisk *, struct qcdisk *, off_t, off_t);
+static void copy_cluster(struct qcdisk *, struct qcdisk *, off_t, off_t);
+static void inc_refs(struct qcdisk *, off_t, int);
static off_t mkcluster(struct qcdisk *, struct qcdisk *, off_t, off_t);
-static int inc_refs(struct qcdisk *, off_t, int);
static int qc2_open(struct qcdisk *, int *, size_t);
static ssize_t qc2_pread(void *, char *, size_t, off_t);
static ssize_t qc2_pwrite(void *, char *, size_t, off_t);
@@ -126,7 +126,7 @@ virtio_init_qcow2(struct virtio_backing *file, off_t *szp,
int *fd, size_t nfd)
if (diskp == NULL)
return -1;
if (qc2_open(diskp, fd, nfd) == -1) {
- log_warnx("%s: could not open qcow2 disk", __func__);
+ log_warnx("could not open qcow2
Re: Qcow2: Clean up logging/error handling
On Tue, Oct 30, 2018 at 10:32:37PM -0700, Ori Bernstein wrote:
> On Tue, 30 Oct 2018 22:29:01 -0700, Mike Larkin wrote:
>
> > > - if (disk->base->clustersz != disk->clustersz) {
> > > - log_warn("%s: all disks must share clustersize",
> > > + fatal("%s: could not open %s", basepath, __func__);
> >
> > is this right?
> >
> > > + if (qc2_open(disk->base, fds + 1, nfd - 1) == -1)
> > > + fatalx("%s: could not open %s", basepath, __func__);
> >
> > same
> >
> > > + if (disk->base->clustersz != disk->clustersz)
> > > + fatalx("%s: all disk parts must share clustersize",
> > > __func__);
> > > - goto error;
> > > - }
> > > - }
>
> Good question. I think from earlier discussion, we wanted to fail if we could
> not open a disk image -- especially since these ones are actually *parts* of
> a disk image, meaning that we've got a corrupt image.
>
> I can easily change these back to warns + error returns.
>
> --
> Ori Bernstein
I was referring to the argument order.
Re: Qcow2: Clean up logging/error handling
On Tue, 30 Oct 2018 22:29:01 -0700, Mike Larkin wrote:
> > - if (disk->base->clustersz != disk->clustersz) {
> > - log_warn("%s: all disks must share clustersize",
> > + fatal("%s: could not open %s", basepath, __func__);
>
> is this right?
>
> > + if (qc2_open(disk->base, fds + 1, nfd - 1) == -1)
> > + fatalx("%s: could not open %s", basepath, __func__);
>
> same
>
> > + if (disk->base->clustersz != disk->clustersz)
> > + fatalx("%s: all disk parts must share clustersize",
> > __func__);
> > - goto error;
> > - }
> > - }
Good question. I think from earlier discussion, we wanted to fail if we could
not open a disk image -- especially since these ones are actually *parts* of
a disk image, meaning that we've got a corrupt image.
I can easily change these back to warns + error returns.
--
Ori Bernstein
Re: Qcow2: Clean up logging/error handling
On Tue, Oct 30, 2018 at 10:01:08PM -0700, Ori Bernstein wrote:
> On Sun, 28 Oct 2018 00:58:57 +0200, Reyk Floeter wrote:
>
> > Most of these are fatal and log_debug. Keep the __func__ there! But we___ll
> > remove it from other logging functions where it was never intended to be
> > used and potentially reword the warnings nicely.
> >
> > Reyk
> >
>
> Alright, updated accordingly, with __func__ on fatals, and without it
> on __warn__s. Also touched a few places where I noticed it in config.c.
>
> Ok?
>
see below.
>
> diff --git usr.sbin/vmd/config.c usr.sbin/vmd/config.c
> index 6e7ed279d6b..7dc2b266dfe 100644
> --- usr.sbin/vmd/config.c
> +++ usr.sbin/vmd/config.c
> @@ -284,8 +284,7 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm,
> uint32_t peerid, uid_t uid)
>*/
> if (kernfd == -1 && !vmboot &&
> (kernfd = open(VM_DEFAULT_BIOS, O_RDONLY)) == -1) {
> - log_warn("%s: can't open %s", __func__,
> - VM_DEFAULT_BIOS);
> + log_warn("can't open %s", VM_DEFAULT_BIOS);
> errno = VMD_BIOS_MISSING;
> goto fail;
> }
> @@ -305,8 +304,7 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm,
> uint32_t peerid, uid_t uid)
> /* Stat cdrom to ensure it is a regular file */
> if ((cdromfd =
> open(vcp->vcp_cdrom, O_RDONLY)) == -1) {
> - log_warn("%s: can't open cdrom %s", __func__,
> - vcp->vcp_cdrom);
> + log_warn("can't open cdrom %s", vcp->vcp_cdrom);
> errno = VMD_CDROM_MISSING;
> goto fail;
> }
> @@ -325,14 +323,14 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm,
> uint32_t peerid, uid_t uid)
> for (i = 0 ; i < vcp->vcp_ndisks; i++) {
> if (strlcpy(path, vcp->vcp_disks[i], sizeof(path))
> >= sizeof(path))
> - log_warnx("%s, disk path too long", __func__);
> + log_warnx("disk path %s too long", vcp->vcp_disks[i]);
> memset(vmc->vmc_diskbases, 0, sizeof(vmc->vmc_diskbases));
> oflags = O_RDWR|O_EXLOCK|O_NONBLOCK;
> aflags = R_OK|W_OK;
> for (j = 0; j < VM_MAX_BASE_PER_DISK; j++) {
> /* Stat disk[i] to ensure it is a regular file */
> if ((diskfds[i][j] = open(path, oflags)) == -1) {
> - log_warn("%s: can't open disk %s", __func__,
> + log_warn("can't open disk %s",
> vcp->vcp_disks[i]);
> errno = VMD_DISK_MISSING;
> goto fail;
> @@ -487,7 +485,7 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm,
> uint32_t peerid, uid_t uid)
>
> fail:
> saved_errno = errno;
> - log_warnx("%s: failed to start vm %s", __func__, vcp->vcp_name);
> + log_warnx("failed to start vm %s", __func__, vcp->vcp_name);
>
> if (kernfd != -1)
> close(kernfd);
> diff --git usr.sbin/vmd/vioqcow2.c usr.sbin/vmd/vioqcow2.c
> index 28b087df39a..de9068827c5 100644
> --- usr.sbin/vmd/vioqcow2.c
> +++ usr.sbin/vmd/vioqcow2.c
> @@ -102,9 +102,9 @@ struct qcdisk {
> extern char *__progname;
>
> static off_t xlate(struct qcdisk *, off_t, int *);
> -static int copy_cluster(struct qcdisk *, struct qcdisk *, off_t, off_t);
> +static void copy_cluster(struct qcdisk *, struct qcdisk *, off_t, off_t);
> +static void inc_refs(struct qcdisk *, off_t, int);
> static off_t mkcluster(struct qcdisk *, struct qcdisk *, off_t, off_t);
> -static int inc_refs(struct qcdisk *, off_t, int);
> static int qc2_open(struct qcdisk *, int *, size_t);
> static ssize_t qc2_pread(void *, char *, size_t, off_t);
> static ssize_t qc2_pwrite(void *, char *, size_t, off_t);
> @@ -126,7 +126,7 @@ virtio_init_qcow2(struct virtio_backing *file, off_t
> *szp, int *fd, size_t nfd)
> if (diskp == NULL)
> return -1;
> if (qc2_open(diskp, fd, nfd) == -1) {
> - log_warnx("%s: could not open qcow2 disk", __func__);
> + log_warnx("could not open qcow2 disk");
> return -1;
> }
> file->p = diskp;
> @@ -137,6 +137,10 @@ virtio_init_qcow2(struct virtio_backing *file, off_t
> *szp, int *fd, size_t nfd)
> return 0;
> }
>
> +/*
> + * Return the path to the base image given a disk image.
> + * Called from vmctl.
> + */
> ssize_t
> virtio_qcow2_get_base(int fd, char *path, size_t npath, const char *dpath)
> {
> @@ -147,11 +151,11 @@ virtio_qcow2_get_base(int fd, char *path, size_t npath,
> const char *dpath)
> char *s = NULL;
>
> if (pread(fd, , sizeof(header), 0) != sizeof(header)) {
> - log_warnx("%s: short read on header", __func__);
> + log_warnx("short
Re: Qcow2: Clean up logging/error handling
On Sun, 28 Oct 2018 00:58:57 +0200, Reyk Floeter wrote:
> Most of these are fatal and log_debug. Keep the __func__ there! But we___ll
> remove it from other logging functions where it was never intended to be
> used and potentially reword the warnings nicely.
>
> Reyk
>
Alright, updated accordingly, with __func__ on fatals, and without it
on __warn__s. Also touched a few places where I noticed it in config.c.
Ok?
diff --git usr.sbin/vmd/config.c usr.sbin/vmd/config.c
index 6e7ed279d6b..7dc2b266dfe 100644
--- usr.sbin/vmd/config.c
+++ usr.sbin/vmd/config.c
@@ -284,8 +284,7 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm,
uint32_t peerid, uid_t uid)
*/
if (kernfd == -1 && !vmboot &&
(kernfd = open(VM_DEFAULT_BIOS, O_RDONLY)) == -1) {
- log_warn("%s: can't open %s", __func__,
- VM_DEFAULT_BIOS);
+ log_warn("can't open %s", VM_DEFAULT_BIOS);
errno = VMD_BIOS_MISSING;
goto fail;
}
@@ -305,8 +304,7 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm,
uint32_t peerid, uid_t uid)
/* Stat cdrom to ensure it is a regular file */
if ((cdromfd =
open(vcp->vcp_cdrom, O_RDONLY)) == -1) {
- log_warn("%s: can't open cdrom %s", __func__,
- vcp->vcp_cdrom);
+ log_warn("can't open cdrom %s", vcp->vcp_cdrom);
errno = VMD_CDROM_MISSING;
goto fail;
}
@@ -325,14 +323,14 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm,
uint32_t peerid, uid_t uid)
for (i = 0 ; i < vcp->vcp_ndisks; i++) {
if (strlcpy(path, vcp->vcp_disks[i], sizeof(path))
>= sizeof(path))
- log_warnx("%s, disk path too long", __func__);
+ log_warnx("disk path %s too long", vcp->vcp_disks[i]);
memset(vmc->vmc_diskbases, 0, sizeof(vmc->vmc_diskbases));
oflags = O_RDWR|O_EXLOCK|O_NONBLOCK;
aflags = R_OK|W_OK;
for (j = 0; j < VM_MAX_BASE_PER_DISK; j++) {
/* Stat disk[i] to ensure it is a regular file */
if ((diskfds[i][j] = open(path, oflags)) == -1) {
- log_warn("%s: can't open disk %s", __func__,
+ log_warn("can't open disk %s",
vcp->vcp_disks[i]);
errno = VMD_DISK_MISSING;
goto fail;
@@ -487,7 +485,7 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm,
uint32_t peerid, uid_t uid)
fail:
saved_errno = errno;
- log_warnx("%s: failed to start vm %s", __func__, vcp->vcp_name);
+ log_warnx("failed to start vm %s", __func__, vcp->vcp_name);
if (kernfd != -1)
close(kernfd);
diff --git usr.sbin/vmd/vioqcow2.c usr.sbin/vmd/vioqcow2.c
index 28b087df39a..de9068827c5 100644
--- usr.sbin/vmd/vioqcow2.c
+++ usr.sbin/vmd/vioqcow2.c
@@ -102,9 +102,9 @@ struct qcdisk {
extern char *__progname;
static off_t xlate(struct qcdisk *, off_t, int *);
-static int copy_cluster(struct qcdisk *, struct qcdisk *, off_t, off_t);
+static void copy_cluster(struct qcdisk *, struct qcdisk *, off_t, off_t);
+static void inc_refs(struct qcdisk *, off_t, int);
static off_t mkcluster(struct qcdisk *, struct qcdisk *, off_t, off_t);
-static int inc_refs(struct qcdisk *, off_t, int);
static int qc2_open(struct qcdisk *, int *, size_t);
static ssize_t qc2_pread(void *, char *, size_t, off_t);
static ssize_t qc2_pwrite(void *, char *, size_t, off_t);
@@ -126,7 +126,7 @@ virtio_init_qcow2(struct virtio_backing *file, off_t *szp,
int *fd, size_t nfd)
if (diskp == NULL)
return -1;
if (qc2_open(diskp, fd, nfd) == -1) {
- log_warnx("%s: could not open qcow2 disk", __func__);
+ log_warnx("could not open qcow2 disk");
return -1;
}
file->p = diskp;
@@ -137,6 +137,10 @@ virtio_init_qcow2(struct virtio_backing *file, off_t *szp,
int *fd, size_t nfd)
return 0;
}
+/*
+ * Return the path to the base image given a disk image.
+ * Called from vmctl.
+ */
ssize_t
virtio_qcow2_get_base(int fd, char *path, size_t npath, const char *dpath)
{
@@ -147,11 +151,11 @@ virtio_qcow2_get_base(int fd, char *path, size_t npath,
const char *dpath)
char *s = NULL;
if (pread(fd, , sizeof(header), 0) != sizeof(header)) {
- log_warnx("%s: short read on header", __func__);
+ log_warnx("short read on header");
return -1;
}
if (strncmp(header.magic, VM_MAGIC_QCOW, strlen(VM_MAGIC_QCOW)) != 0) {
- log_warn("%s: invalid magic numbers", __func__);
Re: Qcow2: Clean up logging/error handling
On Sun, Oct 28, 2018 at 12:58:57AM +0200, Reyk Floeter wrote: > Most of these are fatal and log_debug. Keep the __func__ there! But we’ll > remove it from other logging functions where it was never intended to be used > and potentially reword the warnings nicely. > > Reyk > Yep. If I added some __func__ to non-fatal/debug things, they should be removed too. -ml > > Am 28.10.2018 um 00:39 schrieb Ori Bernstein : > > > >> On Sat, 27 Oct 2018 16:15:32 -0600, "Theo de Raadt" > >> wrote: > >> > >> I quite dislike when software uses __func__ to tell me what internal > >> function had an error. > >> > >> Why should a user see an error with the string virtio_qcow2_get_base, > >> qc2_open, qc2_pwrite, etc? > >> > >> It feels unpolished. > >> > > > > Possibly, but it is consistent with the rest of vmd (16 instances are > > from vioqcow2.c) > > > >$ grep log.*__func__ *.c | wc -l > >162 > > > > But many of them are on lines that wrap. Overcounting a bit: > > > >$ grep __func__ *.c | wc -l > >474 > > > > If we want to change it, I think it's better as an independent patch that > > converts all of vmd. > > > > -- > >Ori Bernstein > > >
Re: Qcow2: Clean up logging/error handling
Most of these are fatal and log_debug. Keep the __func__ there! But we’ll remove it from other logging functions where it was never intended to be used and potentially reword the warnings nicely. Reyk > Am 28.10.2018 um 00:39 schrieb Ori Bernstein : > >> On Sat, 27 Oct 2018 16:15:32 -0600, "Theo de Raadt" >> wrote: >> >> I quite dislike when software uses __func__ to tell me what internal >> function had an error. >> >> Why should a user see an error with the string virtio_qcow2_get_base, >> qc2_open, qc2_pwrite, etc? >> >> It feels unpolished. >> > > Possibly, but it is consistent with the rest of vmd (16 instances are > from vioqcow2.c) > >$ grep log.*__func__ *.c | wc -l >162 > > But many of them are on lines that wrap. Overcounting a bit: > >$ grep __func__ *.c | wc -l >474 > > If we want to change it, I think it's better as an independent patch that > converts all of vmd. > > -- >Ori Bernstein >
Re: Qcow2: Clean up logging/error handling
> > I quite dislike when software uses __func__ to tell me what internal > > function had an error. > > > > Why should a user see an error with the string virtio_qcow2_get_base, > > qc2_open, qc2_pwrite, etc? > > > > It feels unpolished. > > > > Possibly, but it is consistent with the rest of vmd (16 instances are > from vioqcow2.c) > > $ grep log.*__func__ *.c | wc -l > 162 > > But many of them are on lines that wrap. Overcounting a bit: > > $ grep __func__ *.c | wc -l > 474 > > If we want to change it, I think it's better as an independent patch that > converts all of vmd. OK, then I change my comment to consistantly unpolished and getting less polished. Yes, I understand it is a powerful approach during debugging. But vmd should be getting past that stage. Especially at this layer. Each of these conditions are well understood. Dialogue a clear error message and return upwards.
Re: Qcow2: Clean up logging/error handling
On Sat, 27 Oct 2018 16:15:32 -0600, "Theo de Raadt" wrote: > I quite dislike when software uses __func__ to tell me what internal > function had an error. > > Why should a user see an error with the string virtio_qcow2_get_base, > qc2_open, qc2_pwrite, etc? > > It feels unpolished. > Possibly, but it is consistent with the rest of vmd (16 instances are from vioqcow2.c) $ grep log.*__func__ *.c | wc -l 162 But many of them are on lines that wrap. Overcounting a bit: $ grep __func__ *.c | wc -l 474 If we want to change it, I think it's better as an independent patch that converts all of vmd. -- Ori Bernstein
Re: Qcow2: Clean up logging/error handling
I quite dislike when software uses __func__ to tell me what internal function had an error. Why should a user see an error with the string virtio_qcow2_get_base, qc2_open, qc2_pwrite, etc? It feels unpolished.
Re: Qcow2: Clean up logging/error handling
On Wed, 24 Oct 2018 16:23:29 +0800, Michael Mikonos wrote:
> On Tue, Oct 23, 2018 at 09:44:24PM -0700, Ori Bernstein wrote:
> > This patch turns most warnings into errors, and uses the
> > appropriate fatal/fatalx so that we don't print bogus error
> > strings. It also adds checks for unsupported refcount sizes
> > and writes that clobber the header.
> >
> > Ok?
>
> Hello, two comments below.
Updated. Also changed some fatalx'es to fatals, so we get the errno, and
removed status returns plus associated checks from functions that no longer
had a failure path.
diff --git usr.sbin/vmd/vioqcow2.c usr.sbin/vmd/vioqcow2.c
index 3a215599d49..12301650127 100644
--- usr.sbin/vmd/vioqcow2.c
+++ usr.sbin/vmd/vioqcow2.c
@@ -79,15 +79,15 @@ struct qcdisk {
int fd;
uint64_t *l1;
off_t end;
- uint32_t clustersz;
+ off_t clustersz;
off_t disksz; /* In bytes */
- uint32_t cryptmethod;
+ uint32_t cryptmethod;
uint32_t l1sz;
off_tl1off;
off_trefoff;
- uint32_t refsz;
+ off_trefsz;
uint32_t nsnap;
off_tsnapoff;
@@ -102,9 +102,9 @@ struct qcdisk {
extern char *__progname;
static off_t xlate(struct qcdisk *, off_t, int *);
-static int copy_cluster(struct qcdisk *, struct qcdisk *, off_t, off_t);
+static void copy_cluster(struct qcdisk *, struct qcdisk *, off_t, off_t);
+static void inc_refs(struct qcdisk *, off_t, int);
static off_t mkcluster(struct qcdisk *, struct qcdisk *, off_t, off_t);
-static int inc_refs(struct qcdisk *, off_t, int);
static int qc2_open(struct qcdisk *, int *, size_t);
static ssize_t qc2_pread(void *, char *, size_t, off_t);
static ssize_t qc2_pwrite(void *, char *, size_t, off_t);
@@ -137,6 +137,10 @@ virtio_init_qcow2(struct virtio_backing *file, off_t *szp,
int *fd, size_t nfd)
return 0;
}
+/*
+ * Return the path to the base image given a disk image.
+ * Called from vmctl.
+ */
ssize_t
virtio_qcow2_get_base(int fd, char *path, size_t npath, const char *dpath)
{
@@ -151,7 +155,7 @@ virtio_qcow2_get_base(int fd, char *path, size_t npath,
const char *dpath)
return -1;
}
if (strncmp(header.magic, VM_MAGIC_QCOW, strlen(VM_MAGIC_QCOW)) != 0) {
- log_warn("%s: invalid magic numbers", __func__);
+ log_warnx("%s: invalid magic numbers", __func__);
return -1;
}
backingoff = be64toh(header.backingoff);
@@ -160,7 +164,7 @@ virtio_qcow2_get_base(int fd, char *path, size_t npath,
const char *dpath)
return 0;
if (backingsz >= npath - 1) {
- log_warn("%s: snapshot path too long", __func__);
+ log_warnx("%s: snapshot path too long", __func__);
return -1;
}
if (pread(fd, path, backingsz, backingoff) != backingsz) {
@@ -178,20 +182,19 @@ virtio_qcow2_get_base(int fd, char *path, size_t npath,
const char *dpath)
if (path[0] == '/') {
if (realpath(path, expanded) == NULL ||
strlcpy(path, expanded, npath) >= npath) {
- log_warn("unable to resolve %s", path);
+ log_warnx("unable to resolve %s", path);
return -1;
}
} else {
s = dirname(dpath);
if (snprintf(expanded, sizeof(expanded),
"%s/%s", s, path) >= (int)sizeof(expanded)) {
- log_warn("path too long: %s/%s",
- s, path);
+ log_warnx("path too long: %s/%s", s, path);
return -1;
}
if (npath < PATH_MAX ||
realpath(expanded, path) == NULL) {
- log_warn("unable to resolve %s", path);
+ log_warnx("unable to resolve %s", path);
return -1;
}
}
@@ -207,7 +210,7 @@ qc2_open(struct qcdisk *disk, int *fds, size_t nfd)
struct qcheader header;
uint64_t backingoff;
uint32_t backingsz;
- size_t i;
+ off_t i;
int version, fd;
pthread_rwlock_init(>lock, NULL);
@@ -216,15 +219,10 @@ qc2_open(struct qcdisk *disk, int *fds, size_t nfd)
disk->base = NULL;
disk->l1 = NULL;
- if (pread(fd, , sizeof(header), 0) != sizeof(header)) {
- log_warn("%s: short read on header", __func__);
- goto error;
- }
- if (strncmp(header.magic,
- VM_MAGIC_QCOW, strlen(VM_MAGIC_QCOW)) != 0) {
- log_warn("%s: invalid magic numbers", __func__);
- goto error;
- }
+ if (pread(fd, , sizeof(header), 0) != sizeof(header))
+ fatalx("%s: short read on header", __func__);
+ if (strncmp(header.magic, VM_MAGIC_QCOW, strlen(VM_MAGIC_QCOW)) != 0)
+
Re: Qcow2: Clean up logging/error handling
On Tue, Oct 23, 2018 at 09:44:24PM -0700, Ori Bernstein wrote:
> This patch turns most warnings into errors, and uses the
> appropriate fatal/fatalx so that we don't print bogus error
> strings. It also adds checks for unsupported refcount sizes
> and writes that clobber the header.
>
> Ok?
Hello, two comments below.
>
> diff --git usr.sbin/vmd/vioqcow2.c usr.sbin/vmd/vioqcow2.c
> index 3a215599d49..e550f3b84b5 100644
> --- usr.sbin/vmd/vioqcow2.c
> +++ usr.sbin/vmd/vioqcow2.c
> @@ -137,6 +137,12 @@ virtio_init_qcow2(struct virtio_backing *file, off_t
> *szp, int *fd, size_t nfd)
> return 0;
> }
>
> +/*
> + * Return the path to the base image given a disk image.
> + *
> + * This is used when resolving base images from vmd, so it should avoid
> + * fatalx'ing, or we will bring down multiple vms on a corrupt disk.
> + */
> ssize_t
> virtio_qcow2_get_base(int fd, char *path, size_t npath, const char *dpath)
> {
> @@ -151,7 +157,7 @@ virtio_qcow2_get_base(int fd, char *path, size_t npath,
> const char *dpath)
> return -1;
> }
> if (strncmp(header.magic, VM_MAGIC_QCOW, strlen(VM_MAGIC_QCOW)) != 0) {
> - log_warn("%s: invalid magic numbers", __func__);
> + log_warnx("%s: invalid magic numbers", __func__);
> return -1;
> }
> backingoff = be64toh(header.backingoff);
> @@ -160,7 +166,7 @@ virtio_qcow2_get_base(int fd, char *path, size_t npath,
> const char *dpath)
> return 0;
>
> if (backingsz >= npath - 1) {
> - log_warn("%s: snapshot path too long", __func__);
> + log_warnx("%s: snapshot path too long", __func__);
> return -1;
> }
> if (pread(fd, path, backingsz, backingoff) != backingsz) {
> @@ -178,20 +184,19 @@ virtio_qcow2_get_base(int fd, char *path, size_t npath,
> const char *dpath)
> if (path[0] == '/') {
> if (realpath(path, expanded) == NULL ||
> strlcpy(path, expanded, npath) >= npath) {
> - log_warn("unable to resolve %s", path);
> + log_warnx("unable to resolve %s", path);
> return -1;
> }
> } else {
> s = dirname(dpath);
> if (snprintf(expanded, sizeof(expanded),
> "%s/%s", s, path) >= (int)sizeof(expanded)) {
> - log_warn("path too long: %s/%s",
> - s, path);
> + log_warnx("path too long: %s/%s", s, path);
> return -1;
> }
> if (npath < PATH_MAX ||
> realpath(expanded, path) == NULL) {
> - log_warn("unable to resolve %s", path);
> + log_warnx("unable to resolve %s", path);
> return -1;
> }
> }
> @@ -216,15 +221,10 @@ qc2_open(struct qcdisk *disk, int *fds, size_t nfd)
> disk->base = NULL;
> disk->l1 = NULL;
>
> - if (pread(fd, , sizeof(header), 0) != sizeof(header)) {
> - log_warn("%s: short read on header", __func__);
> - goto error;
> - }
> - if (strncmp(header.magic,
> - VM_MAGIC_QCOW, strlen(VM_MAGIC_QCOW)) != 0) {
> - log_warn("%s: invalid magic numbers", __func__);
> - goto error;
> - }
> + if (pread(fd, , sizeof(header), 0) != sizeof(header))
> + fatalx("%s: short read on header", __func__);
> + if (strncmp(header.magic, VM_MAGIC_QCOW, strlen(VM_MAGIC_QCOW)) != 0)
> + fatalx("%s: invalid magic numbers", __func__);
>
> disk->clustersz = (1ull << be32toh(header.clustershift));
> disk->disksz= be64toh(header.disksz);
> @@ -249,79 +249,59 @@ qc2_open(struct qcdisk *disk, int *fds, size_t nfd)
> /*
>* We only know about the dirty or corrupt bits here.
>*/
> - if (disk->incompatfeatures & ~(QCOW2_DIRTY|QCOW2_CORRUPT)) {
> - log_warnx("%s: unsupported features %llx", __func__,
> + if (disk->incompatfeatures & ~(QCOW2_DIRTY|QCOW2_CORRUPT))
> + fatalx("%s: unsupported features %llx", __func__,
> disk->incompatfeatures & ~(QCOW2_DIRTY|QCOW2_CORRUPT));
> - goto error;
> - }
> + if (be32toh(header.reforder) != 4)
> + fatalx("%s: unsupported refcount size\n", __func__);
>
> disk->l1 = calloc(disk->l1sz, sizeof(*disk->l1));
> if (!disk->l1)
> - goto error;
> + fatalx("%s: could not allocate l1 table", __func__);
> if (pread(disk->fd, disk->l1, 8 * disk->l1sz, disk->l1off)
> - != 8 * disk->l1sz) {
> - log_warn("%s: unable to read qcow2 L1 table", __func__);
> - goto error;
> - }
> + != 8 * disk->l1sz)
> + fatalx("%s: unable to read qcow2 L1 table", __func__);
> for (i = 0; i < disk->l1sz; i++)
>
