| Message ID | 20100325143258.GS27260@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
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.
* 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 <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!
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);
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?
* 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).
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.
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
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.
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);
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(-)