off by 1 in pci_piix3_xen_ide_unplug

off by 1 in pci_piix3_xen_ide_unplug

[James Harper]

It seems that qemu isn't unplugging all my disks, leaving my /dev/xvdd plugged in, with obvious consequences.

pci_piix3_xen_ide_unplug appears to only be counting to disk < 3, when it should be <= 3 or < 4.

Where do qemu patches go?

James

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 574b9c1..b6b30a4 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -175,7 +175,7 @@ int pci_piix3_xen_ide_unplug(DeviceState *dev)

     pci_ide = PCI_IDE(dev);

-    for (; i < 3; i++) {
+    for (; i < 4; i++) {
         di = drive_get_by_index(IF_IDE, i);
         if (di != NULL && !di->media_cd) {
             BlockBackend *blk = blk_by_legacy_dinfo(di);

Re: off by 1 in pci_piix3_xen_ide_unplug

[Andrew Cooper]

On 29/10/14 08:21, James Harper wrote:
> It seems that qemu isn't unplugging all my disks, leaving my /dev/xvdd plugged in, with obvious consequences.
>
> pci_piix3_xen_ide_unplug appears to only be counting to disk < 3, when it should be <= 3 or < 4.
>
> Where do qemu patches go?
>
> James

I presume this is qemu-upstream, as qemu-trad doesn't have an hw/ide
directory.

CCing Stefano as the maintainer.

For what its worth, I agree that it really should be 4.

~Andrew

>
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index 574b9c1..b6b30a4 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -175,7 +175,7 @@ int pci_piix3_xen_ide_unplug(DeviceState *dev)
>
>      pci_ide = PCI_IDE(dev);
>
> -    for (; i < 3; i++) {
> +    for (; i < 4; i++) {
>          di = drive_get_by_index(IF_IDE, i);
>          if (di != NULL && !di->media_cd) {
>              BlockBackend *blk = blk_by_legacy_dinfo(di);
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: off by 1 in pci_piix3_xen_ide_unplug

[Stefano Stabellini]

On Wed, 29 Oct 2014, Andrew Cooper wrote:
> On 29/10/14 08:21, James Harper wrote:
> > It seems that qemu isn't unplugging all my disks, leaving my /dev/xvdd plugged in, with obvious consequences.
> >
> > pci_piix3_xen_ide_unplug appears to only be counting to disk < 3, when it should be <= 3 or < 4.
> >
> > Where do qemu patches go?
> >

You need to send them to qemu-devel, CC'ing xen-devel and me.


> I presume this is qemu-upstream, as qemu-trad doesn't have an hw/ide
> directory.
> 
> CCing Stefano as the maintainer.
> 
> For what its worth, I agree that it really should be 4.

I think it should be 4 too. Well spotted!

Could you please resend to qemu-devel with your signed-off-by line?

Thanks!


 
> > diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> > index 574b9c1..b6b30a4 100644
> > --- a/hw/ide/piix.c
> > +++ b/hw/ide/piix.c
> > @@ -175,7 +175,7 @@ int pci_piix3_xen_ide_unplug(DeviceState *dev)
> >
> >      pci_ide = PCI_IDE(dev);
> >
> > -    for (; i < 3; i++) {
> > +    for (; i < 4; i++) {
> >          di = drive_get_by_index(IF_IDE, i);
> >          if (di != NULL && !di->media_cd) {
> >              BlockBackend *blk = blk_by_legacy_dinfo(di);
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 

Re: off by 1 in pci_piix3_xen_ide_unplug

[James Harper]

> > >
> > > Where do qemu patches go?
> > >
> 
> You need to send them to qemu-devel, CC'ing xen-devel and me.
> 

Will do.

The style of the for loop bothers me too...

int i = 0;
...
for (; i < 4; i++) {
...

Given that i is only used here, and the start value is a constant, this is more readable:

int i;
...
for (i = 0; i < 4; i++) {
...

Then it is immediately obvious what the bounds of the loop are without having to look elsewhere for i.

Am I being too pedantic here? If I wanted to propose a change, would that be a separate patch?

James

Re: off by 1 in pci_piix3_xen_ide_unplug

[Stefano Stabellini]

On Wed, 29 Oct 2014, James Harper wrote:
> > > >
> > > > Where do qemu patches go?
> > > >
> > 
> > You need to send them to qemu-devel, CC'ing xen-devel and me.
> > 
> 
> Will do.
> 
> The style of the for loop bothers me too...
> 
> int i = 0;
> ...
> for (; i < 4; i++) {
> ...
> 
> Given that i is only used here, and the start value is a constant, this is more readable:
> 
> int i;
> ...
> for (i = 0; i < 4; i++) {
> ...
> 
> Then it is immediately obvious what the bounds of the loop are without having to look elsewhere for i.
> 
> Am I being too pedantic here? If I wanted to propose a change, would that be a separate patch?

That's fine, you can fold that change in the same patch.