| Message ID | 1414481739-19939-5-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 2014/10/28 15:35, Markus Armbruster wrote: > The next commit will exploit the fact it never fails. This one makes > it obvious. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > hw/net/lance.c | 3 ++- > hw/net/pcnet-pci.c | 3 ++- > hw/net/pcnet.c | 4 +--- > hw/net/pcnet.h | 2 +- > 4 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/hw/net/lance.c b/hw/net/lance.c > index a1c49f1..3663340 100644 > --- a/hw/net/lance.c > +++ b/hw/net/lance.c > @@ -134,7 +134,8 @@ static int lance_init(SysBusDevice *sbd) > > s->phys_mem_read = ledma_memory_read; > s->phys_mem_write = ledma_memory_write; > - return pcnet_common_init(dev, s, &net_lance_info); > + pcnet_common_init(dev, s, &net_lance_info); > + return 0; > } > > static void lance_reset(DeviceState *dev) > diff --git a/hw/net/pcnet-pci.c b/hw/net/pcnet-pci.c > index fb5f5d6..50eb069 100644 > --- a/hw/net/pcnet-pci.c > +++ b/hw/net/pcnet-pci.c > @@ -335,7 +335,8 @@ static int pci_pcnet_init(PCIDevice *pci_dev) > s->phys_mem_write = pci_physical_memory_write; > s->dma_opaque = pci_dev; > > - return pcnet_common_init(DEVICE(pci_dev), s, &net_pci_pcnet_info); > + pcnet_common_init(DEVICE(pci_dev), s, &net_pci_pcnet_info); > + return 0; > } > > static void pci_reset(DeviceState *dev) > diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c > index d344c15..5a081c4 100644 > --- a/hw/net/pcnet.c > +++ b/hw/net/pcnet.c > @@ -1724,7 +1724,7 @@ void pcnet_common_cleanup(PCNetState *d) > d->nic = NULL; > } > > -int pcnet_common_init(DeviceState *dev, PCNetState *s, NetClientInfo *info) > +void pcnet_common_init(DeviceState *dev, PCNetState *s, NetClientInfo *info) Do we need consider to pass an Error **errp argument to it? Best regards, -Gonglei > { > int i; > uint16_t checksum; > @@ -1763,6 +1763,4 @@ int pcnet_common_init(DeviceState *dev, PCNetState *s, NetClientInfo *info) > *(uint16_t *)&s->prom[12] = cpu_to_le16(checksum); > > s->lnkst = 0x40; /* initial link state: up */ > - > - return 0; > } > diff --git a/hw/net/pcnet.h b/hw/net/pcnet.h > index f8e8a6f..cfc196e 100644 > --- a/hw/net/pcnet.h > +++ b/hw/net/pcnet.h > @@ -64,6 +64,6 @@ int pcnet_can_receive(NetClientState *nc); > ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_); > void pcnet_set_link_status(NetClientState *nc); > void pcnet_common_cleanup(PCNetState *d); > -int pcnet_common_init(DeviceState *dev, PCNetState *s, NetClientInfo *info); > +void pcnet_common_init(DeviceState *dev, PCNetState *s, NetClientInfo *info); > extern const VMStateDescription vmstate_pcnet; > #endif
Gonglei <arei.gonglei@huawei.com> writes: > On 2014/10/28 15:35, Markus Armbruster wrote: > >> The next commit will exploit the fact it never fails. This one makes >> it obvious. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> hw/net/lance.c | 3 ++- >> hw/net/pcnet-pci.c | 3 ++- >> hw/net/pcnet.c | 4 +--- >> hw/net/pcnet.h | 2 +- >> 4 files changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/hw/net/lance.c b/hw/net/lance.c >> index a1c49f1..3663340 100644 >> --- a/hw/net/lance.c >> +++ b/hw/net/lance.c >> @@ -134,7 +134,8 @@ static int lance_init(SysBusDevice *sbd) >> >> s->phys_mem_read = ledma_memory_read; >> s->phys_mem_write = ledma_memory_write; >> - return pcnet_common_init(dev, s, &net_lance_info); >> + pcnet_common_init(dev, s, &net_lance_info); >> + return 0; >> } >> >> static void lance_reset(DeviceState *dev) >> diff --git a/hw/net/pcnet-pci.c b/hw/net/pcnet-pci.c >> index fb5f5d6..50eb069 100644 >> --- a/hw/net/pcnet-pci.c >> +++ b/hw/net/pcnet-pci.c >> @@ -335,7 +335,8 @@ static int pci_pcnet_init(PCIDevice *pci_dev) >> s->phys_mem_write = pci_physical_memory_write; >> s->dma_opaque = pci_dev; >> >> - return pcnet_common_init(DEVICE(pci_dev), s, &net_pci_pcnet_info); >> + pcnet_common_init(DEVICE(pci_dev), s, &net_pci_pcnet_info); >> + return 0; >> } >> >> static void pci_reset(DeviceState *dev) >> diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c >> index d344c15..5a081c4 100644 >> --- a/hw/net/pcnet.c >> +++ b/hw/net/pcnet.c >> @@ -1724,7 +1724,7 @@ void pcnet_common_cleanup(PCNetState *d) >> d->nic = NULL; >> } >> >> -int pcnet_common_init(DeviceState *dev, PCNetState *s, NetClientInfo *info) >> +void pcnet_common_init(DeviceState *dev, PCNetState *s, NetClientInfo *info) > > Do we need consider to pass an Error **errp argument to it? This function can't fail. The point of thimy patch is to make "can't fail" obvious. If we add an errp parameter, the caller needs to check it, for robustness. I prefer to keep things simple, and add the error checking only when it's actually needed. > Best regards, > -Gonglei Thanks! [...]
On 2014/10/28 17:41, Markus Armbruster wrote: > Gonglei <arei.gonglei@huawei.com> writes: > >> On 2014/10/28 15:35, Markus Armbruster wrote: >> >>> The next commit will exploit the fact it never fails. This one makes >>> it obvious. >>> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> --- >>> hw/net/lance.c | 3 ++- >>> hw/net/pcnet-pci.c | 3 ++- >>> hw/net/pcnet.c | 4 +--- >>> hw/net/pcnet.h | 2 +- >>> 4 files changed, 6 insertions(+), 6 deletions(-) >>> >>> diff --git a/hw/net/lance.c b/hw/net/lance.c >>> index a1c49f1..3663340 100644 >>> --- a/hw/net/lance.c >>> +++ b/hw/net/lance.c >>> @@ -134,7 +134,8 @@ static int lance_init(SysBusDevice *sbd) >>> >>> s->phys_mem_read = ledma_memory_read; >>> s->phys_mem_write = ledma_memory_write; >>> - return pcnet_common_init(dev, s, &net_lance_info); >>> + pcnet_common_init(dev, s, &net_lance_info); >>> + return 0; >>> } >>> >>> static void lance_reset(DeviceState *dev) >>> diff --git a/hw/net/pcnet-pci.c b/hw/net/pcnet-pci.c >>> index fb5f5d6..50eb069 100644 >>> --- a/hw/net/pcnet-pci.c >>> +++ b/hw/net/pcnet-pci.c >>> @@ -335,7 +335,8 @@ static int pci_pcnet_init(PCIDevice *pci_dev) >>> s->phys_mem_write = pci_physical_memory_write; >>> s->dma_opaque = pci_dev; >>> >>> - return pcnet_common_init(DEVICE(pci_dev), s, &net_pci_pcnet_info); >>> + pcnet_common_init(DEVICE(pci_dev), s, &net_pci_pcnet_info); >>> + return 0; >>> } >>> >>> static void pci_reset(DeviceState *dev) >>> diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c >>> index d344c15..5a081c4 100644 >>> --- a/hw/net/pcnet.c >>> +++ b/hw/net/pcnet.c >>> @@ -1724,7 +1724,7 @@ void pcnet_common_cleanup(PCNetState *d) >>> d->nic = NULL; >>> } >>> >>> -int pcnet_common_init(DeviceState *dev, PCNetState *s, NetClientInfo *info) >>> +void pcnet_common_init(DeviceState *dev, PCNetState *s, NetClientInfo *info) >> >> Do we need consider to pass an Error **errp argument to it? > > This function can't fail. The point of thimy patch is to make "can't > fail" obvious. If we add an errp parameter, the caller needs to check > it, for robustness. Yes, it is. > I prefer to keep things simple, and add the error > checking only when it's actually needed. > OK, it's fine :) Reviewed-by: Gonglei <arei.gonglei@huawei.com> Best regards, -Gonglei
diff --git a/hw/net/lance.c b/hw/net/lance.c index a1c49f1..3663340 100644 --- a/hw/net/lance.c +++ b/hw/net/lance.c @@ -134,7 +134,8 @@ static int lance_init(SysBusDevice *sbd) s->phys_mem_read = ledma_memory_read; s->phys_mem_write = ledma_memory_write; - return pcnet_common_init(dev, s, &net_lance_info); + pcnet_common_init(dev, s, &net_lance_info); + return 0; } static void lance_reset(DeviceState *dev) diff --git a/hw/net/pcnet-pci.c b/hw/net/pcnet-pci.c index fb5f5d6..50eb069 100644 --- a/hw/net/pcnet-pci.c +++ b/hw/net/pcnet-pci.c @@ -335,7 +335,8 @@ static int pci_pcnet_init(PCIDevice *pci_dev) s->phys_mem_write = pci_physical_memory_write; s->dma_opaque = pci_dev; - return pcnet_common_init(DEVICE(pci_dev), s, &net_pci_pcnet_info); + pcnet_common_init(DEVICE(pci_dev), s, &net_pci_pcnet_info); + return 0; } static void pci_reset(DeviceState *dev) diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c index d344c15..5a081c4 100644 --- a/hw/net/pcnet.c +++ b/hw/net/pcnet.c @@ -1724,7 +1724,7 @@ void pcnet_common_cleanup(PCNetState *d) d->nic = NULL; } -int pcnet_common_init(DeviceState *dev, PCNetState *s, NetClientInfo *info) +void pcnet_common_init(DeviceState *dev, PCNetState *s, NetClientInfo *info) { int i; uint16_t checksum; @@ -1763,6 +1763,4 @@ int pcnet_common_init(DeviceState *dev, PCNetState *s, NetClientInfo *info) *(uint16_t *)&s->prom[12] = cpu_to_le16(checksum); s->lnkst = 0x40; /* initial link state: up */ - - return 0; } diff --git a/hw/net/pcnet.h b/hw/net/pcnet.h index f8e8a6f..cfc196e 100644 --- a/hw/net/pcnet.h +++ b/hw/net/pcnet.h @@ -64,6 +64,6 @@ int pcnet_can_receive(NetClientState *nc); ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_); void pcnet_set_link_status(NetClientState *nc); void pcnet_common_cleanup(PCNetState *d); -int pcnet_common_init(DeviceState *dev, PCNetState *s, NetClientInfo *info); +void pcnet_common_init(DeviceState *dev, PCNetState *s, NetClientInfo *info); extern const VMStateDescription vmstate_pcnet; #endif
The next commit will exploit the fact it never fails. This one makes it obvious. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- hw/net/lance.c | 3 ++- hw/net/pcnet-pci.c | 3 ++- hw/net/pcnet.c | 4 +--- hw/net/pcnet.h | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-)