| Message ID | 20180611121655.19616-7-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | pc-dimm: next bunch of cleanups | expand |
On Mon, Jun 11, 2018 at 02:16:50PM +0200, David Hildenbrand wrote: > "size" should not be queried before the device was realized. Let' make > that explicit. > > Signed-off-by: David Hildenbrand <david@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/mem/pc-dimm.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c > index 86fbcf2d0c..5294734529 100644 > --- a/hw/mem/pc-dimm.c > +++ b/hw/mem/pc-dimm.c > @@ -166,6 +166,12 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name, > PCDIMMDevice *dimm = PC_DIMM(obj); > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj); > > + if (!DEVICE(obj)->realized) { > + error_setg(errp, "Property \"%s\" not accessible before realized", > + name); > + return; > + } > + > mr = ddc->get_memory_region(dimm, errp); > if (!mr) { > return;
On Mon, 11 Jun 2018 14:16:50 +0200 David Hildenbrand <david@redhat.com> wrote: > "size" should not be queried before the device was realized. Let' make > that explicit. > > Signed-off-by: David Hildenbrand <david@redhat.com> It's generic property getter, it should work even before realize is called. if issues described in 5/11 are properly fixed, this patch won't be needed. So drop this patch > --- > hw/mem/pc-dimm.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c > index 86fbcf2d0c..5294734529 100644 > --- a/hw/mem/pc-dimm.c > +++ b/hw/mem/pc-dimm.c > @@ -166,6 +166,12 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name, > PCDIMMDevice *dimm = PC_DIMM(obj); > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj); > > + if (!DEVICE(obj)->realized) { > + error_setg(errp, "Property \"%s\" not accessible before realized", > + name); > + return; > + } > + > mr = ddc->get_memory_region(dimm, errp); > if (!mr) { > return;
On 13.06.2018 14:56, Igor Mammedov wrote: > On Mon, 11 Jun 2018 14:16:50 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> "size" should not be queried before the device was realized. Let' make >> that explicit. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> > It's generic property getter, it should work even before realize is called. That's the point, as we can see in NVDIMM code , it is *not* a generic property getter. > > if issues described in 5/11 are properly fixed, this patch won't be needed. > > So drop this patch > > >> --- >> hw/mem/pc-dimm.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c >> index 86fbcf2d0c..5294734529 100644 >> --- a/hw/mem/pc-dimm.c >> +++ b/hw/mem/pc-dimm.c >> @@ -166,6 +166,12 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name, >> PCDIMMDevice *dimm = PC_DIMM(obj); >> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj); >> >> + if (!DEVICE(obj)->realized) { >> + error_setg(errp, "Property \"%s\" not accessible before realized", >> + name); >> + return; >> + } >> + >> mr = ddc->get_memory_region(dimm, errp); >> if (!mr) { >> return; >
On Wed, Jun 13, 2018 at 04:03:35PM +0200, David Hildenbrand wrote: > On 13.06.2018 14:56, Igor Mammedov wrote: > > On Mon, 11 Jun 2018 14:16:50 +0200 > > David Hildenbrand <david@redhat.com> wrote: > > > >> "size" should not be queried before the device was realized. Let' make > >> that explicit. > >> > >> Signed-off-by: David Hildenbrand <david@redhat.com> > > It's generic property getter, it should work even before realize is called. > > That's the point, as we can see in NVDIMM code , it is *not* a generic > property getter. If it were a writeable property, I would agree with Igor because it would make it impossible to find out what's the default value for the property. As it's a read-only property, returning an error seems harmless. Igor, why exactly do you think it's a problem?
On Wed, 13 Jun 2018 18:33:37 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, Jun 13, 2018 at 04:03:35PM +0200, David Hildenbrand wrote: > > On 13.06.2018 14:56, Igor Mammedov wrote: > > > On Mon, 11 Jun 2018 14:16:50 +0200 > > > David Hildenbrand <david@redhat.com> wrote: > > > > > >> "size" should not be queried before the device was realized. Let' make > > >> that explicit. > > >> > > >> Signed-off-by: David Hildenbrand <david@redhat.com> > > > It's generic property getter, it should work even before realize is called. > > > > That's the point, as we can see in NVDIMM code , it is *not* a generic > > property getter. > > If it were a writeable property, I would agree with Igor because > it would make it impossible to find out what's the default value > for the property. > > As it's a read-only property, returning an error seems harmless. > Igor, why exactly do you think it's a problem? I'm not welcoming putting random restrictions on when property could be read. For me it's just another qom property and it should return valid value that corresponds to the current state of the object or error out if it can't return anything useful at the moment. In this particular case it should error out if 'memdev' isn't set and return a value corresponding to current state of [pc|nv]dimm object if 'memdev' is set. This patch is obviously wrong, putting restriction based on "realized" as getter might be easily accessed before Device::realized becomes True, i.e. during setting properties or within device_set_realized() time.
On Wed, 13 Jun 2018 16:03:35 +0200 David Hildenbrand <david@redhat.com> wrote: > On 13.06.2018 14:56, Igor Mammedov wrote: > > On Mon, 11 Jun 2018 14:16:50 +0200 > > David Hildenbrand <david@redhat.com> wrote: > > > >> "size" should not be queried before the device was realized. Let' make > >> that explicit. > >> > >> Signed-off-by: David Hildenbrand <david@redhat.com> > > It's generic property getter, it should work even before realize is called. > > That's the point, as we can see in NVDIMM code , it is *not* a generic > property getter. it is added by object_property_add(obj, PC_DIMM_SIZE_PROP, "uint64", pc_dimm_get_size, NULL, NULL, NULL, &error_abort); hence it's generic QOM property as opposed to memory device class specific callbacks. > > > > > if issues described in 5/11 are properly fixed, this patch won't be needed. > > > > So drop this patch > > > > > >> --- > >> hw/mem/pc-dimm.c | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c > >> index 86fbcf2d0c..5294734529 100644 > >> --- a/hw/mem/pc-dimm.c > >> +++ b/hw/mem/pc-dimm.c > >> @@ -166,6 +166,12 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name, > >> PCDIMMDevice *dimm = PC_DIMM(obj); > >> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj); > >> > >> + if (!DEVICE(obj)->realized) { > >> + error_setg(errp, "Property \"%s\" not accessible before realized", > >> + name); > >> + return; > >> + } > >> + > >> mr = ddc->get_memory_region(dimm, errp); just make ddc->get_memory_region return error if 'memdev' isn't set yet and add local_error handling here as proper property assessor should do. > >> if (!mr) { > >> return; > > > >
On 14.06.2018 15:24, Igor Mammedov wrote: > On Wed, 13 Jun 2018 16:03:35 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> On 13.06.2018 14:56, Igor Mammedov wrote: >>> On Mon, 11 Jun 2018 14:16:50 +0200 >>> David Hildenbrand <david@redhat.com> wrote: >>> >>>> "size" should not be queried before the device was realized. Let' make >>>> that explicit. >>>> >>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>> It's generic property getter, it should work even before realize is called. >> >> That's the point, as we can see in NVDIMM code , it is *not* a generic >> property getter. > it is added by > object_property_add(obj, PC_DIMM_SIZE_PROP, "uint64", pc_dimm_get_size, > NULL, NULL, NULL, &error_abort); > hence it's generic QOM property as opposed to memory device class specific callbacks. > >> >>> >>> if issues described in 5/11 are properly fixed, this patch won't be needed. >>> >>> So drop this patch >>> >>> >>>> --- >>>> hw/mem/pc-dimm.c | 6 ++++++ >>>> 1 file changed, 6 insertions(+) >>>> >>>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c >>>> index 86fbcf2d0c..5294734529 100644 >>>> --- a/hw/mem/pc-dimm.c >>>> +++ b/hw/mem/pc-dimm.c >>>> @@ -166,6 +166,12 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name, >>>> PCDIMMDevice *dimm = PC_DIMM(obj); >>>> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj); >>>> >>>> + if (!DEVICE(obj)->realized) { >>>> + error_setg(errp, "Property \"%s\" not accessible before realized", >>>> + name); >>>> + return; >>>> + } >>>> + >>>> mr = ddc->get_memory_region(dimm, errp); > just make ddc->get_memory_region return error if 'memdev' isn't set yet > and add local_error handling here as proper property assessor should do. > Does not help for NVDIMM.
On Thu, 14 Jun 2018 16:10:14 +0200 David Hildenbrand <david@redhat.com> wrote: > On 14.06.2018 15:24, Igor Mammedov wrote: > > On Wed, 13 Jun 2018 16:03:35 +0200 > > David Hildenbrand <david@redhat.com> wrote: > > > >> On 13.06.2018 14:56, Igor Mammedov wrote: > >>> On Mon, 11 Jun 2018 14:16:50 +0200 > >>> David Hildenbrand <david@redhat.com> wrote: > >>> > >>>> "size" should not be queried before the device was realized. Let' make > >>>> that explicit. > >>>> > >>>> Signed-off-by: David Hildenbrand <david@redhat.com> > >>> It's generic property getter, it should work even before realize is called. > >> > >> That's the point, as we can see in NVDIMM code , it is *not* a generic > >> property getter. > > it is added by > > object_property_add(obj, PC_DIMM_SIZE_PROP, "uint64", pc_dimm_get_size, > > NULL, NULL, NULL, &error_abort); > > hence it's generic QOM property as opposed to memory device class specific callbacks. > > > >> > >>> > >>> if issues described in 5/11 are properly fixed, this patch won't be needed. > >>> > >>> So drop this patch > >>> > >>> > >>>> --- > >>>> hw/mem/pc-dimm.c | 6 ++++++ > >>>> 1 file changed, 6 insertions(+) > >>>> > >>>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c > >>>> index 86fbcf2d0c..5294734529 100644 > >>>> --- a/hw/mem/pc-dimm.c > >>>> +++ b/hw/mem/pc-dimm.c > >>>> @@ -166,6 +166,12 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name, > >>>> PCDIMMDevice *dimm = PC_DIMM(obj); > >>>> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj); > >>>> > >>>> + if (!DEVICE(obj)->realized) { > >>>> + error_setg(errp, "Property \"%s\" not accessible before realized", > >>>> + name); > >>>> + return; > >>>> + } > >>>> + > >>>> mr = ddc->get_memory_region(dimm, errp); > > just make ddc->get_memory_region return error if 'memdev' isn't set yet > > and add local_error handling here as proper property assessor should do. > > > > Does not help for NVDIMM. NVDIMM should be fixed as it's broken now and works just by accident. I don't like tailoring infrastructure changes to a particular broken device especially when the later could be made to work within current framework.
On 15.06.2018 11:16, Igor Mammedov wrote: > On Thu, 14 Jun 2018 16:10:14 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> On 14.06.2018 15:24, Igor Mammedov wrote: >>> On Wed, 13 Jun 2018 16:03:35 +0200 >>> David Hildenbrand <david@redhat.com> wrote: >>> >>>> On 13.06.2018 14:56, Igor Mammedov wrote: >>>>> On Mon, 11 Jun 2018 14:16:50 +0200 >>>>> David Hildenbrand <david@redhat.com> wrote: >>>>> >>>>>> "size" should not be queried before the device was realized. Let' make >>>>>> that explicit. >>>>>> >>>>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>>> It's generic property getter, it should work even before realize is called. >>>> >>>> That's the point, as we can see in NVDIMM code , it is *not* a generic >>>> property getter. >>> it is added by >>> object_property_add(obj, PC_DIMM_SIZE_PROP, "uint64", pc_dimm_get_size, >>> NULL, NULL, NULL, &error_abort); >>> hence it's generic QOM property as opposed to memory device class specific callbacks. >>> >>>> >>>>> >>>>> if issues described in 5/11 are properly fixed, this patch won't be needed. >>>>> >>>>> So drop this patch >>>>> >>>>> >>>>>> --- >>>>>> hw/mem/pc-dimm.c | 6 ++++++ >>>>>> 1 file changed, 6 insertions(+) >>>>>> >>>>>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c >>>>>> index 86fbcf2d0c..5294734529 100644 >>>>>> --- a/hw/mem/pc-dimm.c >>>>>> +++ b/hw/mem/pc-dimm.c >>>>>> @@ -166,6 +166,12 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name, >>>>>> PCDIMMDevice *dimm = PC_DIMM(obj); >>>>>> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj); >>>>>> >>>>>> + if (!DEVICE(obj)->realized) { >>>>>> + error_setg(errp, "Property \"%s\" not accessible before realized", >>>>>> + name); >>>>>> + return; >>>>>> + } >>>>>> + >>>>>> mr = ddc->get_memory_region(dimm, errp); >>> just make ddc->get_memory_region return error if 'memdev' isn't set yet >>> and add local_error handling here as proper property assessor should do. >>> >> >> Does not help for NVDIMM. > NVDIMM should be fixed as it's broken now and works just by accident. > I don't like tailoring infrastructure changes to a particular broken > device especially when the later could be made to work within current > framework. > Yes, but I will go the path of static properties for NVDIMM instead. Trying to initialize the memory region in a property setter ("label-size") and then bailing out because another property (memdev) is not set looks strange. I will do checks+initialization in realize() and in get_memory_region(). This looks better in my point of view and looks like a nice NVDIMM refactoring (at least judging from the patches I have on my list now :) ).
On Fri, 15 Jun 2018 11:25:59 +0200 David Hildenbrand <david@redhat.com> wrote: > On 15.06.2018 11:16, Igor Mammedov wrote: > > On Thu, 14 Jun 2018 16:10:14 +0200 > > David Hildenbrand <david@redhat.com> wrote: > > > >> On 14.06.2018 15:24, Igor Mammedov wrote: > >>> On Wed, 13 Jun 2018 16:03:35 +0200 > >>> David Hildenbrand <david@redhat.com> wrote: > >>> > >>>> On 13.06.2018 14:56, Igor Mammedov wrote: > >>>>> On Mon, 11 Jun 2018 14:16:50 +0200 > >>>>> David Hildenbrand <david@redhat.com> wrote: > >>>>> > >>>>>> "size" should not be queried before the device was realized. Let' make > >>>>>> that explicit. > >>>>>> > >>>>>> Signed-off-by: David Hildenbrand <david@redhat.com> > >>>>> It's generic property getter, it should work even before realize is called. > >>>> > >>>> That's the point, as we can see in NVDIMM code , it is *not* a generic > >>>> property getter. > >>> it is added by > >>> object_property_add(obj, PC_DIMM_SIZE_PROP, "uint64", pc_dimm_get_size, > >>> NULL, NULL, NULL, &error_abort); > >>> hence it's generic QOM property as opposed to memory device class specific callbacks. > >>> > >>>> > >>>>> > >>>>> if issues described in 5/11 are properly fixed, this patch won't be needed. > >>>>> > >>>>> So drop this patch > >>>>> > >>>>> > >>>>>> --- > >>>>>> hw/mem/pc-dimm.c | 6 ++++++ > >>>>>> 1 file changed, 6 insertions(+) > >>>>>> > >>>>>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c > >>>>>> index 86fbcf2d0c..5294734529 100644 > >>>>>> --- a/hw/mem/pc-dimm.c > >>>>>> +++ b/hw/mem/pc-dimm.c > >>>>>> @@ -166,6 +166,12 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name, > >>>>>> PCDIMMDevice *dimm = PC_DIMM(obj); > >>>>>> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj); > >>>>>> > >>>>>> + if (!DEVICE(obj)->realized) { > >>>>>> + error_setg(errp, "Property \"%s\" not accessible before realized", > >>>>>> + name); > >>>>>> + return; > >>>>>> + } > >>>>>> + > >>>>>> mr = ddc->get_memory_region(dimm, errp); > >>> just make ddc->get_memory_region return error if 'memdev' isn't set yet > >>> and add local_error handling here as proper property assessor should do. > >>> > >> > >> Does not help for NVDIMM. > > NVDIMM should be fixed as it's broken now and works just by accident. > > I don't like tailoring infrastructure changes to a particular broken > > device especially when the later could be made to work within current > > framework. > > > > Yes, but I will go the path of static properties for NVDIMM instead. > Trying to initialize the memory region in a property setter > ("label-size") and then bailing out because another property (memdev) is > not set looks strange. I will do checks+initialization in realize() and > in get_memory_region(). This looks better in my point of view and looks > like a nice NVDIMM refactoring (at least judging from the patches I have > on my list now :) ). sure, I worry less about how it's implemented inside of device model as far as it behaves nicely to external user.
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c index 86fbcf2d0c..5294734529 100644 --- a/hw/mem/pc-dimm.c +++ b/hw/mem/pc-dimm.c @@ -166,6 +166,12 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name, PCDIMMDevice *dimm = PC_DIMM(obj); PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj); + if (!DEVICE(obj)->realized) { + error_setg(errp, "Property \"%s\" not accessible before realized", + name); + return; + } + mr = ddc->get_memory_region(dimm, errp); if (!mr) { return;
"size" should not be queried before the device was realized. Let' make that explicit. Signed-off-by: David Hildenbrand <david@redhat.com> --- hw/mem/pc-dimm.c | 6 ++++++ 1 file changed, 6 insertions(+)