| Message ID | 1423501897-30410-4-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 02/09/2015 10:11 AM, Max Reitz wrote: > The tray of an FDD is open iff there is no medium inserted (there are > only two states for an FDD: "medium inserted" or "no medium inserted"). > > This results in the tray being reported as open if qemu has been started > with the default floppy drive, which breaks some tests. Fix them. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > hw/block/fdc.c | 20 +++++++++++++--- > tests/fdc-test.c | 4 +--- > tests/qemu-iotests/067.out | 60 +++++++--------------------------------------- > tests/qemu-iotests/071.out | 2 -- > tests/qemu-iotests/081.out | 1 - > tests/qemu-iotests/087.out | 6 ----- > 6 files changed, 26 insertions(+), 67 deletions(-) > Reviewed-by: Eric Blake <eblake@redhat.com>
Am 09.02.2015 um 18:11 hat Max Reitz geschrieben: > The tray of an FDD is open iff there is no medium inserted (there are > only two states for an FDD: "medium inserted" or "no medium inserted"). > > This results in the tray being reported as open if qemu has been started > with the default floppy drive, which breaks some tests. Fix them. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > hw/block/fdc.c | 20 +++++++++++++--- > tests/fdc-test.c | 4 +--- > tests/qemu-iotests/067.out | 60 +++++++--------------------------------------- > tests/qemu-iotests/071.out | 2 -- > tests/qemu-iotests/081.out | 1 - > tests/qemu-iotests/087.out | 6 ----- > 6 files changed, 26 insertions(+), 67 deletions(-) > > diff --git a/hw/block/fdc.c b/hw/block/fdc.c > index 2bf87c9..0c5a6b4 100644 > --- a/hw/block/fdc.c > +++ b/hw/block/fdc.c > @@ -192,6 +192,8 @@ typedef struct FDrive { > uint8_t ro; /* Is read-only */ > uint8_t media_changed; /* Is media changed */ > uint8_t media_rate; /* Data rate of medium */ > + > + bool media_inserted; /* Is there a medium in the tray */ > } FDrive; > > static void fd_init(FDrive *drv) > @@ -261,7 +263,7 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t track, uint8_t sect, > #endif > drv->head = head; > if (drv->track != track) { > - if (drv->blk != NULL && blk_is_inserted(drv->blk)) { > + if (drv->media_inserted) { I suspect that with the removal of blk_is_inserted() in several places, floppy passthrough (host_floppy block driver) is now even more broken than before, potentially not noticing removal of a medium any more. While checking this, I noticed that since commit 21fcf360, bdrv_media_changed() is completely unused. Media change has therefore probably been broken since at least May 2012. Considering this, it might actually be reasonable enough to remove the block driver. It's definitely better than having it there, but not working any better than host_device. Of course, alternatively you would also be welcome to fix the device model and reintroduce bdrv_media_changed() and blk_is_inserted() calls where necessary. Kevin
On 2015-03-04 at 09:00, Kevin Wolf wrote: > Am 09.02.2015 um 18:11 hat Max Reitz geschrieben: >> The tray of an FDD is open iff there is no medium inserted (there are >> only two states for an FDD: "medium inserted" or "no medium inserted"). >> >> This results in the tray being reported as open if qemu has been started >> with the default floppy drive, which breaks some tests. Fix them. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> hw/block/fdc.c | 20 +++++++++++++--- >> tests/fdc-test.c | 4 +--- >> tests/qemu-iotests/067.out | 60 +++++++--------------------------------------- >> tests/qemu-iotests/071.out | 2 -- >> tests/qemu-iotests/081.out | 1 - >> tests/qemu-iotests/087.out | 6 ----- >> 6 files changed, 26 insertions(+), 67 deletions(-) >> >> diff --git a/hw/block/fdc.c b/hw/block/fdc.c >> index 2bf87c9..0c5a6b4 100644 >> --- a/hw/block/fdc.c >> +++ b/hw/block/fdc.c >> @@ -192,6 +192,8 @@ typedef struct FDrive { >> uint8_t ro; /* Is read-only */ >> uint8_t media_changed; /* Is media changed */ >> uint8_t media_rate; /* Data rate of medium */ >> + >> + bool media_inserted; /* Is there a medium in the tray */ >> } FDrive; >> >> static void fd_init(FDrive *drv) >> @@ -261,7 +263,7 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t track, uint8_t sect, >> #endif >> drv->head = head; >> if (drv->track != track) { >> - if (drv->blk != NULL && blk_is_inserted(drv->blk)) { >> + if (drv->media_inserted) { > I suspect that with the removal of blk_is_inserted() in several places, > floppy passthrough (host_floppy block driver) is now even more broken > than before, potentially not noticing removal of a medium any more. > > While checking this, I noticed that since commit 21fcf360, > bdrv_media_changed() is completely unused. Media change has therefore > probably been broken since at least May 2012. > > Considering this, it might actually be reasonable enough to remove the > block driver. It's definitely better than having it there, but not > working any better than host_device. > > Of course, alternatively you would also be welcome to fix the device > model and reintroduce bdrv_media_changed() and blk_is_inserted() calls > where necessary. Urrggghhh... I guess it's my own fault for touching floppy code. I guess I'll rethink this patch while keeping in mind that bdrv_is_inserted() (as opposed to blk_is_inserted()) actually is necessary here because we won't notice if the host medium has been exchanged otherwise. Max
On 2015-03-04 at 09:00, Kevin Wolf wrote: > Am 09.02.2015 um 18:11 hat Max Reitz geschrieben: >> The tray of an FDD is open iff there is no medium inserted (there are >> only two states for an FDD: "medium inserted" or "no medium inserted"). >> >> This results in the tray being reported as open if qemu has been started >> with the default floppy drive, which breaks some tests. Fix them. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> hw/block/fdc.c | 20 +++++++++++++--- >> tests/fdc-test.c | 4 +--- >> tests/qemu-iotests/067.out | 60 +++++++--------------------------------------- >> tests/qemu-iotests/071.out | 2 -- >> tests/qemu-iotests/081.out | 1 - >> tests/qemu-iotests/087.out | 6 ----- >> 6 files changed, 26 insertions(+), 67 deletions(-) >> >> diff --git a/hw/block/fdc.c b/hw/block/fdc.c >> index 2bf87c9..0c5a6b4 100644 >> --- a/hw/block/fdc.c >> +++ b/hw/block/fdc.c >> @@ -192,6 +192,8 @@ typedef struct FDrive { >> uint8_t ro; /* Is read-only */ >> uint8_t media_changed; /* Is media changed */ >> uint8_t media_rate; /* Data rate of medium */ >> + >> + bool media_inserted; /* Is there a medium in the tray */ >> } FDrive; >> >> static void fd_init(FDrive *drv) >> @@ -261,7 +263,7 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t track, uint8_t sect, >> #endif >> drv->head = head; >> if (drv->track != track) { >> - if (drv->blk != NULL && blk_is_inserted(drv->blk)) { >> + if (drv->media_inserted) { > I suspect that with the removal of blk_is_inserted() in several places, > floppy passthrough (host_floppy block driver) is now even more broken > than before, potentially not noticing removal of a medium any more. > > While checking this, I noticed that since commit 21fcf360, > bdrv_media_changed() is completely unused. Media change has therefore > probably been broken since at least May 2012. > > Considering this, it might actually be reasonable enough to remove the > block driver. It's definitely better than having it there, but not > working any better than host_device. > > Of course, alternatively you would also be welcome to fix the device > model and reintroduce bdrv_media_changed() and blk_is_inserted() calls > where necessary. Status: I thought about why I need this tray status at all. The result is: We need it because otherwise blockdev-close-tray doesn't do anything on floppy disk drives (you'd insert a medium with blockdev-insert-medium and the tray would immediately be reported as being closed, without any TRAY_MOVED event). So we do need it. I tested floppy passthrough in a VM and it's horror, as can be expected. For instance, one cannot start qemu with floppy passthrough if the host drive is empty (which is actually helpful). So what I'll be doing is set media_inserted to load && drv->blk in the media change CB and set it to true in the floppy drive initialization code if there is a BB and blk_is_inserted() is true. The tray will be considered closed if media_inserted && blk_is_inserted(). So why do I check blk_is_inserted() in the initialization code? If we just set media_inserted to true, that would be wrong. Imagine an empty drive (no BDS tree), media_inserted will be set to true, then you inserted a BDS tree (blockdev-insert-medium) and the tray will be considered closed without you having executed blockdev-close-tray (media_inserted is true, and now blk_is_inserted() is true as well). So I have to check whether there's a BDS tree in the initialization code, if there isn't, media_inserted must be false. Why am I not using blk_bs() for that? I tried to explain for the USB patch: blk_bs() returns a BDS, and that's something for the block layer, nobody else should care about that. How could blk_is_inserted() be wrong here, if blk_bs() is the thing that will work? Correct, if blk_bs() && !bdrv_is_inserted(blk_bs()). However, this will never be the case, because as said above, qemu actually cannot start with a host floppy drive without a medium, so !!blk_is_inserted(blk) == !!blk_bs(blk). I see the question popping up "Why don't you just add a bool has_bs(BB) { return blk_bs(BB); } and then not add that test to blk_is_inserted()"? I've asked that myself. Answer: Again, anything outside of the block layer should not care about things like BDS trees. But moreover, bdrv_is_inserted() does not only check whether all the host devices represented by BDSs are inserted, but also whether BDS.drv != NULL, which until this series was the sign for an empty drive. Therefore, checking blk_is_inserted() is the logical conclusion of bdrv_is_inserted() (bdrv_is_inserted() tests whether BDS.drv != NULL, blk_is_inserted() tests whether BLK.bs != NULL). But medium management is now done on the BB level, so a separate function for checking whether there's a BDS tree (because that should be equivalent to "whether there's a medium") seems to have its merits. However, I don't think so. blk_is_inserted() is exactly that function: It's true if there is a BDS tree and, if there is host passthrough, whether all the media are inserted, which is correct. In theory, guest models should not have to distinguish whether a BB does not have a medium because there is no BDS tree or because we're using passthrough and that BDS does not have a medium. So why does floppy have to distinguish? Because it does not have a real tray, but that's the model we have to be working with. Inserting a medium into a passed-through host drive must result in the tray being considered closed immediately; inserting a medium into a guest device through blockdev-insert-medium must not. So our bane is that we need tray status for floppy disks but they don't have a tray, so inserting a medium in a host drive does something different than doing the same in a purely virtual drive. Dropping host_floppy probably solves that problem, but I'm too cautious for that. Max
Am 04.03.2015 um 23:06 hat Max Reitz geschrieben: > On 2015-03-04 at 09:00, Kevin Wolf wrote: > >Am 09.02.2015 um 18:11 hat Max Reitz geschrieben: > >>The tray of an FDD is open iff there is no medium inserted (there are > >>only two states for an FDD: "medium inserted" or "no medium inserted"). > >> > >>This results in the tray being reported as open if qemu has been started > >>with the default floppy drive, which breaks some tests. Fix them. > >> > >>Signed-off-by: Max Reitz <mreitz@redhat.com> > >>--- > >> hw/block/fdc.c | 20 +++++++++++++--- > >> tests/fdc-test.c | 4 +--- > >> tests/qemu-iotests/067.out | 60 +++++++--------------------------------------- > >> tests/qemu-iotests/071.out | 2 -- > >> tests/qemu-iotests/081.out | 1 - > >> tests/qemu-iotests/087.out | 6 ----- > >> 6 files changed, 26 insertions(+), 67 deletions(-) > >> > >>diff --git a/hw/block/fdc.c b/hw/block/fdc.c > >>index 2bf87c9..0c5a6b4 100644 > >>--- a/hw/block/fdc.c > >>+++ b/hw/block/fdc.c > >>@@ -192,6 +192,8 @@ typedef struct FDrive { > >> uint8_t ro; /* Is read-only */ > >> uint8_t media_changed; /* Is media changed */ > >> uint8_t media_rate; /* Data rate of medium */ > >>+ > >>+ bool media_inserted; /* Is there a medium in the tray */ > >> } FDrive; > >> static void fd_init(FDrive *drv) > >>@@ -261,7 +263,7 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t track, uint8_t sect, > >> #endif > >> drv->head = head; > >> if (drv->track != track) { > >>- if (drv->blk != NULL && blk_is_inserted(drv->blk)) { > >>+ if (drv->media_inserted) { > >I suspect that with the removal of blk_is_inserted() in several places, > >floppy passthrough (host_floppy block driver) is now even more broken > >than before, potentially not noticing removal of a medium any more. > > > >While checking this, I noticed that since commit 21fcf360, > >bdrv_media_changed() is completely unused. Media change has therefore > >probably been broken since at least May 2012. > > > >Considering this, it might actually be reasonable enough to remove the > >block driver. It's definitely better than having it there, but not > >working any better than host_device. > > > >Of course, alternatively you would also be welcome to fix the device > >model and reintroduce bdrv_media_changed() and blk_is_inserted() calls > >where necessary. > > Status: > > I thought about why I need this tray status at all. The result is: > We need it because otherwise blockdev-close-tray doesn't do anything > on floppy disk drives (you'd insert a medium with > blockdev-insert-medium and the tray would immediately be reported as > being closed, without any TRAY_MOVED event). So we do need it. > > I tested floppy passthrough in a VM and it's horror, as can be > expected. For instance, one cannot start qemu with floppy > passthrough if the host drive is empty (which is actually helpful). > > So what I'll be doing is set media_inserted to load && drv->blk in > the media change CB and set it to true in the floppy drive > initialization code if there is a BB and blk_is_inserted() is true. > The tray will be considered closed if media_inserted && > blk_is_inserted(). > > So why do I check blk_is_inserted() in the initialization code? If > we just set media_inserted to true, that would be wrong. Imagine an > empty drive (no BDS tree), media_inserted will be set to true, then > you inserted a BDS tree (blockdev-insert-medium) and the tray will > be considered closed without you having executed blockdev-close-tray > (media_inserted is true, and now blk_is_inserted() is true as well). > > So I have to check whether there's a BDS tree in the initialization > code, if there isn't, media_inserted must be false. Why am I not > using blk_bs() for that? I tried to explain for the USB patch: > blk_bs() returns a BDS, and that's something for the block layer, > nobody else should care about that. This is silly, Max. Correctness isn't defined by what you _should_ care about, but by what you actually _need_ to know. This is where you need to start your considerations, not with what you think you're supposed to be asking. If the two contradict, then there are two options: Either the assumption that you shouldn't care is wrong, or your decision that you need the information is. This conflict needs to be resolved rather than just working with the answer to a different question. In this case, I think the problem starts with "empty drive (no BDS tree)". This is not correct, an empty drive is defined by !blk_is_inserted(bs) and not by an empty BDS tree. The passthrough case (with a hypothetical correct implementation) helps you distinguish the two. So your assumption that you really need to check whether there is no BDS tree is wrong here. You need to check whether there is a medium, and that is blk_is_inserted(). (So yes, I agree with your conclusion, but not with the way you got there.) In the USB case, the problem was that a bdrv_*() function is called (which already violates the rule that this is not for outside the block layer) and a NULL check is needed there. In that case the assumption that calling blk_bs() isn't allowed is wrong; it's simply necessary for calling bdrv_*() functions. In any case "I want to know A, but I'm not supposed to ask for A, so I'll just check B" is not the way to go. > How could blk_is_inserted() be wrong here, if blk_bs() is the thing > that will work? Correct, if blk_bs() && !bdrv_is_inserted(blk_bs()). > However, this will never be the case, because as said above, qemu > actually cannot start with a host floppy drive without a medium, so > !!blk_is_inserted(blk) == !!blk_bs(blk). You called it "horror" above and now you want to rely on it? The good thing is that I suspect that blk_bs() does _not_ work and blk_is_inserted() is what you really needed in the first place. > I see the question popping up "Why don't you just add a bool > has_bs(BB) { return blk_bs(BB); } and then not add that test to > blk_is_inserted()"? I've asked that myself. Answer: Again, anything > outside of the block layer should not care about things like BDS > trees. But moreover, bdrv_is_inserted() does not only check whether > all the host devices represented by BDSs are inserted, but also > whether BDS.drv != NULL, which until this series was the sign for an > empty drive. Therefore, checking blk_is_inserted() is the logical > conclusion of bdrv_is_inserted() (bdrv_is_inserted() tests whether > BDS.drv != NULL, blk_is_inserted() tests whether BLK.bs != NULL). > > But medium management is now done on the BB level, so a separate > function for checking whether there's a BDS tree (because that > should be equivalent to "whether there's a medium") seems to have > its merits. However, I don't think so. blk_is_inserted() is exactly > that function: It's true if there is a BDS tree and, if there is > host passthrough, whether all the media are inserted, which is > correct. No, no, no, no, no. Stop talking about implementation details, talk about concepts. The only valid reason for calling blk_is_inserted() is that you want to know whether a medium is inserted. If this is what you want to know, call it. And if it isn't, be sure not to call it. Whether it has a NULL check here or a BDS tree there doesn't matter at all. You can't just call a function because it happens to have the right side effects if it was never meant for the purpose you're using it. For our specific case: blk->bs == NULL implies an empty drive, but that's just an implication, not an equivalence. The other direction isn't true. That's why it's important to distinguish the cases. > In theory, guest models should not have to distinguish whether a BB > does not have a medium because there is no BDS tree or because we're > using passthrough and that BDS does not have a medium. So why does > floppy have to distinguish? Because it does not have a real tray, > but that's the model we have to be working with. Inserting a medium > into a passed-through host drive must result in the tray being > considered closed immediately; inserting a medium into a guest > device through blockdev-insert-medium must not. That's the job of the block layer, not of the floppy emulation. It's actually not much different from CD-ROM passthrough, where you do have a tray on both the guest and the host. Loading the new medium and closing the tray will have to be a single action there, because real CD-ROMs just don't report whether there is a medium in their open tray. Kevin
On 2015-03-05 at 05:11, Kevin Wolf wrote: > Am 04.03.2015 um 23:06 hat Max Reitz geschrieben: >> On 2015-03-04 at 09:00, Kevin Wolf wrote: >>> Am 09.02.2015 um 18:11 hat Max Reitz geschrieben: >>>> The tray of an FDD is open iff there is no medium inserted (there are >>>> only two states for an FDD: "medium inserted" or "no medium inserted"). >>>> >>>> This results in the tray being reported as open if qemu has been started >>>> with the default floppy drive, which breaks some tests. Fix them. >>>> >>>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>>> --- >>>> hw/block/fdc.c | 20 +++++++++++++--- >>>> tests/fdc-test.c | 4 +--- >>>> tests/qemu-iotests/067.out | 60 +++++++--------------------------------------- >>>> tests/qemu-iotests/071.out | 2 -- >>>> tests/qemu-iotests/081.out | 1 - >>>> tests/qemu-iotests/087.out | 6 ----- >>>> 6 files changed, 26 insertions(+), 67 deletions(-) >>>> >>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c >>>> index 2bf87c9..0c5a6b4 100644 >>>> --- a/hw/block/fdc.c >>>> +++ b/hw/block/fdc.c >>>> @@ -192,6 +192,8 @@ typedef struct FDrive { >>>> uint8_t ro; /* Is read-only */ >>>> uint8_t media_changed; /* Is media changed */ >>>> uint8_t media_rate; /* Data rate of medium */ >>>> + >>>> + bool media_inserted; /* Is there a medium in the tray */ >>>> } FDrive; >>>> static void fd_init(FDrive *drv) >>>> @@ -261,7 +263,7 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t track, uint8_t sect, >>>> #endif >>>> drv->head = head; >>>> if (drv->track != track) { >>>> - if (drv->blk != NULL && blk_is_inserted(drv->blk)) { >>>> + if (drv->media_inserted) { >>> I suspect that with the removal of blk_is_inserted() in several places, >>> floppy passthrough (host_floppy block driver) is now even more broken >>> than before, potentially not noticing removal of a medium any more. >>> >>> While checking this, I noticed that since commit 21fcf360, >>> bdrv_media_changed() is completely unused. Media change has therefore >>> probably been broken since at least May 2012. >>> >>> Considering this, it might actually be reasonable enough to remove the >>> block driver. It's definitely better than having it there, but not >>> working any better than host_device. >>> >>> Of course, alternatively you would also be welcome to fix the device >>> model and reintroduce bdrv_media_changed() and blk_is_inserted() calls >>> where necessary. >> Status: >> >> I thought about why I need this tray status at all. The result is: >> We need it because otherwise blockdev-close-tray doesn't do anything >> on floppy disk drives (you'd insert a medium with >> blockdev-insert-medium and the tray would immediately be reported as >> being closed, without any TRAY_MOVED event). So we do need it. >> >> I tested floppy passthrough in a VM and it's horror, as can be >> expected. For instance, one cannot start qemu with floppy >> passthrough if the host drive is empty (which is actually helpful). >> >> So what I'll be doing is set media_inserted to load && drv->blk in >> the media change CB and set it to true in the floppy drive >> initialization code if there is a BB and blk_is_inserted() is true. >> The tray will be considered closed if media_inserted && >> blk_is_inserted(). >> >> So why do I check blk_is_inserted() in the initialization code? If >> we just set media_inserted to true, that would be wrong. Imagine an >> empty drive (no BDS tree), media_inserted will be set to true, then >> you inserted a BDS tree (blockdev-insert-medium) and the tray will >> be considered closed without you having executed blockdev-close-tray >> (media_inserted is true, and now blk_is_inserted() is true as well). >> >> So I have to check whether there's a BDS tree in the initialization >> code, if there isn't, media_inserted must be false. Why am I not >> using blk_bs() for that? I tried to explain for the USB patch: >> blk_bs() returns a BDS, and that's something for the block layer, >> nobody else should care about that. > This is silly, Max. > > Correctness isn't defined by what you _should_ care about, but by what > you actually _need_ to know. This is where you need to start your > considerations, not with what you think you're supposed to be asking. > > If the two contradict, then there are two options: Either the assumption > that you shouldn't care is wrong, or your decision that you need the > information is. This conflict needs to be resolved rather than just > working with the answer to a different question. > > In this case, I think the problem starts with "empty drive (no BDS > tree)". I meant "Imagine an empty drive without a BDS tree" or "Imagine a drive without a BDS tree, thus empty.", so rather "an empty drive, e.g. no BDS tree" than "an empty drive, i.e. no BDS tree". > This is not correct, an empty drive is defined by > !blk_is_inserted(bs) and not by an empty BDS tree. The passthrough case > (with a hypothetical correct implementation) helps you distinguish the > two. So your assumption that you really need to check whether there is > no BDS tree is wrong here. You need to check whether there is a medium, > and that is blk_is_inserted(). (So yes, I agree with your conclusion, > but not with the way you got there.) So what I was justifying is that blk_bs() will work, not that blk_is_inserted() will not work. So why do I think that blk_is_inserted() is not really what we want here? Okay, so imagine the case where !!blk_bs() != blk_is_inserted() at startup. That means, the host drive is empty. Now we insert a medium in the host drive. Floppy being floppy, qemu won't have any idea that a medium has been inserted. This means that media_inserted will still be false and any read to the BB will be stopped in the device model, so qemu will not have a chance of noticing there is a medium now (it only has if the host floppy BDS is accessed). Thus, using blk_is_inserted() without a medium at startup will not allow the guest to ever read from the disk, unless you manually execute blockdev-close-tray over QMP (I'm not even sure whether that works, but I guess it might). > In the USB case, the problem was that a bdrv_*() function is called > (which already violates the rule that this is not for outside the block > layer) and a NULL check is needed there. In that case the assumption > that calling blk_bs() isn't allowed is wrong; it's simply necessary for > calling bdrv_*() functions. > > In any case "I want to know A, but I'm not supposed to ask for A, so > I'll just check B" is not the way to go. Except for when A and B are functionally equivalent. >> How could blk_is_inserted() be wrong here, if blk_bs() is the thing >> that will work? Correct, if blk_bs() && !bdrv_is_inserted(blk_bs()). >> However, this will never be the case, because as said above, qemu >> actually cannot start with a host floppy drive without a medium, so >> !!blk_is_inserted(blk) == !!blk_bs(blk). > You called it "horror" above and now you want to rely on it? I said: It's horror, but it "is actually helpful". And this is what I meant by it. > The good thing is that I suspect that blk_bs() does _not_ work and > blk_is_inserted() is what you really needed in the first place. > >> I see the question popping up "Why don't you just add a bool >> has_bs(BB) { return blk_bs(BB); } and then not add that test to >> blk_is_inserted()"? I've asked that myself. Answer: Again, anything >> outside of the block layer should not care about things like BDS >> trees. But moreover, bdrv_is_inserted() does not only check whether >> all the host devices represented by BDSs are inserted, but also >> whether BDS.drv != NULL, which until this series was the sign for an >> empty drive. Therefore, checking blk_is_inserted() is the logical >> conclusion of bdrv_is_inserted() (bdrv_is_inserted() tests whether >> BDS.drv != NULL, blk_is_inserted() tests whether BLK.bs != NULL). >> >> But medium management is now done on the BB level, so a separate >> function for checking whether there's a BDS tree (because that >> should be equivalent to "whether there's a medium") seems to have >> its merits. However, I don't think so. blk_is_inserted() is exactly >> that function: It's true if there is a BDS tree and, if there is >> host passthrough, whether all the media are inserted, which is >> correct. > No, no, no, no, no. > > Stop talking about implementation details, talk about concepts. The only > valid reason for calling blk_is_inserted() is that you want to know > whether a medium is inserted. If this is what you want to know, call it. > And if it isn't, be sure not to call it. Maybe. I hate host passthrough, and floppy is the worst incarnation of it. What does this have to do with anything? See my reasoning above why blk_is_inserted() is actually sometimes not what we want to use. > Whether it has a NULL check here or a BDS tree there doesn't matter at > all. You can't just call a function because it happens to have the right > side effects if it was never meant for the purpose you're using it. The problem is that blk_is_inserted() was meant for the purpose I'm using it for, as you yourself said, but it just breaks with floppy passthrough because we have to make the guest access the BDS tree even if there is no medium there. > For our specific case: blk->bs == NULL implies an empty drive, but > that's just an implication, not an equivalence. The other direction > isn't true. That's why it's important to distinguish the cases. > >> In theory, guest models should not have to distinguish whether a BB >> does not have a medium because there is no BDS tree or because we're >> using passthrough and that BDS does not have a medium. So why does >> floppy have to distinguish? Because it does not have a real tray, >> but that's the model we have to be working with. Inserting a medium >> into a passed-through host drive must result in the tray being >> considered closed immediately; inserting a medium into a guest >> device through blockdev-insert-medium must not. > That's the job of the block layer, not of the floppy emulation. So do you want to fix the floppy passthrough code? I don't even know how to fix it. I don't even know what's wrong with it, other than that something's ought to be wrong. > > It's actually not much different from CD-ROM passthrough, where you do > have a tray on both the guest and the host. Loading the new medium and > closing the tray will have to be a single action there, because real > CD-ROMs just don't report whether there is a medium in their open tray. > > Kevin So, what I'm taking from this is the following: media_inserted is a kind of strong attribute. If it is false, no accesses to the BB will be generated whatsoever, and thus the host drive status will not be updated. So we need to make sure it's only false if we really don't care about the BB or the BDS tree; and that's only if there is no BDS tree at all. Because I understand your reasoning on "don't use B if A gives you what you really want, and B just happens to behave the same here", I will be using blk_bs() here, too, instead of blk_is_inserted(), unless you don't find my reasoning correct, and although I'm still very much against using blk_bs() somewhere like here. I will be able to live with it here, just because it's FDD emulation and adding a drive status to it is kind of kaput in itself. Max
Max Reitz <mreitz@redhat.com> writes: [...] > So do you want to fix the floppy passthrough code? I don't even know > how to fix it. I don't even know what's wrong with it, other than that > something's ought to be wrong. Having a driver to pass-through hardware next to nobody has anymore is one thing. But maintaining special cases in the generic block layer for it is another thing altogether: media handling is different, and the difference is not fully encapsulated in the driver (at least last time I checked). Can anybody tell me why spending developer time on floppy passthrough is a justifiable investment? If not, let's drop it and move on. [...]
On 2015-03-16 at 11:47, Markus Armbruster wrote: > Max Reitz <mreitz@redhat.com> writes: > > [...] >> So do you want to fix the floppy passthrough code? I don't even know >> how to fix it. I don't even know what's wrong with it, other than that >> something's ought to be wrong. > Having a driver to pass-through hardware next to nobody has anymore is > one thing. But maintaining special cases in the generic block layer for > it is another thing altogether: media handling is different, and the > difference is not fully encapsulated in the driver (at least last time I > checked). > > Can anybody tell me why spending developer time on floppy passthrough is > a justifiable investment? > > If not, let's drop it and move on. Do you want to do it? I don't want to be held responsible for it. ;-) Max
diff --git a/hw/block/fdc.c b/hw/block/fdc.c index 2bf87c9..0c5a6b4 100644 --- a/hw/block/fdc.c +++ b/hw/block/fdc.c @@ -192,6 +192,8 @@ typedef struct FDrive { uint8_t ro; /* Is read-only */ uint8_t media_changed; /* Is media changed */ uint8_t media_rate; /* Data rate of medium */ + + bool media_inserted; /* Is there a medium in the tray */ } FDrive; static void fd_init(FDrive *drv) @@ -261,7 +263,7 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t track, uint8_t sect, #endif drv->head = head; if (drv->track != track) { - if (drv->blk != NULL && blk_is_inserted(drv->blk)) { + if (drv->media_inserted) { drv->media_changed = 0; } ret = 1; @@ -270,7 +272,7 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t track, uint8_t sect, drv->sect = sect; } - if (drv->blk == NULL || !blk_is_inserted(drv->blk)) { + if (!drv->media_inserted) { ret = 2; } @@ -296,7 +298,7 @@ static void fd_revalidate(FDrive *drv) ro = blk_is_read_only(drv->blk); pick_geometry(drv->blk, &nb_heads, &max_track, &last_sect, drv->drive, &drive, &rate); - if (!blk_is_inserted(drv->blk)) { + if (!drv->media_inserted) { FLOPPY_DPRINTF("No disk in drive\n"); } else { FLOPPY_DPRINTF("Floppy disk (%d h %d t %d s) %s\n", nb_heads, @@ -2062,12 +2064,21 @@ static void fdctrl_change_cb(void *opaque, bool load) { FDrive *drive = opaque; + drive->media_inserted = load && drive->blk && blk_is_inserted(drive->blk); + drive->media_changed = 1; fd_revalidate(drive); } +static bool fdctrl_is_tray_open(void *opaque) +{ + FDrive *drive = opaque; + return !drive->media_inserted; +} + static const BlockDevOps fdctrl_block_ops = { .change_media_cb = fdctrl_change_cb, + .is_tray_open = fdctrl_is_tray_open, }; /* Init functions */ @@ -2095,6 +2106,9 @@ static void fdctrl_connect_drives(FDCtrl *fdctrl, Error **errp) fdctrl_change_cb(drive, 0); if (drive->blk) { blk_set_dev_ops(drive->blk, &fdctrl_block_ops, drive); + if (blk_is_inserted(drive->blk)) { + drive->media_inserted = true; + } } } } diff --git a/tests/fdc-test.c b/tests/fdc-test.c index 3c6c83c..f287c10 100644 --- a/tests/fdc-test.c +++ b/tests/fdc-test.c @@ -293,9 +293,7 @@ static void test_media_insert(void) qmp_discard_response("{'execute':'change', 'arguments':{" " 'device':'floppy0', 'target': %s, 'arg': 'raw' }}", test_image); - qmp_discard_response(""); /* ignore event - (FIXME open -> open transition?!) */ - qmp_discard_response(""); /* ignore event */ + qmp_discard_response(""); /* ignore event (open -> close) */ dir = inb(FLOPPY_BASE + reg_dir); assert_bit_set(dir, DSKCHG); diff --git a/tests/qemu-iotests/067.out b/tests/qemu-iotests/067.out index 00b3eae..42bae32 100644 --- a/tests/qemu-iotests/067.out +++ b/tests/qemu-iotests/067.out @@ -69,7 +69,7 @@ Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,if=none,id=disk -device virti "device": "floppy0", "locked": false, "removable": true, - "tray_open": false, + "tray_open": true, "type": "unknown" }, { @@ -131,7 +131,7 @@ Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,if=none,id=disk -device virti "device": "floppy0", "locked": false, "removable": true, - "tray_open": false, + "tray_open": true, "type": "unknown" }, { @@ -165,17 +165,6 @@ Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,if=none,id=disk -device virti "tray-open": true } } -{ - "timestamp": { - "seconds": TIMESTAMP, - "microseconds": TIMESTAMP - }, - "event": "DEVICE_TRAY_MOVED", - "data": { - "device": "floppy0", - "tray-open": true - } -} === -drive/device_add and device_del === @@ -246,7 +235,7 @@ Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,if=none,id=disk "device": "floppy0", "locked": false, "removable": true, - "tray_open": false, + "tray_open": true, "type": "unknown" }, { @@ -312,7 +301,7 @@ Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,if=none,id=disk "device": "floppy0", "locked": false, "removable": true, - "tray_open": false, + "tray_open": true, "type": "unknown" }, { @@ -346,17 +335,6 @@ Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,if=none,id=disk "tray-open": true } } -{ - "timestamp": { - "seconds": TIMESTAMP, - "microseconds": TIMESTAMP - }, - "event": "DEVICE_TRAY_MOVED", - "data": { - "device": "floppy0", - "tray-open": true - } -} === drive_add/device_add and device_del === @@ -386,7 +364,7 @@ Testing: "device": "floppy0", "locked": false, "removable": true, - "tray_open": false, + "tray_open": true, "type": "unknown" }, { @@ -496,7 +474,7 @@ Testing: "device": "floppy0", "locked": false, "removable": true, - "tray_open": false, + "tray_open": true, "type": "unknown" }, { @@ -530,17 +508,6 @@ Testing: "tray-open": true } } -{ - "timestamp": { - "seconds": TIMESTAMP, - "microseconds": TIMESTAMP - }, - "event": "DEVICE_TRAY_MOVED", - "data": { - "device": "floppy0", - "tray-open": true - } -} === blockdev_add/device_add and device_del === @@ -571,7 +538,7 @@ Testing: "device": "floppy0", "locked": false, "removable": true, - "tray_open": false, + "tray_open": true, "type": "unknown" }, { @@ -681,7 +648,7 @@ Testing: "device": "floppy0", "locked": false, "removable": true, - "tray_open": false, + "tray_open": true, "type": "unknown" }, { @@ -760,16 +727,5 @@ Testing: "tray-open": true } } -{ - "timestamp": { - "seconds": TIMESTAMP, - "microseconds": TIMESTAMP - }, - "event": "DEVICE_TRAY_MOVED", - "data": { - "device": "floppy0", - "tray-open": true - } -} *** done diff --git a/tests/qemu-iotests/071.out b/tests/qemu-iotests/071.out index c8ecfaf..904ce15 100644 --- a/tests/qemu-iotests/071.out +++ b/tests/qemu-iotests/071.out @@ -52,7 +52,6 @@ read failed: Input/output error {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}} QEMU_PROG: Failed to flush the L2 table cache: Input/output error QEMU_PROG: Failed to flush the refcount block cache: Input/output error @@ -95,7 +94,6 @@ read failed: Input/output error {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}} QEMU_PROG: Failed to flush the L2 table cache: Input/output error QEMU_PROG: Failed to flush the refcount block cache: Input/output error diff --git a/tests/qemu-iotests/081.out b/tests/qemu-iotests/081.out index 2375916..b1e4909 100644 --- a/tests/qemu-iotests/081.out +++ b/tests/qemu-iotests/081.out @@ -38,7 +38,6 @@ read 10485760/10485760 bytes at offset 0 {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}} == using quorum rewrite corrupted mode == diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out index b0aa06d..55f4951 100644 --- a/tests/qemu-iotests/087.out +++ b/tests/qemu-iotests/087.out @@ -10,7 +10,6 @@ QMP_VERSION {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}} === Duplicate ID === @@ -27,7 +26,6 @@ QMP_VERSION {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}} === aio=native without O_DIRECT === @@ -39,7 +37,6 @@ QMP_VERSION {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}} === Encrypted image === @@ -52,7 +49,6 @@ QMP_VERSION {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}} Testing: QMP_VERSION @@ -61,7 +57,6 @@ QMP_VERSION {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}} === Missing driver === @@ -74,6 +69,5 @@ QMP_VERSION {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}} *** done
The tray of an FDD is open iff there is no medium inserted (there are only two states for an FDD: "medium inserted" or "no medium inserted"). This results in the tray being reported as open if qemu has been started with the default floppy drive, which breaks some tests. Fix them. Signed-off-by: Max Reitz <mreitz@redhat.com> --- hw/block/fdc.c | 20 +++++++++++++--- tests/fdc-test.c | 4 +--- tests/qemu-iotests/067.out | 60 +++++++--------------------------------------- tests/qemu-iotests/071.out | 2 -- tests/qemu-iotests/081.out | 1 - tests/qemu-iotests/087.out | 6 ----- 6 files changed, 26 insertions(+), 67 deletions(-)