Patchwork Add qerror message if the 'change' target filename can't be opened

login
register
mail settings
Submitter Ryan Harper
Date March 25, 2010, 2:32 p.m.
Message ID <20100325143258.GS27260@us.ibm.com>
Download mbox | patch
Permalink /patch/48524/
State New
Headers show

Comments

Ryan Harper - March 25, 2010, 2:32 p.m.
Currently when using the change command to switch the file in the cd drive
the command doesn't complain if the file doesn't exit or can't be opened
and the drive keeps the existing image.  This patch adds a qerror_report
call to print a message out indicating the failure.  This error message
can be used to catch failures.

Current behavior:

QEMU 0.12.50 monitor - type 'help' for more information
(qemu) info block
ide0-hd0: type=hd removable=0 file=/dev/null ro=0 drv=host_device encrypted=0
ide1-cd0: type=cdrom removable=1 locked=0 [not inserted]
floppy0: type=floppy removable=1 locked=0 [not inserted]
sd0: type=floppy removable=1 locked=0 [not inserted]
(qemu) change ide1-cd0 /home/rharper/work/isos/Fedora-9-i386-DVD.iso 
(qemu) info block
ide0-hd0: type=hd removable=0 file=/dev/null ro=0 drv=host_device encrypted=0
ide1-cd0: type=cdrom removable=1 locked=0
file=/home/rharper/work/isos/Fedora-9-i386-DVD.iso ro=0 drv=raw encrypted=0
floppy0: type=floppy removable=1 locked=0 [not inserted]
sd0: type=floppy removable=1 locked=0 [not inserted]
(qemu) change ide1-cd0 /tmp/non_existent_file.iso
(qemu) info block
ide0-hd0: type=hd removable=0 file=/dev/null ro=0 drv=host_device encrypted=0
ide1-cd0: type=cdrom removable=1 locked=0 [not inserted]
floppy0: type=floppy removable=1 locked=0 [not inserted]
sd0: type=floppy removable=1 locked=0 [not inserted]
(qemu)

With patch:
QEMU 0.12.50 monitor - type 'help' for more information
(qemu) change ide1-cd0 /tmp/non_existent_file.iso
Could not open '/tmp/non_existent_file.iso'
(qemu) 


Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
---
 monitor.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
Markus Armbruster - March 25, 2010, 4:40 p.m.
Ryan Harper <ryanh@us.ibm.com> writes:

> Currently when using the change command to switch the file in the cd drive
> the command doesn't complain if the file doesn't exit or can't be opened
> and the drive keeps the existing image.  This patch adds a qerror_report
> call to print a message out indicating the failure.  This error message
> can be used to catch failures.
>
[...]
>
> Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
> ---
>  monitor.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 0448a70..196c7a6 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1099,6 +1099,7 @@ static int do_change_block(Monitor *mon, const char *device,
>          return -1;
>      }
>      if (bdrv_open2(bs, filename, BDRV_O_RDWR, drv) < 0) {
> +        qerror_report(QERR_OPEN_FILE_FAILED, filename);
>          return -1;
>      }
>      return monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
> -- 

We want this fix for QMP.  Without it, we get UndefinedError, and a
complaint ifdef CONFIG_DEBUG_MONITOR.
Ryan Harper - March 25, 2010, 7:20 p.m.
* Markus Armbruster <armbru@redhat.com> [2010-03-25 11:41]:
> Ryan Harper <ryanh@us.ibm.com> writes:
> 
> > Currently when using the change command to switch the file in the cd drive
> > the command doesn't complain if the file doesn't exit or can't be opened
> > and the drive keeps the existing image.  This patch adds a qerror_report
> > call to print a message out indicating the failure.  This error message
> > can be used to catch failures.
> >
> [...]
> >
> > Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
> > ---
> >  monitor.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 0448a70..196c7a6 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -1099,6 +1099,7 @@ static int do_change_block(Monitor *mon, const char *device,
> >          return -1;
> >      }
> >      if (bdrv_open2(bs, filename, BDRV_O_RDWR, drv) < 0) {
> > +        qerror_report(QERR_OPEN_FILE_FAILED, filename);
> >          return -1;
> >      }
> >      return monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
> > -- 
> 
> We want this fix for QMP.  Without it, we get UndefinedError, and a
> complaint ifdef CONFIG_DEBUG_MONITOR.

I'm not terribly familiar with the QMP stuff.  Are you looking for me to
fix some some stuff in the QMP bits?  Are you just indicating that you
need this fix for QMP?
Markus Armbruster - March 25, 2010, 7:42 p.m.
Ryan Harper <ryanh@us.ibm.com> writes:

> * Markus Armbruster <armbru@redhat.com> [2010-03-25 11:41]:
>> Ryan Harper <ryanh@us.ibm.com> writes:
>> 
>> > Currently when using the change command to switch the file in the cd drive
>> > the command doesn't complain if the file doesn't exit or can't be opened
>> > and the drive keeps the existing image.  This patch adds a qerror_report
>> > call to print a message out indicating the failure.  This error message
>> > can be used to catch failures.
>> >
>> [...]
>> >
>> > Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
>> > ---
>> >  monitor.c |    1 +
>> >  1 files changed, 1 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/monitor.c b/monitor.c
>> > index 0448a70..196c7a6 100644
>> > --- a/monitor.c
>> > +++ b/monitor.c
>> > @@ -1099,6 +1099,7 @@ static int do_change_block(Monitor *mon, const char *device,
>> >          return -1;
>> >      }
>> >      if (bdrv_open2(bs, filename, BDRV_O_RDWR, drv) < 0) {
>> > +        qerror_report(QERR_OPEN_FILE_FAILED, filename);
>> >          return -1;
>> >      }
>> >      return monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
>> > -- 
>> 
>> We want this fix for QMP.  Without it, we get UndefinedError, and a
>> complaint ifdef CONFIG_DEBUG_MONITOR.
>
> I'm not terribly familiar with the QMP stuff.  Are you looking for me to
> fix some some stuff in the QMP bits?  Are you just indicating that you
> need this fix for QMP?

The latter.

Thanks for fixing!
Luiz Capitulino - April 1, 2010, 6:15 p.m.
On Thu, 25 Mar 2010 09:32:58 -0500
Ryan Harper <ryanh@us.ibm.com> wrote:

> Currently when using the change command to switch the file in the cd drive
> the command doesn't complain if the file doesn't exit or can't be opened
> and the drive keeps the existing image.  This patch adds a qerror_report
> call to print a message out indicating the failure.  This error message
> can be used to catch failures.

 Looks good to me, but it doesn't keep the existing image, it will silently
eject it instead.

> 
> Current behavior:
> 
> QEMU 0.12.50 monitor - type 'help' for more information
> (qemu) info block
> ide0-hd0: type=hd removable=0 file=/dev/null ro=0 drv=host_device encrypted=0
> ide1-cd0: type=cdrom removable=1 locked=0 [not inserted]
> floppy0: type=floppy removable=1 locked=0 [not inserted]
> sd0: type=floppy removable=1 locked=0 [not inserted]
> (qemu) change ide1-cd0 /home/rharper/work/isos/Fedora-9-i386-DVD.iso 
> (qemu) info block
> ide0-hd0: type=hd removable=0 file=/dev/null ro=0 drv=host_device encrypted=0
> ide1-cd0: type=cdrom removable=1 locked=0
> file=/home/rharper/work/isos/Fedora-9-i386-DVD.iso ro=0 drv=raw encrypted=0
> floppy0: type=floppy removable=1 locked=0 [not inserted]
> sd0: type=floppy removable=1 locked=0 [not inserted]
> (qemu) change ide1-cd0 /tmp/non_existent_file.iso
> (qemu) info block
> ide0-hd0: type=hd removable=0 file=/dev/null ro=0 drv=host_device encrypted=0
> ide1-cd0: type=cdrom removable=1 locked=0 [not inserted]
> floppy0: type=floppy removable=1 locked=0 [not inserted]
> sd0: type=floppy removable=1 locked=0 [not inserted]
> (qemu)
> 
> With patch:
> QEMU 0.12.50 monitor - type 'help' for more information
> (qemu) change ide1-cd0 /tmp/non_existent_file.iso
> Could not open '/tmp/non_existent_file.iso'
> (qemu) 
> 
> 
> Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
> ---
>  monitor.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 0448a70..196c7a6 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1099,6 +1099,7 @@ static int do_change_block(Monitor *mon, const char *device,
>          return -1;
>      }
>      if (bdrv_open2(bs, filename, BDRV_O_RDWR, drv) < 0) {
> +        qerror_report(QERR_OPEN_FILE_FAILED, filename);
>          return -1;
>      }
>      return monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
Luiz Capitulino - April 1, 2010, 6:22 p.m.
On Thu, 1 Apr 2010 15:15:51 -0300
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> On Thu, 25 Mar 2010 09:32:58 -0500
> Ryan Harper <ryanh@us.ibm.com> wrote:
> 
> > Currently when using the change command to switch the file in the cd drive
> > the command doesn't complain if the file doesn't exit or can't be opened
> > and the drive keeps the existing image.  This patch adds a qerror_report
> > call to print a message out indicating the failure.  This error message
> > can be used to catch failures.
> 
>  Looks good to me, but it doesn't keep the existing image, it will silently
> eject it instead.

 And, thinking more about it this seems the wrong behavior to me, if it
fails to open the file, it should not touch the current one.

 Am I right, Kevin?
Ryan Harper - April 1, 2010, 6:23 p.m.
* Luiz Capitulino <lcapitulino@redhat.com> [2010-04-01 13:17]:
> On Thu, 25 Mar 2010 09:32:58 -0500
> Ryan Harper <ryanh@us.ibm.com> wrote:
> 
> > Currently when using the change command to switch the file in the cd drive
> > the command doesn't complain if the file doesn't exit or can't be opened
> > and the drive keeps the existing image.  This patch adds a qerror_report
> > call to print a message out indicating the failure.  This error message
> > can be used to catch failures.
> 
>  Looks good to me, but it doesn't keep the existing image, it will silently
> eject it instead.

