os/drivers: Modify to return errno from compression ioctl

[seokhun-eom24]

compress ioctl

Return proper error codes instead of ERROR
Add error logging for COMPIOC_FCOMP_DECOMPRESS case.

decompress_block

Previous, LzmaUncompress return code cannot be mapped to proper errno.
standardize decompress_block to return OK, -EIO, or -ENOMEM.
Add error logging with the API return value.

[Taejun-Kwon]

I think decompress_block should return one of negative value. You should consider below.

1. LzmaDecode returns one of below.

#define SZ_OK 0
#define SZ_ERROR_DATA 1
#define SZ_ERROR_MEM 2
#define SZ_ERROR_CRC 3
#define SZ_ERROR_UNSUPPORTED 4
#define SZ_ERROR_PARAM 5
#define SZ_ERROR_INPUT_EOF 6
#define SZ_ERROR_OUTPUT_EOF 7
#define SZ_ERROR_READ 8
#define SZ_ERROR_WRITE 9
#define SZ_ERROR_PROGRESS 10
#define SZ_ERROR_FAIL 11
#define SZ_ERROR_THREAD 12
#define SZ_ERROR_ARCHIVE 16
#define SZ_ERROR_NO_ARCHIVE 17

In this case, will you convert above type to errno in errno.h? for example, SZ_ERROR_UNSUPPORTED can be converted to EINVAL. Or you can convert entire of error as EIO and put some log to distinguish error

	ret = mz_uncompress(out_buffer, writesize, read_buffer, *size);
	if (ret != Z_OK) {
		bcmpdbg("Failure to decompress with Miniz's uncompress API; ret = %d\n", ret);
		if (ret == Z_BUF_ERROR) {
			ret = -ENOMEM;
		} else {
			ret = -EIO;
		}
	}	

2. 'decompress_block' is part of os, it located in os/compression. Hence it should not return SZ_OK or something.
   it should return OK or one of errno.

[seokhun-eom24]

I add one more commit to fix that.

[Taejun-Kwon]

return ret;

[seokhun-eom24]

That's right.
I incorrectly thought to ret is EBUSY here.

[Taejun-Kwon]

init is failed here but we can continue compress?

[seokhun-eom24]

I missed it.
It should return here.

[Taejun-Kwon]

-EINVAL means that argument is wrong.
But this error means that file doesn't include compression header in its priv data area.
Hence I think EBADF(bad file descriptor) is most proper errno

[seokhun-eom24]

Thank you.

[Taejun-Kwon]

please refer to below style.

int decompress_block(unsigned char *out_buffer, long unsigned int *writesize, unsigned char *read_buffer, long unsigned int *size)
{
	int ret = ERROR;
#if CONFIG_COMPRESSION_TYPE == LZMA
	/* LZMA specific logic for decompression */
	*size -= (LZMA_PROPS_SIZE);
	int read_size = *size;

	ret = LzmaUncompress(&out_buffer[0], (unsigned int *)writesize, &read_buffer[LZMA_PROPS_SIZE], (unsigned int *)size, &read_buffer[0], LZMA_PROPS_SIZE);
	if (ret == SZ_OK) {
		if (*size < read_size) {
			bcmpdbg("Out buffer allocated is not sufficient, some inputs are still left to be uncompressed\n");
			return -ENOMEM;
		}
		return OK;
	} else {
		bcmpdbg("LzmaUncompress is not SZ_OK ret : %d\n", ret);
		if (ret == SZ_ERROR_INPUT_EOF) {
			return OK;
		}
		return -EIO;
	}
#elif CONFIG_COMPRESSION_TYPE == MINIZ	
	/* Miniz specific logic for decompression */
	ret = mz_uncompress(out_buffer, writesize, read_buffer, *size);
	if (ret != Z_OK) {
		bcmpdbg("mz_uncompress is not Z_OK ret : %d\n", ret);
		if (ret == Z_BUF_ERROR) {
			return -ENOMEM;
		}
		return -EIO;
	}
#endif
	return OK;
}

[seokhun-eom24]

Thank you for guide me a better style.

[Taejun-Kwon]

This is violence of file operation, actually ioctl never be called without open.
you should implement comp_open.

struct comp_dev_s
{
	bool refs; //to check open or close
	sem_t exclsem;
}

static int comp_open(FAR struct file *fileep)
{
	FAR struct inode *inode = filep->f_inode;
	FAR struct comp_dev_s *dev = inode->i_private;
	ret = sem_wait(&dev->exclsem);
	if (dev->refs) {
		return -EMFILE; //we allow only 1 ref count;
	}
	
	dev->refs = true;
	...
}

static int comp_close(FAR struct file *fileep)
{
	...
	dev->refs = false;
	...
}
	

I think init / deinit can be handled in open/close

[seokhun-eom24]

This is violence of file operation, actually ioctl never be called without open. you should implement comp_open.

struct comp_dev_s
{
	bool refs; //to check open or close
	sem_t exclsem;
}

static int comp_open(FAR struct file *fileep)
{
	FAR struct inode *inode = filep->f_inode;
	FAR struct comp_dev_s *dev = inode->i_private;
	ret = sem_wait(&dev->exclsem);
	if (dev->refs) {
		return -EMFILE; //we allow only 1 ref count;
	}
	
	dev->refs = true;
	...
}

static int comp_close(FAR struct file *fileep)
{
	...
	dev->refs = false;
	...
}
	

I think init / deinit can be handled in open/close

Thank you for your comment.
I'll take a look at that part and apply it to another PR.

[seokhun-eom24]

This is violence of file operation, actually ioctl never be called without open. you should implement comp_open.

struct comp_dev_s
{
	bool refs; //to check open or close
	sem_t exclsem;
}

static int comp_open(FAR struct file *fileep)
{
	FAR struct inode *inode = filep->f_inode;
	FAR struct comp_dev_s *dev = inode->i_private;
	ret = sem_wait(&dev->exclsem);
	if (dev->refs) {
		return -EMFILE; //we allow only 1 ref count;
	}
	
	dev->refs = true;
	...
}

static int comp_close(FAR struct file *fileep)
{
	...
	dev->refs = false;
	...
}
	

I think init / deinit can be handled in open/close

I made a PR for implement comp_open comp_close.
#6913