* [PATCH 0/8] virtiofsd: Changed various allocations to GLib functions
@ 2021-03-19 13:25 Mahmoud Mandour
2021-03-19 13:25 ` [PATCH 1/8] virtiofsd: Changed allocations of fuse_req " Mahmoud Mandour
` (8 more replies)
0 siblings, 9 replies; 24+ messages in thread
From: Mahmoud Mandour @ 2021-03-19 13:25 UTC (permalink / raw)
To: qemu-devel; +Cc: Mahmoud Mandour
Replaced allocations done using malloc(), calloc(), and realloc()
to their equivalent functions in GLib.
Memory that is allocated locally and freed when the function exits
are annotated g_autofree so that the deallocation is automatically
handled. Subsequently, I could remove a bunch of free() calls.
Also, tried to keep the semantics of the code as is, but when the
allocation is a small one, or a crucial one, I replaced the
NULL-checking mechanisms with glib's functions that crash on error.
This is related to a patch that I had submitted as a part of a
previous series. The previous patch had some errors. Also, I thought
that it's better to split the patch into smaller pieces.
The previous patch can be found here:
https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg05153.html
Mahmoud Mandour (8):
virtiofsd: Changed allocations of fuse_req to GLib functions
virtiofds: Changed allocations of iovec to GLib's functions
virtiofsd: Changed fuse_pollhandle allocation to GLib's functions
virtiofsd: Changed allocations of fuse_session to GLib's functions
virtiofsd: Changed allocation of lo_map_elems to GLib's functions
virtiofsd: Changed allocations of fv_VuDev & its internals to GLib
functions
virtiofsd/passthrough_ll.c: Changed local allocations to GLib
functions
virtiofsd/fuse_virtio.c: Changed allocations of locals to GLib
tools/virtiofsd/fuse_lowlevel.c | 43 +++++++++++---------------------
tools/virtiofsd/fuse_virtio.c | 34 ++++++++-----------------
tools/virtiofsd/passthrough_ll.c | 21 ++++++----------
3 files changed, 34 insertions(+), 64 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/8] virtiofsd: Changed allocations of fuse_req to GLib functions
2021-03-19 13:25 [PATCH 0/8] virtiofsd: Changed various allocations to GLib functions Mahmoud Mandour
@ 2021-03-19 13:25 ` Mahmoud Mandour
2021-03-23 13:48 ` Stefan Hajnoczi
2021-03-19 13:25 ` [PATCH 2/8] virtiofds: Changed allocations of iovec to GLib's functions Mahmoud Mandour
` (7 subsequent siblings)
8 siblings, 1 reply; 24+ messages in thread
From: Mahmoud Mandour @ 2021-03-19 13:25 UTC (permalink / raw)
To: qemu-devel; +Cc: Mahmoud Mandour
Replaced the allocation and deallocation of fuse_req structs
using calloc()/free() call pairs to a GLib's g_try_new0()
and g_free().
Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
---
tools/virtiofsd/fuse_lowlevel.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
index 1aa26c6333..ba20c73778 100644
--- a/tools/virtiofsd/fuse_lowlevel.c
+++ b/tools/virtiofsd/fuse_lowlevel.c
@@ -106,7 +106,7 @@ static void list_add_req(struct fuse_req *req, struct fuse_req *next)
static void destroy_req(fuse_req_t req)
{
pthread_mutex_destroy(&req->lock);
- free(req);
+ g_free(req);
}
void fuse_free_req(fuse_req_t req)
@@ -130,7 +130,7 @@ static struct fuse_req *fuse_ll_alloc_req(struct fuse_session *se)
{
struct fuse_req *req;
- req = (struct fuse_req *)calloc(1, sizeof(struct fuse_req));
+ req = g_try_new0(struct fuse_req, 1);
if (req == NULL) {
fuse_log(FUSE_LOG_ERR, "fuse: failed to allocate request\n");
} else {
@@ -1684,7 +1684,7 @@ static struct fuse_req *check_interrupt(struct fuse_session *se,
if (curr->u.i.unique == req->unique) {
req->interrupted = 1;
list_del_req(curr);
- free(curr);
+ g_free(curr);
return NULL;
}
}
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 2/8] virtiofds: Changed allocations of iovec to GLib's functions
2021-03-19 13:25 [PATCH 0/8] virtiofsd: Changed various allocations to GLib functions Mahmoud Mandour
2021-03-19 13:25 ` [PATCH 1/8] virtiofsd: Changed allocations of fuse_req " Mahmoud Mandour
@ 2021-03-19 13:25 ` Mahmoud Mandour
2021-03-23 13:57 ` Stefan Hajnoczi
2021-03-19 13:25 ` [PATCH 3/8] virtiofsd: Changed fuse_pollhandle allocation " Mahmoud Mandour
` (6 subsequent siblings)
8 siblings, 1 reply; 24+ messages in thread
From: Mahmoud Mandour @ 2021-03-19 13:25 UTC (permalink / raw)
To: qemu-devel; +Cc: Mahmoud Mandour
Replaced the calls to malloc()/calloc() and their respective
calls to free() of iovec structs with GLib's allocation and
deallocation functions.
Also, in one instance, used g_new0() instead of a calloc() call plus
a null-checking assertion.
iovec structs were created locally and freed as the function
ends. Hence, I used g_autofree and removed the respective calls to
free().
In one instance, a struct fuse_ioctl_iovec pointer is returned from a
function, namely, fuse_ioctl_iovec_copy. There, I used g_steal_pointer()
in conjunction with g_autofree, this gives the ownership of the pointer
to the calling function and still auto-frees the memory when the calling
function finishes (maintaining the symantics of previous code).
Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
---
tools/virtiofsd/fuse_lowlevel.c | 19 +++++++------------
tools/virtiofsd/fuse_virtio.c | 6 +-----
2 files changed, 8 insertions(+), 17 deletions(-)
diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
index ba20c73778..66607100f2 100644
--- a/tools/virtiofsd/fuse_lowlevel.c
+++ b/tools/virtiofsd/fuse_lowlevel.c
@@ -217,9 +217,9 @@ static int send_reply(fuse_req_t req, int error, const void *arg,
int fuse_reply_iov(fuse_req_t req, const struct iovec *iov, int count)
{
int res;
- struct iovec *padded_iov;
+ g_autofree struct iovec *padded_iov;
- padded_iov = malloc((count + 1) * sizeof(struct iovec));
+ padded_iov = g_try_new(struct iovec, count + 1);
if (padded_iov == NULL) {
return fuse_reply_err(req, ENOMEM);
}
@@ -228,7 +228,6 @@ int fuse_reply_iov(fuse_req_t req, const struct iovec *iov, int count)
count++;
res = send_reply_iov(req, 0, padded_iov, count);
- free(padded_iov);
return res;
}
@@ -565,10 +564,10 @@ int fuse_reply_bmap(fuse_req_t req, uint64_t idx)
static struct fuse_ioctl_iovec *fuse_ioctl_iovec_copy(const struct iovec *iov,
size_t count)
{
- struct fuse_ioctl_iovec *fiov;
+ g_autofree struct fuse_ioctl_iovec *fiov;
size_t i;
- fiov = malloc(sizeof(fiov[0]) * count);
+ fiov = g_try_new(fuse_ioctl_iovec, count);
if (!fiov) {
return NULL;
}
@@ -578,7 +577,7 @@ static struct fuse_ioctl_iovec *fuse_ioctl_iovec_copy(const struct iovec *iov,
fiov[i].len = iov[i].iov_len;
}
- return fiov;
+ return g_steal_pointer(&fiov);
}
int fuse_reply_ioctl_retry(fuse_req_t req, const struct iovec *in_iov,
@@ -629,9 +628,6 @@ int fuse_reply_ioctl_retry(fuse_req_t req, const struct iovec *in_iov,
res = send_reply_iov(req, 0, iov, count);
out:
- free(in_fiov);
- free(out_fiov);
-
return res;
enomem:
@@ -663,11 +659,11 @@ int fuse_reply_ioctl(fuse_req_t req, int result, const void *buf, size_t size)
int fuse_reply_ioctl_iov(fuse_req_t req, int result, const struct iovec *iov,
int count)
{
- struct iovec *padded_iov;
+ g_autofree struct iovec *padded_iov;
struct fuse_ioctl_out arg;
int res;
- padded_iov = malloc((count + 2) * sizeof(struct iovec));
+ padded_iov = g_try_new(struct iovec, count + 2);
if (padded_iov == NULL) {
return fuse_reply_err(req, ENOMEM);
}
@@ -680,7 +676,6 @@ int fuse_reply_ioctl_iov(fuse_req_t req, int result, const struct iovec *iov,
memcpy(&padded_iov[2], iov, count * sizeof(struct iovec));
res = send_reply_iov(req, 0, padded_iov, count + 2);
- free(padded_iov);
return res;
}
diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 3e13997406..07e5d91a9f 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -347,8 +347,7 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
* Build a copy of the the in_sg iov so we can skip bits in it,
* including changing the offsets
*/
- struct iovec *in_sg_cpy = calloc(sizeof(struct iovec), in_num);
- assert(in_sg_cpy);
+ g_autofree struct iovec *in_sg_cpy = g_new0(struct iovec, in_num);
memcpy(in_sg_cpy, in_sg, sizeof(struct iovec) * in_num);
/* These get updated as we skip */
struct iovec *in_sg_ptr = in_sg_cpy;
@@ -386,7 +385,6 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
ret = errno;
fuse_log(FUSE_LOG_DEBUG, "%s: preadv failed (%m) len=%zd\n",
__func__, len);
- free(in_sg_cpy);
goto err;
}
fuse_log(FUSE_LOG_DEBUG, "%s: preadv ret=%d len=%zd\n", __func__,
@@ -410,13 +408,11 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
if (ret != len) {
fuse_log(FUSE_LOG_DEBUG, "%s: ret!=len\n", __func__);
ret = EIO;
- free(in_sg_cpy);
goto err;
}
in_sg_left -= ret;
len -= ret;
} while (in_sg_left);
- free(in_sg_cpy);
/* Need to fix out->len on EOF */
if (len) {
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 3/8] virtiofsd: Changed fuse_pollhandle allocation to GLib's functions
2021-03-19 13:25 [PATCH 0/8] virtiofsd: Changed various allocations to GLib functions Mahmoud Mandour
2021-03-19 13:25 ` [PATCH 1/8] virtiofsd: Changed allocations of fuse_req " Mahmoud Mandour
2021-03-19 13:25 ` [PATCH 2/8] virtiofds: Changed allocations of iovec to GLib's functions Mahmoud Mandour
@ 2021-03-19 13:25 ` Mahmoud Mandour
2021-03-23 14:03 ` Stefan Hajnoczi
2021-03-19 13:25 ` [PATCH 4/8] virtiofsd: Changed allocations of fuse_session " Mahmoud Mandour
` (5 subsequent siblings)
8 siblings, 1 reply; 24+ messages in thread
From: Mahmoud Mandour @ 2021-03-19 13:25 UTC (permalink / raw)
To: qemu-devel; +Cc: Mahmoud Mandour
Changed allocation of fuse_pollhandle structs to GLib's g_new().
Removed the null checking as allocating such a small memory segment
should always succeed on a healthy system. Otherwise, the system
is already in a critical state.
Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
---
tools/virtiofsd/fuse_lowlevel.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
index 66607100f2..45527ff703 100644
--- a/tools/virtiofsd/fuse_lowlevel.c
+++ b/tools/virtiofsd/fuse_lowlevel.c
@@ -1755,7 +1755,7 @@ static void do_ioctl(fuse_req_t req, fuse_ino_t nodeid,
void fuse_pollhandle_destroy(struct fuse_pollhandle *ph)
{
- free(ph);
+ g_free(ph);
}
static void do_poll(fuse_req_t req, fuse_ino_t nodeid,
@@ -1778,11 +1778,7 @@ static void do_poll(fuse_req_t req, fuse_ino_t nodeid,
struct fuse_pollhandle *ph = NULL;
if (arg->flags & FUSE_POLL_SCHEDULE_NOTIFY) {
- ph = malloc(sizeof(struct fuse_pollhandle));
- if (ph == NULL) {
- fuse_reply_err(req, ENOMEM);
- return;
- }
+ ph = g_new(struct fuse_pollhandle, 1);
ph->kh = arg->kh;
ph->se = req->se;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 4/8] virtiofsd: Changed allocations of fuse_session to GLib's functions
2021-03-19 13:25 [PATCH 0/8] virtiofsd: Changed various allocations to GLib functions Mahmoud Mandour
` (2 preceding siblings ...)
2021-03-19 13:25 ` [PATCH 3/8] virtiofsd: Changed fuse_pollhandle allocation " Mahmoud Mandour
@ 2021-03-19 13:25 ` Mahmoud Mandour
2021-03-23 14:08 ` Stefan Hajnoczi
2021-03-19 13:25 ` [PATCH 5/8] virtiofsd: Changed allocation of lo_map_elems " Mahmoud Mandour
` (4 subsequent siblings)
8 siblings, 1 reply; 24+ messages in thread
From: Mahmoud Mandour @ 2021-03-19 13:25 UTC (permalink / raw)
To: qemu-devel; +Cc: Mahmoud Mandour
Replaced the allocation and deallocation of fuse_session structs
from calloc() and free() calls to g_new0() and g_free().
Removed the NULL-check and used g_new0() mainly because fuse_session
creation is critical and an exit will occur anyway if fuse_session
allocation failed.
Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
---
tools/virtiofsd/fuse_lowlevel.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
index 45527ff703..b0e9ef29a7 100644
--- a/tools/virtiofsd/fuse_lowlevel.c
+++ b/tools/virtiofsd/fuse_lowlevel.c
@@ -2467,7 +2467,7 @@ void fuse_session_destroy(struct fuse_session *se)
free(se->vu_socket_path);
se->vu_socket_path = NULL;
- free(se);
+ g_free(se);
}
@@ -2490,11 +2490,7 @@ struct fuse_session *fuse_session_new(struct fuse_args *args,
return NULL;
}
- se = (struct fuse_session *)calloc(1, sizeof(struct fuse_session));
- if (se == NULL) {
- fuse_log(FUSE_LOG_ERR, "fuse: failed to allocate fuse object\n");
- goto out1;
- }
+ se = g_new0(struct fuse_session, 1);
se->fd = -1;
se->vu_listen_fd = -1;
se->thread_pool_size = THREAD_POOL_SIZE;
@@ -2550,7 +2546,7 @@ struct fuse_session *fuse_session_new(struct fuse_args *args,
out4:
fuse_opt_free_args(args);
out2:
- free(se);
+ g_free(se);
out1:
return NULL;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 5/8] virtiofsd: Changed allocation of lo_map_elems to GLib's functions
2021-03-19 13:25 [PATCH 0/8] virtiofsd: Changed various allocations to GLib functions Mahmoud Mandour
` (3 preceding siblings ...)
2021-03-19 13:25 ` [PATCH 4/8] virtiofsd: Changed allocations of fuse_session " Mahmoud Mandour
@ 2021-03-19 13:25 ` Mahmoud Mandour
2021-03-23 14:09 ` Stefan Hajnoczi
2021-03-19 13:25 ` [PATCH 6/8] virtiofsd: Changed allocations of fv_VuDev & its internals to GLib functions Mahmoud Mandour
` (3 subsequent siblings)
8 siblings, 1 reply; 24+ messages in thread
From: Mahmoud Mandour @ 2021-03-19 13:25 UTC (permalink / raw)
To: qemu-devel; +Cc: Mahmoud Mandour
Replaced (re)allocation of lo_map_elem structs from realloc() to
GLib's g_try_realloc_n() and replaced the respective free() call
with a g_free().
Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
---
tools/virtiofsd/passthrough_ll.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index b144320e48..d049013428 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -406,7 +406,7 @@ static void lo_map_init(struct lo_map *map)
static void lo_map_destroy(struct lo_map *map)
{
- free(map->elems);
+ g_free(map->elems);
}
static int lo_map_grow(struct lo_map *map, size_t new_nelems)
@@ -418,7 +418,7 @@ static int lo_map_grow(struct lo_map *map, size_t new_nelems)
return 1;
}
- new_elems = realloc(map->elems, sizeof(map->elems[0]) * new_nelems);
+ new_elems = g_try_realloc_n(map->elems, new_nelems, sizeof(map->elems[0]));
if (!new_elems) {
return 0;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 6/8] virtiofsd: Changed allocations of fv_VuDev & its internals to GLib functions
2021-03-19 13:25 [PATCH 0/8] virtiofsd: Changed various allocations to GLib functions Mahmoud Mandour
` (4 preceding siblings ...)
2021-03-19 13:25 ` [PATCH 5/8] virtiofsd: Changed allocation of lo_map_elems " Mahmoud Mandour
@ 2021-03-19 13:25 ` Mahmoud Mandour
2021-03-23 14:10 ` Stefan Hajnoczi
2021-03-19 13:25 ` [PATCH 7/8] virtiofsd/passthrough_ll.c: Changed local allocations " Mahmoud Mandour
` (2 subsequent siblings)
8 siblings, 1 reply; 24+ messages in thread
From: Mahmoud Mandour @ 2021-03-19 13:25 UTC (permalink / raw)
To: qemu-devel; +Cc: Mahmoud Mandour
Changed the allocations of fv_VuDev structs, VuDev structs, and
fv_QueueInfo strcuts from using calloc()/realloc() & free() to using
the equivalent functions from GLib.
In instances, removed the pair of allocation and assertion for
non-NULL checking with a GLib function that aborts on error.
Removed NULL-checking for fv_VuDev struct allocation and used
a GLib function that crashes on error; namely, g_new0(). This
is because allocating one struct should not be a problem on an
healthy system. Also following the pattern of aborting-on-null
behaviour that is taken with allocating VuDev structs and
fv_QueueInfo structs.
Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
---
tools/virtiofsd/fuse_virtio.c | 19 ++++++-------------
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 07e5d91a9f..5828b9a76f 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -729,7 +729,7 @@ static void fv_queue_cleanup_thread(struct fv_VuDev *vud, int qidx)
pthread_mutex_destroy(&ourqi->vq_lock);
close(ourqi->kill_fd);
ourqi->kick_fd = -1;
- free(vud->qi[qidx]);
+ g_free(vud->qi[qidx]);
vud->qi[qidx] = NULL;
}
@@ -760,15 +760,13 @@ static void fv_queue_set_started(VuDev *dev, int qidx, bool started)
if (started) {
/* Fire up a thread to watch this queue */
if (qidx >= vud->nqueues) {
- vud->qi = realloc(vud->qi, (qidx + 1) * sizeof(vud->qi[0]));
- assert(vud->qi);
+ vud->qi = g_realloc_n(vud->qi, qidx + 1, sizeof(vud->qi[0]));
memset(vud->qi + vud->nqueues, 0,
sizeof(vud->qi[0]) * (1 + (qidx - vud->nqueues)));
vud->nqueues = qidx + 1;
}
if (!vud->qi[qidx]) {
- vud->qi[qidx] = calloc(sizeof(struct fv_QueueInfo), 1);
- assert(vud->qi[qidx]);
+ vud->qi[qidx] = g_new0(struct fv_QueueInfo, 1);
vud->qi[qidx]->virtio_dev = vud;
vud->qi[qidx]->qidx = qidx;
} else {
@@ -1034,12 +1032,7 @@ int virtio_session_mount(struct fuse_session *se)
__func__);
/* TODO: Some cleanup/deallocation! */
- se->virtio_dev = calloc(sizeof(struct fv_VuDev), 1);
- if (!se->virtio_dev) {
- fuse_log(FUSE_LOG_ERR, "%s: virtio_dev calloc failed\n", __func__);
- close(data_sock);
- return -1;
- }
+ se->virtio_dev = g_new0(struct fv_VuDev, 1);
se->vu_socketfd = data_sock;
se->virtio_dev->se = se;
@@ -1061,8 +1054,8 @@ void virtio_session_close(struct fuse_session *se)
return;
}
- free(se->virtio_dev->qi);
+ g_free(se->virtio_dev->qi);
pthread_rwlock_destroy(&se->virtio_dev->vu_dispatch_rwlock);
- free(se->virtio_dev);
+ g_free(se->virtio_dev);
se->virtio_dev = NULL;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 7/8] virtiofsd/passthrough_ll.c: Changed local allocations to GLib functions
2021-03-19 13:25 [PATCH 0/8] virtiofsd: Changed various allocations to GLib functions Mahmoud Mandour
` (5 preceding siblings ...)
2021-03-19 13:25 ` [PATCH 6/8] virtiofsd: Changed allocations of fv_VuDev & its internals to GLib functions Mahmoud Mandour
@ 2021-03-19 13:25 ` Mahmoud Mandour
2021-03-23 14:13 ` Stefan Hajnoczi
2021-03-19 13:25 ` [PATCH 8/8] virtiofsd/fuse_virtio.c: Changed allocations of locals to GLib Mahmoud Mandour
2021-03-19 13:47 ` [PATCH 0/8] virtiofsd: Changed various allocations to GLib functions Mahmoud Mandour
8 siblings, 1 reply; 24+ messages in thread
From: Mahmoud Mandour @ 2021-03-19 13:25 UTC (permalink / raw)
To: qemu-devel; +Cc: Mahmoud Mandour
Changed the allocations of some local variables to GLib's allocation
functions, such as g_try_malloc0(), and annotated those variables
as g_autofree. Subsequently, I was able to remove the calls to free().
Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
---
tools/virtiofsd/passthrough_ll.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index d049013428..4263b383f0 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -1653,7 +1653,7 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t ino, size_t size,
struct lo_data *lo = lo_data(req);
struct lo_dirp *d = NULL;
struct lo_inode *dinode;
- char *buf = NULL;
+ g_autofree char *buf = NULL;
char *p;
size_t rem = size;
int err = EBADF;
@@ -1669,7 +1669,7 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t ino, size_t size,
}
err = ENOMEM;
- buf = calloc(1, size);
+ buf = g_try_malloc0(size);
if (!buf) {
goto error;
}
@@ -1755,7 +1755,6 @@ error:
} else {
fuse_reply_buf(req, buf, size - rem);
}
- free(buf);
}
static void lo_readdir(fuse_req_t req, fuse_ino_t ino, size_t size,
@@ -2726,7 +2725,7 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
size_t size)
{
struct lo_data *lo = lo_data(req);
- char *value = NULL;
+ g_autofree char *value = NULL;
char procname[64];
const char *name;
char *mapped_name;
@@ -2767,7 +2766,7 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
ino, name, size);
if (size) {
- value = malloc(size);
+ value = g_try_malloc(size);
if (!value) {
goto out_err;
}
@@ -2806,8 +2805,6 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
fuse_reply_xattr(req, ret);
}
out_free:
- free(value);
-
if (fd >= 0) {
close(fd);
}
@@ -2826,7 +2823,7 @@ out:
static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
{
struct lo_data *lo = lo_data(req);
- char *value = NULL;
+ g_autofree char *value = NULL;
char procname[64];
struct lo_inode *inode;
ssize_t ret;
@@ -2848,7 +2845,7 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
size);
if (size) {
- value = malloc(size);
+ value = g_try_malloc(size);
if (!value) {
goto out_err;
}
@@ -2933,8 +2930,6 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
fuse_reply_xattr(req, ret);
}
out_free:
- free(value);
-
if (fd >= 0) {
close(fd);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 8/8] virtiofsd/fuse_virtio.c: Changed allocations of locals to GLib
2021-03-19 13:25 [PATCH 0/8] virtiofsd: Changed various allocations to GLib functions Mahmoud Mandour
` (6 preceding siblings ...)
2021-03-19 13:25 ` [PATCH 7/8] virtiofsd/passthrough_ll.c: Changed local allocations " Mahmoud Mandour
@ 2021-03-19 13:25 ` Mahmoud Mandour
2021-03-23 14:15 ` Stefan Hajnoczi
2021-03-19 13:47 ` [PATCH 0/8] virtiofsd: Changed various allocations to GLib functions Mahmoud Mandour
8 siblings, 1 reply; 24+ messages in thread
From: Mahmoud Mandour @ 2021-03-19 13:25 UTC (permalink / raw)
To: qemu-devel; +Cc: Mahmoud Mandour
Replaced the allocation of local variables from malloc() to
GLib allocation functions.
In one instance, dropped the usage to an assert after a malloc()
call and used g_malloc() instead.
Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
---
tools/virtiofsd/fuse_virtio.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 5828b9a76f..587403b026 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -472,8 +472,7 @@ static void fv_queue_worker(gpointer data, gpointer user_data)
* They're spread over multiple descriptors in a scatter/gather set
* and we can't trust the guest to keep them still; so copy in/out.
*/
- fbuf.mem = malloc(se->bufsize);
- assert(fbuf.mem);
+ fbuf.mem = g_malloc(se->bufsize);
fuse_mutex_init(&req->ch.lock);
req->ch.fd = -1;
@@ -524,7 +523,7 @@ static void fv_queue_worker(gpointer data, gpointer user_data)
fbuf.size = out_sg[0].iov_len + out_sg[1].iov_len;
/* Allocate the bufv, with space for the rest of the iov */
- pbufv = malloc(sizeof(struct fuse_bufvec) +
+ pbufv = g_try_malloc(sizeof(struct fuse_bufvec) +
sizeof(struct fuse_buf) * (out_num - 2));
if (!pbufv) {
fuse_log(FUSE_LOG_ERR, "%s: pbufv malloc failed\n",
@@ -569,7 +568,7 @@ static void fv_queue_worker(gpointer data, gpointer user_data)
out:
if (allocated_bufv) {
- free(pbufv);
+ g_free(pbufv);
}
/* If the request has no reply, still recycle the virtqueue element */
@@ -588,7 +587,7 @@ out:
}
pthread_mutex_destroy(&req->ch.lock);
- free(fbuf.mem);
+ g_free(fbuf.mem);
free(req);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 0/8] virtiofsd: Changed various allocations to GLib functions
2021-03-19 13:25 [PATCH 0/8] virtiofsd: Changed various allocations to GLib functions Mahmoud Mandour
` (7 preceding siblings ...)
2021-03-19 13:25 ` [PATCH 8/8] virtiofsd/fuse_virtio.c: Changed allocations of locals to GLib Mahmoud Mandour
@ 2021-03-19 13:47 ` Mahmoud Mandour
2021-03-22 15:52 ` Stefan Hajnoczi
8 siblings, 1 reply; 24+ messages in thread
From: Mahmoud Mandour @ 2021-03-19 13:47 UTC (permalink / raw)
To: qemu-devel, Stefan Hajnoczi, Dr. David Alan Gilbert
[-- Attachment #1: Type: text/plain, Size: 1928 bytes --]
On Fri, Mar 19, 2021 at 3:25 PM Mahmoud Mandour <ma.mandourr@gmail.com>
wrote:
> Replaced allocations done using malloc(), calloc(), and realloc()
> to their equivalent functions in GLib.
>
> Memory that is allocated locally and freed when the function exits
> are annotated g_autofree so that the deallocation is automatically
> handled. Subsequently, I could remove a bunch of free() calls.
>
> Also, tried to keep the semantics of the code as is, but when the
> allocation is a small one, or a crucial one, I replaced the
> NULL-checking mechanisms with glib's functions that crash on error.
>
> This is related to a patch that I had submitted as a part of a
> previous series. The previous patch had some errors. Also, I thought
> that it's better to split the patch into smaller pieces.
>
> The previous patch can be found here:
> https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg05153.html
>
> Mahmoud Mandour (8):
> virtiofsd: Changed allocations of fuse_req to GLib functions
> virtiofds: Changed allocations of iovec to GLib's functions
> virtiofsd: Changed fuse_pollhandle allocation to GLib's functions
> virtiofsd: Changed allocations of fuse_session to GLib's functions
> virtiofsd: Changed allocation of lo_map_elems to GLib's functions
> virtiofsd: Changed allocations of fv_VuDev & its internals to GLib
> functions
> virtiofsd/passthrough_ll.c: Changed local allocations to GLib
> functions
> virtiofsd/fuse_virtio.c: Changed allocations of locals to GLib
>
> tools/virtiofsd/fuse_lowlevel.c | 43 +++++++++++---------------------
> tools/virtiofsd/fuse_virtio.c | 34 ++++++++-----------------
> tools/virtiofsd/passthrough_ll.c | 21 ++++++----------
> 3 files changed, 34 insertions(+), 64 deletions(-)
>
> --
> 2.25.1
>
>
Hello,
For some reason, my get_maintainers script auto cc-filling did not work, so
I had to manually cc
you.
Sorry for the inconvenience.
Mahmoud
[-- Attachment #2: Type: text/html, Size: 2609 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/8] virtiofsd: Changed various allocations to GLib functions
2021-03-19 13:47 ` [PATCH 0/8] virtiofsd: Changed various allocations to GLib functions Mahmoud Mandour
@ 2021-03-22 15:52 ` Stefan Hajnoczi
2021-04-16 8:43 ` Mahmoud Mandour
0 siblings, 1 reply; 24+ messages in thread
From: Stefan Hajnoczi @ 2021-03-22 15:52 UTC (permalink / raw)
To: Mahmoud Mandour; +Cc: qemu-devel, Stefan Hajnoczi, Dr. David Alan Gilbert
[-- Attachment #1: Type: text/plain, Size: 2169 bytes --]
On Fri, Mar 19, 2021 at 03:47:03PM +0200, Mahmoud Mandour wrote:
> On Fri, Mar 19, 2021 at 3:25 PM Mahmoud Mandour <ma.mandourr@gmail.com>
> wrote:
>
> > Replaced allocations done using malloc(), calloc(), and realloc()
> > to their equivalent functions in GLib.
> >
> > Memory that is allocated locally and freed when the function exits
> > are annotated g_autofree so that the deallocation is automatically
> > handled. Subsequently, I could remove a bunch of free() calls.
> >
> > Also, tried to keep the semantics of the code as is, but when the
> > allocation is a small one, or a crucial one, I replaced the
> > NULL-checking mechanisms with glib's functions that crash on error.
> >
> > This is related to a patch that I had submitted as a part of a
> > previous series. The previous patch had some errors. Also, I thought
> > that it's better to split the patch into smaller pieces.
> >
> > The previous patch can be found here:
> > https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg05153.html
> >
> > Mahmoud Mandour (8):
> > virtiofsd: Changed allocations of fuse_req to GLib functions
> > virtiofds: Changed allocations of iovec to GLib's functions
> > virtiofsd: Changed fuse_pollhandle allocation to GLib's functions
> > virtiofsd: Changed allocations of fuse_session to GLib's functions
> > virtiofsd: Changed allocation of lo_map_elems to GLib's functions
> > virtiofsd: Changed allocations of fv_VuDev & its internals to GLib
> > functions
> > virtiofsd/passthrough_ll.c: Changed local allocations to GLib
> > functions
> > virtiofsd/fuse_virtio.c: Changed allocations of locals to GLib
> >
> > tools/virtiofsd/fuse_lowlevel.c | 43 +++++++++++---------------------
> > tools/virtiofsd/fuse_virtio.c | 34 ++++++++-----------------
> > tools/virtiofsd/passthrough_ll.c | 21 ++++++----------
> > 3 files changed, 34 insertions(+), 64 deletions(-)
> >
> > --
> > 2.25.1
> >
> >
> Hello,
> For some reason, my get_maintainers script auto cc-filling did not work, so
> I had to manually cc
> you.
> Sorry for the inconvenience.
Thanks, will review tomorrow.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/8] virtiofsd: Changed allocations of fuse_req to GLib functions
2021-03-19 13:25 ` [PATCH 1/8] virtiofsd: Changed allocations of fuse_req " Mahmoud Mandour
@ 2021-03-23 13:48 ` Stefan Hajnoczi
0 siblings, 0 replies; 24+ messages in thread
From: Stefan Hajnoczi @ 2021-03-23 13:48 UTC (permalink / raw)
To: Mahmoud Mandour; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 430 bytes --]
On Fri, Mar 19, 2021 at 03:25:20PM +0200, Mahmoud Mandour wrote:
> Replaced the allocation and deallocation of fuse_req structs
> using calloc()/free() call pairs to a GLib's g_try_new0()
> and g_free().
>
> Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
> ---
> tools/virtiofsd/fuse_lowlevel.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/8] virtiofds: Changed allocations of iovec to GLib's functions
2021-03-19 13:25 ` [PATCH 2/8] virtiofds: Changed allocations of iovec to GLib's functions Mahmoud Mandour
@ 2021-03-23 13:57 ` Stefan Hajnoczi
2021-03-24 12:57 ` Stefan Hajnoczi
0 siblings, 1 reply; 24+ messages in thread
From: Stefan Hajnoczi @ 2021-03-23 13:57 UTC (permalink / raw)
To: Mahmoud Mandour; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 405 bytes --]
On Fri, Mar 19, 2021 at 03:25:21PM +0200, Mahmoud Mandour wrote:
> @@ -629,9 +628,6 @@ int fuse_reply_ioctl_retry(fuse_req_t req, const struct iovec *in_iov,
>
> res = send_reply_iov(req, 0, iov, count);
> out:
> - free(in_fiov);
> - free(out_fiov);
> -
> return res;
>
> enomem:
This hunk doesn't seem related to anything in this patch. Was it
included accidentally?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/8] virtiofsd: Changed fuse_pollhandle allocation to GLib's functions
2021-03-19 13:25 ` [PATCH 3/8] virtiofsd: Changed fuse_pollhandle allocation " Mahmoud Mandour
@ 2021-03-23 14:03 ` Stefan Hajnoczi
0 siblings, 0 replies; 24+ messages in thread
From: Stefan Hajnoczi @ 2021-03-23 14:03 UTC (permalink / raw)
To: Mahmoud Mandour; +Cc: qemu-devel, Dr. David Alan Gilbert
[-- Attachment #1: Type: text/plain, Size: 1843 bytes --]
On Fri, Mar 19, 2021 at 03:25:22PM +0200, Mahmoud Mandour wrote:
> Changed allocation of fuse_pollhandle structs to GLib's g_new().
>
> Removed the null checking as allocating such a small memory segment
> should always succeed on a healthy system. Otherwise, the system
> is already in a critical state.
>
> Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
> ---
> tools/virtiofsd/fuse_lowlevel.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
> index 66607100f2..45527ff703 100644
> --- a/tools/virtiofsd/fuse_lowlevel.c
> +++ b/tools/virtiofsd/fuse_lowlevel.c
> @@ -1755,7 +1755,7 @@ static void do_ioctl(fuse_req_t req, fuse_ino_t nodeid,
>
> void fuse_pollhandle_destroy(struct fuse_pollhandle *ph)
> {
> - free(ph);
> + g_free(ph);
> }
>
> static void do_poll(fuse_req_t req, fuse_ino_t nodeid,
> @@ -1778,11 +1778,7 @@ static void do_poll(fuse_req_t req, fuse_ino_t nodeid,
> struct fuse_pollhandle *ph = NULL;
>
> if (arg->flags & FUSE_POLL_SCHEDULE_NOTIFY) {
> - ph = malloc(sizeof(struct fuse_pollhandle));
> - if (ph == NULL) {
> - fuse_reply_err(req, ENOMEM);
> - return;
> - }
> + ph = g_new(struct fuse_pollhandle, 1);
If the out-of-memory handling code is already there then I don't think
it should be removed. How have you determined that all hope is lost for
virtiofsd if this malloc fails?
By the way, this is dead code since passthrough_ll.c does not implement
the ->poll() callback. It's there because the code comes from upstream
libfuse and the idea was to leave the code relatively unmodified to make
applying future updates from libfuse easier.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/8] virtiofsd: Changed allocations of fuse_session to GLib's functions
2021-03-19 13:25 ` [PATCH 4/8] virtiofsd: Changed allocations of fuse_session " Mahmoud Mandour
@ 2021-03-23 14:08 ` Stefan Hajnoczi
0 siblings, 0 replies; 24+ messages in thread
From: Stefan Hajnoczi @ 2021-03-23 14:08 UTC (permalink / raw)
To: Mahmoud Mandour; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1581 bytes --]
On Fri, Mar 19, 2021 at 03:25:23PM +0200, Mahmoud Mandour wrote:
> Replaced the allocation and deallocation of fuse_session structs
> from calloc() and free() calls to g_new0() and g_free().
>
> Removed the NULL-check and used g_new0() mainly because fuse_session
> creation is critical and an exit will occur anyway if fuse_session
> allocation failed.
>
> Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
> ---
> tools/virtiofsd/fuse_lowlevel.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
> index 45527ff703..b0e9ef29a7 100644
> --- a/tools/virtiofsd/fuse_lowlevel.c
> +++ b/tools/virtiofsd/fuse_lowlevel.c
> @@ -2467,7 +2467,7 @@ void fuse_session_destroy(struct fuse_session *se)
> free(se->vu_socket_path);
> se->vu_socket_path = NULL;
>
> - free(se);
> + g_free(se);
> }
>
>
> @@ -2490,11 +2490,7 @@ struct fuse_session *fuse_session_new(struct fuse_args *args,
> return NULL;
> }
>
> - se = (struct fuse_session *)calloc(1, sizeof(struct fuse_session));
> - if (se == NULL) {
> - fuse_log(FUSE_LOG_ERR, "fuse: failed to allocate fuse object\n");
> - goto out1;
> - }
> + se = g_new0(struct fuse_session, 1);
Exiting with a virtiofsd log message is preferrable to aborting inside
g_new0(). abort(3) is good when you need a coredump to investigate the
problem. Here that doesn't seem appropriate. Please don't remove this
error handling code.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 5/8] virtiofsd: Changed allocation of lo_map_elems to GLib's functions
2021-03-19 13:25 ` [PATCH 5/8] virtiofsd: Changed allocation of lo_map_elems " Mahmoud Mandour
@ 2021-03-23 14:09 ` Stefan Hajnoczi
0 siblings, 0 replies; 24+ messages in thread
From: Stefan Hajnoczi @ 2021-03-23 14:09 UTC (permalink / raw)
To: Mahmoud Mandour; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 443 bytes --]
On Fri, Mar 19, 2021 at 03:25:24PM +0200, Mahmoud Mandour wrote:
> Replaced (re)allocation of lo_map_elem structs from realloc() to
> GLib's g_try_realloc_n() and replaced the respective free() call
> with a g_free().
>
> Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
> ---
> tools/virtiofsd/passthrough_ll.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 6/8] virtiofsd: Changed allocations of fv_VuDev & its internals to GLib functions
2021-03-19 13:25 ` [PATCH 6/8] virtiofsd: Changed allocations of fv_VuDev & its internals to GLib functions Mahmoud Mandour
@ 2021-03-23 14:10 ` Stefan Hajnoczi
0 siblings, 0 replies; 24+ messages in thread
From: Stefan Hajnoczi @ 2021-03-23 14:10 UTC (permalink / raw)
To: Mahmoud Mandour; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 965 bytes --]
On Fri, Mar 19, 2021 at 03:25:25PM +0200, Mahmoud Mandour wrote:
> Changed the allocations of fv_VuDev structs, VuDev structs, and
> fv_QueueInfo strcuts from using calloc()/realloc() & free() to using
> the equivalent functions from GLib.
>
> In instances, removed the pair of allocation and assertion for
> non-NULL checking with a GLib function that aborts on error.
>
> Removed NULL-checking for fv_VuDev struct allocation and used
> a GLib function that crashes on error; namely, g_new0(). This
> is because allocating one struct should not be a problem on an
> healthy system. Also following the pattern of aborting-on-null
> behaviour that is taken with allocating VuDev structs and
> fv_QueueInfo structs.
>
> Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
> ---
> tools/virtiofsd/fuse_virtio.c | 19 ++++++-------------
> 1 file changed, 6 insertions(+), 13 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 7/8] virtiofsd/passthrough_ll.c: Changed local allocations to GLib functions
2021-03-19 13:25 ` [PATCH 7/8] virtiofsd/passthrough_ll.c: Changed local allocations " Mahmoud Mandour
@ 2021-03-23 14:13 ` Stefan Hajnoczi
0 siblings, 0 replies; 24+ messages in thread
From: Stefan Hajnoczi @ 2021-03-23 14:13 UTC (permalink / raw)
To: Mahmoud Mandour; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 517 bytes --]
On Fri, Mar 19, 2021 at 03:25:26PM +0200, Mahmoud Mandour wrote:
> Changed the allocations of some local variables to GLib's allocation
> functions, such as g_try_malloc0(), and annotated those variables
> as g_autofree. Subsequently, I was able to remove the calls to free().
>
> Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
> ---
> tools/virtiofsd/passthrough_ll.c | 17 ++++++-----------
> 1 file changed, 6 insertions(+), 11 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 8/8] virtiofsd/fuse_virtio.c: Changed allocations of locals to GLib
2021-03-19 13:25 ` [PATCH 8/8] virtiofsd/fuse_virtio.c: Changed allocations of locals to GLib Mahmoud Mandour
@ 2021-03-23 14:15 ` Stefan Hajnoczi
[not found] ` <CAD-LL6iYvHZt8mJZdiHLyUYsDq3kBy0HrTR6_K0x6iCLE1POoA@mail.gmail.com>
0 siblings, 1 reply; 24+ messages in thread
From: Stefan Hajnoczi @ 2021-03-23 14:15 UTC (permalink / raw)
To: Mahmoud Mandour; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 432 bytes --]
On Fri, Mar 19, 2021 at 03:25:27PM +0200, Mahmoud Mandour wrote:
> @@ -588,7 +587,7 @@ out:
> }
>
> pthread_mutex_destroy(&req->ch.lock);
> - free(fbuf.mem);
> + g_free(fbuf.mem);
> free(req);
^--- was FVRequest allocation changed in a previous patch?
Maybe an earlier patch forgot to use g_free() here.
Aside from this:
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/8] virtiofds: Changed allocations of iovec to GLib's functions
2021-03-23 13:57 ` Stefan Hajnoczi
@ 2021-03-24 12:57 ` Stefan Hajnoczi
2021-03-24 13:32 ` Mahmoud Mandour
0 siblings, 1 reply; 24+ messages in thread
From: Stefan Hajnoczi @ 2021-03-24 12:57 UTC (permalink / raw)
To: Mahmoud Mandour; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 821 bytes --]
On Tue, Mar 23, 2021 at 01:57:05PM +0000, Stefan Hajnoczi wrote:
> On Fri, Mar 19, 2021 at 03:25:21PM +0200, Mahmoud Mandour wrote:
> > @@ -629,9 +628,6 @@ int fuse_reply_ioctl_retry(fuse_req_t req, const struct iovec *in_iov,
> >
> > res = send_reply_iov(req, 0, iov, count);
> > out:
> > - free(in_fiov);
> > - free(out_fiov);
> > -
> > return res;
> >
> > enomem:
>
> This hunk doesn't seem related to anything in this patch. Was it
> included accidentally?
Thanks for explaining that in_fiov/out_fiov are allocated by
fuse_ioctl_iovec_copy() earlier in this function. I missed that.
Please use Reply-All on mailing list emails so that the mailing like and
all other CC email addresses are included in the discussion.
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 8/8] virtiofsd/fuse_virtio.c: Changed allocations of locals to GLib
[not found] ` <CAD-LL6iYvHZt8mJZdiHLyUYsDq3kBy0HrTR6_K0x6iCLE1POoA@mail.gmail.com>
@ 2021-03-24 13:00 ` Stefan Hajnoczi
0 siblings, 0 replies; 24+ messages in thread
From: Stefan Hajnoczi @ 2021-03-24 13:00 UTC (permalink / raw)
To: Mahmoud Mandour, qemu-devel
On Wed, Mar 24, 2021 at 7:12 AM Mahmoud Mandour <ma.mandourr@gmail.com> wrote:
>
> On Tue, Mar 23, 2021 at 4:15 PM Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>
>> On Fri, Mar 19, 2021 at 03:25:27PM +0200, Mahmoud Mandour wrote:
>> > @@ -588,7 +587,7 @@ out:
>> > }
>> >
>> > pthread_mutex_destroy(&req->ch.lock);
>> > - free(fbuf.mem);
>> > + g_free(fbuf.mem);
>> > free(req);
>>
>> ^--- was FVRequest allocation changed in a previous patch?
>> Maybe an earlier patch forgot to use g_free() here.
>>
>> Aside from this:
>>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>
>
> I did not change the allocation of FVRequest. I believe that's why
> this is left unchanged.
Okay, I see it's allocated by libvhost-user and not directly by virtiofsd code.
Thanks,
Stefan
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/8] virtiofds: Changed allocations of iovec to GLib's functions
2021-03-24 12:57 ` Stefan Hajnoczi
@ 2021-03-24 13:32 ` Mahmoud Mandour
0 siblings, 0 replies; 24+ messages in thread
From: Mahmoud Mandour @ 2021-03-24 13:32 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 292 bytes --]
On Wed, Mar 24, 2021 at 2:57 PM Stefan Hajnoczi <stefanha@gmail.com> wrote:
> Please use Reply-All on mailing list emails so that the mailing like and
> all other CC email addresses are included in the discussion.
>
That's my bad, hopefully this won't happen again in the future.
Mahmoud
[-- Attachment #2: Type: text/html, Size: 701 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/8] virtiofsd: Changed various allocations to GLib functions
2021-03-22 15:52 ` Stefan Hajnoczi
@ 2021-04-16 8:43 ` Mahmoud Mandour
2021-04-19 14:19 ` Stefan Hajnoczi
0 siblings, 1 reply; 24+ messages in thread
From: Mahmoud Mandour @ 2021-04-16 8:43 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-devel, Stefan Hajnoczi, Dr. David Alan Gilbert
[-- Attachment #1: Type: text/plain, Size: 2565 bytes --]
On Mon, Mar 22, 2021 at 5:53 PM Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Fri, Mar 19, 2021 at 03:47:03PM +0200, Mahmoud Mandour wrote:
> > On Fri, Mar 19, 2021 at 3:25 PM Mahmoud Mandour <ma.mandourr@gmail.com>
> > wrote:
> >
> > > Replaced allocations done using malloc(), calloc(), and realloc()
> > > to their equivalent functions in GLib.
> > >
> > > Memory that is allocated locally and freed when the function exits
> > > are annotated g_autofree so that the deallocation is automatically
> > > handled. Subsequently, I could remove a bunch of free() calls.
> > >
> > > Also, tried to keep the semantics of the code as is, but when the
> > > allocation is a small one, or a crucial one, I replaced the
> > > NULL-checking mechanisms with glib's functions that crash on error.
> > >
> > > This is related to a patch that I had submitted as a part of a
> > > previous series. The previous patch had some errors. Also, I thought
> > > that it's better to split the patch into smaller pieces.
> > >
> > > The previous patch can be found here:
> > > https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg05153.html
> > >
> > > Mahmoud Mandour (8):
> > > virtiofsd: Changed allocations of fuse_req to GLib functions
> > > virtiofds: Changed allocations of iovec to GLib's functions
> > > virtiofsd: Changed fuse_pollhandle allocation to GLib's functions
> > > virtiofsd: Changed allocations of fuse_session to GLib's functions
> > > virtiofsd: Changed allocation of lo_map_elems to GLib's functions
> > > virtiofsd: Changed allocations of fv_VuDev & its internals to GLib
> > > functions
> > > virtiofsd/passthrough_ll.c: Changed local allocations to GLib
> > > functions
> > > virtiofsd/fuse_virtio.c: Changed allocations of locals to GLib
> > >
> > > tools/virtiofsd/fuse_lowlevel.c | 43 +++++++++++---------------------
> > > tools/virtiofsd/fuse_virtio.c | 34 ++++++++-----------------
> > > tools/virtiofsd/passthrough_ll.c | 21 ++++++----------
> > > 3 files changed, 34 insertions(+), 64 deletions(-)
> > >
> > > --
> > > 2.25.1
> > >
> > >
> > Hello,
> > For some reason, my get_maintainers script auto cc-filling did not work,
> so
> > I had to manually cc
> > you.
> > Sorry for the inconvenience.
>
> Thanks, will review tomorrow.
>
> Stefan
>
Hello
I wanted to ask whether I need to resend the patch series with updates
utilizing
the feedback I got? There are patches that are overall superfluous, and
others are
"reviewed". Should I resend an updated series with only the patches
reviewed?
Yours,
Mahmoud
[-- Attachment #2: Type: text/html, Size: 3634 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/8] virtiofsd: Changed various allocations to GLib functions
2021-04-16 8:43 ` Mahmoud Mandour
@ 2021-04-19 14:19 ` Stefan Hajnoczi
0 siblings, 0 replies; 24+ messages in thread
From: Stefan Hajnoczi @ 2021-04-19 14:19 UTC (permalink / raw)
To: Mahmoud Mandour; +Cc: Stefan Hajnoczi, qemu-devel, Dr. David Alan Gilbert
[-- Attachment #1: Type: text/plain, Size: 2852 bytes --]
On Fri, Apr 16, 2021 at 10:43:07AM +0200, Mahmoud Mandour wrote:
> On Mon, Mar 22, 2021 at 5:53 PM Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
> > On Fri, Mar 19, 2021 at 03:47:03PM +0200, Mahmoud Mandour wrote:
> > > On Fri, Mar 19, 2021 at 3:25 PM Mahmoud Mandour <ma.mandourr@gmail.com>
> > > wrote:
> > >
> > > > Replaced allocations done using malloc(), calloc(), and realloc()
> > > > to their equivalent functions in GLib.
> > > >
> > > > Memory that is allocated locally and freed when the function exits
> > > > are annotated g_autofree so that the deallocation is automatically
> > > > handled. Subsequently, I could remove a bunch of free() calls.
> > > >
> > > > Also, tried to keep the semantics of the code as is, but when the
> > > > allocation is a small one, or a crucial one, I replaced the
> > > > NULL-checking mechanisms with glib's functions that crash on error.
> > > >
> > > > This is related to a patch that I had submitted as a part of a
> > > > previous series. The previous patch had some errors. Also, I thought
> > > > that it's better to split the patch into smaller pieces.
> > > >
> > > > The previous patch can be found here:
> > > > https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg05153.html
> > > >
> > > > Mahmoud Mandour (8):
> > > > virtiofsd: Changed allocations of fuse_req to GLib functions
> > > > virtiofds: Changed allocations of iovec to GLib's functions
> > > > virtiofsd: Changed fuse_pollhandle allocation to GLib's functions
> > > > virtiofsd: Changed allocations of fuse_session to GLib's functions
> > > > virtiofsd: Changed allocation of lo_map_elems to GLib's functions
> > > > virtiofsd: Changed allocations of fv_VuDev & its internals to GLib
> > > > functions
> > > > virtiofsd/passthrough_ll.c: Changed local allocations to GLib
> > > > functions
> > > > virtiofsd/fuse_virtio.c: Changed allocations of locals to GLib
> > > >
> > > > tools/virtiofsd/fuse_lowlevel.c | 43 +++++++++++---------------------
> > > > tools/virtiofsd/fuse_virtio.c | 34 ++++++++-----------------
> > > > tools/virtiofsd/passthrough_ll.c | 21 ++++++----------
> > > > 3 files changed, 34 insertions(+), 64 deletions(-)
> > > >
> > > > --
> > > > 2.25.1
> > > >
> > > >
> > > Hello,
> > > For some reason, my get_maintainers script auto cc-filling did not work,
> > so
> > > I had to manually cc
> > > you.
> > > Sorry for the inconvenience.
> >
> > Thanks, will review tomorrow.
> >
> > Stefan
> >
>
> Hello
>
> I wanted to ask whether I need to resend the patch series with updates
> utilizing
> the feedback I got? There are patches that are overall superfluous, and
> others are
> "reviewed". Should I resend an updated series with only the patches
> reviewed?
That would be great. Thanks!
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2021-04-19 14:20 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-19 13:25 [PATCH 0/8] virtiofsd: Changed various allocations to GLib functions Mahmoud Mandour
2021-03-19 13:25 ` [PATCH 1/8] virtiofsd: Changed allocations of fuse_req " Mahmoud Mandour
2021-03-23 13:48 ` Stefan Hajnoczi
2021-03-19 13:25 ` [PATCH 2/8] virtiofds: Changed allocations of iovec to GLib's functions Mahmoud Mandour
2021-03-23 13:57 ` Stefan Hajnoczi
2021-03-24 12:57 ` Stefan Hajnoczi
2021-03-24 13:32 ` Mahmoud Mandour
2021-03-19 13:25 ` [PATCH 3/8] virtiofsd: Changed fuse_pollhandle allocation " Mahmoud Mandour
2021-03-23 14:03 ` Stefan Hajnoczi
2021-03-19 13:25 ` [PATCH 4/8] virtiofsd: Changed allocations of fuse_session " Mahmoud Mandour
2021-03-23 14:08 ` Stefan Hajnoczi
2021-03-19 13:25 ` [PATCH 5/8] virtiofsd: Changed allocation of lo_map_elems " Mahmoud Mandour
2021-03-23 14:09 ` Stefan Hajnoczi
2021-03-19 13:25 ` [PATCH 6/8] virtiofsd: Changed allocations of fv_VuDev & its internals to GLib functions Mahmoud Mandour
2021-03-23 14:10 ` Stefan Hajnoczi
2021-03-19 13:25 ` [PATCH 7/8] virtiofsd/passthrough_ll.c: Changed local allocations " Mahmoud Mandour
2021-03-23 14:13 ` Stefan Hajnoczi
2021-03-19 13:25 ` [PATCH 8/8] virtiofsd/fuse_virtio.c: Changed allocations of locals to GLib Mahmoud Mandour
2021-03-23 14:15 ` Stefan Hajnoczi
[not found] ` <CAD-LL6iYvHZt8mJZdiHLyUYsDq3kBy0HrTR6_K0x6iCLE1POoA@mail.gmail.com>
2021-03-24 13:00 ` Stefan Hajnoczi
2021-03-19 13:47 ` [PATCH 0/8] virtiofsd: Changed various allocations to GLib functions Mahmoud Mandour
2021-03-22 15:52 ` Stefan Hajnoczi
2021-04-16 8:43 ` Mahmoud Mandour
2021-04-19 14:19 ` Stefan Hajnoczi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).