[{"id":20096,"web_url":"https://patchwork.libcamera.org/comment/20096/","msgid":"","date":"2021-10-07T10:35:19","subject":"Re: [libcamera-devel] [PATCH 1/2] android: mm: Null check for\n\tCameraBufferManager","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang, thank you for the patch.\n\nOn Thu, Oct 7, 2021 at 6:39 PM Umang Jain wrote:\n>\n> cros::CameraBufferManager can be nullptr if there is an error in\n> its creation. Place a null-check guard to check it.\n>\n> Signed-off-by: Umang Jain \n> ---\n> src/android/mm/cros_camera_buffer.cpp | 5 +++++\n> 1 file changed, 5 insertions(+)\n>\n> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> index 86770135..97a04c68 100644\n> --- a/src/android/mm/cros_camera_buffer.cpp\n> +++ b/src/android/mm/cros_camera_buffer.cpp\n> @@ -60,6 +60,11 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> registered_(false)\n> {\n> bufferManager_ = cros::CameraBufferManager::GetInstance();\n> + if (!bufferManager_) {\n> + LOG(HAL, Error)\n\nI wonder if this should be Fatal.\nI would like to ask others' opinions.\n\nIndeed it is not a big deal.\n\nReviewed-by: Hirokazu Honda \n-Hiro\n> + << \"Failed to get cros CameraBufferManager instance\";\n> + return;\n> + }\n>\n> int ret = bufferManager_->Register(camera3Buffer);\n> if (ret) {\n> --\n> 2.31.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 56B09C323E\n\tfor ;\n\tThu, 7 Oct 2021 10:35:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A9D61691BA;\n\tThu, 7 Oct 2021 12:35:32 +0200 (CEST)","from mail-ed1-x52b.google.com (mail-ed1-x52b.google.com\n\t[IPv6:2a00:1450:4864:20::52b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D4F2A691AD\n\tfor ;\n\tThu, 7 Oct 2021 12:35:30 +0200 (CEST)","by mail-ed1-x52b.google.com with SMTP id l7so21526476edq.3\n\tfor ;\n\tThu, 07 Oct 2021 03:35:30 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"fnruHMP2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=Cd83545FVPI8vQrtyzFBC0McT0ytjakrIca6XK8kmCI=;\n\tb=fnruHMP2ieh48X7k6iIbYD2mi4bdpxyKpDZ38ofgYwWoyjTBWPUwAyjAwgGwkWe8FX\n\t4cfCRAzKgDs0drv3oqMbnBppqE2X+GQn6RNsbWJA8z9Pu8QbquGA3mcfGV+uO6ZIOfXk\n\tXcxnHL26dliFtrwkREx9+ncQTHrDXE0m+vVyY=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=Cd83545FVPI8vQrtyzFBC0McT0ytjakrIca6XK8kmCI=;\n\tb=4M8uaGCgIvH5/RMwmCglNFVQSWBcDa+xxHhPVhbVatKP7xgRHHWajJ8Pj+4iKn8vfK\n\tU699xD97o/dhE78+U5ahk0VK55DDdnTsTbM80tXW9xVpUY74NC7L2LMjyJEnSwCD/Jt/\n\tVNxv2x/arJLTZVrCEfFJTosv8dZ/aQn0Q3tggO+bhb7uGpo/NdTQgWleKZasuLeUx9Nk\n\tTgE7n4ubKWmtjgoCghjGpDm0bEhA+xrNySumkeFdHjukpifYDpViOAEWCoLmRJIY5CHa\n\t1gR3VbDWH7JQ+YUfoIH4C9mj7IsAO6+h65zE1Bph9zztMk7jfZ2pzZKybR6l2m9kgJ64\n\twPzw==","X-Gm-Message-State":"AOAM532wEVErMfkeHbV48N5MOvV50/1eNDWx4YxiJDVosJvZh8TKfNJc\n\t4ou6+62TaxApI6xUjmCcmMzcms8ZJI8hXWBG2dEwHg==","X-Google-Smtp-Source":"ABdhPJw+Zk3I9xoNMtaS9bqRoZvIk4NalL52d/PDgKIt22qoP9HPlTwuV91WOK/rsF7pscEdVVilMKGNu0FaFaFS7fk=","X-Received":"by 2002:a17:906:5801:: with SMTP id\n\tm1mr4652420ejq.296.1633602930057; \n\tThu, 07 Oct 2021 03:35:30 -0700 (PDT)","MIME-Version":"1.0","References":"<20211007093937.121357-1-umang.jain@ideasonboard.com>","In-Reply-To":"<20211007093937.121357-1-umang.jain@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Thu, 7 Oct 2021 19:35:19 +0900","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 1/2] android: mm: Null check for\n\tCameraBufferManager","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20105,"web_url":"https://patchwork.libcamera.org/comment/20105/","msgid":"<20211011102301.7mvtaq4vb7nwhy57@uno.localdomain>","date":"2021-10-11T10:23:01","subject":"Re: [libcamera-devel] [PATCH 1/2] android: mm: Null check for\n\tCameraBufferManager","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Thu, Oct 07, 2021 at 03:09:36PM +0530, Umang Jain wrote:\n> cros::CameraBufferManager can be nullptr if there is an error in\n> its creation. Place a null-check guard to check it.\n>\n> Signed-off-by: Umang Jain \n> ---\n> src/android/mm/cros_camera_buffer.cpp | 5 +++++\n> 1 file changed, 5 insertions(+)\n>\n> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> index 86770135..97a04c68 100644\n> --- a/src/android/mm/cros_camera_buffer.cpp\n> +++ b/src/android/mm/cros_camera_buffer.cpp\n> @@ -60,6 +60,11 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> \t registered_(false)\n> {\n> \tbufferManager_ = cros::CameraBufferManager::GetInstance();\n> +\tif (!bufferManager_) {\n> +\t\tLOG(HAL, Error)\n> +\t\t\t<< \"Failed to get cros CameraBufferManager instance\";\n> +\t\treturn;\n> +\t}\n\nI'm not sure if this could happen or is just a \"just in case\".\nAnyway, the HAL won't be able to operate in that case and this is a\nconstructor so it's impossible to propagate the error up to handle it\ngracefully: I would use Fatal.\n\n>\n> \tint ret = bufferManager_->Register(camera3Buffer);\n> \tif (ret) {\n> --\n> 2.31.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 18704BDC71\n\tfor ;\n\tMon, 11 Oct 2021 10:22:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9338768F4C;\n\tMon, 11 Oct 2021 12:22:14 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E39116012B\n\tfor ;\n\tMon, 11 Oct 2021 12:22:12 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 6849820005;\n\tMon, 11 Oct 2021 10:22:12 +0000 (UTC)"],"Date":"Mon, 11 Oct 2021 12:23:01 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20211011102301.7mvtaq4vb7nwhy57@uno.localdomain>","References":"<20211007093937.121357-1-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211007093937.121357-1-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/2] android: mm: Null check for\n\tCameraBufferManager","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20108,"web_url":"https://patchwork.libcamera.org/comment/20108/","msgid":"","date":"2021-10-11T12:06:49","subject":"Re: [libcamera-devel] [PATCH 1/2] android: mm: Null check for\n\tCameraBufferManager","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 10/11/21 3:53 PM, Jacopo Mondi wrote:\n> Hi Umang,\n>\n> On Thu, Oct 07, 2021 at 03:09:36PM +0530, Umang Jain wrote:\n>> cros::CameraBufferManager can be nullptr if there is an error in\n>> its creation. Place a null-check guard to check it.\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/android/mm/cros_camera_buffer.cpp | 5 +++++\n>> 1 file changed, 5 insertions(+)\n>>\n>> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n>> index 86770135..97a04c68 100644\n>> --- a/src/android/mm/cros_camera_buffer.cpp\n>> +++ b/src/android/mm/cros_camera_buffer.cpp\n>> @@ -60,6 +60,11 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n>> \t registered_(false)\n>> {\n>> \tbufferManager_ = cros::CameraBufferManager::GetInstance();\n>> +\tif (!bufferManager_) {\n>> +\t\tLOG(HAL, Error)\n>> +\t\t\t<< \"Failed to get cros CameraBufferManager instance\";\n>> +\t\treturn;\n>> +\t}\n> I'm not sure if this could happen or is just a \"just in case\".\n> Anyway, the HAL won't be able to operate in that case and this is a\n> constructor so it's impossible to propagate the error up to handle it\n> gracefully: I would use Fatal.\n\n\nThe \"just in case\" scenario is well documented in the framework\n\n       // Gets the singleton instance.  Returns nullptr if any error \noccurrs during\n       // instance creation.\n\nin \n$chromiumos/src/platform2/camera/include/cros-camera/camera_buffer_manager.h \n\n\nI haven't been able to trigger the error condition, but since it's \ndocument it's better to guard it beforehand.\n\nFatal sounds good to me too.\n\n>\n>> \tint ret = bufferManager_->Register(camera3Buffer);\n>> \tif (ret) {\n>> --\n>> 2.31.1\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8A72DBDC71\n\tfor ;\n\tMon, 11 Oct 2021 12:06:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E999C6027F;\n\tMon, 11 Oct 2021 14:06:56 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 233B96012B\n\tfor ;\n\tMon, 11 Oct 2021 14:06:55 +0200 (CEST)","from [192.168.1.106] (unknown [103.251.226.107])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2E4CC2BD;\n\tMon, 11 Oct 2021 14:06:53 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"GEfltDgT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1633954014;\n\tbh=E6RmrzvUXSlCjQVnhtApy/odszmCF3HI++PiYJczx/Q=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=GEfltDgTqvegPfhsPR0M6X1KESi6Nq4WI8DTseFDBywI7LlO8PJCS4lGm36U/6KTS\n\t3w1egEDZkfQ+SDQSa3N/xh6H7By9JfZK3vPTa0VIe0XhTbDbGxf+kvZ36fPfYAVlCW\n\tUuQQF/c6SKEg+k3LijO5ykuRUyic4gNIb0ylehrU=","To":"Jacopo Mondi ","References":"<20211007093937.121357-1-umang.jain@ideasonboard.com>\n\t<20211011102301.7mvtaq4vb7nwhy57@uno.localdomain>","From":"Umang Jain ","Message-ID":"","Date":"Mon, 11 Oct 2021 17:36:49 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20211011102301.7mvtaq4vb7nwhy57@uno.localdomain>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 1/2] android: mm: Null check for\n\tCameraBufferManager","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20161,"web_url":"https://patchwork.libcamera.org/comment/20161/","msgid":"","date":"2021-10-13T01:50:37","subject":"Re: [libcamera-devel] [PATCH 1/2] android: mm: Null check for\n\tCameraBufferManager","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Mon, Oct 11, 2021 at 05:36:49PM +0530, Umang Jain wrote:\n> On 10/11/21 3:53 PM, Jacopo Mondi wrote:\n> > On Thu, Oct 07, 2021 at 03:09:36PM +0530, Umang Jain wrote:\n> >> cros::CameraBufferManager can be nullptr if there is an error in\n> >> its creation. Place a null-check guard to check it.\n> >>\n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> src/android/mm/cros_camera_buffer.cpp | 5 +++++\n> >> 1 file changed, 5 insertions(+)\n> >>\n> >> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> >> index 86770135..97a04c68 100644\n> >> --- a/src/android/mm/cros_camera_buffer.cpp\n> >> +++ b/src/android/mm/cros_camera_buffer.cpp\n> >> @@ -60,6 +60,11 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> >> \t registered_(false)\n> >> {\n> >> \tbufferManager_ = cros::CameraBufferManager::GetInstance();\n> >> +\tif (!bufferManager_) {\n> >> +\t\tLOG(HAL, Error)\n> >> +\t\t\t<< \"Failed to get cros CameraBufferManager instance\";\n> >> +\t\treturn;\n> >> +\t}\n> > I'm not sure if this could happen or is just a \"just in case\".\n> > Anyway, the HAL won't be able to operate in that case and this is a\n> > constructor so it's impossible to propagate the error up to handle it\n> > gracefully: I would use Fatal.\n> \n> \n> The \"just in case\" scenario is well documented in the framework\n> \n>       // Gets the singleton instance.  Returns nullptr if any error \n> occurrs during\n>       // instance creation.\n> \n> in \n> $chromiumos/src/platform2/camera/include/cros-camera/camera_buffer_manager.h \n> \n> \n> I haven't been able to trigger the error condition, but since it's \n> document it's better to guard it beforehand.\n> \n> Fatal sounds good to me too.\n\nWith Fatal,\n\nReviewed-by: Laurent Pinchart \n\n\n> >> \tint ret = bufferManager_->Register(camera3Buffer);\n> >> \tif (ret) {","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 91E4FBDC71\n\tfor ;\n\tWed, 13 Oct 2021 01:50:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E737068F4F;\n\tWed, 13 Oct 2021 03:50:53 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3FA7A604FF\n\tfor ;\n\tWed, 13 Oct 2021 03:50:52 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9CE5F291;\n\tWed, 13 Oct 2021 03:50:51 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"n9JfkUxY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634089851;\n\tbh=qE3QC/Fqpo37drVUgv5RLu8MMSYEJHHKz7l6XGprPzI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=n9JfkUxY34IihYTq7vLdf0bbPL11/2CF/TEKw0YN7MGUdd8OPko7uixMBNJ7lPZ17\n\tCAEJt6xn9AkDsvva1KFYDyJAJz7zcrUobmwjI6baVapPzOzhTAWWiWwtkLC1oIjPNX\n\twHvIdDfrcrrRj6nJE+85wx0Pca4srpmvlWyfnucM=","Date":"Wed, 13 Oct 2021 04:50:37 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211007093937.121357-1-umang.jain@ideasonboard.com>\n\t<20211011102301.7mvtaq4vb7nwhy57@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/2] android: mm: Null check for\n\tCameraBufferManager","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]