[Qemu-devel] [PATCH for-2.5 0/3] spapr_drc: Last minute bug fixes

[Qemu-devel] [PATCH for-2.5 0/3] spapr_drc: Last minute bug fixes

[Markus Armbruster]

I hope we can get these reviewed and applied in time.

Markus Armbruster (3):
  spapr_drc: Handle visitor errors properly
  spapr_drc: Change value of property "fdt" from null back to {}
  spapr_drc: Make device "spapr-dr-connector" unavailable with -device

 hw/ppc/spapr_drc.c | 41 +++++++++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 6 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH for-2.5 1/3] spapr_drc: Handle visitor errors properly

[Markus Armbruster]

Since prop_get_fdt() is only used with QmpOutputVisitor, errors
shouldn't actually happen, so this is only a latent bug.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ppc/spapr_drc.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index f34bc04..4e7a1d3 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -254,6 +254,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, void *opaque,
                         const char *name, Error **errp)
 {
     sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj);
+    Error *err = NULL;
     int fdt_offset_next, fdt_offset, fdt_depth;
     void *fdt;
 
@@ -276,24 +277,43 @@ static void prop_get_fdt(Object *obj, Visitor *v, void *opaque,
         case FDT_BEGIN_NODE:
             fdt_depth++;
             name = fdt_get_name(fdt, fdt_offset, &name_len);
-            visit_start_struct(v, NULL, NULL, name, 0, NULL);
+            visit_start_struct(v, NULL, NULL, name, 0, &err);
+            if (err) {
+                error_propagate(errp, err);
+                return;
+            }
             break;
         case FDT_END_NODE:
             /* shouldn't ever see an FDT_END_NODE before FDT_BEGIN_NODE */
             g_assert(fdt_depth > 0);
-            visit_end_struct(v, NULL);
+            visit_end_struct(v, &err);
+            if (err) {
+                error_propagate(errp, err);
+                return;
+            }
             fdt_depth--;
             break;
         case FDT_PROP: {
             int i;
             prop = fdt_get_property_by_offset(fdt, fdt_offset, &prop_len);
             name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
-            visit_start_list(v, name, NULL);
+            visit_start_list(v, name, &err);
+            if (err) {
+                error_propagate(errp, err);
+                return;
+            }
             for (i = 0; i < prop_len; i++) {
-                visit_type_uint8(v, (uint8_t *)&prop->data[i], NULL, NULL);
-
+                visit_type_uint8(v, (uint8_t *)&prop->data[i], NULL, &err);
+                if (err) {
+                    error_propagate(errp, err);
+                    return;
+                }
+            }
+            visit_end_list(v, &err);
+            if (err) {
+                error_propagate(errp, err);
+                return;
             }
-            visit_end_list(v, NULL);
             break;
         }
         default:
-- 
2.4.3

[Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {}

[Markus Armbruster]

prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't
visit anything.  object_property_get_qobject() happily
object_property_get_qobject().  Amazingly, the latter survives the
misuse.  Turns out we've papered over it long before prop_get_fdt()
existed, in commit 1d10b44.

However, commit 6c2f9a1 changed how we paper over it, and as a side
effect changed qom-get's value from {} to null.  Change it right back
by fixing the visitor misuse.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ppc/spapr_drc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 4e7a1d3..dad157f 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -259,6 +259,11 @@ static void prop_get_fdt(Object *obj, Visitor *v, void *opaque,
     void *fdt;
 
     if (!drc->fdt) {
+        visit_start_struct(v, NULL, NULL, name, 0, &err);
+        if (!err) {
+            visit_end_struct(v, &err);
+        }
+        error_propagate(errp, err);
         return;
     }
 
-- 
2.4.3

[Qemu-devel] [PATCH for-2.5 3/3] spapr_drc: Make device "spapr-dr-connector" unavailable with -device

[Markus Armbruster]

It should only be created via spapr_dr_connector_new().  Attempting to
create it with -device crashes.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ppc/spapr_drc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index dad157f..8be62c3 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -599,6 +599,10 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
     drck->attach = attach;
     drck->detach = detach;
     drck->release_pending = release_pending;
+    /*
+     * Reason: it crashes FIXME find and document the real reason
+     */
+    dk->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo spapr_dr_connector_info = {
-- 
2.4.3

Re: [Qemu-devel] [PATCH for-2.5 1/3] spapr_drc: Handle visitor errors properly

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 848 bytes --]

On 12/03/2015 09:37 AM, Markus Armbruster wrote:
> Since prop_get_fdt() is only used with QmpOutputVisitor, errors
> shouldn't actually happen, so this is only a latent bug.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/ppc/spapr_drc.c | 32 ++++++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)

I posted a similar patch, but using &error_abort rather than
propagating, here:
https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg05788.html

But I like yours better.

> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index f34bc04..4e7a1d3 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {}

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1963 bytes --]

On 12/03/2015 09:37 AM, Markus Armbruster wrote:
> prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't
> visit anything.  object_property_get_qobject() happily
> object_property_get_qobject().  Amazingly, the latter survives the

Something got lost or otherwise corrupted in that sentence.  Were you
trying to say one function happily calls another?  If so, which of the
two "object_property_get_qobject()" strings should be changed, to what?

> misuse.  Turns out we've papered over it long before prop_get_fdt()
> existed, in commit 1d10b44.
> 
> However, commit 6c2f9a1 changed how we paper over it, and as a side
> effect changed qom-get's value from {} to null.  Change it right back
> by fixing the visitor misuse.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/ppc/spapr_drc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 4e7a1d3..dad157f 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -259,6 +259,11 @@ static void prop_get_fdt(Object *obj, Visitor *v, void *opaque,
>      void *fdt;
>  
>      if (!drc->fdt) {
> +        visit_start_struct(v, NULL, NULL, name, 0, &err);

visit_start_struct(v, NULL, "fdt", name, 0, &err)

would give a nicer error message, but only if there were an error
message to give.  As already mentioned on 1/3, we are only using this
for output visitors which don't raise errors (in fact, &error_abort
would do, rather than worrying about error propagation).

But I'm fine with the patch as you proposed it, once the commit message
is fixed.

Reviewed-by: Eric Blake <eblake@redhat.com>

> +        if (!err) {
> +            visit_end_struct(v, &err);
> +        }
> +        error_propagate(errp, err);
>          return;
>      }
>  
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.5 3/3] spapr_drc: Make device "spapr-dr-connector" unavailable with -device

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1135 bytes --]

