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

login
register
mail settings
Submitter Supriya Kannery
Date Nov. 11, 2011, 6:47 a.m.
Message ID <20111111064735.15024.68351.sendpatchset@skannery.in.ibm.com>
Download mbox | patch
Permalink /patch/125096/
State New
Headers show

Comments

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

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 } }"