Re: [Xen-devel] [PATCH v5 12/12] livepatch: Add python bindings for livepatch operations

From: Ross Lagerwall <ross.lagerwall@citrix.com>
To: Pawel Wieczorkiewicz <wipawel@amazon.de>,
	<xen-devel@lists.xenproject.org>
Cc: "Ian Jackson" <ian.jackson@eu.citrix.com>,
	mpohlack@amazon.com, "Wei Liu" <wl@xen.org>,
	"Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>
Subject: Re: [Xen-devel] [PATCH v5 12/12] livepatch: Add python bindings for livepatch operations
Date: Tue, 19 Nov 2019 17:23:07 +0000	[thread overview]
Message-ID: <15774787-b5a3-0817-6547-1c56290ccf5b@citrix.com> (raw)
In-Reply-To: <20191114130653.51185-13-wipawel@amazon.de>

On 11/14/19 1:06 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 None or throws an exception.
> - upload (pyxc_livepatch_upload):
>   Requires a payload name and a module's filename as an input.
>   Returns None or throws an exception.
> - 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@amazon.de>
> Reviewed-by: Martin Mazein <amazein@amazon.de>
> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
> Reviewed-by: Leonard Foerster <foersleo@amazon.de>
> Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
> Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
snip
> +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 = -1, saved_errno;

Does fd actually need to be initialized here?

Also, initializing it to 0 seems odd because 0 is a valid fd.

> +    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 ( fstat(fd, &buf) != 0 )
> +        goto error_fd;
> +
> +    len = buf.st_size;
> +    fbuf = mmap(0, len, PROT_READ, MAP_PRIVATE, fd, 0);
> +    if ( fbuf == MAP_FAILED )
> +        goto error_fd;
> +
> +    rc = xc_livepatch_upload(self->xc_handle, name, fbuf, len);
> +
> +    saved_errno = errno;
> +    munmap(fbuf, len);
> +    errno = saved_errno;
> +
> +error_fd:
> +    close(fd);
> +error:
> +    return rc ? pyxc_error_to_exception(self->xc_handle) : Py_None;
> +}
snip>  static PyMethodDef pyxc_methods[] = {
>      { "domain_create", 
>        (PyCFunction)pyxc_domain_create, 
> @@ -2542,6 +2761,44 @@ static PyMethodDef pyxc_methods[] = {
>        "Returns: [int]: 0 on all permission granted; -1 if any permissions are \
>         denied\n" }, 
>  
> +    { "livepatch_status",
> +      (PyCFunction)pyxc_livepatch_status,
> +      METH_KEYWORDS, "\n"
> +      "Gets current state and return code for a specified module.\n"
> +      " name     [str]: Module name to be used\n"
> +      "Returns: [dict] on success; throwing an exception on error\n"
> +      " state    [int]: Module current state: CHECKED or APPLIED\n"
> +      " rc       [int]: Return code of last module's operation\n" },
> +
> +    { "livepatch_upload",
> +      (PyCFunction)pyxc_livepatch_upload,
> +      METH_KEYWORDS, "\n"
> +      "Uploads a module with specified name from filename.\n"
> +      " name     [str]: Module name to be used\n"
> +      " filename [str]: Filename of a module to be uploaded\n"
> +      "Returns: None on success; throwing an exception on error\n" },
> +
> +    { "livepatch_action",
> +      (PyCFunction)pyxc_livepatch_action,
> +      METH_KEYWORDS, "\n"
> +      "Performs an action (unload, revert, apply or replace) on a specified \
> +       module.\n"
> +      " name      [str]: Module name to be used\n"
> +      " action   [uint]: Action enum id\n"
> +      " timeout  [uint]: Action scheduled execution timeout\n"
> +      " flags   [ulong]: Flags specifying action's extra parameters\n"

Should this be uint and not ulong?

I expect these things could be fixed up on commit.

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel