diff mbox

object_del: Prevent removing an in-use memory backend object

Message ID 1426824898-9936-1-git-send-email-lma@suse.com
State New
Headers show

Commit Message

Lin Ma March 20, 2015, 4:14 a.m. UTC 
showing a memory device whose memdev is removed leads an assert:

(qemu) object_add memory-backend-ram,id=ram0,size=128M
(qemu) device_add pc-dimm,id=d0,memdev=ram0
(qemu) object_del ram0
(qemu) info memory-devices
**
ERROR:qom/object.c:1274:object_get_canonical_path_component:\
                            assertion failed: (obj->parent != NULL)
Aborted

The patch prevents removing an in-use mem backend and outputs an error msg.

Signed-off-by: Lin Ma <lma@suse.com>
---
 qmp.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Igor Mammedov March 20, 2015, 10:17 a.m. UTC | #1 
On Fri, 20 Mar 2015 12:14:58 +0800
Lin Ma <lma@suse.com> wrote:

> showing a memory device whose memdev is removed leads an assert:
> 
> (qemu) object_add memory-backend-ram,id=ram0,size=128M
> (qemu) device_add pc-dimm,id=d0,memdev=ram0
> (qemu) object_del ram0
> (qemu) info memory-devices
> **
> ERROR:qom/object.c:1274:object_get_canonical_path_component:\
>                             assertion failed: (obj->parent != NULL)
> Aborted
> 
> The patch prevents removing an in-use mem backend and outputs an error msg.
> 
> Signed-off-by: Lin Ma <lma@suse.com>
> ---
>  qmp.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/qmp.c b/qmp.c
> index c479e77..0086e2d 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -704,6 +704,7 @@ void qmp_object_del(const char *id, Error **errp)
>  {
>      Object *container;
>      Object *obj;
> +    const char *typename;
>  
>      container = container_get(object_get_root(), "/objects");
>      obj = object_resolve_path_component(container, id);
> @@ -711,6 +712,19 @@ void qmp_object_del(const char *id, Error **errp)
>          error_setg(errp, "object id not found");
>          return;
>      }
> +
> +    typename = object_class_get_name(object_class_get_parent(\
> +                                        object_get_class(OBJECT(obj))));
> +    if (strcmp(typename, TYPE_MEMORY_BACKEND) == 0 ) {
> +        MemoryRegion *mr;
> +        mr = host_memory_backend_get_memory(MEMORY_BACKEND(obj), errp);
> +        if (memory_region_is_mapped(mr)) {
> +            char *path = object_get_canonical_path_component(obj);
> +            error_setg(errp, "memdev %s is in used.", path);
> +            g_free(path);
> +            return;
> +        }
> +    }
How about making it more generic?
i.e
  if (!obj_class->can_be_deleted(obj))
     error out

>      object_unparent(obj);
>  }
>
Lin Ma March 23, 2015, 8:52 a.m. UTC | #2 
在 2015年03月20日 18:17, Igor Mammedov 写道:
> On Fri, 20 Mar 2015 12:14:58 +0800
> Lin Ma <lma@suse.com> wrote:
>
>> showing a memory device whose memdev is removed leads an assert:
>>
>> (qemu) object_add memory-backend-ram,id=ram0,size=128M
>> (qemu) device_add pc-dimm,id=d0,memdev=ram0
>> (qemu) object_del ram0
>> (qemu) info memory-devices
>> **
>> ERROR:qom/object.c:1274:object_get_canonical_path_component:\
>>                              assertion failed: (obj->parent != NULL)
>> Aborted
>>
>> The patch prevents removing an in-use mem backend and outputs an error msg.
>>
>> Signed-off-by: Lin Ma <lma@suse.com>
>> ---
>>   qmp.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/qmp.c b/qmp.c
>> index c479e77..0086e2d 100644
>> --- a/qmp.c
>> +++ b/qmp.c
>> @@ -704,6 +704,7 @@ void qmp_object_del(const char *id, Error **errp)
>>   {
>>       Object *container;
>>       Object *obj;
>> +    const char *typename;
>>   
>>       container = container_get(object_get_root(), "/objects");
>>       obj = object_resolve_path_component(container, id);
>> @@ -711,6 +712,19 @@ void qmp_object_del(const char *id, Error **errp)
>>           error_setg(errp, "object id not found");
>>           return;
>>       }
>> +
>> +    typename = object_class_get_name(object_class_get_parent(\
>> +                                        object_get_class(OBJECT(obj))));
>> +    if (strcmp(typename, TYPE_MEMORY_BACKEND) == 0 ) {
>> +        MemoryRegion *mr;
>> +        mr = host_memory_backend_get_memory(MEMORY_BACKEND(obj), errp);
>> +        if (memory_region_is_mapped(mr)) {
>> +            char *path = object_get_canonical_path_component(obj);
>> +            error_setg(errp, "memdev %s is in used.", path);
>> +            g_free(path);
>> +            return;
>> +        }
>> +    }
> How about making it more generic?
> i.e
>    if (!obj_class->can_be_deleted(obj))
>       error out
>
>>       object_unparent(obj);
>>   }
>>   
>
That makes sense, I couldn't agree more.
I am going to drop this patch and separate it to 2 patches. One for 
adding generic can_be_deleted
callback, The other for adding impl in hostmem-ram.c and hostmem-file.c

thanks,
Lin
Lin Ma March 23, 2015, 9:29 a.m. UTC | #3 
Igor,

It seems my smtp server has some issues, The email including my reply doesn't hit the list.
I paste my reply here and send it again:

That makes sense, I couldn't agree more.
I am going to drop this patch and separate it to 2 patches.
One for adding generic can_be_deleted callback, 
The other for adding the callback impl in hostmem-ram.c and hostmem-file.c

thanks,
Lin
>>> Igor Mammedov <imammedo@redhat.com> 2015-3-20 下午 18:18 >>>
On Fri, 20 Mar 2015 12:14:58 +0800
Lin Ma <lma@suse.com> wrote:

> showing a memory device whose memdev is removed leads an assert:
> 
> (qemu) object_add memory-backend-ram,id=ram0,size=128M
> (qemu) device_add pc-dimm,id=d0,memdev=ram0
> (qemu) object_del ram0
> (qemu) info memory-devices
> **
> ERROR:qom/object.c:1274:object_get_canonical_path_component:\
>                             assertion failed: (obj->parent != NULL)
> Aborted
> 
> The patch prevents removing an in-use mem backend and outputs an error msg.
> 
> Signed-off-by: Lin Ma <lma@suse.com>
> ---
>  qmp.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/qmp.c b/qmp.c
> index c479e77..0086e2d 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -704,6 +704,7 @@ void qmp_object_del(const char *id, Error **errp)
>  {
>      Object *container;
>      Object *obj;
> +    const char *typename;
>  
>      container = container_get(object_get_root(), "/objects");
>      obj = object_resolve_path_component(container, id);
> @@ -711,6 +712,19 @@ void qmp_object_del(const char *id, Error **errp)
>          error_setg(errp, "object id not found");
>          return;
>      }
> +
> +    typename = object_class_get_name(object_class_get_parent(\
> +                                        object_get_class(OBJECT(obj))));
> +    if (strcmp(typename, TYPE_MEMORY_BACKEND) == 0 ) {
> +        MemoryRegion *mr;
> +        mr = host_memory_backend_get_memory(MEMORY_BACKEND(obj), errp);
> +        if (memory_region_is_mapped(mr)) {
> +            char *path = object_get_canonical_path_component(obj);
> +            error_setg(errp, "memdev %s is in used.", path);
> +            g_free(path);
> +            return;
> +        }
> +    }
How about making it more generic?
i.e
  if (!obj_class->can_be_deleted(obj))
     error out

>      object_unparent(obj);
>  }
>
diff mbox

Patch

diff --git a/qmp.c b/qmp.c
index c479e77..0086e2d 100644
--- a/qmp.c
+++ b/qmp.c
@@ -704,6 +704,7 @@  void qmp_object_del(const char *id, Error **errp)
 {
     Object *container;
     Object *obj;
+    const char *typename;
 
     container = container_get(object_get_root(), "/objects");
     obj = object_resolve_path_component(container, id);
@@ -711,6 +712,19 @@  void qmp_object_del(const char *id, Error **errp)
         error_setg(errp, "object id not found");
         return;
     }
+
+    typename = object_class_get_name(object_class_get_parent(\
+                                        object_get_class(OBJECT(obj))));
+    if (strcmp(typename, TYPE_MEMORY_BACKEND) == 0 ) {
+        MemoryRegion *mr;
+        mr = host_memory_backend_get_memory(MEMORY_BACKEND(obj), errp);
+        if (memory_region_is_mapped(mr)) {
+            char *path = object_get_canonical_path_component(obj);
+            error_setg(errp, "memdev %s is in used.", path);
+            g_free(path);
+            return;
+        }
+    }
     object_unparent(obj);
 }