Skip to content

gh-132983: Split _zstd_set_c_parameters #133921

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

gh-132983: Split _zstd_set_c_parameters #133921

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions Lib/test/test_zstd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,8 +196,11 @@ def test_simple_compress_bad_args(self):
self.assertRaises(TypeError, ZstdCompressor, zstd_dict=b"abcd1234")
self.assertRaises(TypeError, ZstdCompressor, zstd_dict={1: 2, 3: 4})

# valid compression level range is [-(1<<17), 22]
with self.assertRaises(ValueError):
ZstdCompressor(2**31)
with self.assertRaises(ValueError):
ZstdCompressor(level=-(2**31))
with self.assertRaises(ValueError):
ZstdCompressor(options={2**31: 100})
Comment on lines +199 to 205
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test also for values out any C integer range: 2**1000 and -2**1000.


Expand Down Expand Up @@ -261,8 +264,10 @@ def test_compress_parameters(self):

# clamp compressionLevel
level_min, level_max = CompressionParameter.compression_level.bounds()
compress(b'', level_max+1)
compress(b'', level_min-1)
with self.assertRaises(ValueError):
compress(b'', level_max+1)
with self.assertRaises(ValueError):
compress(b'', level_min-1)
Comment on lines +267 to +270
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.


compress(b'', options={CompressionParameter.compression_level:level_max+1})
compress(b'', options={CompressionParameter.compression_level:level_min-1})
Expand Down
11 changes: 7 additions & 4 deletions Modules/_zstd/clinic/compressor.c.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

165 changes: 84 additions & 81 deletions Modules/_zstd/compressor.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,98 +45,101 @@ typedef struct {
#include "clinic/compressor.c.h"

static int
_zstd_set_c_parameters(ZstdCompressor *self, PyObject *level_or_options,
const char *arg_name, const char* arg_type)
_zstd_set_c_level(ZstdCompressor *self, const Py_ssize_t level)
{
size_t zstd_ret;
_zstd_state* const mod_state = PyType_GetModuleState(Py_TYPE(self));
if (mod_state == NULL) {
/* Set integer compression level */
const int min_level = ZSTD_minCLevel();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need to use all these consts. They just distract.

const int max_level = ZSTD_maxCLevel();
if (level < min_level || level > max_level) {
PyErr_Format(PyExc_ValueError,
"compression level %zd not in valid range %d <= level <= %d.",
level, min_level, max_level);
return -1;
}

/* Integer compression level */
if (PyLong_Check(level_or_options)) {
int level = PyLong_AsInt(level_or_options);
if (level == -1 && PyErr_Occurred()) {
PyErr_Format(PyExc_ValueError,
"Compression level should be an int value between %d and %d.",
ZSTD_minCLevel(), ZSTD_maxCLevel());
return -1;
}

/* Save for generating ZSTD_CDICT */
self->compression_level = level;
/* Save for generating ZSTD_CDICT */
self->compression_level = (int)level;

/* Set compressionLevel to compression context */
zstd_ret = ZSTD_CCtx_setParameter(self->cctx,
ZSTD_c_compressionLevel,
level);
/* Set compressionLevel to compression context */
const size_t zstd_ret = ZSTD_CCtx_setParameter(
self->cctx, ZSTD_c_compressionLevel, (int)level);

/* Check error */
if (ZSTD_isError(zstd_ret)) {
set_zstd_error(mod_state, ERR_SET_C_LEVEL, zstd_ret);
/* Check error */
if (ZSTD_isError(zstd_ret)) {
const _zstd_state* const st = PyType_GetModuleState(Py_TYPE(self));
if (st == NULL) {
return -1;
}
return 0;
set_zstd_error(st, ERR_SET_C_LEVEL, zstd_ret);
return -1;
}
return 0;
}

/* Options dict */
if (PyDict_Check(level_or_options)) {
PyObject *key, *value;
Py_ssize_t pos = 0;
static int
_zstd_set_c_parameters(ZstdCompressor *self, PyObject *options)
{
/* Set options dict */
_zstd_state* const mod_state = PyType_GetModuleState(Py_TYPE(self));
if (mod_state == NULL) {
return -1;
}

while (PyDict_Next(level_or_options, &pos, &key, &value)) {
/* Check key type */
if (Py_TYPE(key) == mod_state->DParameter_type) {
PyErr_SetString(PyExc_TypeError,
"Key of compression option dict should "
"NOT be DecompressionParameter.");
return -1;
}
if (!PyDict_Check(options)) {
PyErr_Format(PyExc_TypeError, "invalid type for options, expected dict");
return -1;
}

int key_v = PyLong_AsInt(key);
if (key_v == -1 && PyErr_Occurred()) {
PyErr_SetString(PyExc_ValueError,
"Key of options dict should be a CompressionParameter attribute.");
return -1;
}
Py_ssize_t pos = 0;
PyObject *key, *value;
while (PyDict_Next(options, &pos, &key, &value)) {
/* Check key type */
if (Py_TYPE(key) == mod_state->DParameter_type) {
PyErr_SetString(PyExc_TypeError,
"key should NOT be DecompressionParameter.");
return -1;
}

// TODO(emmatyping): check bounds when there is a value error here for better
// error message?
int value_v = PyLong_AsInt(value);
if (value_v == -1 && PyErr_Occurred()) {
PyErr_SetString(PyExc_ValueError,
"Value of option dict should be an int.");
return -1;
}
const int key_v = PyLong_AsInt(key);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

key and value are borrowed references. Incref them before calling PyLong_AsInt().

if (key_v == -1 && PyErr_Occurred()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not override arbitrary exceptions. Check the type of exception before replacing it.

PyErr_SetString(PyExc_ValueError,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it was a TypeError, it is preferable to keep a TypeError. If it was an OverflowError, it can be replaced with ValueError, but TypeError may be appropriate too in this context. Or what set_parameter_error() sets for invalid key values.

"key should be a CompressionParameter attribute.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No period at the end of the error message which is not a full sentence.

return -1;
}

if (key_v == ZSTD_c_compressionLevel) {
/* Save for generating ZSTD_CDICT */
self->compression_level = value_v;
}
else if (key_v == ZSTD_c_nbWorkers) {
/* From the zstd library docs:
1. When nbWorkers >= 1, triggers asynchronous mode when
used with ZSTD_compressStream2().
2, Default value is `0`, aka "single-threaded mode" : no
worker is spawned, compression is performed inside
caller's thread, all invocations are blocking. */
if (value_v != 0) {
self->use_multithread = 1;
}
}
// TODO(emmatyping): check bounds when there is a value error here for better
// error message?
int value_v = PyLong_AsInt(value);
if (value_v == -1 && PyErr_Occurred()) {
PyErr_SetString(PyExc_ValueError,
"options dict value should be an int.");
Comment on lines +112 to +115
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

return -1;
}

/* Set parameter to compression context */
zstd_ret = ZSTD_CCtx_setParameter(self->cctx, key_v, value_v);
if (ZSTD_isError(zstd_ret)) {
set_parameter_error(mod_state, 1, key_v, value_v);
return -1;
if (key_v == ZSTD_c_compressionLevel) {
/* Save for generating ZSTD_CDICT */
self->compression_level = value_v;
}
else if (key_v == ZSTD_c_nbWorkers) {
/* From the zstd library docs:
1. When nbWorkers >= 1, triggers asynchronous mode when
used with ZSTD_compressStream2().
2, Default value is `0`, aka "single-threaded mode" : no
worker is spawned, compression is performed inside
caller's thread, all invocations are blocking. */
if (value_v != 0) {
self->use_multithread = 1;
}
}
return 0;

/* Set parameter to compression context */
const size_t zstd_ret = ZSTD_CCtx_setParameter(self->cctx, key_v, value_v);
if (ZSTD_isError(zstd_ret)) {
set_parameter_error(mod_state, 1, key_v, value_v);
return -1;
}
}
PyErr_Format(PyExc_TypeError, "Invalid type for %s. Expected %s", arg_name, arg_type);
return -1;
return 0;
}

static void
Expand Down Expand Up @@ -314,7 +317,7 @@ _zstd_load_c_dict(ZstdCompressor *self, PyObject *dict)
/*[clinic input]
@classmethod
_zstd.ZstdCompressor.__new__ as _zstd_ZstdCompressor_new
level: object = None
level: Py_ssize_t(c_default='PY_SSIZE_T_MIN', accept={int, NoneType}) = None
The compression level to use. Defaults to COMPRESSION_LEVEL_DEFAULT.
options: object = None
A dict object that contains advanced compression parameters.
Expand All @@ -328,9 +331,9 @@ function instead.
[clinic start generated code]*/

static PyObject *
_zstd_ZstdCompressor_new_impl(PyTypeObject *type, PyObject *level,
_zstd_ZstdCompressor_new_impl(PyTypeObject *type, Py_ssize_t level,
PyObject *options, PyObject *zstd_dict)
/*[clinic end generated code: output=cdef61eafecac3d7 input=92de0211ae20ffdc]*/
/*[clinic end generated code: output=a857ec0dc29fc5e2 input=9899740b24d11319]*/
{
ZstdCompressor* self = PyObject_GC_New(ZstdCompressor, type);
if (self == NULL) {
Expand All @@ -353,20 +356,20 @@ _zstd_ZstdCompressor_new_impl(PyTypeObject *type, PyObject *level,
/* Last mode */
self->last_mode = ZSTD_e_end;

if (level != Py_None && options != Py_None) {
if (level != PY_SSIZE_T_MIN && options != Py_None) {
PyErr_SetString(PyExc_RuntimeError, "Only one of level or options should be used.");
goto error;
}

/* Set compressLevel/options to compression context */
if (level != Py_None) {
if (_zstd_set_c_parameters(self, level, "level", "int") < 0) {
if (level != PY_SSIZE_T_MIN) {
if (_zstd_set_c_level(self, level) < 0) {
goto error;
}
}

if (options != Py_None) {
if (_zstd_set_c_parameters(self, options, "options", "dict") < 0) {
if (_zstd_set_c_parameters(self, options) < 0) {
goto error;
}
}
Expand Down
Loading