hw/9pfs: Drop unused print_sg helper

[PATCH] hw/9pfs: Drop unused print_sg helper

Posted by Osama Abdelkader 1 month ago

Remove the print_sg() debug helper and its always-disabled call sites
in v9fs_read() and v9fs_write(). The function was only reachable via
if (0) blocks, so it has been dead code for a long time.

Signed-off-by: Osama Abdelkader <osama.abdelkader@gmail.com>
---
 hw/9pfs/9p.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index bc4a016ee3..a8de894f4c 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1389,20 +1389,6 @@ static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
     return stat_to_qid(pdu, stbuf, &v9lstat->qid);
 }
 
-static void print_sg(struct iovec *sg, int cnt)
-{
-    int i;
-
-    printf("sg[%d]: {", cnt);
-    for (i = 0; i < cnt; i++) {
-        if (i) {
-            printf(", ");
-        }
-        printf("(%p, %zd)", sg[i].iov_base, sg[i].iov_len);
-    }
-    printf("}\n");
-}
-
 /* Will call this only for path name based fid */
 static void v9fs_fix_path(V9fsPath *dst, V9fsPath *src, int len)
 {
@@ -2468,9 +2454,6 @@ static void coroutine_fn v9fs_read(void *opaque)
         do {
             qemu_iovec_reset(&qiov);
             qemu_iovec_concat(&qiov, &qiov_full, count, qiov_full.size - count);
-            if (0) {
-                print_sg(qiov.iov, qiov.niov);
-            }
             /* Loop in case of EINTR */
             do {
                 len = v9fs_co_preadv(pdu, fidp, qiov.iov, qiov.niov, off);
@@ -2785,9 +2768,6 @@ static void coroutine_fn v9fs_write(void *opaque)
     do {
         qemu_iovec_reset(&qiov);
         qemu_iovec_concat(&qiov, &qiov_full, total, qiov_full.size - total);
-        if (0) {
-            print_sg(qiov.iov, qiov.niov);
-        }
         /* Loop in case of EINTR */
         do {
             len = v9fs_co_pwritev(pdu, fidp, qiov.iov, qiov.niov, off);
-- 
2.43.0

Re: [PATCH] hw/9pfs: Drop unused print_sg helper

Posted by Philippe Mathieu-Daudé 1 month ago

Hi Osama,

On 10/11/25 16:54, Osama Abdelkader wrote:
> Remove the print_sg() debug helper and its always-disabled call sites
> in v9fs_read() and v9fs_write(). The function was only reachable via
> if (0) blocks, so it has been dead code for a long time.
> 
> Signed-off-by: Osama Abdelkader <osama.abdelkader@gmail.com>
> ---
>   hw/9pfs/9p.c | 20 --------------------
>   1 file changed, 20 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index bc4a016ee3..a8de894f4c 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1389,20 +1389,6 @@ static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
>       return stat_to_qid(pdu, stbuf, &v9lstat->qid);
>   }
>   
> -static void print_sg(struct iovec *sg, int cnt)
> -{
> -    int i;
> -
> -    printf("sg[%d]: {", cnt);
> -    for (i = 0; i < cnt; i++) {
> -        if (i) {
> -            printf(", ");
> -        }
> -        printf("(%p, %zd)", sg[i].iov_base, sg[i].iov_len);
> -    }
> -    printf("}\n");
> -}
> -
>   /* Will call this only for path name based fid */
>   static void v9fs_fix_path(V9fsPath *dst, V9fsPath *src, int len)
>   {
> @@ -2468,9 +2454,6 @@ static void coroutine_fn v9fs_read(void *opaque)
>           do {
>               qemu_iovec_reset(&qiov);
>               qemu_iovec_concat(&qiov, &qiov_full, count, qiov_full.size - count);
> -            if (0) {
> -                print_sg(qiov.iov, qiov.niov);

Alternatively, consider converting to trace event so we can keep
dumping the entries, but select that at runtime (see for conversion
example commit 4847c5701a3 "hw/rtc/mc146818rtc: Convert CMOS_DPRINTF
into trace events").
> -            }

Regards,

Phil.

Re: [PATCH] hw/9pfs: Drop unused print_sg helper

Posted by Christian Schoenebeck 1 month ago

On Wednesday, November 12, 2025 8:03:28 AM CET Philippe Mathieu-Daudé wrote:
> Hi Osama,
> 
> On 10/11/25 16:54, Osama Abdelkader wrote:
> > Remove the print_sg() debug helper and its always-disabled call sites
> > in v9fs_read() and v9fs_write(). The function was only reachable via
> > if (0) blocks, so it has been dead code for a long time.
> > 
> > Signed-off-by: Osama Abdelkader <osama.abdelkader@gmail.com>
> > ---

TBH low care level for this on my side. It's just passive debug code and the 
suggested change does not change anything on the resulting binary.

It's not that this debug code is not used at all, but very seldom. Last time I 
personally used it was like 3 years ago.

> > 
> >   hw/9pfs/9p.c | 20 --------------------
> >   1 file changed, 20 deletions(-)
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index bc4a016ee3..a8de894f4c 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -1389,20 +1389,6 @@ static int stat_to_v9stat_dotl(V9fsPDU *pdu, const
> > struct stat *stbuf,> 
> >       return stat_to_qid(pdu, stbuf, &v9lstat->qid);
> >   
> >   }
> > 
> > -static void print_sg(struct iovec *sg, int cnt)
> > -{
> > -    int i;
> > -
> > -    printf("sg[%d]: {", cnt);
> > -    for (i = 0; i < cnt; i++) {
> > -        if (i) {
> > -            printf(", ");
> > -        }
> > -        printf("(%p, %zd)", sg[i].iov_base, sg[i].iov_len);
> > -    }
> > -    printf("}\n");
> > -}
> > -
> > 
> >   /* Will call this only for path name based fid */
> >   static void v9fs_fix_path(V9fsPath *dst, V9fsPath *src, int len)
> >   {
> > 
> > @@ -2468,9 +2454,6 @@ static void coroutine_fn v9fs_read(void *opaque)
> > 
> >           do {
> >           
> >               qemu_iovec_reset(&qiov);
> >               qemu_iovec_concat(&qiov, &qiov_full, count, qiov_full.size -
> >               count);
> > 
> > -            if (0) {
> > -                print_sg(qiov.iov, qiov.niov);
> 
> Alternatively, consider converting to trace event so we can keep
> dumping the entries, but select that at runtime (see for conversion
> example commit 4847c5701a3 "hw/rtc/mc146818rtc: Convert CMOS_DPRINTF
> into trace events").

Probably overkill. We have a bunch of trace events where it makes, especially 
for investigating issues on 9p protocol level. But this debug code is usually 
just enabled if you are working on a virtio transport issue or new virtio 
feature and then you are usually working on this source code already.

But again: no strong opinion about this overall issue whatsoever.

/Christian

Re: [PATCH] hw/9pfs: Drop unused print_sg helper

Posted by Philippe Mathieu-Daudé 1 month ago

On 12/11/25 13:30, Christian Schoenebeck wrote:
> On Wednesday, November 12, 2025 8:03:28 AM CET Philippe Mathieu-Daudé wrote:
>> Hi Osama,
>>
>> On 10/11/25 16:54, Osama Abdelkader wrote:
>>> Remove the print_sg() debug helper and its always-disabled call sites
>>> in v9fs_read() and v9fs_write(). The function was only reachable via
>>> if (0) blocks, so it has been dead code for a long time.
>>>
>>> Signed-off-by: Osama Abdelkader <osama.abdelkader@gmail.com>
>>> ---
> 
> TBH low care level for this on my side. It's just passive debug code and the
> suggested change does not change anything on the resulting binary.
> 
> It's not that this debug code is not used at all, but very seldom. Last time I
> personally used it was like 3 years ago.
> 
>>>
>>>    hw/9pfs/9p.c | 20 --------------------
>>>    1 file changed, 20 deletions(-)
>>>
>>> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
>>> index bc4a016ee3..a8de894f4c 100644
>>> --- a/hw/9pfs/9p.c
>>> +++ b/hw/9pfs/9p.c
>>> @@ -1389,20 +1389,6 @@ static int stat_to_v9stat_dotl(V9fsPDU *pdu, const
>>> struct stat *stbuf,>
>>>        return stat_to_qid(pdu, stbuf, &v9lstat->qid);
>>>    
>>>    }
>>>
>>> -static void print_sg(struct iovec *sg, int cnt)
>>> -{
>>> -    int i;
>>> -
>>> -    printf("sg[%d]: {", cnt);
>>> -    for (i = 0; i < cnt; i++) {
>>> -        if (i) {
>>> -            printf(", ");
>>> -        }
>>> -        printf("(%p, %zd)", sg[i].iov_base, sg[i].iov_len);
>>> -    }
>>> -    printf("}\n");
>>> -}
>>> -
>>>
>>>    /* Will call this only for path name based fid */
>>>    static void v9fs_fix_path(V9fsPath *dst, V9fsPath *src, int len)
>>>    {
>>>
>>> @@ -2468,9 +2454,6 @@ static void coroutine_fn v9fs_read(void *opaque)
>>>
>>>            do {
>>>            
>>>                qemu_iovec_reset(&qiov);
>>>                qemu_iovec_concat(&qiov, &qiov_full, count, qiov_full.size -
>>>                count);
>>>
>>> -            if (0) {
>>> -                print_sg(qiov.iov, qiov.niov);
>>
>> Alternatively, consider converting to trace event so we can keep
>> dumping the entries, but select that at runtime (see for conversion
>> example commit 4847c5701a3 "hw/rtc/mc146818rtc: Convert CMOS_DPRINTF
>> into trace events").
> 
> Probably overkill. We have a bunch of trace events where it makes, especially
> for investigating issues on 9p protocol level. But this debug code is usually
> just enabled if you are working on a virtio transport issue or new virtio
> feature and then you are usually working on this source code already.
> 
> But again: no strong opinion about this overall issue whatsoever.

Fine then!