[for-4.0,v9,15/16] qemu_thread: supplement error handling for touch_all_pages

Supplement the error handling for touch_all_pages: add an Error
parameter for it to propagate the error to its caller to do the
handling in case it fails.

Cc: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Fei Li <fli@suse.com>
---
 util/oslib-posix.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

Fei Li <fli@suse.com> writes:

> Supplement the error handling for touch_all_pages: add an Error
> parameter for it to propagate the error to its caller to do the
> handling in case it fails.
>
> Cc: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Fei Li <fli@suse.com>
> ---
>  util/oslib-posix.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index 251e2f1aea..afc1d99093 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -431,15 +431,17 @@ static inline int get_memset_num_threads(int smp_cpus)
>  }
>  
>  static bool touch_all_pages(char *area, size_t hpagesize, size_t numpages,
> -                            int smp_cpus)
> +                            int smp_cpus, Error **errp)
>  {
>      size_t numpages_per_thread;
>      size_t size_per_thread;
>      char *addr = area;
>      int i = 0;
> +    int started_thread = 0;
>  
>      memset_thread_failed = false;
>      memset_num_threads = get_memset_num_threads(smp_cpus);
> +    started_thread = memset_num_threads;
>      memset_thread = g_new0(MemsetThread, memset_num_threads);
>      numpages_per_thread = (numpages / memset_num_threads);
>      size_per_thread = (hpagesize * numpages_per_thread);
> @@ -448,14 +450,18 @@ static bool touch_all_pages(char *area, size_t hpagesize, size_t numpages,
>          memset_thread[i].numpages = (i == (memset_num_threads - 1)) ?
>                                      numpages : numpages_per_thread;
>          memset_thread[i].hpagesize = hpagesize;
> -        /* TODO: let the callers handle the error instead of abort() here */
> -        qemu_thread_create(&memset_thread[i].pgthread, "touch_pages",
> -                           do_touch_pages, &memset_thread[i],
> -                           QEMU_THREAD_JOINABLE, &error_abort);
> +        if (!qemu_thread_create(&memset_thread[i].pgthread, "touch_pages",
> +                                do_touch_pages, &memset_thread[i],
> +                                QEMU_THREAD_JOINABLE, errp)) {
> +            memset_thread_failed = true;
> +            started_thread = i;
> +            goto out;

break rather than goto, please.

> +        }
>          addr += size_per_thread;
>          numpages -= numpages_per_thread;
>      }
> -    for (i = 0; i < memset_num_threads; i++) {
> +out:
> +    for (i = 0; i < started_thread; i++) {
>          qemu_thread_join(&memset_thread[i].pgthread);
>      }

I don't like how @started_thread is computed.  The name suggests it's
the number of threads started so far.  That's the case when you
initialize it to zero.  But then you immediately set it to
memset_thread().  It again becomes the case only when you break the loop
on error, or when you complete it successfully.

There's no need for @started_thread, since the number of threads created
is readily available as @i:

       memset_num_threads = i;
       for (i = 0; i < memset_num_threads; i++) {
           qemu_thread_join(&memset_thread[i].pgthread);
       }

Rest of the function:

>      g_free(memset_thread);
       memset_thread = NULL;

       return memset_thread_failed;
   }

If do_touch_pages() set memset_thread_failed(), we return false without
setting an error.  I believe you should

       if (memset_thread_failed) {
           error_setg(errp, "os_mem_prealloc: Insufficient free host memory "
               "pages available to allocate guest RAM");
           return false;
       }
       return true;

here, and ...

> @@ -471,6 +477,7 @@ void os_mem_prealloc(int fd, char *area, size_t memory, int smp_cpus,
>      struct sigaction act, oldact;
>      size_t hpagesize = qemu_fd_getpagesize(fd);
>      size_t numpages = DIV_ROUND_UP(memory, hpagesize);
> +    Error *local_err = NULL;
>  
>      memset(&act, 0, sizeof(act));
>      act.sa_handler = &sigbus_handler;
> @@ -484,9 +491,9 @@ void os_mem_prealloc(int fd, char *area, size_t memory, int smp_cpus,
>      }
>  
>      /* touch pages simultaneously */
> -    if (touch_all_pages(area, hpagesize, numpages, smp_cpus)) {
> -        error_setg(errp, "os_mem_prealloc: Insufficient free host memory "
> -            "pages available to allocate guest RAM");
> +    if (touch_all_pages(area, hpagesize, numpages, smp_cpus, &local_err)) {
> +        error_propagate_prepend(errp, local_err, "os_mem_prealloc: Insufficient"
> +            " free host memory pages available to allocate guest RAM: ");
>      }

... not mess with the error message here, i.e.

       touch_all_pages(area, hpagesize, numpages, smp_cpus), errp);

>  
>      ret = sigaction(SIGBUS, &oldact, NULL);

> 在 2019年1月8日，02:13，Markus Armbruster <armbru@redhat.com> 写道：
> 
> Fei Li <fli@suse.com> writes:
> 
>> Supplement the error handling for touch_all_pages: add an Error
>> parameter for it to propagate the error to its caller to do the
>> handling in case it fails.
>> 
>> Cc: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Fei Li <fli@suse.com>
>> ---
>> util/oslib-posix.c | 25 ++++++++++++++++---------
>> 1 file changed, 16 insertions(+), 9 deletions(-)
>> 
>> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
>> index 251e2f1aea..afc1d99093 100644
>> --- a/util/oslib-posix.c
>> +++ b/util/oslib-posix.c
>> @@ -431,15 +431,17 @@ static inline int get_memset_num_threads(int smp_cpus)
>> }
>> 
>> static bool touch_all_pages(char *area, size_t hpagesize, size_t numpages,
>> -                            int smp_cpus)
>> +                            int smp_cpus, Error **errp)
>> {
>>     size_t numpages_per_thread;
>>     size_t size_per_thread;
>>     char *addr = area;
>>     int i = 0;
>> +    int started_thread = 0;
>> 
>>     memset_thread_failed = false;
>>     memset_num_threads = get_memset_num_threads(smp_cpus);
>> +    started_thread = memset_num_threads;
>>     memset_thread = g_new0(MemsetThread, memset_num_threads);
>>     numpages_per_thread = (numpages / memset_num_threads);
>>     size_per_thread = (hpagesize * numpages_per_thread);
>> @@ -448,14 +450,18 @@ static bool touch_all_pages(char *area, size_t hpagesize, size_t numpages,
>>         memset_thread[i].numpages = (i == (memset_num_threads - 1)) ?
>>                                     numpages : numpages_per_thread;
>>         memset_thread[i].hpagesize = hpagesize;
>> -        /* TODO: let the callers handle the error instead of abort() here */
>> -        qemu_thread_create(&memset_thread[i].pgthread, "touch_pages",
>> -                           do_touch_pages, &memset_thread[i],
>> -                           QEMU_THREAD_JOINABLE, &error_abort);
>> +        if (!qemu_thread_create(&memset_thread[i].pgthread, "touch_pages",
>> +                                do_touch_pages, &memset_thread[i],
>> +                                QEMU_THREAD_JOINABLE, errp)) {
>> +            memset_thread_failed = true;
>> +            started_thread = i;
>> +            goto out;
> 
> break rather than goto, please.
Ok
> 
>> +        }
>>         addr += size_per_thread;
>>         numpages -= numpages_per_thread;
>>     }
>> -    for (i = 0; i < memset_num_threads; i++) {
>> +out:
>> +    for (i = 0; i < started_thread; i++) {
>>         qemu_thread_join(&memset_thread[i].pgthread);
>>     }
> 
> I don't like how @started_thread is computed.  The name suggests it's
> the number of threads started so far.  That's the case when you
> initialize it to zero.  But then you immediately set it to
> memset_thread().  It again becomes the case only when you break the loop
> on error, or when you complete it successfully.
> 
> There's no need for @started_thread, since the number of threads created
> is readily available as @i:
> 
>       memset_num_threads = i;
Thanks for this wonderful suggestion! This helps a lot! ;)
>       for (i = 0; i < memset_num_threads; i++) {
>           qemu_thread_join(&memset_thread[i].pgthread);
>       }
> 
> Rest of the function:
> 
>>     g_free(memset_thread);
>       memset_thread = NULL;
> 
>       return memset_thread_failed;
>   }
> 
> If do_touch_pages() set memset_thread_failed(), we return false without
> setting an error.  I believe you should
> 
>       if (memset_thread_failed) {
>           error_setg(errp, "os_mem_prealloc: Insufficient free host memory "
>               "pages available to allocate guest RAM");
>           return false;
>       }
>       return true;
> 
Ok
> here, and ...
> 
>> @@ -471,6 +477,7 @@ void os_mem_prealloc(int fd, char *area, size_t memory, int smp_cpus,
>>     struct sigaction act, oldact;
>>     size_t hpagesize = qemu_fd_getpagesize(fd);
>>     size_t numpages = DIV_ROUND_UP(memory, hpagesize);
>> +    Error *local_err = NULL;
>> 
>>     memset(&act, 0, sizeof(act));
>>     act.sa_handler = &sigbus_handler;
>> @@ -484,9 +491,9 @@ void os_mem_prealloc(int fd, char *area, size_t memory, int smp_cpus,
>>     }
>> 
>>     /* touch pages simultaneously */
>> -    if (touch_all_pages(area, hpagesize, numpages, smp_cpus)) {
>> -        error_setg(errp, "os_mem_prealloc: Insufficient free host memory "
>> -            "pages available to allocate guest RAM");
>> +    if (touch_all_pages(area, hpagesize, numpages, smp_cpus, &local_err)) {
>> +        error_propagate_prepend(errp, local_err, "os_mem_prealloc: Insufficient"
>> +            " free host memory pages available to allocate guest RAM: ");
>>     }
> 
> ... not mess with the error message here, i.e.
> 
>       touch_all_pages(area, hpagesize, numpages, smp_cpus), errp);
Ok, will amend this in the next version. 

Have a nice day, thanks again
Fei
> 
>> 
>>     ret = sigaction(SIGBUS, &oldact, NULL);