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

[Nir Soffer]

On Mon, Jun 17, 2019 at 1:04 PM Pavel Bar <pbar(a)redhat.com&gt; wrote:

>  On Sun, Jun 16, 2019 at 11:50 PM Nir Soffer <nirsof(a)gmail.com&gt; wrote:
>
> > Replace duplicate code for allocating and initializing sanlk_resource
> > struct with a helper.
> >
> > Signed-off-by: Nir Soffer <nsoffer(a)redhat.com&gt;
> > ---
> >  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.


> >      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
> >
> >

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

[Vojtech Juranek]

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&gt;
> wrote:
>  > On Sun, Jun 16, 2019 at 11:50 PM Nir Soffer <nirsof(a)gmail.com&gt; wrote:
>  >> Replace duplicate code for allocating and initializing sanlk_resource
>  >> struct with a helper.
>  >> 
>  >> Signed-off-by: Nir Soffer <nsoffer(a)redhat.com&gt;
>  >> ---
>  >> 
>  >>  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,
>  >> 
>  >> &sector)) {
>  >> 
>  >>          goto finally;
>  >> 
>  >> --
>  >> 2.17.2