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.
> > +
> > + 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?
>
> thanks,
> iustin
Apollon
