Re: [systemd-devel] [PATCH] shared/install: don't report 'static' when unit contains only Also=
Lennart Poettering lenn...@poettering.net writes: On Thu, 06.11.14 10:49, Jan Synacek (jsyna...@redhat.com) wrote: I think that this patch might be a bit ineffective, as it calls unit_file_load() again just to get an InstallContext. I wasn't sure how to get Also= targets in any other way. If such change makes sense, this patch should probably be considered a preview rather than something to be committed right away. Hmm, wouldn't it be nicer to introduce a new UnitFileState enum value for this? Maybe UNIT_FILE_ALSO or so? I am not sure I like the idea of implicitly following the Also= setting here, due to the awkwarndess if multiple units are listed and how to map exotic states of that other unit back to ours... Would that make sense? Lennart Yes, that makes sense. What should a string representation of UNIT_FILE_ALSO be? I don't think that reporting 'also' would feel right. Also, is there a better way to find out if unit has any Also= targets than how I did it? Cheers, -- Jan Synacek Software Engineer, Red Hat signature.asc Description: PGP signature ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] shared/install: don't report 'static' when unit contains only Also=
On Fri, 07.11.14 09:49, Jan Synacek (jsyna...@redhat.com) wrote: Lennart Poettering lenn...@poettering.net writes: On Thu, 06.11.14 10:49, Jan Synacek (jsyna...@redhat.com) wrote: I think that this patch might be a bit ineffective, as it calls unit_file_load() again just to get an InstallContext. I wasn't sure how to get Also= targets in any other way. If such change makes sense, this patch should probably be considered a preview rather than something to be committed right away. Hmm, wouldn't it be nicer to introduce a new UnitFileState enum value for this? Maybe UNIT_FILE_ALSO or so? I am not sure I like the idea of implicitly following the Also= setting here, due to the awkwarndess if multiple units are listed and how to map exotic states of that other unit back to ours... Would that make sense? Lennart Yes, that makes sense. What should a string representation of UNIT_FILE_ALSO be? I don't think that reporting 'also' would feel right. We should always keep the enum name and the string it translates to in sync. I can see that reporting also might be confusing, note sure what we could name it better though. But if we use a different string we should also rename its enum really. Maybe indirect? other? Hmm, see-also could work? With the counterpart UNIT_FILE_SEE_ALSO? Also, is there a better way to find out if unit has any Also= targets than how I did it? I think the best way would be to extend InstallInfo to get a new also strv field, that is upated by config_parse_also(). Then unit_file_can_install() can find it and return the fact that the list is not empty in an extra parameter. Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] shared/install: don't report 'static' when unit contains only Also=
On Fri, Nov 07, 2014 at 01:06:41PM +0100, Lennart Poettering wrote: On Fri, 07.11.14 09:49, Jan Synacek (jsyna...@redhat.com) wrote: Lennart Poettering lenn...@poettering.net writes: On Thu, 06.11.14 10:49, Jan Synacek (jsyna...@redhat.com) wrote: I think that this patch might be a bit ineffective, as it calls unit_file_load() again just to get an InstallContext. I wasn't sure how to get Also= targets in any other way. If such change makes sense, this patch should probably be considered a preview rather than something to be committed right away. Hmm, wouldn't it be nicer to introduce a new UnitFileState enum value for this? Maybe UNIT_FILE_ALSO or so? I am not sure I like the idea of implicitly following the Also= setting here, due to the awkwarndess if multiple units are listed and how to map exotic states of that other unit back to ours... Would that make sense? Lennart Yes, that makes sense. What should a string representation of UNIT_FILE_ALSO be? I don't think that reporting 'also' would feel right. Maybe I'm missing something, but wouldn't be enough to report is as 'enabled'? For some units adding another name from Also= really enables the unit, and for other units the name from Also= is mostly cosmetic. What I'm trying to say is that having or not the Also= name enabled is only approximate information and does not always tell us if the unit will be started. I'd prefer to redefine enabled/disabled/static as this unit has at least on of the declared links in the filesystem/the unit does not have any defined links in the filesystem/this unit does not declare any links in the filesystem, which is something that we can actually check. Zbyszek ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] shared/install: don't report 'static' when unit contains only Also=
Zbigniew Jędrzejewski-Szmek zbys...@in.waw.pl writes: On Fri, Nov 07, 2014 at 01:06:41PM +0100, Lennart Poettering wrote: On Fri, 07.11.14 09:49, Jan Synacek (jsyna...@redhat.com) wrote: Lennart Poettering lenn...@poettering.net writes: On Thu, 06.11.14 10:49, Jan Synacek (jsyna...@redhat.com) wrote: I think that this patch might be a bit ineffective, as it calls unit_file_load() again just to get an InstallContext. I wasn't sure how to get Also= targets in any other way. If such change makes sense, this patch should probably be considered a preview rather than something to be committed right away. Hmm, wouldn't it be nicer to introduce a new UnitFileState enum value for this? Maybe UNIT_FILE_ALSO or so? I am not sure I like the idea of implicitly following the Also= setting here, due to the awkwarndess if multiple units are listed and how to map exotic states of that other unit back to ours... Would that make sense? Lennart Yes, that makes sense. What should a string representation of UNIT_FILE_ALSO be? I don't think that reporting 'also' would feel right. Maybe I'm missing something, but wouldn't be enough to report is as 'enabled'? AFAIK, it can also be disabled... Take systemd-journal-gatewayd.service as an example. It doesn't have anything but Also=systemd-journal-gatewayd.socket in the Install section. If you disable the socket, you would then return enabled, which would be wrong. Howerever, I'm not sure about more complicated setups. For some units adding another name from Also= really enables the unit, and for other units the name from Also= is mostly cosmetic. What I'm trying to say is that having or not the Also= name enabled is only approximate information and does not always tell us if the unit will be started. I'd prefer to redefine enabled/disabled/static as this unit has at least on of the declared links in the filesystem/the unit does not have any defined links in the filesystem/this unit does not declare any links in the filesystem, which is something that we can actually check. Zbyszek Cheers, -- Jan Synacek Software Engineer, Red Hat signature.asc Description: PGP signature ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] shared/install: don't report 'static' when unit contains only Also=
Lennart Poettering lenn...@poettering.net writes: On Fri, 07.11.14 09:49, Jan Synacek (jsyna...@redhat.com) wrote: Lennart Poettering lenn...@poettering.net writes: On Thu, 06.11.14 10:49, Jan Synacek (jsyna...@redhat.com) wrote: I think that this patch might be a bit ineffective, as it calls unit_file_load() again just to get an InstallContext. I wasn't sure how to get Also= targets in any other way. If such change makes sense, this patch should probably be considered a preview rather than something to be committed right away. Hmm, wouldn't it be nicer to introduce a new UnitFileState enum value for this? Maybe UNIT_FILE_ALSO or so? I am not sure I like the idea of implicitly following the Also= setting here, due to the awkwarndess if multiple units are listed and how to map exotic states of that other unit back to ours... Would that make sense? Lennart Yes, that makes sense. What should a string representation of UNIT_FILE_ALSO be? I don't think that reporting 'also' would feel right. We should always keep the enum name and the string it translates to in sync. I can see that reporting also might be confusing, note sure what we could name it better though. But if we use a different string we should also rename its enum really. Maybe indirect? other? Hmm, see-also could work? With the counterpart UNIT_FILE_SEE_ALSO? Also, is there a better way to find out if unit has any Also= targets than how I did it? I think the best way would be to extend InstallInfo to get a new also strv field, that is upated by config_parse_also(). Then unit_file_can_install() can find it and return the fact that the list is not empty in an extra parameter. Thanks for the pointers! I've resubmitted the patch and went with the indirect version, as it felt the best to me. -- Jan Synacek Software Engineer, Red Hat signature.asc Description: PGP signature ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] shared/install: don't report 'static' when unit contains only Also=
On Fri, 07.11.14 14:24, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote: On Fri, Nov 07, 2014 at 01:06:41PM +0100, Lennart Poettering wrote: On Fri, 07.11.14 09:49, Jan Synacek (jsyna...@redhat.com) wrote: Lennart Poettering lenn...@poettering.net writes: On Thu, 06.11.14 10:49, Jan Synacek (jsyna...@redhat.com) wrote: I think that this patch might be a bit ineffective, as it calls unit_file_load() again just to get an InstallContext. I wasn't sure how to get Also= targets in any other way. If such change makes sense, this patch should probably be considered a preview rather than something to be committed right away. Hmm, wouldn't it be nicer to introduce a new UnitFileState enum value for this? Maybe UNIT_FILE_ALSO or so? I am not sure I like the idea of implicitly following the Also= setting here, due to the awkwarndess if multiple units are listed and how to map exotic states of that other unit back to ours... Would that make sense? Lennart Yes, that makes sense. What should a string representation of UNIT_FILE_ALSO be? I don't think that reporting 'also' would feel right. Maybe I'm missing something, but wouldn't be enough to report is as 'enabled'? For some units adding another name from Also= really enables the unit, and for other units the name from Also= is mostly cosmetic. What I'm trying to say is that having or not the Also= name enabled is only approximate information and does not always tell us if the unit will be started. Also= isn't necessarily symmetric. If you have a service A and a service B, then it might very well be that B has Also=A.service, but not the other way round. I think there are only two strategies here: a) if a unit has Also= set, determine the state of the unit files listed in it, any propagate that. This is what Jan's patch did originally. But I am not sure I like this propagation since it leaves so many open questions regarding correct propagation the precise state and what to do if multiple units are listed. or b) just report that Also= is set, which is what I was proposing. This might not be particularly self-explanatory, but maybe we can explain this in the man page or so... I'd prefer to redefine enabled/disabled/static as this unit has at least on of the declared links in the filesystem/the unit does not have any defined links in the filesystem/this unit does not declare any links in the filesystem, which is something that we can actually check. So, are you proposing to follow strategy a) then? Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] shared/install: don't report 'static' when unit contains only Also=
On Fri, Nov 07, 2014 at 03:25:11PM +0100, Jan Synacek wrote: Zbigniew Jędrzejewski-Szmek zbys...@in.waw.pl writes: On Fri, Nov 07, 2014 at 01:06:41PM +0100, Lennart Poettering wrote: On Fri, 07.11.14 09:49, Jan Synacek (jsyna...@redhat.com) wrote: Lennart Poettering lenn...@poettering.net writes: On Thu, 06.11.14 10:49, Jan Synacek (jsyna...@redhat.com) wrote: I think that this patch might be a bit ineffective, as it calls unit_file_load() again just to get an InstallContext. I wasn't sure how to get Also= targets in any other way. If such change makes sense, this patch should probably be considered a preview rather than something to be committed right away. Hmm, wouldn't it be nicer to introduce a new UnitFileState enum value for this? Maybe UNIT_FILE_ALSO or so? I am not sure I like the idea of implicitly following the Also= setting here, due to the awkwarndess if multiple units are listed and how to map exotic states of that other unit back to ours... Would that make sense? Lennart Yes, that makes sense. What should a string representation of UNIT_FILE_ALSO be? I don't think that reporting 'also' would feel right. Maybe I'm missing something, but wouldn't be enough to report is as 'enabled'? AFAIK, it can also be disabled... Take systemd-journal-gatewayd.service as an example. It doesn't have anything but Also=systemd-journal-gatewayd.socket in the Install section. If you disable the socket, you would then return enabled, which would be wrong. Shit, sorry, I was thinking about about Alias=. Please disregard my previous mail, I need to reread the thread while not trying to do something else at the same time. Zbyszek ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] shared/install: don't report 'static' when unit contains only Also=
I think that this patch might be a bit ineffective, as it calls unit_file_load() again just to get an InstallContext. I wasn't sure how to get Also= targets in any other way. If such change makes sense, this patch should probably be considered a preview rather than something to be committed right away. Jan Synacek (1): shared/install: don't report 'static' when unit contains only Also= src/shared/install.c | 23 ++- 1 file changed, 22 insertions(+), 1 deletion(-) -- 1.9.3 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] shared/install: don't report 'static' when unit contains only Also=
If a unit contains only Also=, with no Alias= or WantedBy=, it shouldn't
be reported as static. If any target unit specified in Also= is enabled
or disabled, report this unit as enabled or disabled as well.
https://bugzilla.redhat.com/show_bug.cgi?id=864298
---
src/shared/install.c | 23 ++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/src/shared/install.c b/src/shared/install.c
index cab93e8..781832f 100644
--- a/src/shared/install.c
+++ b/src/shared/install.c
@@ -1874,8 +1874,29 @@ UnitFileState unit_file_get_state(
return r;
else if (r 0)
return UNIT_FILE_DISABLED;
-else if (r == 0)
+else if (r == 0) {
+_cleanup_(install_context_done) InstallContext c = {};
+InstallInfo info = {}, *tmp;
+Iterator it;
+const char *key;
+
+(void) unit_file_load(c, info, path, root_dir, true,
true);
+
+/* At this point, the unit contains only Also=, no
Alias= or WantedBy= are specified.
+ It can be enabled/disabled through any of the Also=
targets, we should check
+ if they're enabled/disabled. */
+if (c.will_install) {
+ORDERED_HASHMAP_FOREACH_KEY(tmp, key,
c.will_install, it) {
+r = find_symlinks_in_scope(scope,
root_dir, key, state);
+if (r 0)
+return r;
+else if (r 0)
+return state;
+}
+return UNIT_FILE_DISABLED;
+}
return UNIT_FILE_STATIC;
+}
}
return r 0 ? r : state;
--
1.9.3
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] shared/install: don't report 'static' when unit contains only Also=
On Thu, 06.11.14 10:49, Jan Synacek (jsyna...@redhat.com) wrote: I think that this patch might be a bit ineffective, as it calls unit_file_load() again just to get an InstallContext. I wasn't sure how to get Also= targets in any other way. If such change makes sense, this patch should probably be considered a preview rather than something to be committed right away. Hmm, wouldn't it be nicer to introduce a new UnitFileState enum value for this? Maybe UNIT_FILE_ALSO or so? I am not sure I like the idea of implicitly following the Also= setting here, due to the awkwarndess if multiple units are listed and how to map exotic states of that other unit back to ours... Would that make sense? Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