On 12/03/2015 09:37 AM, Markus Armbruster wrote:
> It should only be created via spapr_dr_connector_new().  Attempting to
> create it with -device crashes.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/ppc/spapr_drc.c | 4 ++++
>  1 file changed, 4 insertions(+)

For 2.5, this is an appropriate last-minute crash-avoider.  The FIXME
reminds us to do better in 2.6.

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index dad157f..8be62c3 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -599,6 +599,10 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
>      drck->attach = attach;
>      drck->detach = detach;
>      drck->release_pending = release_pending;
> +    /*
> +     * Reason: it crashes FIXME find and document the real reason
> +     */
> +    dk->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo spapr_dr_connector_info = {
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {}

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 12/03/2015 09:37 AM, Markus Armbruster wrote:
>> prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't
>> visit anything.  object_property_get_qobject() happily
>> object_property_get_qobject().  Amazingly, the latter survives the
>
> Something got lost or otherwise corrupted in that sentence.  Were you
> trying to say one function happily calls another?  If so, which of the
> two "object_property_get_qobject()" strings should be changed, to what?

No idea what happened.  Correction: insert "calls" after "happily":

    prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't
    visit anything.  object_property_get_qobject() happily calls
    object_property_get_qobject().  Amazingly, the latter survives the
    misuse.  Turns out we've papered over it long before prop_get_fdt()
    existed, in commit 1d10b44.

>> misuse.  Turns out we've papered over it long before prop_get_fdt()
>> existed, in commit 1d10b44.
>> 
>> However, commit 6c2f9a1 changed how we paper over it, and as a side
>> effect changed qom-get's value from {} to null.  Change it right back
>> by fixing the visitor misuse.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/ppc/spapr_drc.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>> 
>> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
>> index 4e7a1d3..dad157f 100644
>> --- a/hw/ppc/spapr_drc.c
>> +++ b/hw/ppc/spapr_drc.c
>> @@ -259,6 +259,11 @@ static void prop_get_fdt(Object *obj, Visitor *v, void *opaque,
>>      void *fdt;
>>  
>>      if (!drc->fdt) {
>> +        visit_start_struct(v, NULL, NULL, name, 0, &err);
>
> visit_start_struct(v, NULL, "fdt", name, 0, &err)
>
> would give a nicer error message, but only if there were an error
> message to give.  As already mentioned on 1/3, we are only using this
> for output visitors which don't raise errors (in fact, &error_abort
> would do, rather than worrying about error propagation).
>
> But I'm fine with the patch as you proposed it, once the commit message
> is fixed.

Some callers pass a third argument, some don't.  We'll have to audit all
visitor users anyway.  We can decide whether and what to do about this
issue then.

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks for your quick review!

>> +        if (!err) {
>> +            visit_end_struct(v, &err);
>> +        }
>> +        error_propagate(errp, err);
>>          return;
>>      }
>>  
>> 

Re: [Qemu-devel] [PATCH for-2.5 3/3] spapr_drc: Make device "spapr-dr-connector" unavailable with -device

[Thomas Huth]

On 03/12/15 17:37, Markus Armbruster wrote:
> It should only be created via spapr_dr_connector_new().  Attempting to
> create it with -device crashes.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/ppc/spapr_drc.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index dad157f..8be62c3 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -599,6 +599,10 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
>      drck->attach = attach;
>      drck->detach = detach;
>      drck->release_pending = release_pending;
> +    /*
> +     * Reason: it crashes FIXME find and document the real reason
> +     */
> +    dk->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo spapr_dr_connector_info = {

That fix should be fine for 2.5.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {}

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1186 bytes --]

On 12/03/2015 10:13 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 12/03/2015 09:37 AM, Markus Armbruster wrote:
>>> prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't
>>> visit anything.  object_property_get_qobject() happily
>>> object_property_get_qobject().  Amazingly, the latter survives the
>>
>> Something got lost or otherwise corrupted in that sentence.  Were you
>> trying to say one function happily calls another?  If so, which of the
>> two "object_property_get_qobject()" strings should be changed, to what?
> 
> No idea what happened.  Correction: insert "calls" after "happily":
> 
>     prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't
>     visit anything.  object_property_get_qobject() happily calls
>     object_property_get_qobject().

That still reads "A() happily calls A()" - are we talking about
recursion here?  I still wonder if you meant a second function name,
and/or mention of the fact that we are calling a function with NULL
rather than a QObject?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {}

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 12/03/2015 10:13 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> On 12/03/2015 09:37 AM, Markus Armbruster wrote:
>>>> prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't
>>>> visit anything.  object_property_get_qobject() happily
>>>> object_property_get_qobject().  Amazingly, the latter survives the
>>>
>>> Something got lost or otherwise corrupted in that sentence.  Were you
>>> trying to say one function happily calls another?  If so, which of the
>>> two "object_property_get_qobject()" strings should be changed, to what?
>> 
>> No idea what happened.  Correction: insert "calls" after "happily":
>> 
>>     prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't
>>     visit anything.  object_property_get_qobject() happily calls
>>     object_property_get_qobject().
>>
> That still reads "A() happily calls A()" - are we talking about
> recursion here?  I still wonder if you meant a second function name,
> and/or mention of the fact that we are calling a function with NULL
> rather than a QObject?

I'm incapable of proof-reading anything I wrote myself %-}

    prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't
    visit anything.  object_property_get_qobject() happily calls
    qmp_output_get_qobject() then.  Amazingly, the latter survives the
    misuse.  Turns out we've papered over it long before prop_get_fdt()
    existed, in commit 1d10b44.

Re: [Qemu-devel] [PATCH for-2.5 0/3] spapr_drc: Last minute bug fixes

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> I hope we can get these reviewed and applied in time.

Peter, can you apply directly with 2/3's commit message fixed?  Let me
know if you'd prefer a pull request.

Re: [Qemu-devel] [PATCH for-2.5 1/3] spapr_drc: Handle visitor errors properly

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3043 bytes --]

