On 12/9/19 1:50 AM, Christian Ehrhardt wrote: > > > On Fri, Oct 11, 2019 at 9:13 PM Cole Robinson <crobinso@xxxxxxxxxx > <mailto:crobinso@xxxxxxxxxx>> wrote: >> >> Teach virt-aa-helper how to label a qcow2 data_file, tracked internally >> as externalDataStore. It should be treated the same as its sibling >> disk image >> >> Signed-off-by: Cole Robinson <crobinso@xxxxxxxxxx > <mailto:crobinso@xxxxxxxxxx>> >> --- >> Compiled but not runtime tested, I don't have an apparmor setup >> >> src/security/virt-aa-helper.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c >> index 509187ac36..fe6fa12550 100644 >> --- a/src/security/virt-aa-helper.c >> +++ b/src/security/virt-aa-helper.c >> @@ -949,6 +949,10 @@ storage_source_add_files(virStorageSourcePtr src, >> if (add_file_path(tmp, depth, buf) < 0) >> return -1; >> >> + if (src->externalDataStore && >> + storage_source_add_files(src->externalDataStore, buf, > depth) < 0) >> + return -1; >> + > > After thinking twice about it ack to carry over depth as only the first > layer should only ever need write permissions on those extra files as well. > > But I'm unsure about using "src->externalDataStore". > After all this does initialize "tmp = src". > And then iterates the (potential) backing chain. > That way each loop iteration has a different "tmp" along the backing > chain but "src" will always point to the first entry. > > It comes down to a detail question about the design of external data in > qcow. > In a case with a base file and two snapshots, does the chain look like: > > A: > Current -> Snap2 -> Snap1 > \ > external data > > or > B: > > Current -> Snap2 -> Snap1 > \ \ \ > ext-dat2 ext-dat1 ext-dat > > or > C: > > Current -> Snap2 -> Snap1 > \ > external data > > Depending on that it should IMHO be > a) call storage_source_add_files outside the loop and with depth arg > always set to zero > b+c) call storage_source_add_files with tmp instead of src as argument > (also use tmp in the check if externalDataStore exists) Indeed, it should be using tmp instead of src, nicely spotted. I'll send a v2. Thankfully the already committed selinux code got it right - Cole -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
