3:42 a.m.
The major problem was with symlinks. Imagine the following chain of symlinks:
/dev/my_awesome_disk -> /home/user/blaah -> /dev/disk/by-uuid/$uuid -> /dev/sda
We really need to create all those /dev/* symlinks and /dev/sda device. Also,
some other (less critical) bugs are fixed.
Michal Privoznik (10):
virProcessRunInMountNamespace: Report errors from child
util: Introduce virFileReadLink
qemuDomainPrepareDisk: Fix ordering
qemuSecurityRestoreAllLabel: Don't use transactions
qemu_security: Use more transactions
qemuDomain{Attach,Detach}Device NS helpers: Don't relabel devices
qemuDomainCreateDevice: Properly deal with symlinks
qemuDomainCreateDevice: Don't loop endlessly
qemuDomainAttachDeviceMknod: Deal with symlinks
qemuDomainAttachDeviceMknod: Don't loop endlessly
src/libvirt_private.syms | 1 +
src/qemu/qemu_domain.c | 438 +++++++++++++++++++++++++++--------------------
src/qemu/qemu_hotplug.c | 20 +--
src/qemu/qemu_security.c | 137 +++++++++------
src/util/virfile.c | 12 ++
src/util/virfile.h | 2 +
src/util/virprocess.c | 8 +-
7 files changed, 374 insertions(+), 244 deletions(-)
--
2.11.0
3:42 a.m.
New subject: [libvirt] [PATCH 01/10] virProcessRunInMountNamespace: Report errors from child
The comment to the function states that the errors from the child
process are reported. Well, the error buffer is filled with
possible error messages. But then it is thrown away. Among with
important error message from the child process.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virprocess.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index f5c7ebb96..16eb41212 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1137,8 +1137,14 @@ virProcessRunInMountNamespace(pid_t pid,
VIR_FORCE_CLOSE(errfd[1]);
ignore_value(virFileReadHeaderFD(errfd[0], 1024, &buf));
ret = virProcessWait(child, &status, false);
- if (!ret)
+ if (!ret) {
ret = status == EXIT_CANCELED ? -1 : status;
+ if (ret) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("child reported: %s"),
+ NULLSTR(buf));
+ }
+ }
VIR_FREE(buf);
}
--
2.11.0
8:08 a.m.
New subject: [libvirt] [PATCH 01/10] virProcessRunInMountNamespace: Report errors from child
On Fri, Jan 20, 2017 at 10:42:41AM +0100, Michal Privoznik wrote:
The comment to the function states that the errors from the child
process are reported. Well, the error buffer is filled with
possible error messages. But then it is thrown away. Among with
important error message from the child process.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virprocess.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
ACK
3:42 a.m.
New subject: [libvirt] [PATCH 02/10] util: Introduce virFileReadLink
We will need to traverse the symlinks one step at the time.
Therefore we need to see where a symlink is pointing to.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virfile.c | 12 ++++++++++++
src/util/virfile.h | 2 ++
3 files changed, 15 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index cfeb43cf0..7f7dcfe44 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1615,6 +1615,7 @@ virFileReadAllQuiet;
virFileReadBufQuiet;
virFileReadHeaderFD;
virFileReadLimFD;
+virFileReadLink;
virFileRelLinkPointsTo;
virFileRemove;
virFileRemoveLastComponent;
diff --git a/src/util/virfile.c b/src/util/virfile.c
index bf8099e34..49ea1d1f0 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -76,6 +76,7 @@
#include "virutil.h"
#include "c-ctype.h"
+#include "areadlink.h"
#define VIR_FROM_THIS VIR_FROM_NONE
@@ -1614,6 +1615,17 @@ virFileIsLink(const char *linkpath)
return S_ISLNK(st.st_mode) != 0;
}
+/*
+ * Read where symlink is pointing to.
+ *
+ * Returns 0 on success (@linkpath is a successfully read link),
+ * -1 with errno set upon error.
+ */
+int
+virFileReadLink(const char *linkpath, char **resultpath)
+{
+ return (*resultpath = areadlink(linkpath)) ? 0 : -1;
+}
/*
* Finds a requested executable file in the PATH env. e.g.:
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 0343acd5b..981a9e07d 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -166,6 +166,8 @@ int virFileResolveAllLinks(const char *linkpath,
int virFileIsLink(const char *linkpath)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
+int virFileReadLink(const char *linkpath, char **resultpath);
+
char *virFindFileInPath(const char *file);
char *virFileFindResource(const char *filename,
--
2.11.0
8:11 a.m.
New subject: [libvirt] [PATCH 02/10] util: Introduce virFileReadLink
On Fri, Jan 20, 2017 at 10:42:42AM +0100, Michal Privoznik wrote:
We will need to traverse the symlinks one step at the time.
Therefore we need to see where a symlink is pointing to.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virfile.c | 12 ++++++++++++
src/util/virfile.h | 2 ++
3 files changed, 15 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index cfeb43cf0..7f7dcfe44 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1615,6 +1615,7 @@ virFileReadAllQuiet;
virFileReadBufQuiet;
virFileReadHeaderFD;
virFileReadLimFD;
+virFileReadLink;
virFileRelLinkPointsTo;
virFileRemove;
virFileRemoveLastComponent;
diff --git a/src/util/virfile.c b/src/util/virfile.c
index bf8099e34..49ea1d1f0 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -76,6 +76,7 @@
#include "virutil.h"
#include "c-ctype.h"
+#include "areadlink.h"
#define VIR_FROM_THIS VIR_FROM_NONE
@@ -1614,6 +1615,17 @@ virFileIsLink(const char *linkpath)
return S_ISLNK(st.st_mode) != 0;
}
+/*
+ * Read where symlink is pointing to.
+ *
+ * Returns 0 on success (@linkpath is a successfully read link),
+ * -1 with errno set upon error.
+ */
+int
+virFileReadLink(const char *linkpath, char **resultpath)
+{
+ return (*resultpath = areadlink(linkpath)) ? 0 : -1;
+}
I don't really see the benefit of this function, are you trying to
obfuscate it? You essentially double the return information for no
reason and try to push it all into one line.
/*
* Finds a requested executable file in the PATH env. e.g.:
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 0343acd5b..981a9e07d 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -166,6 +166,8 @@ int virFileResolveAllLinks(const char *linkpath,
int virFileIsLink(const char *linkpath)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
+int virFileReadLink(const char *linkpath, char **resultpath);
+
char *virFindFileInPath(const char *file);
char *virFileFindResource(const char *filename,
--
2.11.0
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
9:09 a.m.
New subject: [libvirt] [PATCH 02/10] util: Introduce virFileReadLink
On Thu, Feb 02, 2017 at 03:11:56PM +0100, Martin Kletzander wrote:
On Fri, Jan 20, 2017 at 10:42:42AM +0100, Michal Privoznik wrote:
> We will need to traverse the symlinks one step at the time.
> Therefore we need to see where a symlink is pointing to.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/libvirt_private.syms | 1 +
> src/util/virfile.c | 12 ++++++++++++
> src/util/virfile.h | 2 ++
> 3 files changed, 15 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index cfeb43cf0..7f7dcfe44 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1615,6 +1615,7 @@ virFileReadAllQuiet;
> virFileReadBufQuiet;
> virFileReadHeaderFD;
> virFileReadLimFD;
> +virFileReadLink;
> virFileRelLinkPointsTo;
> virFileRemove;
> virFileRemoveLastComponent;
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index bf8099e34..49ea1d1f0 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -76,6 +76,7 @@
> #include "virutil.h"
>
> #include "c-ctype.h"
> +#include "areadlink.h"
>
> #define VIR_FROM_THIS VIR_FROM_NONE
>
> @@ -1614,6 +1615,17 @@ virFileIsLink(const char *linkpath)
> return S_ISLNK(st.st_mode) != 0;
> }
>
> +/*
> + * Read where symlink is pointing to.
> + *
> + * Returns 0 on success (@linkpath is a successfully read link),
> + * -1 with errno set upon error.
> + */
> +int
> +virFileReadLink(const char *linkpath, char **resultpath)
> +{
> + return (*resultpath = areadlink(linkpath)) ? 0 : -1;
> +}
>
I don't really see the benefit of this function, are you trying to
obfuscate it? You essentially double the return information for no
reason and try to push it all into one line.
It would be useful if you have an ATTRIBUTE_RETURN_CHECK annotation
on it, as it would allow compiler time verification of error checking,
which is not possible when you overload the return value to contain
both the data and error indicator.
> /*
> * Finds a requested executable file in the PATH env. e.g.:
> diff --git a/src/util/virfile.h b/src/util/virfile.h
> index 0343acd5b..981a9e07d 100644
> --- a/src/util/virfile.h
> +++ b/src/util/virfile.h
> @@ -166,6 +166,8 @@ int virFileResolveAllLinks(const char *linkpath,
> int virFileIsLink(const char *linkpath)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
>
> +int virFileReadLink(const char *linkpath, char **resultpath);
eg add ATTRIBUTE_RETURN_CHECK here
> +
> char *virFindFileInPath(const char *file);
>
> char *virFileFindResource(const char *filename,
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
1:57 p.m.
New subject: [libvirt] [PATCH 02/10] util: Introduce virFileReadLink
On Thu, Feb 02, 2017 at 03:09:15PM +0000, Daniel P. Berrange wrote:
On Thu, Feb 02, 2017 at 03:11:56PM +0100, Martin Kletzander wrote:
> On Fri, Jan 20, 2017 at 10:42:42AM +0100, Michal Privoznik wrote:
> > We will need to traverse the symlinks one step at the time.
> > Therefore we need to see where a symlink is pointing to.
> >
> > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> > ---
> > src/libvirt_private.syms | 1 +
> > src/util/virfile.c | 12 ++++++++++++
> > src/util/virfile.h | 2 ++
> > 3 files changed, 15 insertions(+)
> >
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index cfeb43cf0..7f7dcfe44 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -1615,6 +1615,7 @@ virFileReadAllQuiet;
> > virFileReadBufQuiet;
> > virFileReadHeaderFD;
> > virFileReadLimFD;
> > +virFileReadLink;
> > virFileRelLinkPointsTo;
> > virFileRemove;
> > virFileRemoveLastComponent;
> > diff --git a/src/util/virfile.c b/src/util/virfile.c
> > index bf8099e34..49ea1d1f0 100644
> > --- a/src/util/virfile.c
> > +++ b/src/util/virfile.c
> > @@ -76,6 +76,7 @@
> > #include "virutil.h"
> >
> > #include "c-ctype.h"
> > +#include "areadlink.h"
> >
> > #define VIR_FROM_THIS VIR_FROM_NONE
> >
> > @@ -1614,6 +1615,17 @@ virFileIsLink(const char *linkpath)
> > return S_ISLNK(st.st_mode) != 0;
> > }
> >
> > +/*
> > + * Read where symlink is pointing to.
> > + *
> > + * Returns 0 on success (@linkpath is a successfully read link),
> > + * -1 with errno set upon error.
> > + */
> > +int
> > +virFileReadLink(const char *linkpath, char **resultpath)
> > +{
> > + return (*resultpath = areadlink(linkpath)) ? 0 : -1;
> > +}
> >
>
> I don't really see the benefit of this function, are you trying to
> obfuscate it? You essentially double the return information for no
> reason and try to push it all into one line.
It would be useful if you have an ATTRIBUTE_RETURN_CHECK annotation
on it, as it would allow compiler time verification of error checking,
which is not possible when you overload the return value to contain
both the data and error indicator.
More like if there was another logic, e.g. checking that it is a link
and if it is not, returning the same path or similar.
ATTRIBUTE_RETURN_CHECK doesn't make much sense to me here since you can
get the same information from the second parameter and you are basically
making the caller use two values that have the same information.
Moreover you can set ATTRIBUTE_RETURN_CHECK for the function and just
directly return the output of areadlink(), essentially just renaming
it. I don't see the point in that.
It would be different if the function guaranteed non-modification of
that parameter when it errors out, for example. Or set an call
virReportSystemError(errno, ...); so that the error reporting is done
consistently and in one place. I haven't checked yet how Michal uses
it, though, so I can't say what's the best solution for now.
But I now see where it comes from. It's the virFileResolveAllLinks()
that does the same thing. That doesn't mean it's right, though.
My question is this. If returning integer, which does not contain any
other value then just the error, is desired, should we mandate that for
non-simple functions and rewrite, for example, qemuBuildCommandLine()?
> > /*
> > * Finds a requested executable file in the PATH env. e.g.:
> > diff --git a/src/util/virfile.h b/src/util/virfile.h
> > index 0343acd5b..981a9e07d 100644
> > --- a/src/util/virfile.h
> > +++ b/src/util/virfile.h
> > @@ -166,6 +166,8 @@ int virFileResolveAllLinks(const char *linkpath,
> > int virFileIsLink(const char *linkpath)
> > ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
> >
> > +int virFileReadLink(const char *linkpath, char **resultpath);
eg add ATTRIBUTE_RETURN_CHECK here
> > +
> > char *virFindFileInPath(const char *file);
> >
> > char *virFileFindResource(const char *filename,
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
3:54 a.m.
New subject: [libvirt] [PATCH 02/10] util: Introduce virFileReadLink
On Thu, Feb 02, 2017 at 08:57:53PM +0100, Martin Kletzander wrote:
On Thu, Feb 02, 2017 at 03:09:15PM +0000, Daniel P. Berrange wrote:
>On Thu, Feb 02, 2017 at 03:11:56PM +0100, Martin Kletzander wrote:
>> On Fri, Jan 20, 2017 at 10:42:42AM +0100, Michal Privoznik wrote:
>> > We will need to traverse the symlinks one step at the time.
>> > Therefore we need to see where a symlink is pointing to.
>> >
>> > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>> > ---
>> > src/libvirt_private.syms | 1 +
>> > src/util/virfile.c | 12 ++++++++++++
>> > src/util/virfile.h | 2 ++
>> > 3 files changed, 15 insertions(+)
>> >
>> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> > index cfeb43cf0..7f7dcfe44 100644
>> > --- a/src/libvirt_private.syms
>> > +++ b/src/libvirt_private.syms
>> > @@ -1615,6 +1615,7 @@ virFileReadAllQuiet;
>> > virFileReadBufQuiet;
>> > virFileReadHeaderFD;
>> > virFileReadLimFD;
>> > +virFileReadLink;
>> > virFileRelLinkPointsTo;
>> > virFileRemove;
>> > virFileRemoveLastComponent;
>> > diff --git a/src/util/virfile.c b/src/util/virfile.c
>> > index bf8099e34..49ea1d1f0 100644
>> > --- a/src/util/virfile.c
>> > +++ b/src/util/virfile.c
>> > @@ -76,6 +76,7 @@
>> > #include "virutil.h"
>> >
>> > #include "c-ctype.h"
>> > +#include "areadlink.h"
>> >
>> > #define VIR_FROM_THIS VIR_FROM_NONE
>> >
>> > @@ -1614,6 +1615,17 @@ virFileIsLink(const char *linkpath)
>> > return S_ISLNK(st.st_mode) != 0;
>> > }
>> >
>> > +/*
>> > + * Read where symlink is pointing to.
>> > + *
>> > + * Returns 0 on success (@linkpath is a successfully read link),
>> > + * -1 with errno set upon error.
>> > + */
>> > +int
>> > +virFileReadLink(const char *linkpath, char **resultpath)
>> > +{
>> > + return (*resultpath = areadlink(linkpath)) ? 0 : -1;
>> > +}
>> >
>>
>> I don't really see the benefit of this function, are you trying to
>> obfuscate it? You essentially double the return information for no
>> reason and try to push it all into one line.
>
>It would be useful if you have an ATTRIBUTE_RETURN_CHECK annotation
>on it, as it would allow compiler time verification of error checking,
>which is not possible when you overload the return value to contain
>both the data and error indicator.
>
More like if there was another logic, e.g. checking that it is a link
and if it is not, returning the same path or similar.
ATTRIBUTE_RETURN_CHECK doesn't make much sense to me here since you can
get the same information from the second parameter and you are basically
making the caller use two values that have the same information.
Moreover you can set ATTRIBUTE_RETURN_CHECK for the function and just
directly return the output of areadlink(), essentially just renaming
it. I don't see the point in that.
It would be different if the function guaranteed non-modification of
that parameter when it errors out, for example. Or set an call
virReportSystemError(errno, ...); so that the error reporting is done
consistently and in one place. I haven't checked yet how Michal uses
it, though, so I can't say what's the best solution for now.
But I now see where it comes from. It's the virFileResolveAllLinks()
that does the same thing. That doesn't mean it's right, though.
My question is this. If returning integer, which does not contain any
other value then just the error, is desired, should we mandate that for
non-simple functions and rewrite, for example, qemuBuildCommandLine()?
>> > /*
>> > * Finds a requested executable file in the PATH env. e.g.:
>> > diff --git a/src/util/virfile.h b/src/util/virfile.h
>> > index 0343acd5b..981a9e07d 100644
>> > --- a/src/util/virfile.h
>> > +++ b/src/util/virfile.h
>> > @@ -166,6 +166,8 @@ int virFileResolveAllLinks(const char *linkpath,
>> > int virFileIsLink(const char *linkpath)
>> > ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
>> >
>> > +int virFileReadLink(const char *linkpath, char **resultpath);
>
>eg add ATTRIBUTE_RETURN_CHECK here
>
So it seems like Michal is not participating in the discussion about his
own patch. But I talked to him personally and he admitted that the
function prototype was meant to look like it does simply to be just like
the other functions, e.g. virFileResolveAllLinks().
So for now I'm OK with it, we can change how all the functions look
later on, but that's a discussion to be had. So ACK with the
ATTRIBUTE_RETURN_CHECK added here.
>> > +
>> > char *virFindFileInPath(const char *file);
>> >
>> > char *virFileFindResource(const char *filename,
>
>
>Regards,
>Daniel
>--
>|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
>|: http://libvirt.org -o- http://virt-manager.org :|
>|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
3:42 a.m.
New subject: [libvirt] [PATCH 03/10] qemuDomainPrepareDisk: Fix ordering
The current ordering is as follows:
1) set label
2) create the device in namespace
3) allow device in the cgroup
While this might work for now, it will definitely not work if the
security driver would use transactions as in that case there
would be no device to relabel in the domain namespace as the
device is created in the second step.
Swap steps 1) and 2) to allow security driver to use more
transactions.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 5b0a3f515..7493d525a 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -110,15 +110,15 @@ qemuDomainPrepareDisk(virQEMUDriverPtr driver,
vm, disk) < 0)
goto cleanup;
- if (qemuSecuritySetDiskLabel(driver, vm, disk) < 0)
- goto rollback_lock;
-
if (qemuDomainNamespaceSetupDisk(driver, vm, disk) < 0)
- goto rollback_label;
+ goto rollback_lock;
- if (qemuSetupDiskCgroup(vm, disk) < 0)
+ if (qemuSecuritySetDiskLabel(driver, vm, disk) < 0)
goto rollback_namespace;
+ if (qemuSetupDiskCgroup(vm, disk) < 0)
+ goto rollback_label;
+
ret = 0;
goto cleanup;
@@ -126,16 +126,16 @@ qemuDomainPrepareDisk(virQEMUDriverPtr driver,
if (qemuTeardownDiskCgroup(vm, disk) < 0)
VIR_WARN("Unable to tear down cgroup access on %s",
virDomainDiskGetSource(disk));
- rollback_namespace:
- if (qemuDomainNamespaceTeardownDisk(driver, vm, disk) < 0)
- VIR_WARN("Unable to remove /dev entry for %s",
- virDomainDiskGetSource(disk));
-
rollback_label:
if (qemuSecurityRestoreDiskLabel(driver, vm, disk) < 0)
VIR_WARN("Unable to restore security label on %s",
virDomainDiskGetSource(disk));
+ rollback_namespace:
+ if (qemuDomainNamespaceTeardownDisk(driver, vm, disk) < 0)
+ VIR_WARN("Unable to remove /dev entry for %s",
+ virDomainDiskGetSource(disk));
+
rollback_lock:
if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0)
VIR_WARN("Unable to release lock on %s",
--
2.11.0
8:14 a.m.
New subject: [libvirt] [PATCH 03/10] qemuDomainPrepareDisk: Fix ordering
On Fri, Jan 20, 2017 at 10:42:43AM +0100, Michal Privoznik wrote:
The current ordering is as follows:
1) set label
2) create the device in namespace
3) allow device in the cgroup
While this might work for now, it will definitely not work if the
security driver would use transactions as in that case there
would be no device to relabel in the domain namespace as the
device is created in the second step.
Swap steps 1) and 2) to allow security driver to use more
transactions.
ACK, makes more sense if I don't read the message =)
3:42 a.m.
New subject: [libvirt] [PATCH 04/10] qemuSecurityRestoreAllLabel: Don't use transactions
Because of the nature of security driver transactions, it is
impossible to use them properly. The thing is, transactions enter
the domain namespace and commit all the seclabel changes.
However, in RestoreAllLabel() this is impossible - the qemu
process, the only process running in the namespace, is gone. And
thus is the namespace. Therefore we shouldn't use the transactions
as there is no namespace to enter.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_security.c | 25 +++++++++----------------
1 file changed, 9 insertions(+), 16 deletions(-)
diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index 544feeb4a..13d99cdbd 100644
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -73,22 +73,15 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
virDomainObjPtr vm,
bool migrated)
{
- if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
- virSecurityManagerTransactionStart(driver->securityManager) < 0)
- goto cleanup;
-
- if (virSecurityManagerRestoreAllLabel(driver->securityManager,
- vm->def,
- migrated) < 0)
- goto cleanup;
-
- if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
- virSecurityManagerTransactionCommit(driver->securityManager,
- vm->pid) < 0)
- goto cleanup;
-
- cleanup:
- virSecurityManagerTransactionAbort(driver->securityManager);
+ /* In contrast to qemuSecuritySetAllLabel, do not use
+ * secdriver transactions here. This function is called from
+ * qemuProcessStop() which is meant to do cleanup after qemu
+ * process died. If it did do, the namespace is gone as qemu
+ * was the only process running there. We would not succeed
+ * in entering the namespace then. */
+ virSecurityManagerRestoreAllLabel(driver->securityManager,
+ vm->def,
+ migrated);
}
--
2.11.0
4:18 a.m.
New subject: [libvirt] [PATCH 04/10] qemuSecurityRestoreAllLabel: Don't use transactions
On Fri, Jan 20, 2017 at 10:42:44AM +0100, Michal Privoznik wrote:
Because of the nature of security driver transactions, it is
impossible to use them properly. The thing is, transactions enter
the domain namespace and commit all the seclabel changes.
However, in RestoreAllLabel() this is impossible - the qemu
process, the only process running in the namespace, is gone. And
thus is the namespace. Therefore we shouldn't use the transactions
as there is no namespace to enter.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_security.c | 25 +++++++++----------------
1 file changed, 9 insertions(+), 16 deletions(-)
diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index 544feeb4a..13d99cdbd 100644
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -73,22 +73,15 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
virDomainObjPtr vm,
bool migrated)
{
- if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
- virSecurityManagerTransactionStart(driver->securityManager) < 0)
- goto cleanup;
-
- if (virSecurityManagerRestoreAllLabel(driver->securityManager,
- vm->def,
- migrated) < 0)
- goto cleanup;
-
- if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
- virSecurityManagerTransactionCommit(driver->securityManager,
- vm->pid) < 0)
- goto cleanup;
-
- cleanup:
- virSecurityManagerTransactionAbort(driver->securityManager);
+ /* In contrast to qemuSecuritySetAllLabel, do not use
+ * secdriver transactions here. This function is called from
+ * qemuProcessStop() which is meant to do cleanup after qemu
+ * process died. If it did do, the namespace is gone as qemu
+ * was the only process running there. We would not succeed
+ * in entering the namespace then. */
+ virSecurityManagerRestoreAllLabel(driver->securityManager,
+ vm->def,
+ migrated);
This means we'll be running restore on /dev/BLAH in the host namespace,
which is a file we didn't set a label/permissions on originally. I think
this is probably safe though, as we'd just be resetting it to a label
that it should already have and thus be a no-op. I'm a little more
concerned about file permiissions though, as we might end up changing
group from "disk" to "root" for example.
It feels like we need to explicitly skip restore for block devices
if we had namespaces in use previously.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
4:34 a.m.
New subject: [libvirt] [PATCH 04/10] qemuSecurityRestoreAllLabel: Don't use transactions
On 01/20/2017 11:18 AM, Daniel P. Berrange wrote:
On Fri, Jan 20, 2017 at 10:42:44AM +0100, Michal Privoznik wrote:
> Because of the nature of security driver transactions, it is
> impossible to use them properly. The thing is, transactions enter
> the domain namespace and commit all the seclabel changes.
> However, in RestoreAllLabel() this is impossible - the qemu
> process, the only process running in the namespace, is gone. And
> thus is the namespace. Therefore we shouldn't use the transactions
> as there is no namespace to enter.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/qemu/qemu_security.c | 25 +++++++++----------------
> 1 file changed, 9 insertions(+), 16 deletions(-)
>
> diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
> index 544feeb4a..13d99cdbd 100644
> --- a/src/qemu/qemu_security.c
> +++ b/src/qemu/qemu_security.c
> @@ -73,22 +73,15 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> bool migrated)
> {
> - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
> - virSecurityManagerTransactionStart(driver->securityManager) < 0)
> - goto cleanup;
> -
> - if (virSecurityManagerRestoreAllLabel(driver->securityManager,
> - vm->def,
> - migrated) < 0)
> - goto cleanup;
> -
> - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
> - virSecurityManagerTransactionCommit(driver->securityManager,
> - vm->pid) < 0)
> - goto cleanup;
> -
> - cleanup:
> - virSecurityManagerTransactionAbort(driver->securityManager);
> + /* In contrast to qemuSecuritySetAllLabel, do not use
> + * secdriver transactions here. This function is called from
> + * qemuProcessStop() which is meant to do cleanup after qemu
> + * process died. If it did do, the namespace is gone as qemu
> + * was the only process running there. We would not succeed
> + * in entering the namespace then. */
> + virSecurityManagerRestoreAllLabel(driver->securityManager,
> + vm->def,
> + migrated);
This means we'll be running restore on /dev/BLAH in the host namespace,
which is a file we didn't set a label/permissions on originally. I think
this is probably safe though, as we'd just be resetting it to a label
that it should already have and thus be a no-op. I'm a little more
concerned about file permiissions though, as we might end up changing
group from "disk" to "root" for example.
Which is no different from current situation with namespaces disabled or
using just any pre 3.0.0 release.
It feels like we need to explicitly skip restore for block devices
if we had namespaces in use previously.
If we want to go this way, sure. But I'm not quite sure how to design
this. E.g. have a filter callback that would return "yes restore label
on this path", or "no, do not restore anything on this path". And
obviously set the callback only for the RestoreAll() if the namespaces
are enabled? Or what do you suggest?
Michal
4:41 a.m.
New subject: [libvirt] [PATCH 04/10] qemuSecurityRestoreAllLabel: Don't use transactions
On Fri, Jan 20, 2017 at 11:34:06AM +0100, Michal Privoznik wrote:
On 01/20/2017 11:18 AM, Daniel P. Berrange wrote:
> On Fri, Jan 20, 2017 at 10:42:44AM +0100, Michal Privoznik wrote:
>> Because of the nature of security driver transactions, it is
>> impossible to use them properly. The thing is, transactions enter
>> the domain namespace and commit all the seclabel changes.
>> However, in RestoreAllLabel() this is impossible - the qemu
>> process, the only process running in the namespace, is gone. And
>> thus is the namespace. Therefore we shouldn't use the transactions
>> as there is no namespace to enter.
>>
>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>> ---
>> src/qemu/qemu_security.c | 25 +++++++++----------------
>> 1 file changed, 9 insertions(+), 16 deletions(-)
>>
>> diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
>> index 544feeb4a..13d99cdbd 100644
>> --- a/src/qemu/qemu_security.c
>> +++ b/src/qemu/qemu_security.c
>> @@ -73,22 +73,15 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
>> virDomainObjPtr vm,
>> bool migrated)
>> {
>> - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>> - virSecurityManagerTransactionStart(driver->securityManager) < 0)
>> - goto cleanup;
>> -
>> - if (virSecurityManagerRestoreAllLabel(driver->securityManager,
>> - vm->def,
>> - migrated) < 0)
>> - goto cleanup;
>> -
>> - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>> - virSecurityManagerTransactionCommit(driver->securityManager,
>> - vm->pid) < 0)
>> - goto cleanup;
>> -
>> - cleanup:
>> - virSecurityManagerTransactionAbort(driver->securityManager);
>> + /* In contrast to qemuSecuritySetAllLabel, do not use
>> + * secdriver transactions here. This function is called from
>> + * qemuProcessStop() which is meant to do cleanup after qemu
>> + * process died. If it did do, the namespace is gone as qemu
>> + * was the only process running there. We would not succeed
>> + * in entering the namespace then. */
>> + virSecurityManagerRestoreAllLabel(driver->securityManager,
>> + vm->def,
>> + migrated);
>
> This means we'll be running restore on /dev/BLAH in the host namespace,
> which is a file we didn't set a label/permissions on originally. I think
> this is probably safe though, as we'd just be resetting it to a label
> that it should already have and thus be a no-op. I'm a little more
> concerned about file permiissions though, as we might end up changing
> group from "disk" to "root" for example.
>
Which is no different from current situation with namespaces disabled or
using just any pre 3.0.0 release.
Hmm, good point. No need to solve this right now then - it can just be
a todo item
> It feels like we need to explicitly skip restore for block
devices
> if we had namespaces in use previously.
If we want to go this way, sure. But I'm not quite sure how to design
this. E.g. have a filter callback that would return "yes restore label
on this path", or "no, do not restore anything on this path". And
obviously set the callback only for the RestoreAll() if the namespaces
are enabled? Or what do you suggest?
I guess, lets not special case this right now - we have the long standing
problem of (not) correctly restoring permissions for all paths, whether
files or block devs.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
8:16 a.m.
New subject: [libvirt] [PATCH 04/10] qemuSecurityRestoreAllLabel: Don't use transactions
On Fri, Jan 20, 2017 at 10:42:44AM +0100, Michal Privoznik wrote:
Because of the nature of security driver transactions, it is
impossible to use them properly. The thing is, transactions enter
the domain namespace and commit all the seclabel changes.
However, in RestoreAllLabel() this is impossible - the qemu
process, the only process running in the namespace, is gone. And
thus is the namespace. Therefore we shouldn't use the transactions
as there is no namespace to enter.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_security.c | 25 +++++++++----------------
1 file changed, 9 insertions(+), 16 deletions(-)
ACK, no need to duplicate the same information in the commit message and
the comment below IMHO.
diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index 544feeb4a..13d99cdbd 100644
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -73,22 +73,15 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
virDomainObjPtr vm,
bool migrated)
{
- if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
- virSecurityManagerTransactionStart(driver->securityManager) < 0)
- goto cleanup;
-
- if (virSecurityManagerRestoreAllLabel(driver->securityManager,
- vm->def,
- migrated) < 0)
- goto cleanup;
-
- if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
- virSecurityManagerTransactionCommit(driver->securityManager,
- vm->pid) < 0)
- goto cleanup;
-
- cleanup:
- virSecurityManagerTransactionAbort(driver->securityManager);
+ /* In contrast to qemuSecuritySetAllLabel, do not use
+ * secdriver transactions here. This function is called from
+ * qemuProcessStop() which is meant to do cleanup after qemu
+ * process died. If it did do, the namespace is gone as qemu
+ * was the only process running there. We would not succeed
+ * in entering the namespace then. */
+ virSecurityManagerRestoreAllLabel(driver->securityManager,
+ vm->def,
+ migrated);
}
--
2.11.0
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
3:42 a.m.
New subject: [libvirt] [PATCH 05/10] qemu_security: Use more transactions
The idea is to move all the seclabel setting to security driver.
Having the relabel code spread all over the place looks very
messy.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_security.c | 112 +++++++++++++++++++++++++++++++++--------------
1 file changed, 80 insertions(+), 32 deletions(-)
diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index 13d99cdbd..9d1a87971 100644
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -90,14 +90,26 @@ qemuSecuritySetDiskLabel(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainDiskDefPtr disk)
{
- if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) {
- /* Already handled by namespace code. */
- return 0;
- }
+ int ret = -1;
- return virSecurityManagerSetDiskLabel(driver->securityManager,
+ if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
+ virSecurityManagerTransactionStart(driver->securityManager) < 0)
+ goto cleanup;
+
+ if (virSecurityManagerSetDiskLabel(driver->securityManager,
vm->def,
- disk);
+ disk) < 0)
+ goto cleanup;
+
+ if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
+ virSecurityManagerTransactionCommit(driver->securityManager,
+ vm->pid) < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ virSecurityManagerTransactionAbort(driver->securityManager);
+ return ret;
}
@@ -106,14 +118,26 @@ qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainDiskDefPtr disk)
{
- if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) {
- /* Already handled by namespace code. */
- return 0;
- }
-
- return virSecurityManagerRestoreDiskLabel(driver->securityManager,
- vm->def,
- disk);
+ int ret = -1;
+
+ if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
+ virSecurityManagerTransactionStart(driver->securityManager) < 0)
+ goto cleanup;
+
+ if (virSecurityManagerRestoreDiskLabel(driver->securityManager,
+ vm->def,
+ disk) < 0)
+ goto cleanup;
+
+ if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
+ virSecurityManagerTransactionCommit(driver->securityManager,
+ vm->pid) < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ virSecurityManagerTransactionAbort(driver->securityManager);
+ return ret;
}
@@ -122,15 +146,27 @@ qemuSecuritySetHostdevLabel(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainHostdevDefPtr hostdev)
{
- if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) {
- /* Already handled by namespace code. */
- return 0;
- }
-
- return virSecurityManagerSetHostdevLabel(driver->securityManager,
- vm->def,
- hostdev,
- NULL);
+ int ret = -1;
+
+ if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
+ virSecurityManagerTransactionStart(driver->securityManager) < 0)
+ goto cleanup;
+
+ if (virSecurityManagerSetHostdevLabel(driver->securityManager,
+ vm->def,
+ hostdev,
+ NULL) < 0)
+ goto cleanup;
+
+ if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
+ virSecurityManagerTransactionCommit(driver->securityManager,
+ vm->pid) < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ virSecurityManagerTransactionAbort(driver->securityManager);
+ return ret;
}
@@ -139,13 +175,25 @@ qemuSecurityRestoreHostdevLabel(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainHostdevDefPtr hostdev)
{
- if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) {
- /* Already handled by namespace code. */
- return 0;
- }
-
- return virSecurityManagerRestoreHostdevLabel(driver->securityManager,
- vm->def,
- hostdev,
- NULL);
+ int ret = -1;
+
+ if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
+ virSecurityManagerTransactionStart(driver->securityManager) < 0)
+ goto cleanup;
+
+ if (virSecurityManagerRestoreHostdevLabel(driver->securityManager,
+ vm->def,
+ hostdev,
+ NULL) < 0)
+ goto cleanup;
+
+ if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
+ virSecurityManagerTransactionCommit(driver->securityManager,
+ vm->pid) < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ virSecurityManagerTransactionAbort(driver->securityManager);
+ return ret;
}
--
2.11.0
9:03 a.m.
New subject: [libvirt] [PATCH 05/10] qemu_security: Use more transactions
On Fri, Jan 20, 2017 at 10:42:45AM +0100, Michal Privoznik wrote:
The idea is to move all the seclabel setting to security driver.
Having the relabel code spread all over the place looks very
messy.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_security.c | 112 +++++++++++++++++++++++++++++++++--------------
1 file changed, 80 insertions(+), 32 deletions(-)
diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index 13d99cdbd..9d1a87971 100644
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -90,14 +90,26 @@ qemuSecuritySetDiskLabel(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainDiskDefPtr disk)
{
- if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) {
- /* Already handled by namespace code. */
- return 0;
- }
+ int ret = -1;
- return virSecurityManagerSetDiskLabel(driver->securityManager,
+ if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
+ virSecurityManagerTransactionStart(driver->securityManager) < 0)
+ goto cleanup;
+
+ if (virSecurityManagerSetDiskLabel(driver->securityManager,
vm->def,
- disk);
+ disk) < 0)
indentation
+ goto cleanup;
+
+ if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
+ virSecurityManagerTransactionCommit(driver->securityManager,
+ vm->pid) < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ virSecurityManagerTransactionAbort(driver->securityManager);
+ return ret;
}
So much code duplication. Wasn't there an idea to have a callback in
the security manager that would be called before/after the labelling?
Since this is only for a disk and hostdev, let's leave it like this, I
guess, but I'm expecting this much code duplication to bite us back in a
while. None of my ideas for how to make this better are finalized, but
I have some, if you want to discuss.
ACK for the sake of fixing stuff if you fix the indentation as well.
4:02 a.m.
New subject: [libvirt] [PATCH 05/10] qemu_security: Use more transactions
On 02/02/2017 04:03 PM, Martin Kletzander wrote:
On Fri, Jan 20, 2017 at 10:42:45AM +0100, Michal Privoznik wrote:
> The idea is to move all the seclabel setting to security driver.
> Having the relabel code spread all over the place looks very
> messy.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/qemu/qemu_security.c | 112
> +++++++++++++++++++++++++++++++++--------------
> 1 file changed, 80 insertions(+), 32 deletions(-)
>
> diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
> index 13d99cdbd..9d1a87971 100644
> --- a/src/qemu/qemu_security.c
> +++ b/src/qemu/qemu_security.c
> @@ -90,14 +90,26 @@ qemuSecuritySetDiskLabel(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> virDomainDiskDefPtr disk)
> {
> - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) {
> - /* Already handled by namespace code. */
> - return 0;
> - }
> + int ret = -1;
>
> - return virSecurityManagerSetDiskLabel(driver->securityManager,
> + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
> + virSecurityManagerTransactionStart(driver->securityManager) < 0)
> + goto cleanup;
> +
> + if (virSecurityManagerSetDiskLabel(driver->securityManager,
> vm->def,
> - disk);
> + disk) < 0)
indentation
> + goto cleanup;
> +
> + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
> + virSecurityManagerTransactionCommit(driver->securityManager,
> + vm->pid) < 0)
> + goto cleanup;
> +
> + ret = 0;
> + cleanup:
> + virSecurityManagerTransactionAbort(driver->securityManager);
> + return ret;
> }
>
So much code duplication.
I have a patch ready that kills that and replaces it with a macro.
Wasn't there an idea to have a callback in
the security manager that would be called before/after the labelling?
The problem is that security driver sees just virDomainDef while all the
namespace stuff (e.g. pid, enabled namespace bitmap) lives in a domain
object. Therefore security driver can't make such decision on its own.
Thus transactions were born.
Having a chown callback that would enter the namespace and change
ownership proved to be very suboptimal: entering a namespace requires a
fork(). Therefore, instead of forking like crazy, a list of all the
desired chown()-s is constructed, one fork() is called and then all the
operations from the list are performed.
Since this is only for a disk and hostdev, let's leave it like
this, I
guess, but I'm expecting this much code duplication to bite us back in a
while. None of my ideas for how to make this better are finalized, but
I have some, if you want to discuss.
Sure. I'm up for making this better.
Michal
5:05 a.m.
New subject: [libvirt] [PATCH 05/10] qemu_security: Use more transactions
On Tue, Feb 07, 2017 at 11:02:09AM +0100, Michal Privoznik wrote:
On 02/02/2017 04:03 PM, Martin Kletzander wrote:
> On Fri, Jan 20, 2017 at 10:42:45AM +0100, Michal Privoznik wrote:
>> The idea is to move all the seclabel setting to security driver.
>> Having the relabel code spread all over the place looks very
>> messy.
>>
>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>> ---
>> src/qemu/qemu_security.c | 112
>> +++++++++++++++++++++++++++++++++--------------
>> 1 file changed, 80 insertions(+), 32 deletions(-)
>>
>> diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
>> index 13d99cdbd..9d1a87971 100644
>> --- a/src/qemu/qemu_security.c
>> +++ b/src/qemu/qemu_security.c
>> @@ -90,14 +90,26 @@ qemuSecuritySetDiskLabel(virQEMUDriverPtr driver,
>> virDomainObjPtr vm,
>> virDomainDiskDefPtr disk)
>> {
>> - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) {
>> - /* Already handled by namespace code. */
>> - return 0;
>> - }
>> + int ret = -1;
>>
>> - return virSecurityManagerSetDiskLabel(driver->securityManager,
>> + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>> + virSecurityManagerTransactionStart(driver->securityManager) < 0)
>> + goto cleanup;
>> +
>> + if (virSecurityManagerSetDiskLabel(driver->securityManager,
>> vm->def,
>> - disk);
>> + disk) < 0)
>
> indentation
>
>> + goto cleanup;
>> +
>> + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>> + virSecurityManagerTransactionCommit(driver->securityManager,
>> + vm->pid) < 0)
>> + goto cleanup;
>> +
>> + ret = 0;
>> + cleanup:
>> + virSecurityManagerTransactionAbort(driver->securityManager);
>> + return ret;
>> }
>>
>
> So much code duplication.
I have a patch ready that kills that and replaces it with a macro.
> Wasn't there an idea to have a callback in
> the security manager that would be called before/after the labelling?
The problem is that security driver sees just virDomainDef while all the
namespace stuff (e.g. pid, enabled namespace bitmap) lives in a domain
object. Therefore security driver can't make such decision on its own.
Thus transactions were born.
Having a chown callback that would enter the namespace and change
ownership proved to be very suboptimal: entering a namespace requires a
fork(). Therefore, instead of forking like crazy, a list of all the
desired chown()-s is constructed, one fork() is called and then all the
operations from the list are performed.
> Since this is only for a disk and hostdev, let's leave it like this, I
> guess, but I'm expecting this much code duplication to bite us back in a
> while. None of my ideas for how to make this better are finalized, but
> I have some, if you want to discuss.
Sure. I'm up for making this better.
When thinking about them after a while none of them are very clean.
Let's see how it looks with your macros for now.
Michal
3:42 a.m.
New subject: [libvirt] [PATCH 06/10] qemuDomain{Attach, Detach}Device NS helpers: Don't relabel devices
After previous commit this has become redundant step.
Also setting up devices in namespace and setting their label
later on are two different steps and should be not done at once.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_domain.c | 112 +------------------------------------------------
1 file changed, 2 insertions(+), 110 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c67604222..0f45f753e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7565,58 +7565,6 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED,
}
#endif
- switch ((virDomainDeviceType) data->devDef->type) {
- case VIR_DOMAIN_DEVICE_DISK: {
- virDomainDiskDefPtr def = data->devDef->data.disk;
- char *tmpsrc = def->src->path;
- def->src->path = (char *) data->file;
- if (virSecurityManagerSetDiskLabel(data->driver->securityManager,
- data->vm->def, def) < 0) {
- def->src->path = tmpsrc;
- goto cleanup;
- }
- def->src->path = tmpsrc;
- } break;
-
- case VIR_DOMAIN_DEVICE_HOSTDEV: {
- virDomainHostdevDefPtr def = data->devDef->data.hostdev;
- if (virSecurityManagerSetHostdevLabel(data->driver->securityManager,
- data->vm->def, def, NULL) < 0)
- goto cleanup;
- } break;
-
- case VIR_DOMAIN_DEVICE_CHR:
- case VIR_DOMAIN_DEVICE_RNG:
- /* No labelling. */
- break;
-
- case VIR_DOMAIN_DEVICE_NONE:
- case VIR_DOMAIN_DEVICE_LEASE:
- case VIR_DOMAIN_DEVICE_FS:
- case VIR_DOMAIN_DEVICE_NET:
- case VIR_DOMAIN_DEVICE_INPUT:
- case VIR_DOMAIN_DEVICE_SOUND:
- case VIR_DOMAIN_DEVICE_VIDEO:
- case VIR_DOMAIN_DEVICE_WATCHDOG:
- case VIR_DOMAIN_DEVICE_CONTROLLER:
- case VIR_DOMAIN_DEVICE_GRAPHICS:
- case VIR_DOMAIN_DEVICE_HUB:
- case VIR_DOMAIN_DEVICE_REDIRDEV:
- case VIR_DOMAIN_DEVICE_SMARTCARD:
- case VIR_DOMAIN_DEVICE_MEMBALLOON:
- case VIR_DOMAIN_DEVICE_NVRAM:
- case VIR_DOMAIN_DEVICE_SHMEM:
- case VIR_DOMAIN_DEVICE_TPM:
- case VIR_DOMAIN_DEVICE_PANIC:
- case VIR_DOMAIN_DEVICE_MEMORY:
- case VIR_DOMAIN_DEVICE_IOMMU:
- case VIR_DOMAIN_DEVICE_LAST:
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unexpected device type %d"),
- data->devDef->type);
- goto cleanup;
- }
-
ret = 0;
cleanup:
if (ret < 0 && delDevice)
@@ -7707,67 +7655,11 @@ qemuDomainDetachDeviceUnlinkHelper(pid_t pid ATTRIBUTE_UNUSED,
static int
-qemuDomainDetachDeviceUnlink(virQEMUDriverPtr driver,
+qemuDomainDetachDeviceUnlink(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
virDomainObjPtr vm,
- virDomainDeviceDefPtr dev,
+ virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED,
const char *file)
{
- /* Technically, this is not needed. Yet. But in the future
- * security managers might do some reference counting over
- * Set/Restore label and thus for every SetLabel() there
- * should be corresponding RestoreLabel(). */
- switch ((virDomainDeviceType) dev->type) {
- case VIR_DOMAIN_DEVICE_DISK: {
- virDomainDiskDefPtr def = dev->data.disk;
- char *tmpsrc = def->src->path;
- def->src->path = (char *) file;
- if (virSecurityManagerRestoreDiskLabel(driver->securityManager,
- vm->def, def) < 0) {
- def->src->path = tmpsrc;
- return -1;
- }
- def->src->path = tmpsrc;
- } break;
-
- case VIR_DOMAIN_DEVICE_HOSTDEV: {
- virDomainHostdevDefPtr def = dev->data.hostdev;
- if (virSecurityManagerRestoreHostdevLabel(driver->securityManager,
- vm->def, def, NULL) < 0)
- return -1;
- } break;
-
- case VIR_DOMAIN_DEVICE_CHR:
- case VIR_DOMAIN_DEVICE_RNG:
- /* No labelling. */
- break;
-
- case VIR_DOMAIN_DEVICE_NONE:
- case VIR_DOMAIN_DEVICE_LEASE:
- case VIR_DOMAIN_DEVICE_FS:
- case VIR_DOMAIN_DEVICE_NET:
- case VIR_DOMAIN_DEVICE_INPUT:
- case VIR_DOMAIN_DEVICE_SOUND:
- case VIR_DOMAIN_DEVICE_VIDEO:
- case VIR_DOMAIN_DEVICE_WATCHDOG:
- case VIR_DOMAIN_DEVICE_CONTROLLER:
- case VIR_DOMAIN_DEVICE_GRAPHICS:
- case VIR_DOMAIN_DEVICE_HUB:
- case VIR_DOMAIN_DEVICE_REDIRDEV:
- case VIR_DOMAIN_DEVICE_SMARTCARD:
- case VIR_DOMAIN_DEVICE_MEMBALLOON:
- case VIR_DOMAIN_DEVICE_NVRAM:
- case VIR_DOMAIN_DEVICE_SHMEM:
- case VIR_DOMAIN_DEVICE_TPM:
- case VIR_DOMAIN_DEVICE_PANIC:
- case VIR_DOMAIN_DEVICE_MEMORY:
- case VIR_DOMAIN_DEVICE_IOMMU:
- case VIR_DOMAIN_DEVICE_LAST:
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unexpected device type %d"),
- dev->type);
- return -1;
- }
-
if (virProcessRunInMountNamespace(vm->pid,
qemuDomainDetachDeviceUnlinkHelper,
(void *)file) < 0)
--
2.11.0
3:59 a.m.
New subject: [libvirt] [PATCH 06/10] qemuDomain{Attach, Detach}Device NS helpers: Don't relabel devices
On Fri, Jan 20, 2017 at 10:42:46AM +0100, Michal Privoznik wrote:
After previous commit this has become redundant step.
Also setting up devices in namespace and setting their label
later on are two different steps and should be not done at once.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_domain.c | 112 +------------------------------------------------
1 file changed, 2 insertions(+), 110 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c67604222..0f45f753e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7707,67 +7655,11 @@ qemuDomainDetachDeviceUnlinkHelper(pid_t pid ATTRIBUTE_UNUSED,
static int
-qemuDomainDetachDeviceUnlink(virQEMUDriverPtr driver,
+qemuDomainDetachDeviceUnlink(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
virDomainObjPtr vm,
- virDomainDeviceDefPtr dev,
+ virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED,
Any reason for keeping these unused attributes here? Your call on
removing them (from caller as well, of course). ACK either way.
4:07 a.m.
New subject: [libvirt] [PATCH 06/10] qemuDomain{Attach, Detach}Device NS helpers: Don't relabel devices
On 02/07/2017 10:59 AM, Martin Kletzander wrote:
On Fri, Jan 20, 2017 at 10:42:46AM +0100, Michal Privoznik wrote:
> After previous commit this has become redundant step.
> Also setting up devices in namespace and setting their label
> later on are two different steps and should be not done at once.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/qemu/qemu_domain.c | 112
> +------------------------------------------------
> 1 file changed, 2 insertions(+), 110 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index c67604222..0f45f753e 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -7707,67 +7655,11 @@ qemuDomainDetachDeviceUnlinkHelper(pid_t pid
> ATTRIBUTE_UNUSED,
>
>
> static int
> -qemuDomainDetachDeviceUnlink(virQEMUDriverPtr driver,
> +qemuDomainDetachDeviceUnlink(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
> virDomainObjPtr vm,
> - virDomainDeviceDefPtr dev,
> + virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED,
Any reason for keeping these unused attributes here? Your call on
removing them (from caller as well, of course). ACK either way.
As mentioned in previous e-mail, I have another patch set ready that
goes on the top of this and removes couple of unused variables/arguments
(mostly because I'm fixing another bug by calling
qemuDomainNamespaceSetupDisk() from a place where just
virStorageSourcePtr is available. So stay tuned :-)
Michal
3:42 a.m.
New subject: [libvirt] [PATCH 07/10] qemuDomainCreateDevice: Properly deal with symlinks
Imagine you have a disk with the following source set up:
/dev/disk/by-uuid/$uuid (symlink to) -> /dev/sda
After cbc45525cb21 the transitive end of the symlink chain is
created (/dev/sda), but we need to create any item in chain too.
Others might rely on that.
In this case, /dev/disk/by-uuid/$uuid comes from domain XML thus
it is this path that secdriver tries to relabel. Not the resolved
one.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_domain.c | 150 +++++++++++++++++++++++++++++++++++--------------
1 file changed, 108 insertions(+), 42 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 0f45f753e..8cbfb2d16 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -68,6 +68,7 @@
#endif
#include <libxml/xpathInternals.h>
+#include "dosname.h"
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -6958,70 +6959,135 @@ qemuDomainCreateDevice(const char *device,
bool allow_noent)
{
char *devicePath = NULL;
- char *canonDevicePath = NULL;
+ char *target = NULL;
struct stat sb;
int ret = -1;
+ bool isLink = false;
+ bool create = false;
#ifdef WITH_SELINUX
char *tcon = NULL;
#endif
- if (virFileResolveAllLinks(device, &canonDevicePath) < 0) {
+ if (lstat(device, &sb) < 0) {
if (errno == ENOENT && allow_noent) {
/* Ignore non-existent device. */
- ret = 0;
- goto cleanup;
+ return 0;
}
-
- virReportError(errno, _("Unable to canonicalize %s"), device);
- goto cleanup;
- }
-
- if (!STRPREFIX(canonDevicePath, DEVPREFIX)) {
- ret = 0;
- goto cleanup;
+ virReportSystemError(errno, _("Unable to stat %s"), device);
+ return ret;
}
- if (virAsprintf(&devicePath, "%s/%s",
- path, canonDevicePath + strlen(DEVPREFIX)) < 0)
- goto cleanup;
+ isLink = S_ISLNK(sb.st_mode);
- if (stat(canonDevicePath, &sb) < 0) {
- if (errno == ENOENT && allow_noent) {
- /* Ignore non-existent device. */
- ret = 0;
+ /* Here, @device might be whatever path in the system. We
+ * should create the path in the namespace iff its "/dev"
+ * prefixed. However, if it is a symlink, we need to traverse
+ * it too (it might point to something in "/dev"). Just
+ * consider:
+ *
+ * /var/sym1 -> /var/sym2 -> /dev/sda (because users can)
+ *
+ * This means, "/var/sym1" is not created (it's shared with
+ * the parent namespace), nor "/var/sym2", but "/dev/sda".
+ *
+ * TODO Remove all `.' and `..' from the @device path.
+ * Otherwise we might get fooled with `/dev/../var/my_image'.
+ * For now, lets hope callers play nice.
+ */
+ if (STRPREFIX(device, DEVPREFIX)) {
+ if (virAsprintf(&devicePath, "%s/%s",
+ path, device + strlen(DEVPREFIX)) < 0)
goto cleanup;
- }
-
- virReportSystemError(errno, _("Unable to stat %s"), canonDevicePath);
- goto cleanup;
- }
-
- if (virFileMakeParentPath(devicePath) < 0) {
- virReportSystemError(errno,
- _("Unable to create %s"),
- devicePath);
- goto cleanup;
- }
- if (mknod(devicePath, sb.st_mode, sb.st_rdev) < 0) {
- if (errno == EEXIST) {
- ret = 0;
- } else {
+ if (virFileMakeParentPath(devicePath) < 0) {
virReportSystemError(errno,
- _("Failed to make device %s"),
+ _("Unable to create %s"),
devicePath);
+ goto cleanup;
}
+ create = true;
+ }
+
+ if (isLink) {
+ /* We are dealing with a symlink. Create a dangling symlink and descend
+ * down one level which hopefully creates the symlink's target. */
+ if (virFileReadLink(device, &target) < 0) {
+ virReportSystemError(errno,
+ _("unable to resolve symlink %s"),
+ device);
+ goto cleanup;
+ }
+
+ if (create &&
+ symlink(target, devicePath) < 0) {
+ if (errno == EEXIST) {
+ ret = 0;
+ } else {
+ virReportSystemError(errno,
+ _("unable to create symlink %s"),
+ devicePath);
+ }
+ goto cleanup;
+ }
+
+ /* Tricky part. If the target starts with a slash then we need to take
+ * it as it is. Otherwise we need to replace the last component in the
+ * original path with the link target:
+ * /dev/rtc -> rtc0 (want /dev/rtc0)
+ * /dev/disk/by-id/ata-SanDisk_SDSSDXPS480G_161101402485 -> ../../sda
+ * (want /dev/disk/by-id/../../sda)
+ * /dev/stdout -> /proc/self/fd/1 (no change needed)
+ */
+ if (IS_RELATIVE_FILE_NAME(target)) {
+ char *c = NULL, *tmp = NULL, *devTmp = NULL;
+
+ if (VIR_STRDUP(devTmp, device) < 0)
+ goto cleanup;
+
+ if ((c = strrchr(devTmp, '/')))
+ *(c + 1) = '\0';
+
+ if (virAsprintf(&tmp, "%s%s", devTmp, target) < 0) {
+ VIR_FREE(devTmp);
+ goto cleanup;
+ }
+ VIR_FREE(devTmp);
+ VIR_FREE(target);
+ target = tmp;
+ tmp = NULL;
+ }
+
+ if (qemuDomainCreateDevice(target, path, allow_noent) < 0)
+ goto cleanup;
+ } else {
+ if (create &&
+ mknod(devicePath, sb.st_mode, sb.st_rdev) < 0) {
+ if (errno == EEXIST) {
+ ret = 0;
+ } else {
+ virReportSystemError(errno,
+ _("Failed to make device %s"),
+ devicePath);
+ }
+ goto cleanup;
+ }
+ }
+
+ if (!create) {
+ ret = 0;
goto cleanup;
}
- if (chown(devicePath, sb.st_uid, sb.st_gid) < 0) {
+ if (lchown(devicePath, sb.st_uid, sb.st_gid) < 0) {
virReportSystemError(errno,
_("Failed to chown device %s"),
devicePath);
goto cleanup;
}
- if (virFileCopyACLs(canonDevicePath, devicePath) < 0 &&
+ /* Symlinks don't have ACLs. */
+ if (!isLink &&
+ virFileCopyACLs(device, devicePath) < 0 &&
errno != ENOTSUP) {
virReportSystemError(errno,
_("Failed to copy ACLs on device %s"),
@@ -7030,16 +7096,16 @@ qemuDomainCreateDevice(const char *device,
}
#ifdef WITH_SELINUX
- if (getfilecon_raw(canonDevicePath, &tcon) < 0 &&
+ if (lgetfilecon_raw(device, &tcon) < 0 &&
(errno != ENOTSUP && errno != ENODATA)) {
virReportSystemError(errno,
_("Unable to get SELinux label from %s"),
- canonDevicePath);
+ device);
goto cleanup;
}
if (tcon &&
- setfilecon_raw(devicePath, (VIR_SELINUX_CTX_CONST char *) tcon) < 0) {
+ lsetfilecon_raw(devicePath, (VIR_SELINUX_CTX_CONST char *) tcon) < 0) {
VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR
if (errno != EOPNOTSUPP && errno != ENOTSUP) {
VIR_WARNINGS_RESET
@@ -7053,7 +7119,7 @@ qemuDomainCreateDevice(const char *device,
ret = 0;
cleanup:
- VIR_FREE(canonDevicePath);
+ VIR_FREE(target);
VIR_FREE(devicePath);
#ifdef WITH_SELINUX
freecon(tcon);
--
2.11.0
4:18 a.m.
New subject: [libvirt] [PATCH 07/10] qemuDomainCreateDevice: Properly deal with symlinks
On Fri, Jan 20, 2017 at 10:42:47AM +0100, Michal Privoznik wrote:
Imagine you have a disk with the following source set up:
/dev/disk/by-uuid/$uuid (symlink to) -> /dev/sda
After cbc45525cb21 the transitive end of the symlink chain is
created (/dev/sda), but we need to create any item in chain too.
Others might rely on that.
In this case, /dev/disk/by-uuid/$uuid comes from domain XML thus
it is this path that secdriver tries to relabel. Not the resolved
one.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_domain.c | 150 +++++++++++++++++++++++++++++++++++--------------
1 file changed, 108 insertions(+), 42 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 0f45f753e..8cbfb2d16 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -68,6 +68,7 @@
#endif
#include <libxml/xpathInternals.h>
+#include "dosname.h"
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -6958,70 +6959,135 @@ qemuDomainCreateDevice(const char *device,
bool allow_noent)
{
char *devicePath = NULL;
- char *canonDevicePath = NULL;
+ char *target = NULL;
struct stat sb;
int ret = -1;
+ bool isLink = false;
+ bool create = false;
#ifdef WITH_SELINUX
char *tcon = NULL;
#endif
- if (virFileResolveAllLinks(device, &canonDevicePath) < 0) {
+ if (lstat(device, &sb) < 0) {
if (errno == ENOENT && allow_noent) {
/* Ignore non-existent device. */
- ret = 0;
- goto cleanup;
+ return 0;
}
-
- virReportError(errno, _("Unable to canonicalize %s"), device);
- goto cleanup;
- }
-
- if (!STRPREFIX(canonDevicePath, DEVPREFIX)) {
- ret = 0;
- goto cleanup;
+ virReportSystemError(errno, _("Unable to stat %s"), device);
+ return ret;
}
- if (virAsprintf(&devicePath, "%s/%s",
- path, canonDevicePath + strlen(DEVPREFIX)) < 0)
- goto cleanup;
+ isLink = S_ISLNK(sb.st_mode);
- if (stat(canonDevicePath, &sb) < 0) {
- if (errno == ENOENT && allow_noent) {
- /* Ignore non-existent device. */
- ret = 0;
+ /* Here, @device might be whatever path in the system. We
+ * should create the path in the namespace iff its "/dev"
s/its/it's/
ACK
3:42 a.m.
New subject: [libvirt] [PATCH 08/10] qemuDomainCreateDevice: Don't loop endlessly
When working with symlinks it is fairly easy to get into a loop.
Don't.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_domain.c | 28 ++++++++++++++++++++++++----
1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 8cbfb2d16..448583313 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6954,9 +6954,10 @@ qemuDomainGetPreservedMounts(virQEMUDriverPtr driver,
static int
-qemuDomainCreateDevice(const char *device,
- const char *path,
- bool allow_noent)
+qemuDomainCreateDeviceRecursive(const char *device,
+ const char *path,
+ bool allow_noent,
+ unsigned int ttl)
{
char *devicePath = NULL;
char *target = NULL;
@@ -6968,6 +6969,13 @@ qemuDomainCreateDevice(const char *device,
char *tcon = NULL;
#endif
+ if (!ttl) {
+ virReportSystemError(ELOOP,
+ _("Too many levels of symbolic links: %s"),
+ device);
+ return ret;
+ }
+
if (lstat(device, &sb) < 0) {
if (errno == ENOENT && allow_noent) {
/* Ignore non-existent device. */
@@ -7057,7 +7065,8 @@ qemuDomainCreateDevice(const char *device,
tmp = NULL;
}
- if (qemuDomainCreateDevice(target, path, allow_noent) < 0)
+ if (qemuDomainCreateDeviceRecursive(target, path,
+ allow_noent, ttl - 1) < 0)
goto cleanup;
} else {
if (create &&
@@ -7128,6 +7137,17 @@ qemuDomainCreateDevice(const char *device,
}
+static int
+qemuDomainCreateDevice(const char *device,
+ const char *path,
+ bool allow_noent)
+{
+ long symloop_max = sysconf(_SC_SYMLOOP_MAX);
+
+ return qemuDomainCreateDeviceRecursive(device, path,
+ allow_noent, symloop_max);
+}
+
static int
qemuDomainPopulateDevices(virQEMUDriverPtr driver,
--
2.11.0
4:34 a.m.
New subject: [libvirt] [PATCH 08/10] qemuDomainCreateDevice: Don't loop endlessly
On Fri, Jan 20, 2017 at 10:42:48AM +0100, Michal Privoznik wrote:
When working with symlinks it is fairly easy to get into a loop.
Don't.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_domain.c | 28 ++++++++++++++++++++++++----
1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 8cbfb2d16..448583313 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6954,9 +6954,10 @@ qemuDomainGetPreservedMounts(virQEMUDriverPtr driver,
static int
-qemuDomainCreateDevice(const char *device,
- const char *path,
- bool allow_noent)
+qemuDomainCreateDeviceRecursive(const char *device,
+ const char *path,
+ bool allow_noent,
+ unsigned int ttl)
{
char *devicePath = NULL;
char *target = NULL;
@@ -6968,6 +6969,13 @@ qemuDomainCreateDevice(const char *device,
char *tcon = NULL;
#endif
+ if (!ttl) {
+ virReportSystemError(ELOOP,
+ _("Too many levels of symbolic links: %s"),
+ device);
+ return ret;
+ }
+
if (lstat(device, &sb) < 0) {
if (errno == ENOENT && allow_noent) {
/* Ignore non-existent device. */
@@ -7057,7 +7065,8 @@ qemuDomainCreateDevice(const char *device,
tmp = NULL;
}
- if (qemuDomainCreateDevice(target, path, allow_noent) < 0)
+ if (qemuDomainCreateDeviceRecursive(target, path,
+ allow_noent, ttl - 1) < 0)
goto cleanup;
} else {
if (create &&
@@ -7128,6 +7137,17 @@ qemuDomainCreateDevice(const char *device,
}
+static int
+qemuDomainCreateDevice(const char *device,
+ const char *path,
+ bool allow_noent)
+{
+ long symloop_max = sysconf(_SC_SYMLOOP_MAX);
+
+ return qemuDomainCreateDeviceRecursive(device, path,
+ allow_noent, symloop_max);
So you are taking 'long' that can, officially, be '-1' and pass it to a
function as unsigned int. Having a maximum is nice, I have no idea why
on my system there is no limit apparently (sysconf() returns -1 with no
errno set), but if the system doesn't report any (like my case above) it
effectively makes the SYMLOOP_MAX = UINT_MAX in this codepath. Should
we avoid that or just let it be? This way it's still better than
unlimited, so ACK to this version, but I just wanted a (short)
discussion about this.
+}
+
static int
qemuDomainPopulateDevices(virQEMUDriverPtr driver,
--
2.11.0
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
5:20 a.m.
New subject: [libvirt] [PATCH 08/10] qemuDomainCreateDevice: Don't loop endlessly
On 02/07/2017 11:34 AM, Martin Kletzander wrote:
On Fri, Jan 20, 2017 at 10:42:48AM +0100, Michal Privoznik wrote:
> When working with symlinks it is fairly easy to get into a loop.
> Don't.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/qemu/qemu_domain.c | 28 ++++++++++++++++++++++++----
> 1 file changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 8cbfb2d16..448583313 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -6954,9 +6954,10 @@ qemuDomainGetPreservedMounts(virQEMUDriverPtr
> driver,
>
>
> static int
> -qemuDomainCreateDevice(const char *device,
> - const char *path,
> - bool allow_noent)
> +qemuDomainCreateDeviceRecursive(const char *device,
> + const char *path,
> + bool allow_noent,
> + unsigned int ttl)
> {
> char *devicePath = NULL;
> char *target = NULL;
> @@ -6968,6 +6969,13 @@ qemuDomainCreateDevice(const char *device,
> char *tcon = NULL;
> #endif
>
> + if (!ttl) {
> + virReportSystemError(ELOOP,
> + _("Too many levels of symbolic links: %s"),
> + device);
> + return ret;
> + }
> +
> if (lstat(device, &sb) < 0) {
> if (errno == ENOENT && allow_noent) {
> /* Ignore non-existent device. */
> @@ -7057,7 +7065,8 @@ qemuDomainCreateDevice(const char *device,
> tmp = NULL;
> }
>
> - if (qemuDomainCreateDevice(target, path, allow_noent) < 0)
> + if (qemuDomainCreateDeviceRecursive(target, path,
> + allow_noent, ttl - 1) < 0)
> goto cleanup;
> } else {
> if (create &&
> @@ -7128,6 +7137,17 @@ qemuDomainCreateDevice(const char *device,
> }
>
>
> +static int
> +qemuDomainCreateDevice(const char *device,
> + const char *path,
> + bool allow_noent)
> +{
> + long symloop_max = sysconf(_SC_SYMLOOP_MAX);
> +
> + return qemuDomainCreateDeviceRecursive(device, path,
> + allow_noent, symloop_max);
So you are taking 'long' that can, officially, be '-1' and pass it to a
function as unsigned int. Having a maximum is nice, I have no idea why
on my system there is no limit apparently (sysconf() returns -1 with no
errno set), but if the system doesn't report any (like my case above) it
effectively makes the SYMLOOP_MAX = UINT_MAX in this codepath. Should
we avoid that or just let it be? This way it's still better than
unlimited, so ACK to this version, but I just wanted a (short)
discussion about this.
Sure. I think it's safe to assume if the level of symlinks reaches
UINT_MAX your system is terribly broken.
Michal
3:42 a.m.
New subject: [libvirt] [PATCH 09/10] qemuDomainAttachDeviceMknod: Deal with symlinks
Similarly to one of the previous commits, we need to deal
properly with symlinks in hotplug case too.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_domain.c | 120 ++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 94 insertions(+), 26 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 448583313..bcfb2446f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7590,6 +7590,7 @@ struct qemuDomainAttachDeviceMknodData {
virDomainObjPtr vm;
virDomainDeviceDefPtr devDef;
const char *file;
+ const char *target;
struct stat sb;
void *acl;
#ifdef WITH_SELINUX
@@ -7605,6 +7606,7 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED,
struct qemuDomainAttachDeviceMknodData *data = opaque;
int ret = -1;
bool delDevice = false;
+ bool isLink = S_ISLNK(data->sb.st_mode);
virSecurityManagerPostFork(data->driver->securityManager);
@@ -7614,24 +7616,47 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED,
goto cleanup;
}
- VIR_DEBUG("Creating dev %s (%d,%d)",
- data->file, major(data->sb.st_rdev), minor(data->sb.st_rdev));
- if (mknod(data->file, data->sb.st_mode, data->sb.st_rdev) < 0) {
- /* Because we are not removing devices on hotunplug, or
- * we might be creating part of backing chain that
- * already exist due to a different disk plugged to
- * domain, accept EEXIST. */
- if (errno != EEXIST) {
- virReportSystemError(errno,
- _("Unable to create device %s"),
- data->file);
- goto cleanup;
+ if (isLink) {
+ VIR_DEBUG("Creating symlink %s -> %s", data->file,
data->target);
+ if (symlink(data->target, data->file) < 0) {
+ if (errno != EEXIST) {
+ virReportSystemError(errno,
+ _("Unable to create symlink %s"),
+ data->target);
+ goto cleanup;
+ }
+ } else {
+ delDevice = true;
}
} else {
- delDevice = true;
+ VIR_DEBUG("Creating dev %s (%d,%d)",
+ data->file, major(data->sb.st_rdev),
minor(data->sb.st_rdev));
+ if (mknod(data->file, data->sb.st_mode, data->sb.st_rdev) < 0) {
+ /* Because we are not removing devices on hotunplug, or
+ * we might be creating part of backing chain that
+ * already exist due to a different disk plugged to
+ * domain, accept EEXIST. */
+ if (errno != EEXIST) {
+ virReportSystemError(errno,
+ _("Unable to create device %s"),
+ data->file);
+ goto cleanup;
+ }
+ } else {
+ delDevice = true;
+ }
}
- if (virFileSetACLs(data->file, data->acl) < 0 &&
+ if (lchown(data->file, data->sb.st_uid, data->sb.st_gid) < 0) {
+ virReportSystemError(errno,
+ _("Failed to chown device %s"),
+ data->file);
+ goto cleanup;
+ }
+
+ /* Symlinks don't have ACLs. */
+ if (!isLink &&
+ virFileSetACLs(data->file, data->acl) < 0 &&
errno != ENOTSUP) {
virReportSystemError(errno,
_("Unable to set ACLs on %s"), data->file);
@@ -7639,7 +7664,8 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED,
}
#ifdef WITH_SELINUX
- if (setfilecon_raw(data->file, (VIR_SELINUX_CTX_CONST char *) data->tcon) <
0) {
+ if (data->tcon &&
+ lsetfilecon_raw(data->file, (VIR_SELINUX_CTX_CONST char *) data->tcon) <
0) {
VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR
if (errno != EOPNOTSUPP && errno != ENOTSUP) {
VIR_WARNINGS_RESET
@@ -7671,6 +7697,8 @@ qemuDomainAttachDeviceMknod(virQEMUDriverPtr driver,
{
struct qemuDomainAttachDeviceMknodData data;
int ret = -1;
+ char *target = NULL;
+ bool isLink;
memset(&data, 0, sizeof(data));
@@ -7679,21 +7707,55 @@ qemuDomainAttachDeviceMknod(virQEMUDriverPtr driver,
data.devDef = devDef;
data.file = file;
- if (stat(file, &data.sb) < 0) {
+ if (lstat(file, &data.sb) < 0) {
virReportSystemError(errno,
_("Unable to access %s"), file);
return ret;
}
- if (virFileGetACLs(file, &data.acl) < 0 &&
+ isLink = S_ISLNK(data.sb.st_mode);
+
+ if (isLink) {
+ if (virFileReadLink(file, &target) < 0) {
+ virReportSystemError(errno,
+ _("unable to resolve symlink %s"),
+ file);
+ return ret;
+ }
+
+ if (IS_RELATIVE_FILE_NAME(target)) {
+ char *c = NULL, *tmp = NULL, *fileTmp = NULL;
+
+ if (VIR_STRDUP(fileTmp, file) < 0)
+ goto cleanup;
+
+ if ((c = strrchr(fileTmp, '/')))
+ *(c + 1) = '\0';
+
+ if (virAsprintf(&tmp, "%s%s", fileTmp, target) < 0) {
+ VIR_FREE(fileTmp);
+ goto cleanup;
+ }
+ VIR_FREE(fileTmp);
+ VIR_FREE(target);
+ target = tmp;
+ tmp = NULL;
+ }
+
+ data.target = target;
+ }
+
+ /* Symlinks don't have ACLs. */
+ if (!isLink &&
+ virFileGetACLs(file, &data.acl) < 0 &&
errno != ENOTSUP) {
virReportSystemError(errno,
_("Unable to get ACLs on %s"), file);
- return ret;
+ goto cleanup;
}
#ifdef WITH_SELINUX
- if (getfilecon_raw(file, &data.tcon) < 0 &&
+ if (lgetfilecon_raw(file, &data.tcon) < 0 &&
(errno != ENOTSUP && errno != ENODATA)) {
virReportSystemError(errno,
_("Unable to get SELinux label from %s"), file);
@@ -7701,17 +7763,22 @@ qemuDomainAttachDeviceMknod(virQEMUDriverPtr driver,
}
#endif
- if (virSecurityManagerPreFork(driver->securityManager) < 0)
- goto cleanup;
+ if (STRPREFIX(file, DEVPREFIX)) {
+ if (virSecurityManagerPreFork(driver->securityManager) < 0)
+ goto cleanup;
- if (virProcessRunInMountNamespace(vm->pid,
- qemuDomainAttachDeviceMknodHelper,
- &data) < 0) {
+ if (virProcessRunInMountNamespace(vm->pid,
+ qemuDomainAttachDeviceMknodHelper,
+ &data) < 0) {
+ virSecurityManagerPostFork(driver->securityManager);
+ goto cleanup;
+ }
virSecurityManagerPostFork(driver->securityManager);
- goto cleanup;
}
- virSecurityManagerPostFork(driver->securityManager);
+ if (isLink &&
+ qemuDomainAttachDeviceMknod(driver, vm, devDef, target) < 0)
+ goto cleanup;
ret = 0;
cleanup:
@@ -7719,6 +7786,7 @@ qemuDomainAttachDeviceMknod(virQEMUDriverPtr driver,
freecon(data.tcon);
#endif
virFileFreeACLs(&data.acl);
+ VIR_FREE(target);
return ret;
}
--
2.11.0
4:57 a.m.
New subject: [libvirt] [PATCH 09/10] qemuDomainAttachDeviceMknod: Deal with symlinks
On Fri, Jan 20, 2017 at 10:42:49AM +0100, Michal Privoznik wrote:
Similarly to one of the previous commits, we need to deal
properly with symlinks in hotplug case too.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_domain.c | 120 ++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 94 insertions(+), 26 deletions(-)
ACK to this, but ...
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 448583313..bcfb2446f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7701,17 +7763,22 @@ qemuDomainAttachDeviceMknod(virQEMUDriverPtr driver,
}
#endif
- if (virSecurityManagerPreFork(driver->securityManager) < 0)
- goto cleanup;
+ if (STRPREFIX(file, DEVPREFIX)) {
+ if (virSecurityManagerPreFork(driver->securityManager) < 0)
+ goto cleanup;
- if (virProcessRunInMountNamespace(vm->pid,
- qemuDomainAttachDeviceMknodHelper,
- &data) < 0) {
+ if (virProcessRunInMountNamespace(vm->pid,
+ qemuDomainAttachDeviceMknodHelper,
+ &data) < 0) {
... I'm sure you have patches for this somewhere that are not posted or
something =D However now we actually fork for every level of the
symlink. Even when everyone is scared of every single fork(). Can't we
use transactions for this as well? If not, could we enhance them so
that we can use them?
+ virSecurityManagerPostFork(driver->securityManager);
+ goto cleanup;
+ }
virSecurityManagerPostFork(driver->securityManager);
- goto cleanup;
}
- virSecurityManagerPostFork(driver->securityManager);
+ if (isLink &&
+ qemuDomainAttachDeviceMknod(driver, vm, devDef, target) < 0)
+ goto cleanup;
ret = 0;
cleanup:
5:20 a.m.
New subject: [libvirt] [PATCH 09/10] qemuDomainAttachDeviceMknod: Deal with symlinks
On 02/07/2017 11:57 AM, Martin Kletzander wrote:
On Fri, Jan 20, 2017 at 10:42:49AM +0100, Michal Privoznik wrote:
> Similarly to one of the previous commits, we need to deal
> properly with symlinks in hotplug case too.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/qemu/qemu_domain.c | 120
> ++++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 94 insertions(+), 26 deletions(-)
>
ACK to this, but ...
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 448583313..bcfb2446f 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -7701,17 +7763,22 @@ qemuDomainAttachDeviceMknod(virQEMUDriverPtr
> driver,
> }
> #endif
>
> - if (virSecurityManagerPreFork(driver->securityManager) < 0)
> - goto cleanup;
> + if (STRPREFIX(file, DEVPREFIX)) {
> + if (virSecurityManagerPreFork(driver->securityManager) < 0)
> + goto cleanup;
>
> - if (virProcessRunInMountNamespace(vm->pid,
> - qemuDomainAttachDeviceMknodHelper,
> - &data) < 0) {
> + if (virProcessRunInMountNamespace(vm->pid,
> +
> qemuDomainAttachDeviceMknodHelper,
> + &data) < 0) {
... I'm sure you have patches for this somewhere that are not posted or
something =D However now we actually fork for every level of the
symlink. Even when everyone is scared of every single fork(). Can't we
use transactions for this as well? If not, could we enhance them so
that we can use them?
Transactions are security driver specific. But we can imitate them here
too. Instead of direct fork() we would have a list to which we append
all the symlinks we want to create and then fork() once and execute the
list. Good point. I will work on that.
Michal
3:42 a.m.
New subject: [libvirt] [PATCH 10/10] qemuDomainAttachDeviceMknod: Don't loop endlessly
When working with symlinks it is fairly easy to get into a loop.
Don't.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_domain.c | 32 +++++++++++++++++++++++++++-----
1 file changed, 27 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index bcfb2446f..db01bd230 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7690,16 +7690,24 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED,
static int
-qemuDomainAttachDeviceMknod(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainDeviceDefPtr devDef,
- const char *file)
+qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainDeviceDefPtr devDef,
+ const char *file,
+ unsigned int ttl)
{
struct qemuDomainAttachDeviceMknodData data;
int ret = -1;
char *target = NULL;
bool isLink;
+ if (!ttl) {
+ virReportSystemError(ELOOP,
+ _("Too many levels of symbolic links: %s"),
+ file);
+ return ret;
+ }
+
memset(&data, 0, sizeof(data));
data.driver = driver;
@@ -7777,7 +7785,8 @@ qemuDomainAttachDeviceMknod(virQEMUDriverPtr driver,
}
if (isLink &&
- qemuDomainAttachDeviceMknod(driver, vm, devDef, target) < 0)
+ qemuDomainAttachDeviceMknodRecursive(driver, vm, devDef,
+ target, ttl -1) < 0)
goto cleanup;
ret = 0;
@@ -7791,6 +7800,19 @@ qemuDomainAttachDeviceMknod(virQEMUDriverPtr driver,
}
+static int
+qemuDomainAttachDeviceMknod(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainDeviceDefPtr devDef,
+ const char *file)
+{
+ long symloop_max = sysconf(_SC_SYMLOOP_MAX);
+
+ return qemuDomainAttachDeviceMknodRecursive(driver, vm, devDef,
+ file, symloop_max);
+}
+
+
static int
qemuDomainDetachDeviceUnlinkHelper(pid_t pid ATTRIBUTE_UNUSED,
void *opaque)
--
2.11.0
4:58 a.m.
New subject: [libvirt] [PATCH 10/10] qemuDomainAttachDeviceMknod: Don't loop endlessly
On Fri, Jan 20, 2017 at 10:42:50AM +0100, Michal Privoznik wrote:
When working with symlinks it is fairly easy to get into a loop.
Don't.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_domain.c | 32 +++++++++++++++++++++++++++-----
1 file changed, 27 insertions(+), 5 deletions(-)
ACK, same worries as before, but no better soluton exists in my mind.
4:15 a.m.
On 01/20/2017 10:42 AM, Michal Privoznik wrote:
The major problem was with symlinks. Imagine the following chain of
symlinks:
/dev/my_awesome_disk -> /home/user/blaah -> /dev/disk/by-uuid/$uuid -> /dev/sda
We really need to create all those /dev/* symlinks and /dev/sda device. Also,
some other (less critical) bugs are fixed.
Michal Privoznik (10):
virProcessRunInMountNamespace: Report errors from child
util: Introduce virFileReadLink
qemuDomainPrepareDisk: Fix ordering
qemuSecurityRestoreAllLabel: Don't use transactions
qemu_security: Use more transactions
qemuDomain{Attach,Detach}Device NS helpers: Don't relabel devices
qemuDomainCreateDevice: Properly deal with symlinks
qemuDomainCreateDevice: Don't loop endlessly
qemuDomainAttachDeviceMknod: Deal with symlinks
qemuDomainAttachDeviceMknod: Don't loop endlessly
src/libvirt_private.syms | 1 +
src/qemu/qemu_domain.c | 438 +++++++++++++++++++++++++++--------------------
src/qemu/qemu_hotplug.c | 20 +--
src/qemu/qemu_security.c | 137 +++++++++------
src/util/virfile.c | 12 ++
src/util/virfile.h | 2 +
src/util/virprocess.c | 8 +-
7 files changed, 374 insertions(+), 244 deletions(-)
_____ _
| __ (_)
| |__) | _ __ __ _
| ___/ | '_ \ / _` |
| | | | | | | (_| |
|_| |_|_| |_|\__, |
__/ |
|___/
Michal
10:37 a.m.
On Mon, Jan 30, 2017 at 11:15 AM +0100, Michal Privoznik <mprivozn(a)redhat.com>
wrote:
On 01/20/2017 10:42 AM, Michal Privoznik wrote:
> The major problem was with symlinks. Imagine the following chain of symlinks:
>
> /dev/my_awesome_disk -> /home/user/blaah -> /dev/disk/by-uuid/$uuid ->
/dev/sda
>
> We really need to create all those /dev/* symlinks and /dev/sda device. Also,
> some other (less critical) bugs are fixed.
>
> Michal Privoznik (10):
> virProcessRunInMountNamespace: Report errors from child
> util: Introduce virFileReadLink
> qemuDomainPrepareDisk: Fix ordering
> qemuSecurityRestoreAllLabel: Don't use transactions
> qemu_security: Use more transactions
> qemuDomain{Attach,Detach}Device NS helpers: Don't relabel devices
> qemuDomainCreateDevice: Properly deal with symlinks
> qemuDomainCreateDevice: Don't loop endlessly
> qemuDomainAttachDeviceMknod: Deal with symlinks
> qemuDomainAttachDeviceMknod: Don't loop endlessly
>
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_domain.c | 438 +++++++++++++++++++++++++++--------------------
> src/qemu/qemu_hotplug.c | 20 +--
> src/qemu/qemu_security.c | 137 +++++++++------
> src/util/virfile.c | 12 ++
> src/util/virfile.h | 2 +
> src/util/virprocess.c | 8 +-
> 7 files changed, 374 insertions(+), 244 deletions(-)
>
_____ _
| __ (_)
| |__) | _ __ __ _
| ___/ | '_ \ / _` |
| | | | | | | (_| |
|_| |_|_| |_|\__, |
__/ |
|___/
Michal
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Tested the patch series and the problems with v3.0.0
- starting domains with hostdev devices
- hot plugging
seem to be fixed.
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
6:26 a.m.
On 01/20/2017 10:42 AM, Michal Privoznik wrote:
The major problem was with symlinks. Imagine the following chain of
symlinks:
/dev/my_awesome_disk -> /home/user/blaah -> /dev/disk/by-uuid/$uuid -> /dev/sda
We really need to create all those /dev/* symlinks and /dev/sda device. Also,
some other (less critical) bugs are fixed.
Michal Privoznik (10):
virProcessRunInMountNamespace: Report errors from child
util: Introduce virFileReadLink
qemuDomainPrepareDisk: Fix ordering
qemuSecurityRestoreAllLabel: Don't use transactions
qemu_security: Use more transactions
qemuDomain{Attach,Detach}Device NS helpers: Don't relabel devices
qemuDomainCreateDevice: Properly deal with symlinks
qemuDomainCreateDevice: Don't loop endlessly
qemuDomainAttachDeviceMknod: Deal with symlinks
qemuDomainAttachDeviceMknod: Don't loop endlessly
src/libvirt_private.syms | 1 +
src/qemu/qemu_domain.c | 438 +++++++++++++++++++++++++++--------------------
src/qemu/qemu_hotplug.c | 20 +--
src/qemu/qemu_security.c | 137 +++++++++------
src/util/virfile.c | 12 ++
src/util/virfile.h | 2 +
src/util/virprocess.c | 8 +-
7 files changed, 374 insertions(+), 244 deletions(-)
Thanks Martin for the review. I've pushed these.
Michal
2844
days inactive
2862
days old
34 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Marc Hartmayer -
Martin Kletzander -
Michal Privoznik