[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libvir] PATCH: 10/16: general purpose helper APIs



On Mon, Feb 18, 2008 at 10:36:06AM -0500, Daniel Veillard wrote:
> On Tue, Feb 12, 2008 at 04:37:08AM +0000, Daniel P. Berrange wrote:
> > 
> > The SELinux code is optional, and can be replaced with calls to any other
> > library which can provide MAC file labels, or just disabled completely..
> [...]
> > +SELINUX_CFLAGS=
> > +SELINUX_LIBS=
> > +if test "$with_selinux" != "no"; then
> > +  old_cflags="$CFLAGS"
> > +  old_libs="$LIBS"
> > +  if test "$with_selinux" = "check"; then
> > +    AC_CHECK_HEADER([selinux/selinux.h],[],[with_selinux=no])
> > +    AC_CHECK_LIB(selinux, fgetfilecon,[],[with_selinux=no])
> > +    if test "$with_selinux" != "no"; then
> > +      with_selinux="yes"
> > +    fi
> > +  else
> > +    AC_CHECK_HEADER([selinux/selinux.h],[],
> > +       [AC_MSG_ERROR([You must install the SELinux development package in order to compile libvirt])])
> > +    AC_CHECK_LIB(selinux, fgetfilecon,[],
> > +       [AC_MSG_ERROR([You must install the SELinux development package in order to compile and run libvirt])])
> 
>    Hum, does that mean that withough any configure flags and if SELinux is
> not found then configure would fail ? It may be the right thing to do since
> it's a security option (and hence deactivation as an opt-in looks reasonable)
> but this may be viewed as a bit too Fedora centric :-)

The  --with-selinux argument to configure has 3 possible values 'yes', 'no', 'check'.
It will default to the latter. If it finds selinux libs it'll turn it on, otherwise
it'll turn it off. It'll only cause configure to fail if you have it set to 'yes'
and selinux is missing, which is not default behavuour.

> [...]
> > +    if (S_ISREG(sb.st_mode)) {
> > +        vol->allocation = (unsigned long long)sb.st_blocks * (unsigned long long)512;
> 
>   Hum, I think this should be 
> 
>      sb.st_blocks * sb.st_blksize
> 
> since you can have 1k or 4k kind of filesystems too or at least to ensure
> portability.

Good point.

> > +    } else {
> > +        off_t end;
> > +        /* XXX this is POSIX compliant, but doesn't work for for CHAR files,
> > +         * only BLOCK. There is a Linux specific ioctl() for getting
> > +         * size of both CHAR / BLOCK devices we should check for in
> > +         * configure
> > +         */
> > +        end = lseek(fd, 0, SEEK_END);
> > +        if (end == (off_t)-1) {
> > +            virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
> > +                                  "cannot seek to end of file '%s': %d (%s)",
> > +                                  vol->target.path, errno, strerror(errno));
> > +            return -1;
> > +        }
> > +        vol->allocation = end;
> > +        if (withCapacity) vol->capacity = end;
> > +    }
> 
>   It would be good to see what this gives on Solaris, Windows ...

I'll have to double check, but I think with these patches that comment is now
obsolete. IIRC we should only ever execute this code path when we have a plain
file, so the fact that it doesn't work with block/char devices should no longer
matter.

> > +/*
> > + * Run an external program.
> > + *
> > + * Read its output and apply a series of regexes to each line
> > + * When the entire set of regexes has matched consequetively
> > + * then run a callback passing in all the matches
> > + */
> > +int virStorageBackendRunProgRegex(virConnectPtr conn,
> > +                                  virStoragePoolObjPtr pool,
> > +                                  const char **prog,
> > +                                  int nregex,
> > +                                  const char **regex,
> > +                                  int *nvars,
> > +                                  virStorageBackendListVolRegexFunc func,
> > +                                  void *data)
> 
>   Ouch, that's very very specialized ... but if needed okay

Actually it is very generalized ;-)  It lets you spawn any program and
trivially tokenize its output based on regexes - we use this in basically
all the storage drivers for talking to parted, lvs, pvs, iscsiadm, etc


> > +
> > +/*
> > + * Run an external program and read from its standard output
> > + * a stream of tokens from IN_STREAM, applying FUNC to
> > + * each successive sequence of N_COLUMNS tokens.
> > + * If FUNC returns < 0, stop processing input and return -1.
> > + * Return -1 if N_COLUMNS == 0.
> > + * Return -1 upon memory allocation error.
> > + * If the number of input tokens is not a multiple of N_COLUMNS,
> > + * then the final FUNC call will specify a number smaller than N_COLUMNS.
> > + * If there are no input tokens (empty input), call FUNC with N_COLUMNS == 0.
> > + */
> > +int virStorageBackendRunProgNul(virConnectPtr conn,
> > +                                virStoragePoolObjPtr pool,
> > +                                const char **prog,
> > +                                size_t n_columns,
> > +                                virStorageBackendListVolNulFunc func,
> > +                                void *data)
> 
>   yeah we are reaching an impressive level of specialization there...

Again, fairly general purpose, except instead of regexes, it uses NULL
to separate fields. We only use this in one place currently though.

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]