diff mbox series

[3/4] util: Add 'label' field to VIR_ENUM_IMPL

Message ID e138f0b05a715cb88c684d76313e93186b709ef3.1554737741.git.crobinso@redhat.com
State New
Headers show
Series Add 'label' arg to VIR_ENUM_IMPL | expand

Commit Message

Cole Robinson April 8, 2019, 3:48 p.m. UTC 
This allows us to raise error messages from virEnum*String functions.

FromString failure will report this error for value 'zzzz'

  invalid argument: Unknown 'domain type' value 'zzzz'

ToString failure will report this error for unknown type=83

  internal error: Unknown 'domain type' internal value '83'

However we disable the error reporting for now. It should only be
enabled when we decide to begin dropping duplicate error reporting.

Note: this patch will be combined with the next patch when pushing

Signed-off-by: Cole Robinson <crobinso@redhat.com>

---
 docs/apibuild.py   | 12 ++++++++++++
 src/util/virenum.c | 22 ++++++++++++++++++----
 src/util/virenum.h | 15 ++++++++++-----
 3 files changed, 40 insertions(+), 9 deletions(-)

-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Comments

Daniel P. Berrangé April 11, 2019, 10:57 a.m. UTC | #1 
On Mon, Apr 08, 2019 at 11:48:18AM -0400, Cole Robinson wrote:
> This allows us to raise error messages from virEnum*String functions.

> 

> FromString failure will report this error for value 'zzzz'

> 

>   invalid argument: Unknown 'domain type' value 'zzzz'

> 

> ToString failure will report this error for unknown type=83

> 

>   internal error: Unknown 'domain type' internal value '83'

> 

> However we disable the error reporting for now. It should only be

> enabled when we decide to begin dropping duplicate error reporting.


Why don't we *enable* error reporting now, but only if the label
string is non-NULL. Then just change your second patch to pass
NULL everywhere.  We can then incrementally replace the NULL with
a real message at the same time as we update each caller to drop
the existing error reporting.

This avoids any point in time having double reporting.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson April 11, 2019, 4:27 p.m. UTC | #2 
On 4/11/19 6:57 AM, Daniel P. Berrangé wrote:
> On Mon, Apr 08, 2019 at 11:48:18AM -0400, Cole Robinson wrote:
>> This allows us to raise error messages from virEnum*String functions.
>>
>> FromString failure will report this error for value 'zzzz'
>>
>>   invalid argument: Unknown 'domain type' value 'zzzz'
>>
>> ToString failure will report this error for unknown type=83
>>
>>   internal error: Unknown 'domain type' internal value '83'
>>
>> However we disable the error reporting for now. It should only be
>> enabled when we decide to begin dropping duplicate error reporting.
> 
> Why don't we *enable* error reporting now, but only if the label
> string is non-NULL. Then just change your second patch to pass
> NULL everywhere.  We can then incrementally replace the NULL with
> a real message at the same time as we update each caller to drop
> the existing error reporting.
> 
> This avoids any point in time having double reporting.
> 

Yes but this hits the case I mentioned in the cover letter: until the
code base is converted it will be easier for new code to neglect to
raise virReportError when the enum hasn't been converted yet, and IMO
reporting no error is much worse than double reporting.

If you still think it's the way to go I'll rework it. But then maybe we
need to think more about the strategy and timing of how we plan to
convert the code, do we shoot for doing it in a single release window?

The NULL idea is a good one anyway because at least some places like the
Tristate functions we likely won't want to raise errors, but those may
need to be treated differently anyways.

Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Daniel P. Berrangé April 11, 2019, 4:33 p.m. UTC | #3 
On Thu, Apr 11, 2019 at 12:27:56PM -0400, Cole Robinson wrote:
> On 4/11/19 6:57 AM, Daniel P. Berrangé wrote:
> > On Mon, Apr 08, 2019 at 11:48:18AM -0400, Cole Robinson wrote:
> >> This allows us to raise error messages from virEnum*String functions.
> >>
> >> FromString failure will report this error for value 'zzzz'
> >>
> >>   invalid argument: Unknown 'domain type' value 'zzzz'
> >>
> >> ToString failure will report this error for unknown type=83
> >>
> >>   internal error: Unknown 'domain type' internal value '83'
> >>
> >> However we disable the error reporting for now. It should only be
> >> enabled when we decide to begin dropping duplicate error reporting.
> > 
> > Why don't we *enable* error reporting now, but only if the label
> > string is non-NULL. Then just change your second patch to pass
> > NULL everywhere.  We can then incrementally replace the NULL with
> > a real message at the same time as we update each caller to drop
> > the existing error reporting.
> > 
> > This avoids any point in time having double reporting.
> > 
> 
> Yes but this hits the case I mentioned in the cover letter: until the
> code base is converted it will be easier for new code to neglect to
> raise virReportError when the enum hasn't been converted yet, and IMO
> reporting no error is much worse than double reporting.
> 
> If you still think it's the way to go I'll rework it. But then maybe we
> need to think more about the strategy and timing of how we plan to
> convert the code, do we shoot for doing it in a single release window?

Given that the conversion work is essentially just a case of deleting
100's of calls to virReportError, it should be largely mechanical.
So its reasonable to do it all in one release cycle.

Assuming we get it done in a single cycle, I'm not concerned if we
have a few temporary mistakes where we don't report any error.

Regards,
Daniel
Cole Robinson April 11, 2019, 7:34 p.m. UTC | #4 
On 4/11/19 12:33 PM, Daniel P. Berrangé wrote:
> On Thu, Apr 11, 2019 at 12:27:56PM -0400, Cole Robinson wrote:
>> On 4/11/19 6:57 AM, Daniel P. Berrangé wrote:
>>> On Mon, Apr 08, 2019 at 11:48:18AM -0400, Cole Robinson wrote:
>>>> This allows us to raise error messages from virEnum*String functions.
>>>>
>>>> FromString failure will report this error for value 'zzzz'
>>>>
>>>>   invalid argument: Unknown 'domain type' value 'zzzz'
>>>>
>>>> ToString failure will report this error for unknown type=83
>>>>
>>>>   internal error: Unknown 'domain type' internal value '83'
>>>>
>>>> However we disable the error reporting for now. It should only be
>>>> enabled when we decide to begin dropping duplicate error reporting.
>>>
>>> Why don't we *enable* error reporting now, but only if the label
>>> string is non-NULL. Then just change your second patch to pass
>>> NULL everywhere.  We can then incrementally replace the NULL with
>>> a real message at the same time as we update each caller to drop
>>> the existing error reporting.
>>>
>>> This avoids any point in time having double reporting.
>>>
>>
>> Yes but this hits the case I mentioned in the cover letter: until the
>> code base is converted it will be easier for new code to neglect to
>> raise virReportError when the enum hasn't been converted yet, and IMO
>> reporting no error is much worse than double reporting.
>>
>> If you still think it's the way to go I'll rework it. But then maybe we
>> need to think more about the strategy and timing of how we plan to
>> convert the code, do we shoot for doing it in a single release window?
> 
> Given that the conversion work is essentially just a case of deleting
> 100's of calls to virReportError, it should be largely mechanical.
> So its reasonable to do it all in one release cycle.
> 
> Assuming we get it done in a single cycle, I'm not concerned if we
> have a few temporary mistakes where we don't report any error.
> 

Okay I'll send a v2 with the NULL bits

Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
diff mbox series

Patch

diff --git a/docs/apibuild.py b/docs/apibuild.py
index 9e04871220..1450355eeb 100755
--- a/docs/apibuild.py
+++ b/docs/apibuild.py
@@ -1404,6 +1404,18 @@  class CParser:
             self.error("parsing VIR_ENUM_IMPL: expecting ','", token)
         token = self.token()
 
+        # The 'label' field
+        if token[0] != "string":
+            self.error("parsing VIR_ENUM_IMPL: expecting string", token)
+        token = self.token()
+
+        if token[0] != "sep":
+            self.error("parsing VIR_ENUM_IMPL: expecting ','", token)
+
+        if token[1] != ',':
+            self.error("parsing VIR_ENUM_IMPL: expecting ','", token)
+        token = self.token()
+
         # Now the sentinel name
         if token[0] != "name":
             self.error("parsing VIR_ENUM_IMPL: expecting name", token)
diff --git a/src/util/virenum.c b/src/util/virenum.c
index 26093bd795..58968e7357 100644
--- a/src/util/virenum.c
+++ b/src/util/virenum.c
@@ -60,16 +60,23 @@  virTristateSwitchFromBool(bool val)
 int
 virEnumFromString(const char * const *types,
                   unsigned int ntypes,
-                  const char *type)
+                  const char *type,
+                  const char *label ATTRIBUTE_UNUSED)
 {
     size_t i;
     if (!type)
-        return -1;
+        goto error;
 
     for (i = 0; i < ntypes; i++)
         if (STREQ(types[i], type))
             return i;
 
+ error:
+    /* To be enabled at a later date
+     *
+     * virReportError(VIR_ERR_INVALID_ARG,
+     *              _("Unknown '%s' value '%s'"), label, NULLSTR(type));
+     */
     return -1;
 }
 
@@ -77,10 +84,17 @@  virEnumFromString(const char * const *types,
 const char *
 virEnumToString(const char * const *types,
                 unsigned int ntypes,
-                int type)
+                int type,
+                const char *label ATTRIBUTE_UNUSED)
 {
-    if (type < 0 || type >= ntypes)
+    if (type < 0 || type >= ntypes) {
+        /* To be enabled at a later date
+         *
+         * virReportError(VIR_ERR_INTERNAL_ERROR,
+         *                _("Unknown '%s' internal value %d"), label, type);
+         */
         return NULL;
+    }
 
     return types[type];
 }
diff --git a/src/util/virenum.h b/src/util/virenum.h
index 3ae1a70b72..2c68e7f6a0 100644
--- a/src/util/virenum.h
+++ b/src/util/virenum.h
@@ -24,24 +24,29 @@ 
 int
 virEnumFromString(const char * const *types,
                   unsigned int ntypes,
-                  const char *type);
+                  const char *type,
+                  const char *label);
 
 const char *
 virEnumToString(const char * const *types,
                 unsigned int ntypes,
-                int type);
+                int type,
+                const char *label);
 
-# define VIR_ENUM_IMPL(name, lastVal, ...) \
+# define VIR_ENUM_IMPL(name, label, lastVal, ...) \
+    static const char *name ## TypeLabel = label; \
     static const char *const name ## TypeList[] = { __VA_ARGS__ }; \
     const char *name ## TypeToString(int type) { \
         return virEnumToString(name ## TypeList, \
                                ARRAY_CARDINALITY(name ## TypeList), \
-                               type); \
+                               type, \
+                               name ## TypeLabel); \
     } \
     int name ## TypeFromString(const char *type) { \
         return virEnumFromString(name ## TypeList, \
                                  ARRAY_CARDINALITY(name ## TypeList), \
-                                 type); \
+                                 type, \
+                                 name ## TypeLabel); \
     } \
     verify(ARRAY_CARDINALITY(name ## TypeList) == lastVal)