That's true, but that happens whether or not we indicate the change
failed.  I think fixing that is a good idea, but it shouldn't part of
this patch (just indicating the failure).
Luiz Capitulino - April 1, 2010, 6:31 p.m.
On Thu, 1 Apr 2010 13:23:44 -0500
Ryan Harper <ryanh@us.ibm.com> wrote:

> * Luiz Capitulino <lcapitulino@redhat.com> [2010-04-01 13:17]:
> > On Thu, 25 Mar 2010 09:32:58 -0500
> > Ryan Harper <ryanh@us.ibm.com> wrote:
> > 
> > > Currently when using the change command to switch the file in the cd drive
> > > the command doesn't complain if the file doesn't exit or can't be opened
> > > and the drive keeps the existing image.  This patch adds a qerror_report
> > > call to print a message out indicating the failure.  This error message
> > > can be used to catch failures.
> > 
> >  Looks good to me, but it doesn't keep the existing image, it will silently
> > eject it instead.
> 
> That's true, but that happens whether or not we indicate the change
> failed.  I think fixing that is a good idea, but it shouldn't part of
> this patch (just indicating the failure).

 Sure, this patch is good by itself.
Kevin Wolf - April 1, 2010, 7:14 p.m.
Am 01.04.2010 20:22, schrieb Luiz Capitulino:
> On Thu, 1 Apr 2010 15:15:51 -0300
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
> 
>> On Thu, 25 Mar 2010 09:32:58 -0500
>> Ryan Harper <ryanh@us.ibm.com> wrote:
>>
>>> Currently when using the change command to switch the file in the cd drive
>>> the command doesn't complain if the file doesn't exit or can't be opened
>>> and the drive keeps the existing image.  This patch adds a qerror_report
>>> call to print a message out indicating the failure.  This error message
>>> can be used to catch failures.
>>
>>  Looks good to me, but it doesn't keep the existing image, it will silently
>> eject it instead.
> 
>  And, thinking more about it this seems the wrong behavior to me, if it
> fails to open the file, it should not touch the current one.
> 
>  Am I right, Kevin?

Well, it's a monitor command. I guess its meaning has never been clearly
defined, so I tend to say there is no right or wrong. From a user
perspective, intuitively I would expect that it succeeds completely or
maintains the old state, so yes.

However, I don't think it's fixable that easy. You obviously need to
close the image before you can open it. And reopening the image in case
of failure - well, we just had this discussion and I'm sure Juan wants
to comment on it...

It's a case that we should consider if/when we reorganize bdrv_open. The
"bdrv_close, but without closing the fd" thing doesn't work out here
because we need to reuse the same bs. Or maybe open a different bs first
and then copy things over. Could actually work this way.

Kevin
Luiz Capitulino - April 1, 2010, 8:55 p.m.
On Thu, 01 Apr 2010 21:14:30 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 01.04.2010 20:22, schrieb Luiz Capitulino:
> > On Thu, 1 Apr 2010 15:15:51 -0300
> > Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > 
> >> On Thu, 25 Mar 2010 09:32:58 -0500
> >> Ryan Harper <ryanh@us.ibm.com> wrote:
> >>
> >>> Currently when using the change command to switch the file in the cd drive
> >>> the command doesn't complain if the file doesn't exit or can't be opened
> >>> and the drive keeps the existing image.  This patch adds a qerror_report
> >>> call to print a message out indicating the failure.  This error message
> >>> can be used to catch failures.
> >>
> >>  Looks good to me, but it doesn't keep the existing image, it will silently
> >> eject it instead.
> > 
> >  And, thinking more about it this seems the wrong behavior to me, if it
> > fails to open the file, it should not touch the current one.
> > 
> >  Am I right, Kevin?
> 
> Well, it's a monitor command. I guess its meaning has never been clearly
> defined, so I tend to say there is no right or wrong. From a user
> perspective, intuitively I would expect that it succeeds completely or
> maintains the old state, so yes.

 Yes and I'd expect the same from a QMP perspective.
 
> However, I don't think it's fixable that easy. You obviously need to
> close the image before you can open it. And reopening the image in case
> of failure - well, we just had this discussion and I'm sure Juan wants
> to comment on it...
> 
> It's a case that we should consider if/when we reorganize bdrv_open. The
> "bdrv_close, but without closing the fd" thing doesn't work out here
> because we need to reuse the same bs. Or maybe open a different bs first
> and then copy things over. Could actually work this way.

 Yeah, I looked there and realized it wasn't easy but this solution
looks good.

Patch

diff --git a/monitor.c b/monitor.c
index 0448a70..196c7a6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1099,6 +1099,7 @@  static int do_change_block(Monitor *mon, const char *device,
         return -1;
     }
     if (bdrv_open2(bs, filename, BDRV_O_RDWR, drv) < 0) {
+        qerror_report(QERR_OPEN_FILE_FAILED, filename);
         return -1;
     }
     return monitor_read_bdrv_key_start(mon, bs, NULL, NULL);