virt-aa-helper: replace usage of magic constant

[libvirt PATCH] virt-aa-helper: replace usage of magic constant

Posted by Pavel Hrdina via Devel 5 days, 18 hours ago

From: Pavel Hrdina <phrdina@redhat.com>

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---

I wonder if we should just drop that argument and use
QEMU_DOMAIN_STORAGE_SOURCE_CHAIN_MAX_DEPTH inside the function directly
as the only remaining caller with different value is from virstoragetest.c

 src/security/virt-aa-helper.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 211c34f926..443646c0a1 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -901,12 +901,12 @@ get_files(vahControl * ctl)
             continue;
         /* XXX - if we knew the qemu user:group here we could send it in
          *        so that the open could be re-tried as that user:group.
-         *
-         * The maximum depth is limited to 200 layers similarly to the qemu
-         * implementation.
          */
-        if (!disk->src->backingStore)
-            virStorageSourceGetMetadata(disk->src, -1, -1, 200, false);
+        if (!disk->src->backingStore) {
+            virStorageSourceGetMetadata(disk->src, -1, -1,
+                                        QEMU_DOMAIN_STORAGE_SOURCE_CHAIN_MAX_DEPTH,
+                                        false);
+        }
 
          /* XXX should handle open errors more careful than just ignoring them.
          */
-- 
2.52.0

Re: [libvirt PATCH] virt-aa-helper: replace usage of magic constant

Posted by Michal Prívozník via Devel 3 days, 13 hours ago

On 1/27/26 11:19, Pavel Hrdina via Devel wrote:
> From: Pavel Hrdina <phrdina@redhat.com>
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> 
> I wonder if we should just drop that argument and use
> QEMU_DOMAIN_STORAGE_SOURCE_CHAIN_MAX_DEPTH inside the function directly
> as the only remaining caller with different value is from virstoragetest.c

That might be also doable, but in a follow up patch.

> 
>  src/security/virt-aa-helper.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index 211c34f926..443646c0a1 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -901,12 +901,12 @@ get_files(vahControl * ctl)
>              continue;
>          /* XXX - if we knew the qemu user:group here we could send it in
>           *        so that the open could be re-tried as that user:group.
> -         *
> -         * The maximum depth is limited to 200 layers similarly to the qemu
> -         * implementation.
>           */
> -        if (!disk->src->backingStore)
> -            virStorageSourceGetMetadata(disk->src, -1, -1, 200, false);
> +        if (!disk->src->backingStore) {
> +            virStorageSourceGetMetadata(disk->src, -1, -1,
> +                                        QEMU_DOMAIN_STORAGE_SOURCE_CHAIN_MAX_DEPTH,
> +                                        false);
> +        }
>  
>           /* XXX should handle open errors more careful than just ignoring them.
>           */

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal

Re: [libvirt PATCH] virt-aa-helper: replace usage of magic constant

Posted by Michal Prívozník via Devel 3 days, 13 hours ago

On 1/29/26 16:00, Michal Prívozník wrote:
> On 1/27/26 11:19, Pavel Hrdina via Devel wrote:
>> From: Pavel Hrdina <phrdina@redhat.com>
>>
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> ---
>>
>> I wonder if we should just drop that argument and use
>> QEMU_DOMAIN_STORAGE_SOURCE_CHAIN_MAX_DEPTH inside the function directly
>> as the only remaining caller with different value is from virstoragetest.c
> 
> That might be also doable, but in a follow up patch.
> 
>>
>>  src/security/virt-aa-helper.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
>> index 211c34f926..443646c0a1 100644
>> --- a/src/security/virt-aa-helper.c
>> +++ b/src/security/virt-aa-helper.c
>> @@ -901,12 +901,12 @@ get_files(vahControl * ctl)
>>              continue;
>>          /* XXX - if we knew the qemu user:group here we could send it in
>>           *        so that the open could be re-tried as that user:group.
>> -         *
>> -         * The maximum depth is limited to 200 layers similarly to the qemu
>> -         * implementation.
>>           */
>> -        if (!disk->src->backingStore)
>> -            virStorageSourceGetMetadata(disk->src, -1, -1, 200, false);
>> +        if (!disk->src->backingStore) {
>> +            virStorageSourceGetMetadata(disk->src, -1, -1,
>> +                                        QEMU_DOMAIN_STORAGE_SOURCE_CHAIN_MAX_DEPTH,
>> +                                        false);
>> +        }
>>  
>>           /* XXX should handle open errors more careful than just ignoring them.
>>           */
> 
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> 

