From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=hDwTv0wv; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTPS id 632E75A0271 for ; Wed, 19 Nov 2025 10:04:49 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1763543088; 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: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=77vsWREfs31IuPlMQTFygltN2EdlEqCF1fRShf2rjIY=; b=hDwTv0wv92sxykIK82e0RUJVaunwg2JVjskec/QMGmXhFtzDNXdSP1TvKdonDGd3vVD0iP 0j/EHpNd6WpjJnAXTJtQVZr5sC1nz3q3fS/ieRrdIHeaFiIzBPzRCMhKcJak4P4M8kPF+d iDR3xYhdf6j0tN5xHlbxm/iaDTLtlpY= Received: from mail-ej1-f70.google.com (mail-ej1-f70.google.com [209.85.218.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-110-tUKoJSWAMdOMbmWWaOF_RQ-1; Wed, 19 Nov 2025 04:04:46 -0500 X-MC-Unique: tUKoJSWAMdOMbmWWaOF_RQ-1 X-Mimecast-MFC-AGG-ID: tUKoJSWAMdOMbmWWaOF_RQ_1763543085 Received: by mail-ej1-f70.google.com with SMTP id a640c23a62f3a-b735400de44so615167466b.0 for ; Wed, 19 Nov 2025 01:04:46 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1763543085; x=1764147885; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=77vsWREfs31IuPlMQTFygltN2EdlEqCF1fRShf2rjIY=; b=wgsL0VF9WcafBQDM8g7cmDdezAvriE/DPUWRnDrSH7Ru854rS4Cx9O45M346CAMEee 1KJSvh8i9hWK77q4MAvkxV4b0O6mlxLXb6uVkT85pgqJ6joMfzwKmbTadUgHs3Tsa9yV RYCGkHlMnc5Ya6o1rPy8ybdJ8dUyDvZtefD22bwFRJoHw3peJn39GgkRDcICrrt0repj 6WEbJAAYXLViPtc/I6WbEhKLUXbTeLHa/sqRPzFONUqgFFpRGveZZWzP0A6iAC65JpSh uynqqx5nonsz7VKbpcoBKAYvA24HB19pPBSEcEr/SyfMZ2xS2rAF4R4JxKw1rPyVMUXp M73A== X-Forwarded-Encrypted: i=1; AJvYcCWIXZiaMLKIyYFyAGZkh0LcF8NVFbtLlduaO29qJoYZaDSYBaqHHomDx4gmAMXjKHtviMmubSjyv4g=@passt.top X-Gm-Message-State: AOJu0Yzbk25mHXnwUZEodu4mG5sW9QPc7GHLwOdtkIGRikTjzwoeY2Gu 9iImZ1o5NA63guaq6LSaZN+q8BjIBsphPTB2FSK6nXIgNocjEzNAeKXtVhAoCxSCeIQPEJAiXOG WfY74+RHTzi3g26GMuG+a03+TepBre1NtmyF0AsrRtepicFNGqiMr6cSglyTDMFL8q1+CbGISJU TyaHymB8KDHJQmDlgdqUT+lNQ/zREhuGL4Z81TnSQ= X-Gm-Gg: ASbGncummqH+Ss2G7kaoEokx1LSS8GOTElq/IaQi6xyyX5iu6frmUMqTPwlFjrXcBYB ZdnTjtQQTF+2L2kwVdK5SV8WysXp/CkmcSdr44UUONzipU5rDJ0gwWlH0TGmszvXsiMwTtSq6NB STpoJD1yMktD4LVb7OwmD9bdrRStmh2yQMPFvZv8objgY1lkROEgMAOdyQp5k3JSjIxVw= X-Received: by 2002:a05:6402:42c5:b0:641:8d40:8672 with SMTP id 4fb4d7f45d1cf-64350e33055mr18266134a12.16.1763543084655; Wed, 19 Nov 2025 01:04:44 -0800 (PST) X-Google-Smtp-Source: AGHT+IG3xWMzd3F3y0GyvM+4CTSuoIJiCaF02oIusqDIJZ0UBLajYR70qC7B8oZpoyiJCeMZ6AxwKFDxHWb8YtswvPk= X-Received: by 2002:a05:6402:42c5:b0:641:8d40:8672 with SMTP id 4fb4d7f45d1cf-64350e33055mr18266097a12.16.1763543084044; Wed, 19 Nov 2025 01:04:44 -0800 (PST) MIME-Version: 1.0 References: <20251110093137.87705-1-yuhuang@redhat.com> <20251110093137.87705-3-yuhuang@redhat.com> <20251118011938.47d5c1e2@elisabeth> In-Reply-To: From: Yumei Huang Date: Wed, 19 Nov 2025 17:04:31 +0800 X-Gm-Features: AWmQ_bnxR0X6FbCWiBG5rOa-Ku6zvc4SOCABriEkwFb788_6ahScND0y0Ez1Lis Message-ID: Subject: Re: [PATCH v8 2/6] util: Introduce read_file() and read_file_integer() function To: David Gibson X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: 7B6uo1N4fvME-CMnQwBsAEHKrB7f0e3Pg2zHKG266lg_1763543085 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Message-ID-Hash: 7AJR2RTOYLE6WZPIBTJJILUC7YASC667 X-Message-ID-Hash: 7AJR2RTOYLE6WZPIBTJJILUC7YASC667 X-MailFrom: yuhuang@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Stefano Brivio , passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Tue, Nov 18, 2025 at 11:36=E2=80=AFAM David Gibson wrote: > > On Tue, Nov 18, 2025 at 01:19:38AM +0100, Stefano Brivio wrote: > > On Mon, 10 Nov 2025 17:31:33 +0800 > > Yumei Huang wrote: > > > > > Signed-off-by: Yumei Huang > > > Reviewed-by: David Gibson > > > --- > > > util.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++= ++ > > > util.h | 2 ++ > > > 2 files changed, 88 insertions(+) > > > > > > diff --git a/util.c b/util.c > > > index 44c21a3..c4c849c 100644 > > > --- a/util.c > > > +++ b/util.c > > > @@ -590,6 +590,92 @@ int write_file(const char *path, const char *buf= ) > > > return len =3D=3D 0 ? 0 : -1; > > > } > > > > > > +/** > > > + * read_file() - Read contents of file into a NULL-terminated buffer > > > + * @path: Path to file to read > > > + * @buf: Buffer to store file contents > > > + * @buf_size: Size of buffer > > > + * > > > + * Return: number of bytes read on success, -1 on error, -ENOBUFS on= truncation > > > + */ > > > +ssize_t read_file(const char *path, char *buf, size_t buf_size) > > > +{ > > > + int fd =3D open(path, O_RDONLY | O_CLOEXEC); > > > + size_t total_read =3D 0; > > > + ssize_t rc; > > > + > > > + if (fd < 0) { > > > + warn_perror("Could not open %s", path); > > > > While testing something unrelated I realised that this looks like a > > serious failure, but it's perfectly normal on (even slightly) older > > kernels: > > > > --- > > $ git describe --contains ccce324dabfe # tcp: make the first N SYN RTO = backoffs linear > > v6.5-rc1~163^2~299 > > $ git describe --contains 1280c26228bd # tcp: add tcp_rto_max_ms sysctl > > v6.15-rc1~160^2~352^2 > > --- > > > > On a 6.1 kernel, for example: > > > > --- > > $ ./pasta -d --config-net > > [...] > > 0.0258: Could not open /proc/sys/net/ipv4/tcp_syn_linear_timeouts: No s= uch file or directory > > 0.0258: Could not open /proc/sys/net/ipv4/tcp_rto_max_ms: No such file = or directory > > 0.0258: Read sysctl values syn_retries: 6, syn_linear_timeouts: 4, rto_= max: 120 > > --- > > > > and actually the third message isn't accurate, because we didn't read > > those values, we are just using them. > > Right. In fact I'd say here the fact they came from sysctl is less > important here than what we're using them for. So maybe > Using TCP RTO parameters, syn_retries: 6, ... Got it. > > > > > Worse, yet: > > > > --- > > $ ./pasta > > Could not open /proc/sys/net/ipv4/tcp_syn_linear_timeouts: No such file= or directory > > Could not open /proc/sys/net/ipv4/tcp_rto_max_ms: No such file or direc= tory > > --- > > > > so this would be logged *with default options* by Podman, rootlesskit / > > Docker, via libvirt, and by other users too, meaning we would get > > submerged by reports about this "error" if we release it like this (6.1= 5 > > is fairly recent). > > > > So maybe we could turn this (and the messages below) to debug() / > > debug_perror(), and then: > > I think we should remove the error message from read_file() entirely, > just returning an error code. Essentially it is too low level to know > the severity and meaning of a failure, so reporting the problem to the > user is better left to the valler. Then we will have a few more values to return, like errno for open or read file fails, -ENOBUFS, and another error code for when buf size is 0. I'd say it's more complicated than write_fiel(). Maybe we could update with debug()/debug_perror() first as Stefano suggested, and then fix them together later? > > > > + return -1; > > > + } > > > + > > > + while (total_read < buf_size) { > > > + rc =3D read(fd, buf + total_read, buf_size - total_read); > > > + > > > + if (rc < 0) { > > > + warn_perror("Couldn't read from %s", path); > > > + close(fd); > > > + return -1; > > > + } > > > + > > > + if (rc =3D=3D 0) > > > + break; > > > + > > > + total_read +=3D rc; > > > + } > > > + > > > + close(fd); > > > + > > > + if (total_read =3D=3D buf_size) { > > > + warn("File %s contents exceed buffer size %zu", path, > > > + buf_size); > > > + buf[buf_size - 1] =3D '\0'; > > > + return -ENOBUFS; > > > + } > > > + > > > + buf[total_read] =3D '\0'; > > > + > > > + return total_read; > > > +} > > > + > > > +/** > > > + * read_file_integer() - Read an integer value from a file > > > + * @path: Path to file to read > > > + * @fallback: Default value if file can't be read > > > + * > > > + * Return: integer value, @fallback on failure > > > + */ > > > +intmax_t read_file_integer(const char *path, intmax_t fallback) > > > +{ > > > + ssize_t bytes_read; > > > + char buf[BUFSIZ]; > > > + intmax_t value; > > > + char *end; > > > + > > > + bytes_read =3D read_file(path, buf, sizeof(buf)); > > > + > > > + if (bytes_read < 0) > > > + return fallback; > > > > ...say something here, like "using %i as default value", so that it's > > clear that the error doesn't have further consequences. > > > > We should probably do that for all the failure cases here, so you might > > want to 'goto error;' here, and also: > > Arguably read_file_integer() is still too low-level to useful report > the error to the user. But that would require a clunkier interface so > we can return an error code as well as the parsed value. > > Saying it's falling back here would certainly be an improvement. Sure, I can add the falling back message. > > > > + > > > + if (bytes_read =3D=3D 0) { > > > + debug("Empty file %s", path); > > > > here, and below, and then: > > > > > + return fallback; > > > + } > > > + > > > + errno =3D 0; > > > + value =3D strtoimax(buf, &end, 10); > > > + if (*end && *end !=3D '\n') { > > > + debug("Non-numeric content in %s", path); > > > + return fallback; > > > + } > > > + if (errno) { > > > + debug("Out of range value in %s: %s", path, buf); > > > + return fallback; > > > + } > > > + > > > + return value; > > > > error: > > debug("Using ...") > > return fallback; > > > > > +} > > > + > > > #ifdef __ia64__ > > > /* Needed by do_clone() below: glibc doesn't export the prototype of= __clone2(), > > > * use the description from clone(2). > > > diff --git a/util.h b/util.h > > > index a0b2ada..c1502cc 100644 > > > --- a/util.h > > > +++ b/util.h > > > @@ -229,6 +229,8 @@ void pidfile_write(int fd, pid_t pid); > > > int __daemon(int pidfile_fd, int devnull_fd); > > > int fls(unsigned long x); > > > int write_file(const char *path, const char *buf); > > > +ssize_t read_file(const char *path, char *buf, size_t buf_size); > > > +intmax_t read_file_integer(const char *path, intmax_t fallback); > > > int write_all_buf(int fd, const void *buf, size_t len); > > > int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, = size_t skip); > > > int read_all_buf(int fd, void *buf, size_t len); > > > > -- > > Stefano > > > > -- > David Gibson (he or they) | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you, not the other wa= y > | around. > http://www.ozlabs.org/~dgibson --=20 Thanks, Yumei Huang