| Message ID | 1260439868-15061-3-git-send-email-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Dec 10, 2009 at 11:11:06AM +0100, Gerd Hoffmann wrote: > Current PCI code will simply hw_error() and thus abort in case no free > PCI slot is available or the requested PCI slot is already in use by > another device. For the hotplug case this behavior is not acceptable. > This patch makes qemu pass up the error properly, so the calling code > can decide whenever it wants to exit with an error (on startup) or > whenever it wants to continue (hotplug). > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> Good stuff. However > --- > hw/pci.c | 11 +++++++++-- > 1 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/hw/pci.c b/hw/pci.c > index 4f662b7..404eead 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -580,11 +580,13 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, > if (!bus->devices[devfn]) > goto found; > } > - hw_error("PCI: no devfn available for %s, all in use\n", name); > + qemu_error("PCI: no devfn available for %s, all in use\n", name); > + return NULL; > found: ; > } else if (bus->devices[devfn]) { > - hw_error("PCI: devfn %d not available for %s, in use by %s\n", devfn, > + qemu_error("PCI: devfn %d not available for %s, in use by %s\n", devfn, > name, bus->devices[devfn]->name); > + return NULL; > } > pci_dev->bus = bus; > pci_dev->devfn = devfn; > @@ -625,6 +627,9 @@ PCIDevice *pci_register_device(PCIBus *bus, const char *name, > pci_dev = do_pci_register_device(pci_dev, bus, name, devfn, > config_read, config_write, > PCI_HEADER_TYPE_NORMAL); > + if (pci_dev == NULL) { > + hw_error("PCI: can't register device\n"); > + } Can you please use !pci_dev for these checks? > return pci_dev; > } > static target_phys_addr_t pci_to_cpu_addr(target_phys_addr_t addr) > @@ -1376,6 +1381,8 @@ static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base) > pci_dev = do_pci_register_device(pci_dev, bus, base->name, devfn, > info->config_read, info->config_write, > info->header_type); > + if (pci_dev == NULL) > + return -1; And here too. > rc = info->init(pci_dev); > if (rc != 0) > return rc; > -- > 1.6.5.2 > >
Hi, >> + if (pci_dev == NULL) { >> + hw_error("PCI: can't register device\n"); >> + } > > Can you please use !pci_dev for these checks? Why? IMHO the code is more readable that way. It is easy to miss a single '!' character when reading the code, so I tend to write such tests in a more verbose fashion. cheers, Gerd
On Thu, Dec 10, 2009 at 01:19:10PM +0100, Gerd Hoffmann wrote: > Hi, > >>> + if (pci_dev == NULL) { >>> + hw_error("PCI: can't register device\n"); >>> + } >> >> Can you please use !pci_dev for these checks? > > Why? IMHO the code is more readable that way. It is easy to miss a > single '!' character when reading the code, so I tend to write such > tests in a more verbose fashion. > > cheers, > Gerd Reader has limited short term memory. Don't fill it up with irrelevant detail. In places where it might be confusing, a comment might be appropriate, this is not one of them. C is a terse language. !x is a standard idiom. Let's use it.
On 10.12.2009, at 13:08, Michael S. Tsirkin wrote: > On Thu, Dec 10, 2009 at 11:11:06AM +0100, Gerd Hoffmann wrote: >> Current PCI code will simply hw_error() and thus abort in case no free >> PCI slot is available or the requested PCI slot is already in use by >> another device. For the hotplug case this behavior is not acceptable. >> This patch makes qemu pass up the error properly, so the calling code >> can decide whenever it wants to exit with an error (on startup) or >> whenever it wants to continue (hotplug). >> >> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > > Good stuff. However > >> --- >> hw/pci.c | 11 +++++++++-- >> 1 files changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/hw/pci.c b/hw/pci.c >> index 4f662b7..404eead 100644 >> --- a/hw/pci.c >> +++ b/hw/pci.c >> @@ -580,11 +580,13 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, >> if (!bus->devices[devfn]) >> goto found; >> } >> - hw_error("PCI: no devfn available for %s, all in use\n", name); >> + qemu_error("PCI: no devfn available for %s, all in use\n", name); >> + return NULL; >> found: ; >> } else if (bus->devices[devfn]) { >> - hw_error("PCI: devfn %d not available for %s, in use by %s\n", devfn, >> + qemu_error("PCI: devfn %d not available for %s, in use by %s\n", devfn, >> name, bus->devices[devfn]->name); >> + return NULL; >> } >> pci_dev->bus = bus; >> pci_dev->devfn = devfn; >> @@ -625,6 +627,9 @@ PCIDevice *pci_register_device(PCIBus *bus, const char *name, >> pci_dev = do_pci_register_device(pci_dev, bus, name, devfn, >> config_read, config_write, >> PCI_HEADER_TYPE_NORMAL); >> + if (pci_dev == NULL) { >> + hw_error("PCI: can't register device\n"); >> + } > > Can you please use !pci_dev for these checks? > >> return pci_dev; >> } >> static target_phys_addr_t pci_to_cpu_addr(target_phys_addr_t addr) >> @@ -1376,6 +1381,8 @@ static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base) >> pci_dev = do_pci_register_device(pci_dev, bus, base->name, devfn, >> info->config_read, info->config_write, >> info->header_type); >> + if (pci_dev == NULL) >> + return -1; > > And here too. OMG! Broken coding style! :) Alex
On Thu, Dec 10, 2009 at 02:23:05PM +0200, Michael S. Tsirkin wrote: > On Thu, Dec 10, 2009 at 01:19:10PM +0100, Gerd Hoffmann wrote: > > Hi, > > > >>> + if (pci_dev == NULL) { > >>> + hw_error("PCI: can't register device\n"); > >>> + } > >> > >> Can you please use !pci_dev for these checks? > > > > Why? IMHO the code is more readable that way. It is easy to miss a > > single '!' character when reading the code, so I tend to write such > > tests in a more verbose fashion. > > > > cheers, > > Gerd > > Reader has limited short term memory. Don't fill it up with > irrelevant detail. In places where it might be confusing, > a comment might be appropriate, this is not one of them. > > C is a terse language. !x is a standard idiom. Let's use it. > ! is for boolean. pci_dev is not boolean. -- Gleb.
On Thu, Dec 10, 2009 at 03:22:52PM +0200, Gleb Natapov wrote: > On Thu, Dec 10, 2009 at 02:23:05PM +0200, Michael S. Tsirkin wrote: > > On Thu, Dec 10, 2009 at 01:19:10PM +0100, Gerd Hoffmann wrote: > > > Hi, > > > > > >>> + if (pci_dev == NULL) { > > >>> + hw_error("PCI: can't register device\n"); > > >>> + } > > >> > > >> Can you please use !pci_dev for these checks? > > > > > > Why? IMHO the code is more readable that way. It is easy to miss a > > > single '!' character when reading the code, so I tend to write such > > > tests in a more verbose fashion. > > > > > > cheers, > > > Gerd > > > > Reader has limited short term memory. Don't fill it up with > > irrelevant detail. In places where it might be confusing, > > a comment might be appropriate, this is not one of them. > > > > C is a terse language. !x is a standard idiom. Let's use it. > > > ! is for boolean. In which language? Not in C. > pci_dev is not boolean. > -- > Gleb.
On Thu, Dec 10, 2009 at 08:04:56PM +0200, Michael S. Tsirkin wrote: > On Thu, Dec 10, 2009 at 03:22:52PM +0200, Gleb Natapov wrote: > > On Thu, Dec 10, 2009 at 02:23:05PM +0200, Michael S. Tsirkin wrote: > > > On Thu, Dec 10, 2009 at 01:19:10PM +0100, Gerd Hoffmann wrote: > > > > Hi, > > > > > > > >>> + if (pci_dev == NULL) { > > > >>> + hw_error("PCI: can't register device\n"); > > > >>> + } > > > >> > > > >> Can you please use !pci_dev for these checks? > > > > > > > > Why? IMHO the code is more readable that way. It is easy to miss a > > > > single '!' character when reading the code, so I tend to write such > > > > tests in a more verbose fashion. > > > > > > > > cheers, > > > > Gerd > > > > > > Reader has limited short term memory. Don't fill it up with > > > irrelevant detail. In places where it might be confusing, > > > a comment might be appropriate, this is not one of them. > > > > > > C is a terse language. !x is a standard idiom. Let's use it. > > > > > ! is for boolean. > > In which language? Not in C. > In boolean. My point is no need to force your style on everyone if it is not in coding style document. == often much more readable then !. > > pci_dev is not boolean. > > -- > > Gleb. -- Gleb.
On Thu, Dec 10, 2009 at 09:13:06PM +0200, Gleb Natapov wrote: > On Thu, Dec 10, 2009 at 08:04:56PM +0200, Michael S. Tsirkin wrote: > > On Thu, Dec 10, 2009 at 03:22:52PM +0200, Gleb Natapov wrote: > > > On Thu, Dec 10, 2009 at 02:23:05PM +0200, Michael S. Tsirkin wrote: > > > > On Thu, Dec 10, 2009 at 01:19:10PM +0100, Gerd Hoffmann wrote: > > > > > Hi, > > > > > > > > > >>> + if (pci_dev == NULL) { > > > > >>> + hw_error("PCI: can't register device\n"); > > > > >>> + } > > > > >> > > > > >> Can you please use !pci_dev for these checks? > > > > > > > > > > Why? IMHO the code is more readable that way. It is easy to miss a > > > > > single '!' character when reading the code, so I tend to write such > > > > > tests in a more verbose fashion. > > > > > > > > > > cheers, > > > > > Gerd > > > > > > > > Reader has limited short term memory. Don't fill it up with > > > > irrelevant detail. In places where it might be confusing, > > > > a comment might be appropriate, this is not one of them. > > > > > > > > C is a terse language. !x is a standard idiom. Let's use it. > > > > > > > ! is for boolean. > > > > In which language? Not in C. > > > In boolean. My point is no need to force your style on everyone I am not forcing style on anyone - how can I do this? I hacked on pci and by now there's no == NULL in pci.c anywhere ;). I will be happier if none is added now. > if it is not in coding style document. Heh, we don't need to document everything. > == often much more readable then !. Matter of taste probably. > > > pci_dev is not boolean. > > > -- > > > Gleb. > > -- > Gleb.
On Fri, Dec 11, 2009 at 12:37:31PM +0200, Michael S. Tsirkin wrote: > On Thu, Dec 10, 2009 at 09:13:06PM +0200, Gleb Natapov wrote: > > On Thu, Dec 10, 2009 at 08:04:56PM +0200, Michael S. Tsirkin wrote: > > > On Thu, Dec 10, 2009 at 03:22:52PM +0200, Gleb Natapov wrote: > > > > On Thu, Dec 10, 2009 at 02:23:05PM +0200, Michael S. Tsirkin wrote: > > > > > On Thu, Dec 10, 2009 at 01:19:10PM +0100, Gerd Hoffmann wrote: > > > > > > Hi, > > > > > > > > > > > >>> + if (pci_dev == NULL) { > > > > > >>> + hw_error("PCI: can't register device\n"); > > > > > >>> + } > > > > > >> > > > > > >> Can you please use !pci_dev for these checks? > > > > > > > > > > > > Why? IMHO the code is more readable that way. It is easy to miss a > > > > > > single '!' character when reading the code, so I tend to write such > > > > > > tests in a more verbose fashion. > > > > > > > > > > > > cheers, > > > > > > Gerd > > > > > > > > > > Reader has limited short term memory. Don't fill it up with > > > > > irrelevant detail. In places where it might be confusing, > > > > > a comment might be appropriate, this is not one of them. > > > > > > > > > > C is a terse language. !x is a standard idiom. Let's use it. > > > > > > > > > ! is for boolean. > > > > > > In which language? Not in C. > > > > > In boolean. My point is no need to force your style on everyone > > I am not forcing style on anyone - how can I do this? I hacked on pci > and by now there's no == NULL in pci.c anywhere ;). I will be > happier if none is added now. > > > if it is not in coding style document. > > Heh, we don't need to document everything. > > > == often much more readable then !. > > Matter of taste probably. > Exactly! (Or should I say "Exactly=="?) -- Gleb.
diff --git a/hw/pci.c b/hw/pci.c index 4f662b7..404eead 100644 --- a/hw/pci.c +++ b/hw/pci.c @@ -580,11 +580,13 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, if (!bus->devices[devfn]) goto found; } - hw_error("PCI: no devfn available for %s, all in use\n", name); + qemu_error("PCI: no devfn available for %s, all in use\n", name); + return NULL; found: ; } else if (bus->devices[devfn]) { - hw_error("PCI: devfn %d not available for %s, in use by %s\n", devfn, + qemu_error("PCI: devfn %d not available for %s, in use by %s\n", devfn, name, bus->devices[devfn]->name); + return NULL; } pci_dev->bus = bus; pci_dev->devfn = devfn; @@ -625,6 +627,9 @@ PCIDevice *pci_register_device(PCIBus *bus, const char *name, pci_dev = do_pci_register_device(pci_dev, bus, name, devfn, config_read, config_write, PCI_HEADER_TYPE_NORMAL); + if (pci_dev == NULL) { + hw_error("PCI: can't register device\n"); + } return pci_dev; } static target_phys_addr_t pci_to_cpu_addr(target_phys_addr_t addr) @@ -1376,6 +1381,8 @@ static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base) pci_dev = do_pci_register_device(pci_dev, bus, base->name, devfn, info->config_read, info->config_write, info->header_type); + if (pci_dev == NULL) + return -1; rc = info->init(pci_dev); if (rc != 0) return rc;
Current PCI code will simply hw_error() and thus abort in case no free PCI slot is available or the requested PCI slot is already in use by another device. For the hotplug case this behavior is not acceptable. This patch makes qemu pass up the error properly, so the calling code can decide whenever it wants to exit with an error (on startup) or whenever it wants to continue (hotplug). Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/pci.c | 11 +++++++++-- 1 files changed, 9 insertions(+), 2 deletions(-)