Re: [libvirt] [PATCH v2] lxc: Add virCgroupSetOwner()
On Fri, Feb 14, 2014 at 02:25:55PM +0100, Richard Weinberger wrote:
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index a6d60c5..4bef0db 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -3253,6 +3253,66 @@ cleanup:
}
+int virCgroupSetOwner(virCgroupPtr cgroup,
+ uid_t uid,
+ gid_t gid,
+ int controllers)
+{
+size_t i;
+
+for (i = 0; i VIR_CGROUP_CONTROLLER_LAST; i++) {
+char *base, *entry;
+DIR *dh;
+struct dirent *de;
+
+if (!((1 i) controllers))
+continue;
+
+if (!cgroup-controllers[i].mountPoint)
+continue;
+
+if (virAsprintf(base, %s%s, cgroup-controllers[i].mountPoint,
+cgroup-controllers[i].placement) 0) {
+return -1;
+}
Indentation of 'return' is too deep
+
+dh = opendir(base);
+if (!dh) {
+VIR_ERROR(_(Unable to open %s: %s), base, strerror(errno));
+VIR_FREE(base);
+return -1;
+}
This should use virReportSystemError.
+
+while ((de = readdir(dh)) != NULL) {
+if (STREQ(de-d_name, .) ||
+STREQ(de-d_name, ..))
+continue;
+
+if (virAsprintf(entry, %s/%s, base, de-d_name) 0) {
+VIR_FREE(base);
+closedir(dh);
+return -1;
+}
+
+if (chown(entry, uid, gid) 0)
+VIR_WARN(_(cannot chown '%s' to (%u, %u): %s), entry, uid,
gid,
+strerror(errno));
This should use virReportSystemError too, and propagate the error.
+
+VIR_FREE(entry);
+}
+closedir(dh);
+
+if (chown(base, uid, gid) 0)
+VIR_WARN(_(cannot chown '%s' to (%u, %u): %s), entry, uid, gid,
+strerror(errno));
Likewise. Also it uses NULL 'entry' var rather than 'base' in the
message args.
+
+VIR_FREE(base);
+}
+
+return 0;
+}
I'm going to send a followup with these fixes for review, which also
centralizes the error cleanup code.
Regards,
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] lxc: Add virCgroupSetOwner()
Am 24.02.2014 13:20, schrieb Daniel P. Berrange:
On Fri, Feb 14, 2014 at 02:25:55PM +0100, Richard Weinberger wrote:
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index a6d60c5..4bef0db 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -3253,6 +3253,66 @@ cleanup:
}
+int virCgroupSetOwner(virCgroupPtr cgroup,
+ uid_t uid,
+ gid_t gid,
+ int controllers)
+{
+size_t i;
+
+for (i = 0; i VIR_CGROUP_CONTROLLER_LAST; i++) {
+char *base, *entry;
+DIR *dh;
+struct dirent *de;
+
+if (!((1 i) controllers))
+continue;
+
+if (!cgroup-controllers[i].mountPoint)
+continue;
+
+if (virAsprintf(base, %s%s, cgroup-controllers[i].mountPoint,
+cgroup-controllers[i].placement) 0) {
+return -1;
+}
Indentation of 'return' is too deep
Do you have something like a checkpatch.pl? ;=)
+
+dh = opendir(base);
+if (!dh) {
+VIR_ERROR(_(Unable to open %s: %s), base, strerror(errno));
+VIR_FREE(base);
+return -1;
+}
This should use virReportSystemError.
To avoid further confusion. When to use VIR_ERROR() and when
virReportSystemError()?
+
+while ((de = readdir(dh)) != NULL) {
+if (STREQ(de-d_name, .) ||
+STREQ(de-d_name, ..))
+continue;
+
+if (virAsprintf(entry, %s/%s, base, de-d_name) 0) {
+VIR_FREE(base);
+closedir(dh);
+return -1;
+}
+
+if (chown(entry, uid, gid) 0)
+VIR_WARN(_(cannot chown '%s' to (%u, %u): %s), entry,
uid, gid,
+strerror(errno));
This should use virReportSystemError too, and propagate the error.
Do you we really want to propagate this error?
IMHO a failing chown() is not a fatal error.
+
+VIR_FREE(entry);
+}
+closedir(dh);
+
+if (chown(base, uid, gid) 0)
+VIR_WARN(_(cannot chown '%s' to (%u, %u): %s), entry, uid,
gid,
+strerror(errno));
Likewise. Also it uses NULL 'entry' var rather than 'base' in the
message args.
+
+VIR_FREE(base);
+}
+
+return 0;
+}
I'm going to send a followup with these fixes for review, which also
centralizes the error cleanup code.
Okay!
Thanks,
//richard
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] lxc: Add virCgroupSetOwner()
On Mon, Feb 24, 2014 at 01:25:04PM +0100, Richard Weinberger wrote:
Am 24.02.2014 13:20, schrieb Daniel P. Berrange:
On Fri, Feb 14, 2014 at 02:25:55PM +0100, Richard Weinberger wrote:
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index a6d60c5..4bef0db 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -3253,6 +3253,66 @@ cleanup:
}
+int virCgroupSetOwner(virCgroupPtr cgroup,
+ uid_t uid,
+ gid_t gid,
+ int controllers)
+{
+size_t i;
+
+for (i = 0; i VIR_CGROUP_CONTROLLER_LAST; i++) {
+char *base, *entry;
+DIR *dh;
+struct dirent *de;
+
+if (!((1 i) controllers))
+continue;
+
+if (!cgroup-controllers[i].mountPoint)
+continue;
+
+if (virAsprintf(base, %s%s, cgroup-controllers[i].mountPoint,
+cgroup-controllers[i].placement) 0) {
+return -1;
+}
Indentation of 'return' is too deep
Do you have something like a checkpatch.pl? ;=)
Just my eyes in this case ;-P
+dh = opendir(base);
+if (!dh) {
+VIR_ERROR(_(Unable to open %s: %s), base, strerror(errno));
+VIR_FREE(base);
+return -1;
+}
This should use virReportSystemError.
To avoid further confusion. When to use VIR_ERROR() and when
virReportSystemError()?
VIR_ERROR merely puts a message in the logs. It doesn't propagate
anything back to the client making the API call. The virReport*Error
functions actually send an error back to the client app. You basically
always want virReport*Error - there's almost no cases where VIR_ERROR
is the right thing todo.
+
+while ((de = readdir(dh)) != NULL) {
+if (STREQ(de-d_name, .) ||
+STREQ(de-d_name, ..))
+continue;
+
+if (virAsprintf(entry, %s/%s, base, de-d_name) 0) {
+VIR_FREE(base);
+closedir(dh);
+return -1;
+}
+
+if (chown(entry, uid, gid) 0)
+VIR_WARN(_(cannot chown '%s' to (%u, %u): %s), entry,
uid, gid,
+strerror(errno));
This should use virReportSystemError too, and propagate the error.
Do you we really want to propagate this error?
IMHO a failing chown() is not a fatal error.
I don't see a valid reason why chown would fail in normal usage,
so I think it should be fatal.
Regards,
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
