From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 88FFB3858425 for ; Mon, 4 Sep 2023 16:40:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 88FFB3858425 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1693845629; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Bbe64Lp58sAwB41fQUaZBIXXnQS+Lb9mvDiPpA9GK0E=; b=EGw1omuMyDS6eTahU1q7Yxf09x/BISF4Ktf15fKYPeVlIsiTIr1Gekk0YlBOOQKtgKpKDo +IUbBzPxAuws/izNBedHkLaK8MysEDTb6QGZ3EC++iHk6IfZLUfZQnH1fjCtgsIdEhtt0Q bqsNO0sBAvdCJnj3mWx5pw84KID3Yf4= Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-554-974SOY2MMceBoX691RSu4g-1; Mon, 04 Sep 2023 12:40:26 -0400 X-MC-Unique: 974SOY2MMceBoX691RSu4g-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id BFDB229ABA3A; Mon, 4 Sep 2023 16:40:25 +0000 (UTC) Received: from oldenburg3.str.redhat.com (unknown [10.39.194.68]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 1A011140E963; Mon, 4 Sep 2023 16:40:24 +0000 (UTC) From: Florian Weimer To: Cupertino Miranda via Libc-alpha Cc: Cupertino Miranda , "Jose E. Marchesi" , Elena Zannoni Subject: Re: [PING] [PATCH v2] Resolve-flockfile-funlockfile-differences References: <87ilhj3bsp.fsf@oracle.com> Date: Mon, 04 Sep 2023 18:40:23 +0200 In-Reply-To: <87ilhj3bsp.fsf@oracle.com> (Cupertino Miranda via Libc-alpha's message of "Fri, 06 Jan 2023 15:47:18 +0000") Message-ID: <87ledluc20.fsf@oldenburg3.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.7 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-4.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: * Cupertino Miranda via Libc-alpha: > This is a request for review of the patch sent by Patrick McGehearty > based on an inconsistency in flockfile and funlockfile definitions > (macro/function) when used in glibc code. It finally clicked to for me what is going on here. Sorry for taking so long. It's a basic C problem. _IO_funlockfile is defined as a macro and as a function. The function definition in stdio-common/funlockfile.c unlocks the stream unconditionally. The macro version checks if FSETLOCKING_BYCALLER is active and skips the unlocking in that case. Same for _IO_flockfile. With &_IO_funlockfile, we get the unconditional unlock, but with _IO_flockfile (S), we get the conditionalized lock. The current code is clearly buggy. It should be possible to write a test for this, using ftrylockfile. The direction of the change seems right, too. > Should both the macro/function definitions be coherent such that they should be > used interchangeably in glibc code? The external flockfile/funlockfile needs to be unconditional because I think it is expected that these functions can be used to implement the locking that is required if FSETLOCKING_BYCALLER is used. Despite the recursive internal lock, checking for FSETLOCKING_BYCALLER is probably more than a performance optimization because the unexpected locking might introduce deadlocks. So skipping the locking, as you did, seems correct. So I think we need the different internal and external behavior. What is clearly not okay though is that we use the same identifier for those two different operations. We cannot rename the external symbol, but we really should rename the internal identifier. Thanks, Florian