[Qemu-devel] [PATCH] Add qerror message if the 'change' target filename can't be opened

[Qemu-devel] [PATCH] Add qerror message if the 'change' target filename can't be opened

[Ryan Harper]

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(-)

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);
-- 
1.6.3.3


-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com

Re: [Qemu-devel] [PATCH] Add qerror message if the 'change' target filename can't be opened

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH] Add qerror message if the 'change' target filename can't be opened

[Ryan Harper]

* 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?

-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com

Re: [Qemu-devel] [PATCH] Add qerror message if the 'change' target filename can't be opened

[Markus Armbruster]

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!

Re: [Qemu-devel] [PATCH] Add qerror message if the 'change' target filename can't be opened

[Luiz Capitulino]

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);

Re: [Qemu-devel] [PATCH] Add qerror message if the 'change' target filename can't be opened

[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?

Re: [Qemu-devel] [PATCH] Add qerror message if the 'change' target filename can't be opened

[Ryan Harper]

* 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).


-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com

Re: [Qemu-devel] [PATCH] Add qerror message if the 'change' target filename can't be opened

[Luiz Capitulino]

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.

Re: [Qemu-devel] [PATCH] Add qerror message if the 'change' target filename can't be opened

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH] Add qerror message if the 'change' target filename can't be opened

[Luiz Capitulino]

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.