On Wed, Jul 07, 2010 at 05:18:02PM +0300, Apollon Oikonomopoulos wrote:
> On 15:17 Wed 07 Jul , Iustin Pop wrote:
> > On Wed, Jul 07, 2010 at 03:06:18PM +0300, Apollon Oikonomopoulos wrote:
> > > On 10:15 Wed 07 Jul , Iustin Pop wrote:
> > > See the revised patch for that. Just two questions:
> > >
> > > 1. The behaviour I chose is to filter out all paths that are not under
> > > /dev and return data about the valid paths, only warning that some
> > > devices where not under constants.BLOCKDEV_ROOT. Is this an
> > > acceptable behaviour (silently dropping invalid paths), or should an
> > > exception be raised?
> >
> > I think it's better to raise an exception, as otherwise the rpc client
> > has to check manually and warn (to the job log) that some paths were
> > invalid, etc.
>
> I was thinking of the same. Will do, thanks.
>
> >
> > > 2. I chose to default to an empty list if devices is not given (see
> > > call_bdev_list at lib/rpc.py) at the RPC level. Is this OK? Should I
> > > also default on that on the backend level (GetBlockdevList at
> > > lib/blockdev.py)?
> >
> > No, this is not good. Check the other RPC calls, there are no default
> > parameters (we can get into the discussion why, but for the patch, just
> > require it). The same should stand, I think, for backend (so it's find
> > as it is now).
> >
>
> > > + # Normalize the rootpath and append a slash, so that we can use a
> > > simple
> > > + # substring match against device paths. os.path.commonprefix does not
> > > seem to
> > > + # fit, as os.path.commonprefix(['/some/path/a', '/some/patha']) =
> > > '/some/path'
> > > + blockdev_root = os.path.normpath(constants.BLOCKDEV_ROOT) + '/'
> >
> > Wouldn't it be simpler to list the BLOCKDEV_ROOT with a slash at the end
> > in constants? We do this for other directories too.
>
> We could, but also add a suitable comment that it should have a trailing
> slash. Apart from constants.LOG_DIR, all other directories have no
> trailing slash as far as I see.
Ah so. I might have confused things. LGTM for the comment from me.
> > > +
> > > + if devices:
> > > + devpaths = filter(lambda x:
> > > os.path.abspath(x).startswith(blockdev_root),
> > > + devices)
> >
> > Please move this to a list comprehension:
> >
> > [v for v in devices if os.path.abspath(v).startswith…]
>
> I was in doubt about filter vs comprehension as well, thanks.
>
> >
> > > + if len(devpaths) != len(devices):
> > > + logging.warn("Some paths do not lie under %s and have been
> > > skipped." %
> > > + constants.BLOCKDEV_ROOT)
> >
> > Once you change this into an exception, and remove the default devices=[]
> > from rpc.py, this can go in.
>
> Should I do the same if the specified paths are not block devices? What
> about the warning when blockdev --getsize64 fails?
I forget in which cases you call this exactly. If it's just at instance
creation, then an exception is better, indeed.
Basically the question is if this is called automatically (e.g. by the
watcher or such), or only explicitly when an admin does an instace
creation, or a cluster verify. If it's explicit, then an exception is a
good choice.
iustin
