10:59 a.m.
we check for sanlock rv error and return NULL when rv != 0, this if is not
redundant at all.
On Mon, Jun 10, 2019 at 6:55 PM Pavel Bar <pbar(a)redhat.com> wrote:
I was talking about other 2 lines:
> + if (rv != 0)
> + return NULL;
>
On Mon, Jun 10, 2019 at 6:53 PM Amit Bawer <abawer(a)redhat.com> wrote:
> that's why they are called redundant. nobody needs them.
> both pointer are initialized to NULL if not allocated, so call to free is
> safe by ISO-C standards.
>
> On Mon, Jun 10, 2019 at 5:29 PM Pavel Bar <pbar(a)redhat.com> wrote:
>
>>
>>
>> On Mon, Jun 3, 2019 at 11:31 PM Nir Soffer <nirsof(a)gmail.com> wrote:
>>
>>> From: Amit Bawer <abawer(a)redhat.com>
>>>
>>> Implications:
>>>
>>> - Py2 : no difference
>>> - Py3 : if used, only accept bytes for lockspace name.
>>> ---
>>> python/sanlock.c | 33 +++++++++++++++++++--------------
>>> 1 file changed, 19 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/python/sanlock.c b/python/sanlock.c
>>> index 0964671..ed9b4f0 100644
>>> --- a/python/sanlock.c
>>> +++ b/python/sanlock.c
>>> @@ -1226,59 +1226,64 @@ Align can be one of (1048576, 2097152, 4194304,
>>> 8388608).\n\
>>> Sector can be one of (512, 4096).");
>>>
>>> static PyObject *
>>> py_read_resource_owners(PyObject *self __unused, PyObject *args,
>>> PyObject *keywds)
>>> {
>>> - int rv, hss_count = 0;
>>> + int rv = -1, hss_count = 0;
>>> int sector = SECTOR_SIZE_512;
>>> long align = ALIGNMENT_1M;
>>> - const char *lockspace, *resource;
>>> + PyObject *lockspace = NULL, *resource = NULL;
>>> struct sanlk_resource *res = NULL;
>>> struct sanlk_host *hss = NULL;
>>> PyObject *disks, *ls_list = NULL;
>>>
>>> static char *kwlist[] = {"lockspace", "resource",
"disks", "align",
>>> "sector", NULL};
>>>
>>> /* parse python tuple */
>>> - if (!PyArg_ParseTupleAndKeywords(args, keywds, "ssO!|li",
kwlist,
>>> - &lockspace, &resource, &PyList_Type, &disks,
&align, §or))
>>> {
>>> - return NULL;
>>> + if (!PyArg_ParseTupleAndKeywords(args, keywds,
"O&O&O!|li", kwlist,
>>> + convert_to_pybytes, &lockspace, convert_to_pybytes,
&resource,
>>> + &PyList_Type, &disks, &align, §or)) {
>>> + goto finally;
>>> }
>>>
>>> /* parse and check sanlock resource */
>>> if (__parse_resource(disks, &res) < 0) {
>>> - return NULL;
>>> + goto finally;
>>> }
>>>
>>> /* prepare sanlock names */
>>> - strncpy(res->lockspace_name, lockspace, SANLK_NAME_LEN);
>>> - strncpy(res->name, resource, SANLK_NAME_LEN);
>>> + strncpy(res->lockspace_name, PyBytes_AsString(lockspace),
>>> SANLK_NAME_LEN);
>>> + strncpy(res->name, PyBytes_AsString(resource), SANLK_NAME_LEN);
>>>
>>> /* set resource alignment and sector flags */
>>>
>>> if (add_align_flag(align, &res->flags) == -1)
>>> - goto exit_fail;
>>> + goto finally;
>>>
>>> if (add_sector_flag(sector, &res->flags) == -1)
>>> - goto exit_fail;
>>> + goto finally;
>>>
>>> /* read resource owners (gil disabled) */
>>> Py_BEGIN_ALLOW_THREADS
>>> rv = sanlock_read_resource_owners(res, 0, &hss, &hss_count);
>>> Py_END_ALLOW_THREADS
>>>
>>> if (rv != 0) {
>>> __set_exception(rv, "Unable to read resource owners");
>>> - goto exit_fail;
>>> + goto finally;
>>> }
>>>
>>> ls_list = __hosts_to_list(hss, hss_count);
>>>
>>> -exit_fail:
>>> - if (res) free(res);
>>> - if (hss) free(hss);
>>> +finally:
>>> + Py_XDECREF(lockspace);
>>> + Py_XDECREF(resource);
>>> + free(res);
>>> + free(hss);
>>> + if (rv != 0)
>>> + return NULL;
>>>
>>
>> The above 2 lines (the "if" statement) are redundant, that's why
they
>> were not in the original code. Although they do add for readability, so
>> better leave them.
>>
>>
>>> return ls_list;
>>> }
>>>
>>> /* killpath */
>>> PyDoc_STRVAR(pydoc_killpath, "\
>>> --
>>> 2.17.2
>>>
>>>