Ooops, hit 'send' too early. Thing is - virt-aa-helper does not include
qemu_domain.h which is where the macro is declared. So this breaks build
on systems with apparmor.

Michal

Re: [libvirt PATCH] virt-aa-helper: replace usage of magic constant

Posted by Pavel Hrdina via Devel 3 days, 12 hours ago

On Thu, Jan 29, 2026 at 04:04:17PM +0100, Michal Prívozník wrote:
> On 1/29/26 16:00, Michal Prívozník wrote:
> > On 1/27/26 11:19, Pavel Hrdina via Devel wrote:
> >> From: Pavel Hrdina <phrdina@redhat.com>
> >>
> >> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> >> ---
> >>
> >> I wonder if we should just drop that argument and use
> >> QEMU_DOMAIN_STORAGE_SOURCE_CHAIN_MAX_DEPTH inside the function directly
> >> as the only remaining caller with different value is from virstoragetest.c
> > 
> > That might be also doable, but in a follow up patch.
> > 
> >>
> >>  src/security/virt-aa-helper.c | 10 +++++-----
> >>  1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> >> index 211c34f926..443646c0a1 100644
> >> --- a/src/security/virt-aa-helper.c
> >> +++ b/src/security/virt-aa-helper.c
> >> @@ -901,12 +901,12 @@ get_files(vahControl * ctl)
> >>              continue;
> >>          /* XXX - if we knew the qemu user:group here we could send it in
> >>           *        so that the open could be re-tried as that user:group.
> >> -         *
> >> -         * The maximum depth is limited to 200 layers similarly to the qemu
> >> -         * implementation.
> >>           */
> >> -        if (!disk->src->backingStore)
> >> -            virStorageSourceGetMetadata(disk->src, -1, -1, 200, false);
> >> +        if (!disk->src->backingStore) {
> >> +            virStorageSourceGetMetadata(disk->src, -1, -1,
> >> +                                        QEMU_DOMAIN_STORAGE_SOURCE_CHAIN_MAX_DEPTH,
> >> +                                        false);
> >> +        }
> >>  
> >>           /* XXX should handle open errors more careful than just ignoring them.
> >>           */
> > 
> > Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> > 
> 
> Ooops, hit 'send' too early. Thing is - virt-aa-helper does not include
> qemu_domain.h which is where the macro is declared. So this breaks build
> on systems with apparmor.

Right, I'll fix it before pushing, thanks.

> Michal
>

Re: [libvirt PATCH] virt-aa-helper: replace usage of magic constant

Posted by Peter Krempa via Devel 3 days, 12 hours ago

On Thu, Jan 29, 2026 at 16:45:46 +0100, Pavel Hrdina via Devel wrote:
> On Thu, Jan 29, 2026 at 04:04:17PM +0100, Michal Prívozník wrote:
> > On 1/29/26 16:00, Michal Prívozník wrote:
> > > On 1/27/26 11:19, Pavel Hrdina via Devel wrote:
> > >> From: Pavel Hrdina <phrdina@redhat.com>
> > >>
> > >> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > >> ---
> > >>
> > >> I wonder if we should just drop that argument and use
> > >> QEMU_DOMAIN_STORAGE_SOURCE_CHAIN_MAX_DEPTH inside the function directly
> > >> as the only remaining caller with different value is from virstoragetest.c
> > > 
> > > That might be also doable, but in a follow up patch.
> > > 
> > >>
> > >>  src/security/virt-aa-helper.c | 10 +++++-----
> > >>  1 file changed, 5 insertions(+), 5 deletions(-)
> > >>
> > >> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> > >> index 211c34f926..443646c0a1 100644
> > >> --- a/src/security/virt-aa-helper.c
> > >> +++ b/src/security/virt-aa-helper.c
> > >> @@ -901,12 +901,12 @@ get_files(vahControl * ctl)
> > >>              continue;
> > >>          /* XXX - if we knew the qemu user:group here we could send it in
> > >>           *        so that the open could be re-tried as that user:group.
> > >> -         *
> > >> -         * The maximum depth is limited to 200 layers similarly to the qemu
> > >> -         * implementation.
> > >>           */
> > >> -        if (!disk->src->backingStore)
> > >> -            virStorageSourceGetMetadata(disk->src, -1, -1, 200, false);
> > >> +        if (!disk->src->backingStore) {
> > >> +            virStorageSourceGetMetadata(disk->src, -1, -1,
> > >> +                                        QEMU_DOMAIN_STORAGE_SOURCE_CHAIN_MAX_DEPTH,
> > >> +                                        false);
> > >> +        }
> > >>  
> > >>           /* XXX should handle open errors more careful than just ignoring them.
> > >>           */
> > > 
> > > Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> > > 
> > 
> > Ooops, hit 'send' too early. Thing is - virt-aa-helper does not include
> > qemu_domain.h which is where the macro is declared. So this breaks build
> > on systems with apparmor.
> 
> Right, I'll fix it before pushing, thanks.

