Re: [PATCH 06/11] conf: Get rid of virDomainDeviceDefPostParseOne

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Oct 01, 2018 at 02:50 PM +0200, Peter Krempa <pkrempa@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
>> >> >> Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx>
>> >> >> ---
>> >> >>  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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux