[libvirt] [dbus PATCH] connect: fix g_free order in virtDBusConnectFree

Anya Harter posted 1 patch 5 years, 10 months ago
Failed in applying to current master (apply log)
src/connect.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[libvirt] [dbus PATCH] connect: fix g_free order in virtDBusConnectFree
Posted by Anya Harter 5 years, 10 months ago
so that g_free(connect->nodeDevPath) line appears in alphabetical order
with the rest of the lines

Signed-off-by: Anya Harter <aharter@redhat.com>
---
 src/connect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/connect.c b/src/connect.c
index 9ebceaa..9275121 100644
--- a/src/connect.c
+++ b/src/connect.c
@@ -1808,9 +1808,9 @@ virtDBusConnectFree(virtDBusConnect *connect)
     if (connect->connection)
         virtDBusConnectClose(connect, TRUE);
 
-    g_free(connect->nodeDevPath);
     g_free(connect->domainPath);
     g_free(connect->networkPath);
+    g_free(connect->nodeDevPath);
     g_free(connect->nwfilterPath);
     g_free(connect->secretPath);
     g_free(connect->storagePoolPath);
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [dbus PATCH] connect: fix g_free order in virtDBusConnectFree
Posted by Pavel Hrdina 5 years, 9 months ago
On Mon, Jul 02, 2018 at 11:42:08AM -0400, Anya Harter wrote:
> so that g_free(connect->nodeDevPath) line appears in alphabetical order
> with the rest of the lines
> 
> Signed-off-by: Anya Harter <aharter@redhat.com>
> ---
>  src/connect.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

In general we don't send/merge patches that just move code without any
functional reason because it pollutes git history but in this case I
would say it's ok.

Reviewed-by: Pavel Hrdina <phrdina@redhat.com> and pushed.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [dbus PATCH] connect: fix g_free order in virtDBusConnectFree
Posted by Ján Tomko 5 years, 9 months ago
On Tue, Jul 03, 2018 at 02:18:02PM +0200, Pavel Hrdina wrote:
>On Mon, Jul 02, 2018 at 11:42:08AM -0400, Anya Harter wrote:
>> so that g_free(connect->nodeDevPath) line appears in alphabetical order
>> with the rest of the lines
>>
>> Signed-off-by: Anya Harter <aharter@redhat.com>
>> ---
>>  src/connect.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
>In general we don't send/merge patches that just move code without any
>functional reason because it pollutes git history but in this case I
>would say it's ok.
>

What about the gchar *nodeDevPath; declaration in connect.h?
It is also in a different order.

/me runs

Jano

>Reviewed-by: Pavel Hrdina <phrdina@redhat.com> and pushed.



>--
>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] [dbus PATCH] connect: fix g_free order in virtDBusConnectFree
Posted by Anya Harter 5 years, 9 months ago

On 07/03/2018 09:03 AM, Ján Tomko wrote:
> On Tue, Jul 03, 2018 at 02:18:02PM +0200, Pavel Hrdina wrote:
>> On Mon, Jul 02, 2018 at 11:42:08AM -0400, Anya Harter wrote:
>>> so that g_free(connect->nodeDevPath) line appears in alphabetical order
>>> with the rest of the lines
>>>
>>> Signed-off-by: Anya Harter <aharter@redhat.com>
>>> ---
>>>  src/connect.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> In general we don't send/merge patches that just move code without any
>> functional reason because it pollutes git history but in this case I
>> would say it's ok.
>>
> 
> What about the gchar *nodeDevPath; declaration in connect.h?
> It is also in a different order.

This is a good catch.

How would you do something that isn't functional as part of a different
commit? Wouldn't it confuse someone looking at it?

Thanks,

Anya

> 
> /me runs
> 
> Jano
> 
>> Reviewed-by: Pavel Hrdina <phrdina@redhat.com> and pushed.
> 
> 
> 
>> -- 
>> 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] [dbus PATCH] connect: fix g_free order in virtDBusConnectFree
Posted by Pavel Hrdina 5 years, 9 months ago
On Tue, Jul 03, 2018 at 03:57:29PM -0400, Anya Harter wrote:
> 
> 
> On 07/03/2018 09:03 AM, Ján Tomko wrote:
> > On Tue, Jul 03, 2018 at 02:18:02PM +0200, Pavel Hrdina wrote:
> >> On Mon, Jul 02, 2018 at 11:42:08AM -0400, Anya Harter wrote:
> >>> so that g_free(connect->nodeDevPath) line appears in alphabetical order
> >>> with the rest of the lines
> >>>
> >>> Signed-off-by: Anya Harter <aharter@redhat.com>
> >>> ---
> >>>  src/connect.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> In general we don't send/merge patches that just move code without any
> >> functional reason because it pollutes git history but in this case I
> >> would say it's ok.
> >>
> > 
> > What about the gchar *nodeDevPath; declaration in connect.h?
> > It is also in a different order.
> 
> This is a good catch.
> 
> How would you do something that isn't functional as part of a different
> commit? Wouldn't it confuse someone looking at it?

So if you need to move some code in order to introduce some functional
change than the move should be done in separate commit as a preparation
for the actual change.  In that case the move is justified and
desirable, but if the move is just to make things look nice it is not
usually good idea since it pollutes the git history.

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