Move the domainPostParseDataAlloc/Free calls to
virDomainDeviceDefParse. As an consequence
virDomainDeviceDefPostParseOne is superfluous and can therefore be
removed.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
---
src/conf/domain_conf.c | 37 +++++++++++--------------------------
1 file changed, 11 insertions(+), 26 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3c307d325a89..e61f04ea2271 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4900,31 +4900,6 @@ virDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
return 0;
}
-static int
-virDomainDeviceDefPostParseOne(virDomainDeviceDefPtr dev,
- const virDomainDef *def,
- virCapsPtr caps,
- unsigned int flags,
- virDomainXMLOptionPtr xmlopt)
-{
- void *parseOpaque = NULL;
- int ret;
-
- if (xmlopt->config.domainPostParseDataAlloc) {
- if (xmlopt->config.domainPostParseDataAlloc(def, caps, flags,
- xmlopt->config.priv,
- &parseOpaque) < 0)
- return -1;
- }
-
- ret = virDomainDeviceDefPostParse(dev, def, caps, flags, xmlopt, parseOpaque);
-
- if (parseOpaque && xmlopt->config.domainPostParseDataFree)
- xmlopt->config.domainPostParseDataFree(parseOpaque);
-
- return ret;
-}
-
struct virDomainDefPostParseDeviceIteratorData {
virCapsPtr caps;
@@ -16066,6 +16041,7 @@ virDomainDeviceDefParse(const char *xmlStr,
{
xmlDocPtr xml;
xmlNodePtr node;
+ void *parseOpaque = NULL;
xmlXPathContextPtr ctxt = NULL;
virDomainDeviceDefPtr dev = NULL;
char *netprefix;
@@ -16222,8 +16198,15 @@ virDomainDeviceDefParse(const char *xmlStr,
break;
}
+ if (xmlopt->config.domainPostParseDataAlloc) {
+ if (xmlopt->config.domainPostParseDataAlloc(def, caps, flags,
+ xmlopt->config.priv,
+ &parseOpaque) < 0)
+ goto error;
+ }
+
/* callback to fill driver specific device aspects */
- if (virDomainDeviceDefPostParseOne(dev, def, caps, flags, xmlopt) < 0)
+ if (virDomainDeviceDefPostParse(dev, def, caps, flags, xmlopt, parseOpaque) < 0)
goto error;
/* validate the configuration */
@@ -16231,6 +16214,8 @@ virDomainDeviceDefParse(const char *xmlStr,
goto error;
cleanup:
+ if (parseOpaque && xmlopt->config.domainPostParseDataFree)
+ xmlopt->config.domainPostParseDataFree(parseOpaque);
xmlFreeDoc(xml);
xmlXPathFreeContext(ctxt);
return dev;
--
2.17.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 9/20/18 1:44 PM, Marc Hartmayer wrote:
> Move the domainPostParseDataAlloc/Free calls to
> virDomainDeviceDefParse. As an consequence
> virDomainDeviceDefPostParseOne is superfluous and can therefore be
> removed.
>
> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
> ---
> src/conf/domain_conf.c | 37 +++++++++++--------------------------
> 1 file changed, 11 insertions(+), 26 deletions(-)
>
I'm not against this per se; however, I should not that the code was
specifically extracted in commit e168bc8a.
John
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 3c307d325a89..e61f04ea2271 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -4900,31 +4900,6 @@ virDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
> return 0;
> }
>
> -static int
> -virDomainDeviceDefPostParseOne(virDomainDeviceDefPtr dev,
> - const virDomainDef *def,
> - virCapsPtr caps,
> - unsigned int flags,
> - virDomainXMLOptionPtr xmlopt)
> -{
> - void *parseOpaque = NULL;
> - int ret;
> -
> - if (xmlopt->config.domainPostParseDataAlloc) {
> - if (xmlopt->config.domainPostParseDataAlloc(def, caps, flags,
> - xmlopt->config.priv,
> - &parseOpaque) < 0)
> - return -1;
> - }
> -
> - ret = virDomainDeviceDefPostParse(dev, def, caps, flags, xmlopt, parseOpaque);
> -
> - if (parseOpaque && xmlopt->config.domainPostParseDataFree)
> - xmlopt->config.domainPostParseDataFree(parseOpaque);
> -
> - return ret;
> -}
> -
>
> struct virDomainDefPostParseDeviceIteratorData {
> virCapsPtr caps;
> @@ -16066,6 +16041,7 @@ virDomainDeviceDefParse(const char *xmlStr,
> {
> xmlDocPtr xml;
> xmlNodePtr node;
> + void *parseOpaque = NULL;
> xmlXPathContextPtr ctxt = NULL;
> virDomainDeviceDefPtr dev = NULL;
> char *netprefix;
> @@ -16222,8 +16198,15 @@ virDomainDeviceDefParse(const char *xmlStr,
> break;
> }
>
> + if (xmlopt->config.domainPostParseDataAlloc) {
> + if (xmlopt->config.domainPostParseDataAlloc(def, caps, flags,
> + xmlopt->config.priv,
> + &parseOpaque) < 0)
> + goto error;
> + }
> +
> /* callback to fill driver specific device aspects */
> - if (virDomainDeviceDefPostParseOne(dev, def, caps, flags, xmlopt) < 0)
> + if (virDomainDeviceDefPostParse(dev, def, caps, flags, xmlopt, parseOpaque) < 0)
> goto error;
>
> /* validate the configuration */
> @@ -16231,6 +16214,8 @@ virDomainDeviceDefParse(const char *xmlStr,
> goto error;
>
> cleanup:
> + if (parseOpaque && xmlopt->config.domainPostParseDataFree)
> + xmlopt->config.domainPostParseDataFree(parseOpaque);
> xmlFreeDoc(xml);
> xmlXPathFreeContext(ctxt);
> return dev;
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Sat, Sep 29, 2018 at 04:09 AM +0200, John Ferlan <jferlan@redhat.com> wrote:
> On 9/20/18 1:44 PM, Marc Hartmayer wrote:
>> Move the domainPostParseDataAlloc/Free calls to
>> virDomainDeviceDefParse. As an consequence
>> virDomainDeviceDefPostParseOne is superfluous and can therefore be
>> removed.
>>
>> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
>> ---
>> src/conf/domain_conf.c | 37 +++++++++++--------------------------
>> 1 file changed, 11 insertions(+), 26 deletions(-)
>>
>
> I'm not against this per se; however, I should not that the code was
> specifically extracted in commit e168bc8a.
There are the following three functions:
virDomainDeviceDefParse
virDomainDeviceDefPostParse
virDomainDeviceDefPostParseOne
Peter introduced the function “virDomainDeviceDefPostParseOne” to avoid
the allocation of private data across the callbacks. This is absolutely
fine.
What I’ve done is, I moved the domainPostParseDataAlloc/Free calls to
virDomainDeviceDefParse instead of having an extra wrapper function
(virDomainDeviceDefPostParse/One) for that. With this change I can reuse
the QEMU caps for the virDomainDeviceDefValidate call in
virDomainDeviceDefParse as well.
For safety I added Peter to CC.
>
> John
>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 3c307d325a89..e61f04ea2271 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -4900,31 +4900,6 @@ virDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
>> return 0;
>> }
>>
>> -static int
>> -virDomainDeviceDefPostParseOne(virDomainDeviceDefPtr dev,
>> - const virDomainDef *def,
>> - virCapsPtr caps,
>> - unsigned int flags,
>> - virDomainXMLOptionPtr xmlopt)
>> -{
>> - void *parseOpaque = NULL;
>> - int ret;
>> -
>> - if (xmlopt->config.domainPostParseDataAlloc) {
>> - if (xmlopt->config.domainPostParseDataAlloc(def, caps, flags,
>> - xmlopt->config.priv,
>> - &parseOpaque) < 0)
>> - return -1;
>> - }
>> -
>> - ret = virDomainDeviceDefPostParse(dev, def, caps, flags, xmlopt, parseOpaque);
>> -
>> - if (parseOpaque && xmlopt->config.domainPostParseDataFree)
>> - xmlopt->config.domainPostParseDataFree(parseOpaque);
>> -
>> - return ret;
>> -}
>> -
>>
>> struct virDomainDefPostParseDeviceIteratorData {
>> virCapsPtr caps;
>> @@ -16066,6 +16041,7 @@ virDomainDeviceDefParse(const char *xmlStr,
>> {
>> xmlDocPtr xml;
>> xmlNodePtr node;
>> + void *parseOpaque = NULL;
>> xmlXPathContextPtr ctxt = NULL;
>> virDomainDeviceDefPtr dev = NULL;
>> char *netprefix;
>> @@ -16222,8 +16198,15 @@ virDomainDeviceDefParse(const char *xmlStr,
>> break;
>> }
>>
>> + if (xmlopt->config.domainPostParseDataAlloc) {
>> + if (xmlopt->config.domainPostParseDataAlloc(def, caps, flags,
>> + xmlopt->config.priv,
>> + &parseOpaque) < 0)
>> + goto error;
>> + }
>> +
>> /* callback to fill driver specific device aspects */
>> - if (virDomainDeviceDefPostParseOne(dev, def, caps, flags, xmlopt) < 0)
>> + if (virDomainDeviceDefPostParse(dev, def, caps, flags, xmlopt, parseOpaque) < 0)
>> goto error;
>>
>> /* validate the configuration */
>> @@ -16231,6 +16214,8 @@ virDomainDeviceDefParse(const char *xmlStr,
>> goto error;
>>
>> cleanup:
>> + if (parseOpaque && xmlopt->config.domainPostParseDataFree)
>> + xmlopt->config.domainPostParseDataFree(parseOpaque);
>> xmlFreeDoc(xml);
>> xmlXPathFreeContext(ctxt);
>> return dev;
>>
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Oct 01, 2018 at 12:27:41 +0200, Marc Hartmayer wrote: > On Sat, Sep 29, 2018 at 04:09 AM +0200, John Ferlan <jferlan@redhat.com> wrote: > > On 9/20/18 1:44 PM, Marc Hartmayer wrote: > >> Move the domainPostParseDataAlloc/Free calls to > >> virDomainDeviceDefParse. As an consequence > >> virDomainDeviceDefPostParseOne is superfluous and can therefore be > >> removed. > >> > >> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> > >> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> > >> --- > >> src/conf/domain_conf.c | 37 +++++++++++-------------------------- > >> 1 file changed, 11 insertions(+), 26 deletions(-) > >> > > > > I'm not against this per se; however, I should not that the code was > > specifically extracted in commit e168bc8a. > > There are the following three functions: > > virDomainDeviceDefParse > virDomainDeviceDefPostParse > virDomainDeviceDefPostParseOne > > Peter introduced the function “virDomainDeviceDefPostParseOne” to avoid > the allocation of private data across the callbacks. This is absolutely > fine. > > What I’ve done is, I moved the domainPostParseDataAlloc/Free calls to > virDomainDeviceDefParse instead of having an extra wrapper function > (virDomainDeviceDefPostParse/One) for that. With this change I can reuse > the QEMU caps for the virDomainDeviceDefValidate call in > virDomainDeviceDefParse as well. For the above it's not necessary to open-code what virDomainDeviceDefPostParseOne in a rather massive function. You can pass the opaque in if you want. Please don't remove the wrapper. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Oct 01, 2018 at 12:41 PM +0200, Peter Krempa <pkrempa@redhat.com> wrote: > On Mon, Oct 01, 2018 at 12:27:41 +0200, Marc Hartmayer wrote: >> On Sat, Sep 29, 2018 at 04:09 AM +0200, John Ferlan <jferlan@redhat.com> wrote: >> > On 9/20/18 1:44 PM, Marc Hartmayer wrote: >> >> Move the domainPostParseDataAlloc/Free calls to >> >> virDomainDeviceDefParse. As an consequence >> >> virDomainDeviceDefPostParseOne is superfluous and can therefore be >> >> removed. >> >> >> >> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> >> >> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> >> >> --- >> >> src/conf/domain_conf.c | 37 +++++++++++-------------------------- >> >> 1 file changed, 11 insertions(+), 26 deletions(-) >> >> >> > >> > I'm not against this per se; however, I should not that the code was >> > specifically extracted in commit e168bc8a. >> >> There are the following three functions: >> >> virDomainDeviceDefParse >> virDomainDeviceDefPostParse >> virDomainDeviceDefPostParseOne >> >> Peter introduced the function “virDomainDeviceDefPostParseOne” to avoid >> the allocation of private data across the callbacks. This is absolutely >> fine. >> >> What I’ve done is, I moved the domainPostParseDataAlloc/Free calls to >> virDomainDeviceDefParse instead of having an extra wrapper function >> (virDomainDeviceDefPostParse/One) for that. With this change I can reuse >> the QEMU caps for the virDomainDeviceDefValidate call in >> virDomainDeviceDefParse as well. > > For the above it's not necessary to open-code what virDomainDeviceDefPostParseOne > in a rather massive function. You can pass the opaque in if you want. I don’t get it. Where can I pass the opaque? At the end, we must use the same qemuCaps in virDomainDeviceDefValidate that we already used for virDomainDeviceDefPostParse(One). > > Please don't remove the wrapper. > -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Oct 01, 2018 at 14:38:40 +0200, Marc Hartmayer wrote: > On Mon, Oct 01, 2018 at 12:41 PM +0200, Peter Krempa <pkrempa@redhat.com> wrote: > > On Mon, Oct 01, 2018 at 12:27:41 +0200, Marc Hartmayer wrote: > >> On Sat, Sep 29, 2018 at 04:09 AM +0200, John Ferlan <jferlan@redhat.com> wrote: > >> > On 9/20/18 1:44 PM, Marc Hartmayer wrote: > >> >> Move the domainPostParseDataAlloc/Free calls to > >> >> virDomainDeviceDefParse. As an consequence > >> >> virDomainDeviceDefPostParseOne is superfluous and can therefore be > >> >> removed. > >> >> > >> >> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> > >> >> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> > >> >> --- > >> >> src/conf/domain_conf.c | 37 +++++++++++-------------------------- > >> >> 1 file changed, 11 insertions(+), 26 deletions(-) > >> >> > >> > > >> > I'm not against this per se; however, I should not that the code was > >> > specifically extracted in commit e168bc8a. > >> > >> There are the following three functions: > >> > >> virDomainDeviceDefParse > >> virDomainDeviceDefPostParse > >> virDomainDeviceDefPostParseOne > >> > >> Peter introduced the function “virDomainDeviceDefPostParseOne” to avoid > >> the allocation of private data across the callbacks. This is absolutely > >> fine. > >> > >> What I’ve done is, I moved the domainPostParseDataAlloc/Free calls to > >> virDomainDeviceDefParse instead of having an extra wrapper function > >> (virDomainDeviceDefPostParse/One) for that. With this change I can reuse > >> the QEMU caps for the virDomainDeviceDefValidate call in > >> virDomainDeviceDefParse as well. > > > > For the above it's not necessary to open-code what virDomainDeviceDefPostParseOne > > in a rather massive function. You can pass the opaque in if you want. > > I don’t get it. Where can I pass the opaque? virDomainDeviceDefParse is a big spaghettified function and adding other code to it is unwanted. If you need to unify the allocation of private data, move the validation function call to the helper (with appropriate rename) rather than blatantly pasting the code back. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Oct 01, 2018 at 02:50 PM +0200, Peter Krempa <pkrempa@redhat.com> wrote: > On Mon, Oct 01, 2018 at 14:38:40 +0200, Marc Hartmayer wrote: >> On Mon, Oct 01, 2018 at 12:41 PM +0200, Peter Krempa <pkrempa@redhat.com> wrote: >> > On Mon, Oct 01, 2018 at 12:27:41 +0200, Marc Hartmayer wrote: >> >> On Sat, Sep 29, 2018 at 04:09 AM +0200, John Ferlan <jferlan@redhat.com> wrote: >> >> > On 9/20/18 1:44 PM, Marc Hartmayer wrote: >> >> >> Move the domainPostParseDataAlloc/Free calls to >> >> >> virDomainDeviceDefParse. As an consequence >> >> >> virDomainDeviceDefPostParseOne is superfluous and can therefore be >> >> >> removed. >> >> >> >> >> >> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> >> >> >> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> >> >> >> --- >> >> >> src/conf/domain_conf.c | 37 +++++++++++-------------------------- >> >> >> 1 file changed, 11 insertions(+), 26 deletions(-) >> >> >> >> >> > >> >> > I'm not against this per se; however, I should not that the code was >> >> > specifically extracted in commit e168bc8a. >> >> >> >> There are the following three functions: >> >> >> >> virDomainDeviceDefParse >> >> virDomainDeviceDefPostParse >> >> virDomainDeviceDefPostParseOne >> >> >> >> Peter introduced the function “virDomainDeviceDefPostParseOne” to avoid >> >> the allocation of private data across the callbacks. This is absolutely >> >> fine. >> >> >> >> What I’ve done is, I moved the domainPostParseDataAlloc/Free calls to >> >> virDomainDeviceDefParse instead of having an extra wrapper function >> >> (virDomainDeviceDefPostParse/One) for that. With this change I can reuse >> >> the QEMU caps for the virDomainDeviceDefValidate call in >> >> virDomainDeviceDefParse as well. >> > >> > For the above it's not necessary to open-code what virDomainDeviceDefPostParseOne >> > in a rather massive function. You can pass the opaque in if you want. >> >> I don’t get it. Where can I pass the opaque? > > virDomainDeviceDefParse is a big spaghettified function and > adding other code to it is unwanted. If you need to unify the allocation > of private data, move the validation function call to the helper (with > appropriate rename) rather than blatantly pasting the code back. Ahhh okay. Any ideas for the name? Thanks. Side note: What has always surprised me is that the "virDomainDeviceDefPostParse" function is called right out of the context of the function "virDomainDeviceDefParse". Shouldn’t it be called directly _after_ virDomainDeviceDefParse? -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.