[libvirt] [PATCH 02/10] util: Introduce virFileReadLink

[Martin Kletzander]

On Thu, Feb 02, 2017 at 08:57:53PM +0100, Martin Kletzander wrote:
>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 at 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
>>

So it seems like Michal is not participating in the discussion about his
own patch.  But I talked to him personally and he admitted that the
function prototype was meant to look like it does simply to be just like
the other functions, e.g. virFileResolveAllLinks().

So for now I'm OK with it, we can change how all the functions look
later on, but that's a discussion to be had.  So ACK with the
ATTRIBUTE_RETURN_CHECK added 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/ :|



>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170207/18252b9d/attachment-0001.sig>