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=EvExNEKK; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTPS id 6FFC75A0619 for ; Fri, 17 Oct 2025 00:22:37 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1760653356; 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=QkOhbtvOlSKtLF5v1werQ10lyOTZD65vlwmi9bikZ1Y=; b=EvExNEKKgGZegR5Og1qLJQoq1mG1dl3dUGN/SV0dcC0tHZytW/F8r+brTxW9fMD2wkg3v6 2E2/PRrQBZDaUsR4XRkHE6vSbu45k+pweAOZEQHCneHa8wiV+12hO5OT/jHuTruP5MqYg5 EbkFQJJvCjJjhzYe2bZ8r+LT8unjNyY= Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-177-yf1rBfvnOTaAR-X68-q2PQ-1; Thu, 16 Oct 2025 18:22:34 -0400 X-MC-Unique: yf1rBfvnOTaAR-X68-q2PQ-1 X-Mimecast-MFC-AGG-ID: yf1rBfvnOTaAR-X68-q2PQ_1760653353 Received: by mail-wr1-f70.google.com with SMTP id ffacd0b85a97d-42705afbf19so153323f8f.1 for ; Thu, 16 Oct 2025 15:22:34 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1760653353; x=1761258153; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=aFXOHQebXGObCubeatXKSxqGd6guBgiYxvr8UIhq5j0=; b=wQCH4NqBruzJfyFKfndk+LOOGdRO0EBs6F7Pc1nnJzaOD6rNGTRSXmb2j1+pxmxIxH TC57j7o+m/at7f0IeA8uM2mXrK1qqJYHiH09r4O5V5Oz/GFbL6p27UzK9auP3Xm9S7bW 8M5VmW+cNSRlVu4znLUzxIA+uLk/dg/oerwZgib9SR8zub8scRj1haDTndYhxAzsvQaO LkqD/AYp4sQ8fAn8CVCtrHMEFTeq5z7KZlfWg6kQ8xlIS3MIMCEj/4WjUsQn7LiOZzQp CdOzoNjf6EH0lBa42fiQpWhP5oAbyItNQhMP2LRlsGPFzGsVDh72DWczFvO2f1eqi7Ik l/FA== X-Forwarded-Encrypted: i=1; AJvYcCWNINXfeMufEXbcaz21ak5CLuekPvPr+lB4j15/ciztrXGYF+VYl52+qJMJr/n6pchRMOJl/QQDEN8=@passt.top X-Gm-Message-State: AOJu0YzI8M3Fn1pAl3qpLfWVuBaPtNPdyOQEbdzDjtDJrdi1+uPHnolD VT1nEefvaGpHnpW0R/aYzyYJhXW/joECjUnwg12Xu+9n7vtiCl8/N6jpAOwIyRVSSYPUWz+VdB8 nlHAnWYwzje4KoVXTsfhjoNO/QNPDNSjflwJget+RLpMQHTcVQOwOtw== X-Gm-Gg: ASbGncsBqBKG/paj4wFuAt8Ed9BnWwqQvjEHPxjByELcoE+34sPFWky5bi5T/u5pqAM vUWdrnPfmTMeS2u9xY+/9Nx6W1Ln3cdmoDUhKDoO5UvhCH2WAzahUprKtTFKMrfbGiAyV6Vks+f 1AXNYiNm6cdpZjUtnnZoSMun0SdR/+j5/LFcLU8OQL/+hwj8ggcu+RaA0zZ4RMI7K4AIaVLL3Ye OgvME2Ac0KnCylF9a0Gofd60Zke9Q4H71BVBi8+MeYO18J0p9TkrrSpgethA5D34OeSZvZQIhqq O6eAjv6Qq26eJNUHyaEwTmKG7sUgKaXSHq+vySc+2FHSFkoiMQSNmim7X30LMoE93RE+cTnM9L9 rjBDysKvkPA== X-Received: by 2002:a05:6000:26cb:b0:415:24ee:60ac with SMTP id ffacd0b85a97d-426fb6a19f9mr4049031f8f.5.1760653352776; Thu, 16 Oct 2025 15:22:32 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFXx7yHC2VDT9Sk9tiHod4viar8IWXL2+iqtNS3omGaKem7iS9OR/t5eMM8XmhSz0Av1Zv4XA== X-Received: by 2002:a05:6000:26cb:b0:415:24ee:60ac with SMTP id ffacd0b85a97d-426fb6a19f9mr4049014f8f.5.1760653352183; Thu, 16 Oct 2025 15:22:32 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4711441f975sm48699405e9.4.2025.10.16.15.22.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 16 Oct 2025 15:22:31 -0700 (PDT) Date: Fri, 17 Oct 2025 00:22:14 +0200 From: Stefano Brivio To: Yumei Huang Subject: Re: [PATCH v4 2/4] util: Introduce read_file() and read_file_integer() function Message-ID: <20251017002214.3fd4955b@elisabeth> In-Reply-To: References: <20251016023423.8923-1-yuhuang@redhat.com> <20251016023423.8923-3-yuhuang@redhat.com> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: 5S03S1X_KC79Vg_Bs8mmsWGpSdP1beUfwfbStWYbqQA_1760653353 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Message-ID-Hash: LHSCA4QRG3ZYJHX3KB2TCJAWACEDFFYE X-Message-ID-Hash: LHSCA4QRG3ZYJHX3KB2TCJAWACEDFFYE X-MailFrom: sbrivio@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: David Gibson , 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 Thu, 16 Oct 2025 15:49:39 +0800 Yumei Huang wrote: > On Thu, Oct 16, 2025 at 2:30=E2=80=AFPM David Gibson > wrote: > > > > On Thu, Oct 16, 2025 at 10:34:21AM +0800, Yumei Huang wrote: =20 > > > Signed-off-by: Yumei Huang > > > --- > > > util.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++= ++ > > > util.h | 3 +++ > > > 2 files changed, 87 insertions(+) > > > > > > diff --git a/util.c b/util.c > > > index c492f90..197677e 100644 > > > --- a/util.c > > > +++ b/util.c > > > @@ -579,6 +579,90 @@ int write_file(const char *path, const char *buf= ) > > > return len =3D=3D 0 ? 0 : -1; > > > } > > > > > > +/** > > > + * read_file() - Read contents of file into a buffer > > > + * @path: File to read > > > + * @buf: Buffer to store file contents > > > + * @buf_size: Size of buffer > > > + * > > > + * Return: number of bytes read on success, -1 on any error, -2 on t= runcation > > > +*/ > > > +int 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); > > > + 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_perror("File %s truncated, buffer too small", path= ); > > > + return -2; > > > + } > > > + > > > + buf[total_read] =3D '\0'; > > > + > > > + return (int)total_read; =20 > > > > Probably makes more sense for total_read and the return type to be ssiz= e_t. =20 >=20 > Just tried to be consistent with write_file(). I can change it to > ssize_t if needed. ssize_t is the type designed for this, if write_file() has it wrong (I didn't check), we should fix that as well. > > > +} > > > + > > > +/** > > > + * read_file_integer() - Read an integer value from a file > > > + * @path: 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) > > > +{ > > > + char buf[INTMAX_STRLEN]; > > > + char *end; =20 > > > > passt coding style is to list (where possible) local variables in > > reverse order of line length, so this should go after bytes_read. =20 >=20 > Oh, I didn't notice that. Will update later. Rationale (added to my further list for CONTRIBUTING.md): https://hisham.hm/2018/06/16/when-listing-repeated-things-make-pyramids/ and see also https://lwn.net/Articles/758552/. > > =20 > > > + intmax_t value; > > > + int bytes_read; > > > + > > > + bytes_read =3D read_file(path, buf, sizeof(buf)); > > > + > > > + if (bytes_read < 0) > > > + return fallback; > > > + > > > + if (bytes_read =3D=3D 0) { > > > + debug("Empty file %s", path); > > > + return fallback; > > > + } > > > + > > > + errno =3D 0; > > > + value =3D strtoimax(buf, &end, 10); > > > + if (*end && *end !=3D '\n') { > > > + debug("Invalid format in %s", path); > > > + return fallback; > > > + } > > > + if (errno) { > > > + debug("Invalid value in %s: %s", path, buf); > > > + return fallback; > > > + } > > > + > > > + return value; > > > +} > > > + > > > #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 22eaac5..887d795 100644 > > > --- a/util.h > > > +++ b/util.h > > > @@ -222,6 +222,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); > > > +int 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); > > > @@ -249,6 +251,7 @@ static inline const char *af_name(sa_family_t af) > > > } > > > > > > #define UINT16_STRLEN (sizeof("65535")) > > > +#define INTMAX_STRLEN (sizeof("-9223372036854775808")= ) =20 > > > > It's correct for now, and probably for any systems we're likely to run > > on, but I dislike hard-assuming the size of intmax_t here. I feel > > like there must be a better way to derive the correct string length, > > but I haven't figured out what it is yet :(. =20 >=20 > How about this: >=20 > #define INTMAX_STRLEN (sizeof(intmax_t) * 3 + 2) >=20 > Each byte can represent about 2.4 decimal digits as below, > sizeof(intmax_t) * 3 gives us a safe upper bound, +2 for sign and null > terminator. >=20 > 1 bit =3D log=E2=82=81=E2=82=80(2) =E2=89=88 0.30103 decimal digits > 1 byte =3D 8 bits =3D 8 =C3=97 0.30103 =E2=89=88 2.408 decimal digits If it's sourced from https://stackoverflow.com/a/10536254 and comment, don't forget to mention that in whatever implementation / commit message. But I was thinking... what if we keep it much simpler, use BUFSIZ, and error out if the buffer is too small? It would be good to be robust against any potential kernel issue anyway, so I think we need a mechanism like that in any case. It's not a security matter, because if the kernel was compromised, we're compromised too, simply a matter of robustness. --=20 Stefano