Re: [Qemu-block] [PATCH v3 3/7] qapi: Replace qobject_to_X(o) by qobject_to(o, X)

[Eric Blake]

On 02/28/2018 12:08 PM, Max Reitz wrote:


+    options = qobject_to(options_obj, QDict);


Bikeshedding - would it read any easier as:

options = qobject_to(QDict, options_obj);




So at this point, I'm 70:30 in favor of doing the rename to have
qobject_to(type, obj) for consistency with majority of other macros that
take a type name (type names are already unusual as arguments to macros,
whether or not the macro is named with ALL_CAPS).  (Sorry, I know that
means more busy work for you, if you agree with my reasoning)


I agree, because it means I have a decision. :-)


Are you planning on posting a v4, or shall I go ahead and make the swap 
as part of staging this on my QAPI tree?


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [PATCH v3 3/7] qapi: Replace qobject_to_X(o) by qobject_to(o, X)

[Max Reitz]

On 2018-02-27 15:47, Eric Blake wrote:
> On 02/26/2018 06:01 AM, Max Reitz wrote:
> 
 +++ b/block.c
 @@ -1457,7 +1457,7 @@ static QDict *parse_json_filename(const char
 *filename, Error **errp)
    return NULL;
    }
    -    options = qobject_to_qdict(options_obj);
 +    options = qobject_to(options_obj, QDict);
>>>
>>> Bikeshedding - would it read any easier as:
>>>
>>> options = qobject_to(QDict, options_obj);
>>>
>>> ?  If so, your Coccinelle script can be touched up, and patch 2/7 swaps
>>> argument order around, so it would be tolerable but still slightly
>>> busywork to regenerate the series.  But I'm not strongly attached to
>>> either order, and so I'm also willing to take this as-is (especially
>>> since that's less work), if no one else has a strong opinion that
>>> swapping order would aid legibility.
>>
>> Well, same for me. :-)
>>
>> In a template/generic language, we'd write the type first (e.g.
>> qobject_cast(options_obj)).  But maybe we'd write the object
>> first, too (e.g. options_obj.cast()).  And the current order of
>> the arguments follows the order in the name ("qobject" options_obj "to"
>> QDict).  But maybe it's more natural to read it as "qobject to" QDict
>> "applied to" options_obj.
>>
>> I don't know either.
> 
> Okay, after looking for existing uses of type names in macro calls, I see:
> 
> qemu/compiler.h:
> 
> #ifndef container_of
> #define container_of(ptr, type, member) ({  \
>     const typeof(((type *) 0)->member) *__mptr = (ptr); \
>     (type *) ((char *) __mptr - offsetof(type, member));})
> #endif
> 
> /* Convert from a base type to a parent type, with compile time
> checking.  */
> #ifdef __GNUC__
> #define DO_UPCAST(type, field, dev) ( __extension__ ( { \
>     char __attribute__((unused)) offset_must_be_zero[ \
>     -offsetof(type, field)]; \
>     container_of(dev, type, field);}))
> #else
> #define DO_UPCAST(type, field, dev) container_of(dev, type, field)
> #endif
> 
> #define typeof_field(type, field) typeof(((type *)0)->field)
> 
> 
> qapi/clone-visitor.h:
> 
> /*
>  * Deep-clone QAPI object @src of the given @type, and return the result.
>  *
>  * Not usable on QAPI scalars (integers, strings, enums), nor on a
>  * QAPI object that references the 'any' type.  Safe when @src is NULL.
>  */
> #define QAPI_CLONE(type, src)   \
> 
> /*
>  * Copy deep clones of @type members from @src to @dst.
>  *
>  * Not usable on QAPI scalars (integers, strings, enums), nor on a
>  * QAPI object that references the 'any' type.
>  */
> #define QAPI_CLONE_MEMBERS(type, dst, src)  \
> 
> 
> 2 out of 3 macros in compiler.h put the type name first, and
> container_of() puts it in the middle of 3.  It's even weirder because
> DO_UPCAST(t, f, d) calls container_of(d, t, f), where the inconsistency
> makes it a mental struggle to figure out how to read the two macros side
> by side, compared to if we had just been consistent.  Meanwhile, all of
> the macros in qapi put the type name first.
> 
> So at this point, I'm 70:30 in favor of doing the rename to have
> qobject_to(type, obj) for consistency with majority of other macros that
> take a type name (type names are already unusual as arguments to macros,
> whether or not the macro is named with ALL_CAPS).  (Sorry, I know that
> means more busy work for you, if you agree with my reasoning)

I agree, because it means I have a decision. :-)

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3 3/7] qapi: Replace qobject_to_X(o) by qobject_to(o, X)

[Eric Blake]

On 02/26/2018 06:01 AM, Max Reitz wrote:


+++ b/block.c
@@ -1457,7 +1457,7 @@ static QDict *parse_json_filename(const char
*filename, Error **errp)
   return NULL;
   }
   -    options = qobject_to_qdict(options_obj);
+    options = qobject_to(options_obj, QDict);


Bikeshedding - would it read any easier as:

options = qobject_to(QDict, options_obj);

?  If so, your Coccinelle script can be touched up, and patch 2/7 swaps
argument order around, so it would be tolerable but still slightly
busywork to regenerate the series.  But I'm not strongly attached to
either order, and so I'm also willing to take this as-is (especially
since that's less work), if no one else has a strong opinion that
swapping order would aid legibility.


Well, same for me. :-)

In a template/generic language, we'd write the type first (e.g.
qobject_cast(options_obj)).  But maybe we'd write the object
first, too (e.g. options_obj.cast()).  And the current order of
the arguments follows the order in the name ("qobject" options_obj "to"
QDict).  But maybe it's more natural to read it as "qobject to" QDict
"applied to" options_obj.

I don't know either.


Okay, after looking for existing uses of type names in macro calls, I see:

qemu/compiler.h:

#ifndef container_of
#define container_of(ptr, type, member) ({  \
const typeof(((type *) 0)->member) *__mptr = (ptr); \
(type *) ((char *) __mptr - offsetof(type, member));})
#endif

/* Convert from a base type to a parent type, with compile time 
checking.  */

#ifdef __GNUC__
#define DO_UPCAST(type, field, dev) ( __extension__ ( { \
char __attribute__((unused)) offset_must_be_zero[ \
-offsetof(type, field)]; \
container_of(dev, type, field);}))
#else
#define DO_UPCAST(type, field, dev) container_of(dev, type, field)
#endif

#define typeof_field(type, field) typeof(((type *)0)->field)


qapi/clone-visitor.h:

/*
 * Deep-clone QAPI object @src of the given @type, and return the result.
 *
 * Not usable on QAPI scalars (integers, strings, enums), nor on a
 * QAPI object that references the 'any' type.  Safe when @src is NULL.
 */
#define QAPI_CLONE(type, src)   \

/*
 * Copy deep clones of @type members from @src to @dst.
 *
 * Not usable on QAPI scalars (integers, strings, enums), nor on a
 * QAPI object that references the 'any' type.
 */
#define QAPI_CLONE_MEMBERS(type, dst, src)  \


2 out of 3 macros in compiler.h put the type name first, and 
container_of() puts it in the middle of 3.  It's even weirder because 
DO_UPCAST(t, f, d) calls container_of(d, t, f), where the inconsistency 
makes it a mental struggle to figure out how to read the two macros side 
by side, compared to if we had just been consistent.  Meanwhile, all of 
the macros in qapi put the type name first.


So at this point, I'm 70:30 in favor of doing the rename to have 
qobject_to(type, obj) for consistency with majority of other macros that 
take a type name (type names are already unusual as arguments to macros, 
whether or not the macro is named with ALL_CAPS).  (Sorry, I know that 
means more busy work for you, if you agree with my reasoning)


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [PATCH v3 3/7] qapi: Replace qobject_to_X(o) by qobject_to(o, X)

[Max Reitz]

On 2018-02-24 22:04, Eric Blake wrote:
> On 02/24/2018 09:40 AM, Max Reitz wrote:
>> This patch was generated using the following Coccinelle script:
>>
> 
>> and a bit of manual fix-up for overly long lines and three places in
>> tests/check-qjson.c that Coccinelle did not find.
>>
>> Signed-off-by: Max Reitz 
>> Reviewed-by: Alberto Garcia 
>> ---
> 
>> diff --git a/block.c b/block.c
>> index 814e5a02da..cb69fd7ae4 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1457,7 +1457,7 @@ static QDict *parse_json_filename(const char
>> *filename, Error **errp)
>>   return NULL;
>>   }
>>   -    options = qobject_to_qdict(options_obj);
>> +    options = qobject_to(options_obj, QDict);
> 
> Bikeshedding - would it read any easier as:
> 
> options = qobject_to(QDict, options_obj);
> 
> ?  If so, your Coccinelle script can be touched up, and patch 2/7 swaps
> argument order around, so it would be tolerable but still slightly
> busywork to regenerate the series.  But I'm not strongly attached to
> either order, and so I'm also willing to take this as-is (especially
> since that's less work), if no one else has a strong opinion that
> swapping order would aid legibility.