On Thu, Dec 03, 2015 at 05:37:38PM +0100, Markus Armbruster wrote:
> Since prop_get_fdt() is only used with QmpOutputVisitor, errors
> shouldn't actually happen, so this is only a latent bug.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/spapr_drc.c | 32 ++++++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index f34bc04..4e7a1d3 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -254,6 +254,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, void *opaque,
>                          const char *name, Error **errp)
>  {
>      sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj);
> +    Error *err = NULL;
>      int fdt_offset_next, fdt_offset, fdt_depth;
>      void *fdt;
>  
> @@ -276,24 +277,43 @@ static void prop_get_fdt(Object *obj, Visitor *v, void *opaque,
>          case FDT_BEGIN_NODE:
>              fdt_depth++;
>              name = fdt_get_name(fdt, fdt_offset, &name_len);
> -            visit_start_struct(v, NULL, NULL, name, 0, NULL);
> +            visit_start_struct(v, NULL, NULL, name, 0, &err);
> +            if (err) {
> +                error_propagate(errp, err);
> +                return;
> +            }
>              break;
>          case FDT_END_NODE:
>              /* shouldn't ever see an FDT_END_NODE before FDT_BEGIN_NODE */
>              g_assert(fdt_depth > 0);
> -            visit_end_struct(v, NULL);
> +            visit_end_struct(v, &err);
> +            if (err) {
> +                error_propagate(errp, err);
> +                return;
> +            }
>              fdt_depth--;
>              break;
>          case FDT_PROP: {
>              int i;
>              prop = fdt_get_property_by_offset(fdt, fdt_offset, &prop_len);
>              name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
> -            visit_start_list(v, name, NULL);
> +            visit_start_list(v, name, &err);
> +            if (err) {
> +                error_propagate(errp, err);
> +                return;
> +            }
>              for (i = 0; i < prop_len; i++) {
> -                visit_type_uint8(v, (uint8_t *)&prop->data[i], NULL, NULL);
> -
> +                visit_type_uint8(v, (uint8_t *)&prop->data[i], NULL, &err);
> +                if (err) {
> +                    error_propagate(errp, err);
> +                    return;
> +                }
> +            }
> +            visit_end_list(v, &err);
> +            if (err) {
> +                error_propagate(errp, err);
> +                return;
>              }
> -            visit_end_list(v, NULL);
>              break;
>          }
>          default:

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {}

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1911 bytes --]

On Thu, Dec 03, 2015 at 05:37:39PM +0100, Markus Armbruster wrote:
> prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't
> visit anything.  object_property_get_qobject() happily
> object_property_get_qobject().  Amazingly, the latter survives the
> misuse.  Turns out we've papered over it long before prop_get_fdt()
> existed, in commit 1d10b44.
> 
> However, commit 6c2f9a1 changed how we paper over it, and as a side
> effect changed qom-get's value from {} to null.  Change it right back
> by fixing the visitor misuse.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/ppc/spapr_drc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 4e7a1d3..dad157f 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -259,6 +259,11 @@ static void prop_get_fdt(Object *obj, Visitor *v, void *opaque,
>      void *fdt;
>  
>      if (!drc->fdt) {
> +        visit_start_struct(v, NULL, NULL, name, 0, &err);
> +        if (!err) {
> +            visit_end_struct(v, &err);
> +        }
> +        error_propagate(errp, err);
>          return;
>      }
>  

I'm not entirely convinced by this.  IIUC, this makes the output in
the case of NULL (i.e. missing) fdt identical to the output in the
case of an empty, valid fdt - in dtc syntax, this:
	/ {
	};

Those are different cases from the point of view of the code which
actually uses the fdt, and for purposes of debugging it, I suspect we
want to expose that difference.

I don't know what the QOMishly correct way of doing that is, though.
Can we somehow make the "fdt" property disappear entirely if fdt is
NULL?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH for-2.5 3/3] spapr_drc: Make device "spapr-dr-connector" unavailable with -device

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1137 bytes --]

On Thu, Dec 03, 2015 at 05:37:40PM +0100, Markus Armbruster wrote:
> It should only be created via spapr_dr_connector_new().  Attempting to
> create it with -device crashes.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/spapr_drc.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index dad157f..8be62c3 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -599,6 +599,10 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
>      drck->attach = attach;
>      drck->detach = detach;
>      drck->release_pending = release_pending;
> +    /*
> +     * Reason: it crashes FIXME find and document the real reason
> +     */
> +    dk->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo spapr_dr_connector_info = {

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH for-2.5 0/3] spapr_drc: Last minute bug fixes

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 621 bytes --]

On Thu, Dec 03, 2015 at 07:15:15PM +0100, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > I hope we can get these reviewed and applied in time.
> 
> Peter, can you apply directly with 2/3's commit message fixed?  Let me
> know if you'd prefer a pull request.

I've applied 1 & 3 to my ppc-for-2.5 branch, if we want a pull req
from there.  2 I'm holding off for a reply to my query.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {}

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2426 bytes --]

On 12/03/2015 04:54 PM, David Gibson wrote:
> On Thu, Dec 03, 2015 at 05:37:39PM +0100, Markus Armbruster wrote:
>> prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't
>> visit anything.  object_property_get_qobject() happily
>> object_property_get_qobject().  Amazingly, the latter survives the
>> misuse.  Turns out we've papered over it long before prop_get_fdt()
>> existed, in commit 1d10b44.
>>
>> However, commit 6c2f9a1 changed how we paper over it, and as a side
>> effect changed qom-get's value from {} to null.  Change it right back
>> by fixing the visitor misuse.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/ppc/spapr_drc.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>

> I'm not entirely convinced by this.  IIUC, this makes the output in
> the case of NULL (i.e. missing) fdt identical to the output in the
> case of an empty, valid fdt - in dtc syntax, this:
> 	/ {
> 	};
> 
> Those are different cases from the point of view of the code which
> actually uses the fdt, and for purposes of debugging it, I suspect we
> want to expose that difference.

Expressing null may be the right thing, but it should be a conscious
decision, and not a side-effect of an unrelated patch.  This patch is
just about avoiding a regression for 2.5, because outputting {} for both
a missing fdt and an empty one was the behavior we had back in 2.4 (that
is, we've already returned {} in at least one release, so it won't hurt
to do it for one more).  For 2.6 we can revisit things to actually
express what is wanted.

> 
> I don't know what the QOMishly correct way of doing that is, though.
> Can we somehow make the "fdt" property disappear entirely if fdt is
> NULL?

In qapi terms, if a variable is marked optional and has_FOO is false,
then the variable disappears completely.  But I'm not sure if that maps
over to qom.  Maybe you do it by setting errp if drc->fdt is NULL, so
that prop_get_fdt() only succeeds when there is something for it to
return.  Or maybe returning qnull() is right after all, but in that
case, explicitly calling 'QObject *n = qnull(); visit_type_any(v, &n,
NULL, &err) seems like a nicer way than relying on side effects of how
the qmp output visitor behaves when nothing was visited.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {}

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2752 bytes --]

On Thu, Dec 03, 2015 at 07:41:59PM -0700, Eric Blake wrote:
> On 12/03/2015 04:54 PM, David Gibson wrote:
> > On Thu, Dec 03, 2015 at 05:37:39PM +0100, Markus Armbruster wrote:
> >> prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't
> >> visit anything.  object_property_get_qobject() happily
> >> object_property_get_qobject().  Amazingly, the latter survives the
> >> misuse.  Turns out we've papered over it long before prop_get_fdt()
> >> existed, in commit 1d10b44.
> >>
> >> However, commit 6c2f9a1 changed how we paper over it, and as a side
> >> effect changed qom-get's value from {} to null.  Change it right back
> >> by fixing the visitor misuse.
> >>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  hw/ppc/spapr_drc.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> 
> > I'm not entirely convinced by this.  IIUC, this makes the output in
> > the case of NULL (i.e. missing) fdt identical to the output in the
> > case of an empty, valid fdt - in dtc syntax, this:
> > 	/ {
> > 	};
> > 
> > Those are different cases from the point of view of the code which
> > actually uses the fdt, and for purposes of debugging it, I suspect we
> > want to expose that difference.
> 
> Expressing null may be the right thing, but it should be a conscious
> decision, and not a side-effect of an unrelated patch.  This patch is
> just about avoiding a regression for 2.5, because outputting {} for both
> a missing fdt and an empty one was the behavior we had back in 2.4 (that
> is, we've already returned {} in at least one release, so it won't hurt
> to do it for one more).  For 2.6 we can revisit things to actually
> express what is wanted.
> 
> > 
> > I don't know what the QOMishly correct way of doing that is, though.
> > Can we somehow make the "fdt" property disappear entirely if fdt is
> > NULL?
> 
> In qapi terms, if a variable is marked optional and has_FOO is false,
> then the variable disappears completely.  But I'm not sure if that maps
> over to qom.  Maybe you do it by setting errp if drc->fdt is NULL, so
> that prop_get_fdt() only succeeds when there is something for it to
> return.  Or maybe returning qnull() is right after all, but in that
> case, explicitly calling 'QObject *n = qnull(); visit_type_any(v, &n,
> NULL, &err) seems like a nicer way than relying on side effects of how
> the qmp output visitor behaves when nothing was visited.

Ok, thank makes sense.

Acked-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {}

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 12/03/2015 04:54 PM, David Gibson wrote:
>> On Thu, Dec 03, 2015 at 05:37:39PM +0100, Markus Armbruster wrote:
>>> prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't
>>> visit anything.  object_property_get_qobject() happily
>>> object_property_get_qobject().  Amazingly, the latter survives the
>>> misuse.  Turns out we've papered over it long before prop_get_fdt()
>>> existed, in commit 1d10b44.
>>>
>>> However, commit 6c2f9a1 changed how we paper over it, and as a side
>>> effect changed qom-get's value from {} to null.  Change it right back
>>> by fixing the visitor misuse.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  hw/ppc/spapr_drc.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>
>> I'm not entirely convinced by this.  IIUC, this makes the output in
>> the case of NULL (i.e. missing) fdt identical to the output in the
>> case of an empty, valid fdt - in dtc syntax, this:
>> 	/ {
>> 	};
>> 
>> Those are different cases from the point of view of the code which
>> actually uses the fdt, and for purposes of debugging it, I suspect we
>> want to expose that difference.
>
> Expressing null may be the right thing, but it should be a conscious
> decision, and not a side-effect of an unrelated patch.  This patch is
> just about avoiding a regression for 2.5, because outputting {} for both
> a missing fdt and an empty one was the behavior we had back in 2.4 (that
> is, we've already returned {} in at least one release, so it won't hurt
> to do it for one more).  For 2.6 we can revisit things to actually
> express what is wanted.

Yes.

>> I don't know what the QOMishly correct way of doing that is, though.
>> Can we somehow make the "fdt" property disappear entirely if fdt is
>> NULL?
>
> In qapi terms, if a variable is marked optional and has_FOO is false,
> then the variable disappears completely.

If I understand QOM correctly, we should be able to add the property
dynamically, so that it exists exactly when fdt is non-null.

>                                           But I'm not sure if that maps
> over to qom.  Maybe you do it by setting errp if drc->fdt is NULL, so
> that prop_get_fdt() only succeeds when there is something for it to
> return.

Works, but is it really an erroneous state or operation?  If not, a
special value seems more appropriate than an error.

>          Or maybe returning qnull() is right after all, but in that
> case, explicitly calling 'QObject *n = qnull(); visit_type_any(v, &n,
> NULL, &err) seems like a nicer way than relying on side effects of how
> the qmp output visitor behaves when nothing was visited.

I think we should have visit_none(), and attempting to retrieve a visits
value when you haven't visited anything should be an error.

Re: [Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {}

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 712 bytes --]

On 12/03/2015 10:55 AM, Markus Armbruster wrote:

> I'm incapable of proof-reading anything I wrote myself %-}

I know the feeling :)

> 
>     prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't
>     visit anything.  object_property_get_qobject() happily calls
>     qmp_output_get_qobject() then.  Amazingly, the latter survives the
>     misuse.  Turns out we've papered over it long before prop_get_fdt()
>     existed, in commit 1d10b44.

Oh well, it got pushed to master with original confusing spelling after
all (commit ab8bf1d7).  Not the end of the world :)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]