[{"id":28628,"web_url":"https://patchwork.libcamera.org/comment/28628/","msgid":"<20240201075838.GA11551@pendragon.ideasonboard.com>","date":"2024-02-01T07:58:38","subject":"Re: [PATCH 1/2] media_device: Add bool return type to unlock()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello Khem,\n\nThank you for the patch.\n\nOn Wed, Jan 31, 2024 at 09:08:09PM -0800, Khem Raj wrote:\n> unlock uses lockf which is marked with __attribute__\n\ns/unlock/MediaDevice::unlock()/\n\n> ((warn_unused_result)) and compilers warn about it and some treat\n> -Wunused-result as error with -Werror turned on, It would be good to\n> check if lockf failed or succeeded, however, that piece is not changed\n> with this, this fixes build with clang++ 18\n\nI wouldn't mention clang in the commit message, it's not a\ncompiler-specific issue, all compilers will warn about the unused\nresult. Where does that unused result attribute come from though ? I\ndon't see it in glibc :\nhttps://sourceware.org/git/?p=glibc.git;a=blob;f=io/fcntl.h;h=9cee0b5900cb4f7c3bd94344e17824c843815e3d;hb=HEAD#l274\n\n> \n> ../git/src/libcamera/media_device.cpp:167:2: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]\n> 167 | lockf(fd_.get(), F_ULOCK, 0);\n> | ^~~~~ ~~~~~~~~~~~~~~~~~~~~~\n> 1 error generated.\n> \n> Signed-off-by: Khem Raj \n> ---\n> include/libcamera/internal/media_device.h | 2 +-\n> src/libcamera/media_device.cpp | 6 +++---\n> 2 files changed, 4 insertions(+), 4 deletions(-)\n> \n> diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h\n> index eb8cfde4..b09dfd16 100644\n> --- a/include/libcamera/internal/media_device.h\n> +++ b/include/libcamera/internal/media_device.h\n> @@ -33,7 +33,7 @@ public:\n> \tbool busy() const { return acquired_; }\n> \n> \tbool lock();\n> -\tvoid unlock();\n> +\tbool unlock();\n> \n> \tint populate();\n> \tbool isValid() const { return valid_; }\n> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> index 2949816b..eaa2fdb0 100644\n> --- a/src/libcamera/media_device.cpp\n> +++ b/src/libcamera/media_device.cpp\n> @@ -159,12 +159,12 @@ bool MediaDevice::lock()\n> *\n> * \\sa lock()\n> */\n> -void MediaDevice::unlock()\n> +bool MediaDevice::unlock()\n> {\n> \tif (!fd_.isValid())\n> -\t\treturn;\n> +\t\treturn false;\n> \n> -\tlockf(fd_.get(), F_ULOCK, 0);\n> +\treturn lockf(fd_.get(), F_ULOCK, 0) == 0;\n\nIf you don't check the return value in the callers this is just a hack\nto silence the warning, in which case you may as well silence it here\nwithout changing the return value.\n\n> }\n> \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 C5559BDB13\n\tfor ;\n\tThu, 1 Feb 2024 07:58:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1170962805;\n\tThu, 1 Feb 2024 08:58:41 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0745E61D0D\n\tfor ;\n\tThu, 1 Feb 2024 08:58:39 +0100 (CET)","from pendragon.ideasonboard.com (unknown [94.107.229.70])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 361654AB0;\n\tThu, 1 Feb 2024 08:57:19 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"KuAf4XG/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1706774239;\n\tbh=ceCTCBpYHC7DPZs6CBhTW3EJbF6WGMUEyZ3iVADppb4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KuAf4XG/yZSNwmyRG5Lv7FMGHxsH35kS6xkLWM47vyNm2kzDjWYV8tkWUjaWNSCGV\n\tJs/gOD2PaPFZduicmQBow1PZdtog4FJCmlAhVErsfht/WdiVd6pG4Nz/Bvf8SsvpIW\n\t42bQzgyqREeGw2ZoedcVlYB0H26sXBqHgRkFy0GI=","Date":"Thu, 1 Feb 2024 09:58:38 +0200","From":"Laurent Pinchart ","To":"Khem Raj ","Subject":"Re: [PATCH 1/2] media_device: Add bool return type to unlock()","Message-ID":"<20240201075838.GA11551@pendragon.ideasonboard.com>","References":"<20240201050810.3501276-1-raj.khem@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240201050810.3501276-1-raj.khem@gmail.com>","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\" "}}]