Well, same for me. :-)

In a template/generic language, we'd write the type first (e.g.
qobject_cast(options_obj)).  But maybe we'd write the object
first, too (e.g. options_obj.cast()).  And the current order of
the arguments follows the order in the name ("qobject" options_obj "to"
QDict).  But maybe it's more natural to read it as "qobject to" QDict
"applied to" options_obj.

I don't know either.

Max

> Reviewed-by: Eric Blake 
> 
> 
>> +++ b/block/rbd.c
>> @@ -256,14 +256,14 @@ static int qemu_rbd_set_keypairs(rados_t
>> cluster, const char *keypairs_json,
>>   if (!keypairs_json) {
>>   return ret;
>>   }
>> -    keypairs = qobject_to_qlist(qobject_from_json(keypairs_json,
>> -  _abort));
>> +    keypairs = qobject_to(qobject_from_json(keypairs_json,
>> _abort),
>> +  QList);
> 
> The question about legibility gets a bit more obvious when you span lines.
> 
>> @@ -893,8 +893,9 @@ static void simple_number(void)
>>   QNum *qnum;
>>   int64_t val;
>>   -    qnum =
>> qobject_to_qnum(qobject_from_json(test_cases[i].encoded,
>> - _abort));
>> +    qnum = qobject_to(qobject_from_json(test_cases[i].encoded,
>> +    _abort),
>> +  QNum);
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3 3/7] qapi: Replace qobject_to_X(o) by qobject_to(o, X)

[Eric Blake]

On 02/24/2018 09:40 AM, Max Reitz wrote:

This patch was generated using the following Coccinelle script:




and a bit of manual fix-up for overly long lines and three places in
tests/check-qjson.c that Coccinelle did not find.

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---



diff --git a/block.c b/block.c
index 814e5a02da..cb69fd7ae4 100644
--- a/block.c
+++ b/block.c
@@ -1457,7 +1457,7 @@ static QDict *parse_json_filename(const char *filename, 
Error **errp)
  return NULL;
  }
  
-options = qobject_to_qdict(options_obj);

+options = qobject_to(options_obj, QDict);


Bikeshedding - would it read any easier as:

options = qobject_to(QDict, options_obj);

?  If so, your Coccinelle script can be touched up, and patch 2/7 swaps 
argument order around, so it would be tolerable but still slightly 
busywork to regenerate the series.  But I'm not strongly attached to 
either order, and so I'm also willing to take this as-is (especially 
since that's less work), if no one else has a strong opinion that 
swapping order would aid legibility.


Reviewed-by: Eric Blake 



