[v9,2/6] Qemu: Error classes for file reopen and data sync failure

Submitted by Supriya Kannery on Nov. 11, 2011, 6:47 a.m.

Details

Message ID 20111111064735.15024.68351.sendpatchset@skannery.in.ibm.com
State New
Headers show

Commit Message

Supriya Kannery Nov. 11, 2011, 6:47 a.m.
New error classes defined for file reopen failure and data
sync error

Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>

---
 qerror.c |    8 ++++++++
 qerror.h |    6 ++++++
 2 files changed, 14 insertions(+)

Comments

Luiz Capitulino Nov. 17, 2011, 12:50 p.m.
On Fri, 11 Nov 2011 12:17:35 +0530
Supriya Kannery <supriyak@linux.vnet.ibm.com> wrote:

> New error classes defined for file reopen failure and data
> sync error
> 
> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
> 
> ---
>  qerror.c |    8 ++++++++
>  qerror.h |    6 ++++++
>  2 files changed, 14 insertions(+)
> 
> Index: qemu/qerror.c
> ===================================================================
> --- qemu.orig/qerror.c
> +++ qemu/qerror.c
> @@ -97,6 +97,14 @@ static const QErrorStringTable qerror_ta
>          .desc      = "Device '%(device)' is not removable",
>      },
>      {
> +        .error_fmt = QERR_DATA_SYNC_FAILED,
> +        .desc      = "Syncing of data failed for device '%(device)'",
> +    },
> +    {
> +        .error_fmt = QERR_REOPEN_FILE_FAILED,
> +        .desc      = "Could not reopen '%(filename)'",
> +    },

Is this really needed? I think you could use QERR_OPEN_FILE_FAILED.

> +    {
>          .error_fmt = QERR_DEVICE_NO_BUS,
>          .desc      = "Device '%(device)' has no child bus",
>      },
> Index: qemu/qerror.h
> ===================================================================
> --- qemu.orig/qerror.h
> +++ qemu/qerror.h
> @@ -87,6 +87,9 @@ QError *qobject_to_qerror(const QObject 
>  #define QERR_DEVICE_NOT_FOUND \
>      "{ 'class': 'DeviceNotFound', 'data': { 'device': %s } }"
>  
> +#define QERR_DATA_SYNC_FAILED \
> +    "{ 'class': 'DataSyncFailed', 'data': { 'device': %s } }"
> +
>  #define QERR_DEVICE_NOT_REMOVABLE \
>      "{ 'class': 'DeviceNotRemovable', 'data': { 'device': %s } }"
>  
> @@ -144,6 +147,9 @@ QError *qobject_to_qerror(const QObject 
>  #define QERR_OPEN_FILE_FAILED \
>      "{ 'class': 'OpenFileFailed', 'data': { 'filename': %s } }"
>  
> +#define QERR_REOPEN_FILE_FAILED \
> +    "{ 'class': 'ReopenFileFailed', 'data': { 'filename': %s } }"
> +
>  #define QERR_PROPERTY_NOT_FOUND \
>      "{ 'class': 'PropertyNotFound', 'data': { 'device': %s, 'property': %s } }"
>  
>
supriya kannery Nov. 18, 2011, 9:29 a.m.
Luiz Capitulino wrote:
> On Fri, 11 Nov 2011 12:17:35 +0530
> Supriya Kannery <supriyak@linux.vnet.ibm.com> wrote:
>
>   
>> New error classes defined for file reopen failure and data
>> sync error
>>
>> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
>>
>> ---
>>  qerror.c |    8 ++++++++
>>  qerror.h |    6 ++++++
>>  2 files changed, 14 insertions(+)
>>
>> Index: qemu/qerror.c
>> ===================================================================
>> --- qemu.orig/qerror.c
>> +++ qemu/qerror.c
>> @@ -97,6 +97,14 @@ static const QErrorStringTable qerror_ta
>>          .desc      = "Device '%(device)' is not removable",
>>      },
>>      {
>> +        .error_fmt = QERR_DATA_SYNC_FAILED,
>> +        .desc      = "Syncing of data failed for device '%(device)'",
>> +    },
>> +    {
>> +        .error_fmt = QERR_REOPEN_FILE_FAILED,
>> +        .desc      = "Could not reopen '%(filename)'",
>> +    },
>>     
>
> Is this really needed? I think you could use QERR_OPEN_FILE_FAILED.
>
>   

This could confuse the user, as to what was tried on an
already opened file by qemu. So I feel, QERR_REOPEN_FILE_FAILED can
bring in better clarity to the message passed to the user.

- thanks Supriya
Luiz Capitulino Nov. 18, 2011, 12:09 p.m.
On Fri, 18 Nov 2011 14:59:40 +0530
supriya kannery <supriyak@in.ibm.com> wrote:

> Luiz Capitulino wrote:
> > On Fri, 11 Nov 2011 12:17:35 +0530
> > Supriya Kannery <supriyak@linux.vnet.ibm.com> wrote:
> >
> >   
> >> New error classes defined for file reopen failure and data
> >> sync error
> >>
> >> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
> >>
> >> ---
> >>  qerror.c |    8 ++++++++
> >>  qerror.h |    6 ++++++
> >>  2 files changed, 14 insertions(+)
> >>
> >> Index: qemu/qerror.c
> >> ===================================================================
> >> --- qemu.orig/qerror.c
> >> +++ qemu/qerror.c
> >> @@ -97,6 +97,14 @@ static const QErrorStringTable qerror_ta
> >>          .desc      = "Device '%(device)' is not removable",
> >>      },
> >>      {
> >> +        .error_fmt = QERR_DATA_SYNC_FAILED,
> >> +        .desc      = "Syncing of data failed for device '%(device)'",
> >> +    },
> >> +    {
> >> +        .error_fmt = QERR_REOPEN_FILE_FAILED,
> >> +        .desc      = "Could not reopen '%(filename)'",
> >> +    },
> >>     
> >
> > Is this really needed? I think you could use QERR_OPEN_FILE_FAILED.
> >
> >   
> 
> This could confuse the user, as to what was tried on an
> already opened file by qemu. So I feel, QERR_REOPEN_FILE_FAILED can
> bring in better clarity to the message passed to the user.

I think it's the contrary.

First, the fact that we reopen the image is an implementation detail. If we
do it differently in the future than the error won't fit anymore.

Second, what did really fail when REOPEN_FILE_FAILED is triggered? Flushing
the buffers? Closing the image? Opening it with different flags? All these
operations are part of the "reopen" algorithm.

On the other hand, if you see a OPEN_FILE_FAILED then there's only one
single operation that might have failed. Also, OPEN_FILE_FAILED is widely
well known as some version of it exists in any operating system.

Patch hide | download patch | download mbox

Index: qemu/qerror.c
===================================================================
--- qemu.orig/qerror.c
+++ qemu/qerror.c
@@ -97,6 +97,14 @@  static const QErrorStringTable qerror_ta
         .desc      = "Device '%(device)' is not removable",
     },
     {
+        .error_fmt = QERR_DATA_SYNC_FAILED,
+        .desc      = "Syncing of data failed for device '%(device)'",
+    },
+    {
+        .error_fmt = QERR_REOPEN_FILE_FAILED,
+        .desc      = "Could not reopen '%(filename)'",
+    },
+    {
         .error_fmt = QERR_DEVICE_NO_BUS,
         .desc      = "Device '%(device)' has no child bus",
     },
Index: qemu/qerror.h
===================================================================
--- qemu.orig/qerror.h
+++ qemu/qerror.h
@@ -87,6 +87,9 @@  QError *qobject_to_qerror(const QObject 
 #define QERR_DEVICE_NOT_FOUND \
     "{ 'class': 'DeviceNotFound', 'data': { 'device': %s } }"
 
+#define QERR_DATA_SYNC_FAILED \
+    "{ 'class': 'DataSyncFailed', 'data': { 'device': %s } }"
+
 #define QERR_DEVICE_NOT_REMOVABLE \
     "{ 'class': 'DeviceNotRemovable', 'data': { 'device': %s } }"
 
@@ -144,6 +147,9 @@  QError *qobject_to_qerror(const QObject 
 #define QERR_OPEN_FILE_FAILED \
     "{ 'class': 'OpenFileFailed', 'data': { 'filename': %s } }"
 
+#define QERR_REOPEN_FILE_FAILED \
+    "{ 'class': 'ReopenFileFailed', 'data': { 'filename': %s } }"
+
 #define QERR_PROPERTY_NOT_FOUND \
     "{ 'class': 'PropertyNotFound', 'data': { 'device': %s, 'property': %s } }"