[libvirt] [PATCH] virQEMUDriverPtr parameters clean up in function qemuExtTPMStart() in /src/qemu/qemu_tpm.c
--- 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
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
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
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
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
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 :|
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 :|
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 :|
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
participants (6)
-
Daniel P. Berrangé -
Erik Skultety -
Humaid -
Ján Tomko -
Martin Kletzander -
Peter Krempa