+++ b/block/rbd.c
@@ -256,14 +256,14 @@ static int qemu_rbd_set_keypairs(rados_t cluster, const 
char *keypairs_json,
  if (!keypairs_json) {
  return ret;
  }
-keypairs = qobject_to_qlist(qobject_from_json(keypairs_json,
-  _abort));
+keypairs = qobject_to(qobject_from_json(keypairs_json, _abort),
+  QList);


The question about legibility gets a bit more obvious when you span lines.


@@ -893,8 +893,9 @@ static void simple_number(void)
  QNum *qnum;
  int64_t val;
  
-qnum = qobject_to_qnum(qobject_from_json(test_cases[i].encoded,

- _abort));
+qnum = qobject_to(qobject_from_json(test_cases[i].encoded,
+_abort),
+  QNum);


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[Qemu-block] [PATCH v3 3/7] qapi: Replace qobject_to_X(o) by qobject_to(o, X)

[Max Reitz]

This patch was generated using the following Coccinelle script:

@@
expression Obj;
@@
(
- qobject_to_qnum(Obj)
+ qobject_to(Obj, QNum)
|
- qobject_to_qstring(Obj)
+ qobject_to(Obj, QString)
|
- qobject_to_qdict(Obj)
+ qobject_to(Obj, QDict)
|
- qobject_to_qlist(Obj)
+ qobject_to(Obj, QList)
|
- qobject_to_qbool(Obj)
+ qobject_to(Obj, QBool)
)

and a bit of manual fix-up for overly long lines and three places in
tests/check-qjson.c that Coccinelle did not find.

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 block.c |  2 +-
 block/qapi.c| 12 -
 block/rbd.c |  8 +++---
 blockdev.c  |  7 ++---
 hw/i386/acpi-build.c| 14 +-
 monitor.c   |  8 +++---
 qapi/qmp-dispatch.c |  2 +-
 qapi/qobject-input-visitor.c| 20 +++---
 qapi/qobject-output-visitor.c   |  4 +--
 qga/main.c  |  2 +-
 qmp.c   |  2 +-
 qobject/json-parser.c   |  2 +-
 qobject/qbool.c |  4 +--
 qobject/qdict.c | 38 +-
 qobject/qjson.c | 10 +++
 qobject/qlist.c |  6 ++---
 qobject/qlit.c  | 10 +++
 qobject/qnum.c  |  6 ++---
 qobject/qstring.c   |  6 ++---
 qom/object.c|  8 +++---
 target/i386/cpu.c   |  2 +-
 target/s390x/cpu_models.c   |  2 +-
 tests/check-qdict.c | 20 +++---
 tests/check-qjson.c | 41 ++--
 tests/check-qlist.c |  4 +--
 tests/check-qlit.c  |  2 +-
 tests/check-qnum.c  |  4 +--
 tests/check-qobject.c   |  2 +-
 tests/check-qstring.c   |  2 +-
 tests/device-introspect-test.c  | 14 +-
 tests/libqtest.c|  6 ++---
 tests/numa-test.c   |  8 +++---
 tests/qom-test.c|  4 +--
 tests/test-char.c   |  2 +-
 tests/test-keyval.c |  8 +++---
 tests/test-qga.c| 19 ++---
 tests/test-qmp-commands.c   | 12 -
 tests/test-qmp-event.c  | 16 +--
 tests/test-qobject-input-visitor.c  | 10 +++
 tests/test-qobject-output-visitor.c | 54 ++---
 tests/test-x86-cpuid-compat.c   | 17 ++--
 util/keyval.c   |  4 +--
 util/qemu-config.c  |  2 +-
 util/qemu-option.c  |  6 ++---
 44 files changed, 218 insertions(+), 214 deletions(-)

diff --git a/block.c b/block.c
index 814e5a02da..cb69fd7ae4 100644
--- a/block.c
+++ b/block.c
@@ -1457,7 +1457,7 @@ static QDict *parse_json_filename(const char *filename, 
Error **errp)
 return NULL;
 }
 
-options = qobject_to_qdict(options_obj);
+options = qobject_to(options_obj, QDict);
 if (!options) {
 qobject_decref(options_obj);
 error_setg(errp, "Invalid JSON object given");
diff --git a/block/qapi.c b/block/qapi.c
index 1fdeb1ef2f..0a3d4261df 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -647,29 +647,29 @@ static void dump_qobject(fprintf_function func_fprintf, 
void *f,
 {
 switch (qobject_type(obj)) {
 case QTYPE_QNUM: {
-QNum *value = qobject_to_qnum(obj);
+QNum *value = qobject_to(obj, QNum);
 char *tmp = qnum_to_string(value);
 func_fprintf(f, "%s", tmp);
 g_free(tmp);
 break;
 }
 case QTYPE_QSTRING: {
-QString *value = qobject_to_qstring(obj);
+QString *value = qobject_to(obj, QString);
 func_fprintf(f, "%s", qstring_get_str(value));
 break;
 }
 case QTYPE_QDICT: {
-QDict *value = qobject_to_qdict(obj);
+QDict *value = qobject_to(obj, QDict);
 dump_qdict(func_fprintf, f, comp_indent, value);
 break;
 }
 case QTYPE_QLIST: {
-QList *value = qobject_to_qlist(obj);
+QList *value = qobject_to(obj, QList);
 dump_qlist(func_fprintf, f, comp_indent, value);
 break;
 }
 case QTYPE_QBOOL: {
-QBool *value = qobject_to_qbool(obj);
+QBool *value = qobject_to(obj, QBool);
 func_fprintf(f, "%s", qbool_get_bool(value) ? "true" : "false");
 break;
 }
@@ -730,7 +730,7 @@ void bdrv_image_info_specific_dump(fprintf_function 
func_fprintf, void *f,
 
 visit_type_ImageInfoSpecific(v, NULL, _spec, _abort);
 visit_complete(v, );
-data = qdict_get(qobject_to_qdict(obj), "data");
+data = qdict_get(qobject_to(obj, QDict), "data");