If devfn is assigned automatically, 'else' clauses will never be
executed. And if it does not matter for the reserved and available
devfn, because we have already checked it, the check for function0
needs to be done again.

Signed-off-by: Julia Suvorova <jusual@redhat.com>
---
 hw/pci/pci.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index de0fae10ab..ae132b0b52 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1034,8 +1034,9 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
                    PCI_SLOT(devfn), PCI_FUNC(devfn), name,
                    bus->devices[devfn]->name);
         return NULL;
-    } else if (dev->hotplugged &&
-               pci_get_function_0(pci_dev)) {
+    };
+
+    if (dev->hotplugged && pci_get_function_0(pci_dev)) {
         error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s,"
                    " new func %s cannot be exposed to guest.",
                    PCI_SLOT(pci_get_function_0(pci_dev)->devfn),
-- 
2.25.4

On Wed, Sep 23, 2020 at 11:26:36AM +0200, Julia Suvorova wrote:
> If devfn is assigned automatically, 'else' clauses will never be
> executed. And if it does not matter for the reserved and available
> devfn, because we have already checked it, the check for function0
> needs to be done again.
> 
> Signed-off-by: Julia Suvorova <jusual@redhat.com>

This is just cosmetics right? I wouldn't describe this as
a "fix" then - "simplify" would be clearer.

> ---
>  hw/pci/pci.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index de0fae10ab..ae132b0b52 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1034,8 +1034,9 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
>                     PCI_SLOT(devfn), PCI_FUNC(devfn), name,
>                     bus->devices[devfn]->name);
>          return NULL;
> -    } else if (dev->hotplugged &&
> -               pci_get_function_0(pci_dev)) {
> +    };
> +
> +    if (dev->hotplugged && pci_get_function_0(pci_dev)) {
>          error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s,"
>                     " new func %s cannot be exposed to guest.",
>                     PCI_SLOT(pci_get_function_0(pci_dev)->devfn),
> -- 
> 2.25.4

On Wed, Sep 23, 2020 at 5:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Sep 23, 2020 at 11:26:36AM +0200, Julia Suvorova wrote:
> > If devfn is assigned automatically, 'else' clauses will never be
> > executed. And if it does not matter for the reserved and available
> > devfn, because we have already checked it, the check for function0
> > needs to be done again.
> >
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
>
> This is just cosmetics right? I wouldn't describe this as
> a "fix" then - "simplify" would be clearer.

No, this is a bug fix. For example, if you had a root port with a
device on it already, and you do
'device_add new_device,bus=the_same_root_port', then it will miss the
last 'if' and will be added to slot 1.

> > ---
> >  hw/pci/pci.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index de0fae10ab..ae132b0b52 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -1034,8 +1034,9 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
> >                     PCI_SLOT(devfn), PCI_FUNC(devfn), name,
> >                     bus->devices[devfn]->name);
> >          return NULL;
> > -    } else if (dev->hotplugged &&
> > -               pci_get_function_0(pci_dev)) {
> > +    };
> > +
> > +    if (dev->hotplugged && pci_get_function_0(pci_dev)) {
> >          error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s,"
> >                     " new func %s cannot be exposed to guest.",
> >                     PCI_SLOT(pci_get_function_0(pci_dev)->devfn),
> > --
> > 2.25.4
>

On Wed, Sep 23, 2020 at 05:57:19PM +0200, Julia Suvorova wrote:
> On Wed, Sep 23, 2020 at 5:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Wed, Sep 23, 2020 at 11:26:36AM +0200, Julia Suvorova wrote:
> > > If devfn is assigned automatically, 'else' clauses will never be
> > > executed. And if it does not matter for the reserved and available
> > > devfn, because we have already checked it, the check for function0
> > > needs to be done again.
> > >
> > > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> >
> > This is just cosmetics right? I wouldn't describe this as
> > a "fix" then - "simplify" would be clearer.
> 
> No, this is a bug fix. For example, if you had a root port with a
> device on it already, and you do
> 'device_add new_device,bus=the_same_root_port', then it will miss the
> last 'if' and will be added to slot 1.

OK and I think that is the only example - if devfn is not supplied
and is auto-assigned. Can you add this to commit log pls?


> > > ---
> > >  hw/pci/pci.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index de0fae10ab..ae132b0b52 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -1034,8 +1034,9 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
> > >                     PCI_SLOT(devfn), PCI_FUNC(devfn), name,
> > >                     bus->devices[devfn]->name);
> > >          return NULL;
> > > -    } else if (dev->hotplugged &&
> > > -               pci_get_function_0(pci_dev)) {
> > > +    };
> > > +
> > > +    if (dev->hotplugged && pci_get_function_0(pci_dev)) {
> > >          error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s,"
> > >                     " new func %s cannot be exposed to guest.",
> > >                     PCI_SLOT(pci_get_function_0(pci_dev)->devfn),
> > > --
> > > 2.25.4
> >