Padiri Sivanandha Reddy authored 4 years ago and Ritesh Raj Sarraf committed 4 years ago

Rhosydd VSS implementation

VSS data model - review2 changes

Otherwise monitor-vehicle will sit there forever doing nothing.

Reviewed-by: Mathieu Duponchelle <mathieu.duponchelle@collabora.co.uk>
Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>
Differential Revision: https://phabricator.apertis.org/D3723

Validate zone paths and attribute names.

Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>
Differential Revision: https://phabricator.apertis.org/D3510

It does not make sense for a wildcard subscription to restrict its
signal emissions by period — a period criterion can only apply to a
single attribute, otherwise we can end up with pathological cases where
one attribute is emitted often enough to cause rate limiting; and all
other attributes are emitted rarely enough to be accidentally dropped on
one of the attribute emissions which is rate limited. RsdSubscription is
not set up to allow the emission rates for each attribute to be tracked
individually, which is what’s needed to implement period criteria for
wildcard attributes properly.

Instead, acknowledge that if the client has chosen to use a wildcard,
they aren’t particularly fussy about what attributes they receive, and
just give them everything, with no period limiting.

Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>
Differential Revision: https://phabricator.apertis.org/D3509

Each vehicle has its own clock domain, which all timestamps for
attributes from that vehicle are relative to. This means that the
timestamps for two attributes from a given vehicle can be compared to
see which was updated more recently (and how much more recently, in
microseconds).

However, there was previously no way to compare timestamps between
vehicles, or to compare the timestamp for a given attribute to the
current time. Given that the clock domain for a vehicle could differ
arbitrarily from the system clock domain (g_get_monotonic_time()), this
meant that clients could never work out how long ago (in system clock
terms) an attribute was modified.

Fix that by exposing the current time in the vehicle’s clock domain with
the return value from each RsdVehicle getter which returns a property or
property metadata. This allows each attribute in those results to be
compared to the clock at the time of returning the results, to find out
how old that attribute value is, in microseconds.

This patch deliberately does not expose the current value of each
vehicle’s clock domain as a value by itself (for example, as a
get_current_time() method) as that would both tempt clients to poll it,
and would introduce unavoidable race conditions between getting the
current time and getting a set of attributes.

This breaks a lot of API.

The implementation in VddAggregateVehicle is worth paying attention to
when reviewing, as it needs to aggregate attributes from multiple
vehicles into a single aggregate vehicle. That means adjusting their
last-updated times to be in a single aggregate clock domain. This is
done by defining the aggregate clock domain to be g_get_monotonic_time()
and tracking the difference between that and each of the source
vehicles’ clocks with each value returned from those source vehicles.

Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>
Differential Revision: https://phabricator.apertis.org/D3407

The canonical way of handling a UNIX signal is to catch it, reset the
signal handler to the default, do cleanup, and then re-emit the signal
using raise(). At this point, it goes to the default handler, typically
causing the process to terminate.

Previously we were handling signals by catching them, doing cleanup,
then exiting with a non-zero exit status, which wasn’t technically
correct.

This tidies up a bit of the intertwined GCancellable handling in the
CsrService and CsrBackend code, so that functions don’t end up calling
g_cancellable_cancel() on GCancellables they were passed from their
callers.

This adds a  new error code to CsrServiceError, and changes the API of
csr_service_exit().

Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>
Differential Revision: https://phabricator.apertis.org/D3180

This expands and completes the AppArmor profiles for the daemon and mock
backends, and adds a new profile for rhosydd-client.

This relies on the new polkit AppArmor profile (T1685).

Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>
Differential Revision: https://phabricator.apertis.org/D3164

Instead of returning the error and results separately, why not simply
return the GAsyncResult and do the finish() call in the main function?

This introduces no behavioural changes.

Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>
Differential Revision: https://phabricator.apertis.org/D3160

