Re: [libvirt] [PATCH 02/10] util: Introduce virFileReadLink
On Thu, Feb 02, 2017 at 03:09:15PM +0000, Daniel P. Berrange wrote:
On Thu, Feb 02, 2017 at 03:11:56PM +0100, Martin Kletzander wrote:
> On Fri, Jan 20, 2017 at 10:42:42AM +0100, Michal Privoznik wrote:
> > We will need to traverse the symlinks one step at the time.
> > Therefore we need to see where a symlink is pointing to.
> >
> > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> > ---
> > src/libvirt_private.syms | 1 +
> > src/util/virfile.c | 12 ++++++++++++
> > src/util/virfile.h | 2 ++
> > 3 files changed, 15 insertions(+)
> >
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index cfeb43cf0..7f7dcfe44 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -1615,6 +1615,7 @@ virFileReadAllQuiet;
> > virFileReadBufQuiet;
> > virFileReadHeaderFD;
> > virFileReadLimFD;
> > +virFileReadLink;
> > virFileRelLinkPointsTo;
> > virFileRemove;
> > virFileRemoveLastComponent;
> > diff --git a/src/util/virfile.c b/src/util/virfile.c
> > index bf8099e34..49ea1d1f0 100644
> > --- a/src/util/virfile.c
> > +++ b/src/util/virfile.c
> > @@ -76,6 +76,7 @@
> > #include "virutil.h"
> >
> > #include "c-ctype.h"
> > +#include "areadlink.h"
> >
> > #define VIR_FROM_THIS VIR_FROM_NONE
> >
> > @@ -1614,6 +1615,17 @@ virFileIsLink(const char *linkpath)
> > return S_ISLNK(st.st_mode) != 0;
> > }
> >
> > +/*
> > + * Read where symlink is pointing to.
> > + *
> > + * Returns 0 on success (@linkpath is a successfully read link),
> > + * -1 with errno set upon error.
> > + */
> > +int
> > +virFileReadLink(const char *linkpath, char **resultpath)
> > +{
> > + return (*resultpath = areadlink(linkpath)) ? 0 : -1;
> > +}
> >
>
> I don't really see the benefit of this function, are you trying to
> obfuscate it? You essentially double the return information for no
> reason and try to push it all into one line.
It would be useful if you have an ATTRIBUTE_RETURN_CHECK annotation
on it, as it would allow compiler time verification of error checking,
which is not possible when you overload the return value to contain
both the data and error indicator.
More like if there was another logic, e.g. checking that it is a link
and if it is not, returning the same path or similar.
ATTRIBUTE_RETURN_CHECK doesn't make much sense to me here since you can
get the same information from the second parameter and you are basically
making the caller use two values that have the same information.
Moreover you can set ATTRIBUTE_RETURN_CHECK for the function and just
directly return the output of areadlink(), essentially just renaming
it. I don't see the point in that.
It would be different if the function guaranteed non-modification of
that parameter when it errors out, for example. Or set an call
virReportSystemError(errno, ...); so that the error reporting is done
consistently and in one place. I haven't checked yet how Michal uses
it, though, so I can't say what's the best solution for now.
But I now see where it comes from. It's the virFileResolveAllLinks()
that does the same thing. That doesn't mean it's right, though.
My question is this. If returning integer, which does not contain any
other value then just the error, is desired, should we mandate that for
non-simple functions and rewrite, for example, qemuBuildCommandLine()?
> > /*
> > * Finds a requested executable file in the PATH env. e.g.:
> > diff --git a/src/util/virfile.h b/src/util/virfile.h
> > index 0343acd5b..981a9e07d 100644
> > --- a/src/util/virfile.h
> > +++ b/src/util/virfile.h
> > @@ -166,6 +166,8 @@ int virFileResolveAllLinks(const char *linkpath,
> > int virFileIsLink(const char *linkpath)
> > ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
> >
> > +int virFileReadLink(const char *linkpath, char **resultpath);
eg add ATTRIBUTE_RETURN_CHECK here
> > +
> > char *virFindFileInPath(const char *file);
> >
> > char *virFileFindResource(const char *filename,
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
Attachments:
- signature.asc (application/pgp-signature — 833 bytes)