From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <legion@altlinux.ru>
Date: Mon, 15 May 2023 11:05:56 +0200
From: Alexey Gladkov <legion@altlinux.ru>
To: make-initrd@lists.altlinux.org
Message-ID: <ZGH19N3Dv4DlFNqg@example.org>
References: <cover.1683200226.git.gladkov.alexey@gmail.com>
 <6b444ed922286eb3df8f5322b1bddf9c55753eb8.1683200226.git.gladkov.alexey@gmail.com>
 <f73ff65f-409b-2c67-fe42-969d089e5c24@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <f73ff65f-409b-2c67-fe42-969d089e5c24@gmail.com>
Subject: Re: [make-initrd] [PATCH 1/3] Reimplement ueventd
X-BeenThere: make-initrd@lists.altlinux.org
X-Mailman-Version: 2.1.12
Precedence: list
Reply-To: make-initrd@lists.altlinux.org
List-Id: <make-initrd.lists.altlinux.org>
List-Unsubscribe: <https://lists.altlinux.org/mailman/options/make-initrd>,
 <mailto:make-initrd-request@lists.altlinux.org?subject=unsubscribe>
List-Archive: <http://lists.altlinux.org/pipermail/make-initrd>
List-Post: <mailto:make-initrd@lists.altlinux.org>
List-Help: <mailto:make-initrd-request@lists.altlinux.org?subject=help>
List-Subscribe: <https://lists.altlinux.org/mailman/listinfo/make-initrd>,
 <mailto:make-initrd-request@lists.altlinux.org?subject=subscribe>
X-List-Received-Date: Mon, 15 May 2023 09:05:59 -0000
Archived-At: <http://lore.altlinux.org/make-initrd/ZGH19N3Dv4DlFNqg@example.org/>
List-Archive: <http://lore.altlinux.org/make-initrd/>

On Mon, May 15, 2023 at 02:08:09AM +0300, Leonid Krivoshein wrote:
> Тут ошибок нет, но есть над чем позудеть.))
> 
> 
> On 5/4/23 16:42, Alexey Gladkov wrote:
> > [...]
> > diff --git a/datasrc/ueventd/path.c b/datasrc/ueventd/path.c
> > new file mode 100644
> > index 00000000..7880ad5c
> > --- /dev/null
> > +++ b/datasrc/ueventd/path.c
> > @@ -0,0 +1,80 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#include <stdlib.h>
> > +#include <dirent.h>
> > +#include <fcntl.h>
> > +#include <errno.h>
> > +
> > +#include "ueventd.h"
> > +
> > +int is_dot_dir(struct dirent *ent)
> > +{
> > +	return (ent->d_type == DT_DIR &&
> > +	        (ent->d_name[0] == '.' && (ent->d_name[1] == '\0' ||
> > +	                                   (ent->d_name[1] == '.' && ent->d_name[2] == '\0') )));
> > +}
> > +
> > +DIR *xopendir(const char *path)
> > +{
> > +	DIR *dir = opendir(path);
> > +	if (!dir)
> > +		fatal("opendir: %s: %m", path);
> > +	return dir;
> > +}
> > +
> > +
> > +struct dirent *xreaddir(DIR *d, const char *path)
> > +{
> > +	struct dirent *ent;
> > +
> > +	errno = 0;
> > +	ent = readdir(d);
> > +	if (!ent) {
> > +		if (!errno)
> > +			return NULL;
> 
> Создавать этот NULL дольше, чем вернуть готовый ent. Я бы поменял 
> условие на if (!ent && errno) fatal(...); Получится даже короче и 
> читабельней.

Такие оптимизации я оставлю до момента, когда буду трогать эту функцию
снова.

> 
> > +		fatal("readdir: %s: %m", path);
> > +	}
> > +	return ent;
> > +}
> > +
> > +int empty_directory(const char *path)
> > +{
> > +	struct dirent *ent;
> > +	int is_empty = 1;
> > +	DIR *d = xopendir(path);
> > +
> > +	while ((ent = xreaddir(d, path)) != NULL) {
> > +		if (ent->d_name[0] != '.') {
> > +			is_empty = 0;
> > +			break;
> > +		}
> > +	}
> > +	closedir(d);
> > +
> > +	return is_empty;
> > +}
> 
> Назначение функции по имени кажется двусмысленным и можно обойтись без 
> этой переменной в стеке, даже немного сократив код:
> 
> int is_dir_empty(const char *path)
> {
>          struct dirent *ent;
>          DIR *d = xopendir(path);
> 
>          while ((ent = xreaddir(d, path)) != NULL) {
>                  if (ent->d_name[0] != '.') {
>                          closedir(d);
>                          return 0;
>                  }
>          }
>          closedir(d);
> 
>          return -1;
> }

В моём коде лишняя переменная. В твоём два вызова closedir.

> Не помню, какую задачу решает этот минус, но кажется правильней для 
> логического значения.
> 
> 
> > +
> > +ssize_t read_retry(int fd, void *buf, size_t count)
> > +{
> > +	return TEMP_FAILURE_RETRY(read(fd, buf, count));
> > +}
> > +
> > +ssize_t write_retry(int fd, const void *buf, size_t count)
> > +{
> > +	return TEMP_FAILURE_RETRY(write(fd, buf, count));
> > +}
> > +
> > +ssize_t write_loop(int fd, const char *buffer, size_t count)
> > +{
> > +	ssize_t offset = 0;
> > +
> > +	while (count > 0) {
> > +		ssize_t block = write_retry(fd, &buffer[offset], count);
> > +
> > +		if (block <= 0)
> > +			return offset ? : block;
> 
> Первый раз встречаю в такой нотации. Перед двоеточием имеется ввиду тоже 
> offset?

Да.

> 
> > +		offset += block;
> > +		count -= (size_t) block;
> > +	}
> > +	return offset;
> > +}
> > [...]
> 
> 
> -- 
> WBR, Leonid Krivoshein.
> _______________________________________________
> Make-initrd mailing list
> Make-initrd@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/make-initrd

-- 
Rgrds, legion