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
--
|: