| Message ID | 1427105442-23484-1-git-send-email-lma@suse.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Mar 23, 2015 at 3:40 PM, Lin Ma <lma@suse.com> wrote: > Add can_be_deleted callback, If it is not null and returns false, > The qmp_object_del won't delete the given object. > > Signed-off-by: Lin Ma <lma@suse.com> > --- > include/qom/object.h | 12 ++++++++++++ > qmp.c | 7 +++++++ > qom/object.c | 12 ++++++++++++ > 3 files changed, 31 insertions(+) > > diff --git a/include/qom/object.h b/include/qom/object.h > index d2d7748..6e78cb0 100644 > --- a/include/qom/object.h > +++ b/include/qom/object.h > @@ -428,6 +428,8 @@ struct Object > * function. > * @abstract: If this field is true, then the class is considered abstract and > * cannot be directly instantiated. > + * @can_be_deleted: If this function returns true, then the object can be > + deleted safely. > * @class_size: The size of the class object (derivative of #ObjectClass) > * for this object. If @class_size is 0, then the size of the class will be > * assumed to be the size of the parent class. This allows a type to avoid > @@ -463,6 +465,8 @@ struct TypeInfo > bool abstract; > size_t class_size; > > + bool (*can_be_deleted)(Object *obj); > + I don't think TypeInfo is the right place for this. You can however define function hooks for Object in ObjectClass. See the unparent field of ObjectClass for a precedent. > void (*class_init)(ObjectClass *klass, void *data); > void (*class_base_init)(ObjectClass *klass, void *data); > void (*class_finalize)(ObjectClass *klass, void *data); > @@ -671,6 +675,14 @@ ObjectClass *object_get_class(Object *obj); > const char *object_get_typename(Object *obj); > > /** > + * object_can_be_deleted: > + * @obj: The object to obtain the deletion for. > + * > + * Returns: %true if @obj can be deleted safely, %false otherwise. > + */ > +bool object_can_be_deleted(Object *obj); > + > +/** > * type_register_static: > * @info: The #TypeInfo of the new type. > * > diff --git a/qmp.c b/qmp.c > index c479e77..dbbcb37 100644 > --- a/qmp.c > +++ b/qmp.c > @@ -711,6 +711,13 @@ void qmp_object_del(const char *id, Error **errp) > error_setg(errp, "object id not found"); > return; > } > + > + if (!object_can_be_deleted(obj)) { > + char *path = object_get_canonical_path_component(obj); > + error_setg(errp, "%s is in used, can not be deleted", path); > + g_free(path); > + return; > + } > object_unparent(obj); But is a better way to do this to add error handling to object_unparent API and override object_unparent for your device in question to throw the error? Then your change doesn't have to be limited to QMP. Regards, Peter > } > > diff --git a/qom/object.c b/qom/object.c > index d167038..dcec108 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -57,6 +57,8 @@ struct TypeImpl > > bool abstract; > > + bool (*can_be_deleted)(Object *obj); > + > const char *parent; > TypeImpl *parent_type; > > @@ -121,6 +123,8 @@ static TypeImpl *type_new(const TypeInfo *info) > > ti->abstract = info->abstract; > > + ti->can_be_deleted = info->can_be_deleted; > + > for (i = 0; info->interfaces && info->interfaces[i].type; i++) { > ti->interfaces[i].typename = g_strdup(info->interfaces[i].type); > } > @@ -584,6 +588,14 @@ const char *object_get_typename(Object *obj) > return obj->class->type->name; > } > > +bool object_can_be_deleted(Object *obj) > +{ > + if (obj->class->type->can_be_deleted) > + return obj->class->type->can_be_deleted(obj); > + else > + return true; > +} > + > ObjectClass *object_get_class(Object *obj) > { > return obj->class; > -- > 2.1.4 > >
On 23/03/2015 11:36, Peter Crosthwaite wrote: > I don't think TypeInfo is the right place for this. You can however > define function hooks for Object in ObjectClass. See the unparent > field of ObjectClass for a precedent. In this case, the right place could be UserCreatable. Alternatively... > But is a better way to do this to add error handling to > object_unparent API and override object_unparent for your device in > question to throw the error? Then your change doesn't have to be > limited to QMP. ... this is also a good choice. Paolo
Hi Lin, Am 23.03.2015 um 11:10 schrieb Lin Ma: > Add can_be_deleted callback, If it is not null and returns false, > The qmp_object_del won't delete the given object. > > Signed-off-by: Lin Ma <lma@suse.com> > --- > include/qom/object.h | 12 ++++++++++++ > qmp.c | 7 +++++++ > qom/object.c | 12 ++++++++++++ > 3 files changed, 31 insertions(+) Why am I not being CC'ed here as QOM maintainer? You can use scripts/get_maintainer.pl to assist you, or read MAINTAINERS file. Also, when sending more than one patch, always include a cover letter for grouping. It further seems that this is the second version already, so you should've used --subject-prefix="PATCH v2" and included a change log in the cover letter. Cf. http://wiki.qemu-project.org/Contribute/SubmitAPatch Regards, Andreas
Hi, For consistency in git-log, please use "qom:" rather than "object:". Am 23.03.2015 um 13:06 schrieb Paolo Bonzini: > On 23/03/2015 11:36, Peter Crosthwaite wrote: >> I don't think TypeInfo is the right place for this. You can however >> define function hooks for Object in ObjectClass. See the unparent >> field of ObjectClass for a precedent. Agree. > In this case, the right place could be UserCreatable. Maybe, not so familiar with that interface myself. Does object_del allow to delete non-UserCreatable objects? Then it wouldn't help much. > Alternatively... > >> But is a better way to do this to add error handling to >> object_unparent API and override object_unparent for your device in >> question to throw the error? Then your change doesn't have to be >> limited to QMP. > > ... this is also a good choice. Well, I have doubts about asking someone who's not ultimately familiar with that code to refactor the API. For instance, we wouldn't want QEMU on shutdown or in error cases refusing to unparent some object. Doing it at QMP level (ObjectClass/UserCreatable) seems safer, given that Chun Yan's trivial block option fix ended up respinning a QemuOpts refactoring some twenty times before it got merged. Regards, Andreas
On Mon, 23 Mar 2015 14:13:07 +0100 Andreas Färber <afaerber@suse.de> wrote: > Hi, > > For consistency in git-log, please use "qom:" rather than "object:". > > Am 23.03.2015 um 13:06 schrieb Paolo Bonzini: > > On 23/03/2015 11:36, Peter Crosthwaite wrote: > >> I don't think TypeInfo is the right place for this. You can however > >> define function hooks for Object in ObjectClass. See the unparent > >> field of ObjectClass for a precedent. > > Agree. > > > In this case, the right place could be UserCreatable. > > Maybe, not so familiar with that interface myself. Does object_del allow > to delete non-UserCreatable objects? Then it wouldn't help much. object_del() works only with /objects children, and so far the only way that child placed there is via object_add() which requires a new object to have TYPE_USER_CREATABLE interface. I'd go this way rather than overhaul object_unparent() in this case. > > > Alternatively... > > > >> But is a better way to do this to add error handling to > >> object_unparent API and override object_unparent for your device in > >> question to throw the error? Then your change doesn't have to be > >> limited to QMP. > > > > ... this is also a good choice. > > Well, I have doubts about asking someone who's not ultimately familiar > with that code to refactor the API. For instance, we wouldn't want QEMU > on shutdown or in error cases refusing to unparent some object. > > Doing it at QMP level (ObjectClass/UserCreatable) seems safer, given > that Chun Yan's trivial block option fix ended up respinning a QemuOpts > refactoring some twenty times before it got merged. > > Regards, > Andreas >
在 2015年03月23日 20:52, Andreas Färber 写道: > Hi Lin, > > Am 23.03.2015 um 11:10 schrieb Lin Ma: >> Add can_be_deleted callback, If it is not null and returns false, >> The qmp_object_del won't delete the given object. >> >> Signed-off-by: Lin Ma <lma@suse.com> >> --- >> include/qom/object.h | 12 ++++++++++++ >> qmp.c | 7 +++++++ >> qom/object.c | 12 ++++++++++++ >> 3 files changed, 31 insertions(+) > Why am I not being CC'ed here as QOM maintainer? You can use > scripts/get_maintainer.pl to assist you, or read MAINTAINERS file. Really sorry about that. > > Also, when sending more than one patch, always include a cover letter > for grouping. Thanks for reminding. > It further seems that this is the second version already, so you > should've used --subject-prefix="PATCH v2" and included a change log in > the cover letter. > > Cf. http://wiki.qemu-project.org/Contribute/SubmitAPatch > > Regards, > Andreas > Because the patch v1 is a single patch and not generic, So I dropped it. That's why I didn't used --subject-prefix here. But I'll add all the info & format that you mentioned in patch v3. Thanks, Lin
在 2015年03月23日 20:06, Paolo Bonzini 写道: > > On 23/03/2015 11:36, Peter Crosthwaite wrote: >> I don't think TypeInfo is the right place for this. You can however >> define function hooks for Object in ObjectClass. See the unparent >> field of ObjectClass for a precedent. > In this case, the right place could be UserCreatable. Alternatively... Well... Because I'm not familiar with qom very much, Let me dig more details about UserCreatable first :-) > >> But is a better way to do this to add error handling to >> object_unparent API and override object_unparent for your device in >> question to throw the error? Then your change doesn't have to be >> limited to QMP. > ... this is also a good choice. > > Paolo >
在 2015年03月23日 21:30, Igor Mammedov 写道: > On Mon, 23 Mar 2015 14:13:07 +0100 > Andreas Färber <afaerber@suse.de> wrote: > >> Hi, >> >> For consistency in git-log, please use "qom:" rather than "object:". >> >> Am 23.03.2015 um 13:06 schrieb Paolo Bonzini: >>> On 23/03/2015 11:36, Peter Crosthwaite wrote: >>>> I don't think TypeInfo is the right place for this. You can however >>>> define function hooks for Object in ObjectClass. See the unparent >>>> field of ObjectClass for a precedent. >> Agree. >> >>> In this case, the right place could be UserCreatable. >> Maybe, not so familiar with that interface myself. Does object_del allow >> to delete non-UserCreatable objects? Then it wouldn't help much. > object_del() works only with /objects children, and so far the only way > that child placed there is via object_add() which requires a new object > to have TYPE_USER_CREATABLE interface. > I'd go this way rather than overhaul object_unparent() in this case. What about these changes? 1. Add a callback function named 'ObjectCanBeDeleted *can_be_deleted' to struct ObjectClass in object.h 2. Call the function in qmp_object_del of qmp.c, says: ObjectClass *obj_class = object_get_class(obj); if (obj_class->can_be_deleted) if (!obj_class->can_be_deleted(obj)) error out 3. Then implement can_be_deleted callback in backends, says: static bool host_memory_backend_can_be_deleted(Object *obj) {......} static void host_memory_backend_class_init(ObjectClass *oc, void *data) { ...... oc->can_be_deleted = host_memory_backend_can_be_deleted; } > >>> Alternatively... >>> >>>> But is a better way to do this to add error handling to >>>> object_unparent API and override object_unparent for your device in >>>> question to throw the error? Then your change doesn't have to be >>>> limited to QMP. >>> ... this is also a good choice. >> Well, I have doubts about asking someone who's not ultimately familiar >> with that code to refactor the API. For instance, we wouldn't want QEMU >> on shutdown or in error cases refusing to unparent some object. >> >> Doing it at QMP level (ObjectClass/UserCreatable) seems safer, given >> that Chun Yan's trivial block option fix ended up respinning a QemuOpts >> refactoring some twenty times before it got merged. >> >> Regards, >> Andreas >> > >
On Wed, 25 Mar 2015 23:47:46 +0800 Lin Ma <lma@suse.com> wrote: > > 在 2015年03月23日 21:30, Igor Mammedov 写道: > > On Mon, 23 Mar 2015 14:13:07 +0100 > > Andreas Färber <afaerber@suse.de> wrote: > > > >> Hi, > >> > >> For consistency in git-log, please use "qom:" rather than "object:". > >> > >> Am 23.03.2015 um 13:06 schrieb Paolo Bonzini: > >>> On 23/03/2015 11:36, Peter Crosthwaite wrote: > >>>> I don't think TypeInfo is the right place for this. You can however > >>>> define function hooks for Object in ObjectClass. See the unparent > >>>> field of ObjectClass for a precedent. > >> Agree. > >> > >>> In this case, the right place could be UserCreatable. > >> Maybe, not so familiar with that interface myself. Does object_del allow > >> to delete non-UserCreatable objects? Then it wouldn't help much. > > object_del() works only with /objects children, and so far the only way > > that child placed there is via object_add() which requires a new object > > to have TYPE_USER_CREATABLE interface. > > I'd go this way rather than overhaul object_unparent() in this case. > What about these changes? > 1. Add a callback function named 'ObjectCanBeDeleted *can_be_deleted' to > struct ObjectClass in object.h > 2. Call the function in qmp_object_del of qmp.c, says: > ObjectClass *obj_class = object_get_class(obj); > if (obj_class->can_be_deleted) > if (!obj_class->can_be_deleted(obj)) > error out > > 3. Then implement can_be_deleted callback in backends, says: > static bool host_memory_backend_can_be_deleted(Object *obj) {......} > > static void host_memory_backend_class_init(ObjectClass *oc, void > *data) { > ...... > oc->can_be_deleted = host_memory_backend_can_be_deleted; > } all backends currently implement TYPE_USER_CREATABLE interface, so I'd rather extend UserCreatableClass with: bool can_be_deleted(UserCreatable *uc) callback, which backends could implement if needed. > > > >>> Alternatively... > >>> > >>>> But is a better way to do this to add error handling to > >>>> object_unparent API and override object_unparent for your device in > >>>> question to throw the error? Then your change doesn't have to be > >>>> limited to QMP. > >>> ... this is also a good choice. > >> Well, I have doubts about asking someone who's not ultimately familiar > >> with that code to refactor the API. For instance, we wouldn't want QEMU > >> on shutdown or in error cases refusing to unparent some object. > >> > >> Doing it at QMP level (ObjectClass/UserCreatable) seems safer, given > >> that Chun Yan's trivial block option fix ended up respinning a QemuOpts > >> refactoring some twenty times before it got merged. > >> > >> Regards, > >> Andreas > >> > > > > >
Am 26.03.2015 um 11:05 schrieb Igor Mammedov: > On Wed, 25 Mar 2015 23:47:46 +0800 > Lin Ma <lma@suse.com> wrote: >> 在 2015年03月23日 21:30, Igor Mammedov 写道: >>> On Mon, 23 Mar 2015 14:13:07 +0100 >>> Andreas Färber <afaerber@suse.de> wrote: >>>> Am 23.03.2015 um 13:06 schrieb Paolo Bonzini: >>>>> On 23/03/2015 11:36, Peter Crosthwaite wrote: >>>>>> I don't think TypeInfo is the right place for this. You can however >>>>>> define function hooks for Object in ObjectClass. See the unparent >>>>>> field of ObjectClass for a precedent. >>>> Agree. >>>> >>>>> In this case, the right place could be UserCreatable. >>>> Maybe, not so familiar with that interface myself. Does object_del allow >>>> to delete non-UserCreatable objects? Then it wouldn't help much. >>> object_del() works only with /objects children, and so far the only way >>> that child placed there is via object_add() which requires a new object >>> to have TYPE_USER_CREATABLE interface. >>> I'd go this way rather than overhaul object_unparent() in this case. >> What about these changes? >> 1. Add a callback function named 'ObjectCanBeDeleted *can_be_deleted' to >> struct ObjectClass in object.h >> 2. Call the function in qmp_object_del of qmp.c, says: >> ObjectClass *obj_class = object_get_class(obj); >> if (obj_class->can_be_deleted) >> if (!obj_class->can_be_deleted(obj)) >> error out >> >> 3. Then implement can_be_deleted callback in backends, says: >> static bool host_memory_backend_can_be_deleted(Object *obj) {......} >> >> static void host_memory_backend_class_init(ObjectClass *oc, void >> *data) { >> ...... >> oc->can_be_deleted = host_memory_backend_can_be_deleted; >> } > > all backends currently implement TYPE_USER_CREATABLE interface, > so I'd rather extend UserCreatableClass with: > > bool can_be_deleted(UserCreatable *uc) > > callback, which backends could implement if needed. As for the implementation, could we simply look at Object::ref field? Regards, Andreas
On Thu, 26 Mar 2015 11:07:27 +0100 Andreas Färber <afaerber@suse.de> wrote: > Am 26.03.2015 um 11:05 schrieb Igor Mammedov: > > On Wed, 25 Mar 2015 23:47:46 +0800 > > Lin Ma <lma@suse.com> wrote: > >> 在 2015年03月23日 21:30, Igor Mammedov 写道: > >>> On Mon, 23 Mar 2015 14:13:07 +0100 > >>> Andreas Färber <afaerber@suse.de> wrote: > >>>> Am 23.03.2015 um 13:06 schrieb Paolo Bonzini: > >>>>> On 23/03/2015 11:36, Peter Crosthwaite wrote: > >>>>>> I don't think TypeInfo is the right place for this. You can however > >>>>>> define function hooks for Object in ObjectClass. See the unparent > >>>>>> field of ObjectClass for a precedent. > >>>> Agree. > >>>> > >>>>> In this case, the right place could be UserCreatable. > >>>> Maybe, not so familiar with that interface myself. Does object_del allow > >>>> to delete non-UserCreatable objects? Then it wouldn't help much. > >>> object_del() works only with /objects children, and so far the only way > >>> that child placed there is via object_add() which requires a new object > >>> to have TYPE_USER_CREATABLE interface. > >>> I'd go this way rather than overhaul object_unparent() in this case. > >> What about these changes? > >> 1. Add a callback function named 'ObjectCanBeDeleted *can_be_deleted' to > >> struct ObjectClass in object.h > >> 2. Call the function in qmp_object_del of qmp.c, says: > >> ObjectClass *obj_class = object_get_class(obj); > >> if (obj_class->can_be_deleted) > >> if (!obj_class->can_be_deleted(obj)) > >> error out > >> > >> 3. Then implement can_be_deleted callback in backends, says: > >> static bool host_memory_backend_can_be_deleted(Object *obj) {......} > >> > >> static void host_memory_backend_class_init(ObjectClass *oc, void > >> *data) { > >> ...... > >> oc->can_be_deleted = host_memory_backend_can_be_deleted; > >> } > > > > all backends currently implement TYPE_USER_CREATABLE interface, > > so I'd rather extend UserCreatableClass with: > > > > bool can_be_deleted(UserCreatable *uc) > > > > callback, which backends could implement if needed. > > As for the implementation, could we simply look at Object::ref field? What value of ref, one would use to decide if deletion is possible? In generic case object can have ref > 1 but still be eligible for deleting via object-del. > > Regards, > Andreas >
On 26/03/2015 11:29, Igor Mammedov wrote: > What value of ref, one would use to decide if deletion is possible? > > In generic case object can have ref > 1 but still be eligible for deleting > via object-del. Right, for example devices are unparented with ref > 1. Still, it could be a sane default implementation. It doesn't even need a function pointer until someone comes up with an object that has different needs. Paolo
On Thu, 26 Mar 2015 14:37:13 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 26/03/2015 11:29, Igor Mammedov wrote: > > What value of ref, one would use to decide if deletion is possible? > > > > In generic case object can have ref > 1 but still be eligible for deleting > > via object-del. > > Right, for example devices are unparented with ref > 1. > > Still, it could be a sane default implementation. It doesn't even need > a function pointer until someone comes up with an object that has > different needs. Not in case of memory backend, its 'ref' in unused state is 2 since it contains at least one child, backend object is owner of MemoryRegion. > Paolo
diff --git a/include/qom/object.h b/include/qom/object.h index d2d7748..6e78cb0 100644 --- a/include/qom/object.h +++ b/include/qom/object.h @@ -428,6 +428,8 @@ struct Object * function. * @abstract: If this field is true, then the class is considered abstract and * cannot be directly instantiated. + * @can_be_deleted: If this function returns true, then the object can be + deleted safely. * @class_size: The size of the class object (derivative of #ObjectClass) * for this object. If @class_size is 0, then the size of the class will be * assumed to be the size of the parent class. This allows a type to avoid @@ -463,6 +465,8 @@ struct TypeInfo bool abstract; size_t class_size; + bool (*can_be_deleted)(Object *obj); + void (*class_init)(ObjectClass *klass, void *data); void (*class_base_init)(ObjectClass *klass, void *data); void (*class_finalize)(ObjectClass *klass, void *data); @@ -671,6 +675,14 @@ ObjectClass *object_get_class(Object *obj); const char *object_get_typename(Object *obj); /** + * object_can_be_deleted: + * @obj: The object to obtain the deletion for. + * + * Returns: %true if @obj can be deleted safely, %false otherwise. + */ +bool object_can_be_deleted(Object *obj); + +/** * type_register_static: * @info: The #TypeInfo of the new type. * diff --git a/qmp.c b/qmp.c index c479e77..dbbcb37 100644 --- a/qmp.c +++ b/qmp.c @@ -711,6 +711,13 @@ void qmp_object_del(const char *id, Error **errp) error_setg(errp, "object id not found"); return; } + + if (!object_can_be_deleted(obj)) { + char *path = object_get_canonical_path_component(obj); + error_setg(errp, "%s is in used, can not be deleted", path); + g_free(path); + return; + } object_unparent(obj); } diff --git a/qom/object.c b/qom/object.c index d167038..dcec108 100644 --- a/qom/object.c +++ b/qom/object.c @@ -57,6 +57,8 @@ struct TypeImpl bool abstract; + bool (*can_be_deleted)(Object *obj); + const char *parent; TypeImpl *parent_type; @@ -121,6 +123,8 @@ static TypeImpl *type_new(const TypeInfo *info) ti->abstract = info->abstract; + ti->can_be_deleted = info->can_be_deleted; + for (i = 0; info->interfaces && info->interfaces[i].type; i++) { ti->interfaces[i].typename = g_strdup(info->interfaces[i].type); } @@ -584,6 +588,14 @@ const char *object_get_typename(Object *obj) return obj->class->type->name; } +bool object_can_be_deleted(Object *obj) +{ + if (obj->class->type->can_be_deleted) + return obj->class->type->can_be_deleted(obj); + else + return true; +} + ObjectClass *object_get_class(Object *obj) { return obj->class;
Add can_be_deleted callback, If it is not null and returns false, The qmp_object_del won't delete the given object. Signed-off-by: Lin Ma <lma@suse.com> --- include/qom/object.h | 12 ++++++++++++ qmp.c | 7 +++++++ qom/object.c | 12 ++++++++++++ 3 files changed, 31 insertions(+)