Re: [Xen-devel] [PATCH v3 12/12] livepatch: Add python bindings for livepatch operations
> On 25. Sep 2019, at 18:47, Ross Lagerwall wrote:
>
> On 9/16/19 12:40 PM, Pawel Wieczorkiewicz wrote:
>> Extend the XC python bindings library to support also all common
>> livepatch operations and actions.
>> Add the python bindings for the following operations:
>> - status (pyxc_livepatch_status):
>> Requires a payload name as an input.
>> Returns a status dict containing a state string and a return code
>> integer.
>> - action (pyxc_livepatch_action):
>> Requires a payload name and an action id as an input. Timeout and
>> flags are optional parameters.
>> Returns a return code integer.
>> - upload (pyxc_livepatch_upload):
>> Requires a payload name and a module's filename as an input.
>> Returns a return code integer.
>> - list (pyxc_livepatch_list):
>> Takes no parameters.
>> Returns a list of dicts containing each payload's:
>> * name as a string
>> * state as a string
>> * return code as an integer
>> * list of metadata key=value strings
>> Each functions throws an exception error based on the errno value
>> received from its corresponding libxc function call.
>> Signed-off-by: Pawel Wieczorkiewicz
>> Reviewed-by: Martin Mazein
>> Reviewed-by: Andra-Irina Paraschiv
>> Reviewed-by: Leonard Foerster
>> Reviewed-by: Norbert Manthey
>> Acked-by: Marek Marczykowski-Górecki
>
> This will be very useful, thanks!
>
>> ---
>> Changed since v1:
>> * changed PyList_Append() with PyList_SetItem() as requested by
>> Marek
>> tools/python/xen/lowlevel/xc/xc.c | 273
>> ++
>> 1 file changed, 273 insertions(+)
> snip> +static PyObject *pyxc_livepatch_action(XcObject *self,
>> + PyObject *args,
>> + PyObject *kwds)
>> +{
>> +int (*action_func)(xc_interface *xch, char *name, uint32_t timeout,
>> uint64_t flags);
>> +char *name;
>> +unsigned int action;
>> +uint32_t timeout;
>> +uint64_t flags;
>> +int rc;
>> +
>> +static char *kwd_list[] = { "name", "action", "timeout", "flags", NULL
>> };
>> +
>> +if ( !PyArg_ParseTupleAndKeywords(args, kwds, "sI|Ik", kwd_list,
>> + , , , ) )
>> +goto error;
>> +
>> +switch (action)
>> +{
>> +case LIVEPATCH_ACTION_UNLOAD:
>> +action_func = xc_livepatch_unload;
>> +break;
>> +case LIVEPATCH_ACTION_REVERT:
>> +action_func = xc_livepatch_revert;
>> +break;
>> +case LIVEPATCH_ACTION_APPLY:
>> +action_func = xc_livepatch_apply;
>> +break;
>> +case LIVEPATCH_ACTION_REPLACE:
>> +action_func = xc_livepatch_replace;
>> +break;
>> +default:
>> +goto error;
>> +}
>> +
>> +rc = action_func(self->xc_handle, name, timeout, flags);
>> +if ( rc )
>> +goto error;
>> +
>> +return Py_BuildValue("i", rc);
>
> For this and all the other functions which return zero on success, IMO
> returning None would be more Pythonic.
>
OK, will change.
>> +error:
>> +return pyxc_error_to_exception(self->xc_handle);
>> +}
>> +
>> +static PyObject *pyxc_livepatch_upload(XcObject *self,
>> + PyObject *args,
>> + PyObject *kwds)
>> +{
>> +unsigned char *fbuf = MAP_FAILED;
>> +char *name, *filename;
>> +struct stat buf;
>> +int fd = 0, rc;
>> +ssize_t len;
>> +
>> +static char *kwd_list[] = { "name", "filename", NULL };
>> +
>> +if ( !PyArg_ParseTupleAndKeywords(args, kwds, "ss", kwd_list,
>> + , ))
>> +goto error;
>> +
>> +fd = open(filename, O_RDONLY);
>> +if ( fd < 0 )
>> +goto error;
>> +
>> +if ( stat(filename, ) != 0 )
>> +goto error;
>
> I think it would be better to use fstat() to avoid a second path lookup
> potentially pointing to a different file.
>
Ah, certainly! Will fix.
>> +
>> +len = buf.st_size;
>> +fbuf = mmap(0, len, PROT_READ, MAP_PRIVATE, fd, 0);
>> +if ( fbuf == MAP_FAILED )
>> +goto error;
>> +
>> +rc = xc_livepatch_upload(self->xc_handle, name, fbuf, len);
>> +if ( rc )
>> +goto error;
>> +
>> +if ( munmap(fbuf, len) )
>> +{
>> +fbuf = MAP_FAILED;
>> +goto error;
>> +}
>> +close(fd);
>> +
>> +return Py_BuildValue("i", rc);;
>
> Stray semicolon
ACK
>
>> +error:
>> +if ( fbuf != MAP_FAILED )
>> +munmap(fbuf, len);
>> +if ( fd >= 0 )
>> +close(fd);
>
> You should probably save & restore errno so you can return the original error.
>
Yes, that’s right. Will fix.
>> +return pyxc_error_to_exception(self->xc_handle);
>
> Maybe you can have a conditional return to avoid duplicating the munmap() &
> close()? E.g.
>
> return rc ? pyxc_error_to_exception(self->xc_handle) : …
>
Oh, this indeed can work. Let me apply that. Thanks.
>> +}
>> +
>> +static
Re: [Xen-devel] [PATCH v3 12/12] livepatch: Add python bindings for livepatch operations
On 9/16/19 12:40 PM, Pawel Wieczorkiewicz wrote:
Extend the XC python bindings library to support also all common
livepatch operations and actions.
Add the python bindings for the following operations:
- status (pyxc_livepatch_status):
Requires a payload name as an input.
Returns a status dict containing a state string and a return code
integer.
- action (pyxc_livepatch_action):
Requires a payload name and an action id as an input. Timeout and
flags are optional parameters.
Returns a return code integer.
- upload (pyxc_livepatch_upload):
Requires a payload name and a module's filename as an input.
Returns a return code integer.
- list (pyxc_livepatch_list):
Takes no parameters.
Returns a list of dicts containing each payload's:
* name as a string
* state as a string
* return code as an integer
* list of metadata key=value strings
Each functions throws an exception error based on the errno value
received from its corresponding libxc function call.
Signed-off-by: Pawel Wieczorkiewicz
Reviewed-by: Martin Mazein
Reviewed-by: Andra-Irina Paraschiv
Reviewed-by: Leonard Foerster
Reviewed-by: Norbert Manthey
Acked-by: Marek Marczykowski-Górecki
This will be very useful, thanks!
---
Changed since v1:
* changed PyList_Append() with PyList_SetItem() as requested by
Marek
tools/python/xen/lowlevel/xc/xc.c | 273 ++
1 file changed, 273 insertions(+)
snip> +static PyObject *pyxc_livepatch_action(XcObject *self,
+ PyObject *args,
+ PyObject *kwds)
+{
+int (*action_func)(xc_interface *xch, char *name, uint32_t timeout,
uint64_t flags);
+char *name;
+unsigned int action;
+uint32_t timeout;
+uint64_t flags;
+int rc;
+
+static char *kwd_list[] = { "name", "action", "timeout", "flags", NULL };
+
+if ( !PyArg_ParseTupleAndKeywords(args, kwds, "sI|Ik", kwd_list,
+ , , , ) )
+goto error;
+
+switch (action)
+{
+case LIVEPATCH_ACTION_UNLOAD:
+action_func = xc_livepatch_unload;
+break;
+case LIVEPATCH_ACTION_REVERT:
+action_func = xc_livepatch_revert;
+break;
+case LIVEPATCH_ACTION_APPLY:
+action_func = xc_livepatch_apply;
+break;
+case LIVEPATCH_ACTION_REPLACE:
+action_func = xc_livepatch_replace;
+break;
+default:
+goto error;
+}
+
+rc = action_func(self->xc_handle, name, timeout, flags);
+if ( rc )
+goto error;
+
+return Py_BuildValue("i", rc);
For this and all the other functions which return zero on success, IMO
returning None would be more Pythonic.
+error:
+return pyxc_error_to_exception(self->xc_handle);
+}
+
+static PyObject *pyxc_livepatch_upload(XcObject *self,
+ PyObject *args,
+ PyObject *kwds)
+{
+unsigned char *fbuf = MAP_FAILED;
+char *name, *filename;
+struct stat buf;
+int fd = 0, rc;
+ssize_t len;
+
+static char *kwd_list[] = { "name", "filename", NULL };
+
+if ( !PyArg_ParseTupleAndKeywords(args, kwds, "ss", kwd_list,
+ , ))
+goto error;
+
+fd = open(filename, O_RDONLY);
+if ( fd < 0 )
+goto error;
+
+if ( stat(filename, ) != 0 )
+goto error;
I think it would be better to use fstat() to avoid a second path lookup
potentially pointing to a different file.
+
+len = buf.st_size;
+fbuf = mmap(0, len, PROT_READ, MAP_PRIVATE, fd, 0);
+if ( fbuf == MAP_FAILED )
+goto error;
+
+rc = xc_livepatch_upload(self->xc_handle, name, fbuf, len);
+if ( rc )
+goto error;
+
+if ( munmap(fbuf, len) )
+{
+fbuf = MAP_FAILED;
+goto error;
+}
+close(fd);
+
+return Py_BuildValue("i", rc);;
Stray semicolon
+error:
+if ( fbuf != MAP_FAILED )
+munmap(fbuf, len);
+if ( fd >= 0 )
+close(fd);
You should probably save & restore errno so you can return the original
error.
+return pyxc_error_to_exception(self->xc_handle);
Maybe you can have a conditional return to avoid duplicating the
munmap() & close()? E.g.
return rc ? pyxc_error_to_exception(self->xc_handle) : ...
+}
+
+static PyObject *pyxc_livepatch_list(XcObject *self)
+{
+PyObject *list;
+unsigned int nr, done, left, i;
+xen_livepatch_status_t *info = NULL;
+char *name = NULL;
+char *metadata = NULL;
+uint32_t *len = NULL;
+uint32_t *metadata_len = NULL;
+uint64_t name_total_size, metadata_total_size;
+off_t name_off, metadata_off;
+int rc;
+
+rc = xc_livepatch_list_get_sizes(self->xc_handle, ,
+ _total_size, _total_size);
+if ( rc )
+goto error;
+
+if ( nr == 0 )
+return PyList_New(0);
+
+rc =
