subject:"\[libvirt\] \[PATCH 02\/10\] util\: Introduce virFileReadLink"

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

2017-02-07 Thread Martin Kletzander

On Thu, Feb 02, 2017 at 08:57:53PM +0100, Martin Kletzander wrote:

On Thu, Feb 02, 2017 at 03:09:15PM +, 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 
> ---
> 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2017-02-02 Thread Martin Kletzander

On Thu, Feb 02, 2017 at 03:09:15PM +, 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 
> ---
> 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/ :|

signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2017-02-02 Thread Daniel P. Berrange

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 
> > ---
> > 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.

> > /*
> >  * 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/ :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2017-02-02 Thread Martin Kletzander


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 
---
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.


/*
 * 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);
+
char *virFindFileInPath(const char *file);

char *virFileFindResource(const char *filename,
--
2.11.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2017-01-20 Thread Michal Privoznik

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 
---
 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;
+}
 
 /*
  * 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);
+
 char *virFindFileInPath(const char *file);
 
 char *virFileFindResource(const char *filename,
-- 
2.11.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

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

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

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

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

5 matches

Site Navigation

Mail list logo

Footer information