Re: [Spice-devel] patch[1/1] fix a memory leak in qxl_screen_init
On 03/26/2014 11:30 AM, longguang.yue wrote: please forgive my ignore. jwhite you are right, i post a new patch as you suggest. ACKed (and pushed) Thanks! Jeremy thanks At 2013-11-19 20:21:35,Marian Krcmarik mkrcm...@redhat.com wrote: ping.. bigclouds, are you willing to finish the patch and address Jeremy's comment? It's imo worthy to fix and It would be nice If you can finish that. - Original Message - From: Jeremy White jwh...@codeweavers.com To: bigclouds bigclo...@163.com Cc: spice-devel@lists.freedesktop.org Sent: Wednesday, November 6, 2013 4:28:45 PM Subject: Re: [Spice-devel] patch[1/1] fix a memory leak in qxl_screen_init On 11/06/2013 08:48 AM, bigclouds wrote: it is needed to detect the return of qxl_uxa_init in qxl_screen_init . I don't think we have understood each other. There is a duplicate allocation; you are trying to fix that. As I understand it, your patch removes the allocation in qxl_uxa_init and leaves only the one in qxl_screen_init. I said that it seems to me it would be better to remove the allocation in qxl_screen_init and leave only the one in qxl_uxa_init. If you're concerned about error conditions propagating, you can change qxl_driver to check the return status of qxl_uxa_init. Cheers, Jeremy At 2013-11-06 21:59:58,Jeremy White jwh...@codeweavers.com wrote: Nice catch! On 11/06/2013 03:37 AM, bigclouds wrote: hi, it allocate twice memory for qxl-uxa in function qxl_screen_init and qxl_uxa_init - diff --git a/src/qxl_driver.c b/src/qxl_driver.c index 91ba6c2..6be61e4 100644 --- a/src/qxl_driver.c +++ b/src/qxl_driver.c @@ -746,7 +746,9 @@ qxl_screen_init (SCREEN_INIT_ARGS_DECL) } qxl-uxa = uxa_driver_alloc (); - +if (qxl-uxa == NULL) +return FALSE; + Wouldn't it be better to just delete this instance of the allocation and leave it all in qxl_uxa.c? Also, just a kibitz, but most open source projects require a full name on a submitted patch. Cheers, Jeremy ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] patch[1/1] fix a memory leak in qxl_screen_init
On 03/26/2014 11:30 AM, longguang.yue wrote: please forgive my ignore. jwhite you are right, i post a new patch as you suggest. Yes, this patch does look correct. I have been meaning to get time to put it through some further testing, and then push it. I hope to do that this week. Cheers, Jeremy ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] patch[1/1] fix a memory leak in qxl_screen_init
please forgive my ignore. jwhite you are right, i post a new patch as you suggest. thanks At 2013-11-19 20:21:35,Marian Krcmarik mkrcm...@redhat.com wrote: ping.. bigclouds, are you willing to finish the patch and address Jeremy's comment? It's imo worthy to fix and It would be nice If you can finish that. - Original Message - From: Jeremy White jwh...@codeweavers.com To: bigclouds bigclo...@163.com Cc: spice-devel@lists.freedesktop.org Sent: Wednesday, November 6, 2013 4:28:45 PM Subject: Re: [Spice-devel] patch[1/1] fix a memory leak in qxl_screen_init On 11/06/2013 08:48 AM, bigclouds wrote: it is needed to detect the return of qxl_uxa_init in qxl_screen_init . I don't think we have understood each other. There is a duplicate allocation; you are trying to fix that. As I understand it, your patch removes the allocation in qxl_uxa_init and leaves only the one in qxl_screen_init. I said that it seems to me it would be better to remove the allocation in qxl_screen_init and leave only the one in qxl_uxa_init. If you're concerned about error conditions propagating, you can change qxl_driver to check the return status of qxl_uxa_init. Cheers, Jeremy At 2013-11-06 21:59:58,Jeremy White jwh...@codeweavers.com wrote: Nice catch! On 11/06/2013 03:37 AM, bigclouds wrote: hi, it allocate twice memory for qxl-uxa in function qxl_screen_init and qxl_uxa_init - diff --git a/src/qxl_driver.c b/src/qxl_driver.c index 91ba6c2..6be61e4 100644 --- a/src/qxl_driver.c +++ b/src/qxl_driver.c @@ -746,7 +746,9 @@ qxl_screen_init (SCREEN_INIT_ARGS_DECL) } qxl-uxa = uxa_driver_alloc (); - +if (qxl-uxa == NULL) +return FALSE; + Wouldn't it be better to just delete this instance of the allocation and leave it all in qxl_uxa.c? Also, just a kibitz, but most open source projects require a full name on a submitted patch. Cheers, Jeremy ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel 0001-fix-memory-leak-when-alloc-uxa.patch Description: Binary data ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] patch[1/1] fix a memory leak in qxl_screen_init
ping.. bigclouds, are you willing to finish the patch and address Jeremy's comment? It's imo worthy to fix and It would be nice If you can finish that. - Original Message - From: Jeremy White jwh...@codeweavers.com To: bigclouds bigclo...@163.com Cc: spice-devel@lists.freedesktop.org Sent: Wednesday, November 6, 2013 4:28:45 PM Subject: Re: [Spice-devel] patch[1/1] fix a memory leak in qxl_screen_init On 11/06/2013 08:48 AM, bigclouds wrote: it is needed to detect the return of qxl_uxa_init in qxl_screen_init . I don't think we have understood each other. There is a duplicate allocation; you are trying to fix that. As I understand it, your patch removes the allocation in qxl_uxa_init and leaves only the one in qxl_screen_init. I said that it seems to me it would be better to remove the allocation in qxl_screen_init and leave only the one in qxl_uxa_init. If you're concerned about error conditions propagating, you can change qxl_driver to check the return status of qxl_uxa_init. Cheers, Jeremy At 2013-11-06 21:59:58,Jeremy White jwh...@codeweavers.com wrote: Nice catch! On 11/06/2013 03:37 AM, bigclouds wrote: hi, it allocate twice memory for qxl-uxa in function qxl_screen_init and qxl_uxa_init - diff --git a/src/qxl_driver.c b/src/qxl_driver.c index 91ba6c2..6be61e4 100644 --- a/src/qxl_driver.c +++ b/src/qxl_driver.c @@ -746,7 +746,9 @@ qxl_screen_init (SCREEN_INIT_ARGS_DECL) } qxl-uxa = uxa_driver_alloc (); - +if (qxl-uxa == NULL) +return FALSE; + Wouldn't it be better to just delete this instance of the allocation and leave it all in qxl_uxa.c? Also, just a kibitz, but most open source projects require a full name on a submitted patch. Cheers, Jeremy ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] patch[1/1] fix a memory leak in qxl_screen_init
it is needed to detect the return of qxl_uxa_init in qxl_screen_init . At 2013-11-06 21:59:58,Jeremy White jwh...@codeweavers.com wrote: Nice catch! On 11/06/2013 03:37 AM, bigclouds wrote: hi, it allocate twice memory for qxl-uxa in function qxl_screen_init and qxl_uxa_init - diff --git a/src/qxl_driver.c b/src/qxl_driver.c index 91ba6c2..6be61e4 100644 --- a/src/qxl_driver.c +++ b/src/qxl_driver.c @@ -746,7 +746,9 @@ qxl_screen_init (SCREEN_INIT_ARGS_DECL) } qxl-uxa = uxa_driver_alloc (); - +if (qxl-uxa == NULL) +return FALSE; + Wouldn't it be better to just delete this instance of the allocation and leave it all in qxl_uxa.c? Also, just a kibitz, but most open source projects require a full name on a submitted patch. Cheers, Jeremy ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