It’s confusing to pass in the object which is in the middle of being
created to the function which will then ‘finish’ creating it and return
it. Just pass the GAsyncResult and GError instead.

Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>
Differential Revision: https://phabricator.apertis.org/D3133

Various commands of rhosydd-client were missing error handling for if
the specified vehicle didn’t exist.

Reviewed-by: Héctor Orón Martínez <hector.oron@collabora.co.uk>
Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>
Differential Revision: https://phabricator.apertis.org/D3099

It’s redundant as the header files are all installed into namespaced
directories; includes now change from:
   #include <librhosydd/rsd-types.h>
to
   #include <librhosydd/types.h>

This makes tab-completion of the header names a little easier.

This diff is entirely the result of search-and-replace.

Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>
Differential Revision: https://phabricator.apertis.org/D3098

This allows moving the validation of zone paths one stage higher in the
input sequence, as we can assume that an instantiated RsdZone has a
valid path, whereas we can’t assume an arbitrary input string is a valid
path.

There are some call sites where we still need to take advantage of this
optimisation; this patch does a simple replacement of zone paths with
zones. It uses RsdStaticZone as a validation wrapper around zone paths,
effectively.

This changes the RsdVehicle API significantly.

Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>
Differential Revision: https://phabricator.apertis.org/D3086

This should clarify the units it’s in throughout the codebase. This does
not change the semantics of any of the places it’s used.

Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>
Differential Revision: https://phabricator.apertis.org/D3062

‘Property’ is a bit of an overloaded term, especially when using GObject
and D-Bus. Rather than naming the things which we get from vehicles
‘properties’, let’s call them ‘attributes’ instead, to clarify things.

This updates all the API and documentation I could find. There may be a
few residual references to ‘property’ which are incorrect, as this was
not entirely automatable.

Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>
Differential Revision: https://phabricator.apertis.org/D3061

Otherwise the cleanup code will be operating on uninitialised memory, if
it gets called before the variables are set for the first time.

Reviewed-by: Simon McVittie <git log --oneline -1>
Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>
Differential Revision: https://phabricator.apertis.org/D3044

Conventionally, this is only used to cause configuration files to be
reloaded (in daemons; but presumably also in utilities). We don’t need
that.

Reviewed-by: Simon McVittie <git log --oneline -1>
Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>
Differential Revision: https://phabricator.apertis.org/D3042

The order in which the variables in this function are freed is not
specified: if the GCancellable is destroyed before the UNIX signal
handlers are removed, it’s possible for a SIGINT signal to cause a
use-after-free in the signal handler when the cancellable is cancelled.

Avoid that by holding a reference to the GCancellable in each of the
signal handlers.

Reviewed-by: Simon McVittie <git log --oneline -1>
Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>
Differential Revision: https://phabricator.apertis.org/D2921

Add an RsdSubscriptionManager to track subscriptions for a particular
client; and to integrate that with the various RsdVehicle
implementations.

https://phabricator.apertis.org/T1435


Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>

Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>

Let’s be consistent throughout the code in using GETTEXT_PACKAGE to
define the name of the gettext package to load.

Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>

LOCALEDIR is what’s suggested in the gettext manual.

Also use $(localedir) itself rather than constructing the path from
$(datadir) and friends.

Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>

Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>

This will run until killed, and print out all signalled property updates
from the given vehicle.

Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>

Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>

Rather than RsdPropertyInfo with an entirely redundant RsdProperty
member.

Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>

Rather than RsdPropertyInfo with an entirely redundant RsdProperty
member.

Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>

Rather than having them in RsdPropertyInfo. This means we can
legitimately return RsdPropertyMetadata from various functions without
having to provide the zone_path and name separately. Changes to do that
will follow shortly.

Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>

Add some new virtual methods to RsdVehicle (and implement them
everywhere) to move all of the core functionality of the Rhosydd client
into librhosydd.

Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>

New commands:
 • show-vehicle
 • show-vehicle-metadata
 • get-property
 • get-property-metadata

Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>

Currently, it can list vehicles, and that’s it.

Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>