Francesco Romani has posted comments on this change.
Change subject: core: containers: add the container support module
Patch Set 25:
PS25, Line 26: class AttrDict(MutableMapping):
> Do we *really* **need** this? Also, it's scope is broader than containers/c
Surely not, it was just nicer to me. It is a relic of the times on which
convirt was an external, isolated project. Let me try to kill it.
PS25, Line 34: subnet='10.1.0.0',
No good reason, I copied from somewhere so I don't have a clear rationale for
PS25, Line 87:
: # for testing purposes
: def clear(self):
: with self._lock:
> I don't think it's my first time saying this, but we should not pollute mod
surely it is not your first time, will clear this up
PS25, Line 99: root.fire(event_id, dom, *args)
> As a linux/unix user, I feel unsafe seeing this. :) Can you spare a few wor
will add a proper focstring.
PS25, Line 31: def rm_file(target):
> Why separate module within containers? There is utils rmFile. If you consid
(applies to the following comment too)
again, relic of times on which convirt was external module. We don't actually
need this module anymore, will remove it.
PS25, Line 24: cgroups is linux-specific
> So is VDSM.
Good point, will remove this comment.
PS25, Line 36: Stats = None
> This could really use class type hint. Not sure if easily possible.
Will look into this.
PS25, Line 126: pass # we don't support some cgroups
it is a pending TODO :)
PS25, Line 152: @property
: def cgroups(self):
: return self._cgroups
: def pid(self):
: return self._pid
> There is no reason for these to be property. We can move them to properties
Right. I just dislike those to be externally modifiable. But let's trust people
PS25, Line 166: def _read_keyvalue(path, sep=' '):
> I have a feeling we do already have such function, and if not, many reimple
Will kill duplication.
Part of reader: doesn't use any of the Reader state, so one could argue it
doesn't belong there :)
PS25, Line 35: class UnconfiguredXML(Exception):
> Uhm, to avoid having to read the docstring to understand the exception, wha
Better, will rename
PS25, Line 47: encoding = 'unicode' if six.PY3 else 'utf-8'
> I'd expect PY4 to keep the unicode.
right, let's reverse this check.
PS25, Line 66: cached
> Is this really a cache?
This - no. So the message is confusing and will be removed.
The point is that this XML is saved once the VM is started and never changed;
the FS acts as a cache here.
PS25, Line 71: cached
PS25, Line 76: cached
To view, visit https://gerrit.ovirt.org/59824
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-Owner: Francesco Romani <from...@redhat.com>
Gerrit-Reviewer: Francesco Romani <from...@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com>
Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org>
vdsm-patches mailing list -- firstname.lastname@example.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org