Re: [LTP] [PATCH v4 24/31] testcases: sysfs: Add sys_block_loop01

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> > +	fd = SAFE_OPEN(dev_path, O_RDWR);
> > +	info.lo_flags = LO_FLAGS_AUTOCLEAR;
> > +	SAFE_IOCTL(fd, LOOP_SET_STATUS64, &info);
> > +	SAFE_CLOSE(fd);
> > +
> > +	/* autoclear runs asynchronously off a kernel workqueue, poll for it */
> > +	size = TST_RETRY_FN_EXP_BACKOFF(read_loop_size(), TST_RETVAL_EQ0, 5);
> > +
> > +	if (size == 0) {
> > +		tst_res(TPASS, "Device auto-detached");
> > +		attached = 0;
> > +	} else {
> > +		tst_res(TFAIL, "Device did not auto-detach in time");
> > +		tst_detach_device(dev_path);
> > +		attached = 0;
> > +	}
> nit: attached = 0 can be only once here below if/else.
> Also, I'm not sure why it's set to 0 because tst_detach_device() was not called,
> but obviously I just miss something obvious.

That's what the LO_FLAGS_AUTOCLEAR is for, the file is supposed to be
detached when last fd pointing to the device is closed.

> > +}
> > +
> > +static void run(void)
> > +{
> > +	if (tst_attach_device(dev_path, BACKING_FILE))
> > +		tst_brk(TBROK, "Failed to attach %s to %s", BACKING_FILE,
> > +			dev_path);
> > +	attached = 1;
> > +
> > +	check_attached();
> > +	check_reconfigured();
> > +
> > +	if (tst_detach_device(dev_path))
> IMHO it'd be safer to guard this with check attached in case of later code changes:
> 
> 	if (attached && tst_detach_device(dev_path))
> 		tst_brk(TBROK, "Failed to detach %s", dev_path);

I do not like adding code "just in case" it's usually more confusing
that keeping it that way.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp