8:42 a.m.
On pondělí 17. června 2019 15:07:07 CEST Nir Soffer wrote:
On Mon, Jun 17, 2019 at 1:04 PM Pavel Bar <pbar(a)redhat.com>
wrote:
> On Sun, Jun 16, 2019 at 11:50 PM Nir Soffer <nirsof(a)gmail.com> wrote:
>> Replace duplicate code for allocating and initializing sanlk_resource
>> struct with a helper.
>>
>> Signed-off-by: Nir Soffer <nsoffer(a)redhat.com>
>> ---
>>
>> python/sanlock.c | 46 +++++++++++++++++++++++-----------------------
>> 1 file changed, 23 insertions(+), 23 deletions(-)
>>
>> diff --git a/python/sanlock.c b/python/sanlock.c
>> index 5c2fd1a..6824e01 100644
>> --- a/python/sanlock.c
>> +++ b/python/sanlock.c
>> @@ -170,29 +170,38 @@ parse_single_disk(PyObject* disk, struct
>> sanlk_disk* res_disk)
>>
>> finally:
>> Py_XDECREF(path);
>> return rv;
>>
>> }
>>
>> +static struct sanlk_resource *
>> +create_resource(int num_disks)
>> +{
>> + size_t size = sizeof(struct sanlk_resource) +
>> + sizeof(struct sanlk_disk) * num_disks;
>> +
>> + struct sanlk_resource *res = calloc(1, size);
>> + if (res == NULL) {
>> + PyErr_NoMemory();
>> + return NULL;
>> + }
>> +
>> + res->num_disks = num_disks;
>> +
>> + return res;
>> +}
>> +
>>
>> static int
>> parse_disks(PyObject *obj, struct sanlk_resource **res_ret)
>> {
>>
>> - int num_disks, res_len;
>> + int num_disks;
>>
>> struct sanlk_resource *res;
>
> What about declaring the local variables when first used, thus immediately
> initializing them? I think C99 provides that.
> I.e.,
> int num_disks = PyList_Size(obj);
> struct sanlk_resource *res = create_resource(num_disks);
> Moreover, in this case they will be at the beginning of the block anyway.
This is not needed to fix the problem I'm trying to solve here.
We can do this work later if needed.
ok, LGTM then
>> num_disks = PyList_Size(obj);
>>
>> - res_len = sizeof(struct sanlk_resource) +
>> - (sizeof(struct sanlk_disk) * num_disks);
>> - res = malloc(res_len);
>> -
>> - if (res == NULL) {
>> - PyErr_NoMemory();
>> + res = create_resource(num_disks);
>> + if (res == NULL)
>>
>> return -1;
>>
>> - }
>> -
>> - memset(res, 0, res_len);
>> - res->num_disks = num_disks;
>>
>> for (int i = 0; i < num_disks; i++) {
>>
>> PyObject *disk = PyList_GetItem(obj,i);
>>
>> if (!parse_single_disk(disk, &(res->disks[i]))) {
>>
>> @@ -637,30 +646,21 @@ Align can be one of (1048576, 2097152, 4194304,
>> 8388608).\n\
>>
>> Sector can be one of (512, 4096).");
>>
>> static PyObject *
>> py_read_resource(PyObject *self __unused, PyObject *args, PyObject
>>
>> *keywds)
>>
>> {
>>
>> - int rv = -1, res_len, sector = SECTOR_SIZE_512;
>> + int rv = -1, sector = SECTOR_SIZE_512;
>>
>> long align = ALIGNMENT_1M;
>> PyObject *path = NULL;
>> struct sanlk_resource *res;
>> PyObject *res_info = NULL;
>>
>> static char *kwlist[] = {"path", "offset",
"align", "sector", NULL};
>>
>> - /* allocate the needed memory for the resource and one disk */
>> - res_len = sizeof(struct sanlk_resource) + sizeof(struct sanlk_disk);
>> - res = malloc(res_len);
>> -
>> - if (res == NULL) {
>> - PyErr_NoMemory();
>> + res = create_resource(1 /* num_disks */);
>
> Not sure the comment is needed.
>
>> + if (res == NULL)
>>
>> return NULL;
>>
>> - }
>> -
>> - /* initialize resource and disk structures */
>> - memset(res, 0, res_len);
>> - res->num_disks = 1;
>>
>> /* parse python tuple */
>> if (!PyArg_ParseTupleAndKeywords(args, keywds, "O&|kli",
kwlist,
>>
>> pypath_converter, &path, &(res->disks[0].offset),
&align,
>>
>> §or)) {
>>
>> goto finally;
>>
>> --
>> 2.17.2