IMO virt-aa-helper must not include qemu_domain.h.

Similarly if you'd want to use the constant in virStorageSourceGetMetadata
it would need to be moved to the utilities functions first and the qemu
driver fixed to use that constant afterwards.

I do agree that there's no sane reason why a different driver would want
a different value for virStorageSourceGetMetadata, so that refactor
should be feasible removing the need for this patch and the problems it
brings.

Re: [libvirt PATCH] virt-aa-helper: replace usage of magic constant

Posted by Pavel Hrdina via Devel 3 days, 12 hours ago

On Thu, Jan 29, 2026 at 04:55:06PM +0100, Peter Krempa wrote:
> On Thu, Jan 29, 2026 at 16:45:46 +0100, Pavel Hrdina via Devel wrote:
> > On Thu, Jan 29, 2026 at 04:04:17PM +0100, Michal Prívozník wrote:
> > > On 1/29/26 16:00, Michal Prívozník wrote:
> > > > On 1/27/26 11:19, Pavel Hrdina via Devel wrote:
> > > >> From: Pavel Hrdina <phrdina@redhat.com>
> > > >>
> > > >> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > > >> ---
> > > >>
> > > >> I wonder if we should just drop that argument and use
> > > >> QEMU_DOMAIN_STORAGE_SOURCE_CHAIN_MAX_DEPTH inside the function directly
> > > >> as the only remaining caller with different value is from virstoragetest.c
> > > > 
> > > > That might be also doable, but in a follow up patch.
> > > > 
> > > >>
> > > >>  src/security/virt-aa-helper.c | 10 +++++-----
> > > >>  1 file changed, 5 insertions(+), 5 deletions(-)
> > > >>
> > > >> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> > > >> index 211c34f926..443646c0a1 100644
> > > >> --- a/src/security/virt-aa-helper.c
> > > >> +++ b/src/security/virt-aa-helper.c
> > > >> @@ -901,12 +901,12 @@ get_files(vahControl * ctl)
> > > >>              continue;
> > > >>          /* XXX - if we knew the qemu user:group here we could send it in
> > > >>           *        so that the open could be re-tried as that user:group.
> > > >> -         *
> > > >> -         * The maximum depth is limited to 200 layers similarly to the qemu
> > > >> -         * implementation.
> > > >>           */
> > > >> -        if (!disk->src->backingStore)
> > > >> -            virStorageSourceGetMetadata(disk->src, -1, -1, 200, false);
> > > >> +        if (!disk->src->backingStore) {
> > > >> +            virStorageSourceGetMetadata(disk->src, -1, -1,
> > > >> +                                        QEMU_DOMAIN_STORAGE_SOURCE_CHAIN_MAX_DEPTH,
> > > >> +                                        false);
> > > >> +        }
> > > >>  
> > > >>           /* XXX should handle open errors more careful than just ignoring them.
> > > >>           */
> > > > 
> > > > Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> > > > 
> > > 
> > > Ooops, hit 'send' too early. Thing is - virt-aa-helper does not include
> > > qemu_domain.h which is where the macro is declared. So this breaks build
> > > on systems with apparmor.
> > 
> > Right, I'll fix it before pushing, thanks.
> 
> IMO virt-aa-helper must not include qemu_domain.h.

I was wondering there mist be a reason it's not used there, there is.

> Similarly if you'd want to use the constant in virStorageSourceGetMetadata
> it would need to be moved to the utilities functions first and the qemu
> driver fixed to use that constant afterwards.
> 
> I do agree that there's no sane reason why a different driver would want
> a different value for virStorageSourceGetMetadata, so that refactor
> should be feasible removing the need for this patch and the problems it
> brings.

Agreed, no need to push this patch, I'll look into the refactor.

Pavel