On Fri, Oct 19, 2018 at 11:41:45AM +0200, Philippe Mathieu-Daudé wrote:
> On 09/10/2018 15:04, Daniel P. Berrangé wrote:
> > Add a QAuthZListFile object type that implements the QAuthZ interface. This
> > built-in implementation is a proxy around the QAtuhZList object type,
> > initializing it from an external file, and optionally, automatically
> > reloading it whenever it changes.
> >
> > To create an instance of this object via the QMP monitor, the syntax
> > used would be:
> >
> > {
> > "execute": "object-add",
> > "arguments": {
> > "qom-type": "authz-list-file",
> > "id": "authz0",
> > "parameters": {
> > "filename": "/etc/qemu/vnc.acl",
> > "refresh": "yes"
> > }
> > }
> > }
> >
> > If "refresh" is "yes", inotify is used to monitor the file,
> > automatically reloading changes. If an error occurs during reloading,
> > all authorizations will fail until the file is next successfully
> > loaded.
> >
> > The /etc/qemu/vnc.acl file would contain a JSON representation of a
> > QAuthZList object
> >
> > {
> > "rules": [
> > { "match": "fred", "policy": "allow", "format": "exact" },
> > { "match": "bob", "policy": "allow", "format": "exact" },
> > { "match": "danb", "policy": "deny", "format": "glob" },
> > { "match": "dan*", "policy": "allow", "format": "exact" },
> > ],
> > "policy": "deny"
> > }
> >
> > This sets up an authorization rule that allows 'fred', 'bob' and anyone
> > whose name starts with 'dan', except for 'danb'. Everyone unmatched is
> > denied.
> >
> > The object can be loaded on the comand line using
> >
> > -object authz-list-file,id=authz0,filename=/etc/qemu/vnc.acl,refresh=yes
> >
> > Signed-off-by: Daniel P. Berrangé <berra...@redhat.com>
> > ---
> > authz/Makefile.objs | 1 +
> > authz/listfile.c | 284 +++++++++++++++++++++++++++++++++++++++
> > authz/trace-events | 4 +
> > include/authz/listfile.h | 110 +++++++++++++++
> > qemu-options.hx | 47 +++++++
> > 5 files changed, 446 insertions(+)
> > create mode 100644 authz/listfile.c
> > create mode 100644 include/authz/listfile.h
> >
> > +static void
> > +qauthz_list_file_event(int wd G_GNUC_UNUSED,
> > + QFileMonitorEvent ev G_GNUC_UNUSED,
> > + const char *name G_GNUC_UNUSED,
> > + void *opaque)
> > +{
> > + QAuthZListFile *fauthz = opaque;
> > + Error *err = NULL;
> > +
> > + if (ev != QFILE_MONITOR_EVENT_MODIFIED &&
> > + ev != QFILE_MONITOR_EVENT_CREATED)
>
> You missed:
>
> {
>
> > + return;
>
> }
Opps, yes.
> > +static void
> > +qauthz_list_file_complete(UserCreatable *uc, Error **errp)
> > +{
> > + QAuthZListFile *fauthz = QAUTHZ_LIST_FILE(uc);
> > +
> > + fauthz->list = qauthz_list_file_load(fauthz, errp);
> > +
> > + if (fauthz->refresh) {
>
> Can we invert this condition?
Yes, will do
>
> > + gchar *dir, *file;
> > + fauthz->file_monitor = qemu_file_monitor_get_instance(errp);
> > + if (!fauthz->file_monitor) {
> > + return;
> > + }
> > +
> > + dir = g_path_get_dirname(fauthz->filename);
> > + if (g_str_equal(dir, ".")) {
> > + error_setg(errp, "Filename must be an absolute path");
>
> What about:
>
> goto cleanup;
Yep.
>
> > + g_free(dir);
> > + return;
> > + }
> > + file = g_path_get_basename(fauthz->filename);
> > + if (g_str_equal(file, ".")) {
> > + error_setg(errp, "Path has no trailing filename component");
>
> goto cleanup;
>
> > + g_free(file);
> > + g_free(dir);
> > + return;
> > + }
> > +
> > + fauthz->file_watch = qemu_file_monitor_add_watch(
> > + fauthz->file_monitor, dir, file,
> > + qauthz_list_file_event, fauthz, errp);
> > + g_free(file);
> > + g_free(dir);
> > + if (fauthz->file_watch < 0) {
>
> Is this really useful? Do you plan to add more code here?
I just want to make it clear to anyone who changes the code
in future that there's an expected failure condition here.
> > + return;
> > + }
> > + }
> > +}
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
