diff mbox series

[5/6] tools/pygrub: Expose libfsimage's fdopen() to python

Message ID 20231106150508.22665-6-alejandro.vallejo@cloud.com (mailing list archive)
State New, archived
Headers show
Series Pygrub security enhancements and bugfixes | expand

Commit Message

Alejandro Vallejo Nov. 6, 2023, 3:05 p.m. UTC 
Create a wrapper for the new fdopen() function of libfsimage.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 tools/pygrub/src/fsimage/fsimage.c | 33 ++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

Comments

Andrew Cooper Nov. 22, 2023, 10:35 p.m. UTC | #1 
On 06/11/2023 3:05 pm, Alejandro Vallejo wrote:
> Create a wrapper for the new fdopen() function of libfsimage.
>
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>

I'd appreciate it if Marek would cast his eye (as python maintainer)
over it.

That said, ...

> diff --git a/tools/pygrub/src/fsimage/fsimage.c b/tools/pygrub/src/fsimage/fsimage.c
> index 12dfcff6e3..216f265331 100644
> --- a/tools/pygrub/src/fsimage/fsimage.c
> +++ b/tools/pygrub/src/fsimage/fsimage.c
> @@ -270,6 +270,30 @@ fsimage_open(PyObject *o, PyObject *args, PyObject *kwargs)
>  	return (PyObject *)fs;
>  }
>  
> +static PyObject *
> +fsimage_fdopen(PyObject *o, PyObject *args, PyObject *kwargs)
> +{
> +	static char *kwlist[] = { "fd", "offset", "options", NULL };
> +	int fd;
> +	char *options = NULL;
> +	uint64_t offset = 0;
> +	fsimage_fs_t *fs;
> +
> +	if (!PyArg_ParseTupleAndKeywords(args, kwargs, "i|Ls", kwlist,
> +	    &fd, &offset, &options))
> +		return (NULL);
> +
> +	if ((fs = PyObject_NEW(fsimage_fs_t, &fsimage_fs_type)) == NULL)
> +		return (NULL);
> +
> +	if ((fs->fs = fsi_fdopen_fsimage(fd, offset, options)) == NULL) {
> +		PyErr_SetFromErrno(PyExc_IOError);

Don't we need a Py_DECREF(fs) here to avoid leaking it?

~Andrew

> +		return (NULL);
> +	}
> +
> +	return (PyObject *)fs;
> +}
> +
>  static PyObject *
>  fsimage_getbootstring(PyObject *o, PyObject *args)
>  {
> @@ -302,6 +326,13 @@ PyDoc_STRVAR(fsimage_open__doc__,
>      "offset - offset of file system within file image.\n"
>      "options - mount options string.\n");
>  
> +PyDoc_STRVAR(fsimage_fdopen__doc__,
> +    "fdopen(fd, [offset=off]) - Use the file provided by the given fd as a filesystem image.\n"
> +    "\n"
> +    "fd - File descriptor to use.\n"
> +    "offset - offset of file system within file image.\n"
> +    "options - mount options string.\n");
> +
>  PyDoc_STRVAR(fsimage_getbootstring__doc__,
>      "getbootstring(fs) - Return the boot string needed for this file system "
>      "or NULL if none is needed.\n");
> @@ -315,6 +346,8 @@ static struct PyMethodDef fsimage_module_methods[] = {
>  	    METH_VARARGS, fsimage_init__doc__ },
>  	{ "open", (PyCFunction)fsimage_open,
>  	    METH_VARARGS|METH_KEYWORDS, fsimage_open__doc__ },
> +	{ "fdopen", (PyCFunction)fsimage_fdopen,
> +	    METH_VARARGS|METH_KEYWORDS, fsimage_fdopen__doc__ },
>  	{ "getbootstring", (PyCFunction)fsimage_getbootstring,
>  	    METH_VARARGS, fsimage_getbootstring__doc__ },
>  	{ NULL, NULL, 0, NULL }
Alejandro Vallejo Nov. 23, 2023, 5:10 p.m. UTC | #2 
On 22/11/2023 22:35, Andrew Cooper wrote:
> On 06/11/2023 3:05 pm, Alejandro Vallejo wrote:
>> Create a wrapper for the new fdopen() function of libfsimage.
>>
>> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> 
> I'd appreciate it if Marek would cast his eye (as python maintainer)
> over it.
> 
> That said, ...
> 
>> diff --git a/tools/pygrub/src/fsimage/fsimage.c b/tools/pygrub/src/fsimage/fsimage.c
>> index 12dfcff6e3..216f265331 100644
>> --- a/tools/pygrub/src/fsimage/fsimage.c
>> +++ b/tools/pygrub/src/fsimage/fsimage.c
>> @@ -270,6 +270,30 @@ fsimage_open(PyObject *o, PyObject *args, PyObject *kwargs)
>>   	return (PyObject *)fs;
>>   }
>>   
>> +static PyObject *
>> +fsimage_fdopen(PyObject *o, PyObject *args, PyObject *kwargs)
>> +{
>> +	static char *kwlist[] = { "fd", "offset", "options", NULL };
>> +	int fd;
>> +	char *options = NULL;
>> +	uint64_t offset = 0;
>> +	fsimage_fs_t *fs;
>> +
>> +	if (!PyArg_ParseTupleAndKeywords(args, kwargs, "i|Ls", kwlist,
>> +	    &fd, &offset, &options))
>> +		return (NULL);
>> +
>> +	if ((fs = PyObject_NEW(fsimage_fs_t, &fsimage_fs_type)) == NULL)
>> +		return (NULL);
>> +
>> +	if ((fs->fs = fsi_fdopen_fsimage(fd, offset, options)) == NULL) {
>> +		PyErr_SetFromErrno(PyExc_IOError);
> 
> Don't we need a Py_DECREF(fs) here to avoid leaking it?
> 
> ~Andrew
If so, there's a bug in fsimage_open() as well. The logic here identical
to the logic there.

Cheers,
Alejandro
diff mbox series

Patch

diff --git a/tools/pygrub/src/fsimage/fsimage.c b/tools/pygrub/src/fsimage/fsimage.c
index 12dfcff6e3..216f265331 100644
--- a/tools/pygrub/src/fsimage/fsimage.c
+++ b/tools/pygrub/src/fsimage/fsimage.c
@@ -270,6 +270,30 @@  fsimage_open(PyObject *o, PyObject *args, PyObject *kwargs)
 	return (PyObject *)fs;
 }
 
+static PyObject *
+fsimage_fdopen(PyObject *o, PyObject *args, PyObject *kwargs)
+{
+	static char *kwlist[] = { "fd", "offset", "options", NULL };
+	int fd;
+	char *options = NULL;
+	uint64_t offset = 0;
+	fsimage_fs_t *fs;
+
+	if (!PyArg_ParseTupleAndKeywords(args, kwargs, "i|Ls", kwlist,
+	    &fd, &offset, &options))
+		return (NULL);
+
+	if ((fs = PyObject_NEW(fsimage_fs_t, &fsimage_fs_type)) == NULL)
+		return (NULL);
+
+	if ((fs->fs = fsi_fdopen_fsimage(fd, offset, options)) == NULL) {
+		PyErr_SetFromErrno(PyExc_IOError);
+		return (NULL);
+	}
+
+	return (PyObject *)fs;
+}
+
 static PyObject *
 fsimage_getbootstring(PyObject *o, PyObject *args)
 {
@@ -302,6 +326,13 @@  PyDoc_STRVAR(fsimage_open__doc__,
     "offset - offset of file system within file image.\n"
     "options - mount options string.\n");
 
+PyDoc_STRVAR(fsimage_fdopen__doc__,
+    "fdopen(fd, [offset=off]) - Use the file provided by the given fd as a filesystem image.\n"
+    "\n"
+    "fd - File descriptor to use.\n"
+    "offset - offset of file system within file image.\n"
+    "options - mount options string.\n");
+
 PyDoc_STRVAR(fsimage_getbootstring__doc__,
     "getbootstring(fs) - Return the boot string needed for this file system "
     "or NULL if none is needed.\n");
@@ -315,6 +346,8 @@  static struct PyMethodDef fsimage_module_methods[] = {
 	    METH_VARARGS, fsimage_init__doc__ },
 	{ "open", (PyCFunction)fsimage_open,
 	    METH_VARARGS|METH_KEYWORDS, fsimage_open__doc__ },
+	{ "fdopen", (PyCFunction)fsimage_fdopen,
+	    METH_VARARGS|METH_KEYWORDS, fsimage_fdopen__doc__ },
 	{ "getbootstring", (PyCFunction)fsimage_getbootstring,
 	    METH_VARARGS, fsimage_getbootstring__doc__ },
 	{ NULL, NULL, 0, NULL }