| Message ID | 1370249911-19708-3-git-send-email-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 06/03/2013 02:58 AM, Stefan Hajnoczi wrote: > If the user fails to specify 'chardev' or 'shm' then we cannot continue. > Exit right away so that we don't invoke shm_open(3) with a NULL pointer. > > It would be nice to replace exit(1) with error returns in the PCI device > .init() function, but leave that for another patch since exit(1) is > currently used elsewhere. > > Spotted by Coverity. > > Cc: Cam Macdonell <cam@cs.ualberta.ca> > Cc: qemu-stable@nongnu.org > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > hw/misc/ivshmem.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c > index a19a6d6..5658f73 100644 > --- a/hw/misc/ivshmem.c > +++ b/hw/misc/ivshmem.c > @@ -735,6 +735,7 @@ static int pci_ivshmem_init(PCIDevice *dev) > > if (s->shmobj == NULL) { > fprintf(stderr, "Must specify 'chardev' or 'shm' to ivshmem\n"); > + exit(1); Reviewed-by: Eric Blake <eblake@redhat.com> However, I prefer exit(EXIT_FAILURE) rather than exit(1), to make it a bit easier to grep for known failure exits. Libvirt has a syntax checker (taken from gnulib) that enforces such a style, if qemu would like to adopt that style.
On Mon, Jun 03, 2013 at 06:26:22AM -0600, Eric Blake wrote: > On 06/03/2013 02:58 AM, Stefan Hajnoczi wrote: > > If the user fails to specify 'chardev' or 'shm' then we cannot continue. > > Exit right away so that we don't invoke shm_open(3) with a NULL pointer. > > > > It would be nice to replace exit(1) with error returns in the PCI device > > .init() function, but leave that for another patch since exit(1) is > > currently used elsewhere. > > > > Spotted by Coverity. > > > > Cc: Cam Macdonell <cam@cs.ualberta.ca> > > Cc: qemu-stable@nongnu.org > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > hw/misc/ivshmem.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c > > index a19a6d6..5658f73 100644 > > --- a/hw/misc/ivshmem.c > > +++ b/hw/misc/ivshmem.c > > @@ -735,6 +735,7 @@ static int pci_ivshmem_init(PCIDevice *dev) > > > > if (s->shmobj == NULL) { > > fprintf(stderr, "Must specify 'chardev' or 'shm' to ivshmem\n"); > > + exit(1); > > Reviewed-by: Eric Blake <eblake@redhat.com> > > However, I prefer exit(EXIT_FAILURE) rather than exit(1), to make it a > bit easier to grep for known failure exits. Libvirt has a syntax > checker (taken from gnulib) that enforces such a style, if qemu would > like to adopt that style. I like EXIT_FAILURE too but the rest of the file inconsistently uses exit(1) and exit(-1). I decided exit(1) was the least evil but the longer term fix is to return instead of exiting.
On Mon, Jun 03, 2013 at 10:58:31AM +0200, Stefan Hajnoczi wrote: > If the user fails to specify 'chardev' or 'shm' then we cannot continue. > Exit right away so that we don't invoke shm_open(3) with a NULL pointer. > > It would be nice to replace exit(1) with error returns in the PCI device > .init() function, but leave that for another patch since exit(1) is > currently used elsewhere. > > Spotted by Coverity. > > Cc: Cam Macdonell <cam@cs.ualberta.ca> > Cc: qemu-stable@nongnu.org > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > hw/misc/ivshmem.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c > index a19a6d6..5658f73 100644 > --- a/hw/misc/ivshmem.c > +++ b/hw/misc/ivshmem.c > @@ -735,6 +735,7 @@ static int pci_ivshmem_init(PCIDevice *dev) > > if (s->shmobj == NULL) { > fprintf(stderr, "Must specify 'chardev' or 'shm' to ivshmem\n"); > + exit(1); > } > > IVSHMEM_DPRINTF("using shm_open (shm object = %s)\n", s->shmobj); > -- > 1.8.1.4 > Ping?
Am 03.06.2013 15:11, schrieb Stefan Hajnoczi: > On Mon, Jun 03, 2013 at 06:26:22AM -0600, Eric Blake wrote: >> On 06/03/2013 02:58 AM, Stefan Hajnoczi wrote: >>> If the user fails to specify 'chardev' or 'shm' then we cannot continue. >>> Exit right away so that we don't invoke shm_open(3) with a NULL pointer. >>> >>> It would be nice to replace exit(1) with error returns in the PCI device >>> .init() function, but leave that for another patch since exit(1) is >>> currently used elsewhere. >>> >>> Spotted by Coverity. >>> >>> Cc: Cam Macdonell <cam@cs.ualberta.ca> >>> Cc: qemu-stable@nongnu.org >>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> >>> --- >>> hw/misc/ivshmem.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c >>> index a19a6d6..5658f73 100644 >>> --- a/hw/misc/ivshmem.c >>> +++ b/hw/misc/ivshmem.c >>> @@ -735,6 +735,7 @@ static int pci_ivshmem_init(PCIDevice *dev) >>> >>> if (s->shmobj == NULL) { >>> fprintf(stderr, "Must specify 'chardev' or 'shm' to ivshmem\n"); >>> + exit(1); >> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> >> However, I prefer exit(EXIT_FAILURE) rather than exit(1), to make it a >> bit easier to grep for known failure exits. Libvirt has a syntax >> checker (taken from gnulib) that enforces such a style, if qemu would >> like to adopt that style. > > I like EXIT_FAILURE too but the rest of the file inconsistently uses > exit(1) and exit(-1). I decided exit(1) was the least evil but the > longer term fix is to return instead of exiting. QOM realize would solve that, but similar to virtio the PCI qdev initfn actually does something (allocating the config region among others) so faces the same design questions of how to best inherit QOM methods as currently being discussed. Shouldn't hold up a bug fix like this one. Andreas
12.06.2013 11:22, Stefan Hajnoczi wrote: >> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c >> index a19a6d6..5658f73 100644 >> --- a/hw/misc/ivshmem.c >> +++ b/hw/misc/ivshmem.c >> @@ -735,6 +735,7 @@ static int pci_ivshmem_init(PCIDevice *dev) >> >> if (s->shmobj == NULL) { >> fprintf(stderr, "Must specify 'chardev' or 'shm' to ivshmem\n"); >> + exit(1); >> } >> >> IVSHMEM_DPRINTF("using shm_open (shm object = %s)\n", s->shmobj); > > Ping? Ow. I missed it because part 1/2 were applied to scsi branch by Paolo and I marked whole thread as "done". Applied this 2/2 now to the trivial patches tree. Thank you! /mjt
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c index a19a6d6..5658f73 100644 --- a/hw/misc/ivshmem.c +++ b/hw/misc/ivshmem.c @@ -735,6 +735,7 @@ static int pci_ivshmem_init(PCIDevice *dev) if (s->shmobj == NULL) { fprintf(stderr, "Must specify 'chardev' or 'shm' to ivshmem\n"); + exit(1); } IVSHMEM_DPRINTF("using shm_open (shm object = %s)\n", s->shmobj);
If the user fails to specify 'chardev' or 'shm' then we cannot continue. Exit right away so that we don't invoke shm_open(3) with a NULL pointer. It would be nice to replace exit(1) with error returns in the PCI device .init() function, but leave that for another patch since exit(1) is currently used elsewhere. Spotted by Coverity. Cc: Cam Macdonell <cam@cs.ualberta.ca> Cc: qemu-stable@nongnu.org Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- hw/misc/ivshmem.c | 1 + 1 file changed, 1 insertion(+)