Re: [PATCH v2 4/8] python: Extract create_resource() helper

[Vojtech Juranek]

On pondělí 17. června 2019 12:04:35 CEST Pavel Bar wrote:
> On Sun, Jun 16, 2019 at 11:50 PM Nir Soffer  wrote:
> > Replace duplicate code for allocating and initializing sanlk_resource
> > struct with a helper.
> > Signed-off-by: Nir Soffer
> >
> > 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.

agree, IMHO good idea to initialize variables once they are declared
> >  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,
> >
> > &sector)) {
> >      goto finally;
> >
> > --
> > 2.17.2