1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] virQEMUDriverPtr parameters clean up in function qemuExtTPMStart() in /src/qemu/qemu_tpm.c

Humaid posted 1 patch 5 years, 1 month ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190323163442.6735-1-syedhumaidbinharoon@gmail.com
src/qemu/qemu_tpm.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
[libvirt] [PATCH] virQEMUDriverPtr parameters clean up in function qemuExtTPMStart() in /src/qemu/qemu_tpm.c
Posted by Humaid 5 years, 1 month ago 
---
 src/qemu/qemu_tpm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index 835a9caf46..b60e443f14 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -834,16 +834,16 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver,
 
 
 int
-qemuExtTPMStart(virQEMUDriverPtr driver,
-                virDomainObjPtr vm,
+qemuExtTPMStart(virDomainObjPtr vm,
                 qemuDomainLogContextPtr logCtxt)
 {
     int ret = 0;
+    qemuDomainObjPrivatePtr priv = vm->privateData;
     virDomainTPMDefPtr tpm = vm->def->tpm;
 
     switch (tpm->type) {
     case VIR_DOMAIN_TPM_TYPE_EMULATOR:
-        ret = qemuExtTPMStartEmulator(driver, vm, logCtxt);
+        ret = qemuExtTPMStartEmulator(priv->driver, vm, logCtxt);
         break;
     case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
     case VIR_DOMAIN_TPM_TYPE_LAST:
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virQEMUDriverPtr parameters clean up in function qemuExtTPMStart() in /src/qemu/qemu_tpm.c
Posted by Erik Skultety 5 years, 1 month ago 
You're missing:
- commit message explaining the change
- Your full name as author
- compliance with developer certificate of origin, see [1]

On Sat, Mar 23, 2019 at 08:34:42PM +0400, Humaid wrote:
> ---
>  src/qemu/qemu_tpm.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
> index 835a9caf46..b60e443f14 100644
> --- a/src/qemu/qemu_tpm.c
> +++ b/src/qemu/qemu_tpm.c
> @@ -834,16 +834,16 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver,
>
>
>  int
> -qemuExtTPMStart(virQEMUDriverPtr driver,
> -                virDomainObjPtr vm,
> +qemuExtTPMStart(virDomainObjPtr vm,
>                  qemuDomainLogContextPtr logCtxt)
>  {
>      int ret = 0;
> +    qemuDomainObjPrivatePtr priv = vm->privateData;

A quick grep through the code base shows that we could do this at many more
places actually.

Regards,
Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virQEMUDriverPtr parameters clean up in function qemuExtTPMStart() in /src/qemu/qemu_tpm.c
Posted by Erik Skultety 5 years, 1 month ago 
On Mon, Mar 25, 2019 at 09:14:38AM +0100, Erik Skultety wrote:
> You're missing:
> - commit message explaining the change
> - Your full name as author
> - compliance with developer certificate of origin, see [1]
>
> On Sat, Mar 23, 2019 at 08:34:42PM +0400, Humaid wrote:
> > ---
> >  src/qemu/qemu_tpm.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
> > index 835a9caf46..b60e443f14 100644
> > --- a/src/qemu/qemu_tpm.c
> > +++ b/src/qemu/qemu_tpm.c
> > @@ -834,16 +834,16 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver,
> >
> >
> >  int
> > -qemuExtTPMStart(virQEMUDriverPtr driver,
> > -                virDomainObjPtr vm,
> > +qemuExtTPMStart(virDomainObjPtr vm,
> >                  qemuDomainLogContextPtr logCtxt)
> >  {
> >      int ret = 0;
> > +    qemuDomainObjPrivatePtr priv = vm->privateData;
>
> A quick grep through the code base shows that we could do this at many more
> places actually.
>
> Regards,
> Erik

Forgot the reference:
[1] https://libvirt.org/hacking.html

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virQEMUDriverPtr parameters clean up in function qemuExtTPMStart() in /src/qemu/qemu_tpm.c
Posted by Peter Krempa 5 years, 1 month ago 
On Mon, Mar 25, 2019 at 09:15:23 +0100, Erik Skultety wrote:
> On Mon, Mar 25, 2019 at 09:14:38AM +0100, Erik Skultety wrote:
> > You're missing:
> > - commit message explaining the change
> > - Your full name as author
> > - compliance with developer certificate of origin, see [1]

https://www.redhat.com/archives/libvir-list/2019-March/msg01148.html

> >
> > On Sat, Mar 23, 2019 at 08:34:42PM +0400, Humaid wrote:
> > > ---
> > >  src/qemu/qemu_tpm.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
> > > index 835a9caf46..b60e443f14 100644
> > > --- a/src/qemu/qemu_tpm.c
> > > +++ b/src/qemu/qemu_tpm.c
> > > @@ -834,16 +834,16 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver,
> > >
> > >
> > >  int
> > > -qemuExtTPMStart(virQEMUDriverPtr driver,
> > > -                virDomainObjPtr vm,
> > > +qemuExtTPMStart(virDomainObjPtr vm,
> > >                  qemuDomainLogContextPtr logCtxt)
> > >  {
> > >      int ret = 0;
> > > +    qemuDomainObjPrivatePtr priv = vm->privateData;
> >
> > A quick grep through the code base shows that we could do this at many more
> > places actually.

Daniel pointed out that it's not actually worth doing as a separate
cleanup:

https://www.redhat.com/archives/libvir-list/2019-March/msg01147.html
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virQEMUDriverPtr parameters clean up in function qemuExtTPMStart() in /src/qemu/qemu_tpm.c
Posted by Martin Kletzander 5 years, 1 month ago 
On Mon, Mar 25, 2019 at 10:03:31AM +0100, Peter Krempa wrote:
>On Mon, Mar 25, 2019 at 09:15:23 +0100, Erik Skultety wrote:
>> On Mon, Mar 25, 2019 at 09:14:38AM +0100, Erik Skultety wrote:
>> > You're missing:
>> > - commit message explaining the change
>> > - Your full name as author
>> > - compliance with developer certificate of origin, see [1]
>
>https://www.redhat.com/archives/libvir-list/2019-March/msg01148.html
>
>> >
>> > On Sat, Mar 23, 2019 at 08:34:42PM +0400, Humaid wrote:
>> > > ---
>> > >  src/qemu/qemu_tpm.c | 6 +++---
>> > >  1 file changed, 3 insertions(+), 3 deletions(-)
>> > >
>> > > diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
>> > > index 835a9caf46..b60e443f14 100644
>> > > --- a/src/qemu/qemu_tpm.c
>> > > +++ b/src/qemu/qemu_tpm.c
>> > > @@ -834,16 +834,16 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver,
>> > >
>> > >
>> > >  int
>> > > -qemuExtTPMStart(virQEMUDriverPtr driver,
>> > > -                virDomainObjPtr vm,
>> > > +qemuExtTPMStart(virDomainObjPtr vm,
>> > >                  qemuDomainLogContextPtr logCtxt)
>> > >  {
>> > >      int ret = 0;
>> > > +    qemuDomainObjPrivatePtr priv = vm->privateData;
>> >
>> > A quick grep through the code base shows that we could do this at many more
>> > places actually.
>
>Daniel pointed out that it's not actually worth doing as a separate
>cleanup:
>
>https://www.redhat.com/archives/libvir-list/2019-March/msg01147.html

For cleaning things up I think this makes sense, and I understood the above as
Daniel not being convinced because there was no reasoning behind that at all (no
commit message, etc.), hopefully I am not mistaken.

Martin

>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virQEMUDriverPtr parameters clean up in function qemuExtTPMStart() in /src/qemu/qemu_tpm.c
Posted by Daniel P. Berrangé 5 years, 1 month ago 
On Mon, Mar 25, 2019 at 04:12:34PM +0100, Martin Kletzander wrote:
> On Mon, Mar 25, 2019 at 10:03:31AM +0100, Peter Krempa wrote:
> > On Mon, Mar 25, 2019 at 09:15:23 +0100, Erik Skultety wrote:
> > > On Mon, Mar 25, 2019 at 09:14:38AM +0100, Erik Skultety wrote:
> > > > You're missing:
> > > > - commit message explaining the change
> > > > - Your full name as author
> > > > - compliance with developer certificate of origin, see [1]
> > 
> > https://www.redhat.com/archives/libvir-list/2019-March/msg01148.html
> > 
> > > >
> > > > On Sat, Mar 23, 2019 at 08:34:42PM +0400, Humaid wrote:
> > > > > ---
> > > > >  src/qemu/qemu_tpm.c | 6 +++---
> > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
> > > > > index 835a9caf46..b60e443f14 100644
> > > > > --- a/src/qemu/qemu_tpm.c
> > > > > +++ b/src/qemu/qemu_tpm.c
> > > > > @@ -834,16 +834,16 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver,
> > > > >
> > > > >
> > > > >  int
> > > > > -qemuExtTPMStart(virQEMUDriverPtr driver,
> > > > > -                virDomainObjPtr vm,
> > > > > +qemuExtTPMStart(virDomainObjPtr vm,
> > > > >                  qemuDomainLogContextPtr logCtxt)
> > > > >  {
> > > > >      int ret = 0;
> > > > > +    qemuDomainObjPrivatePtr priv = vm->privateData;
> > > >
> > > > A quick grep through the code base shows that we could do this at many more
> > > > places actually.
> > 
> > Daniel pointed out that it's not actually worth doing as a separate
> > cleanup:
> > 
> > https://www.redhat.com/archives/libvir-list/2019-March/msg01147.html
> 
> For cleaning things up I think this makes sense, and I understood the above as
> Daniel not being convinced because there was no reasoning behind that at all (no
> commit message, etc.), hopefully I am not mistaken.

No, I was saying I didn't see any point in doing this change. I don't
think it is a benefit to reduce the parameter count in exchange for
increasing the local variable count. This just feels like repainting
the bikeshed a different colour.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virQEMUDriverPtr parameters clean up in function qemuExtTPMStart() in /src/qemu/qemu_tpm.c
Posted by Martin Kletzander 5 years, 1 month ago 
On Mon, Mar 25, 2019 at 03:48:59PM +0000, Daniel P. Berrangé wrote:
>On Mon, Mar 25, 2019 at 04:12:34PM +0100, Martin Kletzander wrote:
>> On Mon, Mar 25, 2019 at 10:03:31AM +0100, Peter Krempa wrote:
>> > On Mon, Mar 25, 2019 at 09:15:23 +0100, Erik Skultety wrote:
>> > > On Mon, Mar 25, 2019 at 09:14:38AM +0100, Erik Skultety wrote:
>> > > > You're missing:
>> > > > - commit message explaining the change
>> > > > - Your full name as author
>> > > > - compliance with developer certificate of origin, see [1]
>> >
>> > https://www.redhat.com/archives/libvir-list/2019-March/msg01148.html
>> >
>> > > >
>> > > > On Sat, Mar 23, 2019 at 08:34:42PM +0400, Humaid wrote:
>> > > > > ---
>> > > > >  src/qemu/qemu_tpm.c | 6 +++---
>> > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
>> > > > >
>> > > > > diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
>> > > > > index 835a9caf46..b60e443f14 100644
>> > > > > --- a/src/qemu/qemu_tpm.c
>> > > > > +++ b/src/qemu/qemu_tpm.c
>> > > > > @@ -834,16 +834,16 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver,
>> > > > >
>> > > > >
>> > > > >  int
>> > > > > -qemuExtTPMStart(virQEMUDriverPtr driver,
>> > > > > -                virDomainObjPtr vm,
>> > > > > +qemuExtTPMStart(virDomainObjPtr vm,
>> > > > >                  qemuDomainLogContextPtr logCtxt)
>> > > > >  {
>> > > > >      int ret = 0;
>> > > > > +    qemuDomainObjPrivatePtr priv = vm->privateData;
>> > > >
>> > > > A quick grep through the code base shows that we could do this at many more
>> > > > places actually.
>> >
>> > Daniel pointed out that it's not actually worth doing as a separate
>> > cleanup:
>> >
>> > https://www.redhat.com/archives/libvir-list/2019-March/msg01147.html
>>
>> For cleaning things up I think this makes sense, and I understood the above as
>> Daniel not being convinced because there was no reasoning behind that at all (no
>> commit message, etc.), hopefully I am not mistaken.
>
>No, I was saying I didn't see any point in doing this change. I don't
>think it is a benefit to reduce the parameter count in exchange for
>increasing the local variable count. This just feels like repainting
>the bikeshed a different colour.
>

OK, sorry for the misunderstanding then.  Although it starts to make sense when
the parameters bubble through more than one function, the function calls more functions like this and/or the function does not need the driver pointer at all.  The code would be more concise.

Feel free to disagree and let me know if I should remove this from the bite
sized tasks on the wiki.  Pity we didn't reach that conclusion earlier then.

>Regards,
>Daniel
>-- 
>|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
>|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
>|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virQEMUDriverPtr parameters clean up in function qemuExtTPMStart() in /src/qemu/qemu_tpm.c
Posted by Daniel P. Berrangé 5 years, 1 month ago 
On Tue, Mar 26, 2019 at 03:40:12PM +0100, Martin Kletzander wrote:
> On Mon, Mar 25, 2019 at 03:48:59PM +0000, Daniel P. Berrangé wrote:
> > On Mon, Mar 25, 2019 at 04:12:34PM +0100, Martin Kletzander wrote:
> > > On Mon, Mar 25, 2019 at 10:03:31AM +0100, Peter Krempa wrote:
> > > > On Mon, Mar 25, 2019 at 09:15:23 +0100, Erik Skultety wrote:
> > > > > On Mon, Mar 25, 2019 at 09:14:38AM +0100, Erik Skultety wrote:
> > > > > > You're missing:
> > > > > > - commit message explaining the change
> > > > > > - Your full name as author
> > > > > > - compliance with developer certificate of origin, see [1]
> > > >
> > > > https://www.redhat.com/archives/libvir-list/2019-March/msg01148.html
> > > >
> > > > > >
> > > > > > On Sat, Mar 23, 2019 at 08:34:42PM +0400, Humaid wrote:
> > > > > > > ---
> > > > > > >  src/qemu/qemu_tpm.c | 6 +++---
> > > > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > > >
> > > > > > > diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
> > > > > > > index 835a9caf46..b60e443f14 100644
> > > > > > > --- a/src/qemu/qemu_tpm.c
> > > > > > > +++ b/src/qemu/qemu_tpm.c
> > > > > > > @@ -834,16 +834,16 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver,
> > > > > > >
> > > > > > >
> > > > > > >  int
> > > > > > > -qemuExtTPMStart(virQEMUDriverPtr driver,
> > > > > > > -                virDomainObjPtr vm,
> > > > > > > +qemuExtTPMStart(virDomainObjPtr vm,
> > > > > > >                  qemuDomainLogContextPtr logCtxt)
> > > > > > >  {
> > > > > > >      int ret = 0;
> > > > > > > +    qemuDomainObjPrivatePtr priv = vm->privateData;
> > > > > >
> > > > > > A quick grep through the code base shows that we could do this at many more
> > > > > > places actually.
> > > >
> > > > Daniel pointed out that it's not actually worth doing as a separate
> > > > cleanup:
> > > >
> > > > https://www.redhat.com/archives/libvir-list/2019-March/msg01147.html
> > > 
> > > For cleaning things up I think this makes sense, and I understood the above as
> > > Daniel not being convinced because there was no reasoning behind that at all (no
> > > commit message, etc.), hopefully I am not mistaken.
> > 
> > No, I was saying I didn't see any point in doing this change. I don't
> > think it is a benefit to reduce the parameter count in exchange for
> > increasing the local variable count. This just feels like repainting
> > the bikeshed a different colour.
> > 
> 
> OK, sorry for the misunderstanding then.  Although it starts to make sense when
> the parameters bubble through more than one function, the function calls more functions like this and/or the function does not need the driver pointer at all.  The code would be more concise.
> 
> Feel free to disagree and let me know if I should remove this from the bite
> sized tasks on the wiki.  Pity we didn't reach that conclusion earlier then.

I don't feel strongly enough to nack the patch, but equally I wouldn't
encourage people to do more of it as I think there's more useful changes
they could spend time on.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virQEMUDriverPtr parameters clean up in function qemuExtTPMStart() in /src/qemu/qemu_tpm.c
Posted by Ján Tomko 5 years, 1 month ago 
On Sat, Mar 23, 2019 at 08:34:42PM +0400, Humaid wrote:
>---
> src/qemu/qemu_tpm.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
>index 835a9caf46..b60e443f14 100644
>--- a/src/qemu/qemu_tpm.c
>+++ b/src/qemu/qemu_tpm.c
>@@ -834,16 +834,16 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver,
>
>
> int
>-qemuExtTPMStart(virQEMUDriverPtr driver,
>-                virDomainObjPtr vm,
>+qemuExtTPMStart(virDomainObjPtr vm,
>                 qemuDomainLogContextPtr logCtxt)

This is not a static function, so you also need to change its prototype
in the header file to get this to compile.

> {
>     int ret = 0;
>+    qemuDomainObjPrivatePtr priv = vm->privateData;
>     virDomainTPMDefPtr tpm = vm->def->tpm;
>
>     switch (tpm->type) {
>     case VIR_DOMAIN_TPM_TYPE_EMULATOR:
>-        ret = qemuExtTPMStartEmulator(driver, vm, logCtxt);
>+        ret = qemuExtTPMStartEmulator(priv->driver, vm, logCtxt);

It's not necessary to pass 'driver' to qemuExtTPMStartEmulator either
since we're already passing 'vm'.

Jano

>         break;
>     case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
>     case VIR_DOMAIN_TPM_TYPE_LAST:
>-- 
>2.17.1
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.