Re: [Qemu-devel] [PATCH v4 3/6] hypertrace: [*-user] Add QEMU-side proxy to "guest_hypertrace" event

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3135 bytes --]

On Mon, Jan 16, 2017 at 06:05:26PM +0100, Lluís Vilanova wrote:
> Stefan Hajnoczi writes:
> > On Mon, Dec 26, 2016 at 09:34:54PM +0100, Lluís Vilanova wrote:
> >> +void hypertrace_init_config(struct hypertrace_config *config,
> >> +                            unsigned int max_clients)
> >> +{
> >> +    config->max_clients = max_clients;
> >> +    config->client_args = CONFIG_HYPERTRACE_ARGS;
> >> +    config->client_data_size = config->client_args * sizeof(uint64_t);
> >> +    config->control_size = QEMU_ALIGN_UP(
> >> +        config->max_clients * sizeof(uint64_t), TARGET_PAGE_SIZE);
> 
> > This needs to be host page size aligned, too.  Otherwise protect will
> > affect bytes beyond the end of the control region.
> 
> Ummm, so right. Although I think only host page alignment is required (there's
> no soft TLB in user-mode, right?).

Yes.

> >> +static void init_channel(const char *base, const char *suffix, size_t size,
> >> +                         char **path, int *fd, uint64_t **addr)
> >> +{
> >> +    *path = g_malloc(strlen(base) + strlen(suffix) + 1);
> >> +    sprintf(*path, "%s%s", base, suffix);
> >> +
> >> +    *fd = open(*path, O_CREAT | O_EXCL | O_RDWR, S_IRUSR | S_IWUSR);
> >> +    if (*fd == -1) {
> >> +        error_report("error: open(%s): %s", *path, strerror(errno));
> >> +        abort();
> >> +    }
> 
> > open() can fail for reasons outside QEMU's control.  This isn't an
> > internal error.  Please exit cleanly instead of using abort(3).
> 
> By cleanly you mean exit with a non-zero code, right? It still is an error that
> cannot be recovered.

Right, it's an error.

> Also, if this goes with exit() what about the abort()s I have added in other
> places? (e.g., on a failed call to sigaction)

abort(3) is useful for internal errors where a core dump and debugging
are required.

exit(3) is useful for graceful exit (both successful and unsuccessful).
Over the past few years the codebase has been moving towards using Error
**errp and letting the top-level functions handle errors instead of
exiting deep inside QEMU.  This is necessary because lots of things can
be initialized at runtime (like device hotplug) and shouldn't bring down
QEMU.  But it's okay to exit in initialization code that will only be
called once.

> >> +
> >> +    } else {
> >> +        /* proxy to next handler */
> >> +        if (segv_next.sa_sigaction != NULL) {
> >> +            segv_next.sa_sigaction(signum, siginfo, sigctxt);
> >> +        } else if (segv_next.sa_handler != NULL) {
> >> +            segv_next.sa_handler(signum);
> >> +        }
> 
> > Is there a case when no signal handler was installed (i.e. default
> > action)?
> 
> Yes, before calling hypertrace_init() or if it is called without a
> "hypertrace_base" argument set (i.e., the user has not enabled hypertrace in the
> command line).

I meant "what happens if !segv_next.sa_action &&
!segv_next.sa_handler?".  The default signal disposition should take
effect.  This code is ignoring that case, turning everything into
SIG_IGN but there is also SIG_DFL.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]