On 13.06.2018 15:57, Igor Mammedov wrote: > On Wed, 13 Jun 2018 13:05:53 +0200 > David Hildenbrand <da...@redhat.com> wrote: > >> On 13.06.2018 13:01, Igor Mammedov wrote: >>> On Mon, 11 Jun 2018 14:16:49 +0200 >>> David Hildenbrand <da...@redhat.com> wrote: >>> >>>> This might look like a step backwards, but it is not. get_memory_region() >>>> should not be called on uninititalized devices. In general, only >>>> properties should be access, but no "derived" satte like the memory >>>> region. >>>> >>>> 1. We need duplicate error checks if memdev is actually already set. >>>> realize() performs these checks, no need to duplicate. >>> it's not duplicate, if a machine doesn't access to memory region >>> in preplug time (hence doesn't check), then device itself would check it, >>> check won't be missed by accident. >>> (yep it's more code but more robust at the same time, so I'd leave it as >>> is) >> >> Checking at two places for the same thing == duplicate checks > device models and there users are separate entities hence I consider > checks are separate. If user code can be written without adding extra checks > it's fine. But if device model doesn't have its own checks when and is > used in by new user code without checks also, it's going to break. > > So it would be hard to convince me that consolidating error handling > between in-depended layers is a good idea in general and particularly > in this case. > > I'd just drop this error cleanups altogether so that they won't get > in the way of actual changes you are aiming for (unless you have to do it). > >>>> 2. This is bad practise as one can see when looking at the NVDIMM >>>> implemetation. The call does not return sane data before the device >>>> is realized. Although spapr does not use NVDIMM, conceptually it is >>>> wrong. >>>> >>>> So let's just move this call to the right place. We can then cleanup >>>> get_memory_region(). >>> So I have to say no to this particular patch. >>> It is indeed a step backwards and it looks like workaround for broken >>> nvdimm impl. >>> >>> Firstly, memdev property must be set for dimm device and >>> a user accessing memory region first must check for error. >>> More details below. >>> >> >> You assume that any class function can be called at any time. And I >> don't think this is the way to go. > Not any time, in the case of get_memory_region() it should work at > pre_plug() time as all the pieces for it are already there. > So we should make it work correctly for NVDIMM instead of > succumbing to it and running wild. > we should stick to canonical sequence > object_new -> set props -> realize > I don't see any reason to violate it in this case other than laziness. >
See my replay to patch nr. 7. In contrast to what you suggest, I propose converting all nvdimm properties to static properties (because that's what they actually are). The validation/initialization of them should happen at a central place, not at various places (realize(), property setters, or even get_memory_region()) what you suggest. I propose a separate function for this (prepare() on DeviceClass), where we can split of the applicable parts of realize() that just perform property checks + basic initialization (e.g. of derived properties like the memory region in case of NVDIMM). -- Thanks, David / dhildenb
