From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 792A04BEC4 for ; Thu, 18 Jul 2024 09:26:52 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 741DE68D97D; Thu, 18 Jul 2024 12:26:50 +0300 (EEST) Received: from iq.passwd.hu (iq.passwd.hu [217.27.212.140]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 967ED68D784 for ; Thu, 18 Jul 2024 12:26:43 +0300 (EEST) Received: from localhost (localhost [127.0.0.1]) by iq.passwd.hu (Postfix) with ESMTP id 2A029EAAC3 for ; Thu, 18 Jul 2024 11:26:43 +0200 (CEST) X-Virus-Scanned: amavisd-new at passwd.hu Received: from iq.passwd.hu ([127.0.0.1]) by localhost (iq.passwd.hu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id xelGNvL5lqDT for ; Thu, 18 Jul 2024 11:26:41 +0200 (CEST) Received: from iq (iq [217.27.212.140]) by iq.passwd.hu (Postfix) with ESMTPS id C981CEA964 for ; Thu, 18 Jul 2024 11:26:40 +0200 (CEST) Date: Thu, 18 Jul 2024 11:26:40 +0200 (CEST) From: Marton Balint To: FFmpeg development discussions and patches In-Reply-To: <20240716111543.2437488-1-ffmpeg-devel@pileofstuff.org> Message-ID: References: <9e16396a-4ef2-fdb3-fce8-94e19a141473@passwd.hu> <20240716111543.2437488-1-ffmpeg-devel@pileofstuff.org> MIME-Version: 1.0 Subject: Re: [FFmpeg-devel] [RFC PATCH v2 0/1] avutil/error: Provide better feedback about unknown error codes X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: On Tue, 16 Jul 2024, Andrew Sayers wrote: > I'm having trouble managing this conversation. On one hand, you've brought up > several important details that would need to be included in a new patch. > On the other hand, I'm pretty sure we're talking past each other on the big > problems, and need to start over. So let's fork the discussion. > > # First, let's haggle over some details > > The patch below fixes a number of small issues brought up by your comments... > > Error numbers are always expressed in the code as either uppercase hex numbers > or FourCCs (or ThreeCCs, but you get the point). This patch prints error codes > as hex, which is no less unintelligible for ordinary users, might make problems > easier to find on Google, and will sometimes make them easier to grep for. > > Having said that, this patch prints non-negative numbers in decimal, > because all bets are off if that manages to happen. > > A developer could create an error code that just happens to be valid ASCII. > In that situation, the previous patch would have printed something like > "Unrecognised error code \"~!X\"" occurred", which is worse than the current > behaviour. This patch includes both (hex) number and name in those messages. > > This patch adds "please report this bug" for all unknown error messages. > I'll cover the reasoning below, but the relevant detail is that the previous > patch just gave users a different heiroglyphic before abandoning them. > > # Second, let's talk about the big picture > > Consider the following scenario: > > 1. a new developer adds some code to FFmpeg that calls an existing function > 2. it turns out that function sometimes calls another function that > returns a variety of internal error codes (FFERROR_REDO among others) > 3. their testing uncovers a situation that intermittently returns an unknown > error number, but they don't notice there are two different numbers > 4. they spend a lot of time tracking down an error message based on a random > number, and eventually fix "the" bug (actually one of two intermittent bugs) > 5. the review doesn't catch the other bug, and the new code goes live > 6. a user trips over the other bug and sees "Error number occurred" > 7. the user wastes a lot of time trying to work out what they did wrong, > badmouthing FFmpeg to anyone who will listen as they do > 8. they eventually catch the attention of a developer > 9. that developer spends a lot of time bisecting the bug > 10. the new developer is expected to fix this patch, and feels like they're > to blame for the whole situation > > An error message like "Unrecognised error code \"REDO\" occurred, please report > this bug" would give the newbie a fighting chance to catch both bugs at step 3, > would make step 4 much shorter, and would optimise steps 7-10 to almost nothing. > > Catching this in a fate test would involve checking for an unknown function > returning an unknown number that gets reused in a context it's subtly > inappropriate for. I have no idea where to begin with that, and anyway it > wouldn't help a developer in the process of tracking down an intermittent bug. The fate test should be added for checking that all ffmpeg-specific errors (defined with AVERROR_ prefix in error.h) has a textual representation. That does not help the FFERROR_REDO case, but it does help if somebody adds a new AVERROR_xxx constant but forget to add the text counterpart for it. > > As mentioned above, the v2 patch adds "please report this bug" in a few places. > Any negative error code can be valid, but returning a raw error number is always > a bug, so it's all the same to users - if they see this message, they're not > expected to fix it themselves, they're expected to let us know. It is not necessarily a bug though. AVERROR values can be based on any system errno, and not all errno-s used by system libraries necessarily are supported by the platform strerrro_r() or our drop-in replacement if that is not available. I still feel like you are adding a lot of code for questionable benefit, so I suggest the following simple change: diff --git a/libavutil/error.c b/libavutil/error.c index 90bab7b9d3..f78c4b35b4 100644 --- a/libavutil/error.c +++ b/libavutil/error.c @@ -20,6 +20,7 @@ #define _XOPEN_SOURCE 600 /* XSI-compliant version of strerror_r */ #include #include +#include "avutil.h" #include "config.h" #include "avstring.h" #include "error.h" @@ -126,7 +127,7 @@ int av_strerror(int errnum, char *errbuf, size_t errbuf_size) ret = -1; #endif if (ret < 0) - snprintf(errbuf, errbuf_size, "Error number %d occurred", errnum); + snprintf(errbuf, errbuf_size, "Error number %d (%s) occurred", errnum, av_fourcc2str(-errnum)); } return ret; Regards, Marton _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".