Re: [PATCH v3 04/17] hw/usb/hcd-ehci.c: Replace fprintf(stderr, ...) with qemu_log_mask(LOG_GUEST_ERROR)

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 16/4/26 03:49, Jamin Lin wrote:
> Replace direct fprintf(stderr, ...) calls with qemu_log_mask()
> using LOG_GUEST_ERROR in the EHCI controller implementation.
> 
> Direct writes to stderr are discouraged in QEMU as they bypass
> the logging framework and cannot be filtered or controlled at
> runtime. Using qemu_log_mask() ensures that guest-visible errors
> are properly categorized and can be managed via QEMU logging
> options.
> 
> All affected messages correspond to guest-triggerable error
> conditions, so LOG_GUEST_ERROR is used consistently.
> 
> This change improves integration with QEMU's logging infrastructure
> and aligns with upstream coding guidelines.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>   hw/usb/hcd-ehci.c | 43 +++++++++++++++++++++++++------------------
>   1 file changed, 25 insertions(+), 18 deletions(-)


> @@ -532,7 +533,7 @@ static void ehci_free_packet(EHCIPacket *p)
>       }
>       if (p->async == EHCI_ASYNC_FINISHED &&
>               p->packet.status == USB_RET_SUCCESS) {
> -        fprintf(stderr,
> +        qemu_log_mask(LOG_GUEST_ERROR,
>                   "EHCI: Dropping completed packet from halted %s ep %02X\n",
>                   (p->pid == USB_TOKEN_IN) ? "in" : "out",
>                   get_field(p->queue->qh.epchar, QH_EPCHAR_EP));
> @@ -1042,8 +1043,9 @@ static void ehci_opreg_write(void *ptr, hwaddr addr,
>   
>           /* not supporting dynamic frame list size at the moment */
>           if ((val & USBCMD_FLS) && !(s->usbcmd & USBCMD_FLS)) {
> -            fprintf(stderr, "attempt to set frame list size -- value %d\n",
> -                    (int)val & USBCMD_FLS);
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "attempt to set frame list size -- value %d\n",

PRId64

> +                          (int)val & USBCMD_FLS);
>               val &= ~USBCMD_FLS;
>           }

> @@ -1584,7 +1589,8 @@ static int ehci_state_fetchentry(EHCIState *ehci, int async)
>   
>       /* section 4.8, only QH in async schedule */
>       if (async && (NLPTR_TYPE_GET(entry) != NLPTR_TYPE_QH)) {
> -        fprintf(stderr, "non queue head request in async schedule\n");
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "non queue head request in async schedule\n");
>           return -1;
>       }
>   
> @@ -1606,7 +1612,7 @@ static int ehci_state_fetchentry(EHCIState *ehci, int async)
>   
>       default:
>           /* TODO: handle FSTN type */
> -        fprintf(stderr, "FETCHENTRY: entry at %X is of type %u "
> +        qemu_log_mask(LOG_GUEST_ERROR, "FETCHENTRY: entry at %X is of type %u "

"0x%x"

>                   "which is not supported yet\n", entry, NLPTR_TYPE_GET(entry));
>           return -1;
>       }
> @@ -2112,13 +2118,14 @@ static void ehci_advance_state(EHCIState *ehci, int async)
>               break;
>   
>           default:
> -            fprintf(stderr, "Bad state!\n");
> +            qemu_log_mask(LOG_GUEST_ERROR, "Bad state!\n");

Just remove for "bad state" entries?

>               g_assert_not_reached();
>           }
>   
>           if (again < 0 || itd_count > 16) {
>               /* TODO: notify guest (raise HSE irq?) */
> -            fprintf(stderr, "processing error - resetting ehci HC\n");
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "processing error - resetting ehci HC\n");
>               ehci_reset(ehci);
>               again = 0;
>           }
> @@ -2175,7 +2182,7 @@ static void ehci_advance_async_state(EHCIState *ehci)
>   
>       default:
>           /* this should only be due to a developer mistake */
> -        fprintf(stderr, "ehci: Bad asynchronous state %d. "
> +        qemu_log_mask(LOG_GUEST_ERROR, "ehci: Bad asynchronous state %d. "
>                   "Resetting to active\n", ehci->astate);

Ditto.

>           g_assert_not_reached();
>       }
> @@ -2225,7 +2232,7 @@ static void ehci_advance_periodic_state(EHCIState *ehci)
>   
>       default:
>           /* this should only be due to a developer mistake */
> -        fprintf(stderr, "ehci: Bad periodic state %d. "
> +        qemu_log_mask(LOG_GUEST_ERROR, "ehci: Bad periodic state %d. "
>                   "Resetting to active\n", ehci->pstate);
>           g_assert_not_reached();

Ditto.

>       }