| Message ID | dfcc5dc7e2efc0283bc38e3036da2c0323621cdb.1611647111.git.mprivozn@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] hostmem: Don't report pmem attribute if unsupported | expand |
On 1/26/21 8:48 AM, Michal Privoznik wrote: > When management applications (like Libvirt) want to check whether > memory-backend-file.pmem is supported they can list object > properties using 'qom-list-properties'. However, 'pmem' is > declared always (and thus reported always) and only at runtime > QEMU errors out if it was built without libpmem (and thus can not > guarantee write persistence). This is suboptimal since we have > ability to declare attributes at compile time. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1915216 > Signed-off-by: Michal Privoznik <mprivozn@redhat.com> > Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> > Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com> > Reviewed-by: Igor Mammedov <imammedo@redhat.com> > --- > > This is just a resend of a patch I've sent earlier with Reviewed-by and > Tested-by added: > > https://lists.nongnu.org/archive/html/qemu-devel/2021-01/msg04558.html > > backends/hostmem-file.c | 13 ++++--------- > 1 file changed, 4 insertions(+), 9 deletions(-) Polite ping, please. Michal
On Tue, Jan 26, 2021 at 08:48:25AM +0100, Michal Privoznik wrote: > When management applications (like Libvirt) want to check whether > memory-backend-file.pmem is supported they can list object > properties using 'qom-list-properties'. However, 'pmem' is > declared always (and thus reported always) and only at runtime > QEMU errors out if it was built without libpmem (and thus can not > guarantee write persistence). This is suboptimal since we have > ability to declare attributes at compile time. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1915216 > Signed-off-by: Michal Privoznik <mprivozn@redhat.com> > Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> > Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com> > Reviewed-by: Igor Mammedov <imammedo@redhat.com> I'm not a fan of making availability of properties conditional (even if at compile time), but if this helps libvirt I guess it makes sense. CCing John, who has been thinking a lot about these questions. I'm queueing this on machine-next. Thanks, and sorry for the delay! > --- > > This is just a resend of a patch I've sent earlier with Reviewed-by and > Tested-by added: > > https://lists.nongnu.org/archive/html/qemu-devel/2021-01/msg04558.html > > backends/hostmem-file.c | 13 ++++--------- > 1 file changed, 4 insertions(+), 9 deletions(-) > > diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c > index 40e1e5b3e3..7e30eb5985 100644 > --- a/backends/hostmem-file.c > +++ b/backends/hostmem-file.c > @@ -123,6 +123,7 @@ static void file_memory_backend_set_align(Object *o, Visitor *v, > fb->align = val; > } > > +#ifdef CONFIG_LIBPMEM > static bool file_memory_backend_get_pmem(Object *o, Error **errp) > { > return MEMORY_BACKEND_FILE(o)->is_pmem; > @@ -139,17 +140,9 @@ static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp) > return; > } > > -#ifndef CONFIG_LIBPMEM > - if (value) { > - error_setg(errp, "Lack of libpmem support while setting the 'pmem=on'" > - " of %s. We can't ensure data persistence.", > - object_get_typename(o)); > - return; > - } > -#endif > - > fb->is_pmem = value; > } > +#endif /* CONFIG_LIBPMEM */ > > static void file_backend_unparent(Object *obj) > { > @@ -180,8 +173,10 @@ file_backend_class_init(ObjectClass *oc, void *data) > file_memory_backend_get_align, > file_memory_backend_set_align, > NULL, NULL); > +#ifdef CONFIG_LIBPMEM > object_class_property_add_bool(oc, "pmem", > file_memory_backend_get_pmem, file_memory_backend_set_pmem); > +#endif > } > > static void file_backend_instance_finalize(Object *o) > -- > 2.26.2 >
On 2/16/21 5:23 PM, Eduardo Habkost wrote: > On Tue, Jan 26, 2021 at 08:48:25AM +0100, Michal Privoznik wrote: >> When management applications (like Libvirt) want to check whether >> memory-backend-file.pmem is supported they can list object >> properties using 'qom-list-properties'. However, 'pmem' is >> declared always (and thus reported always) and only at runtime >> QEMU errors out if it was built without libpmem (and thus can not >> guarantee write persistence). This is suboptimal since we have >> ability to declare attributes at compile time. >> >> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1915216 >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> >> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> >> Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com> >> Reviewed-by: Igor Mammedov <imammedo@redhat.com> > > I'm not a fan of making availability of properties conditional > (even if at compile time), but if this helps libvirt I guess it > makes sense. > Compile time might be OK, but if we want to describe everything via QAPI eventually, we just need to be able to describe that compile-time requisite appropriately. Conditional at run-time is I think the thing that absolutely has to go wherever it surfaces. > CCing John, who has been thinking a lot about these questions. > Thanks for the heads up. Good reminder that libvirt uses the existence of properties as a bellwether for feature support. I don't think I like that idea, but I like breaking libvirt even less. --js > I'm queueing this on machine-next. Thanks, and sorry for the delay! > >> --- >> >> This is just a resend of a patch I've sent earlier with Reviewed-by and >> Tested-by added: >> >> https://lists.nongnu.org/archive/html/qemu-devel/2021-01/msg04558.html >> >> backends/hostmem-file.c | 13 ++++--------- >> 1 file changed, 4 insertions(+), 9 deletions(-) >> >> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c >> index 40e1e5b3e3..7e30eb5985 100644 >> --- a/backends/hostmem-file.c >> +++ b/backends/hostmem-file.c >> @@ -123,6 +123,7 @@ static void file_memory_backend_set_align(Object *o, Visitor *v, >> fb->align = val; >> } >> >> +#ifdef CONFIG_LIBPMEM >> static bool file_memory_backend_get_pmem(Object *o, Error **errp) >> { >> return MEMORY_BACKEND_FILE(o)->is_pmem; >> @@ -139,17 +140,9 @@ static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp) >> return; >> } >> >> -#ifndef CONFIG_LIBPMEM >> - if (value) { >> - error_setg(errp, "Lack of libpmem support while setting the 'pmem=on'" >> - " of %s. We can't ensure data persistence.", >> - object_get_typename(o)); >> - return; >> - } >> -#endif >> - >> fb->is_pmem = value; >> } >> +#endif /* CONFIG_LIBPMEM */ >> >> static void file_backend_unparent(Object *obj) >> { >> @@ -180,8 +173,10 @@ file_backend_class_init(ObjectClass *oc, void *data) >> file_memory_backend_get_align, >> file_memory_backend_set_align, >> NULL, NULL); >> +#ifdef CONFIG_LIBPMEM >> object_class_property_add_bool(oc, "pmem", >> file_memory_backend_get_pmem, file_memory_backend_set_pmem); >> +#endif >> } >> >> static void file_backend_instance_finalize(Object *o) >> -- >> 2.26.2 >> >
On 2/17/21 12:07 AM, John Snow wrote: > On 2/16/21 5:23 PM, Eduardo Habkost wrote: >> On Tue, Jan 26, 2021 at 08:48:25AM +0100, Michal Privoznik wrote: >>> When management applications (like Libvirt) want to check whether >>> memory-backend-file.pmem is supported they can list object >>> properties using 'qom-list-properties'. However, 'pmem' is >>> declared always (and thus reported always) and only at runtime >>> QEMU errors out if it was built without libpmem (and thus can not >>> guarantee write persistence). This is suboptimal since we have >>> ability to declare attributes at compile time. >>> >>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1915216 >>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> >>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> >>> Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com> >>> Reviewed-by: Igor Mammedov <imammedo@redhat.com> >> >> I'm not a fan of making availability of properties conditional >> (even if at compile time), but if this helps libvirt I guess it >> makes sense. >> > > Compile time might be OK, but if we want to describe everything via QAPI > eventually, we just need to be able to describe that compile-time > requisite appropriately. > > Conditional at run-time is I think the thing that absolutely has to go > wherever it surfaces. I'm open for discussion. How do you think libvirt (or any other mgmt tool/user) should inspect whether given feature is available? What libvirt currently does it issues 'qom-list-properties' with 'typename=memory-backend-file' and inspects whether pmem attribute is available. Is 'qom-list' preferred? > >> CCing John, who has been thinking a lot about these questions. >> > > Thanks for the heads up. Good reminder that libvirt uses the existence > of properties as a bellwether for feature support. I don't think I like > that idea, but I like breaking libvirt even less. That was at hand solution. If libvirt's not doing it right, I'm happy to make things better. Michal
On 2/17/21 2:31 AM, Michal Privoznik wrote: > On 2/17/21 12:07 AM, John Snow wrote: >> On 2/16/21 5:23 PM, Eduardo Habkost wrote: >>> On Tue, Jan 26, 2021 at 08:48:25AM +0100, Michal Privoznik wrote: >>>> When management applications (like Libvirt) want to check whether >>>> memory-backend-file.pmem is supported they can list object >>>> properties using 'qom-list-properties'. However, 'pmem' is >>>> declared always (and thus reported always) and only at runtime >>>> QEMU errors out if it was built without libpmem (and thus can not >>>> guarantee write persistence). This is suboptimal since we have >>>> ability to declare attributes at compile time. >>>> >>>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1915216 >>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> >>>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> >>>> Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com> >>>> Reviewed-by: Igor Mammedov <imammedo@redhat.com> >>> >>> I'm not a fan of making availability of properties conditional >>> (even if at compile time), but if this helps libvirt I guess it >>> makes sense. >>> >> >> Compile time might be OK, but if we want to describe everything via >> QAPI eventually, we just need to be able to describe that compile-time >> requisite appropriately. >> >> Conditional at run-time is I think the thing that absolutely has to go >> wherever it surfaces. > > I'm open for discussion. How do you think libvirt (or any other mgmt > tool/user) should inspect whether given feature is available? > What libvirt currently does it issues 'qom-list-properties' with > 'typename=memory-backend-file' and inspects whether pmem attribute is > available. Is 'qom-list' preferred? > > >> >>> CCing John, who has been thinking a lot about these questions. >>> >> >> Thanks for the heads up. Good reminder that libvirt uses the existence >> of properties as a bellwether for feature support. I don't think I >> like that idea, but I like breaking libvirt even less. > > That was at hand solution. If libvirt's not doing it right, I'm happy to > make things better. > > Michal No, libvirt is doing it exactly correct. QAPI/QMP was designed exactly in this way with exactly this use-case in mind. (So far as I understand it.) My concerns that may have guided some patches by Eduardo that might have caused problems for you relate to my ability to publish an SDK for generic builds of QEMU, where if-conditionals that actually compile fields out of certain data structures can be difficult to deal with at static analysis time. Until we connect to the server, we don't know if type FooStruct has field XYZ or not. Generally the way you handle this is by always having that field in the SDK and erroring out at runtime if for some reason it is not supportable. In the long term, we want to (I think) bridge the data gap between QOM and QAPI and provide a unified set of types that we can use to construct a "QEMU Config File" that can be validated statically against, say, "qemu 6.0." In this scenario, I have some nebulous but not necessarily meticulously reasoned out concerns about compile-time conditional fields. In this scenario, using the presence or absence of a field in a data type becomes a poor way to do feature detection. QMP offers the "features" flag for certain commands where we use the presence or absence of that flag as the introspection data in order to determine behavior. Going forward, I suspect I will push for representing formerly-compile-time flags as runtime introspection feature flags instead. ...But that's stuff that isn't here now, so just keep doing what you've been doing, and I will take careful notice not to break that kind of introspection without a well-advertised alternative. Especially right now, QOM stuff isn't in QAPI, so we don't have those kind of feature flags at all, so I think there really isn't another way at all, short of adding more capabilities and complexity to the existing introspection stuff, which I don't think we'd do. --js
diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c index 40e1e5b3e3..7e30eb5985 100644 --- a/backends/hostmem-file.c +++ b/backends/hostmem-file.c @@ -123,6 +123,7 @@ static void file_memory_backend_set_align(Object *o, Visitor *v, fb->align = val; } +#ifdef CONFIG_LIBPMEM static bool file_memory_backend_get_pmem(Object *o, Error **errp) { return MEMORY_BACKEND_FILE(o)->is_pmem; @@ -139,17 +140,9 @@ static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp) return; } -#ifndef CONFIG_LIBPMEM - if (value) { - error_setg(errp, "Lack of libpmem support while setting the 'pmem=on'" - " of %s. We can't ensure data persistence.", - object_get_typename(o)); - return; - } -#endif - fb->is_pmem = value; } +#endif /* CONFIG_LIBPMEM */ static void file_backend_unparent(Object *obj) { @@ -180,8 +173,10 @@ file_backend_class_init(ObjectClass *oc, void *data) file_memory_backend_get_align, file_memory_backend_set_align, NULL, NULL); +#ifdef CONFIG_LIBPMEM object_class_property_add_bool(oc, "pmem", file_memory_backend_get_pmem, file_memory_backend_set_pmem); +#endif } static void file_backend_instance_finalize(Object *o)