export/fuse: Use coroutines and multi-threading

[PATCH 02/15] fuse: Ensure init clean-up even with error_fatal

Posted by Hanna Czenczek 1 week ago

When exports are created on the command line (with the storage daemon),
errp is going to point to error_fatal.  Without ERRP_GUARD, we would
exit immediately when *errp is set, i.e. skip the clean-up code under
the `fail` label.  Use ERRP_GUARD so we always run that code.

As far as I know, this has no actual impact right now[1], but it is
still better to make this right.

[1] Not cleaning up the mount point is the only thing I can imagine
    would be problematic, but that is the last thing we attempt, so if
    it fails, it will clean itself up.

Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
---
 block/export/fuse.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/export/fuse.c b/block/export/fuse.c
index a12f479492..7c035dd6ca 100644
--- a/block/export/fuse.c
+++ b/block/export/fuse.c
@@ -119,6 +119,7 @@ static int fuse_export_create(BlockExport *blk_exp,
                               BlockExportOptions *blk_exp_args,
                               Error **errp)
 {
+    ERRP_GUARD(); /* ensure clean-up even with error_fatal */
     FuseExport *exp = container_of(blk_exp, FuseExport, common);
     BlockExportOptionsFuse *args = &blk_exp_args->u.fuse;
     int ret;
-- 
2.48.1

Re: [PATCH 02/15] fuse: Ensure init clean-up even with error_fatal

Posted by Stefan Hajnoczi 5 days, 22 hours ago

On Tue, Mar 25, 2025 at 05:06:42PM +0100, Hanna Czenczek wrote:
> When exports are created on the command line (with the storage daemon),
> errp is going to point to error_fatal.  Without ERRP_GUARD, we would
> exit immediately when *errp is set, i.e. skip the clean-up code under
> the `fail` label.  Use ERRP_GUARD so we always run that code.
> 
> As far as I know, this has no actual impact right now[1], but it is
> still better to make this right.
> 
> [1] Not cleaning up the mount point is the only thing I can imagine
>     would be problematic, but that is the last thing we attempt, so if
>     it fails, it will clean itself up.
> 
> Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
> ---
>  block/export/fuse.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [PATCH 02/15] fuse: Ensure init clean-up even with error_fatal

Posted by Markus Armbruster 1 week ago

Hanna Czenczek <hreitz@redhat.com> writes:

> When exports are created on the command line (with the storage daemon),
> errp is going to point to error_fatal.  Without ERRP_GUARD, we would
> exit immediately when *errp is set, i.e. skip the clean-up code under
> the `fail` label.  Use ERRP_GUARD so we always run that code.
>
> As far as I know, this has no actual impact right now[1], but it is
> still better to make this right.
>
> [1] Not cleaning up the mount point is the only thing I can imagine
>     would be problematic, but that is the last thing we attempt, so if
>     it fails, it will clean itself up.

Hmm.

The pattern is "no cleanup with &error_fatal or &error_abort, but not
cleaning up then is harmless".  How many instances do we have?  My gut
feeling is in the hundreds.  Why is "fixing" just this one worth the
bother?

> Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
> ---
>  block/export/fuse.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/block/export/fuse.c b/block/export/fuse.c
> index a12f479492..7c035dd6ca 100644
> --- a/block/export/fuse.c
> +++ b/block/export/fuse.c
> @@ -119,6 +119,7 @@ static int fuse_export_create(BlockExport *blk_exp,
>                                BlockExportOptions *blk_exp_args,
>                                Error **errp)
>  {
> +    ERRP_GUARD(); /* ensure clean-up even with error_fatal */
>      FuseExport *exp = container_of(blk_exp, FuseExport, common);
>      BlockExportOptionsFuse *args = &blk_exp_args->u.fuse;
>      int ret;

Re: [PATCH 02/15] fuse: Ensure init clean-up even with error_fatal

Posted by Hanna Czenczek 1 week ago

On 26.03.25 06:47, Markus Armbruster wrote:
> Hanna Czenczek <hreitz@redhat.com> writes:
>
>> When exports are created on the command line (with the storage daemon),
>> errp is going to point to error_fatal.  Without ERRP_GUARD, we would
>> exit immediately when *errp is set, i.e. skip the clean-up code under
>> the `fail` label.  Use ERRP_GUARD so we always run that code.
>>
>> As far as I know, this has no actual impact right now[1], but it is
>> still better to make this right.
>>
>> [1] Not cleaning up the mount point is the only thing I can imagine
>>      would be problematic, but that is the last thing we attempt, so if
>>      it fails, it will clean itself up.
> Hmm.
>
> The pattern is "no cleanup with &error_fatal or &error_abort, but not
> cleaning up then is harmless".  How many instances do we have?  My gut
> feeling is in the hundreds.  Why is "fixing" just this one worth the
> bother?

Because:
1. This one is in FUSE code, which I’m reworking in this series.
2. I did encounter this issue while playing around with manual mounting 
last year.  I don’t think it has visible impact when mounting with 
libfuse, but why leave out a fix for something that can be triggered by 
making valid changes to the code?

Hanna

>
>> Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
>> ---
>>   block/export/fuse.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/block/export/fuse.c b/block/export/fuse.c
>> index a12f479492..7c035dd6ca 100644
>> --- a/block/export/fuse.c
>> +++ b/block/export/fuse.c
>> @@ -119,6 +119,7 @@ static int fuse_export_create(BlockExport *blk_exp,
>>                                 BlockExportOptions *blk_exp_args,
>>                                 Error **errp)
>>   {
>> +    ERRP_GUARD(); /* ensure clean-up even with error_fatal */
>>       FuseExport *exp = container_of(blk_exp, FuseExport, common);
>>       BlockExportOptionsFuse *args = &blk_exp_args->u.fuse;
>>       int ret;