|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 12/12] livepatch: Add python bindings for livepatch operations
> On 25. Sep 2019, at 18:47, Ross Lagerwall <ross.lagerwall@xxxxxxxxxx> 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 <wipawel@xxxxxxxxx>
>> Reviewed-by: Martin Mazein <amazein@xxxxxxxxx>
>> Reviewed-by: Andra-Irina Paraschiv <andraprs@xxxxxxxxxx>
>> Reviewed-by: Leonard Foerster <foersleo@xxxxxxxxx>
>> Reviewed-by: Norbert Manthey <nmanthey@xxxxxxxxx>
>> Acked-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
>
> 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,
>> + &name, &action, &timeout, &flags) )
>> + 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,
>> + &name, &filename))
>> + goto error;
>> +
>> + fd = open(filename, O_RDONLY);
>> + if ( fd < 0 )
>> + goto error;
>> +
>> + if ( stat(filename, &buf) != 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 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, &nr,
>> + &name_total_size,
>> &metadata_total_size);
>> + if ( rc )
>> + goto error;
>> +
>> + if ( nr == 0 )
>> + return PyList_New(0);
>> +
>> + rc = ENOMEM;
>> + info = malloc(nr * sizeof(*info));
>> + if ( !info )
>> + goto error;
>> +
>> + name = malloc(name_total_size * sizeof(*name));
>> + if ( !name )
>> + goto error;
>> +
>> + len = malloc(nr * sizeof(*len));
>> + if ( !len )
>> + goto error;
>> +
>> + metadata = malloc(metadata_total_size * sizeof(*metadata));
>> + if ( !metadata )
>> + goto error;
>> +
>> + metadata_len = malloc(nr * sizeof(*metadata_len));
>> + if ( !metadata_len )
>> + goto error;
>> +
>> + rc = xc_livepatch_list(self->xc_handle, nr, 0, info,
>> + name, len, name_total_size,
>> + metadata, metadata_len, metadata_total_size,
>> + &done, &left);
>> + if ( rc )
>> + goto error;
>
> Should you also check done and left as is done in xen-livepatch.c?
>
> if ( rc || done != nr || left > 0)
>
Yes, I will add that.
>> +
>> + list = PyList_New(done);
>> + name_off = metadata_off = 0;
>> + for ( i = 0; i < done; i++ )
>> + {
>> + PyObject *info_dict, *metadata_list;
>> + char *name_str, *metadata_str;
>> +
>> + name_str = name + name_off;
>> + metadata_str = metadata + metadata_off;
>> +
>> + metadata_list = PyList_New(0);
>> + for ( char *s = metadata_str; s < metadata_str + metadata_len[i]; s
>> += strlen(s) + 1 )
>> + {
>> + PyObject *field = Py_BuildValue("s", s);
>> + if ( field == NULL )
>> + {
>> + Py_DECREF(list);
>> + Py_DECREF(metadata_list);
>> + rc = EFAULT;
>> + goto error;
>> + }
>> +
>> + PyList_Append(metadata_list, field);
>> + Py_DECREF(field);
>> + }
>> +
>> + info_dict = Py_BuildValue(
>> + "{s:s,s:i,s:i,s:N}",
>> + "name", name_str,
>> + "state", info[i].state,
>> + "rc", info[i].rc,
>> + "metadata", metadata_list);
>> +
>> + if ( info_dict == NULL )
>> + {
>> + Py_DECREF(list);
>> + Py_DECREF(metadata_list);
>> + rc = EFAULT;
>> + goto error;
>> + }
>> + PyList_SetItem(list, i, info_dict);
>> + Py_DECREF(info_dict);
>
> You can use PyList_SET_ITEM() to avoid the need for PyDECREF.
OK, will do.
>
> Thanks,
> --
> Ross Lagerwall
Best Regards,
Pawel Wieczorkiewicz
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |